Hi Jean,
On Wed, Nov 03, 2010 at 06:11:35AM -0400, Jean Delvare wrote:
> Hi Guenter,
>
> Thanks for the fast review.
>
> On Tue, 2 Nov 2010 11:40:37 -0700, Guenter Roeck wrote:
> > Hi Jean,
> >
> > On Tue, 2010-11-02 at 12:40 -0400, Jean Delvare wrote:
> > > We have a standard intrusion detection interface now, drivers should
> > > implement it. I've left the old interface in place for the time being,
> > > with a deprecation warning, it will be removed later.
> > >
> > Good idea!
> >
> > > Signed-off-by: Jean Delvare <khali@xxxxxxxxxxxx>
> > > ---
> > > Documentation/hwmon/adm9240 | 2 +-
> > > drivers/hwmon/adm9240.c | 31 ++++++++++++++++++++++++++++---
> > > 2 files changed, 29 insertions(+), 4 deletions(-)
> > >
> > > --- linux-2.6.37-rc1.orig/drivers/hwmon/adm9240.c 2010-11-02 14:32:58.000000000 +0100
> > > +++ linux-2.6.37-rc1/drivers/hwmon/adm9240.c 2010-11-02 16:50:55.000000000 +0100
> > > @@ -20,7 +20,7 @@
> > > * Alarms 16-bit map of active alarms
> > > * Analog Out 0..1250 mV output
> > > *
> > > - * Chassis Intrusion: clear CI latch with 'echo 1 > chassis_clear'
> > > + * Chassis Intrusion: clear CI latch with 'echo 0 > intrusion0_alarm'
> > > *
> > > * Test hardware: Intel SE440BX-2 desktop motherboard --Grant
> > > *
> > > @@ -476,13 +476,15 @@ static ssize_t set_aout(struct device *d
> > > static DEVICE_ATTR(aout_output, S_IRUGO | S_IWUSR, show_aout, set_aout);
> > >
> > > /* chassis_clear */
> > > -static ssize_t chassis_clear(struct device *dev,
> > > +static ssize_t chassis_clear_legacy(struct device *dev,
> > > struct device_attribute *attr,
> > > const char *buf, size_t count)
> > > {
> > > struct i2c_client *client = to_i2c_client(dev);
> > > unsigned long val = simple_strtol(buf, NULL, 10);
> > >
> > > + dev_warn(&client->dev, "Attribute chassis_clear is deprecated, "
> > > + "use intrusion0_alarm instead\n");
> > > if (val == 1) {
> > > i2c_smbus_write_byte_data(client,
> > > ADM9240_REG_CHASSIS_CLEAR, 0x80);
> > > @@ -490,7 +492,29 @@ static ssize_t chassis_clear(struct devi
> > > }
> > > return count;
> > > }
> > > -static DEVICE_ATTR(chassis_clear, S_IWUSR, NULL, chassis_clear);
> > > +static DEVICE_ATTR(chassis_clear, S_IWUSR, NULL, chassis_clear_legacy);
> > > +
> > > +static ssize_t chassis_clear(struct device *dev,
> > > + struct device_attribute *attr,
> > > + const char *buf, size_t count)
> > > +{
> > > + struct i2c_client *client = to_i2c_client(dev);
> > > + struct adm9240_data *data = i2c_get_clientdata(client);
> > > + unsigned long val;
> > > +
> > > + if (strict_strtoul(buf, 10, &val) || val != 0)
> > > + return -EINVAL;
> > > +
> > > + mutex_lock(&data->update_lock);
> > > + data->alarms &= ~(1 << 12);
> >
> > Not sure if that is a good idea. The original code doesn't do it,
>
> Hardly a valid reason per se... There are plenty of cases where the
> original code does the wrong thing ;)
>
Yes, but it is a data point ...
> > and you can not really know for sure if the alarm will be reset;
> > after all, the chassis might still be open.
>
> This OTOH is a very valid point, which I had missed.
>
> > I think it would be better
> > to leave updating the flag to the attribute update handler.
>
> The problem is that this infringes the rule that register value caching
> should be transparent to the caller. When we set limits, we make sure
> that the cache contains the new value as it is written to the chip, so
> that user-space sees the change immediately. Behaving differently for
> the intrusion detection case would be unexpected and confusing.
>
> > If you
> > really want to update the flag immediately, I think you should re-read
> > the register and not just reset the flag.
>
> It's not so easy. For most chips, the intrusion alarm flag is part of
> an alarm register which contains many other flags, which are latched
> when the error condition is spotted and cleared when the register is
> read. Reading such registers other than on user request may lose
> temporary error condition alarms.
>
> What I can offer is resetting data->valid to 0, to force a cache
> refresh on next access from user-space. This should let the driver
> always return the right value. This also has the advantage of being
> cheap from a code perspective. The run-time overhead is negligible
> IMHO, as I don't expect the chassis intrusion alarm to be cleared by
> the user frequently.
Ok, that makes sense.
Since you are at it, would it make sense to make that change
in the w83795 driver as well (ie set data->valid to 0) ?
Guenter
>
> --
> Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
