| Message ID | 20170629214343.882576048@linutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Thomas, 2017-06-30 6:33 GMT+09:00 Thomas Gleixner <tglx@linutronix.de>: > The irq chip callbacks irq_request/release_resources() have absolutely no > business with masking and unmasking the irq. > > The core code unmasks the interrupt after complete setup and masks it > before invoking irq_release_resources(). > > The unmask is actually harmful as it happens before the interrupt is > completely initialized in __setup_irq(). > > Remove it. Good catch, thanks! (Note that the original patch of mine [1] did that in .irq_startup()/.irq_shutdown(), which was for some reason changed later, but I don't remember the exact story.) [1] https://patchwork.kernel.org/patch/4466431/ Acked-by: Tomasz Figa <tomasz.figa@gmail.com> Sylwester, Krzysztof, would you be able to do some basic test? Best regards, Tomasz > > Fixes: f6a8249f9e55 ("pinctrl: exynos: Lock GPIOs as interrupts when used as EINTs") > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > Cc: Tomasz Figa <tomasz.figa@gmail.com> > Cc: Krzysztof Kozlowski <krzk@kernel.org> > Cc: Sylwester Nawrocki <s.nawrocki@samsung.com> > Cc: Linus Walleij <linus.walleij@linaro.org> > Cc: Kukjin Kim <kgene@kernel.org> > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-samsung-soc@vger.kernel.org > Cc: linux-gpio@vger.kernel.org > --- > drivers/pinctrl/samsung/pinctrl-exynos.c | 4 ---- > 1 file changed, 4 deletions(-) > > --- a/drivers/pinctrl/samsung/pinctrl-exynos.c > +++ b/drivers/pinctrl/samsung/pinctrl-exynos.c > @@ -205,8 +205,6 @@ static int exynos_irq_request_resources( > > spin_unlock_irqrestore(&bank->slock, flags); > > - exynos_irq_unmask(irqd); > - > return 0; > } > > @@ -226,8 +224,6 @@ static void exynos_irq_release_resources > shift = irqd->hwirq * bank_type->fld_width[PINCFG_TYPE_FUNC]; > mask = (1 << bank_type->fld_width[PINCFG_TYPE_FUNC]) - 1; > > - exynos_irq_mask(irqd); > - > spin_lock_irqsave(&bank->slock, flags); > > con = readl(bank->eint_base + reg_con); > >
On Fri, Jun 30, 2017 at 4:47 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote: > Hi Thomas, > > 2017-06-30 6:33 GMT+09:00 Thomas Gleixner <tglx@linutronix.de>: >> The irq chip callbacks irq_request/release_resources() have absolutely no >> business with masking and unmasking the irq. >> >> The core code unmasks the interrupt after complete setup and masks it >> before invoking irq_release_resources(). >> >> The unmask is actually harmful as it happens before the interrupt is >> completely initialized in __setup_irq(). >> >> Remove it. > > Good catch, thanks! (Note that the original patch of mine [1] did that > in .irq_startup()/.irq_shutdown(), which was for some reason changed > later, but I don't remember the exact story.) > > [1] https://patchwork.kernel.org/patch/4466431/ > > Acked-by: Tomasz Figa <tomasz.figa@gmail.com> > > Sylwester, Krzysztof, would you be able to do some basic test? I suppose this was not tested so yes - I have platforms do this. I understand that checking any non-shared GPIO interrupt should be sufficient to test, right? Best regards, Krzysztof
2017-06-30 15:02 GMT+09:00 Krzysztof Kozlowski <krzk@kernel.org>: > On Fri, Jun 30, 2017 at 4:47 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote: >> Hi Thomas, >> >> 2017-06-30 6:33 GMT+09:00 Thomas Gleixner <tglx@linutronix.de>: >>> The irq chip callbacks irq_request/release_resources() have absolutely no >>> business with masking and unmasking the irq. >>> >>> The core code unmasks the interrupt after complete setup and masks it >>> before invoking irq_release_resources(). >>> >>> The unmask is actually harmful as it happens before the interrupt is >>> completely initialized in __setup_irq(). >>> >>> Remove it. >> >> Good catch, thanks! (Note that the original patch of mine [1] did that >> in .irq_startup()/.irq_shutdown(), which was for some reason changed >> later, but I don't remember the exact story.) >> >> [1] https://patchwork.kernel.org/patch/4466431/ >> >> Acked-by: Tomasz Figa <tomasz.figa@gmail.com> >> >> Sylwester, Krzysztof, would you be able to do some basic test? > > I suppose this was not tested so yes - I have platforms do this. I > understand that checking any non-shared GPIO interrupt should be > sufficient to test, right? I think any interrupt from the Exynos pin controller should work, even shared one. I'd expect irq_request_resources() to be invoked for shared interrupts as well, otherwise we have a problem... (I quickly looked through kernel/irq/manage.c and it seems to be invoked for the first __setup_irq() call even for shared interrupts.) Thanks. Best regards, Tomasz
On Thu, Jun 29, 2017 at 11:33 PM, Thomas Gleixner <tglx@linutronix.de> wrote: > The irq chip callbacks irq_request/release_resources() have absolutely no > business with masking and unmasking the irq. > > The core code unmasks the interrupt after complete setup and masks it > before invoking irq_release_resources(). > > The unmask is actually harmful as it happens before the interrupt is > completely initialized in __setup_irq(). > > Remove it. > > Fixes: f6a8249f9e55 ("pinctrl: exynos: Lock GPIOs as interrupts when used as EINTs") > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > Cc: Tomasz Figa <tomasz.figa@gmail.com> > Cc: Krzysztof Kozlowski <krzk@kernel.org> > Cc: Sylwester Nawrocki <s.nawrocki@samsung.com> > Cc: Linus Walleij <linus.walleij@linaro.org> > Cc: Kukjin Kim <kgene@kernel.org> > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-samsung-soc@vger.kernel.org > Cc: linux-gpio@vger.kernel.org Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Does this patch have a dependency on the rest of the series or should I just apply it as-is? Yours, Linus Walleij
On Fri, 30 Jun 2017, Linus Walleij wrote: > On Thu, Jun 29, 2017 at 11:33 PM, Thomas Gleixner <tglx@linutronix.de> wrote: > > > The irq chip callbacks irq_request/release_resources() have absolutely no > > business with masking and unmasking the irq. > > > > The core code unmasks the interrupt after complete setup and masks it > > before invoking irq_release_resources(). > > > > The unmask is actually harmful as it happens before the interrupt is > > completely initialized in __setup_irq(). > > > > Remove it. > > > > Fixes: f6a8249f9e55 ("pinctrl: exynos: Lock GPIOs as interrupts when used as EINTs") > > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > > Cc: Tomasz Figa <tomasz.figa@gmail.com> > > Cc: Krzysztof Kozlowski <krzk@kernel.org> > > Cc: Sylwester Nawrocki <s.nawrocki@samsung.com> > > Cc: Linus Walleij <linus.walleij@linaro.org> > > Cc: Kukjin Kim <kgene@kernel.org> > > Cc: linux-arm-kernel@lists.infradead.org > > Cc: linux-samsung-soc@vger.kernel.org > > Cc: linux-gpio@vger.kernel.org > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > Does this patch have a dependency on the rest of the series or should > I just apply it as-is? Has no dependecies at all.
On Thu, Jun 29, 2017 at 11:33 PM, Thomas Gleixner <tglx@linutronix.de> wrote: > The irq chip callbacks irq_request/release_resources() have absolutely no > business with masking and unmasking the irq. > > The core code unmasks the interrupt after complete setup and masks it > before invoking irq_release_resources(). > > The unmask is actually harmful as it happens before the interrupt is > completely initialized in __setup_irq(). > > Remove it. > > Fixes: f6a8249f9e55 ("pinctrl: exynos: Lock GPIOs as interrupts when used as EINTs") > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > Cc: Tomasz Figa <tomasz.figa@gmail.com> > Cc: Krzysztof Kozlowski <krzk@kernel.org> > Cc: Sylwester Nawrocki <s.nawrocki@samsung.com> > Cc: Linus Walleij <linus.walleij@linaro.org> > Cc: Kukjin Kim <kgene@kernel.org> > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-samsung-soc@vger.kernel.org > Cc: linux-gpio@vger.kernel.org Patch applied directly since it has no deps and fixes queued stuff for v4.13. I guess Krysztof will make sure it doesn't break anything. Yours, Linus Walleij
On Fri, Jun 30, 2017 at 03:53:18PM +0200, Linus Walleij wrote: > On Thu, Jun 29, 2017 at 11:33 PM, Thomas Gleixner <tglx@linutronix.de> wrote: > > > The irq chip callbacks irq_request/release_resources() have absolutely no > > business with masking and unmasking the irq. > > > > The core code unmasks the interrupt after complete setup and masks it > > before invoking irq_release_resources(). > > > > The unmask is actually harmful as it happens before the interrupt is > > completely initialized in __setup_irq(). > > > > Remove it. > > > > Fixes: f6a8249f9e55 ("pinctrl: exynos: Lock GPIOs as interrupts when used as EINTs") > > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > > Cc: Tomasz Figa <tomasz.figa@gmail.com> > > Cc: Krzysztof Kozlowski <krzk@kernel.org> > > Cc: Sylwester Nawrocki <s.nawrocki@samsung.com> > > Cc: Linus Walleij <linus.walleij@linaro.org> > > Cc: Kukjin Kim <kgene@kernel.org> > > Cc: linux-arm-kernel@lists.infradead.org > > Cc: linux-samsung-soc@vger.kernel.org > > Cc: linux-gpio@vger.kernel.org > > Patch applied directly since it has no deps and fixes queued stuff for v4.13. > I guess Krysztof will make sure it doesn't break anything. Everything seems to work fine with the patchset. Best regards, Krzysztof
--- a/drivers/pinctrl/samsung/pinctrl-exynos.c +++ b/drivers/pinctrl/samsung/pinctrl-exynos.c @@ -205,8 +205,6 @@ static int exynos_irq_request_resources( spin_unlock_irqrestore(&bank->slock, flags); - exynos_irq_unmask(irqd); - return 0; } @@ -226,8 +224,6 @@ static void exynos_irq_release_resources shift = irqd->hwirq * bank_type->fld_width[PINCFG_TYPE_FUNC]; mask = (1 << bank_type->fld_width[PINCFG_TYPE_FUNC]) - 1; - exynos_irq_mask(irqd); - spin_lock_irqsave(&bank->slock, flags); con = readl(bank->eint_base + reg_con);
The irq chip callbacks irq_request/release_resources() have absolutely no business with masking and unmasking the irq. The core code unmasks the interrupt after complete setup and masks it before invoking irq_release_resources(). The unmask is actually harmful as it happens before the interrupt is completely initialized in __setup_irq(). Remove it. Fixes: f6a8249f9e55 ("pinctrl: exynos: Lock GPIOs as interrupts when used as EINTs") Signed-off-by: Thomas Gleixner <tglx@linutronix.de> Cc: Tomasz Figa <tomasz.figa@gmail.com> Cc: Krzysztof Kozlowski <krzk@kernel.org> Cc: Sylwester Nawrocki <s.nawrocki@samsung.com> Cc: Linus Walleij <linus.walleij@linaro.org> Cc: Kukjin Kim <kgene@kernel.org> Cc: linux-arm-kernel@lists.infradead.org Cc: linux-samsung-soc@vger.kernel.org Cc: linux-gpio@vger.kernel.org --- drivers/pinctrl/samsung/pinctrl-exynos.c | 4 ---- 1 file changed, 4 deletions(-)