[PATCH 2/4] gpiolib: add bitmask for valid GPIO lines

[Archit Taneja]

Hi,

On 11/08/2017 04:37 AM, Timur Tabi wrote:
> Add support for specifying that some GPIOs within a range are unavailable.
> Some systems have a sparse list of GPIOs, where a range of GPIOs is
> specified (usually 0 to n-1), but some subset within that range is
> absent or unavailable for whatever reason.
> 
> To support this, allow drivers to specify a bitmask of GPIOs that
> are present or absent.  Gpiolib will then block access to those that
> are absent.
> 
> Signed-off-by: Timur Tabi <timur at codeaurora.org>
> ---
>   drivers/gpio/gpiolib.c      | 43 +++++++++++++++++++++++++++++++++++--------
>   include/linux/gpio/driver.h |  2 ++
>   2 files changed, 37 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 60553af4c004..c32387936cdd 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1481,22 +1481,36 @@ static struct gpio_chip *find_chip_by_name(const char *name)
>   
>   static int gpiochip_irqchip_init_valid_mask(struct gpio_chip *gpiochip)
>   {
> -	if (!gpiochip->irq_need_valid_mask)
> -		return 0;
> +	if (gpiochip->irq_need_valid_mask) {
> +		gpiochip->irq_valid_mask =
> +			kcalloc(BITS_TO_LONGS(gpiochip->ngpio),
> +				sizeof(long), GFP_KERNEL);
> +		if (!gpiochip->irq_valid_mask)
> +			return -ENOMEM;
>   
> -	gpiochip->irq_valid_mask = kcalloc(BITS_TO_LONGS(gpiochip->ngpio),
> -					   sizeof(long), GFP_KERNEL);
> -	if (!gpiochip->irq_valid_mask)
> -		return -ENOMEM;
> +		/* Assume by default all GPIOs are valid */
> +		bitmap_fill(gpiochip->irq_valid_mask, gpiochip->ngpio);
> +	}
>   
> -	/* Assume by default all GPIOs are valid */
> -	bitmap_fill(gpiochip->irq_valid_mask, gpiochip->ngpio);
> +	if (gpiochip->line_need_valid_mask) {
> +		gpiochip->line_valid_mask =
> +			kcalloc(BITS_TO_LONGS(gpiochip->ngpio),
> +				sizeof(long), GFP_KERNEL);
> +		if (!gpiochip->line_valid_mask)
> +			return -ENOMEM;
> +
> +		/* Assume by default all GPIOs are valid */
> +		bitmap_fill(gpiochip->line_valid_mask, gpiochip->ngpio);
> +	}
>   
>   	return 0;
>   }
>   
>   static void gpiochip_irqchip_free_valid_mask(struct gpio_chip *gpiochip)
>   {
> +	kfree(gpiochip->line_valid_mask);
> +	gpiochip->line_valid_mask = NULL;
> +
>   	kfree(gpiochip->irq_valid_mask);
>   	gpiochip->irq_valid_mask = NULL;
>   }
> @@ -1510,6 +1524,15 @@ static bool gpiochip_irqchip_irq_valid(const struct gpio_chip *gpiochip,
>   	return test_bit(offset, gpiochip->irq_valid_mask);
>   }
>   
> +static bool gpiochip_irqchip_line_valid(const struct gpio_chip *gpiochip,
> +					unsigned int offset)
> +{
> +	/* No mask means all valid */
> +	if (likely(!gpiochip->line_valid_mask))
> +		return true;
> +	return test_bit(offset, gpiochip->line_valid_mask);
> +}
> +
>   /**
>    * gpiochip_set_cascaded_irqchip() - connects a cascaded irqchip to a gpiochip
>    * @gpiochip: the gpiochip to set the irqchip chain to
> @@ -3320,6 +3343,10 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
>   		return desc;
>   	}
>   
> +	/* Make sure the GPIO is valid before we request it. */
> +	if (!gpiochip_irqchip_line_valid(desc->gdev->chip, idx))
> +		return ERR_PTR(-EACCES);

We're hitting an issue here ^ when a consumer calls the devm_gpiod_get() API. This API
internally sets idx in gpiod_get_index to always 0. For example, in the call sequence
below, the idx argument to gpiochip_irqchip_line_valid is always 0:

devm_gpiod_get(dev, con_id, flags)
    devm_gpiod_get_index(dev, con_id, 0, flags)
       gpiod_get_index(dev, con_id, 0, flags)
          if (!gpiochip_irqchip_line_valid(desc->gdev->chip, 0))
              return ERR_PTR(-EACCES);

Therefore, with these patches, if we create a sparse gpio map, and the
0th gpio is set to "not accessible", all gpio requests end up failing with EACCESS
because idx is always passed as 0.

In gpiochip_irqchip_line_valid, shouldn't we test the nth bit (that corresponds to
this gpio_desc) in chip->line_valid_mask instead of idx passed above, which is always
0?

Thanks,
Archit


> +
>   	status = gpiod_request(desc, con_id);
>   	if (status < 0)
>   		return ERR_PTR(status);
> diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
> index 424e5139ff10..853828ccabc8 100644
> --- a/include/linux/gpio/driver.h
> +++ b/include/linux/gpio/driver.h
> @@ -173,6 +173,8 @@ struct gpio_chip {
>   	bool			irq_nested;
>   	bool			irq_need_valid_mask;
>   	unsigned long		*irq_valid_mask;
> +	bool			line_need_valid_mask;
> +	unsigned long		*line_valid_mask;
>   	struct lock_class_key	*lock_key;
>   #endif
>   
> 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project