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:
