| Message ID | 20191104100908.10880-1-amelie.delaunay@st.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] pinctrl: stmfx: fix valid_mask init sequence | expand |
Hi Amélie, On 11/4/19 11:09 AM, Amelie Delaunay wrote: > With stmfx_pinctrl_gpio_init_valid_mask callback, gpio_valid_mask was used > to initialize gpiochip valid_mask for gpiolib. But gpio_valid_mask was not > yet initialized. gpio_valid_mask required gpio-ranges to be registered, > this is the case after gpiochip_add_data call. But init_valid_mask > callback is also called under gpiochip_add_data. gpio_valid_mask > initialization cannot be moved before gpiochip_add_data because > gpio-ranges are not registered. > So, it is not possible to use init_valid_mask callback. > To avoid this issue, get rid of valid_mask and rely on ranges. > > Fixes: da9b142ab2c5 ("pinctrl: stmfx: Use the callback to populate valid_mask") > Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com> Acked-by: Alexandre TORGUE <alexandre.torgue@st.com> > --- > drivers/pinctrl/pinctrl-stmfx.c | 14 -------------- > 1 file changed, 14 deletions(-) > > diff --git a/drivers/pinctrl/pinctrl-stmfx.c b/drivers/pinctrl/pinctrl-stmfx.c > index 564660028fcc..ccdf0bb21414 100644 > --- a/drivers/pinctrl/pinctrl-stmfx.c > +++ b/drivers/pinctrl/pinctrl-stmfx.c > @@ -585,19 +585,6 @@ static int stmfx_pinctrl_gpio_function_enable(struct stmfx_pinctrl *pctl) > return stmfx_function_enable(pctl->stmfx, func); > } > > -static int stmfx_pinctrl_gpio_init_valid_mask(struct gpio_chip *gc, > - unsigned long *valid_mask, > - unsigned int ngpios) > -{ > - struct stmfx_pinctrl *pctl = gpiochip_get_data(gc); > - u32 n; > - > - for_each_clear_bit(n, &pctl->gpio_valid_mask, ngpios) > - clear_bit(n, valid_mask); > - > - return 0; > -} > - > static int stmfx_pinctrl_probe(struct platform_device *pdev) > { > struct stmfx *stmfx = dev_get_drvdata(pdev->dev.parent); > @@ -660,7 +647,6 @@ static int stmfx_pinctrl_probe(struct platform_device *pdev) > pctl->gpio_chip.ngpio = pctl->pctl_desc.npins; > pctl->gpio_chip.can_sleep = true; > pctl->gpio_chip.of_node = np; > - pctl->gpio_chip.init_valid_mask = stmfx_pinctrl_gpio_init_valid_mask; > > ret = devm_gpiochip_add_data(pctl->dev, &pctl->gpio_chip, pctl); > if (ret) { >
On Mon, Nov 4, 2019 at 11:09 AM Amelie Delaunay <amelie.delaunay@st.com> wrote: > With stmfx_pinctrl_gpio_init_valid_mask callback, gpio_valid_mask was used > to initialize gpiochip valid_mask for gpiolib. But gpio_valid_mask was not > yet initialized. gpio_valid_mask required gpio-ranges to be registered, > this is the case after gpiochip_add_data call. But init_valid_mask > callback is also called under gpiochip_add_data. gpio_valid_mask > initialization cannot be moved before gpiochip_add_data because > gpio-ranges are not registered. Sorry but this doesn't add up, look at this call graph: gpiochip_add_data() gpiochip_add_data_with_key() gpiochip_alloc_valid_mask() of_gpiochip_add() of_gpiochip_add_pin_range() gpiochip_init_valid_mask() So the .initi_valid_mask() is clearly called *after* of_gpiochip_add_pin_range() so this cannot be the real reason, provided that the ranges come from the device tree. AFAICT that is the case with the stmfx. Can you check and see if the problem is something else? Yours, Linus Walleij
On 11/5/19 3:32 PM, Linus Walleij wrote: > On Mon, Nov 4, 2019 at 11:09 AM Amelie Delaunay <amelie.delaunay@st.com> > wrote: > >> With stmfx_pinctrl_gpio_init_valid_mask callback, gpio_valid_mask was used >> to initialize gpiochip valid_mask for gpiolib. But gpio_valid_mask was not >> yet initialized. gpio_valid_mask required gpio-ranges to be registered, >> this is the case after gpiochip_add_data call. But init_valid_mask >> callback is also called under gpiochip_add_data. gpio_valid_mask >> initialization cannot be moved before gpiochip_add_data because >> gpio-ranges are not registered. > > Sorry but this doesn't add up, look at this call graph: > > gpiochip_add_data() > gpiochip_add_data_with_key() > gpiochip_alloc_valid_mask() > of_gpiochip_add() > of_gpiochip_add_pin_range() > gpiochip_init_valid_mask() > > So the .initi_valid_mask() is clearly called *after* > of_gpiochip_add_pin_range() so this cannot be the real reason, > provided that the ranges come from the device tree. AFAICT that > is the case with the stmfx. > > Can you check and see if the problem is something else? > stmfx_pinctrl_gpio_init_valid_mask uses pctl->gpio_valid_mask to initialize gpiochip valid_mask. pctl->gpio_valid_mask is initialized in stmfx_pinctrl_gpio_function_enable depending on gpio ranges. stmfx_pinctrl_gpio_function_enable is called after gpiochip_add_data because it requires gpio ranges to be registered. So, in stmfx driver the call graph is stmfx_pinctrl_probe gpiochip_add_data() gpiochip_add_data_with_key() gpiochip_alloc_valid_mask() of_gpiochip_add() of_gpiochip_add_pin_range() gpiochip_init_valid_mask() stmfx_pinctrl_gpio_init_valid_mask (but pctl->gpio_valid_mask is not yet initialized so gpiochip valid_mask is wrong) stmfx_pinctrl_gpio_function_enable (pctl->gpio_valid_mask is going to be initialized thanks to gpio ranges) When consumer tries to take a pin (it is the case for the joystick on stm32mp157c-ev1), it gets the following issue: [ 3.347391] irq: :soc:i2c@40013000:stmfx@42:stmfx-pin-controller didn't like hwirq-0x0 to VIRQ92 mapping (rc=-6) [ 3.356418] irq: :soc:i2c@40013000:stmfx@42:stmfx-pin-controller didn't like hwirq-0x1 to VIRQ92 mapping (rc=-6) [ 3.366512] irq: :soc:i2c@40013000:stmfx@42:stmfx-pin-controller didn't like hwirq-0x2 to VIRQ92 mapping (rc=-6) [ 3.376671] irq: :soc:i2c@40013000:stmfx@42:stmfx-pin-controller didn't like hwirq-0x3 to VIRQ92 mapping (rc=-6) [ 3.387169] irq: :soc:i2c@40013000:stmfx@42:stmfx-pin-controller didn't like hwirq-0x4 to VIRQ92 mapping (rc=-6) [ 3.397065] gpio-keys joystick: Found button without gpio or irq [ 3.403041] gpio-keys: probe of joystick failed with error -22 I can reword the commit message to make it clearer. Regards, Amelie
On Mon, Nov 4, 2019 at 11:09 AM Amelie Delaunay <amelie.delaunay@st.com> wrote: > With stmfx_pinctrl_gpio_init_valid_mask callback, gpio_valid_mask was used > to initialize gpiochip valid_mask for gpiolib. But gpio_valid_mask was not > yet initialized. gpio_valid_mask required gpio-ranges to be registered, > this is the case after gpiochip_add_data call. But init_valid_mask > callback is also called under gpiochip_add_data. gpio_valid_mask > initialization cannot be moved before gpiochip_add_data because > gpio-ranges are not registered. > So, it is not possible to use init_valid_mask callback. > To avoid this issue, get rid of valid_mask and rely on ranges. > > Fixes: da9b142ab2c5 ("pinctrl: stmfx: Use the callback to populate valid_mask") > Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com> Patch applied for fixes. Yours, Linus Walleij
On Tue, Nov 5, 2019 at 4:14 PM Amelie DELAUNAY <amelie.delaunay@st.com> wrote: > On 11/5/19 3:32 PM, Linus Walleij wrote: > > On Mon, Nov 4, 2019 at 11:09 AM Amelie Delaunay <amelie.delaunay@st.com> > > wrote: > > > >> With stmfx_pinctrl_gpio_init_valid_mask callback, gpio_valid_mask was used > >> to initialize gpiochip valid_mask for gpiolib. But gpio_valid_mask was not > >> yet initialized. gpio_valid_mask required gpio-ranges to be registered, > >> this is the case after gpiochip_add_data call. But init_valid_mask > >> callback is also called under gpiochip_add_data. gpio_valid_mask > >> initialization cannot be moved before gpiochip_add_data because > >> gpio-ranges are not registered. > > > > Sorry but this doesn't add up, look at this call graph: > > > > gpiochip_add_data() > > gpiochip_add_data_with_key() > > gpiochip_alloc_valid_mask() > > of_gpiochip_add() > > of_gpiochip_add_pin_range() > > gpiochip_init_valid_mask() > > > > So the .initi_valid_mask() is clearly called *after* > > of_gpiochip_add_pin_range() so this cannot be the real reason, > > provided that the ranges come from the device tree. AFAICT that > > is the case with the stmfx. > > > > Can you check and see if the problem is something else? > > > > stmfx_pinctrl_gpio_init_valid_mask uses pctl->gpio_valid_mask to > initialize gpiochip valid_mask. > > pctl->gpio_valid_mask is initialized in > stmfx_pinctrl_gpio_function_enable depending on gpio ranges. > > stmfx_pinctrl_gpio_function_enable is called after gpiochip_add_data > because it requires gpio ranges to be registered. > > So, in stmfx driver the call graph is > > stmfx_pinctrl_probe > gpiochip_add_data() > gpiochip_add_data_with_key() > gpiochip_alloc_valid_mask() > of_gpiochip_add() > of_gpiochip_add_pin_range() > gpiochip_init_valid_mask() > stmfx_pinctrl_gpio_init_valid_mask (but pctl->gpio_valid_mask > is not yet initialized so gpiochip valid_mask is wrong) > stmfx_pinctrl_gpio_function_enable (pctl->gpio_valid_mask is going to > be initialized thanks to gpio ranges) > > When consumer tries to take a pin (it is the case for the joystick on > stm32mp157c-ev1), it gets the following issue: > [ 3.347391] irq: :soc:i2c@40013000:stmfx@42:stmfx-pin-controller > didn't like hwirq-0x0 to VIRQ92 mapping (rc=-6) > [ 3.356418] irq: :soc:i2c@40013000:stmfx@42:stmfx-pin-controller > didn't like hwirq-0x1 to VIRQ92 mapping (rc=-6) > [ 3.366512] irq: :soc:i2c@40013000:stmfx@42:stmfx-pin-controller > didn't like hwirq-0x2 to VIRQ92 mapping (rc=-6) > [ 3.376671] irq: :soc:i2c@40013000:stmfx@42:stmfx-pin-controller > didn't like hwirq-0x3 to VIRQ92 mapping (rc=-6) > [ 3.387169] irq: :soc:i2c@40013000:stmfx@42:stmfx-pin-controller > didn't like hwirq-0x4 to VIRQ92 mapping (rc=-6) > [ 3.397065] gpio-keys joystick: Found button without gpio or irq > [ 3.403041] gpio-keys: probe of joystick failed with error -22 > > I can reword the commit message to make it clearer. No need I understand it now, thanks for explaining! We need to populate the valid mask some other way if you want to safeguard this, I don't know if the existing gpio-reserved-ranges would work? But it feels a bit unsafe if you actually determine this some other way. Yours, Linus Walleij
On 11/7/19 10:09 AM, Linus Walleij wrote: > On Tue, Nov 5, 2019 at 4:14 PM Amelie DELAUNAY <amelie.delaunay@st.com> wrote: >> On 11/5/19 3:32 PM, Linus Walleij wrote: >>> On Mon, Nov 4, 2019 at 11:09 AM Amelie Delaunay <amelie.delaunay@st.com> >>> wrote: >>> >>>> With stmfx_pinctrl_gpio_init_valid_mask callback, gpio_valid_mask was used >>>> to initialize gpiochip valid_mask for gpiolib. But gpio_valid_mask was not >>>> yet initialized. gpio_valid_mask required gpio-ranges to be registered, >>>> this is the case after gpiochip_add_data call. But init_valid_mask >>>> callback is also called under gpiochip_add_data. gpio_valid_mask >>>> initialization cannot be moved before gpiochip_add_data because >>>> gpio-ranges are not registered. >>> >>> Sorry but this doesn't add up, look at this call graph: >>> >>> gpiochip_add_data() >>> gpiochip_add_data_with_key() >>> gpiochip_alloc_valid_mask() >>> of_gpiochip_add() >>> of_gpiochip_add_pin_range() >>> gpiochip_init_valid_mask() >>> >>> So the .initi_valid_mask() is clearly called *after* >>> of_gpiochip_add_pin_range() so this cannot be the real reason, >>> provided that the ranges come from the device tree. AFAICT that >>> is the case with the stmfx. >>> >>> Can you check and see if the problem is something else? >>> >> >> stmfx_pinctrl_gpio_init_valid_mask uses pctl->gpio_valid_mask to >> initialize gpiochip valid_mask. >> >> pctl->gpio_valid_mask is initialized in >> stmfx_pinctrl_gpio_function_enable depending on gpio ranges. >> >> stmfx_pinctrl_gpio_function_enable is called after gpiochip_add_data >> because it requires gpio ranges to be registered. >> >> So, in stmfx driver the call graph is >> >> stmfx_pinctrl_probe >> gpiochip_add_data() >> gpiochip_add_data_with_key() >> gpiochip_alloc_valid_mask() >> of_gpiochip_add() >> of_gpiochip_add_pin_range() >> gpiochip_init_valid_mask() >> stmfx_pinctrl_gpio_init_valid_mask (but pctl->gpio_valid_mask >> is not yet initialized so gpiochip valid_mask is wrong) >> stmfx_pinctrl_gpio_function_enable (pctl->gpio_valid_mask is going to >> be initialized thanks to gpio ranges) >> >> When consumer tries to take a pin (it is the case for the joystick on >> stm32mp157c-ev1), it gets the following issue: >> [ 3.347391] irq: :soc:i2c@40013000:stmfx@42:stmfx-pin-controller >> didn't like hwirq-0x0 to VIRQ92 mapping (rc=-6) >> [ 3.356418] irq: :soc:i2c@40013000:stmfx@42:stmfx-pin-controller >> didn't like hwirq-0x1 to VIRQ92 mapping (rc=-6) >> [ 3.366512] irq: :soc:i2c@40013000:stmfx@42:stmfx-pin-controller >> didn't like hwirq-0x2 to VIRQ92 mapping (rc=-6) >> [ 3.376671] irq: :soc:i2c@40013000:stmfx@42:stmfx-pin-controller >> didn't like hwirq-0x3 to VIRQ92 mapping (rc=-6) >> [ 3.387169] irq: :soc:i2c@40013000:stmfx@42:stmfx-pin-controller >> didn't like hwirq-0x4 to VIRQ92 mapping (rc=-6) >> [ 3.397065] gpio-keys joystick: Found button without gpio or irq >> [ 3.403041] gpio-keys: probe of joystick failed with error -22 >> >> I can reword the commit message to make it clearer. > > No need I understand it now, thanks for explaining! > > We need to populate the valid mask some other way if you > want to safeguard this, I don't know if the existing > gpio-reserved-ranges would work? But it feels a bit unsafe > if you actually determine this some other way. > Before this patch, I made a "draft" version using the gpio-reserved-ranges property but then I had to use gpiochip_line_is_valid in pinconf_get/_set/_dbg_show in addition of pinctrl_find_gpio_range_from_pin... With an update of the bindings for optional property gpio-reserved-ranges. I was not really fond of this solution, it sounded redundant. Thanks for applying the patch. Regards, Amelie
diff --git a/drivers/pinctrl/pinctrl-stmfx.c b/drivers/pinctrl/pinctrl-stmfx.c index 564660028fcc..ccdf0bb21414 100644 --- a/drivers/pinctrl/pinctrl-stmfx.c +++ b/drivers/pinctrl/pinctrl-stmfx.c @@ -585,19 +585,6 @@ static int stmfx_pinctrl_gpio_function_enable(struct stmfx_pinctrl *pctl) return stmfx_function_enable(pctl->stmfx, func); } -static int stmfx_pinctrl_gpio_init_valid_mask(struct gpio_chip *gc, - unsigned long *valid_mask, - unsigned int ngpios) -{ - struct stmfx_pinctrl *pctl = gpiochip_get_data(gc); - u32 n; - - for_each_clear_bit(n, &pctl->gpio_valid_mask, ngpios) - clear_bit(n, valid_mask); - - return 0; -} - static int stmfx_pinctrl_probe(struct platform_device *pdev) { struct stmfx *stmfx = dev_get_drvdata(pdev->dev.parent); @@ -660,7 +647,6 @@ static int stmfx_pinctrl_probe(struct platform_device *pdev) pctl->gpio_chip.ngpio = pctl->pctl_desc.npins; pctl->gpio_chip.can_sleep = true; pctl->gpio_chip.of_node = np; - pctl->gpio_chip.init_valid_mask = stmfx_pinctrl_gpio_init_valid_mask; ret = devm_gpiochip_add_data(pctl->dev, &pctl->gpio_chip, pctl); if (ret) {
With stmfx_pinctrl_gpio_init_valid_mask callback, gpio_valid_mask was used to initialize gpiochip valid_mask for gpiolib. But gpio_valid_mask was not yet initialized. gpio_valid_mask required gpio-ranges to be registered, this is the case after gpiochip_add_data call. But init_valid_mask callback is also called under gpiochip_add_data. gpio_valid_mask initialization cannot be moved before gpiochip_add_data because gpio-ranges are not registered. So, it is not possible to use init_valid_mask callback. To avoid this issue, get rid of valid_mask and rely on ranges. Fixes: da9b142ab2c5 ("pinctrl: stmfx: Use the callback to populate valid_mask") Signed-off-by: Amelie Delaunay <amelie.delaunay@st.com> --- drivers/pinctrl/pinctrl-stmfx.c | 14 -------------- 1 file changed, 14 deletions(-)