Re: [PATCH] iio: light: al3010: Fix an error handling path in al3010_probe()

[Jonathan Cameron]

On Tue, 10 Sep 2024 20:36:06 +0200
Christophe JAILLET <christophe.jaillet@xxxxxxxxxx> wrote:

> If i2c_smbus_write_byte_data() fails in al3010_init(),
> al3010_set_pwr(false) is not called.
> 
> In order to avoid such a situation, move the devm_add_action_or_reset()
> witch calls al3010_set_pwr(false) right after a successful
> al3010_set_pwr(true).
> 
> Fixes: c36b5195ab70 ("iio: light: add Dyna-Image AL3010 driver")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@xxxxxxxxxx>
Looks correct to me, but given the upshot of the bug is that in
a case where an bus access fails we don't power down (which requires a bus
access). It's unlikely to happen in practice and outcome is device
remains powered up when it shouldn't be which isn't too bad an outcome.

Hence I'll queue this up the slow way.
Applied to the testing branch of iio.git for 0-day to play with it before
I rebase on rc1 once available.

Thanks,

Jonathan

> ---
> Compile tested only.
> This patch is speculative, review with care
> ---
>  drivers/iio/light/al3010.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/light/al3010.c b/drivers/iio/light/al3010.c
> index 53569587ccb7..7cbb8b203300 100644
> --- a/drivers/iio/light/al3010.c
> +++ b/drivers/iio/light/al3010.c
> @@ -87,7 +87,12 @@ static int al3010_init(struct al3010_data *data)
>  	int ret;
>  
>  	ret = al3010_set_pwr(data->client, true);
> +	if (ret < 0)
> +		return ret;
>  
> +	ret = devm_add_action_or_reset(&data->client->dev,
> +				       al3010_set_pwr_off,
> +				       data);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -190,12 +195,6 @@ static int al3010_probe(struct i2c_client *client)
>  		return ret;
>  	}
>  
> -	ret = devm_add_action_or_reset(&client->dev,
> -					al3010_set_pwr_off,
> -					data);
> -	if (ret < 0)
> -		return ret;
> -
>  	return devm_iio_device_register(&client->dev, indio_dev);
>  }
>