Message ID | 20230607081803.778223-1-jiawenwu@trustnetic.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:994d:0:b0:3d9:f83d:47d9 with SMTP id k13csp100899vqr; Wed, 7 Jun 2023 01:35:09 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ6NdeT4oWvuly+tA2hllcaAIYmDDLhJuVRvIhCa2+p3L5IxEdEXdB62GQEJDKqy8e1GLeWA X-Received: by 2002:a05:6a21:3a85:b0:117:fd0:9694 with SMTP id zv5-20020a056a213a8500b001170fd09694mr1395102pzb.15.1686126908667; Wed, 07 Jun 2023 01:35:08 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1686126908; cv=none; d=google.com; s=arc-20160816; b=uyKj50jAodUMnDeOcWcd0evP3laDeFMYa0/6G+FW3ko4f+10FeXDtL4YYqBMb8tGsl 1mx3A5ch1wNKoKZt2rC+ZfeulliJb/GNwTTOYAM1Eifv01o2FQxRuAz1p0MKVL2V9KpB c71tYMdpjylC9plMM9Xxr6qrlqeK0wj99pdV3QoTwGisj7Nrp2W/3IumB7UVl6NCMCJN 2/vrEuSTlBDQ9Izg+X2xhVkF7lFZPEDQkf6E5G2IPrWOBk1/7+oBnS96Cpd9w3H5KBX6 cfxxR5IA+gZv3ZWzWuVvOOJxcxoPSo8ZDnPEY7HU3lz30kLyJkvv4i0m6BM2lhtCjFIW TUkw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:feedback-id:content-transfer-encoding :mime-version:message-id:date:subject:cc:to:from; bh=8KH2EOUtzNpoHkNRIVw9bJG5SpQlt7xPL9FfHwh6eNw=; b=yRCMTZLjJ4fmhuxVxyBPQenjY68JOY7Z5Yxj6U6ZqxPq895Odq91iueCi4RuqQIWne RfP/CX+xoGA+rNEJhcQH0gExsog+mYqXt9AxSeIi+IqBGlbtfB5MYdgtxVD/gvUneAsz WNq8M5AobZ5tLQkTHl5EEB5SSd3gFbseC3EgPWlQFFv5gd+iNW6+JTHhZwQSsHu58xgM LNYDsbzX5cBQgcqYMLHtkyAVeUincxxh38hqc+DFVxPeUzsu2T/rndAKeF+BTC86RbAQ 7yhnaly9QemqTLGyzyWRDNIyLxX+e+Plcc6wHkBSngqB4lleHfGSspNZrA7kAVw6IZNH DKGA== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id u7-20020a17090341c700b001b04bc1f28csi8827186ple.517.2023.06.07.01.34.53; Wed, 07 Jun 2023 01:35:08 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S239117AbjFGIVX (ORCPT <rfc822;literming00@gmail.com> + 99 others); Wed, 7 Jun 2023 04:21:23 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:60664 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S239098AbjFGIVG (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 7 Jun 2023 04:21:06 -0400 Received: from smtpbg153.qq.com (smtpbg153.qq.com [13.245.218.24]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id C60851FDC; Wed, 7 Jun 2023 01:20:52 -0700 (PDT) X-QQ-mid: bizesmtp81t1686126015tij1hqko Received: from wxdbg.localdomain.com ( [122.235.137.64]) by bizesmtp.qq.com (ESMTP) with id ; Wed, 07 Jun 2023 16:20:06 +0800 (CST) X-QQ-SSF: 01400000000000J0Z000000A0000000 X-QQ-FEAT: +ynUkgUhZJlWz8h9pkBblPfRUT02IjaMSbr4acK3xfZ6D3K7hFxp/p1AGukcB twgiMIpKrFXolURkQR1vr51Zd9RxTg8ZuBnaSDm8iHlBl2RWNYAMaR350cG1JVezHidLoEX ZIrT8u7K1YybOuoUpMfCA8UGUPBQPmOYOnaNRheQ6xKboqrdmJ8B0i9tqXpKA86mE69XEWQ CbdZhy3Fb2SiAq8sBTRoGDo3YWI/ng/sWYvtLyPw7ae5PEkfri+DMfx5uRa6QkWPiZTM2AK 6UV6zrqzGBRICvX0XNxubrWESmBTu79LaDXHIbCIoNMYxvyDqL2y8s/DrxevhTGYntkFW+e KG4QAOntDaxCGDjcaQSYV9zPkE30/FR8LRuMYRCJB5T3g1D2KxeBQkcL+Xk5Q== X-QQ-GoodBg: 2 X-BIZMAIL-ID: 16544725819709452032 From: Jiawen Wu <jiawenwu@trustnetic.com> To: linus.walleij@linaro.org, brgl@bgdev.pl, shreeya.patel@collabora.com, andy.shevchenko@gmail.com, linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org Cc: Jiawen Wu <jiawenwu@trustnetic.com> Subject: [PATCH v2] gpiolib: Fix GPIO chip IRQ initialization restriction Date: Wed, 7 Jun 2023 16:18:03 +0800 Message-Id: <20230607081803.778223-1-jiawenwu@trustnetic.com> X-Mailer: git-send-email 2.27.0 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-QQ-SENDSIZE: 520 Feedback-ID: bizesmtp:trustnetic.com:qybglogicsvrgz:qybglogicsvrgz5a-1 X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H5,RCVD_IN_MSPIKE_WL,SPF_HELO_PASS,SPF_PASS, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1768032209416741773?= X-GMAIL-MSGID: =?utf-8?q?1768032209416741773?= |
Series |
[v2] gpiolib: Fix GPIO chip IRQ initialization restriction
|
|
Commit Message
Jiawen Wu
June 7, 2023, 8:18 a.m. UTC
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(+)
Comments
+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);