Bob Bennett wrote:
On Tue, 2005-06-21 at 15:15, Jim Cromie wrote:
Jim Cromie wrote:
Im working with scx200_gpio driver, and extending it to operate
gpio pins on pc87366 chip, present on the soekris net-4801
the chip has many functional-units, some of which are driven by
i2c/chips/pc87360.c
both drivers have keep their own locks, which prevent other
uses of the same driver from corrupting operations,
but these offer no protection from interference from the other driver.
Is that correct ?
And if so, how do I resolve it ?
1. 3rd module, which exports a single lock
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.
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.
+#define SHARED_LOCKS 1
+
+#if SHARED_LOCKS
+extern struct semaphore sio_lock;
+extern struct semaphore sio_update_lock;
+
+#define down_lock down(&sio_lock)
+#define down_updatelock down(&sio_update_lock)
+#define up_lock up(&sio_lock)
+#define up_updatelock up(&sio_update_lock)
+
+#else
+#define down_lock down(&(data->lock))
+#define down_updatelock down(&(data->update_lock))
+#define up_lock up(&(data->lock))
+#define up_updatelock up(&(data->update_lock))
+#endif
I do not believe it is recommended to define macros that do specific
function calls like this, as the resulting source code is unclear as
to what exactly is being locked without looking back at the macro
definition. I believe the correct method is to include the function
call.
That was the simplest / clearest way for a sanity test.
Will extract/isolate the ifdefs as you suggest before submitting,
assuming that make sense.
Thanks again
jimc
--
Kernelnewbies: Help each other learn about the Linux kernel.
Archive: http://mail.nl.linux.org/kernelnewbies/
FAQ: http://kernelnewbies.org/faq/
