| Message ID | 20230607081803.778223-1-jiawenwu@trustnetic.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] gpiolib: Fix GPIO chip IRQ initialization restriction | expand |
+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(). :)
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);
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(+)