Re: [PATCH v2 3/3] hwmon: (lm87) Check I2C errors during detection and setup

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] hwmon: (lm87) premature devm_add_action() in lm87_init_client() on error path
--

commit c6264ca6c303f980a5123962ced0900012b6d8d6
Author: Baek Kyu, Kim <pocj8ur4in@gmail.com>

hwmon: (lm87) Check I2C errors during detection and setup

This commit adds error checking for SMBus read and write operations in
lm87_detect(), lm87_restore_config(), and lm87_init_client().

> diff --git a/drivers/hwmon/lm87.c b/drivers/hwmon/lm87.c
> index a66d486b314cf..32ec44f583eb8 100644
> --- a/drivers/hwmon/lm87.c
> +++ b/drivers/hwmon/lm87.c

[ ... ]

> @@ -989,7 +994,12 @@ static void lm87_restore_config(void *arg)
>  	struct i2c_client *client = arg;
>  	struct lm87_data *data = i2c_get_clientdata(client);
>  
> -	lm87_write_value(client, LM87_REG_CONFIG, data->config);
> +	int err;
> +
> +	err = lm87_write_value(client, LM87_REG_CONFIG, data->config);
> +	if (err < 0)
> +		dev_warn(&client->dev, "Failed to restore configuration: %d\n",
> +			 err);
>  }
>  
>  static int lm87_init_client(struct i2c_client *client)
> @@ -1013,16 +1023,26 @@ static int lm87_init_client(struct i2c_client *client)

[ ... ]

> -	data->config = lm87_read_value(client, LM87_REG_CONFIG) & 0x6F;
> +	rc = lm87_read_value(client, LM87_REG_CONFIG);
> +	if (rc < 0)
> +		return rc;
> +	data->config = rc & 0x6F;
>  
>  	rc = devm_add_action(&client->dev, lm87_restore_config, client);
>  	if (rc)
> @@ -1033,28 +1053,52 @@ static int lm87_init_client(struct i2c_client *client)
>  
>  		/* Limits are left uninitialized after power-up */
>  		for (i = 1; i < 6; i++) {
> -			lm87_write_value(client, LM87_REG_IN_MIN(i), 0x00);
> -			lm87_write_value(client, LM87_REG_IN_MAX(i), 0xFF);
> +			rc = lm87_write_value(client, LM87_REG_IN_MIN(i), 0x00);
> +			if (rc < 0)
> +				return rc;
> +			rc = lm87_write_value(client, LM87_REG_IN_MAX(i), 0xFF);
> +			if (rc < 0)
> +				return rc;
>  		}

[ ... ]

>  	/* Make sure Start is set and INT#_Clear is clear */
>  	if ((data->config & 0x09) != 0x01)
> -		lm87_write_value(client, LM87_REG_CONFIG,
> -				 (data->config & 0x77) | 0x01);
> +		return lm87_write_value(client, LM87_REG_CONFIG,
> +					(data->config & 0x77) | 0x01);
>  	return 0;
>  }

[Severity: Low]
In drivers/hwmon/lm87.c, lm87_init_client() registers lm87_restore_config()
via devm_add_action() before writing the sensor limit registers and before
updating LM87_REG_CONFIG at the end of the function:

	/* Make sure Start is set and INT#_Clear is clear */
	if ((data->config & 0x09) != 0x01)
		return lm87_write_value(client, LM87_REG_CONFIG,
					(data->config & 0x77) | 0x01);

Now that lm87_init_client() returns early when any of the limit register
writes or the final LM87_REG_CONFIG write fails, devres cleanup will run
lm87_restore_config() during probe unwind even though LM87_REG_CONFIG was
never modified. On an already failing bus, that issues an extra write to
LM87_REG_CONFIG and logs a "Failed to restore configuration" warning.

Should devm_add_action() (or devm_add_action_or_reset()) only be called
after LM87_REG_CONFIG has been successfully modified?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029164016.338450-1-pocj8ur4in@gmail.com?part=3