lkml.org 
[lkml]   [2021]   [Sep]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v3 1/3] hwmon: tmp421: handle I2C errors
    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:

    \
     
     \ /
      Last update: 2021-09-24 14:00    [W:6.699 / U:0.096 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site