[lm-sensors] issue with clearing alarms

[lm-sensors] issue with clearing alarms

[Jim Cromie]

hi Jean,

Ive hacked separate voltage alarms into pc87360, and Ive chosen to
add a set_in_min_alarm() callback, which is contrary to the design given in
Doc/hwmon/sysfs-interface:

in[0-*]_min_alarm
in[0-*]_max_alarm
fan[1-*]_min_alarm
temp[1-*]_min_alarm
temp[1-*]_max_alarm
temp[1-*]_crit_alarm
                Limit alarm
                0: no alarm
                1: alarm
                RO                             <--------  conflict here.

So, to clear the alarm, I can now do
    echo 1 > in8_min_alarm
    or echo 0 > in8_min_alarm
to clear the alarm. I ignore the value, since otherwize writing 0 would 
do nothing,
and writing 0 is a reasonable expectation to turn off an alarm.


Heres my reason:

the Channel  Hi/Lo  Limit Exceeded bits are RW1C bits, ie write 1 to clear.
They are both part of the status register, which is a non-obvious way to 
clear the
alarm, and bit-position too, and the status attr-file is readonly anyway.

So here it is in action:

# sensors -s
# sensors
pc87366-isa-6620
Adapter: ISA adapter
avi0:      +3.01 V  (min =  +0.00 V, max =  +3.01 V)
VCORE:     +1.99 V  (min =  +0.00 V, max =  +3.01 V)
VCC:       +4.96 V  (min =  +0.00 V, max =  +6.03 V)
VPWR:     +12.34 V  (min =  +5.93 V, max = +28.02 V)
+12V:     +11.74 V  (min =  +0.00 V, max = +14.46 V)
-12V:     -12.97 V  (min = -60.61 V, max =  -2.76 V)
GND:       +0.00 V  (min =  +0.00 V, max =  +3.01 V)
Vsb:       +3.31 V  (min =  +3.00 V, max =  +3.59 V)
Vdd:       +2.95 V  (min =  +3.00 V, max =  +3.59 V)       ALARM
Vbat:      +3.01 V  (min =  +2.40 V, max =  +3.01 V)
AVdd:      +3.28 V  (min =  +3.00 V, max =  +3.59 V)
Temp:       +104 C  (low  =    +0 C, high =  +125 C)
Critical:   +126 C
Voltage ID:
          +0.000 V  (VRM Version 9.0)

as you can see, Vdd is lower than min, and ALARM is 'issued' by sensors

soekris:/sys/devices/platform/i2c-9191/9191-6620# ls in8*|more; cat in8*
in8_alarm
in8_input
in8_max
in8_max_alarm
in8_min
in8_min_alarm
in8_status
0
1477
1796
0
1501
2
131

here, I lower minimum, but alarm stays on, cuz its not (yet) explicitly 
cleared.

soekris:/sys/devices/platform/i2c-9191/9191-6620# echo 1450 > in8_min
soekris:/sys/devices/platform/i2c-9191/9191-6620# ls in8*|more; cat in8*
in8_alarm
in8_input
in8_max
in8_max_alarm
in8_min
in8_min_alarm
in8_status
0
1465
1796
0
1453
2
131

now I clear it

soekris:/sys/devices/platform/i2c-9191/9191-6620# echo 0 > in8_min_alarm
soekris:/sys/devices/platform/i2c-9191/9191-6620# ls in8*|more; cat in8*
in8_alarm
in8_input
in8_max
in8_max_alarm
in8_min
in8_min_alarm
in8_status
0
1477
1796
0
1453
0
145

<digressions>

status = 145 = 0x91.
bit 7 = 1 means:  New data not yet read.
this suggests that the conversion thats been read is incomplete, and 
presumably inaccurate in the LSBs

also,  in pc87360_detect, you set
        data->vrm = 90;

looking at hwmon-vid.c, the vrm value is clearly related to CPU voltages,
but this chip is a peripheral, so its not clear to me why you hardwired 
this number.
Not that it matters here, since the device is set up (by bios, AFAICT)
such that it doesnt print this
                    printk(KERN_INFO "pc87360: VID "
                           "inputs routed (mode %u)\n",

when it runs this, in static int __init pc87360_find

            /* Are we using thermistors? */
            if (*devid = 0xE9) { /* PC87366 */
                /* These registers are not logical-device
                   specific, just that we won't need them if
                   we don't use the VLM device */
                confreg[2] = superio_inb(sioaddr, 0x2B);
                confreg[3] = superio_inb(sioaddr, 0x25);

                if (confreg[2] & 0x40) {
                    printk(KERN_INFO "pc87360: Using "
                           "thermistors for temperature "
                           "monitoring\n");
                }
                if (confreg[3] & 0xE0) {
                    printk(KERN_INFO "pc87360: VID "
                           "inputs routed (mode %u)\n",
                           confreg[3] >> 5);
                }
            }

[ 7174.482859] pc87360: Device 0x09 not activated
[ 7175.525280] pc87360 9191-6620: VLM conversion set to 1s period, 160us 
delay

btw, 1s conversion period could be related to the "New data not yet read"

comments ?

[lm-sensors] issue with clearing alarms

[Jean Delvare]

Hi Jim,

> Ive hacked separate voltage alarms into pc87360, and Ive chosen to
> add a set_in_min_alarm() callback, which is contrary to the design given in
> Doc/hwmon/sysfs-interface:
> 
> in[0-*]_min_alarm
> in[0-*]_max_alarm
> fan[1-*]_min_alarm
> temp[1-*]_min_alarm
> temp[1-*]_max_alarm
> temp[1-*]_crit_alarm
>                 Limit alarm
>                 0: no alarm
>                 1: alarm
>                 RO                             <--------  conflict here.
> 
> So, to clear the alarm, I can now do
>     echo 1 > in8_min_alarm
>     or echo 0 > in8_min_alarm
> to clear the alarm. I ignore the value, since otherwize writing 0 would 
> do nothing,
> and writing 0 is a reasonable expectation to turn off an alarm.
> 
> 
> Heres my reason:
> 
> the Channel  Hi/Lo  Limit Exceeded bits are RW1C bits, ie write 1 to clear.
> They are both part of the status register, which is a non-obvious way to 
> clear the
> alarm, and bit-position too, and the status attr-file is readonly anyway.
> 
> So here it is in action:
> 
> # sensors -s
> # sensors
> pc87366-isa-6620
> Adapter: ISA adapter
> avi0:      +3.01 V  (min =  +0.00 V, max =  +3.01 V)
> VCORE:     +1.99 V  (min =  +0.00 V, max =  +3.01 V)
> VCC:       +4.96 V  (min =  +0.00 V, max =  +6.03 V)
> VPWR:     +12.34 V  (min =  +5.93 V, max = +28.02 V)
> +12V:     +11.74 V  (min =  +0.00 V, max = +14.46 V)
> -12V:     -12.97 V  (min = -60.61 V, max =  -2.76 V)
> GND:       +0.00 V  (min =  +0.00 V, max =  +3.01 V)
> Vsb:       +3.31 V  (min =  +3.00 V, max =  +3.59 V)
> Vdd:       +2.95 V  (min =  +3.00 V, max =  +3.59 V)       ALARM
> Vbat:      +3.01 V  (min =  +2.40 V, max =  +3.01 V)
> AVdd:      +3.28 V  (min =  +3.00 V, max =  +3.59 V)
> Temp:       +104 C  (low  =    +0 C, high =  +125 C)
> Critical:   +126 C
> Voltage ID:
>           +0.000 V  (VRM Version 9.0)
> 
> as you can see, Vdd is lower than min, and ALARM is 'issued' by sensors
> 
> soekris:/sys/devices/platform/i2c-9191/9191-6620# ls in8*|more; cat in8*
> in8_alarm
> in8_input
> in8_max
> in8_max_alarm
> in8_min
> in8_min_alarm
> in8_status
> 0
> 1477
> 1796
> 0
> 1501
> 2
> 131
> 
> here, I lower minimum, but alarm stays on, cuz its not (yet) explicitly 
> cleared.
> 
> soekris:/sys/devices/platform/i2c-9191/9191-6620# echo 1450 > in8_min
> soekris:/sys/devices/platform/i2c-9191/9191-6620# ls in8*|more; cat in8*
> in8_alarm
> in8_input
> in8_max
> in8_max_alarm
> in8_min
> in8_min_alarm
> in8_status
> 0
> 1465
> 1796
> 0
> 1453
> 2
> 131

It doesn't mean a thing at this point. Alarms stay as long as they
haven't been read at least once, by design. You'd need to "cat in8*"
again after a couple seconds, and my guess is that the alarm would
actually be cleared, because the driver does take care of it:

		/* Voltages */
		for (i = 0; i < data->innr; i++) {
			data->in_status[i] = pc87360_read_value(data, LD_IN, i,
					     PC87365_REG_IN_STATUS);
			/* Clear bits */
			pc87360_write_value(data, LD_IN, i,
					    PC87365_REG_IN_STATUS,
					    data->in_status[i]);

So I think you are trying to fix a problem which doesn't exist.

> 
> now I clear it
> 
> soekris:/sys/devices/platform/i2c-9191/9191-6620# echo 0 > in8_min_alarm
> soekris:/sys/devices/platform/i2c-9191/9191-6620# ls in8*|more; cat in8*
> in8_alarm
> in8_input
> in8_max
> in8_max_alarm
> in8_min
> in8_min_alarm
> in8_status
> 0
> 1477
> 1796
> 0
> 1453
> 0
> 145

Side note: bit 4 was set in the process, doesn't sound right to me.

> <digressions>
> 
> status = 145 = 0x91.
> bit 7 = 1 means:  New data not yet read.
> this suggests that the conversion thats been read is incomplete, and 
> presumably inaccurate in the LSBs

Where in the datasheet do you see this?

To me, "new data not yet read" simply means that at least one sampling
cycle was completed since our last read. There's nothing wrong with
that, as the chip samples its inputs continuously and we only read the
values from times to times.

> 
> also,  in pc87360_detect, you set
>         data->vrm = 90;
> 
> looking at hwmon-vid.c, the vrm value is clearly related to CPU voltages,
> but this chip is a peripheral, so its not clear to me why you hardwired 
> this number.

Because I wrote the pc87360 driver for Linux 2.4 in the first place,
where hwmon-vid and VRM detection didn't exist. Then I ported it to
Linux 2.6.10, where hwmon-vid still didn't existed, as it was
introduced in 2.6.14 if memory serves.

It would be better to use the autodetection now that we have code doing
that, you are right. Care to submit a patch doing this?

> btw, 1s conversion period could be related to the "New data not yet read"

I don't think so, but again this bit being set is not a problem as far
as I can see.

-- 
Jean Delvare

[lm-sensors] issue with clearing alarms

[Jim Cromie]

Jean Delvare wrote:
> Hi Jim,
>
>   
>> as you can see, Vdd is lower than min, and ALARM is 'issued' by sensors
>>
>> here, I lower minimum, but alarm stays on, cuz its not (yet) explicitly 
>> cleared.
>>     
> It doesn't mean a thing at this point. Alarms stay as long as they
> haven't been read at least once, by design. You'd need to "cat in8*"
> again after a couple seconds, and my guess is that the alarm would
> actually be cleared, because the driver does take care of it:
>
>   

You are correct. The 2nd cat shows it cleared.  I'll yank that setter 
callback.
<backpedalling> Im a bit surprised that in the course of playing with 
it, I didnt run 2 cats
(iirc, the console stuff I pasted was a trim-down of what I did)
Anyway, thanks.

soekris:~# cd /sys/devices/platform/i2c-9191/9191-6620/
soekris:/sys/devices/platform/i2c-9191/9191-6620# cat in8*
1477
1796
1501
147
soekris:/sys/devices/platform/i2c-9191/9191-6620# echo 1450 > in8_min
soekris:/sys/devices/platform/i2c-9191/9191-6620# cat in8*
1477
1796
1453
147
soekris:/sys/devices/platform/i2c-9191/9191-6620# cat in8*
1477
1796
1453
145


>
> Side note: bit 4 was set in the process, doesn't sound right to me.
>
>   
It happens w/o my separate-alarms hacks - the above listing is taken 
from unaltered rc6-mm1
rc5 shows same.  I'll poke at it some more.

>> <digressions>
>>
>> status = 145 = 0x91.
>> bit 7 = 1 means:  New data not yet read.
>> this suggests that the conversion thats been read is incomplete, and 
>> presumably inaccurate in the LSBs
>>     
>
> Where in the datasheet do you see this?
>
>   
in Winbond's PC87366.pdf, section 11.5.12, table 3  (page 189)
bit 7 is named "End of Conversion"

bit 4 is "Alarm Output Enable".
I dont understand what its enabling, I'll (get around to) add some 
temporary code to poke at it,
and see whats happening.

> To me, "new data not yet read" simply means that at least one sampling
> cycle was completed since our last read. There's nothing wrong with
> that, as the chip samples its inputs continuously and we only read the
> values from times to times.
>
>   
bit 7 is an RW1C bit, which means that the code you quoted above should 
clear it,
esp when I do: cat in8*; cat in8*
oddly, it does not reset the bit.
Im not seeing anything misbehaving because of it.

>> also,  in pc87360_detect, you set
>>         data->vrm = 90;
>>
>>
>>     
> It would be better to use the autodetection now that we have code doing
> that, you are right. Care to submit a patch doing this?
>
>   
just sent.


>> btw, 1s conversion period could be related to the "New data not yet read"
>>     
>
> I don't think so, but again this bit being set is not a problem as far
> as I can see.
>
>   

Again, Ive not seen any misbehavior, just poking around..

thanks Jean,
jimc

[lm-sensors] issue with clearing alarms

[Jean Delvare]

Hi Jim,

> bit 7 is an RW1C bit, which means that the code you quoted above should 
> clear it,
> esp when I do: cat in8*; cat in8*
> oddly, it does not reset the bit.

I think it does, but given that the PC87366 samples values faster than
the driver allows you to read them (due to the register cache) you
can't see it. You'd need to add some code to the driver or poke the
registers directly from user-space (using isaset and isadump) to see
the bit being actually reset.

But anyway, we don't care about this bit at all.

-- 
Jean Delvare

[lm-sensors] issue with clearing alarms

[Jim Cromie]

Jean Delvare wrote:
> Hi Jim,
>
>   
> To me, "new data not yet read" simply means that at least one sampling
> cycle was completed since our last read. There's nothing wrong with
> that, as the chip samples its inputs continuously and we only read the
> values from times to times.
>
>   


Looking again, I see a point of confusion at least, and a possible problem.
It may be one of interpretation, but is worth checking, and possibly 
documenting.

The *not-yet-read* property pertains to the scan done by update_device(),
since thats what reads all sensors the driver supports.
But its easy to misconstrue not-yet-read as a property on the individual 
alarms.

More importantly, any single read of any property will clear unrelated 
alarms.
Let me demonstrate:

1- raise then lower the min setting, then check the alarm,
    which is (properly) on.  A 2nd read shows it cleared. (also proper)

soekris:/sys/devices/platform/i2c-9191/9191-6620# echo 1500 > in8_min
soekris:/sys/devices/platform/i2c-9191/9191-6620# echo 1400 > in8_min
soekris:/sys/devices/platform/i2c-9191/9191-6620# cat in8_min_alarm
2
soekris:/sys/devices/platform/i2c-9191/9191-6620# cat in8_min_alarm
0

2- repeat raise & lower, then read an *unrelated* attr
    now 1st read of in8_min_alarm shows 0, the alarm state has been *missed*

soekris:/sys/devices/platform/i2c-9191/9191-6620# echo 1500 > in8_min
soekris:/sys/devices/platform/i2c-9191/9191-6620# echo 1400 > in8_min
soekris:/sys/devices/platform/i2c-9191/9191-6620# cat in2_min_alarm
0
soekris:/sys/devices/platform/i2c-9191/9191-6620# cat in8_min_alarm
0

3- redo test 2, this time reading a setting, then reading alarm.  its 
off, should be on.
    this time, its a readback of config value, which doensnt *need* to 
trigger a scan,
    but does cuz its simpler (I guess)

soekris:/sys/devices/platform/i2c-9191/9191-6620# echo 1500 > in8_min
soekris:/sys/devices/platform/i2c-9191/9191-6620# echo 1400 > in8_min
soekris:/sys/devices/platform/i2c-9191/9191-6620# cat in8_min
1394
soekris:/sys/devices/platform/i2c-9191/9191-6620# cat in8_min_alarm
0




IOW, 2 reads of any 2 sysfs-files, separated by this much time, will 
reset the alarm,
even though userspace has never seen the alarm.

    if (time_after(jiffies, data->last_updated + HZ * 2) || !data->valid) {
        dev_dbg(&client->dev, "Data update\n");




So, with the usual caveats (Ive done something wrong, immature code, etc)
I see a few possibilities:

1 - Document the fact - tell users not to do that.
    this isnt unreasonable, and far simpler..
2 - sensors-deamon locks the directory to preclude loss of alarms (is 
this possible ? bad idea ?)
3 - dunno

[lm-sensors] issue with clearing alarms

[Jean Delvare]

Hi Jim,

> Looking again, I see a point of confusion at least, and a possible problem.
> It may be one of interpretation, but is worth checking, and possibly 
> documenting.
> 
> The *not-yet-read* property pertains to the scan done by update_device(),
> since thats what reads all sensors the driver supports.
> But its easy to misconstrue not-yet-read as a property on the individual 
> alarms.
> 
> More importantly, any single read of any property will clear unrelated 
> alarms.

You are 100% correct. Things are as you describe, by driver design.

> So, with the usual caveats (Ive done something wrong, immature code, etc)
> I see a few possibilities:
> 
> 1 - Document the fact - tell users not to do that.
>     this isnt unreasonable, and far simpler..

The user can't do anything. The ones we need to tell are the
application authors. The end user looks at the temperatures in his nice
GUI, and doesn't decide what is read and when. At best he/she can
select the global refresh rate, and that's about it.

> 2 - sensors-deamon locks the directory to preclude loss of alarms (is 
> this possible ? bad idea ?)

Which daemon?

> 3 - dunno

4 - don't care:

What's the point in spending efforts to make sure that the application
will see the one-time alarm, while you have absolutely no way to know
if the user will see it? Unless the application keeps the alarm on
until the user clicks to confirm that he/she did see the alarm... This
doesn't sound very friendly.

All GUIs I know of do not present the alarm data at all, by the way.
Could be because libsensors doesn't make their access very friendly at
the moment... But it could also be because end users don't care - they
can simply compare the measurement to the limits, or even judge the
measurement by themselves. The alarm bits are redundant.

Furthermore, I don't expect any application to actually do selective
reads. I expect them to read all the values at once, and our drivers
will work just fine in that case. Do you know of any exiting
application that does otherwise?

In the real world, one-time alarms don't matter. I don't get why chip
designers almost all did implement alarms that way. Their main use
right now is for us to write and debug drivers.

So, as much as I agree that there is a flaw in our design, I'd say
fixing it isn't worth the effort.

-- 
Jean Delvare

[lm-sensors] issue with clearing alarms

[Jim Cromie]

Jean Delvare wrote:
> Hi Jim,
>
>   
>> Looking again, I see a point of confusion at least, and a possible problem.
>> It may be one of interpretation, but is worth checking, and possibly 
>> documenting.
>>
>> The *not-yet-read* property pertains to the scan done by update_device(),
>> since thats what reads all sensors the driver supports.
>> But its easy to misconstrue not-yet-read as a property on the individual 
>> alarms.
>>
>> More importantly, any single read of any property will clear unrelated 
>> alarms.
>>     
>
> You are 100% correct. Things are as you describe, by driver design.
>
>   
>> So, with the usual caveats (Ive done something wrong, immature code, etc)
>> I see a few possibilities:
>>
>> 1 - Document the fact - tell users not to do that.
>>     this isnt unreasonable, and far simpler..
>>     
>
> The user can't do anything. The ones we need to tell are the
> application authors. The end user looks at the temperatures in his nice
> GUI, and doesn't decide what is read and when. At best he/she can
> select the global refresh rate, and that's about it.
>
>   


>> 2 - sensors-deamon locks the directory to preclude loss of alarms (is 
>> this possible ? bad idea ?)
>>     
>
> Which daemon?
>
>   
sensorsd - not that it matters.
>> 3 - dunno
>>     
>
> 4 - don't care:
>
> What's the point in spending efforts to make sure that the application
> will see the one-time alarm, while you have absolutely no way to know
> if the user will see it? Unless the application keeps the alarm on
> until the user clicks to confirm that he/she did see the alarm... This
> doesn't sound very friendly.
>
> All GUIs I know of do not present the alarm data at all, by the way.
> Could be because libsensors doesn't make their access very friendly at
> the moment... But it could also be because end users don't care - they
> can simply compare the measurement to the limits, or even judge the
> measurement by themselves. The alarm bits are redundant.
>
> Furthermore, I don't expect any application to actually do selective
> reads. I expect them to read all the values at once, and our drivers
> will work just fine in that case. Do you know of any exiting
> application that does otherwise?
>
>   
no, but sensors is all Ive ever needed.

> In the real world, one-time alarms don't matter. I don't get why chip
> designers almost all did implement alarms that way. Their main use
> right now is for us to write and debug drivers.
>
> So, as much as I agree that there is a flaw in our design, I'd say
> fixing it isn't worth the effort.
>
>   
Works for me :-)

But Ive added a couple lines to Doc/hwmon/sysfs-interface, on the 
assumption that
app-writers will read it, where they now learn of libsensors, but might 
miss this fact otherwise.


Signed-off-by:  Jim Cromie  <jim.cromie at gmail.com>

$ diffstat diff.doc-touches.20060926.140844
 Documentation/hwmon/sysfs-interface |    8 ++++----


diff -ruNp -X dontdiff -X exclude-diffs linux-2.6.18-rc6-mm2-sk/Documentation/hwmon/sysfs-interface doc-touches/Documentation/hwmon/sysfs-interface
--- linux-2.6.18-rc6-mm2-sk/Documentation/hwmon/sysfs-interface	2006-09-07 16:09:40.000000000 -0600
+++ doc-touches/Documentation/hwmon/sysfs-interface	2006-09-26 14:04:19.000000000 -0600
@@ -23,6 +23,7 @@ voltages between 0 and +4V. Other voltag
 range using external resistors. Since the values of these resistors
 can change from motherboard to motherboard, the conversions cannot be
 hard coded into the driver and have to be done in user space.
+Therefore, all sysfs values are fixed point numbers.
 
 For this reason, even if we aim at a chip-independent libsensors, it will
 still require a configuration file (e.g. /etc/sensors.conf) for proper
@@ -35,8 +36,9 @@ access this data in a simple and consist
 will have to implement conversion, labeling and hiding of inputs. For
 this reason, it is still not recommended to bypass the library.
 
-If you are developing a userspace application please send us feedback on
-this standard.
+If you are developing a userspace application please send us feedback
+on this standard.  Note that applications are expected to read all the
+sysfs files at once, you may miss transient alarms otherwise.
 
 Note that this standard isn't completely established yet, so it is subject
 to changes. If you are writing a new hardware monitoring driver those
@@ -48,8 +50,6 @@ Each chip gets its own directory in the 
 find all sensor chips, it is easier to follow the device symlinks from
 /sys/class/hwmon/hwmon*.
 
-All sysfs values are fixed point numbers.
-
 There is only one value per file, unlike the older /proc specification.
 The common scheme for files naming is: <type><number>_<item>. Usual
 types for sensor chips are "in" (voltage), "temp" (temperature) and

[lm-sensors] issue with clearing alarms

[Jean Delvare]

> > > 2 - sensors-deamon locks the directory to preclude loss of alarms (is 
> > > this possible ? bad idea ?)
> >
> > Which daemon?
>
> sensorsd - not that it matters.

The only sensorsd I know of runs on OpenBSD. Did you mean sensord?
Sensord wasn't designed to be a central access point to the sensors
data, it's merely a resident monitoring application and nothing more.

> But Ive added a couple lines to Doc/hwmon/sysfs-interface, on the 
> assumption that
> app-writers will read it, where they now learn of libsensors, but might 
> miss this fact otherwise.
> 
> 
> Signed-off-by:  Jim Cromie  <jim.cromie at gmail.com>
> 
> $ diffstat diff.doc-touches.20060926.140844
>  Documentation/hwmon/sysfs-interface |    8 ++++----
> 
> 
> diff -ruNp -X dontdiff -X exclude-diffs linux-2.6.18-rc6-mm2-sk/Documentation/hwmon/sysfs-interface doc-touches/Documentation/hwmon/sysfs-interface
> --- linux-2.6.18-rc6-mm2-sk/Documentation/hwmon/sysfs-interface	2006-09-07 16:09:40.000000000 -0600
> +++ doc-touches/Documentation/hwmon/sysfs-interface	2006-09-26 14:04:19.000000000 -0600
> @@ -23,6 +23,7 @@ voltages between 0 and +4V. Other voltag
>  range using external resistors. Since the values of these resistors
>  can change from motherboard to motherboard, the conversions cannot be
>  hard coded into the driver and have to be done in user space.
> +Therefore, all sysfs values are fixed point numbers.

I see no cause-to-consequence relation here, so why the "therefore"?
What problem are you trying to address by moving this sentence around?

>  For this reason, even if we aim at a chip-independent libsensors, it will
>  still require a configuration file (e.g. /etc/sensors.conf) for proper
> @@ -35,8 +36,9 @@ access this data in a simple and consist
>  will have to implement conversion, labeling and hiding of inputs. For
>  this reason, it is still not recommended to bypass the library.
>  
> -If you are developing a userspace application please send us feedback on
> -this standard.
> +If you are developing a userspace application please send us feedback
> +on this standard.  Note that applications are expected to read all the
> +sysfs files at once, you may miss transient alarms otherwise.

Or you could leave the "on" where it was, so that the diff doesn't look
bigger than it really is?

You are putting the things in the wrong order, I think. The
applications are not expected to read all the sysfs values at once, it
just happens to be what they all do at the moment. And nothing really
wrong will happen if they don't. The different behavior of transient
alarms is a minor annoyance, because, as I explained earlier, nobody
really cares about transient alarms. In fact, people may even be
_happy_ to miss them. So why frighten them with something we don't
expect to bother them at all?

>  Note that this standard isn't completely established yet, so it is subject
>  to changes. If you are writing a new hardware monitoring driver those
> @@ -48,8 +50,6 @@ Each chip gets its own directory in the 
>  find all sensor chips, it is easier to follow the device symlinks from
>  /sys/class/hwmon/hwmon*.
>  
> -All sysfs values are fixed point numbers.
> -
>  There is only one value per file, unlike the older /proc specification.
>  The common scheme for files naming is: <type><number>_<item>. Usual
>  types for sensor chips are "in" (voltage), "temp" (temperature) and

-- 
Jean Delvare

[lm-sensors] issue with clearing alarms

[Jim Cromie]

Jean Delvare wrote:
>>>> 2 - sensors-deamon locks the directory to preclude loss of alarms (is 
>>>> this possible ? bad idea ?)
>>>>         
>>> Which daemon?
>>>       
>> sensorsd - not that it matters.
>>     
>
> The only sensorsd I know of runs on OpenBSD. Did you mean sensord?
> Sensord wasn't designed to be a central access point to the sensors
> data, it's merely a resident monitoring application and nothing more.
>
>   
>> But Ive added a couple lines to Doc/hwmon/sysfs-interface, on the 
>> assumption that
>> app-writers will read it, where they now learn of libsensors, but might 
>> miss this fact otherwise.
>>
>>
>> Signed-off-by:  Jim Cromie  <jim.cromie at gmail.com>
>>
>> $ diffstat diff.doc-touches.20060926.140844
>>  Documentation/hwmon/sysfs-interface |    8 ++++----
>>
>>
>> diff -ruNp -X dontdiff -X exclude-diffs linux-2.6.18-rc6-mm2-sk/Documentation/hwmon/sysfs-interface doc-touches/Documentation/hwmon/sysfs-interface
>> --- linux-2.6.18-rc6-mm2-sk/Documentation/hwmon/sysfs-interface	2006-09-07 16:09:40.000000000 -0600
>> +++ doc-touches/Documentation/hwmon/sysfs-interface	2006-09-26 14:04:19.000000000 -0600
>> @@ -23,6 +23,7 @@ voltages between 0 and +4V. Other voltag
>>  range using external resistors. Since the values of these resistors
>>  can change from motherboard to motherboard, the conversions cannot be
>>  hard coded into the driver and have to be done in user space.
>> +Therefore, all sysfs values are fixed point numbers.
>>     
>
> I see no cause-to-consequence relation here, so why the "therefore"?
> What problem are you trying to address by moving this sentence around?
>
>   
sure, its a little white lie.  I wondered if it would draw comment.
Its easier than explaining why the kernel doesnt allow FP.
Let me turn it around.  Why do you bother to say it at all ?
Motivated readers would just try it, then either google to find out
why its disallowed (I have), or <gasp> ask on some ML.
Besides, the whole paragraph is sufficient reason, to move the sentence,
even if FP were allowed.
>>  For this reason, even if we aim at a chip-independent libsensors, it will
>>  still require a configuration file (e.g. /etc/sensors.conf) for proper
>> @@ -35,8 +36,9 @@ access this data in a simple and consist
>>  will have to implement conversion, labeling and hiding of inputs. For
>>  this reason, it is still not recommended to bypass the library.
>>  
>> -If you are developing a userspace application please send us feedback on
>> -this standard.
>> +If you are developing a userspace application please send us feedback
>> +on this standard.  Note that applications are expected to read all the
>> +sysfs files at once, you may miss transient alarms otherwise.
>>     
>
> Or you could leave the "on" where it was, so that the diff doesn't look
> bigger than it really is?
>
>   
emacs autowrap.  But thats not really the issue is it ?

> You are putting the things in the wrong order, I think. The
> applications are not expected to read all the sysfs values at once, it
> just happens to be what they all do at the moment. And nothing really
> wrong will happen if they don't. The different behavior of transient
> alarms is a minor annoyance, because, as I explained earlier, nobody
> really cares about transient alarms. In fact, people may even be
> _happy_ to miss them. So why frighten them with something we don't
> expect to bother them at all?
>
>   
ignorance != apathy
And if we dont care about alarms, why bother to split them out, 1 per 
sensor ?
>>  Note that this standard isn't completely established yet, so it is subject
>>  to changes. If you are writing a new hardware monitoring driver those
>> @@ -48,8 +50,6 @@ Each chip gets its own directory in the 
>>  find all sensor chips, it is easier to follow the device symlinks from
>>  /sys/class/hwmon/hwmon*.
>>  
>> -All sysfs values are fixed point numbers.
>> -
>>  There is only one value per file, unlike the older /proc specification.
>>  The common scheme for files naming is: <type><number>_<item>. Usual
>>  types for sensor chips are "in" (voltage), "temp" (temperature) and
>>     
>
>   

[lm-sensors] issue with clearing alarms

[Jean Delvare]

Hi Jim,

> > > --- linux-2.6.18-rc6-mm2-sk/Documentation/hwmon/sysfs-interface	2006-09-07 16:09:40.000000000 -0600
> > > +++ doc-touches/Documentation/hwmon/sysfs-interface	2006-09-26 14:04:19.000000000 -0600
> > > @@ -23,6 +23,7 @@ voltages between 0 and +4V. Other voltag
> > >  range using external resistors. Since the values of these resistors
> > >  can change from motherboard to motherboard, the conversions cannot be
> > >  hard coded into the driver and have to be done in user space.
> > > +Therefore, all sysfs values are fixed point numbers.
> >
> > I see no cause-to-consequence relation here, so why the "therefore"?
> > What problem are you trying to address by moving this sentence around?
>
> sure, its a little white lie.  I wondered if it would draw comment.
> Its easier than explaining why the kernel doesnt allow FP.

It's easier, and irrelevant. We just don't need floating point here.

> Let me turn it around.  Why do you bother to say it at all ?

Because it's true, and relevant. This is an important design decision,
and also a difference compared to the Linux 2.4 interface. Well, 2.4
used fixed point as well, but exported the values to procfs as real
numbers, as opposed to integers as we do now.

> Motivated readers would just try it, then either google to find out
> why its disallowed (I have), or <gasp> ask on some ML.
> Besides, the whole paragraph is sufficient reason, to move the sentence,
> even if FP were allowed.

I don't follow you here. You seem to agree that there is no real
relation between the paragraph above, and the fixed point sentence, yet
you still want to move the two next to each other?

> > >  For this reason, even if we aim at a chip-independent libsensors, it will
> > >  still require a configuration file (e.g. /etc/sensors.conf) for proper
> > > @@ -35,8 +36,9 @@ access this data in a simple and consist
> > >  will have to implement conversion, labeling and hiding of inputs. For
> > >  this reason, it is still not recommended to bypass the library.
> > >  
> > > -If you are developing a userspace application please send us feedback on
> > > -this standard.
> > > +If you are developing a userspace application please send us feedback
> > > +on this standard.  Note that applications are expected to read all the
> > > +sysfs files at once, you may miss transient alarms otherwise.
> >
> > Or you could leave the "on" where it was, so that the diff doesn't look
> > bigger than it really is?
>
> emacs autowrap.  But thats not really the issue is it ?

It is. If your text editor reformats the lines as it sees fit, you
can't really work with me. But I guess you can actually reconfigure it
not to misbehave that way.

> > You are putting the things in the wrong order, I think. The
> > applications are not expected to read all the sysfs values at once, it
> > just happens to be what they all do at the moment. And nothing really
> > wrong will happen if they don't. The different behavior of transient
> > alarms is a minor annoyance, because, as I explained earlier, nobody
> > really cares about transient alarms. In fact, people may even be
> > _happy_ to miss them. So why frighten them with something we don't
> > expect to bother them at all?
>
> ignorance != apathy

Certainly, and your point is?

> And if we dont care about alarms, why bother to split them out, 1 per 
> sensor ?

My point is essentially that transient alarms don't matter. Alarms by
themselves, are somewhat more important. Their redundancy helps us
debug the drivers, and we can also use them to quickly understand
configuration issues on user systems, for example negative voltages
with their limits swapped, or the reason why the system is beeping,
etc.

More importantly, we have been providing this data to the applications
for 8 years now, so just dropping it is likely to cause some complaints
if anyone is really interested in them. I would be happy myself without
the alarm bits, and this would get around a lot of work in the next
months as well, so don't tempt me please ;)

-- 
Jean Delvare