Bob Bennett wrote:
On Tue, 2005-06-21 at 16:07, Jim Cromie wrote:
Bob Bennett wrote:
On Tue, 2005-06-21 at 15:15, Jim Cromie wrote:
Jim Cromie wrote:
...
You shouldn't need a 3rd module just to contain the exported
semaphores. We need to think about what we are protecting with the
locks. Locks are used to protect shared data. When you replace a
semaphore that is embedded in a structure with a global semaphore,
what are you protecting? What was the problem with how it was
implemented before? Maybe I'm missing something.
Bob
Thanks for reading Bob,
the 3rd module is there because I ahve another module (in the works)
which also uses it.
pc87366_gpio operates the GPIO pins on the chip (duh),
which dont really belong in the i2c driver.
Now I get it (I think). You are protecting access to the gpio pins on
the chip, which will be read/written to from more than one driver?
Not so much the GPIO pins as all the units on the chip.
I'll clarify now the way I probly should have last time.
pc87360 and its improved version, 66, are SuperIO devices (hence sio).
The 66 contains multiple (15) functional units,
+struct pc87366_unit pc87366_units[] = {the
+ { 8, "Floppy Disk Controller (FDC)" },
+ { 8, "Parallel Port (PP)" }, // also 6 more at 0x400
+ { 8, "Serial Port 2 with IR (SP2)" },
+ { 8, "Serial Port 1 (SP1)" },
+ { 0x10, "System Wake-Up Control (SWC)" },
+ { 0x10, "Keyboard and Mouse Controller (KBC) - Mouse interface" },
+ { 0x10, "Keyboard and Mouse Controller (KBC) - Keyboard interface" },
+ { 0x0c, "General-Purpose I/O (GPIO) Ports" },
+ { 6, "ACCESS.bus Interface (ACB)" },
+ { 15, "Fan Speed Control and Monitor (FSCM)" },
+ { 4, "WATCHDOG Timer (WDT)" },
+ { 0x0d, "Game Port (GMP)" },
+ { 3, "Musical Instrument Digital Interface (MIDI) Port" },
+ { 0x10, "Voltage Level Monitor (VLM)" },
+ { 0x0f, "Temperature Sensor (TMS)" },
+ { 0, NULL }
+};
which, you can see, are unrelated except that theyre all good to have in
an SBC PC.
And that theyre resident on a single chip, with one control/config
interface.
the i2c/chips/pc87360 driver operates several of them, VLM, TMS, ACB
and the GPIO units are architecturally similar to those in the
Geode-SC1100 integrated CPU, and can be operated via
the scx200_gpio module.
the sio port on the chip is a slightly odd beast - it uses 2 8 bit ports,
a comand-reg and a data-reg at 2e, 2f, to access a 256 address config-space.
The config-space is further segmented; 60h-ffh are paged, 1 block for
each of the 15 units,
with a unit/device select-register in a low-addy byte doing the paging.
The GPIO pins are further 'paged' with a pin-select register within the
GPIO page.
So there are multiple IO-bus accesses needed to accomplish anything,
and without proper locking, 2-N separate modules, controlling multiple
units,
could stomp on each others work.
One other thing the separate module allows - storing of the last
command-reg written
in a static variable. With this and the locking, the module could
(under paranoid mode)
sound the alarms if any of the shadow-regs dont match the real ones, ie
the chip has been
used by another module, which is potentially vulnerable to corruption.
In
that case, putting the semaphores in a 3rd module would be a viable
(though messy) solution. Alternatively, depending on how frequently you
are accessing the pins, you could use kernel_lock()/kernel_unlock() for
the critical sections of code that access the pins, but that is also
messy, since it can effect other code not related to your driver, and
use of this lock is generally avoided.
BKL ? the lm-sensors module provides a working module - Im not gonna
propose
a switch. Too many battle scars from poorly-conceived/argued ideas in
my past lives.
If you are going to export those semaphores from a module for sharing
with more than one driver, it is probably a good idea to give them more
descriptive names, such as "pc87366_gpio_pin_lock" as opposed to
"sio_lock".
Ack.
the locks are global to make them available to both modules which use them.
Only other alternative is to have one depend upon the other (which is
workable,
but having a char-driver dependent upon i2c seems too much, and vice versa
seems untenable unless the new module is simple/trivial (like this one) )
More comments below..
+
+//static DEFINE_SPINLOCK(sio_lock);
+
+struct semaphore sio_lock;
+struct semaphore sio_update_lock;
+
semaphore structs are not initialized. should use
DECLARE_MUTEX(sio_lock) to declare and initialize.
OK. I wasnt sure about the equivalence - even though theyre in the same .h,
theyre not named the same mutex vs semaphore.
A mutex is a special case of a semaphore in which the usage count is 1
(the number of simultaneous lock holders allowed). A question that
comes to my mind is, do we need to have separate locks for both read
(lock) and update (update_lock).
That choice (>1 lock) was made by i2c/chips/pc87360, Im just respecting
that choice,
since I dont know better.
But I have a plausible explanation - the chip has a config interface,
via the sio ports,
and a runtime interface, on IO addresses that its programmed to respond
to via the
config interface. They could be ioremap'd into memory space, though I
havent done
so. In my case the SBC's BIOS sets the GPIO up on 0x6600
Is it really necessary for a task to
have exclusive access when simply reading the pin? If not, we could
replace both locks with a reader-writer semaphore, and do down_read to
read the value and down_write to update it.
Id certainly entertain that notion, but am not ready to propose it to
lm-sensors.
I'll read up on it though, thanks for the explanation.
BTW, whats the macro incantation for a read-write lock ? DECLARE_* ??
Bob
again, thanks for answering.
jimc
PS. Its beyond the scope of this thread, and probably beyond the kenn
of this
mailing list (the newbies at least0, but what about a
intermodule-shared-lock
service module, to fill the gap between a one-off shared-lock module
like the above,
and a full-blown cluster-ready Distributed lock manager
IE,
spinlock_t* get_or_create_spinlock_by_name (char * lock_name);
etc...
which are referenced at module-load time, and used normally thereafter
to mediate access
--
Kernelnewbies: Help each other learn about the Linux kernel.
Archive: http://mail.nl.linux.org/kernelnewbies/
FAQ: http://kernelnewbies.org/faq/
