[lm-sensors] superio lock coordinator

[Jean Delvare]

Hi Jim,

On Tue, 15 Jul 2008 01:56:00 -0600, Jim Cromie wrote:
> these are on my current todo / partly done
> 
> - struct superio_common - stuff thats copied from struct superio-search 
> to struct superio when reservation is taken
> 
> - add a u8 flags to struct superio
> -- carries bit for tracking enter/exit state -
>     should support warnings on situations like enter-on-entered, 
> exit-on-exited
>  - could carry a toggle bit for a union superio_exit { fnptr; 
> exit_seq[X] } or somesuch.
> 
> -- following another thread - we'd
> --- reserve region when reservation taken, after devid is validated,
>     might help keep other semi-rogue drivers out.

Actually, _before_ devid is validated. You shouldn't make _any_ I/O
access without first requesting the region. You can always release the
region afterwards if you didn't find anything interesting.

> 
> = sort out enter / exit
> 
> As David noted in this thread, the ehf driver has an odd exit sequence,
> 
> The combined combined superio_exit() function I have coded currently is 
> really quite hideous,
> and has me thinking towards  David's function-pointer idea.
> 
> - the other alternative is that David's driver should include its own 
> custom exit-fn, which
> he would use instead of the regular superio_exit().  It could use the 
> superio_enter routine as-is iirc.

Please read my reply to David's concern, and why having a per-chip exit
sequence doesn't make that much sense. I would use the same exit
function as isadump and sensors-detect have been using for months, as
this appears to work fine.

> my stall points - (fishing for answers;)
> 
> - enter / exit should be tracked by superio-locks, in flags bit.   
> (recent idea, seems sound)
> - enter / exit should be vaguely discouraged in kernel-doc - due to 
> expectations of isadump etc.

Which expectations? User-space should stay away from I/O ports the
kernel has requested. There's little point in trying to sort out
concurrent accesses which simply shouldn't happen in the first place.

> 
> 
> I guess I gotta stop overcomplicating things, and get busy on the patch-set.
> 
> 
> > More interesting questions IMHO are:
> > 1) is hwmon the correct subsystem to put the .c file in (somewhat 
> > superfical
> >   really, but we need a place to put the lock coordinator (and enable 
> > / disable
> >   its building).
> 
> I put it here for proximity to the center-of-mass of the drivers I found.

That's not necessarily a bad initial location. We can have superio live
in drivers/hwmon at first, and once all hwmon drivers use it, move it
to a neutral ground and convert the other drivers in the kernel tree
(mainly watchdog drivers I think.)

> > 2) non hwmon drivers need to be edited for superio register use too, 
> > and then
> >   modified to use the lock coordinator if they touch the superio 
> > registers.
> >   This will also be a good exercise to see if our API is generic enough.
> >
> I guess there are drivers I missed ?

Best way I think is to grep for each super-I/O chip name.

> thanks again Hans for pulling me back into this, I'll commit to staying 
> with it now.
> Should I infer from your comments that the code at the link incorporates 
> your feedback ?
> They seem to have aquired some momentum recently 8-)
> 
> Jean, you raised the idea of a sysfs interface.
> Does the October patchset look to fit with what youre thinking ?

I don't think your patch offered a sysfs interface? I'm not really
thinking of anything in particular anyway. We just want a way for
sensors-detect to identify supported Super-I/O chips without actually
accessing the I/O ports if a kernel driver already requested them.

> - the most simple I can think of :
> --expose the kzalloced array of superio-fields as readonly attributes.
> --values go non-zero when superio-port is registered by some user-driver
> --it would be nice to see client-drivers list on each slot, but that 
> involves more code and mem

What we really need for sensors-detect is simply the value of registers
0x20 and 0x21, and values of registers 0x30, 0x60 and 0x61 for a
selected logical device. In what form we export these, needs to be
discussed.

> David,
> do you have a preference wrt fn-ptr ? and if so, a single fn with 1 arg 
> :  0,1,2 for enter/exit/query ?
> do you have any use for a flag bit ?  is there generality in it ?
> 
> does anyone think s/superio/sio/g is appropriate ?

No. This was discussed before and I have always objected to the name
"sio". It's too short and could mean about anything (starting with
"synchronous I/O", as opposed to "asynchronous I/O" which is very often
spelled AIO.)

> anything else ?

-- 
Jean Delvare