Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To:     Kangjie Lu <kjlu@umn.edu>
Cc:     pakki001@umn.edu, Archit Taneja <architt@codeaurora.org>,
        Andrzej Hajda <a.hajda@samsung.com>,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] gpu: anx7808: fix a missing check of return value
Date:   Fri, 21 Dec 2018 07:02:11 +0200
Message-ID: <3157460.hcpWlpvoCn@avalon>
Organization: Ideas on Board Oy
In-Reply-To: <20181220074116.40546-1-kjlu@umn.edu>
References: <20181220074116.40546-1-kjlu@umn.edu>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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@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