[PATCH v2 0/4] gpio: Hide and obey valid_mask

[PATCH v2 0/4] gpio: Hide and obey valid_mask

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 1942 bytes --]

GPIO controllers may have some pins which can be excluded from the GPIO
usage on certain hardware configurations. The valid_mask member of the
struct gpio_chip has been used to denote usable pins if some pins should
be excluded.

The GPIO request should fail for GPIOs which are masked. Under certain
conditions this was only done when GPIO chip provided the 'request'
callback. We fix this to be always done.

The valid_mask member of the gpio_chip should no longer be directly
populated by the drivers but GPIO core does this (unconditionally,
overwriting any mask set directly by the drivers). Drivers are intended
to populate the valid_mask using the init_valid_mask -callback.

This series enforces using the init_valid_mask by hiding the valid_mask
in structure which is internal to the GPIO core. A single in-tree driver
was found to access the valid_mask directly. This series also removes
those direct accesses as has been discussed [1]. Additionally, we
introduce a getter-function which can be used to obtain the valid_mask.

[1]: https://lore.kernel.org/all/TY3PR01MB11346EC54C8672C4D28F931F686CC2@TY3PR01MB11346.jpnprd01.prod.outlook.com/

Revision history:
v1 => v2:
 - Hide the 'valid_mask' instead of documenting it to be internal
 - Make the gpio_request() to obey the valid_mask whether the gpio_chip
   has the 'request' -callback populated or not.

---

Matti Vaittinen (4):
  gpio: Respect valid_mask when requesting GPIOs
  gpio: Add a valid_mask getter
  gpio: gpio-rcar: Drop direct use of valid_mask
  gpio: Hide valid_mask from direct assignments

 drivers/gpio/gpio-rcar.c    | 13 +++++-------
 drivers/gpio/gpiolib.c      | 40 ++++++++++++++++++++++++++-----------
 drivers/gpio/gpiolib.h      |  3 +++
 include/linux/gpio/driver.h |  9 +--------
 4 files changed, 37 insertions(+), 28 deletions(-)


base-commit: d082ecbc71e9e0bf49883ee4afd435a77a5101b6
-- 
2.48.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v2 1/4] gpio: Respect valid_mask when requesting GPIOs

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 1967 bytes --]

When GPIOs were requested the validity of GPIOs were checked only when
the GPIO-chip had the request -callback populated. This made using
masked GPIOs possible.

The GPIO chip driver authors may find it difficult to understand the
relation of enforsing the GPIO validity and the 'request' -callback
because the current documentation for the 'request' callback does not
mention this. It only states:

 * @request: optional hook for chip-specific activation, such as
 *      enabling module power and clock; may sleep

The validity of the GPIO line should be checked whether the driver
provides the 'request' callback or not.

Unconditionally check the GPIO validity when GPIO is being requested.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
Revision history:
v1 => v2:
 - New patch (born as a spin-off from the discussion to v1:
   https://lore.kernel.org/all/Z71qphikHPGB0Yuv@mva-rohm/

I'm not sure if this warrants a Fixes -tag.
---
 drivers/gpio/gpiolib.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index fc19df5a64c2..98543d79f69c 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2326,6 +2326,10 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
 	if (!guard.gc)
 		return -ENODEV;
 
+	offset = gpio_chip_hwgpio(desc);
+	if (!gpiochip_line_is_valid(guard.gc, offset))
+		return -EINVAL;
+
 	if (test_and_set_bit(FLAG_REQUESTED, &desc->flags))
 		return -EBUSY;
 
@@ -2334,11 +2338,7 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
 	 */
 
 	if (guard.gc->request) {
-		offset = gpio_chip_hwgpio(desc);
-		if (gpiochip_line_is_valid(guard.gc, offset))
-			ret = guard.gc->request(guard.gc, offset);
-		else
-			ret = -EINVAL;
+		ret = guard.gc->request(guard.gc, offset);
 		if (ret)
 			goto out_clear_bit;
 	}
-- 
2.48.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/4] gpio: Respect valid_mask when requesting GPIOs

[Linus Walleij]

On Fri, Feb 28, 2025 at 1:35 PM Matti Vaittinen
<mazziesaccount@gmail.com> wrote:

> When GPIOs were requested the validity of GPIOs were checked only when
> the GPIO-chip had the request -callback populated. This made using
> masked GPIOs possible.
>
> The GPIO chip driver authors may find it difficult to understand the
> relation of enforsing the GPIO validity and the 'request' -callback
> because the current documentation for the 'request' callback does not
> mention this. It only states:
>
>  * @request: optional hook for chip-specific activation, such as
>  *      enabling module power and clock; may sleep
>
> The validity of the GPIO line should be checked whether the driver
> provides the 'request' callback or not.
>
> Unconditionally check the GPIO validity when GPIO is being requested.
>
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

Good catch!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH v2 2/4] gpio: Add a valid_mask getter

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 2368 bytes --]

The valid_mask member of the struct gpio_chip is unconditionally written
by the GPIO core at driver registration. It shouldn't be directly
populated by drivers. This can be prevented by moving it from the struct
gpio_chip to struct gpio_device, which is internal to the GPIO core.

As a preparatory step, provide a getter function which can be used by
those drivers which need the valid_mask information.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
Revision history:
v1 => v2:
  - New patch
  (spin-off from discussion to v1:
   https://lore.kernel.org/all/Z71qphikHPGB0Yuv@mva-rohm/)
---
 drivers/gpio/gpiolib.c      | 16 ++++++++++++++++
 include/linux/gpio/driver.h |  1 +
 2 files changed, 17 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 98543d79f69c..7e36894bab11 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -703,6 +703,22 @@ static int gpiochip_add_pin_ranges(struct gpio_chip *gc)
 	return 0;
 }
 
+/**
+ * gpiochip_query_valid_mask - return the GPIO validity information
+ * @gc:	gpio chip which validity information is queried
+ *
+ * Returns: bitmap representing valid GPIOs or NULL if all GPIOs are valid
+ *
+ * Some GPIO chips may support configurations where some of the pins aren't
+ * available. These chips can have valid_mask set to represent the valid
+ * GPIOs. This function can be used to retrieve this information.
+ */
+const unsigned long *gpiochip_query_valid_mask(const struct gpio_chip *gc)
+{
+	return gc->valid_mask;
+}
+EXPORT_SYMBOL_GPL(gpiochip_query_valid_mask);
+
 bool gpiochip_line_is_valid(const struct gpio_chip *gc,
 				unsigned int offset)
 {
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 2dd7cb9cc270..7dfb8341b0e2 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -678,6 +678,7 @@ bool gpiochip_line_is_open_source(struct gpio_chip *gc, unsigned int offset);
 /* Sleep persistence inquiry for drivers */
 bool gpiochip_line_is_persistent(struct gpio_chip *gc, unsigned int offset);
 bool gpiochip_line_is_valid(const struct gpio_chip *gc, unsigned int offset);
+const unsigned long *gpiochip_query_valid_mask(const struct gpio_chip *gc);
 
 /* get driver data */
 void *gpiochip_get_data(struct gpio_chip *gc);
-- 
2.48.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/4] gpio: Add a valid_mask getter

[Linus Walleij]

On Fri, Feb 28, 2025 at 1:35 PM Matti Vaittinen
<mazziesaccount@gmail.com> wrote:

> The valid_mask member of the struct gpio_chip is unconditionally written
> by the GPIO core at driver registration. It shouldn't be directly
> populated by drivers. This can be prevented by moving it from the struct
> gpio_chip to struct gpio_device, which is internal to the GPIO core.
>
> As a preparatory step, provide a getter function which can be used by
> those drivers which need the valid_mask information.
>
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH v2 3/4] gpio: gpio-rcar: Drop direct use of valid_mask

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 2404 bytes --]

The valid_mask member of the struct gpio_chip is unconditionally written
by the GPIO core at driver registration. It should not be directly
populated by the drivers. Hiding the valid_mask in struct gpio_device
makes it clear it is not meant to be directly populated by drivers. This
means drivers should not access it directly from the struct gpio_chip.

The gpio-rcar checks the valid mask in set/get_multiple() operations.
This is no longer needed [1]. Drop these checks.

Additionally, the valid_mask is needed for enabling the GPIO inputs at
probe time. Use the new valid_mask -getter function instead of accessing
it directly from the struct gpio_chip.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
Please note that this change is compile-tested only. All reviewing and
testing is highly appreciated.

Revision history:
v1 => v2:
 - New patch

[1]: https://lore.kernel.org/all/TY3PR01MB11346EC54C8672C4D28F931F686CC2@TY3PR01MB11346.jpnprd01.prod.outlook.com/
---
 drivers/gpio/gpio-rcar.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/gpio/gpio-rcar.c b/drivers/gpio/gpio-rcar.c
index 2ecee3269a0c..e32d731d0473 100644
--- a/drivers/gpio/gpio-rcar.c
+++ b/drivers/gpio/gpio-rcar.c
@@ -336,9 +336,6 @@ static int gpio_rcar_get_multiple(struct gpio_chip *chip, unsigned long *mask,
 	unsigned long flags;
 
 	bankmask = mask[0] & GENMASK(chip->ngpio - 1, 0);
-	if (chip->valid_mask)
-		bankmask &= chip->valid_mask[0];
-
 	if (!bankmask)
 		return 0;
 
@@ -380,9 +377,6 @@ static void gpio_rcar_set_multiple(struct gpio_chip *chip, unsigned long *mask,
 	u32 val, bankmask;
 
 	bankmask = mask[0] & GENMASK(chip->ngpio - 1, 0);
-	if (chip->valid_mask)
-		bankmask &= chip->valid_mask[0];
-
 	if (!bankmask)
 		return;
 
@@ -482,10 +476,13 @@ static int gpio_rcar_parse_dt(struct gpio_rcar_priv *p, unsigned int *npins)
 static void gpio_rcar_enable_inputs(struct gpio_rcar_priv *p)
 {
 	u32 mask = GENMASK(p->gpio_chip.ngpio - 1, 0);
+	const unsigned long *valid_mask;
+
+	valid_mask = gpiochip_query_valid_mask(&p->gpio_chip);
 
 	/* Select "Input Enable" in INEN */
-	if (p->gpio_chip.valid_mask)
-		mask &= p->gpio_chip.valid_mask[0];
+	if (valid_mask)
+		mask &= valid_mask[0];
 	if (mask)
 		gpio_rcar_write(p, INEN, gpio_rcar_read(p, INEN) | mask);
 }
-- 
2.48.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 3/4] gpio: gpio-rcar: Drop direct use of valid_mask

[Linus Walleij]

On Fri, Feb 28, 2025 at 1:36 PM Matti Vaittinen
<mazziesaccount@gmail.com> wrote:

> The valid_mask member of the struct gpio_chip is unconditionally written
> by the GPIO core at driver registration. It should not be directly
> populated by the drivers. Hiding the valid_mask in struct gpio_device
> makes it clear it is not meant to be directly populated by drivers. This
> means drivers should not access it directly from the struct gpio_chip.
>
> The gpio-rcar checks the valid mask in set/get_multiple() operations.
> This is no longer needed [1]. Drop these checks.
>
> Additionally, the valid_mask is needed for enabling the GPIO inputs at
> probe time. Use the new valid_mask -getter function instead of accessing
> it directly from the struct gpio_chip.
>
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH v2 4/4] gpio: Hide valid_mask from direct assignments

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 4782 bytes --]

The valid_mask member of the struct gpio_chip is unconditionally written
by the GPIO core at driver registration. Current documentation does not
mention this but just says the valid_mask is used if it's not NULL. This
lured me to try populating it directly in the GPIO driver probe instead
of using the init_valid_mask() callback. It took some retries with
different bitmaps and eventually a bit of code-reading to understand why
the valid_mask was not obeyed. I could've avoided this trial and error if
the valid_mask was hidden in the struct gpio_device instead of being a
visible member of the struct gpio_chip.

Help the next developer who decides to directly populate the valid_mask
in struct gpio_chip by hiding the valid_mask in struct gpio_device and
keep it internal to the GPIO core.

Suggested-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
Revision history:
v1 => v2:
 - Hide the valid_mask instead of documenting it as internal to GPIO
   core as suggested by Linus W.
https://lore.kernel.org/all/Z71qphikHPGB0Yuv@mva-rohm/
---
 drivers/gpio/gpiolib.c      | 16 ++++++++--------
 drivers/gpio/gpiolib.h      |  3 +++
 include/linux/gpio/driver.h |  8 --------
 3 files changed, 11 insertions(+), 16 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 7e36894bab11..37e1f277b0a8 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -652,7 +652,7 @@ static int gpiochip_apply_reserved_ranges(struct gpio_chip *gc)
 		if (start >= gc->ngpio || start + count > gc->ngpio)
 			continue;
 
-		bitmap_clear(gc->valid_mask, start, count);
+		bitmap_clear(gc->gpiodev->valid_mask, start, count);
 	}
 
 	kfree(ranges);
@@ -666,8 +666,8 @@ static int gpiochip_init_valid_mask(struct gpio_chip *gc)
 	if (!(gpiochip_count_reserved_ranges(gc) || gc->init_valid_mask))
 		return 0;
 
-	gc->valid_mask = gpiochip_allocate_mask(gc);
-	if (!gc->valid_mask)
+	gc->gpiodev->valid_mask = gpiochip_allocate_mask(gc);
+	if (!gc->gpiodev->valid_mask)
 		return -ENOMEM;
 
 	ret = gpiochip_apply_reserved_ranges(gc);
@@ -676,7 +676,7 @@ static int gpiochip_init_valid_mask(struct gpio_chip *gc)
 
 	if (gc->init_valid_mask)
 		return gc->init_valid_mask(gc,
-					   gc->valid_mask,
+					   gc->gpiodev->valid_mask,
 					   gc->ngpio);
 
 	return 0;
@@ -684,7 +684,7 @@ static int gpiochip_init_valid_mask(struct gpio_chip *gc)
 
 static void gpiochip_free_valid_mask(struct gpio_chip *gc)
 {
-	gpiochip_free_mask(&gc->valid_mask);
+	gpiochip_free_mask(&gc->gpiodev->valid_mask);
 }
 
 static int gpiochip_add_pin_ranges(struct gpio_chip *gc)
@@ -715,7 +715,7 @@ static int gpiochip_add_pin_ranges(struct gpio_chip *gc)
  */
 const unsigned long *gpiochip_query_valid_mask(const struct gpio_chip *gc)
 {
-	return gc->valid_mask;
+	return gc->gpiodev->valid_mask;
 }
 EXPORT_SYMBOL_GPL(gpiochip_query_valid_mask);
 
@@ -723,9 +723,9 @@ bool gpiochip_line_is_valid(const struct gpio_chip *gc,
 				unsigned int offset)
 {
 	/* No mask means all valid */
-	if (likely(!gc->valid_mask))
+	if (likely(!gc->gpiodev->valid_mask))
 		return true;
-	return test_bit(offset, gc->valid_mask);
+	return test_bit(offset, gc->gpiodev->valid_mask);
 }
 EXPORT_SYMBOL_GPL(gpiochip_line_is_valid);
 
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 147156ec502b..b9a4f161db53 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -32,6 +32,8 @@
  * @chip: pointer to the corresponding gpiochip, holding static
  * data for this device
  * @descs: array of ngpio descriptors.
+ * @valid_mask: If not %NULL, holds bitmask of GPIOs which are valid to be
+ * used from the chip.
  * @desc_srcu: ensures consistent state of GPIO descriptors exposed to users
  * @ngpio: the number of GPIO lines on this GPIO device, equal to the size
  * of the @descs array.
@@ -65,6 +67,7 @@ struct gpio_device {
 	struct module		*owner;
 	struct gpio_chip __rcu	*chip;
 	struct gpio_desc	*descs;
+	unsigned long		*valid_mask;
 	struct srcu_struct	desc_srcu;
 	unsigned int		base;
 	u16			ngpio;
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 7dfb8341b0e2..0e8621be7272 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -499,14 +499,6 @@ struct gpio_chip {
 	struct gpio_irq_chip irq;
 #endif /* CONFIG_GPIOLIB_IRQCHIP */
 
-	/**
-	 * @valid_mask:
-	 *
-	 * If not %NULL, holds bitmask of GPIOs which are valid to be used
-	 * from the chip.
-	 */
-	unsigned long *valid_mask;
-
 #if defined(CONFIG_OF_GPIO)
 	/*
 	 * If CONFIG_OF_GPIO is enabled, then all GPIO controllers described in
-- 
2.48.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 4/4] gpio: Hide valid_mask from direct assignments

[Linus Walleij]

On Fri, Feb 28, 2025 at 1:36 PM Matti Vaittinen
<mazziesaccount@gmail.com> wrote:

> The valid_mask member of the struct gpio_chip is unconditionally written
> by the GPIO core at driver registration. Current documentation does not
> mention this but just says the valid_mask is used if it's not NULL. This
> lured me to try populating it directly in the GPIO driver probe instead
> of using the init_valid_mask() callback. It took some retries with
> different bitmaps and eventually a bit of code-reading to understand why
> the valid_mask was not obeyed. I could've avoided this trial and error if
> the valid_mask was hidden in the struct gpio_device instead of being a
> visible member of the struct gpio_chip.
>
> Help the next developer who decides to directly populate the valid_mask
> in struct gpio_chip by hiding the valid_mask in struct gpio_device and
> keep it internal to the GPIO core.
>
> Suggested-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v2 0/4] gpio: Hide and obey valid_mask

[Bartosz Golaszewski]

On Fri, Feb 28, 2025 at 1:35 PM Matti Vaittinen
<mazziesaccount@gmail.com> wrote:
>
> GPIO controllers may have some pins which can be excluded from the GPIO
> usage on certain hardware configurations. The valid_mask member of the
> struct gpio_chip has been used to denote usable pins if some pins should
> be excluded.
>
> The GPIO request should fail for GPIOs which are masked. Under certain
> conditions this was only done when GPIO chip provided the 'request'
> callback. We fix this to be always done.
>
> The valid_mask member of the gpio_chip should no longer be directly
> populated by the drivers but GPIO core does this (unconditionally,
> overwriting any mask set directly by the drivers). Drivers are intended
> to populate the valid_mask using the init_valid_mask -callback.
>
> This series enforces using the init_valid_mask by hiding the valid_mask
> in structure which is internal to the GPIO core. A single in-tree driver
> was found to access the valid_mask directly. This series also removes
> those direct accesses as has been discussed [1]. Additionally, we
> introduce a getter-function which can be used to obtain the valid_mask.
>
> [1]: https://lore.kernel.org/all/TY3PR01MB11346EC54C8672C4D28F931F686CC2@TY3PR01MB11346.jpnprd01.prod.outlook.com/
>

Hey! This no longer applies to my gpio/for-next branch, would you mind
rebasing and resending?

Bart