[lm-sensors] [patch 0/2] hwmon: add a superio_locks coordinator

[lm-sensors] [patch 0/2] hwmon: add a superio_locks coordinator

[Jim Cromie]

this 2 part patch-set adds a superio_locks module to hwmon.
The module makes it simple for a driver using a Super-IO chip
to obtain a shared lock, and thereby coordinate with any other users
of that chip, without knowing who they are.

I went for a simple helper approach:

- provides an array of lock-structs (mod-param sets size),
    which are handed out upon request.

- does *not* search for super-IO ports,
    it expects drivers to know what ports are on the chips theyre 
written for,
    and to request them specifically.
    (forex: pc87360 must be at 2e or 4e)

- supports driver doing the 'search'
    driver makes 1 call to get_superio_gate_any() to:
       check several command-addresses, for several device ids
       save the return value, which is the gate* (locks are in gates)
    assumes data-addr is *always* command-addr + 1

- allocates a lock for 1st requester of a (sio-port,device-id) pair, if 
found
    2nd requester shares that lock.
    ie its a singleton

- doesnt / cannot protect against rogue driver
    (I wouldnt know how)

- drivers are expected to do their own locking, unlocking
   driver chooses efficiency vs long-lock-time

- superio lock structure (gate) is exposed in header
    allow user-drivers to get inside it
    sioaddr and devid are both useful to driver logic.

So, the API:

struct superio_gate {
       struct semaphore sema;  /* lock is here, for no-offset cast */
       int sioaddr;            /* this port's cmd-address */
       int devid;            /* device id found by 1st user */
};
 
struct superio_gate* get_sio_gate(int sioaddr, int devid_addr, int 
devid_val);
struct superio_gate* get_sio_gate_any(u8 sioaddr[], int devid_addr, u8 
devid_val[]);
void sio_lock(struct superio_gate*);
void sio_unlock(struct superio_gate*);
int  sio_inb(struct superio_gate*, int reg);
void sio_outb(struct superio_gate*, int reg, int val );
void sio_exit(struct superio_gate*);


users will do:
    struct superio_gate* gate = get_sio_gate_any( sioaddrs, devid_addr, 
wanted_ids );

and then either:
   superio_lock(gate);
   lock_mutex(&gate->mutex);

Once users have taken the lock, they can use (with some prep in latter 
examples)
   superio_inb(gate->sioaddr);
   superio_inb(mysioaddr);
   sio_inb(gate);
   inb(..)



2nd patch converts pc87360 to use it.   This patch has 2 versions;
    diff.uselocks.alpha-hilock
    diff.uselocks.alpha-lolock

the lolock version puts the lock-unlock pair around the low level routines;
pc87360_write_value, pc87360_read_value.  This is the way to do it
if your priority is short lock times.

the hilock version puts the locking in users of those routines,
which means less locking, but holds the lock much longer,
(thru a full scan of all sensor-attrs)

[lm-sensors] [patch 0/2] hwmon: add a superio_locks coordinator

[Rudolf Marek]

Hello

Sorry for the delay I have been rather busy with the school finishing stuff.
And now my comments. I studied both patches and I really like a simplistic aproach

So far I know sio is used in:

mmc card driver wbsd.c
parallel port drivers parport_pc.c
several watchdog drivers
irda drivers

After studying those drivers and your minimalistic approach in mind I came to this conclusion

what about to create this API:


unlock_seq[] = {0x87,0x87,0x00} maybe 0x00 to terminate the list?

sio_enter(u8 port, u8 *unlock_seq)
sio_exit(u8 port, u8 *lock_seq)
sio_inb(u8 port)
sio_outb(u8 port, u8 val)
sio_select(u8 port, u8 ldn)

Now the big thing ;) the semaphore/mutex for the access locking would be hidden in sio_enter
and sio_exit

there could be just some port-> index mapping so you will try to lock/unlock just a lock to right address
This would solve all problems with concurrent access.

This approach should work and it is consistent with the idea of SIO, just enter the configuration space,
alter something and exit... Just lock it between enter and exit.

The locking in some other places cannot be done because the drivers can switch the bank and this is
not good. They can try to lock/unlock the sio itself. This approach will solve those problems...

What do you say? If you like it maybe you can implement it or come with something else...

Regards
Rudolf

[lm-sensors] [patch 0/2] hwmon: add a superio_locks coordinator

[Jim Cromie]

Rudolf Marek wrote:
> Hello
>
> Sorry for the delay I have been rather busy with the school finishing stuff.
> And now my comments. I studied both patches and I really like a simplistic aproach
>
>   
thanks.  I also just noticed your reply.


> So far I know sio is used in:
>
> mmc card driver wbsd.c
> parallel port drivers parport_pc.c
> several watchdog drivers
> irda drivers
>
> After studying those drivers and your minimalistic approach in mind I came to this conclusion
>
> what about to create this API:
>
>   
How well does this API fit those drivers you surveyed ?
Is it a loose combo of the uses you saw, or a more formal one ?
I did my strawman in a vacuum, and I need to go look at those drivers
(I havent yet, I wanted to answer 1st )  thanks for IDg them

> unlock_seq[] = {0x87,0x87,0x00} maybe 0x00 to terminate the list?
>
>   
I dont get what unlock_seq is for.
> sio_enter(u8 port, u8 *unlock_seq)
> sio_exit(u8 port, u8 *lock_seq)
> sio_inb(u8 port)
> sio_outb(u8 port, u8 val)
> sio_select(u8 port, u8 ldn)
>
>   

Ive also been a little sloppy with sio vs superio names,
(pc87360 has superio_inb, superio_outb, superio_exit
I hope this hasn't confused anything.

I'll infer that you used sio_* as non-conflicting names that the 
mentioned drivers
could easily be patched to use.


> Now the big thing ;) the semaphore/mutex for the access locking would be hidden in sio_enter
> and sio_exit
>
>   
Im not sure hiding them has any value - exposing the locking lets the 
driver do it
for its needs - a driver (forex pc87360) could (not unreasonably) lock 
the sio,
then scan all its sensors, then unlock.
Im not saying thats how pc87360 should do it, just that the driver writer
can reasonably make the max-latency vs speed tradeoff.

you chose enter/exit  as your locking primitives.  This seems less 
obvious than
lock/unlock, I presume your names are taken from their presence in the 
drivers you surveyed.

FWIW, superio_exit in pc87360 looks like this:

static inline void superio_exit(int sioaddr)
{
        outb(0x02, sioaddr);
        outb(0x02, sioaddr+1);
}

I dont know whether that 02 value is something that is chip specific,
or whether its semi-standard in the world of superio-chips.


> there could be just some port-> index mapping so you will try to lock/unlock just a lock to right address
> This would solve all problems with concurrent access.
>
>   
Im not sure what you mean here - It sounds vaguely like what I proposed 
/ intended
with get_sio_gate()


> This approach should work and it is consistent with the idea of SIO, just enter the configuration space,
> alter something and exit... Just lock it between enter and exit.
>
> The locking in some other places cannot be done because the drivers can switch the bank and this is
> not good. They can try to lock/unlock the sio itself. This approach will solve those problems...
>
>   
I dont follow you.   One reason to make the sio_lock/unlock explicit is to
avoid the hassles with bank switching etc -
In diff.use*locks*.alpha-hilock  (patch 2 - flavor 1)
I take the lock once, and all sensor-scan accesses do the bank switching
they need to once, rather than re-doing it each time, just in case someone
else 'simultaneously' used the SIO and changed the bank out from under us.


> What do you say? If you like it maybe you can implement it or come with something else...
>
>   
Thanks for the dialog,  Id like to continue it, and eventually come up 
with a patch.
I have a semi-clear idea what I was proposing, Im less clear on what you 
are thinking.
Could you go back over my bullet items, and comment on each that 
warrants it ?

Forex:
do you like/dislike the struct superio_gate ?
   - I do, the new type gives some manner of type-safety,
    semantic separation from other uses of SIO port

Do you think the gate metaphor works ?
locks themselves dont have addresses, but when installed into doors etc
in your home, they protect stuff there.

do you think the singleton aspect works to coordinate SIO-port using 
drivers ?

Im gonna look at those drivers you mentioned,
to see if I glean the same API that you have.

> Regards
> Rudolf
>
>   


thanks,
jimc