Re: [PATCH 5/5] hwmon: (lm90) Add SMBus alert support

[Trent Piepho <tpiepho-KZfg59tc24xl57MIdRCFDg@public.gmane.org>]

On Sat, 13 Feb 2010, Jean Delvare wrote:
> Tested successfully with an ADM1032 chip on its evaluation board. It
> should work fine with all other chips as well.
>
> At this point this is more of a proof-of-concept, we don't do anything
> terribly useful on SMBus alert: we simply log the event. But this could
> later evolve into libsensors signaling so that user-space applications
> can take an appropriate action.

When I added alert support to the lm63 driver, I had it wake up any process
that's select()ing on the the alarm* files.  I also wrote a hw monitor
daemon that supports this system.  It worked pretty well.  Much more
efficient than polling once per second, and also much faster to resond to
an alert event.

I didn't bother to use ARA, since I know what device is generating the
interrupt.  Does your system support devices with an alert line that don't
use SMBus ARA?

> +static void lm90_alert(struct i2c_client *client, unsigned int flag)
> +{
> +	struct lm90_data *data = i2c_get_clientdata(client);
> +	u8 config, alarms;
> +
> +	lm90_read_reg(client, LM90_REG_R_STATUS, &alarms);

Does lm90_read_reg() update the driver's cached copy of the alarm status
register?  Because if it doesn't, then won't anything reading the alarm
status via sysfs not see the alarms?

What happens if a process reads one of the sysfs attributes after the
interrupt is generated but before the driver runs this code?

In the lm63, the alarm register is clear on read.  So you get the alert,
check alarm status, and think there are no alarms.

What I did for the lm63 driver was assume that if there was an alert
generated, there must have been an alarm bit set.  If there isn't one set
now, then the only way it may have been cleared is if the driver read the
alarm status.  In which case the driver's cache copy of the alarm status
should be used to see what generated the alert.

Another thing I found is that after getting notified of the alert, the most
natural thing for userspace to do is check the temperature/fan/etc
attribute that just alarmed.  "Fan Alert, fan1 speed of 2000 RPM is below
minimum of 300 RPM" Huh?  2000 < 300?  What's going on?  The 2000 RPM is
one second old cached data!  The current speed should cause an alarm, but
the 1 HZ max update rate the driver makes userspace wait for it, somewhat
defeating the purpose of the alert irq.  I have the lm63 driver invalidate
the cached data on alert, so that if userspace reads an attribute it gets
fresh data.

> +		/* Re-enable ALERT# output if it was originally enabled and
> +		 * relevant alarms are all clear */
> +		if ((data->config_orig & 0x80) == 0
> +		 && (data->alarms & data->alert_alarms) == 0) {
> +			u8 config;
> +
> +			lm90_read_reg(client, LM90_REG_R_CONFIG1, &config);
> +			if (config & 0x80) {
> +				dev_dbg(&client->dev, "Re-enabling ALERT#\n");
> +				i2c_smbus_write_byte_data(client,
> +							  LM90_REG_W_CONFIG1,
> +							  config & ~0x80);
> +			}
> +		}

I didn't like the idea of having to have userspace poll attributes to get
alarms re-enabled.  I have the driver start a kernel timer going that
checks the alarm status.  It also notified processes sleeping in the alarm
attributes when the check back to 0.  The time period the kernel polling
can be based on the driver's knowledge of the chip's update rate.  It can
be set to poll faster when there is an alarm.  Of course you rejected my
patch to let the lm63 driver set and know the update rate because that
information could never be useful to anyone...