| Message ID | 4547ca90d910d60cab3d56d864d59ddde47a5e93.1741180097.git.mazziesaccount@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: Hide and obey valid_mask | expand |
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
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
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 >
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