[PATCH] gpu: anx7808: fix a missing check of return value

[Kangjie Lu]

Thanks for the tip! Will submit patch V2 soon.

On Thu, Dec 20, 2018 at 11:07 PM Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

> Hi Kangjie,
>
> Thank you for the patch.
>
> On Thursday, 20 December 2018 09:41:16 EET Kangjie Lu wrote:
> > Both anx78xx_set_bits() and anx78xx_clear_bits() in the poweron process
> > may fail. The fix inserts checks for their return values. If the poweron
> > process fails, it calls anx78xx_poweroff().
> >
> > Signed-off-by: Kangjie Lu <kjlu at umn.edu>
> > ---
> >  drivers/gpu/drm/bridge/analogix-anx78xx.c | 26 ++++++++++++++++-------
> >  1 file changed, 18 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/analogix-anx78xx.c
> > b/drivers/gpu/drm/bridge/analogix-anx78xx.c index
> > f8433c93f463..a57104c71739 100644
> > --- a/drivers/gpu/drm/bridge/analogix-anx78xx.c
> > +++ b/drivers/gpu/drm/bridge/analogix-anx78xx.c
> > @@ -610,20 +610,20 @@ static int anx78xx_enable_interrupts(struct anx78xx
> > *anx78xx) return 0;
> >  }
> >
> > -static void anx78xx_poweron(struct anx78xx *anx78xx)
> > +static int anx78xx_poweron(struct anx78xx *anx78xx)
> >  {
> >       struct anx78xx_platform_data *pdata = &anx78xx->pdata;
> > -     int err;
> > +     int err = 0;
> >
> >       if (WARN_ON(anx78xx->powered))
> > -             return;
> > +             return err;
>
> You can return 0 here.
>
> >
> >       if (pdata->dvdd10) {
> >               err = regulator_enable(pdata->dvdd10);
> >               if (err) {
> >                       DRM_ERROR("Failed to enable DVDD10 regulator:
> %d\n",
> >                                 err);
> > -                     return;
> > +                     return err;
> >               }
> >
> >               usleep_range(1000, 2000);
> > @@ -638,12 +638,18 @@ static void anx78xx_poweron(struct anx78xx
> *anx78xx)
> >       gpiod_set_value_cansleep(pdata->gpiod_reset, 0);
> >
> >       /* Power on registers module */
> > -     anx78xx_set_bits(anx78xx->map[I2C_IDX_TX_P2],
> SP_POWERDOWN_CTRL_REG,
> > +     err = anx78xx_set_bits(anx78xx->map[I2C_IDX_TX_P2],
> SP_POWERDOWN_CTRL_REG,
> > SP_HDCP_PD | SP_AUDIO_PD | SP_VIDEO_PD | SP_LINK_PD);
> > -     anx78xx_clear_bits(anx78xx->map[I2C_IDX_TX_P2],
> SP_POWERDOWN_CTRL_REG,
> > +     err |= anx78xx_clear_bits(anx78xx->map[I2C_IDX_TX_P2],
> > SP_POWERDOWN_CTRL_REG, SP_REGISTER_PD | SP_TOTAL_PD);
>
> If both functions fail with a different error code, this may result in a
> meaningless error being returned. One option is to do
>
>         err = anx78xx_set_bits(...);
>         if (!err)
>                 err = anx78xx_clear_bits(...);
>
> The construct gets quickly ugly though, so I sometimes define register
> accessors as taking an int * for the error code instead of returning it.
>
> void write(..., int *status)
> {
>         if (*status)
>                 return;
>
>         *status = real_write(...);
> }
>
> and then use it as
>
>         int status = 0;
>
>         write(..., &status);
>         write(..., &status);
>         write(..., &status);
>         write(..., &status);
>         write(..., &status);
>
>         return status;
>
> This may be overkill here.
>
> > +     if (err) {
> > +             anx78xx_poweroff(anx78xx);
> > +             return err;
> > +     }
> >
> >       anx78xx->powered = true;
> > +
> > +     return err;
>
> And return 0 here too, removing the need to initialize the err variable to
> 0.
>
> >  }
> >
> >  static void anx78xx_poweroff(struct anx78xx *anx78xx)
> > @@ -1144,7 +1150,9 @@ static irqreturn_t anx78xx_hpd_threaded_handler(int
> > irq, void *data) mutex_lock(&anx78xx->lock);
> >
> >       /* Cable is pulled, power on the chip */
> > -     anx78xx_poweron(anx78xx);
> > +     err = anx78xx_poweron(anx78xx);
> > +     if (err)
> > +             DRM_ERROR("Failed to power on the chip: %d\n", err);
>
> Wouldn't it be better to move the error message to the an78xx_poweron()
> function ? That way it would also be printed in the probe() path.
>
> >       err = anx78xx_enable_interrupts(anx78xx);
> >       if (err)
> > @@ -1379,7 +1387,9 @@ static int anx78xx_i2c_probe(struct i2c_client
> > *client, }
> >
> >       /* Look for supported chip ID */
> > -     anx78xx_poweron(anx78xx);
> > +     err = anx78xx_poweron(anx78xx);
> > +     if (err)
> > +             goto err_poweroff;
>
> If poweron fails, doesn't it clean up after itself ? Do you need to call
> poweroff here ?
>
> >       err = regmap_read(anx78xx->map[I2C_IDX_TX_P2], SP_DEVICE_IDL_REG,
> >                         &idl);
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>
>

-- 
Kangjie Lu
Assistant Professor
Department of Computer Science and Engineering
University of Minnesota
Personal homepage <https://www-users.cs.umn.edu/~kjlu>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20181221/e766e3cd/attachment-0001.html>