Re: [PATCH v3] iio: light: cm32181: Unregister second I2C client if present

[Hans de Goede]

Hi,

On 2/22/23 17:24, Kai-Heng Feng wrote:
> If a second dummy client that talks to the actual I2C address was
> created in probe(), there should be a proper cleanup on driver and
> device removal to avoid leakage.
> 
> So unregister the dummy client via another callback.
> 
> Suggested-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> Fixes: c1e62062ff54 ("iio: light: cm32181: Handle CM3218 ACPI devices with 2 I2C resources")
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2152281
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx>
> ---
> v3:
>  - Use devm_add_action_or_reset() in a correct place.
>  - Wording.
> 
> v2:
>  - Use devm_add_action_or_reset() instead of remove() callback to avoid
>    race.
> 
>  drivers/iio/light/cm32181.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
> index b1674a5bfa368..b3da7a517aaea 100644
> --- a/drivers/iio/light/cm32181.c
> +++ b/drivers/iio/light/cm32181.c
> @@ -429,6 +429,14 @@ static const struct iio_info cm32181_info = {
>  	.attrs			= &cm32181_attribute_group,
>  };
>  
> +static void cm32181_unregister_dummy_client(void *data)
> +{
> +	struct i2c_client *client = data;
> +
> +	/* Unregister the dummy client */
> +	i2c_unregister_device(client);
> +}
> +
>  static int cm32181_probe(struct i2c_client *client)
>  {
>  	struct device *dev = &client->dev;
> @@ -460,6 +468,12 @@ static int cm32181_probe(struct i2c_client *client)
>  		client = i2c_acpi_new_device(dev, 1, &board_info);
>  		if (IS_ERR(client))
>  			return PTR_ERR(client);
> +
> +		ret = devm_add_action_or_reset(dev, cm32181_unregister_dummy_client, client);
> +		if (ret) {
> +			dev_err(dev, "%s: add devres action failed\n", __func__);
> +			return ret;
> +		}

Sorry to be a bit pedantic here, but the only way devm_add_action_or_reset() can fail is
if kmalloc fails and we generally do not log errors for kmalloc failures (because the MM
layer will already complain loudly).

So this can be simplified to just:

		ret = devm_add_action_or_reset(dev, cm32181_unregister_dummy_client, client);
		if (ret)
			return ret;

Can you do a v4 with this changed please ?  With this fixed feel free to add my:

Reviewed-by: Hans de Goede <hdegoede@xxxxxxxxxx>

to v4.

Regards,

Hans