Re: [lm-sensors] [ patch 0/4 RFC for 2.6.23-rc0 ] superio-locks

Re: [lm-sensors] [ patch 0/4 RFC for 2.6.23-rc0 ] superio-locks

[Hans de Goede]

Hi All,

Jim Cromie <jim.cromie at gmail.com> Wrote:

 > this patch set
 >
 > 01 - adds superio_locks -
 >
 > drivers needing superio access register, and get a 'slot'with port-id
 > info and a mutex,
 > multiple drivers share the same 'slot', and coordinate access with its
 > mutex.
 >
 >     superio_find()     - finds and reserves the slot, returned as ptr or
null
 >     superio_release()  - relinguishes the slot (ref-counted)
 >
 > Once theyve got the reservation in struct superio * gate (as done in
 > patches 2,3,4)
 > they *may* use
 >
 >     superio_lock(gate)
 >     superio_inb(gate, addr),
 >     superio_outb(gate, addr, val)
 >
 > or they can do it themselves with inb/outb, and gate->sioaddr, etc.
 > My objective was to be as
 >
 > superio_find was chosen to fit the idiom used in hwmon/*.c

I noticed on Mark's kernel todo list that this patch needed someone to review 
it. But first lets discuss the API for this, I'm hoping for some input from 
others (Jean, Mark) here, once the API is ok-ed I can review this.

Personally I would like to see the following changes to the API:

1) Make the cmdreg_addrs and device_ids members of the superio_search struct 
fixed size arrays instead of pointers, using [2] for cmdreg_addrs and [8] for 
device_ids, stopping at a 0 addr/id or end of array.

This way code like this:

static u16 cmd_addrs[] = { 0x2E, 0x4E, 0 };
static u16 devid_vals[] = { 0xE1, 0xE8, 0xE4, 0xE5, 0xE9, 0 };

static struct superio_search where = {
	.cmdreg_addrs = cmd_addrs,
	.device_ids   = devid_vals,
	.devid_addr   = SIO_DEVID,
	.devid_mask   = 0,
	.devid_word   = 0,
};

Can be changed into:

static struct superio_search where = {
	.cmdreg_addrs = { 0x2E, 0x4E},
	.device_ids   = { 0xE1, 0xE8, 0xE4, 0xE5, 0xE9, 0 },
	.devid_addr   = SIO_DEVID,
	.devid_mask   = 0,
	.devid_word   = 0,
};

Also the superio_search structs in the using drivers should be declared on the 
stack as they are not needed anymore after the superio_find call.

2) make superio_find return the device_id which was found through a by 
reference param, this way drivers who support multiple id's do not need to read 
the device_id registers again, but can use the returned value.

3) move superio_select from the drivers to the superio code.


 > CAVEATS
 > - see 4
Explain?

 > - Ive not thought much about the 16-bit device-id check
     ( I need hardware to sharpen this focus )
I can check that on / with the f71882fg

 > - superio_get() feels clumsy.
It looks fine to me

 > - superio_{enter,exit} are no-ops on my natsemi chip, so again the code
 > is {}
 >
 > the prob is (of course) that every chip does it differently,
 > and superio_locks shouldnt care.  Probably best to drop them
 > from superio-locks API, and expect drivers to do them themselves.

I think its fine to leave any additional handshaking to the drivers, besides 
that, as you say with this locking that won't be necessary anymore.

 > - no device knowledge (digression - for later)
 >
 > its tempting to (create another module) to have a Logical-device table,
 > with strings to describe the LDs, etc.  I dont want to dwell on it now,
 > except to avoid foreclosing design options by bad choices now (in
 > superio_locks).
 >
 > struct sio_logical_device pc87366_units[] = {
 >        { 8,    "Floppy Disk Controller (FDC)" },
 >        { 8,    "Parallel Port (PP)" },         /* also 6 more at 0x400 */
 >        { 8,    "Serial Port 2 with IR (SP2)" },
 >       ...
 >
 > I suspect that a pointer field in struct superio could
 > carry whatever info a notional pc8736x_sio.ko would need,
 > but might pose some reclamation issues.
 >
 > such a module should support interrogating the LD's config-regs,
 > extracting ISA addresses, etc, and could for example, add a bit
 > more detail for display in /proc/ioports, which currently shows:

Why all this trouble ? Remember most superio devices are already in 
/proc/ioports with proper descriptions through there resp. drivers, for example 
floppy, serial, parallel_pc, kbd, etc.

I can see added value in a get superio_get_base_address(gate, logical_device, 
base_address_no) function, but I think more then that is more trouble then its 
worth. Its important here to realize that most superio logical devices are 
being driven by legacy drivers and that those drivers do not care (and should 
not need to care) wether the legacy device is in an lpc connected superio, or 
truely on the isa bus.

Regards,

Hans




_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [ patch 0/4 RFC for 2.6.23-rc0 ] superio-locks

[Hans de Goede]

Jim Cromie wrote:
> Hans de Goede wrote:
>> Jim Cromie wrote:
>>> Hans de Goede wrote:
>>>
>>> Hi Hans,
>>>
>>> thanks for stepping up.
>>>
>>
>> Your welcome
>>
>>>> 2) make superio_find return the device_id which was found through a 
>>>> by reference param, this way drivers who support multiple id's do 
>>>> not need to read the device_id registers again, but can use the 
>>>> returned value.
>>>>
>>>
>>> No - devid is available via gate->devid     (modulo spelling)
>>> will provide inline wrapper
>>>
>>
>> Ah I missed that because your chip driver patches didn't use it, 
>> instead the chip drivers read the devid registers again to get the dev 
>> register.
>>
>>>> 3) move superio_select from the drivers to the superio code.
>>>>
>>> disagreed at 1st, but added ld_addr field to superio_search,
>>> and copied it into gate in superio_find.
>>>
>>
>> Why add a ld_addr there? That address is defined in the lpc spec 
>> afaik, then again so is the devid address, yet some devices deviate. I 
>> have a good idea, make these both configurable through the search 
>> struct, but allow them to be 0, and in case of 0 use the default 
>> addresses from the lpc spec (which 90% of all drivers use atm afaik).
>>
> I was unaware that its standard.  I'll go with your idea 0 -> default.
> 
>>>> > - Ive not thought much about the 16-bit device-id check
>>>>     ( I need hardware to sharpen this focus )
>>>> I can check that on / with the f71882fg
>>>>
>>> I'll leave that patch to you - as an API clarity test.
>>> If the API is confusing to you (non-newbie), then its flawed.
>>>
>>
>> Will do.
>>
>>> And after I get rev 3 out ( Im calling last one rev2 - postfacto),
>>> I'll review your driver.  Can you send me the datasheet ?
>>>
>>
>> Thanks for the review offer, however I already have a reviewer :)
>> The datasheet is publicly available, see the link from the devices 
>> section on the wiki.
>>
>>>> > - superio_{enter,exit} are no-ops on my natsemi chip, so again the 
>>>> code
>>>> > is {}
>>>> >
>>>> > the prob is (of course) that every chip does it differently,
>>>> > and superio_locks shouldnt care.  Probably best to drop them
>>>> > from superio-locks API, and expect drivers to do them themselves.
>>>>
>>>> I think its fine to leave any additional handshaking to the drivers, 
>>>> besides that, as you say with this locking that won't be necessary 
>>>> anymore.
>>>>
>>> my reasoning as commented in superio_locks.h is questionable.
>>> enter/exit is orthogonal to mutex, it gives no protection to drivers
>>> that properly use enter/exit.
>>>
>>> drivers could reasonably just enter, never exit (since theres no 
>>> protection)
>>> mutexs must be used to give transaction guarantees.
>>>
>>> Maybe more to say later..
>>>
>>
>>
>> I cannot follow you here, what exactly do you mean with enter / exit, 
>> some kind ahandshaking between different users which can be done 
>> through the chip, like testing the highest bit of the index reg? That 
>> will only work if you shortly disable any and all taskswitching as you 
>> first need to read it and then set it and you cannot do this atomically .
>>
> 
> superio_enter/exit - theyre write sequences that enable/disable the 
> superio-port
> (the isa port still works)  I guess it protects against random access and
> wrong drivers ( driver for Y dont know how to unlock chip X)
> 

Ah, you mean the unlock code to "unlock" the superio ports, thats indeed to 
stop software who accidentally access these ports to wreck havoc, this offers 
no protection against a gpio versus hwmom driver conflict though, where both 
drivers know howto unlock.

You could try to integrate this enter / exit stuff into the locking though, but 
I'm not sure if we want this, we should first take a look to see when drivers 
open up the io ports by sending the key sequence, and when the close them again 
by sending the closing key, if this matches with moments when a drivers should 
call superio_enter and superio_exit, then we can make superio_enter and exit do 
the sending of the enter and leave codes, this would require adding these codes 
to the gate structure though,

Regards,

Hans

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [ patch 0/4 RFC for 2.6.23-rc0 ] superio-locks

[Jim Cromie]

Hans de Goede wrote:

Hi Hans,

thanks for stepping up.


> Personally I would like to see the following changes to the API:
>
> 1) Make the cmdreg_addrs and device_ids members of the superio_search 
> struct fixed size arrays instead of pointers, using [2] for 
> cmdreg_addrs and [8] for device_ids, stopping at a 0 addr/id or end of 
> array.
>

Yes - completely sensible - Doing it.


> Also the superio_search structs in the using drivers should be 
> declared on the stack as they are not needed anymore after the 
> superio_find call.
>
This too - Doing so.

> 2) make superio_find return the device_id which was found through a by 
> reference param, this way drivers who support multiple id's do not 
> need to read the device_id registers again, but can use the returned 
> value.
>

No - devid is available via gate->devid     (modulo spelling)
will provide inline wrapper

> 3) move superio_select from the drivers to the superio code.
>
disagreed at 1st, but added ld_addr field to superio_search,
and copied it into gate in superio_find.

>
> > CAVEATS
> > - see 4
> Explain?
>
Basic hedge since only 1 client driver tested with it.
All others compile-tested only, and just barely.

Now have 2nd chip, which has real enter/exit, so next version
will be known to work with both. 

Other drivers still rather sloppy ATM ..


> > - Ive not thought much about the 16-bit device-id check
>     ( I need hardware to sharpen this focus )
> I can check that on / with the f71882fg
>
I'll leave that patch to you - as an API clarity test.
If the API is confusing to you (non-newbie), then its flawed.

And after I get rev 3 out ( Im calling last one rev2 - postfacto),
I'll review your driver.  Can you send me the datasheet ?

> > - superio_get() feels clumsy.
> It looks fine to me
>
> > - superio_{enter,exit} are no-ops on my natsemi chip, so again the code
> > is {}
> >
> > the prob is (of course) that every chip does it differently,
> > and superio_locks shouldnt care.  Probably best to drop them
> > from superio-locks API, and expect drivers to do them themselves.
>
> I think its fine to leave any additional handshaking to the drivers, 
> besides that, as you say with this locking that won't be necessary 
> anymore.
>
my reasoning as commented in superio_locks.h is questionable.
enter/exit is orthogonal to mutex, it gives no protection to drivers
that properly use enter/exit.

drivers could reasonably just enter, never exit (since theres no protection)
mutexs must be used to give transaction guarantees.

Maybe more to say later..

> > - no device knowledge (digression - for later)
> >
> > its tempting to (create another module) to have a Logical-device table,
> > with strings to describe the LDs, etc.  I dont want to dwell on it now,
> > except to avoid foreclosing design options by bad choices now (in
> > superio_locks).
> >
> > struct sio_logical_device pc87366_units[] = {
> >        { 8,    "Floppy Disk Controller (FDC)" },
> >        { 8,    "Parallel Port (PP)" },         /* also 6 more at 
> 0x400 */
> >        { 8,    "Serial Port 2 with IR (SP2)" },
> >       ...
> >
> > I suspect that a pointer field in struct superio could
> > carry whatever info a notional pc8736x_sio.ko would need,
> > but might pose some reclamation issues.
> >
> > such a module should support interrogating the LD's config-regs,
> > extracting ISA addresses, etc, and could for example, add a bit
> > more detail for display in /proc/ioports, which currently shows:
>
> Why all this trouble ? Remember most superio devices are already in 
> /proc/ioports with proper descriptions through there resp. drivers, 
> for example floppy, serial, parallel_pc, kbd, etc.
>

it would be nice to see the LD-name appended to these lines

6620-662f : pc87360
6640-664f : pc87360


> I can see added value in a get superio_get_base_address(gate, 
> logical_device, base_address_no) function, but I think more then that 
> is more trouble then its worth. 

Probably true.  ATM I just want to avoid painting myself into a corner.

maybe superio_get_isa_baseaddr is worth adding to superio_locks,
subject to what (new) fields such support requires..

<aside> s/then/than/  
since English is your 2nd or 3rd language, it might not just be a typo.

> Its important here to realize that most superio logical devices are 
> being driven by legacy drivers and that those drivers do not care (and 
> should not need to care) wether the legacy device is in an lpc 
> connected superio, or truely on the isa bus.
>

> Regards,
>
> Hans
>
>
>
>
thanks again.
-jimc

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

[lm-sensors] [ patch 0/4 RFC for 2.6.23-rc0 ] superio-locks

[Jim Cromie]

this patch set

01 - adds superio_locks -

drivers needing superio access register, and get a 'slot'with port-id 
info and a mutex,
multiple drivers share the same 'slot', and coordinate access with its 
mutex.

    superio_find()     - finds and reserves the slot, returned as ptr or 
null
    superio_release()  - relinguishes the slot (ref-counted)

Once theyve got the reservation in struct superio * gate (as done in 
patches 2,3,4)
they *may* use

    superio_lock(gate)
    superio_inb(gate, addr),
    superio_outb(gate, addr, val)

or they can do it themselves with inb/outb, and gate->sioaddr, etc.
My objective was to be as

superio_find was chosen to fit the idiom used in hwmon/*.c


02 - use it in pc8736x_gpio

this driver keeps the slot for the lifetime of the module ( __init til 
__exit )


03 - uses it in  pc87360

this driver keeps the slot for only during __init, since it only needs to
read the ISA addresses of the Logical Devices in the chip.


04 - use it in rest of drivers/hwmon/*.c

this patch is compile-tested only, please review for sanity before you 
try running them.


OPERATION

Here they are, working on my soekris box
 - with #define DEBUG 1,  and  sysctl -w kernel.printk=8

simple sharing:

soekris:~# modprobe pc8736x_gpio
[  201.308543] platform pc8736x_gpio.0: NatSemi pc8736x GPIO Driver 
Initializing
[  201.319118] got e9 want e1
[  201.322297] no devid:e1 at port:2e
[  201.325996] got e9 want e8
[  201.328770] no devid:e8 at port:2e
[  201.332375] got e9 want e4
[  201.335259] no devid:e4 at port:2e
[  201.338847] got e9 want e5
[  201.341728] no devid:e5 at port:2e
[  201.349112] allocating slot 0, addr 2e for device e9
[  201.354345] found devid:e9 port:2e users:1
[  201.371547] platform pc8736x_gpio.0: GPIO ioport 6600 reserved
soekris:~#
soekris:~# modprobe pc87360
[  201.757736] got e9 want e1
[  201.760694] no devid:e1 at port:2e
[  201.764317] got e9 want e8
[  201.767263] no devid:e8 at port:2e
[  201.770899] got e9 want e4
[  201.773669] no devid:e4 at port:2e
[  201.777248] got e9 want e5
[  201.780135] no devid:e5 at port:2e
[  201.783731] sharing port:2e dev:e9 users:2
[  201.788015] found devid:e9 port:2e users:2
[  201.806848] pc87360: Device 0x09 not activated
[  201.827262] pc87360 pc87360.26144: VLM conversion set to 1s period, 
160us delay
soekris:~#



soekris:~# rmmod pc8736x_gpio
[  206.934821] releasing last user of superio-port 2e
soekris:~# rmmod pc87360    

I removed both, but the slot released after the 1st,
because the other module released it at end of init,
and is no longer holding the reservation


soekris:~# modprobe pc87360
[  208.173676] got e9 want e1
[  208.176483] no devid:e1 at port:2e
[  208.180115] got e9 want e8
[  208.183028] no devid:e8 at port:2e
[  208.186625] got e9 want e4
[  208.189515] no devid:e4 at port:2e
[  208.192986] got e9 want e5
[  208.195865] no devid:e5 at port:2e
[  208.203700] allocating slot 0, addr 2e for device e9
[  208.209125] found devid:e9 port:2e users:1
[  208.214555] pc87360: Device 0x09 not activated
[  208.219853] releasing last user of superio-port 2e
[  208.252041] pc87360 pc87360.26144: VLM conversion set to 1s period, 
160us delay
soekris:~#

again, can see init-only usage by driver, and last-release message.


soekris:~# modprobe pc8736x_gpio
[  208.633842] platform pc8736x_gpio.0: NatSemi pc8736x GPIO Driver 
Initializing
[  208.643221] got e9 want e1
[  208.646017] no devid:e1 at port:2e
[  208.649634] got e9 want e8
[  208.652566] no devid:e8 at port:2e
[  208.656161] got e9 want e4
[  208.659108] no devid:e4 at port:2e
[  208.662584] got e9 want e5
[  208.665457] no devid:e5 at port:2e
[  208.680637] allocating slot 0, addr 2e for device e9
[  208.685846] found devid:e9 port:2e users:1
[  208.693420] platform pc8736x_gpio.0: GPIO ioport 6600 reserved
soekris:~#

can see here that we're the only holder of that reservation.



CAVEATS

- see 4
- Ive not thought much about the 16-bit device-id check
    ( I need hardware to sharpen this focus )
- superio_get() feels clumsy.

- superio_{enter,exit} are no-ops on my natsemi chip, so again the code 
is {}

the prob is (of course) that every chip does it differently,
and superio_locks shouldnt care.  Probably best to drop them
from superio-locks API, and expect drivers to do them themselves.

Callbacks are possible but pointless, but given that the driver author
must call enter & exit explicitly, they should just call their function 
directly.

- no device knowledge (digression - for later)

its tempting to (create another module) to have a Logical-device table,
with strings to describe the LDs, etc.  I dont want to dwell on it now,
except to avoid foreclosing design options by bad choices now (in 
superio_locks).

struct sio_logical_device pc87366_units[] = {
        { 8,    "Floppy Disk Controller (FDC)" },
        { 8,    "Parallel Port (PP)" },         /* also 6 more at 0x400 */
        { 8,    "Serial Port 2 with IR (SP2)" },
       ...

I suspect that a pointer field in struct superio could
carry whatever info a notional pc8736x_sio.ko would need,
but might pose some reclamation issues.

such a module should support interrogating the LD's config-regs,
extracting ISA addresses, etc, and could for example, add a bit
more detail for display in /proc/ioports, which currently shows:

6008-600d : NatSemi SCx200 High-Resolution Timer
6100-613f : 0000:00:12.0
  6100-612b : NatSemi SCx200 GPIO
6200-623f : 0000:00:12.0
6300-63ff : 0000:00:12.1
6500-653f : 0000:00:12.5
6600-660f : pc8736x_gpio
6620-662f : pc87360
6640-664f : pc87360
  6640-664f : pc87360

I suspect that the multiple pc87360 regions are for the LDs controlled
by hwmon/pc87360, since pc8736x_gpio also shows up (once).
It would be cool if they said which ldn, something like

6620-662f : pc87360.voltages
6640-664f : pc87360.temps
  6640-664f : pc87360.dunno

<aside> Does it look 'right' to you ?


- superio_locks may be relevant elsewhere (ACPI ?) but I havent thought 
about them


WHATS NEXT

- see if any drivers in patch 4 are sane/insane
    probably break 4 into separate patches ..

- get the feedback --> fix the code
- submit formally
     here ? LKML ? other subsystem MLs ?


thanks,
-jimc

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors