On Tue, Jun 16, 2009 at 10:10:27AM +0200, Jean Delvare wrote:
> On Tue, 16 Jun 2009 09:58:31 +0200, Andre Prendel wrote:
> > On Mon, Jun 15, 2009 at 10:52:24PM +0200, Jean Delvare wrote:
> > > On Mon, 15 Jun 2009 09:43:20 +0200, Andre Prendel wrote:
> > > > +static int temp_from_s16(s16 reg)
> > > > +{
> > > > + int temp = reg;
> > > > +
> > > > + return (temp * 1000 + 128) / 256;
> > > > +}
> > > > +
> > > > +static int temp_from_u16(u16 reg)
> > > > +{
> > > > + int temp = reg;
> > > > +
> > > > + /* Add offset for extended temperature range. */
> > > > + temp -= 64 * 256;
> > > > +
> > > > + return (temp * 1000 + 128) / 256;
> > > > +}
> > > > +
> > > > +static ssize_t show_temp_value(struct device *dev,
> > > > + struct device_attribute *devattr, char *buf)
> > > > +{
> > > > + int index = to_sensor_dev_attr(devattr)->index;
> > > > + struct tmp421_data *data = tmp421_update_device(dev);
> > > > + int temp;
> > > > +
> > > > + if (data->config & TMP421_CONFIG_RANGE)
> > > > + temp = temp_from_u16(data->temp[index]);
> > > > + else
> > > > + temp = temp_from_s16(data->temp[index]);
> > >
> > > This lacks protection: you have no guarantee that data->config and
> > > data->temp[index] belong to the same read cycle.
> >
> > The *data pointer comes from tmp421_update_device(dev). So this should
> > be from the same cycle, shouldn't be. I don't know how to
> > protect. Hardware access is protected in tmp421_update_device(). Maybe
> > I misunderstood something.
>
> You can have concurrent accesses to the sysfs attribute files. Let's
> say you have two files being accessed, the first access when the cache
> is still valid (tmp421_update_device is a no-op) and the second when
> the cache just wore off and needs to be refreshed (tmp421_update_device
> reads from registers).
>
> The first access will test data->config based on the cached value. At
> this point, the second access could cause the cache to be refreshed. I
> agree this is all highly unlikely, but in theory this could still
> happen. Then the first access will use data->temp[index], from the
> refreshed cache. If data->config changed meanwhile, then you just
> called the wrong conversion function for the temperature value.
>
> It's pretty easy to fix:
>
> mutex_lock(&data->update_lock);
> if (data->config & TMP421_CONFIG_RANGE)
> temp = temp_from_u16(data->temp[index]);
> else
> temp = temp_from_s16(data->temp[index]);
> mutex_unlock(&data->update_lock);
>
> This is sufficient to prevent a cache update while you're using the
> cached values.
Hans, Jean, thanks for the explanation. I'll fix it too.
Andre
>
> --
> Jean Delvare
