Re: [PATCH] gpio: mmio: Also read bits that are zero

[Clemens Gruber <clemens.gruber@pqgruber.com>]

Hi Linus,

On Tue, Jan 16, 2018 at 10:17:14AM +0100, Linus Walleij wrote:
> The code for .get_multiple() has bugs:
> 
> 1. The simple .get_multiple() just reads a register, masks it
> and sets the return value. This is not correct: we only want to
> assign values (whether 0 or 1) to the bits that are set in the
> mask. Fix this by using &= ~mask to clear all bits in the mask
> and then |= val & mask to set the corresponding bits from the
> read.
> 
> 2. The bgpio_get_multiple_be() call has a similar problem: it
> uses the |= operator to set the bits, so only the bits in the
> mask are affected, but it misses to clear all returned bits
> from the mask initially, so some bits will be returned
> erroneously set to 1.
> 
> 3. The bgpio_get_set_multiple() again fails to clear the bits
> from the mask.
> 
> Fixes: 80057cb417b2 ("gpio-mmio: Use the new .get_multiple() callback")
> Cc: Bartosz Golaszewski <brgl@bgdev.pl>
> Reported-by: Clemens Gruber <clemens.gruber@pqgruber.com>
> Reported-by: Lukas Wunner <lukas@wunner.de>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> Clemens: it would be great if you could test this, I want to
> push the fix ASAP if it solves the problem.

It does not fix the regression, there is still an infinite loop. I
commented below, what I had to do to fix it:

> ---
>  drivers/gpio/gpio-mmio.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-mmio.c b/drivers/gpio/gpio-mmio.c
> index f9042bcc27a4..eae078d43611 100644
> --- a/drivers/gpio/gpio-mmio.c
> +++ b/drivers/gpio/gpio-mmio.c
> @@ -154,6 +154,9 @@ static int bgpio_get_set_multiple(struct gpio_chip *gc, unsigned long *mask,
>  	unsigned long set_mask = 0;
>  	int bit = 0;

Changing this to int bit = -1;

>  
> +	/* Make sure we first clear any bits that are zero when we read the register */
> +	*bits &= ~*mask;
> +
>  	while ((bit = find_next_bit(mask, gc->ngpio, bit)) != gc->ngpio) {

And this to ..        find_next_bit(mask, gc->ngpio, bit + 1) < gc->ngpio) {

Fixes the problem for me.

Lukas suggested we could remove the while loop and assign the masks
directly, which would be even simpler/better, e.g.
set_mask = mask & gc->bgpio_dir;
get_mask = mask & ~gc->bgpio_dir;

>  		if (gc->bgpio_dir & BIT(bit))
>  			set_mask |= BIT(bit);
> @@ -176,13 +179,13 @@ static int bgpio_get(struct gpio_chip *gc, unsigned int gpio)
>  
>  /*
>   * This only works if the bits in the GPIO register are in native endianness.
> - * It is dirt simple and fast in this case. (Also the most common case.)
>   */
>  static int bgpio_get_multiple(struct gpio_chip *gc, unsigned long *mask,
>  			      unsigned long *bits)
>  {
> -
> -	*bits = gc->read_reg(gc->reg_dat) & *mask;
> +	/* Make sure we first clear any bits that are zero when we read the register */
> +	*bits &= ~*mask;
> +	*bits |= gc->read_reg(gc->reg_dat) & *mask;
>  	return 0;
>  }
>  
> @@ -196,6 +199,9 @@ static int bgpio_get_multiple_be(struct gpio_chip *gc, unsigned long *mask,
>  	unsigned long val;
>  	int bit;
>  
> +	/* Make sure we first clear any bits that are zero when we read the register */
> +	*bits &= ~*mask;
> +
>  	/* Create a mirrored mask */
>  	bit = 0;
>  	while ((bit = find_next_bit(mask, gc->ngpio, bit)) != gc->ngpio)
> -- 
> 2.14.3
> 

Cheers,
Clemens