Re: [PATCH v3 1/3] hwmon: tmp421: handle I2C errors

[Guenter Roeck]

On Fri, Sep 24, 2021 at 12:30:09PM +0300, Paul Fertser wrote:
> Function i2c_smbus_read_byte_data() can return a negative error number
> instead of the data read if I2C transaction failed for whatever reason.
> 
> Lack of error checking can lead to serious issues on production
> hardware, e.g. errors treated as temperatures produce spurious critical
> temperature-crossed-threshold errors in BMC logs for OCP server
> hardware. The patch was tested with Mellanox OCP Mezzanine card
> emulating TMP421 protocol for temperature sensing which sometimes leads
> to I2C protocol error during early boot up stage.
> 
> Fixes: 9410700b881f ("hwmon: Add driver for Texas Instruments TMP421/422/423 sensor chips")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Paul Fertser <fercerpav@xxxxxxxxx>

Applied.

Thanks,
Guenter

> ---
> 
> Changes from v2:
>  - Do not change data->valid type as that's an unrelated cleanup
>  - Add Fixes: tag
>  - Remove clutter from the commit message
> 
> Changes from v1:
>  - Reorganise code following excellent suggestion by Guenter Roeck
>    to use tagged errors consistently
> 
>  drivers/hwmon/tmp421.c | 41 +++++++++++++++++++++++++++++++----------
>  1 file changed, 31 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/hwmon/tmp421.c b/drivers/hwmon/tmp421.c
> index ede66ea6a730..e6b2b31d17c8 100644
> --- a/drivers/hwmon/tmp421.c
> +++ b/drivers/hwmon/tmp421.c
> @@ -119,38 +119,59 @@ static int temp_from_u16(u16 reg)
>  	return (temp * 1000 + 128) / 256;
>  }
>  
> -static struct tmp421_data *tmp421_update_device(struct device *dev)
> +static int tmp421_update_device(struct tmp421_data *data)
>  {
> -	struct tmp421_data *data = dev_get_drvdata(dev);
>  	struct i2c_client *client = data->client;
> +	int ret = 0;
>  	int i;
>  
>  	mutex_lock(&data->update_lock);
>  
>  	if (time_after(jiffies, data->last_updated + (HZ / 2)) ||
>  	    !data->valid) {
> -		data->config = i2c_smbus_read_byte_data(client,
> -			TMP421_CONFIG_REG_1);
> +		ret = i2c_smbus_read_byte_data(client,
> +					       TMP421_CONFIG_REG_1);
> +		if (ret < 0)
> +			goto exit;
> +		data->config = ret;
>  
>  		for (i = 0; i < data->channels; i++) {
> -			data->temp[i] = i2c_smbus_read_byte_data(client,
> -				TMP421_TEMP_MSB[i]) << 8;
> -			data->temp[i] |= i2c_smbus_read_byte_data(client,
> -				TMP421_TEMP_LSB[i]);
> +			ret = i2c_smbus_read_byte_data(client,
> +						       TMP421_TEMP_MSB[i]);
> +			if (ret < 0)
> +				goto exit;
> +			data->temp[i] = ret << 8;
> +
> +			ret = i2c_smbus_read_byte_data(client,
> +						       TMP421_TEMP_LSB[i]);
> +			if (ret < 0)
> +				goto exit;
> +			data->temp[i] |= ret;
>  		}
>  		data->last_updated = jiffies;
>  		data->valid = 1;
>  	}
>  
> +exit:
>  	mutex_unlock(&data->update_lock);
>  
> -	return data;
> +	if (ret < 0) {
> +		data->valid = 0;
> +		return ret;
> +	}
> +
> +	return 0;
>  }
>  
>  static int tmp421_read(struct device *dev, enum hwmon_sensor_types type,
>  		       u32 attr, int channel, long *val)
>  {
> -	struct tmp421_data *tmp421 = tmp421_update_device(dev);
> +	struct tmp421_data *tmp421 = dev_get_drvdata(dev);
> +	int ret = 0;
> +
> +	ret = tmp421_update_device(tmp421);
> +	if (ret)
> +		return ret;
>  
>  	switch (attr) {
>  	case hwmon_temp_input: