[v2] gpiolib: Fix GPIO chip IRQ initialization restriction

  In case of gpio-regmap, IRQ chip is added by regmap-irq and associated with
GPIO chip by gpiochip_irqchip_add_domain(). The initialization flag was not
added in gpiochip_irqchip_add_domain(), causing gpiochip_to_irq() to return
-EPROBE_DEFER.

Fixes: 5467801f1fcb ("gpio: Restrict usage of GPIO chip irq members before initialization")
Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
---
v1 -> v2:
- add compiler barrier
---
 drivers/gpio/gpiolib.c | 8 ++++++++
 1 file changed, 8 insertions(+)
  

+Cc: Michael

On Wed, Jun 7, 2023 at 11:20 AM Jiawen Wu <jiawenwu@trustnetic.com> wrote:
>
> In case of gpio-regmap, IRQ chip is added by regmap-irq and associated with
> GPIO chip by gpiochip_irqchip_add_domain(). The initialization flag was not
> added in gpiochip_irqchip_add_domain(), causing gpiochip_to_irq() to return
> -EPROBE_DEFER.

Makes sense to me.
FWIW,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

But it would be nice to hear from Michael about this.

> Fixes: 5467801f1fcb ("gpio: Restrict usage of GPIO chip irq members before initialization")
> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
> ---
> v1 -> v2:
> - add compiler barrier
> ---
>  drivers/gpio/gpiolib.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index a7220e04a93e..9ecf93cbd801 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1792,6 +1792,14 @@ int gpiochip_irqchip_add_domain(struct gpio_chip *gc,
>         gc->to_irq = gpiochip_to_irq;
>         gc->irq.domain = domain;
>
> +       /*
> +        * Using barrier() here to prevent compiler from reordering
> +        * gc->irq.initialized before adding irqdomain.
> +        */
> +       barrier();
> +
> +       gc->irq.initialized = true;
> +
>         return 0;
>  }
>  EXPORT_SYMBOL_GPL(gpiochip_irqchip_add_domain);
> --
> 2.27.0
>
  

On Wed, Jun 7, 2023 at 10:20 AM Jiawen Wu <jiawenwu@trustnetic.com> wrote:

> In case of gpio-regmap, IRQ chip is added by regmap-irq and associated with
> GPIO chip by gpiochip_irqchip_add_domain(). The initialization flag was not
> added in gpiochip_irqchip_add_domain(), causing gpiochip_to_irq() to return
> -EPROBE_DEFER.
>
> Fixes: 5467801f1fcb ("gpio: Restrict usage of GPIO chip irq members before initialization")
> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>

Looks correct.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours.
Linus Walleij
  

On Wed, Jun 7, 2023 at 10:20 AM Jiawen Wu <jiawenwu@trustnetic.com> wrote:
>
> In case of gpio-regmap, IRQ chip is added by regmap-irq and associated with
> GPIO chip by gpiochip_irqchip_add_domain(). The initialization flag was not
> added in gpiochip_irqchip_add_domain(), causing gpiochip_to_irq() to return
> -EPROBE_DEFER.
>
> Fixes: 5467801f1fcb ("gpio: Restrict usage of GPIO chip irq members before initialization")
> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
> ---
> v1 -> v2:
> - add compiler barrier
> ---
>  drivers/gpio/gpiolib.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index a7220e04a93e..9ecf93cbd801 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1792,6 +1792,14 @@ int gpiochip_irqchip_add_domain(struct gpio_chip *gc,
>         gc->to_irq = gpiochip_to_irq;
>         gc->irq.domain = domain;
>
> +       /*
> +        * Using barrier() here to prevent compiler from reordering
> +        * gc->irq.initialized before adding irqdomain.
> +        */
> +       barrier();
> +
> +       gc->irq.initialized = true;
> +
>         return 0;
>  }
>  EXPORT_SYMBOL_GPL(gpiochip_irqchip_add_domain);
> --
> 2.27.0
>

Applied, thanks!

Bart
  

Am 2023-06-07 16:12, schrieb Andy Shevchenko:
> +Cc: Michael
> 
> On Wed, Jun 7, 2023 at 11:20 AM Jiawen Wu <jiawenwu@trustnetic.com> 
> wrote:
>> 
>> In case of gpio-regmap, IRQ chip is added by regmap-irq and associated 
>> with
>> GPIO chip by gpiochip_irqchip_add_domain(). The initialization flag 
>> was not
>> added in gpiochip_irqchip_add_domain(), causing gpiochip_to_irq() to 
>> return
>> -EPROBE_DEFER.
> 
> Makes sense to me.
> FWIW,
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 
> But it would be nice to hear from Michael about this.

Thanks for bringing this to my attention. In fact, currently
my sl28cpld is broken due to this. So:

Reviewed-by: Michael Walle <michael@walle.cc>
Tested-by: Michael Walle <michael@walle.cc> # on kontron-sl28

>> Fixes: 5467801f1fcb ("gpio: Restrict usage of GPIO chip irq members 
>> before initialization")
>> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>

-michael
  

On 15/06/23 14:56, Michael Walle wrote:
> Am 2023-06-07 16:12, schrieb Andy Shevchenko:
>> +Cc: Michael
>>
>> On Wed, Jun 7, 2023 at 11:20 AM Jiawen Wu <jiawenwu@trustnetic.com> 
>> wrote:
>>>
>>> In case of gpio-regmap, IRQ chip is added by regmap-irq and 
>>> associated with
>>> GPIO chip by gpiochip_irqchip_add_domain(). The initialization flag 
>>> was not
>>> added in gpiochip_irqchip_add_domain(), causing gpiochip_to_irq() to 
>>> return
>>> -EPROBE_DEFER.
>>
>> Makes sense to me.
>> FWIW,
>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>>
>> But it would be nice to hear from Michael about this.
>
> Thanks for bringing this to my attention. In fact, currently
> my sl28cpld is broken due to this. So:
>

Sorry about your sl28cpld.
Seems like I ended up breaking other boards while fixing this issue for 
steam deck :(


Regards,
Shreeya Patel


> Reviewed-by: Michael Walle <michael@walle.cc>
> Tested-by: Michael Walle <michael@walle.cc> # on kontron-sl28
>
>>> Fixes: 5467801f1fcb ("gpio: Restrict usage of GPIO chip irq members 
>>> before initialization")
>>> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
>
> -michael
  

On Thursday, June 15, 2023 5:26 PM, Michael Walle wrote:
> Am 2023-06-07 16:12, schrieb Andy Shevchenko:
> > +Cc: Michael
> >
> > On Wed, Jun 7, 2023 at 11:20 AM Jiawen Wu <jiawenwu@trustnetic.com>
> > wrote:
> >>
> >> In case of gpio-regmap, IRQ chip is added by regmap-irq and associated
> >> with
> >> GPIO chip by gpiochip_irqchip_add_domain(). The initialization flag
> >> was not
> >> added in gpiochip_irqchip_add_domain(), causing gpiochip_to_irq() to
> >> return
> >> -EPROBE_DEFER.
> >
> > Makes sense to me.
> > FWIW,
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> >
> > But it would be nice to hear from Michael about this.
> 
> Thanks for bringing this to my attention. In fact, currently
> my sl28cpld is broken due to this. So:

Thanks for your test, it's exciting for me to actually fix a bug.

BTW, I wonder if it has problems when unregistering gpio-regmap.
Call Trace of irq_domain_remove() always exits in my test:
https://lore.kernel.org/all/011c01d98d3d$99e6c6e0$cdb454a0$@trustnetic.com/

Of course, it could be because there was something wrong with my
test code. But I want to be clear about this.

> 
> Reviewed-by: Michael Walle <michael@walle.cc>
> Tested-by: Michael Walle <michael@walle.cc> # on kontron-sl28
> 
> >> Fixes: 5467801f1fcb ("gpio: Restrict usage of GPIO chip irq members
> >> before initialization")
> >> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
> 
> -michael
>
  

Hi,

> BTW, I wonder if it has problems when unregistering gpio-regmap.
> Call Trace of irq_domain_remove() always exits in my test:
> https://lore.kernel.org/all/011c01d98d3d$99e6c6e0$cdb454a0$@trustnetic.com/
> 
> Of course, it could be because there was something wrong with my
> test code. But I want to be clear about this.

Mh, you've said you don't use the devm_ variant of 
regmap_add_irq_chip(),
correct? Do you call regmap_del_irq_chip() yourself?

It seems that gpiolib is already removing the domain itself. Mh.
I guess if the the domain is set via gpiochip_irqchip_add_domain()
gpiolib must not call irq_domain_remove() because the domain resource
is handled externally (i.e. gpiolib doesn't allocate the domain
itself) in our case.

Nice finding! Looks like it has been broken since the beginning
when I've introduced the gpiochip_irqchip_add_domain(). Will you
do another fixes patch for that? I'm not sure where to store
that information though. Maybe a new bool "no_domain_free"
in struct gpio_irq_chip?

-michael
  

On Thu, Jun 15, 2023 at 1:45 PM Michael Walle <michael@walle.cc> wrote:
> > BTW, I wonder if it has problems when unregistering gpio-regmap.
> > Call Trace of irq_domain_remove() always exits in my test:
> > https://lore.kernel.org/all/011c01d98d3d$99e6c6e0$cdb454a0$@trustnetic.com/
> >
> > Of course, it could be because there was something wrong with my
> > test code. But I want to be clear about this.
>
> Mh, you've said you don't use the devm_ variant of
> regmap_add_irq_chip(),
> correct? Do you call regmap_del_irq_chip() yourself?
>
> It seems that gpiolib is already removing the domain itself. Mh.
> I guess if the the domain is set via gpiochip_irqchip_add_domain()
> gpiolib must not call irq_domain_remove() because the domain resource
> is handled externally (i.e. gpiolib doesn't allocate the domain
> itself) in our case.
>
> Nice finding! Looks like it has been broken since the beginning
> when I've introduced the gpiochip_irqchip_add_domain(). Will you
> do another fixes patch for that? I'm not sure where to store
> that information though. Maybe a new bool "no_domain_free"
> in struct gpio_irq_chip?

While reading this I also thought about flag, but please use positive
notation, e.g. "irq_domain_is_ext".
  

On Thursday, June 15, 2023 9:56 PM, Andy Shevchenko wrote:
> On Thu, Jun 15, 2023 at 1:45 PM Michael Walle <michael@walle.cc> wrote:
> > > BTW, I wonder if it has problems when unregistering gpio-regmap.
> > > Call Trace of irq_domain_remove() always exits in my test:
> > > https://lore.kernel.org/all/011c01d98d3d$99e6c6e0$cdb454a0$@trustnetic.com/
> > >
> > > Of course, it could be because there was something wrong with my
> > > test code. But I want to be clear about this.
> >
> > Mh, you've said you don't use the devm_ variant of
> > regmap_add_irq_chip(),
> > correct? Do you call regmap_del_irq_chip() yourself?

Yes, devm_regmap_del_irq_chip() also led to a call trace. I thought it
might be the order of release, so I called it myself without devm_.

> > It seems that gpiolib is already removing the domain itself. Mh.
> > I guess if the the domain is set via gpiochip_irqchip_add_domain()
> > gpiolib must not call irq_domain_remove() because the domain resource
> > is handled externally (i.e. gpiolib doesn't allocate the domain
> > itself) in our case.
> >
> > Nice finding! Looks like it has been broken since the beginning
> > when I've introduced the gpiochip_irqchip_add_domain(). Will you
> > do another fixes patch for that? 

I used to be rough at fixing in my test, I tried to set gc->irq.domain = NULL
after calling irq_domain_remove() in gpiochip_irqchip_remove(). But
there seemed to be some other issue that was causing my device to not
work, so I didn't go further. I wonder what risks such fix introduces.

Sorry I may not be able to do the fix patch for a while. I'm working on
other patches, this test will disrupt my work.

> > I'm not sure where to store
> > that information though. Maybe a new bool "no_domain_free"
> > in struct gpio_irq_chip?
> 
> While reading this I also thought about flag, but please use positive
> notation, e.g. "irq_domain_is_ext".
  

> I used to be rough at fixing in my test, I tried to set gc->irq.domain = NULL
> after calling irq_domain_remove() in gpiochip_irqchip_remove().

I'm sorry I remember wrong, 'gc->irq.domain = NULL' was set in regmap_del_irq_chip()
and then called gpiochip_irqchip_remove(). :)