[PATCH] spi-gpio: Sanitize MISO bitvalue

[PATCH] spi-gpio: Sanitize MISO bitvalue

[Michael Buesch]

gpio_get_value() returns 0 or nonzero, but getmiso() expects 0 or 1.
Sanitize the value to a 0/1 boolean.

Signed-off-by: Michael Buesch <mb@bu3sch.de>

---

Well, we could also change the bitbang helpers in linux/spi/spi_bitbang.h
or change the way the gpio_get_value API is defined, but I personally think
this patch is pretty good as is.
In any case, it fixes a real bug on platforms like the bcm47xx which
return 0 or nonzero for gpio_get_value.

Index: linux-2.6/drivers/spi/spi_gpio.c
===================================================================
--- linux-2.6.orig/drivers/spi/spi_gpio.c	2009-02-14 21:37:14.000000000 +0100
+++ linux-2.6/drivers/spi/spi_gpio.c	2009-02-15 16:27:16.000000000 +0100
@@ -114,7 +114,7 @@ static inline void setmosi(const struct 
 
 static inline int getmiso(const struct spi_device *spi)
 {
-	return gpio_get_value(SPI_MISO_GPIO);
+	return !!gpio_get_value(SPI_MISO_GPIO);
 }
 
 #undef pdata

-- 
Greetings, Michael.

Re: [PATCH] spi-gpio: Sanitize MISO bitvalue

[David Brownell]

On Sunday 15 February 2009, Michael Buesch wrote:
> gpio_get_value() returns 0 or nonzero, but getmiso() expects 0 or 1.
> Sanitize the value to a 0/1 boolean.
> 
> Signed-off-by: Michael Buesch <mb@bu3sch.de>

Acked-by: David Brownell <dbrownell@users.sourceforge.net>


> ---
> 
> Well, we could also change the bitbang helpers in linux/spi/spi_bitbang.h

That would be the main alternate fix to consider, and
it's worth updating the docs there to specify that getmiso()
needs to return 0/1 ... 

> or change the way the gpio_get_value API is defined,

Not an option.  The more instructions thrown in these
inner loops, the harder it is to get even inlined code to
clock over 1 MHz.


> but I personally think 
> this patch is pretty good as is.
> In any case, it fixes a real bug on platforms like the bcm47xx which
> return 0 or nonzero for gpio_get_value.
> 
> Index: linux-2.6/drivers/spi/spi_gpio.c
> ===================================================================
> --- linux-2.6.orig/drivers/spi/spi_gpio.c	2009-02-14 21:37:14.000000000 +0100
> +++ linux-2.6/drivers/spi/spi_gpio.c	2009-02-15 16:27:16.000000000 +0100
> @@ -114,7 +114,7 @@ static inline void setmosi(const struct 
>  
>  static inline int getmiso(const struct spi_device *spi)
>  {
> -	return gpio_get_value(SPI_MISO_GPIO);
> +	return !!gpio_get_value(SPI_MISO_GPIO);
>  }
>  
>  #undef pdata
> 
> -- 
> Greetings, Michael.
> 
> 

Re: [PATCH] spi-gpio: Sanitize MISO bitvalue

[Andrew Morton]

On Sun, 15 Feb 2009 16:30:41 +0100
Michael Buesch <mb@bu3sch.de> wrote:

> gpio_get_value() returns 0 or nonzero, but getmiso() expects 0 or 1.
> Sanitize the value to a 0/1 boolean.
> 
> Signed-off-by: Michael Buesch <mb@bu3sch.de>
> 
> ---
> 
> Well, we could also change the bitbang helpers in linux/spi/spi_bitbang.h
> or change the way the gpio_get_value API is defined, but I personally think
> this patch is pretty good as is.
> In any case, it fixes a real bug on platforms like the bcm47xx which
> return 0 or nonzero for gpio_get_value.
> 
> Index: linux-2.6/drivers/spi/spi_gpio.c
> ===================================================================
> --- linux-2.6.orig/drivers/spi/spi_gpio.c	2009-02-14 21:37:14.000000000 +0100
> +++ linux-2.6/drivers/spi/spi_gpio.c	2009-02-15 16:27:16.000000000 +0100
> @@ -114,7 +114,7 @@ static inline void setmosi(const struct 
>  
>  static inline int getmiso(const struct spi_device *spi)
>  {
> -	return gpio_get_value(SPI_MISO_GPIO);
> +	return !!gpio_get_value(SPI_MISO_GPIO);
>  }
>  
>  #undef pdata
> 

Seems somewhat pointless, really.  It's a very common C idiom to treat
any non-zero value as true, and the above just adds a couple more
instructions which we didn't need to execute.

If this function is speed-critical (which is what David's comment
implies) then perhaps this should be "fixed" by tightening up the
(presently apparently undocumented) interface?  And then speeding up
all the other getmiso() implementations?

Re: [PATCH] spi-gpio: Sanitize MISO bitvalue

[Michael Buesch]

On Wednesday 18 February 2009 22:04:26 Andrew Morton wrote:
> On Sun, 15 Feb 2009 16:30:41 +0100
> Michael Buesch <mb@bu3sch.de> wrote:
> 
> > gpio_get_value() returns 0 or nonzero, but getmiso() expects 0 or 1.
> > Sanitize the value to a 0/1 boolean.
> > 
> > Signed-off-by: Michael Buesch <mb@bu3sch.de>
> > 
> > ---
> > 
> > Well, we could also change the bitbang helpers in linux/spi/spi_bitbang.h
> > or change the way the gpio_get_value API is defined, but I personally think
> > this patch is pretty good as is.
> > In any case, it fixes a real bug on platforms like the bcm47xx which
> > return 0 or nonzero for gpio_get_value.
> > 
> > Index: linux-2.6/drivers/spi/spi_gpio.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/spi/spi_gpio.c	2009-02-14 21:37:14.000000000 +0100
> > +++ linux-2.6/drivers/spi/spi_gpio.c	2009-02-15 16:27:16.000000000 +0100
> > @@ -114,7 +114,7 @@ static inline void setmosi(const struct 
> >  
> >  static inline int getmiso(const struct spi_device *spi)
> >  {
> > -	return gpio_get_value(SPI_MISO_GPIO);
> > +	return !!gpio_get_value(SPI_MISO_GPIO);
> >  }
> >  
> >  #undef pdata
> > 
> 
> Seems somewhat pointless, really.  It's a very common C idiom to treat
> any non-zero value as true, and the above just adds a couple more
> instructions which we didn't need to execute.

No you must look at the user of getmiso().
It does something like this:

	for (bitnr = 0; bitnr < x; bitnr++) {
		foo = getmiso() << bitnr;
		...
	}

> If this function is speed-critical (which is what David's comment
> implies) then perhaps this should be "fixed" by tightening up the
> (presently apparently undocumented) interface?  And then speeding up
> all the other getmiso() implementations?

He was talking about gpio_get_value() and my (silly) suggestion to change
it to return 0 or 1. I knew that he would reject that, because we already talked
about this in the past. So changing getmiso() _is_ the way to go. It is the cheapest
way to do this, in fact. Doing it _inside_ of getmiso() would mean that it could
possibly be redundant, if upper layers already did it.

David suggested documenting the fact that getmiso() expects 0/1.
He can easily do that in yet another patch if he likes this.

My patch is just supposed to fix a real-world bug, which isn't in a released kernel, yet.
So if we hurry up, we can still get it into .29.

The documentation change can still go in later.

-- 
Greetings, Michael.

Re: [PATCH] spi-gpio: Sanitize MISO bitvalue

[David Brownell]

On Wednesday 18 February 2009, Andrew Morton wrote:
> If this function is speed-critical (which is what David's comment
> implies) then perhaps this should be "fixed" by tightening up the
> (presently apparently undocumented) interface?  And then speeding up
> all the other getmiso() implementations?

All the per-bit GPIO functions can be speed-critical,
which is why they use the zero/nonzero convention
instead of demanding extra instructions to switch to
the zero/one convention.

This particular function needs the zero/one convention;
in that respect it's unusual.