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? 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. 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". > 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). 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. Bob -- Kernelnewbies: Help each other learn about the Linux kernel. Archive: http://mail.nl.linux.org/kernelnewbies/ FAQ: http://kernelnewbies.org/faq/
