On Wed, Mar 03, 2010 at 01:43:56PM +0100, Jean Delvare wrote:
Hi,
> Hi Dan,
>
> On Wed, 3 Mar 2010 10:09:14 +0300, Dan Carpenter wrote:
> > Smatch complains about: e5e9f44c2 i2c: Drop I2C_CLIENT_INSMOD_2 to 8
> >
> > The original define had an "any_chip", but this patch removed it.
> >
> > -#define I2C_CLIENT_INSMOD_8(chip1, chip2, chip3, chip4, chip5, chip6, chip7, chip8) \
> > -enum chips { any_chip, chip1, chip2, chip3, chip4, chip5, chip6, \
> > - chip7, chip8 }
> >
> > This causes some buffer under runs.
> >
> > drivers/hwmon/fschmd.c +1040 fschmd_detect(33) warn: array offset '(kind-1)' can be negative (-1)
> > drivers/hwmon/tmp401.c +527 tmp401_detect(37) warn: array offset '(kind-1)' can be negative (-1)
> > drivers/hwmon/tmp421.c +255 tmp421_detect(30) warn: array offset '(kind-1)' can be negative (-1)
> > drivers/hwmon/tmp421.c +256 tmp421_detect(31) warn: array offset '(kind-1)' can be negative (-1)
> >
> > (From a test version of smatch).
>
> Thanks a lot for reporting. I was well aware that these drivers would
> need special care when getting rid of the I2C_CLIENT_INSMOD_* macros,
> and I _thought_ I had taken care, I even seem to remember changing the
> code in question... but apparently I managed to lose the changes
> somewhere in the process. Sigh.
>
> The following patch should fix it. Please apply it, run smatch again
> and confirm the warnings are gone.
>
> Hans, please confirm that I got the fschmd driver right. Andre, please
> confirm that I got the tmp421 driver right (I decided to change the
> device data to be explicitly the channel count, otherwise the code was
> simply too confusing.) As for the tmp401 driver, it is simple enough
> that I am confident I got it right.
The patch for tmp401 looks good. As you said it's the simple one. The
changes for tmp421 looks good too but I want to investigate it in
detail and give it a test. Unfortunatly I don't have a recent kernel
so I'm building one right now. Tomorrow you will get the result of my
tests.
Regards,
Andre
> * * * * *
>
> From: Jean Delvare <khali@xxxxxxxxxxxx>
> Subject: hwmon: Fix off-by-one kind values
>
> Recent changes on the I2C front have left off-by-one array indexes in
> 3 hwmon drivers. Fix them.
>
> Reported-by: Dan Carpenter <error27@xxxxxxxxx>
> Signed-off-by: Jean Delvare <khali@xxxxxxxxxxxx>
> Cc: Hans de Goede <hdegoede@xxxxxxxxxx>
> Cc: Andre Prendel <andre.prendel@xxxxxx>
> ---
> drivers/hwmon/fschmd.c | 15 ++++++---------
> drivers/hwmon/tmp401.c | 7 +++----
> drivers/hwmon/tmp421.c | 18 +++++++++---------
> 3 files changed, 18 insertions(+), 22 deletions(-)
>
> --- linux-2.6.34-rc0.orig/drivers/hwmon/fschmd.c 2010-03-03 11:22:50.000000000 +0100
> +++ linux-2.6.34-rc0/drivers/hwmon/fschmd.c 2010-03-03 12:15:07.000000000 +0100
> @@ -267,7 +267,7 @@ struct fschmd_data {
> struct list_head list; /* member of the watchdog_data_list */
> struct kref kref;
> struct miscdevice watchdog_miscdev;
> - int kind;
> + enum chips kind;
> unsigned long watchdog_is_open;
> char watchdog_expect_close;
> char watchdog_name[10]; /* must be unique to avoid sysfs conflict */
> @@ -325,8 +325,7 @@ static ssize_t show_in_value(struct devi
> int index = to_sensor_dev_attr(devattr)->index;
> struct fschmd_data *data = fschmd_update_device(dev);
>
> - /* fscher / fschrc - 1 as data->kind is an array index, not a chips */
> - if (data->kind == (fscher - 1) || data->kind >= (fschrc - 1))
> + if (data->kind == fscher || data->kind >= fschrc)
> return sprintf(buf, "%d\n", (data->volt[index] * dmi_vref *
> dmi_mult[index]) / 255 + dmi_offset[index]);
> else
> @@ -492,7 +491,7 @@ static ssize_t show_pwm_auto_point1_pwm(
> int val = data->fan_min[index];
>
> /* 0 = allow turning off (except on the syl), 1-255 = 50-100% */
> - if (val || data->kind == fscsyl - 1)
> + if (val || data->kind == fscsyl)
> val = val / 2 + 128;
>
> return sprintf(buf, "%d\n", val);
> @@ -506,7 +505,7 @@ static ssize_t store_pwm_auto_point1_pwm
> unsigned long v = simple_strtoul(buf, NULL, 10);
>
> /* reg: 0 = allow turning off (except on the syl), 1-255 = 50-100% */
> - if (v || data->kind == fscsyl - 1) {
> + if (v || data->kind == fscsyl) {
> v = SENSORS_LIMIT(v, 128, 255);
> v = (v - 128) * 2 + 1;
> }
> @@ -1037,7 +1036,7 @@ static int fschmd_detect(struct i2c_clie
> else
> return -ENODEV;
>
> - strlcpy(info->type, fschmd_id[kind - 1].name, I2C_NAME_SIZE);
> + strlcpy(info->type, fschmd_id[kind].name, I2C_NAME_SIZE);
>
> return 0;
> }
> @@ -1065,6 +1064,7 @@ static int fschmd_probe(struct i2c_clien
> (where the client is found through a data ptr instead of the
> otherway around) */
> data->client = client;
> + data->kind = kind;
>
> if (kind == fscpos) {
> /* The Poseidon has hardwired temp limits, fill these
> @@ -1085,9 +1085,6 @@ static int fschmd_probe(struct i2c_clien
> }
> }
>
> - /* i2c kind goes from 1-6, we want from 0-5 to address arrays */
> - data->kind = kind - 1;
> -
> /* Read in some never changing registers */
> data->revision = i2c_smbus_read_byte_data(client, FSCHMD_REG_REVISION);
> data->global_control = i2c_smbus_read_byte_data(client,
> --- linux-2.6.34-rc0.orig/drivers/hwmon/tmp401.c 2010-02-25 09:12:22.000000000 +0100
> +++ linux-2.6.34-rc0/drivers/hwmon/tmp401.c 2010-03-03 11:40:35.000000000 +0100
> @@ -134,7 +134,7 @@ struct tmp401_data {
> struct mutex update_lock;
> char valid; /* zero until following fields are valid */
> unsigned long last_updated; /* in jiffies */
> - int kind;
> + enum chips kind;
>
> /* register values */
> u8 status;
> @@ -524,7 +524,7 @@ static int tmp401_detect(struct i2c_clie
> if (reg > 15)
> return -ENODEV;
>
> - strlcpy(info->type, tmp401_id[kind - 1].name, I2C_NAME_SIZE);
> + strlcpy(info->type, tmp401_id[kind].name, I2C_NAME_SIZE);
>
> return 0;
> }
> @@ -572,8 +572,7 @@ static int tmp401_probe(struct i2c_clien
> goto exit_remove;
> }
>
> - dev_info(&client->dev, "Detected TI %s chip\n",
> - names[data->kind - 1]);
> + dev_info(&client->dev, "Detected TI %s chip\n", names[data->kind]);
>
> return 0;
>
> --- linux-2.6.34-rc0.orig/drivers/hwmon/tmp421.c 2010-02-25 09:12:22.000000000 +0100
> +++ linux-2.6.34-rc0/drivers/hwmon/tmp421.c 2010-03-03 12:56:05.000000000 +0100
> @@ -61,9 +61,9 @@ static const u8 TMP421_TEMP_LSB[4] = {
> #define TMP423_DEVICE_ID 0x23
>
> static const struct i2c_device_id tmp421_id[] = {
> - { "tmp421", tmp421 },
> - { "tmp422", tmp422 },
> - { "tmp423", tmp423 },
> + { "tmp421", 2 },
> + { "tmp422", 3 },
> + { "tmp423", 4 },
> { }
> };
> MODULE_DEVICE_TABLE(i2c, tmp421_id);
> @@ -73,7 +73,7 @@ struct tmp421_data {
> struct mutex update_lock;
> char valid;
> unsigned long last_updated;
> - int kind;
> + int channels;
> u8 config;
> s16 temp[4];
> };
> @@ -107,7 +107,7 @@ static struct tmp421_data *tmp421_update
> data->config = i2c_smbus_read_byte_data(client,
> TMP421_CONFIG_REG_1);
>
> - for (i = 0; i <= data->kind; i++) {
> + 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,
> @@ -166,7 +166,7 @@ static mode_t tmp421_is_visible(struct k
> devattr = container_of(a, struct device_attribute, attr);
> index = to_sensor_dev_attr(devattr)->index;
>
> - if (data->kind > index)
> + if (index < data->channels)
> return a->mode;
>
> return 0;
> @@ -252,9 +252,9 @@ static int tmp421_detect(struct i2c_clie
> return -ENODEV;
> }
>
> - strlcpy(info->type, tmp421_id[kind - 1].name, I2C_NAME_SIZE);
> + strlcpy(info->type, tmp421_id[kind].name, I2C_NAME_SIZE);
> dev_info(&adapter->dev, "Detected TI %s chip at 0x%02x\n",
> - names[kind - 1], client->addr);
> + names[kind], client->addr);
>
> return 0;
> }
> @@ -271,7 +271,7 @@ static int tmp421_probe(struct i2c_clien
>
> i2c_set_clientdata(client, data);
> mutex_init(&data->update_lock);
> - data->kind = id->driver_data;
> + data->channels = id->driver_data;
>
> err = tmp421_init_client(client);
> if (err)
>
>
> --
> Jean Delvare
>
> _______________________________________________
> lm-sensors mailing list
> lm-sensors@xxxxxxxxxxxxxx
> http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
