| Message ID | 20210924093011.26083-1-fercerpav@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v3,1/3] hwmon: tmp421: handle I2C errors | expand |
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@vger.kernel.org > Signed-off-by: Paul Fertser <fercerpav@gmail.com> 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:
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:
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@vger.kernel.org Signed-off-by: Paul Fertser <fercerpav@gmail.com> --- 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(-)