[lm-sensors] Proposal to individual alarm/beep sysfs files

[lm-sensors] Proposal to individual alarm/beep sysfs files

[Jean Delvare]

Hi all,

Last week, in the course of writing a hardware monitoring driver for the
SMSC LPC47M192 chip, Hartmut Rick and I discussed how alarms could be
represented under sysfs in a standard way, so that no knowledge of the
chip is needed for userspace libraries and tools. I'll try to summarize
our conclusion here, and also add a few thoughts I had since. Hopefully
this can lead us to a fully standard solution; this is the last step I
am aware of before a chip-independant library can be written.

The original discussion was between per-type files versus per-channel
files. I think we agreed that individual files were better, allowing a
much cleaner implementation. Per-type files would require us to reorder
all bits when the chip has them in a random order (which is most often
the case, unfortunately). With per-channel files, we just have to
extract the correct bit, which is easy. We can even use the same
callback for all alarms, as demonstrated at the end of this post.

Now, some chips offer more than one alarm for a given channel. For
example the LM90/ADM1032 has a separate alarm for low limit crossed,
high limit crossed and critical limit crossed. It was suggested that we
could have non-boolean alarm files, and it makes some sense. However,
on second though, it means that we would have to define a standard bit
order inside such alarm files, and we'll have to build the contents
from the registers. This won't be easier than having per-type files,
which we said we did not want to do.

So my new suggestion is to have per-limit alarm files for chips which
implement things that way. This makes every alarm file a boolean. The
only drawback I can think of is that we'll end up with many more sysfs
files defined in each driver. However, the size increase seems to be
acceptable, thanks to the dynamic sysfs callbacks and the array of
attributes which we can use now.

Fault conditions and beep masks can be handled exactly the same way.

Here is a proposed patch to sysfs-interface which summarizes my views.
It's only a draft for now, comments are welcome.

--- linux-2.6.16-rc5.orig/Documentation/hwmon/sysfs-interface	2006-03-05 17:41:07.000000000 +0100
+++ linux-2.6.16-rc5/Documentation/hwmon/sysfs-interface	2006-03-05 22:38:34.000000000 +0100
@@ -246,9 +246,68 @@
 		Read only.
 
 
-*********
-* Other *
-*********
+**********
+* Alarms *
+**********
+
+Each channel or limit may have an associated alarm file, containing a
+boolean value. 1 means than an alarm condition exists, 0 means no alarm.
+
+Usually a given chip will either use channel-related alarms, or
+limit-related alarms, not both. The driver should just reflect the hardware
+implementation.
+
+in[0-n]_alarm
+fan[1-n]_alarm
+temp[1-n]_alarm
+		Channel alarm
+		Boolean
+		Read-only
+
+OR
+
+in[0-n]_min_alarm
+in[0-n]_max_alarm
+fan[1-n]_min_alarm
+temp[1-n]_min_alarm
+temp[1-n]_max_alarm
+temp[1-n]_crit_alarm
+		Limit alarm
+		Boolean
+		Read-only
+
+In theory, a chip could provide per-limit beep masking, but no such chip
+was seen so far.
+
+Each input channel may have an associated fault file. This can be used
+to notify open diodes, unconnected fans etc. where the hardware
+supports it. When this boolean has value 1, the measurement for that
+channel should not be trusted.
+
+fan[1-n]_input_fault
+temp[1-n]_input_fault
+		Input fault condition
+		Boolean
+		Read-only
+
+Some chips also offer the possibility to get beeped when an alarm occurs:
+
+beep_enable	Master beep enable
+		0 to disable.
+		1 to enable.
+		Read/Write
+
+in[0-n]_beep
+fan[1-n]_beep
+temp[1-n]_beep
+		Channel beep
+		0 to disable.
+		1 to enable.
+		Read/write
+
+Old drivers provided a different, non-standard interface to alarms and
+beeps. These interface files are deprecated, but will be kept around
+for compatibility reasons:
 
 alarms		Alarm bitmask.
 		Read only.
@@ -259,33 +318,22 @@
 		if it is still valid.
 		Generally a direct representation of a chip's internal
 		alarm registers; there is no standard for the position
-		of individual bits.
+		of individual bits. For this reason, the use of this
+		interface file for new drivers is discouraged. Use
+		individual *_alarm and *_fault files instead.
 		Bits are defined in kernel/include/sensors.h.
 
-alarms_in	Alarm bitmask relative to in (voltage) channels
-		Read only
-		A '1' bit means an alarm, LSB corresponds to in0 and so on
-		Prefered to 'alarms' for newer chips
-
-alarms_fan	Alarm bitmask relative to fan channels
-		Read only
-		A '1' bit means an alarm, LSB corresponds to fan1 and so on
-		Prefered to 'alarms' for newer chips
-
-alarms_temp	Alarm bitmask relative to temp (temperature) channels
-		Read only
-		A '1' bit means an alarm, LSB corresponds to temp1 and so on
-		Prefered to 'alarms' for newer chips
-
-beep_enable	Beep/interrupt enable
-		0 to disable.
-		1 to enable.
-		Read/Write
-
 beep_mask	Bitmask for beep.
-		Same format as 'alarms' with the same bit locations.
+		Same format as 'alarms' with the same bit locations,
+		use discouraged for the same reason. Use individual
+		*_beep files instead.
 		Read/Write
 
+
+*********
+* Other *
+*********
+
 eeprom		Raw EEPROM data in binary form.
 		Read only.
 

Additionally, you can check these three driver conversions I did
already as an experiment:
http://khali.linux-fr.org/devel/i2c/linux-2.6/hwmon-f71805f-individual-alarm-files.patch
http://khali.linux-fr.org/devel/i2c/linux-2.6/hwmon-lm63-individual-alarm-files.patch
http://khali.linux-fr.org/devel/i2c/linux-2.6/hwmon-lm90-individual-alarm-files.patch

Note that we will have to leave all old "alarms" files in place anyway,
as they have been there for so long now, and we don't want to rewrite
libsensors and sensors anyway. But the new files will make it possible
to write a new library... someday. New drivers will also only have the
individual files.

Thanks,
-- 
Jean Delvare

[lm-sensors] Proposal to individual alarm/beep sysfs files

[Philip Pokorny]

Jean Delvare wrote:

>The original discussion was between per-type files versus per-channel
>files. I think we agreed that individual files were better, allowing a
>much cleaner implementation. Per-type files would require us to reorder
>all bits when the chip has them in a random order (which is most often
>the case, unfortunately). With per-channel files, we just have to
>extract the correct bit, which is easy. We can even use the same
>callback for all alarms, as demonstrated at the end of this post.
>
>Now, some chips offer more than one alarm for a given channel. For
>example the LM90/ADM1032 has a separate alarm for low limit crossed,
>high limit crossed and critical limit crossed. It was suggested that we
>could have non-boolean alarm files, and it makes some sense. However,
>on second though, it means that we would have to define a standard bit
>order inside such alarm files, and we'll have to build the contents
>from the registers. This won't be easier than having per-type files,
>which we said we did not want to do.
>
>So my new suggestion is to have per-limit alarm files for chips which
>implement things that way. This makes every alarm file a boolean. The
>only drawback I can think of is that we'll end up with many more sysfs
>files defined in each driver. However, the size increase seems to be
>acceptable, thanks to the dynamic sysfs callbacks and the array of
>attributes which we can use now.
>
>Fault conditions and beep masks can be handled exactly the same way.
>  
>

What about using strings in the alarm files instead of booleans?  Then 
you could report the *name* of the limit or condition causing the alarm 
(or alarms) or an empty string if there is no alarm?

When you look at the code in libsensors, it seems that alarms are almost 
always converted into text strings.  Why not move that code into the 
driver itself and then user-space tools would just print the contents of 
the ALARM file rather than having to parse the alarm file name to 
determine what limit or condition is causing the alarm or fault.

:v)

[lm-sensors] Proposal to individual alarm/beep sysfs files

[Hans de Goede]

Jean Delvare wrote:

>The original discussion was between per-type files versus per-channel
>files. I think we agreed that individual files were better, allowing a
>much cleaner implementation. Per-type files would require us to reorder
>all bits when the chip has them in a random order (which is most often
>the case, unfortunately). With per-channel files, we just have to
>extract the correct bit, which is easy. We can even use the same
>callback for all alarms, as demonstrated at the end of this post.
>
>Now, some chips offer more than one alarm for a given channel. For
>example the LM90/ADM1032 has a separate alarm for low limit crossed,
>high limit crossed and critical limit crossed. It was suggested that we
>could have non-boolean alarm files, and it makes some sense. However,
>on second though, it means that we would have to define a standard bit
>order inside such alarm files, and we'll have to build the contents
>from the registers. This won't be easier than having per-type files,
>which we said we did not want to do.
>
>So my new suggestion is to have per-limit alarm files for chips which
>implement things that way. This makes every alarm file a boolean. The
>only drawback I can think of is that we'll end up with many more sysfs
>files defined in each driver. However, the size increase seems to be
>acceptable, thanks to the dynamic sysfs callbacks and the array of
>attributes which we can use now.
>
>Fault conditions and beep masks can be handled exactly the same way.
>  
>

I still vote for having one file per type of alarm. All you need is an 
array which contains the correct masks per sensor type for the alarm reg 
and then a loop which contructs a new virtual alarm reg , by masking the 
real alarm reg with the values in the array and storing the true/false 
results linear in a new mask.

I think having a zillion files in sysfs is a bad idea, besides the 
obvious driver size this also requires a kobject per file, which is a 
refcount ed object and thus not so light.

Either way we need a standard and we need it yesterday.

Regards,

Hans

[lm-sensors] Proposal to individual alarm/beep sysfs files

[Jean Delvare]

Hi Philip,

Thanks for joining the discussion, it's much appreciated.

On 2006-03-06, Philip Pokorny wrote:
> > So my new suggestion is to have per-limit alarm files for chips which
> > implement things that way. This makes every alarm file a boolean. The
> > only drawback I can think of is that we'll end up with many more sysfs
> > files defined in each driver. However, the size increase seems to be
> > acceptable, thanks to the dynamic sysfs callbacks and the array of
> > attributes which we can use now.
> >
> > Fault conditions and beep masks can be handled exactly the same way.
>
> What about using strings in the alarm files instead of booleans?  Then
> you could report the *name* of the limit or condition causing the alarm
> (or alarms) or an empty string if there is no alarm?

A given input channel may have several alarms active at the same time,
for example the high and critical limits may be active at the same time
if the temperature is above both limits. It's even possible to have the
low and high limit alarms active at the same time if the chip is
misproperly configured ;)

How would you handle this situation? Returning more than one string per
file is sort of against the sysfs philosophy. Unless we decide to see it
as a single string... I guess it's a matter of point of view.

> When you look at the code in libsensors, it seems that alarms are almost
> always converted into text strings.  Why not move that code into the
> driver itself and then user-space tools would just print the contents of
> the ALARM file rather than having to parse the alarm file name to
> determine what limit or condition is causing the alarm or fault.

Please try to forget all you know about libsensors here. If just for
libsensors, we would not be discussing this new interface, for the old
one would be enough. Likewise, don't think too much about the
"sensors" program. It's really only one hardware monitoring tool
among a dozen.

So, thinking about a new library, relying on a chip-independant sysfs
interface, I don't think your proposal will help most of the programs
using it. Graphical tools may want to express alarm conditions with a
different color, or with some blinking led-like picture. If the sysfs
interface file says "HIGH" instead of "1", it doesn't help them at
all, does it?

Also, even for text-based programs, authors may want to internationalize
them, so they'll need to parse the file contents anyway, rather than
displaying it directly. Good coders will want to parse the file contents
anyway, because it wouldn't be safe to let a string go through without
some minimum checks.

So all in all I don't see much added value to having drivers export
alarm conditions as strings, sorry. I'm still carefully listening to
counter arguments though, and interested in seeing actual code for
alternative proposals.

Thanks,
--
Jean Delvare

[lm-sensors] Proposal to individual alarm/beep sysfs files

[Jean Delvare]

Hi Hans,

Now I remember that you posted a sysfs interface update proposal some
times ago, which I failed to review... I'm sorry about that, I'll try
to gove it a try this week. Whatever, thanks for joining this one
discussion.

On 2006-03-06, Hans de Goede wrote:
> > The original discussion was between per-type files versus per-channel
> > files. I think we agreed that individual files were better, allowing a
> > much cleaner implementation. Per-type files would require us to reorder
> > all bits when the chip has them in a random order (which is most often
> > the case, unfortunately). With per-channel files, we just have to
> > extract the correct bit, which is easy. We can even use the same
> > callback for all alarms, as demonstrated at the end of this post.
> > (...)
> > So my new suggestion is to have per-limit alarm files for chips which
> > implement things that way. This makes every alarm file a boolean. The
> > only drawback I can think of is that we'll end up with many more sysfs
> > files defined in each driver. However, the size increase seems to be
> > acceptable, thanks to the dynamic sysfs callbacks and the array of
> > attributes which we can use now.
> >
> >Fault conditions and beep masks can be handled exactly the same way.
>
> I still vote for having one file per type of alarm. All you need is an
> array which contains the correct masks per sensor type for the alarm reg
> and then a loop which contructs a new virtual alarm reg , by masking the
> real alarm reg with the values in the array and storing the true/false
> results linear in a new mask.

I thought that way in the first place, and actually my recently merged
f71805f driver does this. However, there are two reasons for which I no
more think this was a good idea:

1* How do you handle the case where a given channel has several possible
alarm conditions, as described above? Take a look at the lm90 driver and
LM90 or ADM1032 datasheet for a concrete example. I would welcome a
patch to the lm90 driver demonstrating how you would implement your
proposal for this driver.

2* The code doing that would probably be horrible and hard to check and
maintain. I admit I did not even give it a try, as it looked too
frightening right away ;) Do you want to do it? Please propose a patch
to the w83627hf driver, implementing the alarm interface the way you
want (while still keeping the old "alarms" file in place.) We'll see
what it looks like.

> I think having a zillion files in sysfs is a bad idea, besides the
> obvious driver size this also requires a kobject per file, which is a
> refcount ed object and thus not so light.

My experiments show that the driver size really isn't a problem. I
should have posted actual numbers with my proposal, but forgot, sorry
about that. I'll provide exact figures this evening. From my memory,
for a driver with an original size of around 11 kB (all of f71805f, lm63
and lm90 fall into that category) the size increase was below 0.5 kB. As
long as we can use a single callback for all sysfs files, I don't
expect the driver size to be a problem.

Now, the memory consumption is a different issue. I don't quite know how
to evaluate the generated overhead. Can you help with this? Does lsmod
show how much total memory a driver uses? I don't think so. If we
can't measure this overhead easily, maybe we can at least evaluate it.
How large is a kobject typically? Or, even more importantly, how much
memory does every additional sysfs file consume?

> Either way we need a standard and we need it yesterday.

Absolutely...

I'll try to convert more drivers to see if there are any more different
cases which need to be taken into account. In fact, ideally I would
convert them *all* before submitting my proposal again, but I'm not
sure if I'll have the time to actually do that. Others are welcome to
propose concrete patches and actual numbers for alternative
implementations so that we can compare all proposals on a technical
ground.

Thanks,
--
Jean Delvare

[lm-sensors] Proposal to individual alarm/beep sysfs files

[Mark M. Hoffman]

Hi Jean, Hans, et. al.:

I've been following this thread and the later, related thread very closely
(although I've been quiet until now.)

* Jean Delvare <khali at linux-fr.org> [2006-03-05 23:11:05 +0100]:
> Last week, in the course of writing a hardware monitoring driver for the
> SMSC LPC47M192 chip, Hartmut Rick and I discussed how alarms could be
> represented under sysfs in a standard way, so that no knowledge of the
> chip is needed for userspace libraries and tools. I'll try to summarize
> our conclusion here, and also add a few thoughts I had since. Hopefully
> this can lead us to a fully standard solution; this is the last step I
> am aware of before a chip-independant library can be written.

> The original discussion was between per-type files versus per-channel
> files. I think we agreed that individual files were better, allowing a
> much cleaner implementation. Per-type files would require us to reorder
> all bits when the chip has them in a random order (which is most often
> the case, unfortunately). With per-channel files, we just have to
> extract the correct bit, which is easy. We can even use the same
> callback for all alarms, as demonstrated at the end of this post.

I like one file per alarm as a standard.  No alternative (short of dropping
the sysfs filesystem altogether) is better.  But I have a suggestion...

> Now, some chips offer more than one alarm for a given channel. For
> example the LM90/ADM1032 has a separate alarm for low limit crossed,
> high limit crossed and critical limit crossed. It was suggested that we
> could have non-boolean alarm files, and it makes some sense. However,
> on second though, it means that we would have to define a standard bit
> order inside such alarm files, and we'll have to build the contents
> from the registers. This won't be easier than having per-type files,
> which we said we did not want to do.
> 
> So my new suggestion is to have per-limit alarm files for chips which
> implement things that way. This makes every alarm file a boolean. The
> only drawback I can think of is that we'll end up with many more sysfs
> files defined in each driver. However, the size increase seems to be
> acceptable, thanks to the dynamic sysfs callbacks and the array of
> attributes which we can use now.
> 
> Fault conditions and beep masks can be handled exactly the same way.
> 
> Here is a proposed patch to sysfs-interface which summarizes my views.
> It's only a draft for now, comments are welcome.

<snip>

> +Old drivers provided a different, non-standard interface to alarms and
> +beeps. These interface files are deprecated, but will be kept around
> +for compatibility reasons:
>  
>  alarms		Alarm bitmask.
>  		Read only.

Why deprecate these?  It is easy enough to leave the few extra files
there.  Then, if some embedded app really needs that last 1-2k, they
can chop out (by way of CONFIG_ option) all of the individual alarm
files and save the space.  At any rate, it is useful to 'cat .../alarms'
at a glance to see if it's nonzero.  Not every user needs the full
sensors(1) & libsensors.

Also BTW... in the related thread, Hans wrote:
> (I've learned programming on a 8051, so 9k additional data is _huge_ to me).

I hear that, Hans.  Linux is the most scalable software I've ever seen, but
it can't (and AFAICT, won't ever) scale down to the level of the stuff I (we?)
work on at the day job.  Suffice to say, if you need hwmon in a Linux box
and you can't afford 1-2k more, you're going to be patching quite a lot of
stuff already anyway.  E.g. have you noticed this one?

>    text    data     bss     dec     hex filename
>    8095      31      24    8150    1fd6 drivers/pci/quirks.o

Guaranteed most of that doesn't apply to any given machine, and (almost) none
of it can be CONFIG'ed away.  (I know it's all marked __init or __devinit,
but it's still in the kernel image.)

Regards,

-- 
Mark M. Hoffman
mhoffman at lightlink.com

[lm-sensors] Proposal to individual alarm/beep sysfs files

[Jean Delvare]

Hi Mark,

> > +Old drivers provided a different, non-standard interface to alarms and
> > +beeps. These interface files are deprecated, but will be kept around
> > +for compatibility reasons:
> >  
> >  alarms		Alarm bitmask.
> >  		Read only.
> 
> Why deprecate these?  It is easy enough to leave the few extra files
> there.  Then, if some embedded app really needs that last 1-2k, they
> can chop out (by way of CONFIG_ option) all of the individual alarm
> files and save the space.  At any rate, it is useful to 'cat .../alarms'
> at a glance to see if it's nonzero.  Not every user needs the full
> sensors(1) & libsensors.

By "deprecate" I meant that I don't expect new drivers to provide that
file. For all the drivers which already exists, it's pretty clear to me
that we won't drop the "alarms" file, it's been around for so long now,
and none of us is willing to update support for all chips in
libsensors anyway. Maybe in a very long time, when everyone is using
2.6.17+ kernels and the future version of libsensors (which will only
rely on standard interface files), we'll offer a configuration option
to strip these non-standard files out of the drivers - but the
corresponding source code is likely to stay in there forever.

You seem to want new drivers to still include this chip-dependant
alarms file. I don't think it's that useful even for embedded designs.
You might always have some alarm stuck for an input you don't use, so a
non-zero "alarms" value might not be a problem. Then for valid alarms,
what to do depends on which alarm triggered, so further investigation
will be needed anyway. Thus Hans' reordered alarm files have much more
value here.

Now, if enough people ask for the alarms file to be back in new
drivers, why not. As you said, it's easy enough. But I'd prefer to see
if anyone actually asks for that feature before doing so.

Thanks,
-- 
Jean Delvare

[lm-sensors] Proposal to individual alarm/beep sysfs files

[Greg KH]

On Mon, Mar 06, 2006 at 08:30:11AM +0100, Hans de Goede wrote:
> Jean Delvare wrote:
> 
> >The original discussion was between per-type files versus per-channel
> >files. I think we agreed that individual files were better, allowing a
> >much cleaner implementation. Per-type files would require us to reorder
> >all bits when the chip has them in a random order (which is most often
> >the case, unfortunately). With per-channel files, we just have to
> >extract the correct bit, which is easy. We can even use the same
> >callback for all alarms, as demonstrated at the end of this post.
> >
> >Now, some chips offer more than one alarm for a given channel. For
> >example the LM90/ADM1032 has a separate alarm for low limit crossed,
> >high limit crossed and critical limit crossed. It was suggested that we
> >could have non-boolean alarm files, and it makes some sense. However,
> >on second though, it means that we would have to define a standard bit
> >order inside such alarm files, and we'll have to build the contents
> >from the registers. This won't be easier than having per-type files,
> >which we said we did not want to do.
> >
> >So my new suggestion is to have per-limit alarm files for chips which
> >implement things that way. This makes every alarm file a boolean. The
> >only drawback I can think of is that we'll end up with many more sysfs
> >files defined in each driver. However, the size increase seems to be
> >acceptable, thanks to the dynamic sysfs callbacks and the array of
> >attributes which we can use now.
> >
> >Fault conditions and beep masks can be handled exactly the same way.
> >  
> >
> 
> I still vote for having one file per type of alarm. All you need is an 
> array which contains the correct masks per sensor type for the alarm reg 
> and then a loop which contructs a new virtual alarm reg , by masking the 
> real alarm reg with the values in the array and storing the true/false 
> results linear in a new mask.
> 
> I think having a zillion files in sysfs is a bad idea, besides the 
> obvious driver size this also requires a kobject per file, which is a 
> refcount ed object and thus not so light.

No, a zillion files in sysfs is a fine idea.  One value per file is the
rule here for sysfs, and having binary alarm masks that must be decoded
is a mess.  It's also fixing your api into stone for pretty much
forever.

However if you have one file per alarm type, you can add new alarm types
in the future and not support older ones just fine, the file just will
not be there.

Also, sysfs does (well, will, in 2.6.17, and -mm right now) support
polling sysfs files to allow userspace to wake up when something has
happened.  With this interface, the files do not needed to be read from
all the time to see if something happened, just make a poll() call, and
when woken up, go read the file that changed.  This makes userspace much
simpler and lighter-weight on the whole system.  It also allows your
userspace program to easily handle many multiple different alarms at the
same time (when woken up, you know which file descriptor has changed so
you go read only that one.)

So, in short, lots of sysfs files are not something to be scared about,
they are a good thing.

thanks,

greg k-h