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

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

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 1995 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:
v2 => v3:
 - Rebased on top of the gpio/for-next
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: 44fe79020b91a1a8620e44d4f361b389e8fc552f
-- 
2.48.1


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

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

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 2046 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>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
Revision history:
v2 => v3:
 - Rebase to gpio/for-next
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 8724c7d8459e..b5f472beb3bd 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2358,16 +2358,16 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
 	if (test_and_set_bit(FLAG_REQUESTED, &desc->flags))
 		return -EBUSY;
 
+	offset = gpio_chip_hwgpio(desc);
+	if (!gpiochip_line_is_valid(guard.gc, offset))
+		return -EINVAL;
+
 	/* NOTE:  gpio_request() can be called in early boot,
 	 * before IRQs are enabled, for non-sleeping (SOC) GPIOs.
 	 */
 
 	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 > 0)
 			ret = -EBADE;
 		if (ret)
-- 
2.48.1


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

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

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 2462 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>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
Revision history:
v2 => v3:
 - Rebase to gpio/for-next
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 b5f472beb3bd..4c15a70d4d80 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -723,6 +723,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 83e0a7e86962..e3b59fda62e0 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -720,6 +720,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 --]

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

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 2517 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>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
Revision history:
v2 => v3:
 - Rebase to gpio/for-next
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 --]

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

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 4876 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>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
Revision history:
v2 => v3:
 - Rebase to gpio/for-next
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 4c15a70d4d80..e5eb3f0ee071 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -672,7 +672,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);
@@ -686,8 +686,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);
@@ -696,7 +696,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;
@@ -704,7 +704,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)
@@ -735,7 +735,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);
 
@@ -743,9 +743,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 58af0491e60e..a738e6c647d8 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 e3b59fda62e0..e6e5304c99ca 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -514,14 +514,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 v3 4/4] gpio: Hide valid_mask from direct assignments

[Doug Anderson]

Hi,

On Wed, Mar 5, 2025 at 5:23 AM 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>
> ---
> Revision history:
> v2 => v3:
>  - Rebase to gpio/for-next
> 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(-)

FWIW, I've found that this patch is crashing me at bootup on my
sc7180-trogdor board. The problem is pretty obvious in gdb.
"gc->gpiodev" is NULL in gpiochip_line_is_valid().

0xffff80008066c760 in gpiochip_line_is_valid (gc=0xffff000083223890,
offset=offset@entry=66) at drivers/gpio/gpiolib.c:746
746             if (likely(!gc->gpiodev->valid_mask))
(gdb) bt
#0  0xffff80008066c760 in gpiochip_line_is_valid
(gc=0xffff000083223890, offset=offset@entry=66) at
drivers/gpio/gpiolib.c:746
#1  0xffff800080666338 in msm_pinmux_request (pctldev=<optimized out>,
offset=66) at drivers/pinctrl/qcom/pinctrl-msm.c:152
#2  0xffff800080662314 in pin_request (pctldev=0xffff000082686ac0,
pin=66, owner=0xffff000082c02790 "3500000.pinctrl", gpio_range=0x0)
    at drivers/pinctrl/pinmux.c:176
#3  0xffff800080662900 in pinmux_enable_setting
(setting=0xffff000082684b40) at drivers/pinctrl/pinmux.c:445
#4  0xffff80008065fd54 in pinctrl_commit_state (p=0xffff000083a07520,
state=0xffff000082684a40) at drivers/pinctrl/core.c:1300
#5  0xffff8000806605bc in pinctrl_select_state (p=0xffff000083223890,
p@entry=0xffff000082686ac0, state=0x42) at drivers/pinctrl/core.c:1381
#6  pinctrl_claim_hogs (pctldev=0xffff000082686ac0) at
drivers/pinctrl/core.c:2136
#7  pinctrl_enable (pctldev=0xffff000082686ac0) at drivers/pinctrl/core.c:2156
#8  0xffff800080660814 in pinctrl_register
(pctldesc=0xffff000083223a90, dev=0xffff000081406410,
driver_data=0xffff000083223880) at drivers/pinctrl/core.c:2193
#9  0xffff800080660df4 in devm_pinctrl_register
(dev=0xffff000081406410, pctldesc=0xffff000083223a90,
driver_data=0xffff000083223880) at drivers/pinctrl/core.c:2313
#10 0xffff8000806657b4 in msm_pinctrl_probe (pdev=0xffff000081406400,
soc_data=<optimized out>) at drivers/pinctrl/qcom/pinctrl-msm.c:1579
#11 0xffff80008066afcc in sc7180_pinctrl_probe
(pdev=0xffff000083223890) at
drivers/pinctrl/qcom/pinctrl-sc7180.c:1147
#12 0xffff80008089583c in platform_probe (_dev=0xffff000081406410) at
drivers/base/platform.c:1404

(gdb) print gc->gpiodev
$1 = (struct gpio_device *) 0x0

-Doug

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

[Matti Vaittinen]

Hi Doug,

On 13/04/2025 02:00, Doug Anderson wrote:
> Hi,
> 
> On Wed, Mar 5, 2025 at 5:23 AM 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>
>> ---
>> Revision history:
>> v2 => v3:
>>   - Rebase to gpio/for-next
>> 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(-)
> 
> FWIW, I've found that this patch is crashing me at bootup on my
> sc7180-trogdor board. The problem is pretty obvious in gdb.
> "gc->gpiodev" is NULL in gpiochip_line_is_valid().

Thanks for debugging this! I find this odd. It seems to me the 
pinctrl-msm.c is calling the gpiochip_add_data() for the chip, in the 
msm_gpio_init() - which is called from the msm_pinctrl_probe().

The gpiochip_add_data() should go to the gpiochip_add_data_with_key() - 
where the gpiodev should be allocated and set.

I don't spot any successful code path where the gpiodev was not allocated.

> 
> 0xffff80008066c760 in gpiochip_line_is_valid (gc=0xffff000083223890,
> offset=offset@entry=66) at drivers/gpio/gpiolib.c:746
> 746             if (likely(!gc->gpiodev->valid_mask))
> (gdb) bt
> #0  0xffff80008066c760 in gpiochip_line_is_valid
> (gc=0xffff000083223890, offset=offset@entry=66) at
> drivers/gpio/gpiolib.c:746
> #1  0xffff800080666338 in msm_pinmux_request (pctldev=<optimized out>,

Ah, but now I see the call comes from the pinmux. Looking at the 
msm_pinctrl_probe() - the pincontroller is registered before the gpio. 
Maybe, with unlucky timing, the request happens right after registering 
the pinctrl - but before registering the gpios.

This, I think, can be a bug even before this change (because the 
valid_mask is not initialized prior the gpio registration) - but this 
change now made it obvious.

I see the probe is actually an exported function, and there are mentions 
about ACPI support etc. I don't really know if there are valid cases 
where the pincontroller should be usable without the gpiochip. If this 
is the case, the unconditional call to the gpiochip_line_is_valid() from 
the msm_pinmux_request() smells wrong.

I am not sure about the right fix. One could try:

@@ -1568,6 +1568,10 @@ int msm_pinctrl_probe(struct platform_device *pdev,
         if (pctrl->irq < 0)
                 return pctrl->irq;

+       ret = msm_gpio_init(pctrl);
+       if (ret)
+               return ret;
+
         pctrl->desc.owner = THIS_MODULE;
         pctrl->desc.pctlops = &msm_pinctrl_ops;
         pctrl->desc.pmxops = &msm_pinmux_ops;
@@ -1582,10 +1586,6 @@ int msm_pinctrl_probe(struct platform_device *pdev,
                 return PTR_ERR(pctrl->pctrl);
         }

-       ret = msm_gpio_init(pctrl);
-       if (ret)
-               return ret;
-
         platform_set_drvdata(pdev, pctrl);

         dev_dbg(&pdev->dev, "Probed Qualcomm pinctrl driver\n")

but I am not at all this is the fix we're looking after. I wonder if 
Krzysztof has any suggestions? (Seeing he has been authoring some 
changes here :] )

Yours,
	-- Matti


> offset=66) at drivers/pinctrl/qcom/pinctrl-msm.c:152
> #2  0xffff800080662314 in pin_request (pctldev=0xffff000082686ac0,
> pin=66, owner=0xffff000082c02790 "3500000.pinctrl", gpio_range=0x0)
>      at drivers/pinctrl/pinmux.c:176
> #3  0xffff800080662900 in pinmux_enable_setting
> (setting=0xffff000082684b40) at drivers/pinctrl/pinmux.c:445
> #4  0xffff80008065fd54 in pinctrl_commit_state (p=0xffff000083a07520,
> state=0xffff000082684a40) at drivers/pinctrl/core.c:1300
> #5  0xffff8000806605bc in pinctrl_select_state (p=0xffff000083223890,
> p@entry=0xffff000082686ac0, state=0x42) at drivers/pinctrl/core.c:1381
> #6  pinctrl_claim_hogs (pctldev=0xffff000082686ac0) at
> drivers/pinctrl/core.c:2136
> #7  pinctrl_enable (pctldev=0xffff000082686ac0) at drivers/pinctrl/core.c:2156
> #8  0xffff800080660814 in pinctrl_register
> (pctldesc=0xffff000083223a90, dev=0xffff000081406410,
> driver_data=0xffff000083223880) at drivers/pinctrl/core.c:2193
> #9  0xffff800080660df4 in devm_pinctrl_register
> (dev=0xffff000081406410, pctldesc=0xffff000083223a90,
> driver_data=0xffff000083223880) at drivers/pinctrl/core.c:2313
> #10 0xffff8000806657b4 in msm_pinctrl_probe (pdev=0xffff000081406400,
> soc_data=<optimized out>) at drivers/pinctrl/qcom/pinctrl-msm.c:1579
> #11 0xffff80008066afcc in sc7180_pinctrl_probe
> (pdev=0xffff000083223890) at
> drivers/pinctrl/qcom/pinctrl-sc7180.c:1147
> #12 0xffff80008089583c in platform_probe (_dev=0xffff000081406410) at
> drivers/base/platform.c:1404
> 
> (gdb) print gc->gpiodev
> $1 = (struct gpio_device *) 0x0
> 
> -Doug

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

[Matti Vaittinen]

On 13/04/2025 11:08, Matti Vaittinen wrote:
> Hi Doug,
> 
> On 13/04/2025 02:00, Doug Anderson wrote:
>> Hi,
>>
>> On Wed, Mar 5, 2025 at 5:23 AM 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>
>>> ---
>>> Revision history:
>>> v2 => v3:
>>>   - Rebase to gpio/for-next
>>> 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(-)
>>
>> FWIW, I've found that this patch is crashing me at bootup on my
>> sc7180-trogdor board. The problem is pretty obvious in gdb.
>> "gc->gpiodev" is NULL in gpiochip_line_is_valid().
> 
> Thanks for debugging this! I find this odd. It seems to me the pinctrl- 
> msm.c is calling the gpiochip_add_data() for the chip, in the 
> msm_gpio_init() - which is called from the msm_pinctrl_probe().
> 
> The gpiochip_add_data() should go to the gpiochip_add_data_with_key() - 
> where the gpiodev should be allocated and set.
> 
> I don't spot any successful code path where the gpiodev was not allocated.
> 
>>
>> 0xffff80008066c760 in gpiochip_line_is_valid (gc=0xffff000083223890,
>> offset=offset@entry=66) at drivers/gpio/gpiolib.c:746
>> 746             if (likely(!gc->gpiodev->valid_mask))
>> (gdb) bt
>> #0  0xffff80008066c760 in gpiochip_line_is_valid
>> (gc=0xffff000083223890, offset=offset@entry=66) at
>> drivers/gpio/gpiolib.c:746
>> #1  0xffff800080666338 in msm_pinmux_request (pctldev=<optimized out>,
> 
> Ah, but now I see the call comes from the pinmux. Looking at the 
> msm_pinctrl_probe() - the pincontroller is registered before the gpio. 
> Maybe, with unlucky timing, the request happens right after registering 
> the pinctrl - but before registering the gpios.
> 
> This, I think, can be a bug even before this change (because the 
> valid_mask is not initialized prior the gpio registration) - but this 
> change now made it obvious.
> 
> I see the probe is actually an exported function, and there are mentions 
> about ACPI support etc. I don't really know if there are valid cases 
> where the pincontroller should be usable without the gpiochip. If this 
> is the case, the unconditional call to the gpiochip_line_is_valid() from 
> the msm_pinmux_request() smells wrong.
> 
> I am not sure about the right fix. One could try:
> 
> @@ -1568,6 +1568,10 @@ int msm_pinctrl_probe(struct platform_device *pdev,
>          if (pctrl->irq < 0)
>                  return pctrl->irq;
> 
> +       ret = msm_gpio_init(pctrl);
> +       if (ret)
> +               return ret;
> +
>          pctrl->desc.owner = THIS_MODULE;
>          pctrl->desc.pctlops = &msm_pinctrl_ops;
>          pctrl->desc.pmxops = &msm_pinmux_ops;
> @@ -1582,10 +1586,6 @@ int msm_pinctrl_probe(struct platform_device *pdev,
>                  return PTR_ERR(pctrl->pctrl);
>          }
> 
> -       ret = msm_gpio_init(pctrl);
> -       if (ret)
> -               return ret;
> -
>          platform_set_drvdata(pdev, pctrl);
> 
>          dev_dbg(&pdev->dev, "Probed Qualcomm pinctrl driver\n")
> 
> but I am not at all this is the fix we're looking after. I wonder if 
> Krzysztof has any suggestions? (Seeing he has been authoring some 
> changes here :] )
> 

+Björn

> Yours,
>      -- Matti
> 
> 
>> offset=66) at drivers/pinctrl/qcom/pinctrl-msm.c:152
>> #2  0xffff800080662314 in pin_request (pctldev=0xffff000082686ac0,
>> pin=66, owner=0xffff000082c02790 "3500000.pinctrl", gpio_range=0x0)
>>      at drivers/pinctrl/pinmux.c:176
>> #3  0xffff800080662900 in pinmux_enable_setting
>> (setting=0xffff000082684b40) at drivers/pinctrl/pinmux.c:445
>> #4  0xffff80008065fd54 in pinctrl_commit_state (p=0xffff000083a07520,
>> state=0xffff000082684a40) at drivers/pinctrl/core.c:1300
>> #5  0xffff8000806605bc in pinctrl_select_state (p=0xffff000083223890,
>> p@entry=0xffff000082686ac0, state=0x42) at drivers/pinctrl/core.c:1381
>> #6  pinctrl_claim_hogs (pctldev=0xffff000082686ac0) at
>> drivers/pinctrl/core.c:2136
>> #7  pinctrl_enable (pctldev=0xffff000082686ac0) at drivers/pinctrl/ 
>> core.c:2156
>> #8  0xffff800080660814 in pinctrl_register
>> (pctldesc=0xffff000083223a90, dev=0xffff000081406410,
>> driver_data=0xffff000083223880) at drivers/pinctrl/core.c:2193
>> #9  0xffff800080660df4 in devm_pinctrl_register
>> (dev=0xffff000081406410, pctldesc=0xffff000083223a90,
>> driver_data=0xffff000083223880) at drivers/pinctrl/core.c:2313
>> #10 0xffff8000806657b4 in msm_pinctrl_probe (pdev=0xffff000081406400,
>> soc_data=<optimized out>) at drivers/pinctrl/qcom/pinctrl-msm.c:1579
>> #11 0xffff80008066afcc in sc7180_pinctrl_probe
>> (pdev=0xffff000083223890) at
>> drivers/pinctrl/qcom/pinctrl-sc7180.c:1147
>> #12 0xffff80008089583c in platform_probe (_dev=0xffff000081406410) at
>> drivers/base/platform.c:1404
>>
>> (gdb) print gc->gpiodev
>> $1 = (struct gpio_device *) 0x0
>>
>> -Doug
> 

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

[Dmitry Baryshkov]

On Sun, Apr 13, 2025 at 11:51:29AM +0300, Matti Vaittinen wrote:
> On 13/04/2025 11:08, Matti Vaittinen wrote:
> > Hi Doug,
> > 
> > On 13/04/2025 02:00, Doug Anderson wrote:
> > > Hi,
> > > 
> > > On Wed, Mar 5, 2025 at 5:23 AM 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>
> > > > ---
> > > > Revision history:
> > > > v2 => v3:
> > > >   - Rebase to gpio/for-next
> > > > 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(-)
> > > 
> > > FWIW, I've found that this patch is crashing me at bootup on my
> > > sc7180-trogdor board. The problem is pretty obvious in gdb.
> > > "gc->gpiodev" is NULL in gpiochip_line_is_valid().
> > 
> > Thanks for debugging this! I find this odd. It seems to me the pinctrl-
> > msm.c is calling the gpiochip_add_data() for the chip, in the
> > msm_gpio_init() - which is called from the msm_pinctrl_probe().
> > 
> > The gpiochip_add_data() should go to the gpiochip_add_data_with_key() -
> > where the gpiodev should be allocated and set.
> > 
> > I don't spot any successful code path where the gpiodev was not allocated.
> > 
> > > 
> > > 0xffff80008066c760 in gpiochip_line_is_valid (gc=0xffff000083223890,
> > > offset=offset@entry=66) at drivers/gpio/gpiolib.c:746
> > > 746             if (likely(!gc->gpiodev->valid_mask))
> > > (gdb) bt
> > > #0  0xffff80008066c760 in gpiochip_line_is_valid
> > > (gc=0xffff000083223890, offset=offset@entry=66) at
> > > drivers/gpio/gpiolib.c:746
> > > #1  0xffff800080666338 in msm_pinmux_request (pctldev=<optimized out>,
> > 
> > Ah, but now I see the call comes from the pinmux. Looking at the
> > msm_pinctrl_probe() - the pincontroller is registered before the gpio.
> > Maybe, with unlucky timing, the request happens right after registering
> > the pinctrl - but before registering the gpios.
> > 
> > This, I think, can be a bug even before this change (because the
> > valid_mask is not initialized prior the gpio registration) - but this
> > change now made it obvious.
> > 
> > I see the probe is actually an exported function, and there are mentions
> > about ACPI support etc. I don't really know if there are valid cases
> > where the pincontroller should be usable without the gpiochip. If this
> > is the case, the unconditional call to the gpiochip_line_is_valid() from
> > the msm_pinmux_request() smells wrong.
> > 
> > I am not sure about the right fix. One could try:
> > 
> > @@ -1568,6 +1568,10 @@ int msm_pinctrl_probe(struct platform_device *pdev,
> >          if (pctrl->irq < 0)
> >                  return pctrl->irq;
> > 
> > +       ret = msm_gpio_init(pctrl);
> > +       if (ret)
> > +               return ret;
> > +
> >          pctrl->desc.owner = THIS_MODULE;
> >          pctrl->desc.pctlops = &msm_pinctrl_ops;
> >          pctrl->desc.pmxops = &msm_pinmux_ops;
> > @@ -1582,10 +1586,6 @@ int msm_pinctrl_probe(struct platform_device *pdev,
> >                  return PTR_ERR(pctrl->pctrl);
> >          }
> > 
> > -       ret = msm_gpio_init(pctrl);
> > -       if (ret)
> > -               return ret;
> > -
> >          platform_set_drvdata(pdev, pctrl);
> > 
> >          dev_dbg(&pdev->dev, "Probed Qualcomm pinctrl driver\n")
> > 
> > but I am not at all this is the fix we're looking after. I wonder if
> > Krzysztof has any suggestions? (Seeing he has been authoring some
> > changes here :] )

I think a correct fix for the pinctrl-msm driver would to use
devm_pinctrl_register_and_init() and then pinctrl_enable() after
registering GPIO chip, I'm going to submit a relevant patch. However I
can't stop but notice that pinctrl-msm is not unique in the pattern of
simply calling [devm_]pinctrl_register() and then registering a GPIO
chip. This patch makes this pattern much more fragile.

> 
> +Björn
> 
> > Yours,
> >      -- Matti
> > 
> > 
> > > offset=66) at drivers/pinctrl/qcom/pinctrl-msm.c:152
> > > #2  0xffff800080662314 in pin_request (pctldev=0xffff000082686ac0,
> > > pin=66, owner=0xffff000082c02790 "3500000.pinctrl", gpio_range=0x0)
> > >      at drivers/pinctrl/pinmux.c:176
> > > #3  0xffff800080662900 in pinmux_enable_setting
> > > (setting=0xffff000082684b40) at drivers/pinctrl/pinmux.c:445
> > > #4  0xffff80008065fd54 in pinctrl_commit_state (p=0xffff000083a07520,
> > > state=0xffff000082684a40) at drivers/pinctrl/core.c:1300
> > > #5  0xffff8000806605bc in pinctrl_select_state (p=0xffff000083223890,
> > > p@entry=0xffff000082686ac0, state=0x42) at drivers/pinctrl/core.c:1381
> > > #6  pinctrl_claim_hogs (pctldev=0xffff000082686ac0) at
> > > drivers/pinctrl/core.c:2136
> > > #7  pinctrl_enable (pctldev=0xffff000082686ac0) at drivers/pinctrl/
> > > core.c:2156
> > > #8  0xffff800080660814 in pinctrl_register
> > > (pctldesc=0xffff000083223a90, dev=0xffff000081406410,
> > > driver_data=0xffff000083223880) at drivers/pinctrl/core.c:2193
> > > #9  0xffff800080660df4 in devm_pinctrl_register
> > > (dev=0xffff000081406410, pctldesc=0xffff000083223a90,
> > > driver_data=0xffff000083223880) at drivers/pinctrl/core.c:2313
> > > #10 0xffff8000806657b4 in msm_pinctrl_probe (pdev=0xffff000081406400,
> > > soc_data=<optimized out>) at drivers/pinctrl/qcom/pinctrl-msm.c:1579
> > > #11 0xffff80008066afcc in sc7180_pinctrl_probe
> > > (pdev=0xffff000083223890) at
> > > drivers/pinctrl/qcom/pinctrl-sc7180.c:1147
> > > #12 0xffff80008089583c in platform_probe (_dev=0xffff000081406410) at
> > > drivers/base/platform.c:1404
> > > 
> > > (gdb) print gc->gpiodev
> > > $1 = (struct gpio_device *) 0x0
> > > 
> > > -Doug
> > 
> 

-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

On Sat, May 03, 2025 at 12:41:30AM +0300, Dmitry Baryshkov wrote:
> On Sun, Apr 13, 2025 at 11:51:29AM +0300, Matti Vaittinen wrote:
> > On 13/04/2025 11:08, Matti Vaittinen wrote:
> > > Hi Doug,
> > > 
> > > On 13/04/2025 02:00, Doug Anderson wrote:
> > > > Hi,
> > > > 
> > > > On Wed, Mar 5, 2025 at 5:23 AM 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>
> > > > > ---
> > > > > Revision history:
> > > > > v2 => v3:
> > > > >   - Rebase to gpio/for-next
> > > > > 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(-)
> > > > 
> > > > FWIW, I've found that this patch is crashing me at bootup on my
> > > > sc7180-trogdor board. The problem is pretty obvious in gdb.
> > > > "gc->gpiodev" is NULL in gpiochip_line_is_valid().
> > > 
> > > Thanks for debugging this! I find this odd. It seems to me the pinctrl-
> > > msm.c is calling the gpiochip_add_data() for the chip, in the
> > > msm_gpio_init() - which is called from the msm_pinctrl_probe().
> > > 
> > > The gpiochip_add_data() should go to the gpiochip_add_data_with_key() -
> > > where the gpiodev should be allocated and set.
> > > 
> > > I don't spot any successful code path where the gpiodev was not allocated.
> > > 
> > > > 
> > > > 0xffff80008066c760 in gpiochip_line_is_valid (gc=0xffff000083223890,
> > > > offset=offset@entry=66) at drivers/gpio/gpiolib.c:746
> > > > 746             if (likely(!gc->gpiodev->valid_mask))
> > > > (gdb) bt
> > > > #0  0xffff80008066c760 in gpiochip_line_is_valid
> > > > (gc=0xffff000083223890, offset=offset@entry=66) at
> > > > drivers/gpio/gpiolib.c:746
> > > > #1  0xffff800080666338 in msm_pinmux_request (pctldev=<optimized out>,
> > > 
> > > Ah, but now I see the call comes from the pinmux. Looking at the
> > > msm_pinctrl_probe() - the pincontroller is registered before the gpio.
> > > Maybe, with unlucky timing, the request happens right after registering
> > > the pinctrl - but before registering the gpios.
> > > 
> > > This, I think, can be a bug even before this change (because the
> > > valid_mask is not initialized prior the gpio registration) - but this
> > > change now made it obvious.
> > > 
> > > I see the probe is actually an exported function, and there are mentions
> > > about ACPI support etc. I don't really know if there are valid cases
> > > where the pincontroller should be usable without the gpiochip. If this
> > > is the case, the unconditional call to the gpiochip_line_is_valid() from
> > > the msm_pinmux_request() smells wrong.
> > > 
> > > I am not sure about the right fix. One could try:
> > > 
> > > @@ -1568,6 +1568,10 @@ int msm_pinctrl_probe(struct platform_device *pdev,
> > >          if (pctrl->irq < 0)
> > >                  return pctrl->irq;
> > > 
> > > +       ret = msm_gpio_init(pctrl);
> > > +       if (ret)
> > > +               return ret;
> > > +
> > >          pctrl->desc.owner = THIS_MODULE;
> > >          pctrl->desc.pctlops = &msm_pinctrl_ops;
> > >          pctrl->desc.pmxops = &msm_pinmux_ops;
> > > @@ -1582,10 +1586,6 @@ int msm_pinctrl_probe(struct platform_device *pdev,
> > >                  return PTR_ERR(pctrl->pctrl);
> > >          }
> > > 
> > > -       ret = msm_gpio_init(pctrl);
> > > -       if (ret)
> > > -               return ret;
> > > -
> > >          platform_set_drvdata(pdev, pctrl);
> > > 
> > >          dev_dbg(&pdev->dev, "Probed Qualcomm pinctrl driver\n")
> > > 
> > > but I am not at all this is the fix we're looking after. I wonder if
> > > Krzysztof has any suggestions? (Seeing he has been authoring some
> > > changes here :] )
> 
> I think a correct fix for the pinctrl-msm driver would to use
> devm_pinctrl_register_and_init() and then pinctrl_enable() after
> registering GPIO chip, I'm going to submit a relevant patch. However I
> can't stop but notice that pinctrl-msm is not unique in the pattern of
> simply calling [devm_]pinctrl_register() and then registering a GPIO
> chip. This patch makes this pattern much more fragile.

Unfortunately... my idea to split pinctrl_register() into _and_init()
and pinctrl_enable() calls didn't work. Internally GPIOchip reigstration
calls of_pinctrl_get(), gets NULL and fails gpiochip registration with
-EPROBE_DEFER.

> 
> > 
> > +Björn
> > 
> > > Yours,
> > >      -- Matti
> > > 
> > > 
> > > > offset=66) at drivers/pinctrl/qcom/pinctrl-msm.c:152
> > > > #2  0xffff800080662314 in pin_request (pctldev=0xffff000082686ac0,
> > > > pin=66, owner=0xffff000082c02790 "3500000.pinctrl", gpio_range=0x0)
> > > >      at drivers/pinctrl/pinmux.c:176
> > > > #3  0xffff800080662900 in pinmux_enable_setting
> > > > (setting=0xffff000082684b40) at drivers/pinctrl/pinmux.c:445
> > > > #4  0xffff80008065fd54 in pinctrl_commit_state (p=0xffff000083a07520,
> > > > state=0xffff000082684a40) at drivers/pinctrl/core.c:1300
> > > > #5  0xffff8000806605bc in pinctrl_select_state (p=0xffff000083223890,
> > > > p@entry=0xffff000082686ac0, state=0x42) at drivers/pinctrl/core.c:1381
> > > > #6  pinctrl_claim_hogs (pctldev=0xffff000082686ac0) at
> > > > drivers/pinctrl/core.c:2136
> > > > #7  pinctrl_enable (pctldev=0xffff000082686ac0) at drivers/pinctrl/
> > > > core.c:2156
> > > > #8  0xffff800080660814 in pinctrl_register
> > > > (pctldesc=0xffff000083223a90, dev=0xffff000081406410,
> > > > driver_data=0xffff000083223880) at drivers/pinctrl/core.c:2193
> > > > #9  0xffff800080660df4 in devm_pinctrl_register
> > > > (dev=0xffff000081406410, pctldesc=0xffff000083223a90,
> > > > driver_data=0xffff000083223880) at drivers/pinctrl/core.c:2313
> > > > #10 0xffff8000806657b4 in msm_pinctrl_probe (pdev=0xffff000081406400,
> > > > soc_data=<optimized out>) at drivers/pinctrl/qcom/pinctrl-msm.c:1579
> > > > #11 0xffff80008066afcc in sc7180_pinctrl_probe
> > > > (pdev=0xffff000083223890) at
> > > > drivers/pinctrl/qcom/pinctrl-sc7180.c:1147
> > > > #12 0xffff80008089583c in platform_probe (_dev=0xffff000081406410) at
> > > > drivers/base/platform.c:1404
> > > > 
> > > > (gdb) print gc->gpiodev
> > > > $1 = (struct gpio_device *) 0x0
> > > > 
> > > > -Doug
> > > 
> > 
> 
> -- 
> With best wishes
> Dmitry

-- 
With best wishes
Dmitry

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

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>


On Wed, 05 Mar 2025 15:11:59 +0200, Matti Vaittinen 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.
> 
> [...]

Applied, thanks!

[1/4] gpio: Respect valid_mask when requesting GPIOs
      commit: a501624864f3fd9ab785f1f674f48dca535b198c
[2/4] gpio: Add a valid_mask getter
      commit: f636d4f60ac477187a466a573f947731fa762059
[3/4] gpio: gpio-rcar: Drop direct use of valid_mask
      commit: 43b665c961a6468fa8416805ef71daa5e7a152e7
[4/4] gpio: Hide valid_mask from direct assignments
      commit: 8015443e24e76fac97268603e91c4793970ce657

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@linaro.org>