[lm-sensors] [patch 0/3] pc87360 - fix unchecked

[lm-sensors] [patch 0/3] pc87360 - fix unchecked

[Jim Cromie]

Ok, this series of patches should get it right, finally.
IOW, they use multiple sysfs groups to organize the attributes
along the boundaries of sensor-type, configurations, and
sysfs_create/remove_group()s where appropriate.

1- code-move (prep patch)
    moves get-set-decl tuples for 3 items:
        cpu0_vid, vrm, alarms_in
    up, to just after the get-set-decl tuple for Voltages.
    These items are already 'activated' together, so the move
        reinforces the grouping thats made explicit in next patch.

2- add 4 explicit attribute groups for the 5 sensor types:
    voltage (in), therm, temp, and fan & pwm (together in one group).
    use sysfs_remove_group() to drop them,
    but keeps the existing startup code, which calls device_create_file 
in loops.

3- rework sys-device-interface startup
    use sysfs_create_group() for 2 sensor-types which are chip-model 
invariant.
       ie all-or-nothing attribute groups
    other 2 groups vary too much due to configuration, etc,
       so we keep the loops of device_create_file(), but now check their 
returns.

Patch 2 adds the groups, so enlarges the .text (by a bit)
Patch 3 shrinks the function in question, but corrects an omission in 
pc87360_detach_client,
which enlarges it overall (by ~200 bytes)

[jimc at harpo hwmon-stuff]$ size ac*/drivers/hwmon/pc87360.ko
   text    data     bss     dec     hex filename
  14246    3384      32   17662    44fe ac-0/drivers/hwmon/pc87360.ko
  14246    3384      32   17662    44fe ac-1/drivers/hwmon/pc87360.ko
  14398    3800      32   18230    4736 ac-2/drivers/hwmon/pc87360.ko
  14454    3800      32   18286    476e ac-3/drivers/hwmon/pc87360.ko

FWIW -
The patchset survived this stress-test:
     for i in `seq 1000`; do { rmmod pc87360;  modprobe pc87360;  
sensors -s; sensors; }  done
It takes about 3.5 secs to re-mod, and about 0.3 sec to reset and query 
sensors.


Thanks also to Mark Hoffman and GregKH,

who both explained that the static *_ATTR declarations are reusable for
multiple devices, and then repeated themselves enough for me to understand.
This kinda defeats the general applicability of my idea to disable
attrs (by setting mode=0) before theyre created by sysfs_create_group. 

While the above works well enough for 'de-configuring' some of a group's 
members,
it falls down if a 2nd set is wanted, for a 2nd device, unless it 
happens to want the
identical de-configuration.

So, to paraphrase the lesson (and risk screwing up ;)
sysfs attr-groups are 'file-lists', and a file-name can appear in 
multiple sub-directories,
(just like foo/bar, ack/bar), which is what happens if the group has a 
name (foo, ack).
Callbacks also get a struct device *p, with which it can differentiate 
amongst multiple instances
of the group.  (Im still handwaving past how sysfs core provides the 
right devp,
but I havent yet used the source, its easier to just ponder a bit 1st ;)


thanks
~jimc

[lm-sensors] [patch 0/3] pc87360 - fix unchecked

[Jean Delvare]

Jim,

> FWIW -
> The patchset survived this stress-test:
>      for i in `seq 1000`; do { rmmod pc87360;  modprobe pc87360;  
> sensors -s; sensors; }  done
> It takes about 3.5 secs to re-mod, and about 0.3 sec to reset and query 
> sensors.

Given that we aren't suspecting a race condition, this type of test
isn't really helpful. More interesting would be a comparison of the
contents of /sys/class/hwmon/hwmon0/device before and after the
patches. If the contents differ, something's wrong, else everything
should be OK - or at least there is no regression.

Thanks,
-- 
Jean Delvare

[lm-sensors] [patch 0/3] pc87360 - fix unchecked

[Jim Cromie]

Jean Delvare wrote:

< Ive dropped Mark from cc, Im sure he's seen enough from me already ;->

> Jim,
>
>   
>> FWIW -
>> The patchset survived this stress-test:
>>      for i in `seq 1000`; do { rmmod pc87360;  modprobe pc87360;  
>> sensors -s; sensors; }  done
>> It takes about 3.5 secs to re-mod, and about 0.3 sec to reset and query 
>> sensors.
>>     
>
> Given that we aren't suspecting a race condition, this type of test
> isn't really helpful.
Ok, it aint worth much, but it was free ;-)


>  More interesting would be a comparison of the
> contents of /sys/class/hwmon/hwmon0/device before and after the
> patches. If the contents differ, something's wrong, else everything
> should be OK - or at least there is no regression.
>
> Thanks,
>   

Your choice of words leaves it ambiguous whether this is an observation,
or a requirement.  In any case, its easy, so here it is, plus some other 
observations.

by contents, I assume you mean the files within the dir ?

soekris:~# diff -u sys-files-2.6.1*
--- sys-files-2.6.17-ipipe-139-sk       2006-08-21 08:54:36.000000000 -0600
+++ sys-files-2.6.18-rc4-mm1-sk 2006-08-21 09:05:51.000000000 -0600
@@ -49,6 +49,7 @@
 in9_min
 in9_status
 name
+subsystem@
 temp1_crit
 temp1_input
 temp1_max

The files content can change each sample, usually on VPWR, but others too.
diffs are always in small fractions of a volt.

Forex:  these 2 samples show some milli-volt noise,
and these are from a 2.6.17 kernel.

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:     +11.88 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.93 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:       +103 C  (low  =    +0 C, high =  +125 C)
Critical:   +126 C

soekris:/sys/class/hwmon/hwmon0/device# sensors; 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.12 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.28 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:       +102 C  (low  =    +0 C, high =  +125 C)
Critical:   +126 C


One other thing / oddity I note (again on old kernel).
Datestamps on the 'files' is not uniform.

soekris:/sys/class/hwmon/hwmon0/device# ll -tr
total 0
< partly snipped >
lrwxrwxrwx 1 root root    0 Aug 20 23:39 driver -> 
../../../../bus/i2c/drivers/pc87360/
lrwxrwxrwx 1 root root    0 Aug 20 23:39 bus -> ../../../../bus/i2c/
-r--r--r-- 1 root root 4096 Aug 20 23:39 name
-r--r--r-- 1 root root 4096 Aug 20 23:39 alarms_in
-r--r--r-- 1 root root 4096 Aug 20 23:39 alarms_temp
-r--r--r-- 1 root root 4096 Aug 20 23:39 in1_input
-r--r--r-- 1 root root 4096 Aug 20 23:39 in10_input
-r--r--r-- 1 root root 4096 Aug 20 23:39 temp1_input
-r--r--r-- 1 root root 4096 Aug 20 23:39 temp6_input
-rw-r--r-- 1 root root 4096 Aug 20 23:39 vrm
-r--r--r-- 1 root root 4096 Aug 20 23:39 cpu0_vid
-r--r--r-- 1 root root 4096 Aug 21 08:04 temp6_status
-rw-r--r-- 1 root root 4096 Aug 21 08:04 temp6_min
-rw-r--r-- 1 root root 4096 Aug 21 08:04 temp5_min
-r--r--r-- 1 root root 4096 Aug 21 08:04 temp4_status

IOW, there are 2 datestamps : Aug 20 23:39  and Aug 21 08:04

After a reboot (to 2.6.17) shows the same 2 sets of files, this time 
with closer stamps.
-r--r--r-- 1 root root 4096 Aug 21 08:42 temp6_input
-r--r--r-- 1 root root 4096 Aug 21 08:42 cpu0_vid
-rw-r--r-- 1 root root 4096 Aug 21 08:42 vrm
-r--r--r-- 1 root root 4096 Aug 21 08:46 temp6_status
-rw-r--r-- 1 root root 4096 Aug 21 08:46 temp6_max

Also, 8:42 is near the bootup time, but not exactly the same.
# uptime
 08:50:13 up 10 min,  1 user,  load average: 3.00, 2.72, 1.54

rmmod & modprobing recreates the files with current & uniform timestamps.

[lm-sensors] [patch 0/3] pc87360 - fix unchecked

[Jim Cromie]

Jean Delvare wrote:

< Ive dropped Mark from cc, Im sure he's seen enough from me already ;->

> Jim,
>
>   
>> FWIW -
>> The patchset survived this stress-test:
>>      for i in `seq 1000`; do { rmmod pc87360;  modprobe pc87360;  
>> sensors -s; sensors; }  done
>> It takes about 3.5 secs to re-mod, and about 0.3 sec to reset and query 
>> sensors.
>>     
>
> Given that we aren't suspecting a race condition, this type of test
> isn't really helpful.
Ok, it aint worth much, but it was free ;-)


>  More interesting would be a comparison of the
> contents of /sys/class/hwmon/hwmon0/device before and after the
> patches. If the contents differ, something's wrong, else everything
> should be OK - or at least there is no regression.
>
> Thanks,
>   

Your choice of words leaves me unsure whether this is an observation,
or a requirement.  In any case, its easy, so here it is, plus some other 
observations.

By contents, I assume you mean the files within the dir:

 diff sys-files-2.6.1*
51a52
 > subsystem@


The files contents change each sample, usually on VPWR, but others too.
diffs are always in small fractions of a volt.

Forex:  these 2 samples show some milli-volt noise,
and these are from a 2.6.17 kernel.

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:     +11.88 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.93 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:       +103 C  (low  =    +0 C, high =  +125 C)
Critical:   +126 C

soekris:/sys/class/hwmon/hwmon0/device# sensors; 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.12 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.28 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:       +102 C  (low  =    +0 C, high =  +125 C)
Critical:   +126 C


One other thing / oddity I note (again on old kernel, and new/patched).
Datestamps on the 'files' is not uniform.

soekris:/sys/class/hwmon/hwmon0/device# ll -tr
total 0
< partly snipped >
lrwxrwxrwx 1 root root    0 Aug 20 23:39 driver -> 
../../../../bus/i2c/drivers/pc87360/
lrwxrwxrwx 1 root root    0 Aug 20 23:39 bus -> ../../../../bus/i2c/
-r--r--r-- 1 root root 4096 Aug 20 23:39 name
-r--r--r-- 1 root root 4096 Aug 20 23:39 alarms_in
-r--r--r-- 1 root root 4096 Aug 20 23:39 alarms_temp
-r--r--r-- 1 root root 4096 Aug 20 23:39 in1_input
-r--r--r-- 1 root root 4096 Aug 20 23:39 in10_input
-r--r--r-- 1 root root 4096 Aug 20 23:39 temp1_input
-r--r--r-- 1 root root 4096 Aug 20 23:39 temp6_input
-rw-r--r-- 1 root root 4096 Aug 20 23:39 vrm
-r--r--r-- 1 root root 4096 Aug 20 23:39 cpu0_vid
-r--r--r-- 1 root root 4096 Aug 21 08:04 temp6_status
-rw-r--r-- 1 root root 4096 Aug 21 08:04 temp6_min
-rw-r--r-- 1 root root 4096 Aug 21 08:04 temp5_min
-r--r--r-- 1 root root 4096 Aug 21 08:04 temp4_status

IOW, there are 2 datestamps : Aug 20 23:39  and Aug 21 08:04

After a reboot (to 2.6.17) shows the same 2 sets of files, this time 
with closer stamps.
-r--r--r-- 1 root root 4096 Aug 21 08:42 temp6_input
-r--r--r-- 1 root root 4096 Aug 21 08:42 cpu0_vid
-rw-r--r-- 1 root root 4096 Aug 21 08:42 vrm
-r--r--r-- 1 root root 4096 Aug 21 08:46 temp6_status
-rw-r--r-- 1 root root 4096 Aug 21 08:46 temp6_max

Also, 8:42 is near the bootup time, but not exactly the same.
# uptime
 08:50:13 up 10 min,  1 user,  load average: 3.00, 2.72, 1.54

rmmod & modprobing recreates the files with current & uniform timestamps.

[lm-sensors] [patch 0/3] pc87360 - fix unchecked

[Jean Delvare]

Jim,

> > More interesting would be a comparison of the
> > contents of /sys/class/hwmon/hwmon0/device before and after the
> > patches. If the contents differ, something's wrong, else everything
> > should be OK - or at least there is no regression.
> 
> Your choice of words leaves me unsure whether this is an observation,
> or a requirement.  In any case, its easy, so here it is, plus some other 
> observations.

What difference does it make? I can't force you to do it, and I can't
verify by myself. I can only hope that you want to provide good patches
to the community, and will test your work as much as possible.

It's really about what you want to give to us, rather than what I
request or suggest.

> By contents, I assume you mean the files within the dir:

Yes.

>  diff sys-files-2.6.1*
> 51a52
>  > subsystem@

No idea where this comes from, but not from your patch, for sure. I
guess it's something new, I have such links on 2.6.18-rc4, but I don't
remember seeing them before.

If that's the only difference it means your patch didn't omit any file,
so it works as designed for your chip. Good.

> The files contents change each sample, usually on VPWR, but others too.
> diffs are always in small fractions of a volt.

I didn't mean to compare the contents of the files, of course I know
they can change, and this is completely unrelated with your patch, or
any other change for that matter. We wouldn't be monitoring if values
weren't to change every now and then ;)

> One other thing / oddity I note (again on old kernel, and new/patched).
> Datestamps on the 'files' is not uniform.
> 
> soekris:/sys/class/hwmon/hwmon0/device# ll -tr
> total 0
> < partly snipped >
> lrwxrwxrwx 1 root root    0 Aug 20 23:39 driver -> 
> ../../../../bus/i2c/drivers/pc87360/
> lrwxrwxrwx 1 root root    0 Aug 20 23:39 bus -> ../../../../bus/i2c/
> -r--r--r-- 1 root root 4096 Aug 20 23:39 name
> -r--r--r-- 1 root root 4096 Aug 20 23:39 alarms_in
> -r--r--r-- 1 root root 4096 Aug 20 23:39 alarms_temp
> -r--r--r-- 1 root root 4096 Aug 20 23:39 in1_input
> -r--r--r-- 1 root root 4096 Aug 20 23:39 in10_input
> -r--r--r-- 1 root root 4096 Aug 20 23:39 temp1_input
> -r--r--r-- 1 root root 4096 Aug 20 23:39 temp6_input
> -rw-r--r-- 1 root root 4096 Aug 20 23:39 vrm
> -r--r--r-- 1 root root 4096 Aug 20 23:39 cpu0_vid
> -r--r--r-- 1 root root 4096 Aug 21 08:04 temp6_status
> -rw-r--r-- 1 root root 4096 Aug 21 08:04 temp6_min
> -rw-r--r-- 1 root root 4096 Aug 21 08:04 temp5_min
> -r--r--r-- 1 root root 4096 Aug 21 08:04 temp4_status
> 
> IOW, there are 2 datestamps : Aug 20 23:39  and Aug 21 08:04
> 
> After a reboot (to 2.6.17) shows the same 2 sets of files, this time 
> with closer stamps.
> -r--r--r-- 1 root root 4096 Aug 21 08:42 temp6_input
> -r--r--r-- 1 root root 4096 Aug 21 08:42 cpu0_vid
> -rw-r--r-- 1 root root 4096 Aug 21 08:42 vrm
> -r--r--r-- 1 root root 4096 Aug 21 08:46 temp6_status
> -rw-r--r-- 1 root root 4096 Aug 21 08:46 temp6_max

Yeah, I see similar patterns here. Looks like the files start with
their creation time, then the timestamp gets updated when you write
to (always) or read from them (first time only?)

I can't explain it all, but it makes some sense, and I don't see any
serious problem here anyway. Again, your patch can't have anything to
do with this.

> Also, 8:42 is near the bootup time, but not exactly the same.
> # uptime
>  08:50:13 up 10 min,  1 user,  load average: 3.00, 2.72, 1.54

Most probably the time difference is the time it takes before you reach
the init script which loads the pc87360 driver and/or runs sensors -s.

> rmmod & modprobing recreates the files with current & uniform timestamps.

Until you run "sensors -s" and/or "sensors" (and the read value
changes?), I guess.

-- 
Jean Delvare

[lm-sensors] [patch 0/3] pc87360 - fix unchecked

[Jim Cromie]

Jean Delvare wrote:
> Jim,
>
>   
>>> More interesting would be a comparison of the
>>> contents of /sys/class/hwmon/hwmon0/device before and after the
>>> patches. If the contents differ, something's wrong, else everything
>>> should be OK - or at least there is no regression.
>>>       
>> Your choice of words leaves me unsure whether this is an observation,
>> or a requirement.  In any case, its easy, so here it is, plus some other 
>> observations.
>>     
>
> What difference does it make? I can't force you to do it, and I can't
> verify by myself. I can only hope that you want to provide good patches
> to the community, and will test your work as much as possible.
>
>   
It seems I could also have chosen my wording a bit better ;-)

> It's really about what you want to give to us, rather than what I
> request or suggest.
>
>   
>> By contents, I assume you mean the files within the dir:
>>     
>
> Yes.
>
>   
>>  diff sys-files-2.6.1*
>> 51a52
>>  > subsystem@
>>     
>
> No idea where this comes from, but not from your patch, for sure. I
> guess it's something new, I have such links on 2.6.18-rc4, but I don't
> remember seeing them before.
>
> If that's the only difference it means your patch didn't omit any file,
> so it works as designed for your chip. Good.
>
>   


>> One other thing / oddity I note (again on old kernel, and new/patched).
>> Datestamps on the 'files' is not uniform.
>>
>> IOW, there are 2 datestamps : Aug 20 23:39  and Aug 21 08:04
>>
>>     
> Yeah, I see similar patterns here. Looks like the files start with
> their creation time, then the timestamp gets updated when you write
> to (always) or read from them (first time only?)
>
>   
only thing that chgs date is re-modding.
None of these affect the date.
    sensors; sensors -s; cat /sys/class/hwmon/hwmon0/device/*


> I can't explain it all, but it makes some sense, and I don't see any
> serious problem here anyway. Again, your patch can't have anything to
> do with this.
>
>   

ack.  Im also seeing this on laptop running fc4 kernel.


thanks

[lm-sensors] [patch 0/3] pc87360 - fix unchecked

[Jim Cromie]

Jean Delvare wrote:
>> The files contents change each sample, usually on VPWR, but others too.
>> diffs are always in small fractions of a volt.
>>     
>
> I didn't mean to compare the contents of the files, of course I know
> they can change, and this is completely unrelated with your patch, or
> any other change for that matter. We wouldn't be monitoring if values
> weren't to change every now and then ;)
>   


Well, Ive always been slightly bothered by the fluctuations, since its a 
single-board-computer
that is doing little work, has no cpufreq (ie fixed freq), etc.

I suppose the wifi card draws more when transmitting, and hlt instruction
saves CPU power intermittently, and the PSU is not oversized for 
board+wifi-card.

And I guess, if the numbers had looked sufficiently weird you'd have 
noted it
(which is in part why I included it)

thanks
jimc