unexpected side effect of "gpiolib: override irq_enable/disable"

unexpected side effect of "gpiolib: override irq_enable/disable"

[Ludovic Desroches]

Hi,

Using next-20180912, my kernel hangs during the boot. Git bisect tell me
that the cause of my issue is the commit "gpiolib: override
irq_enable/disable"

I dug further and this patch can have some side effects. When booting, I
have an infinite loop when trying to enable a gpio irq. I don't know if
the pinctrl-at91 driver is the only one concerned or not.

The pattern leading to this issue is quite simple: we have several gpio
controllers sharing the same irq_chip structure. Installing the
irq_enable/irq_disable hook works well the first time. The second time,
since the irq_enable has been altered to use gpiochip_irq_enable,
this latest function will call itself again and again by calling
irq_enable.

I think it should be better to have one irq_chip structure per gpio
controller. I am going to do a patch for pinctrl-at91. Excepting if you
think it has to be solved in a different way.


Ludovic

unexpected side effect of "gpiolib: override irq_enable/disable"

[Hans Verkuil]

Hi Ludovic,

On 09/12/2018 04:58 PM, Ludovic Desroches wrote:
> Hi,
> 
> Using next-20180912, my kernel hangs during the boot. Git bisect tell me
> that the cause of my issue is the commit "gpiolib: override
> irq_enable/disable"
> 
> I dug further and this patch can have some side effects. When booting, I
> have an infinite loop when trying to enable a gpio irq. I don't know if
> the pinctrl-at91 driver is the only one concerned or not.
> 
> The pattern leading to this issue is quite simple: we have several gpio
> controllers sharing the same irq_chip structure. Installing the
> irq_enable/irq_disable hook works well the first time. The second time,
> since the irq_enable has been altered to use gpiochip_irq_enable,
> this latest function will call itself again and again by calling
> irq_enable.
> 
> I think it should be better to have one irq_chip structure per gpio
> controller. I am going to do a patch for pinctrl-at91. Excepting if you
> think it has to be solved in a different way.
> 
> 
> Ludovic
> 

Can you try this patch first?

Thanks!

	Hans

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index efce534a269b..e09e4e439928 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1859,6 +1859,8 @@ static void gpiochip_set_irq_hooks(struct gpio_chip *gpiochip)
 	}
 	if (WARN_ON(gpiochip->irq.irq_enable))
 		return;
+	if (irqchip->irq_enable == gpiochip_irq_enable)
+		return;
 	gpiochip->irq.irq_enable = irqchip->irq_enable;
 	gpiochip->irq.irq_disable = irqchip->irq_disable;
 	irqchip->irq_enable = gpiochip_irq_enable;

unexpected side effect of "gpiolib: override irq_enable/disable"

[Ludovic Desroches]

Hi Hans,

On Wed, Sep 12, 2018 at 05:30:35PM +0200, Hans Verkuil wrote:
> Hi Ludovic,
> 
> On 09/12/2018 04:58 PM, Ludovic Desroches wrote:
> > Hi,
> > 
> > Using next-20180912, my kernel hangs during the boot. Git bisect tell me
> > that the cause of my issue is the commit "gpiolib: override
> > irq_enable/disable"
> > 
> > I dug further and this patch can have some side effects. When booting, I
> > have an infinite loop when trying to enable a gpio irq. I don't know if
> > the pinctrl-at91 driver is the only one concerned or not.
> > 
> > The pattern leading to this issue is quite simple: we have several gpio
> > controllers sharing the same irq_chip structure. Installing the
> > irq_enable/irq_disable hook works well the first time. The second time,
> > since the irq_enable has been altered to use gpiochip_irq_enable,
> > this latest function will call itself again and again by calling
> > irq_enable.
> > 
> > I think it should be better to have one irq_chip structure per gpio
> > controller. I am going to do a patch for pinctrl-at91. Excepting if you
> > think it has to be solved in a different way.
> > 
> > 
> > Ludovic
> > 
> 
> Can you try this patch first?

Yes it works, I also have this kind of change in mind.

I am not sure which solution is the best. Of course I prefer this one as
I don't have to modify the pinctrl-at91 driver but if I have to modify
it to not share the same irq_chip structure, I'll handle it. Just let me
know.

Thanks

Regards

Ludovic

> 
> Thanks!
> 
> 	Hans
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index efce534a269b..e09e4e439928 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1859,6 +1859,8 @@ static void gpiochip_set_irq_hooks(struct gpio_chip *gpiochip)
>  	}
>  	if (WARN_ON(gpiochip->irq.irq_enable))
>  		return;
> +	if (irqchip->irq_enable == gpiochip_irq_enable)
> +		return;
>  	gpiochip->irq.irq_enable = irqchip->irq_enable;
>  	gpiochip->irq.irq_disable = irqchip->irq_disable;
>  	irqchip->irq_enable = gpiochip_irq_enable;

unexpected side effect of "gpiolib: override irq_enable/disable"

[Hans Verkuil]

On 09/13/18 09:47, Ludovic Desroches wrote:
> Hi Hans,
> 
> On Wed, Sep 12, 2018 at 05:30:35PM +0200, Hans Verkuil wrote:
>> Hi Ludovic,
>>
>> On 09/12/2018 04:58 PM, Ludovic Desroches wrote:
>>> Hi,
>>>
>>> Using next-20180912, my kernel hangs during the boot. Git bisect tell me
>>> that the cause of my issue is the commit "gpiolib: override
>>> irq_enable/disable"
>>>
>>> I dug further and this patch can have some side effects. When booting, I
>>> have an infinite loop when trying to enable a gpio irq. I don't know if
>>> the pinctrl-at91 driver is the only one concerned or not.
>>>
>>> The pattern leading to this issue is quite simple: we have several gpio
>>> controllers sharing the same irq_chip structure. Installing the
>>> irq_enable/irq_disable hook works well the first time. The second time,
>>> since the irq_enable has been altered to use gpiochip_irq_enable,
>>> this latest function will call itself again and again by calling
>>> irq_enable.
>>>
>>> I think it should be better to have one irq_chip structure per gpio
>>> controller. I am going to do a patch for pinctrl-at91. Excepting if you
>>> think it has to be solved in a different way.
>>>
>>>
>>> Ludovic
>>>
>>
>> Can you try this patch first?
> 
> Yes it works, I also have this kind of change in mind.

Good. I think this patch needs to go in regardless, since this makes it
a lot more robust. My main question is whether this situation is indicative
of a bad driver design, or if this is normal behavior that this code just
has to support.

> I am not sure which solution is the best. Of course I prefer this one as
> I don't have to modify the pinctrl-at91 driver but if I have to modify
> it to not share the same irq_chip structure, I'll handle it. Just let me
> know.

I tried to figure out where this happens exactly in this driver (sharing the
same irq_chip structure), but I'm not quite sure I fully understand it. Can
you point out where this happens?

One concern I have with the approach taken in this driver is that while placing
the hooks works fine (with this patch applied), but when the gpiochip that contains
the old callbacks for this irqchip is removed, the hooks are also removed for
all gpiochips using this irqchip. I don't think there is anything I can do
about that without adding a refcount or somthing in struct irq_chip.

So from that perspective it's better to have separate irq_chip structs. But
if there are (a lot?) more drivers doing this, than I need to look for a
better solution.

Regards,

	Hans

> 
> Thanks
> 
> Regards
> 
> Ludovic
> 
>>
>> Thanks!
>>
>> 	Hans
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> ---
>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>> index efce534a269b..e09e4e439928 100644
>> --- a/drivers/gpio/gpiolib.c
>> +++ b/drivers/gpio/gpiolib.c
>> @@ -1859,6 +1859,8 @@ static void gpiochip_set_irq_hooks(struct gpio_chip *gpiochip)
>>  	}
>>  	if (WARN_ON(gpiochip->irq.irq_enable))
>>  		return;
>> +	if (irqchip->irq_enable == gpiochip_irq_enable)
>> +		return;
>>  	gpiochip->irq.irq_enable = irqchip->irq_enable;
>>  	gpiochip->irq.irq_disable = irqchip->irq_disable;
>>  	irqchip->irq_enable = gpiochip_irq_enable;

unexpected side effect of "gpiolib: override irq_enable/disable"

[Ludovic Desroches]

On Thu, Sep 13, 2018 at 10:42:49AM +0200, Hans Verkuil wrote:
> On 09/13/18 09:47, Ludovic Desroches wrote:
> > Hi Hans,
> > 
> > On Wed, Sep 12, 2018 at 05:30:35PM +0200, Hans Verkuil wrote:
> >> Hi Ludovic,
> >>
> >> On 09/12/2018 04:58 PM, Ludovic Desroches wrote:
> >>> Hi,
> >>>
> >>> Using next-20180912, my kernel hangs during the boot. Git bisect tell me
> >>> that the cause of my issue is the commit "gpiolib: override
> >>> irq_enable/disable"
> >>>
> >>> I dug further and this patch can have some side effects. When booting, I
> >>> have an infinite loop when trying to enable a gpio irq. I don't know if
> >>> the pinctrl-at91 driver is the only one concerned or not.
> >>>
> >>> The pattern leading to this issue is quite simple: we have several gpio
> >>> controllers sharing the same irq_chip structure. Installing the
> >>> irq_enable/irq_disable hook works well the first time. The second time,
> >>> since the irq_enable has been altered to use gpiochip_irq_enable,
> >>> this latest function will call itself again and again by calling
> >>> irq_enable.
> >>>
> >>> I think it should be better to have one irq_chip structure per gpio
> >>> controller. I am going to do a patch for pinctrl-at91. Excepting if you
> >>> think it has to be solved in a different way.
> >>>
> >>>
> >>> Ludovic
> >>>
> >>
> >> Can you try this patch first?
> > 
> > Yes it works, I also have this kind of change in mind.
> 
> Good. I think this patch needs to go in regardless, since this makes it
> a lot more robust. My main question is whether this situation is indicative
> of a bad driver design, or if this is normal behavior that this code just
> has to support.
> 
> > I am not sure which solution is the best. Of course I prefer this one as
> > I don't have to modify the pinctrl-at91 driver but if I have to modify
> > it to not share the same irq_chip structure, I'll handle it. Just let me
> > know.
> 
> I tried to figure out where this happens exactly in this driver (sharing the
> same irq_chip structure), but I'm not quite sure I fully understand it. Can
> you point out where this happens?
> 

There are 5 instances of the gpio controller (A, B, C, D, E) and they all use
the same irqchip since the behaviour is common.

When adding A as a gpiochip_irqchip (in at91_gpio_of_irq_setup()), we set the
hooks so:
gpiochip->irq.irq_enable = irqchip->irq_enable (= NULL);
irqchip->irq_enable = gpiochip_irq_enable;

When adding B and others 'gpiochip_irqchip's, the hooks are set in this
way:
gpiochip->irq.irq_enable = irqchip->irq_enable (= gpiochip_irq_enable());
irqchip->irq_enable = gpiochip_irq_enable;

So in the hook we will call chip->irq.irq_enable()
(=gpiochip_irq_enable()).

I hope I am clear.

> One concern I have with the approach taken in this driver is that while placing
> the hooks works fine (with this patch applied), but when the gpiochip that contains
> the old callbacks for this irqchip is removed, the hooks are also removed for
> all gpiochips using this irqchip. I don't think there is anything I can do
> about that without adding a refcount or somthing in struct irq_chip.
> 
> So from that perspective it's better to have separate irq_chip structs. But
> if there are (a lot?) more drivers doing this, than I need to look for a
> better solution.

>From Linux answer, it seems there is another driver which shares the
irqchip among several instances.

Regards

Ludovic

unexpected side effect of "gpiolib: override irq_enable/disable"

[Hans Verkuil]

On 09/13/2018 11:02 AM, Ludovic Desroches wrote:
> On Thu, Sep 13, 2018 at 10:42:49AM +0200, Hans Verkuil wrote:
>> On 09/13/18 09:47, Ludovic Desroches wrote:
>>> Hi Hans,
>>>
>>> On Wed, Sep 12, 2018 at 05:30:35PM +0200, Hans Verkuil wrote:
>>>> Hi Ludovic,
>>>>
>>>> On 09/12/2018 04:58 PM, Ludovic Desroches wrote:
>>>>> Hi,
>>>>>
>>>>> Using next-20180912, my kernel hangs during the boot. Git bisect tell me
>>>>> that the cause of my issue is the commit "gpiolib: override
>>>>> irq_enable/disable"
>>>>>
>>>>> I dug further and this patch can have some side effects. When booting, I
>>>>> have an infinite loop when trying to enable a gpio irq. I don't know if
>>>>> the pinctrl-at91 driver is the only one concerned or not.
>>>>>
>>>>> The pattern leading to this issue is quite simple: we have several gpio
>>>>> controllers sharing the same irq_chip structure. Installing the
>>>>> irq_enable/irq_disable hook works well the first time. The second time,
>>>>> since the irq_enable has been altered to use gpiochip_irq_enable,
>>>>> this latest function will call itself again and again by calling
>>>>> irq_enable.
>>>>>
>>>>> I think it should be better to have one irq_chip structure per gpio
>>>>> controller. I am going to do a patch for pinctrl-at91. Excepting if you
>>>>> think it has to be solved in a different way.
>>>>>
>>>>>
>>>>> Ludovic
>>>>>
>>>>
>>>> Can you try this patch first?
>>>
>>> Yes it works, I also have this kind of change in mind.
>>
>> Good. I think this patch needs to go in regardless, since this makes it
>> a lot more robust. My main question is whether this situation is indicative
>> of a bad driver design, or if this is normal behavior that this code just
>> has to support.
>>
>>> I am not sure which solution is the best. Of course I prefer this one as
>>> I don't have to modify the pinctrl-at91 driver but if I have to modify
>>> it to not share the same irq_chip structure, I'll handle it. Just let me
>>> know.
>>
>> I tried to figure out where this happens exactly in this driver (sharing the
>> same irq_chip structure), but I'm not quite sure I fully understand it. Can
>> you point out where this happens?
>>
> 
> There are 5 instances of the gpio controller (A, B, C, D, E) and they all use
> the same irqchip since the behaviour is common.
> 
> When adding A as a gpiochip_irqchip (in at91_gpio_of_irq_setup()), we set the
> hooks so:

Ah, now I see: gpio_irqchip is a static struct. That's the bit I missed when I
looked at the code earlier.

Thanks,

	Hans

unexpected side effect of "gpiolib: override irq_enable/disable"

[Hans Verkuil]

(apologies if this is a repost, there was an email hickup when I sent it
the first time)

On 09/13/18 09:47, Ludovic Desroches wrote:
> Hi Hans,
> 
> On Wed, Sep 12, 2018 at 05:30:35PM +0200, Hans Verkuil wrote:
>> Hi Ludovic,
>>
>> On 09/12/2018 04:58 PM, Ludovic Desroches wrote:
>>> Hi,
>>>
>>> Using next-20180912, my kernel hangs during the boot. Git bisect tell me
>>> that the cause of my issue is the commit "gpiolib: override
>>> irq_enable/disable"
>>>
>>> I dug further and this patch can have some side effects. When booting, I
>>> have an infinite loop when trying to enable a gpio irq. I don't know if
>>> the pinctrl-at91 driver is the only one concerned or not.
>>>
>>> The pattern leading to this issue is quite simple: we have several gpio
>>> controllers sharing the same irq_chip structure. Installing the
>>> irq_enable/irq_disable hook works well the first time. The second time,
>>> since the irq_enable has been altered to use gpiochip_irq_enable,
>>> this latest function will call itself again and again by calling
>>> irq_enable.
>>>
>>> I think it should be better to have one irq_chip structure per gpio
>>> controller. I am going to do a patch for pinctrl-at91. Excepting if you
>>> think it has to be solved in a different way.
>>>
>>>
>>> Ludovic
>>>
>>
>> Can you try this patch first?
> 
> Yes it works, I also have this kind of change in mind.

Good. I think this patch needs to go in regardless, since this makes it
a lot more robust. My main question is whether this situation is indicative
of a bad driver design, or if this is normal behavior that this code just
has to support.

> I am not sure which solution is the best. Of course I prefer this one as
> I don't have to modify the pinctrl-at91 driver but if I have to modify
> it to not share the same irq_chip structure, I'll handle it. Just let me
> know.

I tried to figure out where this happens exactly in this driver (sharing the
same irq_chip structure), but I'm not quite sure I fully understand it. Can
you point out where this happens?

One concern I have with the approach taken in this driver is that while placing
the hooks works fine (with this patch), but when the gpiochip that contains
the old callbacks for this irqchip is removed, the hooks are also removed for
all gpiochips using this irqchip. I don't think there is anything I can do
about that without hacking struct irq_chip.

So from that perspective it's better to have separate irq_chip structs. But
if there are (a lot?) more drivers doing this, than I need to look for a
better solution.

Regards,

	Hans

> 
> Thanks
> 
> Regards
> 
> Ludovic
> 
>>
>> Thanks!
>>
>> 	Hans
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> ---
>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>> index efce534a269b..e09e4e439928 100644
>> --- a/drivers/gpio/gpiolib.c
>> +++ b/drivers/gpio/gpiolib.c
>> @@ -1859,6 +1859,8 @@ static void gpiochip_set_irq_hooks(struct gpio_chip *gpiochip)
>>  	}
>>  	if (WARN_ON(gpiochip->irq.irq_enable))
>>  		return;
>> +	if (irqchip->irq_enable == gpiochip_irq_enable)
>> +		return;
>>  	gpiochip->irq.irq_enable = irqchip->irq_enable;
>>  	gpiochip->irq.irq_disable = irqchip->irq_disable;
>>  	irqchip->irq_enable = gpiochip_irq_enable;

unexpected side effect of "gpiolib: override irq_enable/disable"

[Linus Walleij]

On Thu, Sep 13, 2018 at 11:07 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:

> > I am not sure which solution is the best. Of course I prefer this one as
> > I don't have to modify the pinctrl-at91 driver but if I have to modify
> > it to not share the same irq_chip structure, I'll handle it. Just let me
> > know.
(...)
> One concern I have with the approach taken in this driver is that while placing
> the hooks works fine (with this patch), but when the gpiochip that contains
> the old callbacks for this irqchip is removed, the hooks are also removed for
> all gpiochips using this irqchip. I don't think there is anything I can do
> about that without hacking struct irq_chip.
>
> So from that perspective it's better to have separate irq_chip structs. But
> if there are (a lot?) more drivers doing this, than I need to look for a
> better solution.

I think it's something like that before it was an OK practice to have
the same irqchip for multiple GPIO chips, but after this patch it
becomes a bad practice.

I do not think it is a big problem. Very few systems would even
consider randomly unloading their gpiochips, they are mostly
vital infrastructure, and we don't go building complex solutions
around theoretical problems.

I think what we should do is apply this fix, then annotate the
drivers that use the same irqchip with multiple gpiochips with
some
FIXME: this is bad practice
comments.

Yours,
Linus Walleij

unexpected side effect of "gpiolib: override irq_enable/disable"

[Linus Walleij]

On Thu, Sep 13, 2018 at 11:27 AM Linus Walleij <linus.walleij@linaro.org> wrote:

> I think what we should do is apply this fix, then annotate the
> drivers that use the same irqchip with multiple gpiochips with
> some
> FIXME: this is bad practice
> comments.

Bonus: if you can get the core to also print a little naggy
pr_info-level note to fix up the gpio driver, that would be great.

Yours,
Linus Walleij

unexpected side effect of "gpiolib: override irq_enable/disable"

[Hans Verkuil]

On 09/13/18 11:28, Linus Walleij wrote:
> On Thu, Sep 13, 2018 at 11:27 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> 
>> I think what we should do is apply this fix, then annotate the
>> drivers that use the same irqchip with multiple gpiochips with
>> some
>> FIXME: this is bad practice
>> comments.
> 
> Bonus: if you can get the core to also print a little naggy
> pr_info-level note to fix up the gpio driver, that would be great.

Let me see what I can do tomorrow. Jeez, you give 'em a hand and
they want the whole arm! :-)

Regards,

	Hans

unexpected side effect of "gpiolib: override irq_enable/disable"

[Linus Walleij]

On Wed, Sep 12, 2018 at 5:30 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:

> Can you try this patch first?
>
> Thanks!
>
>         Hans
>
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index efce534a269b..e09e4e439928 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1859,6 +1859,8 @@ static void gpiochip_set_irq_hooks(struct gpio_chip *gpiochip)
>         }
>         if (WARN_ON(gpiochip->irq.irq_enable))
>                 return;
> +       if (irqchip->irq_enable == gpiochip_irq_enable)
> +               return;

This or somthing like it (like keeping some cookie around to see
if we use the same irqchip) makes sense to me.

drivers/pinctrl/nomadik/* also use the same irqchip twice, and
I bet some more.

Looking forward to proper patch.

Yours,
Linus Walleij