Patchwork netwinder: nw_gpio_lock is a raw_spinlock_t

login
register
mail settings
Submitter Christian Dietrich
Date May 29, 2012, 10:06 a.m.
Message ID <20120529100619.GA23186@faui49q.informatik.uni-erlangen.de>
Download mbox | patch
Permalink /patch/161727/
State New
Headers show

Comments

Christian Dietrich - May 29, 2012, 10:06 a.m.
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(-)
David Woodhouse - May 29, 2012, 10:11 a.m.
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?
Artem Bityutskiy - May 29, 2012, 10:24 a.m.
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.
David Woodhouse - May 29, 2012, 10:52 a.m.
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.

Patch

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