[PATCH 1/1] pinctrl: stmfx: fix valid_mask init sequence

[PATCH 1/1] pinctrl: stmfx: fix valid_mask init sequence

[Amelie Delaunay]

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(-)

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) {
-- 
2.17.1

Re: [PATCH 1/1] pinctrl: stmfx: fix valid_mask init sequence

[Alexandre Torgue]

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) {
> 

Re: [PATCH 1/1] pinctrl: stmfx: fix valid_mask init sequence

[Linus Walleij]

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

Re: [PATCH 1/1] pinctrl: stmfx: fix valid_mask init sequence

[Amelie DELAUNAY]

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

Re: [PATCH 1/1] pinctrl: stmfx: fix valid_mask init sequence

[Linus Walleij]

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

Re: [PATCH 1/1] pinctrl: stmfx: fix valid_mask init sequence

[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

Re: [PATCH 1/1] pinctrl: stmfx: fix valid_mask init sequence

[Amelie DELAUNAY]

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