| Message ID | 20120529100619.GA23186@faui49q.informatik.uni-erlangen.de |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2012-05-29 at 12:06 +0200, Christian Dietrich wrote: > Since nw_gpio_lock is a raw_spinlock_t it should be used with the > raw_spinlock_* functions and not the spinlock_* variants. Functionally > this is equivalent at the moment, because the raw_spinlock_t is the > first field of spinlock_t, and therefore &nw_gpio_lock == > &(nw_gpio_lock->rlock). But when other spinlock_t functions use other > field they read and write random memory. Hm, why are we exposing a raw spinlock to drivers? Should we export a helper function (or macro, I suppose) which does the appropriate locking *and* the GPIO operation?
On Tue, 2012-05-29 at 11:11 +0100, David Woodhouse wrote: > On Tue, 2012-05-29 at 12:06 +0200, Christian Dietrich wrote: > > Since nw_gpio_lock is a raw_spinlock_t it should be used with the > > raw_spinlock_* functions and not the spinlock_* variants. Functionally > > this is equivalent at the moment, because the raw_spinlock_t is the > > first field of spinlock_t, and therefore &nw_gpio_lock == > > &(nw_gpio_lock->rlock). But when other spinlock_t functions use other > > field they read and write random memory. > > Hm, why are we exposing a raw spinlock to drivers? > > Should we export a helper function (or macro, I suppose) which does the > appropriate locking *and* the GPIO operation? AFAIR, raw spinlock is the one that will not be turned into a "preemptable" spinlock in the RT tree. E.g., this is needed when dealing with interrupts. And what if not drivers should use them? But this commit message did not explain still (although I requested) _why_ it needs a raw spinlock, which problems it solves? This URL may be useful for Christian: http://who-t.blogspot.com/2009/12/on-commit-messages.html I like that blog-post.
On Tue, 2012-05-29 at 12:06 +0200, Christian Dietrich wrote: > --- a/drivers/char/ds1620.c > +++ b/drivers/char/ds1620.c > @@ -74,21 +74,21 @@ static inline void netwinder_ds1620_reset(void) > > static inline void netwinder_lock(unsigned long *flags) > { > - spin_lock_irqsave(&nw_gpio_lock, *flags); > + raw_spin_lock_irqsave(&nw_gpio_lock, *flags); > } > > static inline void netwinder_unlock(unsigned long *flags) > { > - spin_unlock_irqrestore(&nw_gpio_lock, *flags); > + raw_spin_unlock_irqrestore(&nw_gpio_lock, *flags); > } If you were to make these functions public by shifting them into arch/arm/mach-footbridge/include/mach/hardware.h that would be a lot nicer, and other places wouldn't have to touch the raw spinlock directly. Also... while we're thinking about preemption and netwinder, note that the write enable is valid for only 2ms or so. So all the functions in dc21285.c that you just touched should probably *also* be disabling preemption when they're run on a netwinder, to ensure that that time doesn't expire before they actually get to run.
diff --git a/drivers/char/ds1620.c b/drivers/char/ds1620.c index aab9605..7d86139 100644 --- a/drivers/char/ds1620.c +++ b/drivers/char/ds1620.c @@ -74,21 +74,21 @@ static inline void netwinder_ds1620_reset(void) static inline void netwinder_lock(unsigned long *flags) { - spin_lock_irqsave(&nw_gpio_lock, *flags); + raw_spin_lock_irqsave(&nw_gpio_lock, *flags); } static inline void netwinder_unlock(unsigned long *flags) { - spin_unlock_irqrestore(&nw_gpio_lock, *flags); + raw_spin_unlock_irqrestore(&nw_gpio_lock, *flags); } static inline void netwinder_set_fan(int i) { unsigned long flags; - spin_lock_irqsave(&nw_gpio_lock, flags); + netwinder_lock(&flags); nw_gpio_modify_op(GPIO_FAN, i ? GPIO_FAN : 0); - spin_unlock_irqrestore(&nw_gpio_lock, flags); + netwinder_unlock(&flags); } static inline int netwinder_get_fan(void) diff --git a/drivers/char/nwflash.c b/drivers/char/nwflash.c index d45c334..04e2a94 100644 --- a/drivers/char/nwflash.c +++ b/drivers/char/nwflash.c @@ -617,9 +617,9 @@ static void kick_open(void) * we want to write a bit pattern XXX1 to Xilinx to enable * the write gate, which will be open for about the next 2ms. */ - spin_lock_irqsave(&nw_gpio_lock, flags); + raw_spin_lock_irqsave(&nw_gpio_lock, flags); nw_cpld_modify(CPLD_FLASH_WR_ENABLE, CPLD_FLASH_WR_ENABLE); - spin_unlock_irqrestore(&nw_gpio_lock, flags); + raw_spin_unlock_irqrestore(&nw_gpio_lock, flags); /* * let the ISA bus to catch on... diff --git a/drivers/mtd/maps/dc21285.c b/drivers/mtd/maps/dc21285.c index 080f060..86598a1 100644 --- a/drivers/mtd/maps/dc21285.c +++ b/drivers/mtd/maps/dc21285.c @@ -38,9 +38,9 @@ static void nw_en_write(void) * we want to write a bit pattern XXX1 to Xilinx to enable * the write gate, which will be open for about the next 2ms. */ - spin_lock_irqsave(&nw_gpio_lock, flags); + raw_spin_lock_irqsave(&nw_gpio_lock, flags); nw_cpld_modify(CPLD_FLASH_WR_ENABLE, CPLD_FLASH_WR_ENABLE); - spin_unlock_irqrestore(&nw_gpio_lock, flags); + raw_spin_unlock_irqrestore(&nw_gpio_lock, flags); /* * let the ISA bus to catch on... diff --git a/sound/oss/waveartist.c b/sound/oss/waveartist.c index 24c430f..672af8b 100644 --- a/sound/oss/waveartist.c +++ b/sound/oss/waveartist.c @@ -1482,9 +1482,9 @@ vnc_mute_spkr(wavnc_info *devc) { unsigned long flags; - spin_lock_irqsave(&nw_gpio_lock, flags); + raw_spin_lock_irqsave(&nw_gpio_lock, flags); nw_cpld_modify(CPLD_UNMUTE, devc->spkr_mute_state ? 0 : CPLD_UNMUTE); - spin_unlock_irqrestore(&nw_gpio_lock, flags); + raw_spin_unlock_irqrestore(&nw_gpio_lock, flags); } static void
Since nw_gpio_lock is a raw_spinlock_t it should be used with the raw_spinlock_* functions and not the spinlock_* variants. Functionally this is equivalent at the moment, because the raw_spinlock_t is the first field of spinlock_t, and therefore &nw_gpio_lock == &(nw_gpio_lock->rlock). But when other spinlock_t functions use other field they read and write random memory. Signed-off-by: Christian Dietrich <christian.dietrich@informatik.uni-erlangen.de> --- drivers/char/ds1620.c | 8 ++++---- drivers/char/nwflash.c | 4 ++-- drivers/mtd/maps/dc21285.c | 4 ++-- sound/oss/waveartist.c | 4 ++-- 4 files changed, 10 insertions(+), 10 deletions(-)