* Re: [PATCH 2/2] pinctrl: mediatek: Update cur_mask in mask/mask ops
@ 2019-05-30 17:12 ` Evan Green
0 siblings, 0 replies; 50+ messages in thread
From: Evan Green @ 2019-05-30 17:12 UTC (permalink / raw)
To: Nicolas Boichat
Cc: Chuanjia Liu, Linus Walleij, Sean Wang, lkml, Stephen Boyd,
linux-gpio, moderated list:ARM/Mediatek SoC support,
Matthias Brugger, Thomas Gleixner, linux-arm Mailing List
On Wed, May 15, 2019 at 1:05 AM Nicolas Boichat <drinkcat@chromium.org> wrote:
>
> On Wed, May 15, 2019 at 4:14 AM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Nicolas Boichat (2019-05-13 18:37:58)
> > > On Tue, May 14, 2019 at 6:29 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > > >
> > > > Quoting Nicolas Boichat (2019-04-28 20:55:15)
> > > > > During suspend/resume, mtk_eint_mask may be called while
> > > > > wake_mask is active. For example, this happens if a wake-source
> > > > > with an active interrupt handler wakes the system:
> > > > > irq/pm.c:irq_pm_check_wakeup would disable the interrupt, so
> > > > > that it can be handled later on in the resume flow.
> > > > >
> > > > > However, this may happen before mtk_eint_do_resume is called:
> > > > > in this case, wake_mask is loaded, and cur_mask is restored
> > > > > from an older copy, re-enabling the interrupt, and causing
> > > > > an interrupt storm (especially for level interrupts).
> > > > >
> > > > > Instead, we just record mask/unmask changes in cur_mask. This
> > > > > also avoids the need to read the current mask in eint_do_suspend,
> > > > > and we can remove mtk_eint_chip_read_mask function.
> > > > >
> > > > > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> > > >
> > > > It looks an awful lot like you should just use IRQCHIP_MASK_ON_SUSPEND
> > > > here. Isn't that what's happening? All non-wake irqs should be masked at
> > > > the hardware level so they can't cause a wakeup during suspend and on
> > > > resume they can be unmasked?
> > >
> > > No, this is for an line that has both wake and interrupt enabled. To
> > > reword the commit message above:
> >
> > Is my understanding correct that there isn't a different "wake up"
> > register that can be written to cause a GPIO to be configured to wake
> > the system from suspend? The only way to do so is to leave the GPIO
> > unmasked in the hardware by having EINT_EN[irq] = 1? And thus any
> > interrupts that we don't want to wake us up during suspend should be
> > masked in the hardware?
>
> Yes, that's my understanding as well.
>
> And then, what this driver does is to emulate the behaviour of a
> controller that would actually have separate irq and wake enable
> registers.
>
> > If that's true, the code here that's trying to keep track of enabled
> > irqs and wakeup enabled irqs can be replaced with the irqchip flag so
> > that wakeup irqs are not masked while non-wakeups are masked.
>
> Correct, but with the caveat that I don't see anything that definitely
> requires an interrupt to be enabled to be a wake source. See below...
>
> >
> > > 1. cur_mask[irq] = 1; wake_mask[irq] = 1; EINT_EN[irq] = 1 (interrupt
> > > enabled at hardware level)
> > > 2. System suspends, resumes due to that line (at this stage EINT_HW
> > > == wake_mask)
> > > 3. irq_pm_check_wakeup is called, and disables the interrupt =>
> > > EINT_EN[irq] = 0, but we still have cur_mask[irq] = 1
> > > 4. mtk_eint_do_resume is called, and restores EINT_EN = cur_mask, so
> > > it reenables EINT_EN[irq] = 1 => interrupt storm.
> > >
> > > This patch fixes the issue in step 3. So that the interrupt can be
> > > re-enabled properly later on, sometimes after mtk_eint_do_resume, when
> > > the driver is ready to handle it.
> >
> > Right, we'd rather not see irqchip drivers working around the genirq
> > layer to do these things like tracking cur_mask and wake_mask. That
> > leads to subtle bugs and makes the driver maintain state across the
> > irqchip callbacks and system suspend/resume.
> >
> > >
> > > Also, IRQCHIP_MASK_ON_SUSPEND does not handle lines that are enabled
> > > as a wake source, but without interrupt enabled (e.g. cros_ec driver
> > > does that), which we do want to support.
> >
> > Hmm. I thought that even if the irq is disabled by a driver, that would
> > be a lazy disable so it isn't really masked in the hardware. Then if an
> > interrupt comes in during suspend on a wake configured irq line, the
> > hardware will have left it unmasked because IRQCHIP_MASK_ON_SUSPEND in
> > combination with lazy disable would mean that the line is left unmasked
> > (ignoring whatever this mediatek driver is doing to mask and unmask in
> > PM hooks).
>
> At the very least, that's not what happens with this system. The
> interrupt is definitely not kept enabled in suspend, and the system
> would not wake from an EC interrupt. (see also this series, BTW:
> https://patchwork.kernel.org/cover/10921121/).
>
> > Just reading Documentation/power/suspend-and-interrupts.txt I'm led to
> > believe that the cros_ec driver shouldn't call disable_irq() on the
> > interrupt if it wants to wakeup from it:
> >
> > "Calling enable_irq_wake() causes suspend_device_irqs() to treat the
> > given IRQ in a special way. Namely, the IRQ remains enabled, by on the
> > first interrupt it will be disabled, marked as pending and "suspended"
> > so that it will be re-enabled by resume_device_irqs() during the
> > subsequent system resume. Also the PM core is notified about the event
> > which causes the system suspend in progress to be aborted (that doesn't
> > have to happen immediately, but at one of the points where the suspend
> > thread looks for pending wakeup events)."
>
> I think this describes the behaviour when you keep both enabled.
>
> > I suppose the problem is an irq line disabled in hardware that has
> > wakeup armed on it? Is this even valid? Shouldn't an irq be enabled for
> > wakeup to work?
>
> I couldn't really find a definite answer, but there are a bunch of
> examples of other drivers in the kernel:
> - drivers/extcon/extcon-usb-gpio.c:usb_extcon_suspend
> - drivers/hid/i2c-hid/i2c-hid.c:i2c_hid_suspend
> - drivers/mfd/max77843.c:max77843_suspend
> (not exhaustive, this is quite hard to grep for...)
>
> > We could immediately unmask those lines in the hardware when the
> > set_wake() callback is called. That way the genirq layer can use the
> > driver to do what it wants with the hardware and the driver can make
> > sure that set_wake() will always cause the wakeup interrupt to be
> > delivered to genirq even when software has disabled it.
> >
> > But I think that there might be a problem with how genirq understands
> > the masked state of a line when the wakeup implementation conflates
> > masked state with wakeup armed state. Consider this call-flow:
> >
> > irq masked in hardware, IRQD_IRQ_MASKED is set
> > enable_irq_wake()
> > unmask_irq in hardware
> > IRQD_WAKEUP_ARMED is set
> > <suspend and wakeup from irq>
> > handle_level_irq()
> > mask_ack_irq()
> > mask_irq()
> > if (irqd_irq_masked()) -> returns true and skips masking!
> > if (desc->irq_data.chip->irq_ack)
> > ...
> > irq_may_run()
> > irq_pm_check_wakeup()
> > irq_disable()
> > mask_irq() -> does nothing again
> >
> > In the above flow, we never mask the irq because we thought it was
> > already masked when it was disabled, but the irqchip implementation
> > unmasked it to make wakeup work. Maybe we should always mask the irq if
> > wakeup is armed and we're trying to call mask_irq()? Looks hacky.
> >
> > diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> > index 51128bea3846..20257d528880 100644
> > --- a/kernel/irq/chip.c
> > +++ b/kernel/irq/chip.c
> > @@ -411,7 +411,7 @@ static inline void mask_ack_irq(struct irq_desc *desc)
> >
> > void mask_irq(struct irq_desc *desc)
> > {
> > - if (irqd_irq_masked(&desc->irq_data))
> > + if (!irqd_is_wakeup_armed(&desc->irq_data) && irqd_irq_masked(&desc->irq_data))
> > return;
> >
> > if (desc->irq_data.chip->irq_mask) {
>
> I'm... really not sure what's the best approach here. But basically,
> yes, if we can find a way to properly handle wake and interrupt
> behaviour for drivers with a single mask, that'd be good.
> IRQCHIP_MASK_ON_SUSPEND only seems to be doing half of the work, since
> it does not cover the disable+wake source case.
>
> Thanks,
I finally got around to studying this patch. This series seems okay to
me. The underlying problem is really that the hardware IRQ enabled
state is out of sync with what Linux thinks. This happens during
suspend because Linux thinks the irq is disabled, but due to the
hardware constraints on this platform, the interrupt has to be enabled
for it to be a wake source. So the mtk driver re-enables the
interrupt, and then has to find a way to get back in sync with Linux's
IRQ mask state.
One possible approach is mentioned above by Stephen: stop calling
disable_irq in the cros EC driver. Then both linux and mtk agree the
interrupt is enabled at suspend time. I think this ran into other
problems though, where the EC gets its interrupt but is unable to
silence it because the underlying SPI bus is still suspended.
The other approach, taken here, is to mask the interrupt when it first
comes in, getting Linux and mtk back in agreement that yes, the
interrupt is masked. Outside of enlightening the generic IRQ core
about these types of interrupts that need to get re-enabled to be wake
sources, this seems like a reasonable approach.
-Evan
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH 2/2] pinctrl: mediatek: Update cur_mask in mask/mask ops
@ 2019-05-30 17:12 ` Evan Green
0 siblings, 0 replies; 50+ messages in thread
From: Evan Green @ 2019-05-30 17:12 UTC (permalink / raw)
To: Nicolas Boichat
Cc: Stephen Boyd, moderated list:ARM/Mediatek SoC support, Sean Wang,
Linus Walleij, Matthias Brugger, linux-gpio,
linux-arm Mailing List, lkml, Chuanjia Liu, Thomas Gleixner
On Wed, May 15, 2019 at 1:05 AM Nicolas Boichat <drinkcat@chromium.org> wrote:
>
> On Wed, May 15, 2019 at 4:14 AM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Nicolas Boichat (2019-05-13 18:37:58)
> > > On Tue, May 14, 2019 at 6:29 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > > >
> > > > Quoting Nicolas Boichat (2019-04-28 20:55:15)
> > > > > During suspend/resume, mtk_eint_mask may be called while
> > > > > wake_mask is active. For example, this happens if a wake-source
> > > > > with an active interrupt handler wakes the system:
> > > > > irq/pm.c:irq_pm_check_wakeup would disable the interrupt, so
> > > > > that it can be handled later on in the resume flow.
> > > > >
> > > > > However, this may happen before mtk_eint_do_resume is called:
> > > > > in this case, wake_mask is loaded, and cur_mask is restored
> > > > > from an older copy, re-enabling the interrupt, and causing
> > > > > an interrupt storm (especially for level interrupts).
> > > > >
> > > > > Instead, we just record mask/unmask changes in cur_mask. This
> > > > > also avoids the need to read the current mask in eint_do_suspend,
> > > > > and we can remove mtk_eint_chip_read_mask function.
> > > > >
> > > > > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> > > >
> > > > It looks an awful lot like you should just use IRQCHIP_MASK_ON_SUSPEND
> > > > here. Isn't that what's happening? All non-wake irqs should be masked at
> > > > the hardware level so they can't cause a wakeup during suspend and on
> > > > resume they can be unmasked?
> > >
> > > No, this is for an line that has both wake and interrupt enabled. To
> > > reword the commit message above:
> >
> > Is my understanding correct that there isn't a different "wake up"
> > register that can be written to cause a GPIO to be configured to wake
> > the system from suspend? The only way to do so is to leave the GPIO
> > unmasked in the hardware by having EINT_EN[irq] = 1? And thus any
> > interrupts that we don't want to wake us up during suspend should be
> > masked in the hardware?
>
> Yes, that's my understanding as well.
>
> And then, what this driver does is to emulate the behaviour of a
> controller that would actually have separate irq and wake enable
> registers.
>
> > If that's true, the code here that's trying to keep track of enabled
> > irqs and wakeup enabled irqs can be replaced with the irqchip flag so
> > that wakeup irqs are not masked while non-wakeups are masked.
>
> Correct, but with the caveat that I don't see anything that definitely
> requires an interrupt to be enabled to be a wake source. See below...
>
> >
> > > 1. cur_mask[irq] = 1; wake_mask[irq] = 1; EINT_EN[irq] = 1 (interrupt
> > > enabled at hardware level)
> > > 2. System suspends, resumes due to that line (at this stage EINT_HW
> > > == wake_mask)
> > > 3. irq_pm_check_wakeup is called, and disables the interrupt =>
> > > EINT_EN[irq] = 0, but we still have cur_mask[irq] = 1
> > > 4. mtk_eint_do_resume is called, and restores EINT_EN = cur_mask, so
> > > it reenables EINT_EN[irq] = 1 => interrupt storm.
> > >
> > > This patch fixes the issue in step 3. So that the interrupt can be
> > > re-enabled properly later on, sometimes after mtk_eint_do_resume, when
> > > the driver is ready to handle it.
> >
> > Right, we'd rather not see irqchip drivers working around the genirq
> > layer to do these things like tracking cur_mask and wake_mask. That
> > leads to subtle bugs and makes the driver maintain state across the
> > irqchip callbacks and system suspend/resume.
> >
> > >
> > > Also, IRQCHIP_MASK_ON_SUSPEND does not handle lines that are enabled
> > > as a wake source, but without interrupt enabled (e.g. cros_ec driver
> > > does that), which we do want to support.
> >
> > Hmm. I thought that even if the irq is disabled by a driver, that would
> > be a lazy disable so it isn't really masked in the hardware. Then if an
> > interrupt comes in during suspend on a wake configured irq line, the
> > hardware will have left it unmasked because IRQCHIP_MASK_ON_SUSPEND in
> > combination with lazy disable would mean that the line is left unmasked
> > (ignoring whatever this mediatek driver is doing to mask and unmask in
> > PM hooks).
>
> At the very least, that's not what happens with this system. The
> interrupt is definitely not kept enabled in suspend, and the system
> would not wake from an EC interrupt. (see also this series, BTW:
> https://patchwork.kernel.org/cover/10921121/).
>
> > Just reading Documentation/power/suspend-and-interrupts.txt I'm led to
> > believe that the cros_ec driver shouldn't call disable_irq() on the
> > interrupt if it wants to wakeup from it:
> >
> > "Calling enable_irq_wake() causes suspend_device_irqs() to treat the
> > given IRQ in a special way. Namely, the IRQ remains enabled, by on the
> > first interrupt it will be disabled, marked as pending and "suspended"
> > so that it will be re-enabled by resume_device_irqs() during the
> > subsequent system resume. Also the PM core is notified about the event
> > which causes the system suspend in progress to be aborted (that doesn't
> > have to happen immediately, but at one of the points where the suspend
> > thread looks for pending wakeup events)."
>
> I think this describes the behaviour when you keep both enabled.
>
> > I suppose the problem is an irq line disabled in hardware that has
> > wakeup armed on it? Is this even valid? Shouldn't an irq be enabled for
> > wakeup to work?
>
> I couldn't really find a definite answer, but there are a bunch of
> examples of other drivers in the kernel:
> - drivers/extcon/extcon-usb-gpio.c:usb_extcon_suspend
> - drivers/hid/i2c-hid/i2c-hid.c:i2c_hid_suspend
> - drivers/mfd/max77843.c:max77843_suspend
> (not exhaustive, this is quite hard to grep for...)
>
> > We could immediately unmask those lines in the hardware when the
> > set_wake() callback is called. That way the genirq layer can use the
> > driver to do what it wants with the hardware and the driver can make
> > sure that set_wake() will always cause the wakeup interrupt to be
> > delivered to genirq even when software has disabled it.
> >
> > But I think that there might be a problem with how genirq understands
> > the masked state of a line when the wakeup implementation conflates
> > masked state with wakeup armed state. Consider this call-flow:
> >
> > irq masked in hardware, IRQD_IRQ_MASKED is set
> > enable_irq_wake()
> > unmask_irq in hardware
> > IRQD_WAKEUP_ARMED is set
> > <suspend and wakeup from irq>
> > handle_level_irq()
> > mask_ack_irq()
> > mask_irq()
> > if (irqd_irq_masked()) -> returns true and skips masking!
> > if (desc->irq_data.chip->irq_ack)
> > ...
> > irq_may_run()
> > irq_pm_check_wakeup()
> > irq_disable()
> > mask_irq() -> does nothing again
> >
> > In the above flow, we never mask the irq because we thought it was
> > already masked when it was disabled, but the irqchip implementation
> > unmasked it to make wakeup work. Maybe we should always mask the irq if
> > wakeup is armed and we're trying to call mask_irq()? Looks hacky.
> >
> > diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> > index 51128bea3846..20257d528880 100644
> > --- a/kernel/irq/chip.c
> > +++ b/kernel/irq/chip.c
> > @@ -411,7 +411,7 @@ static inline void mask_ack_irq(struct irq_desc *desc)
> >
> > void mask_irq(struct irq_desc *desc)
> > {
> > - if (irqd_irq_masked(&desc->irq_data))
> > + if (!irqd_is_wakeup_armed(&desc->irq_data) && irqd_irq_masked(&desc->irq_data))
> > return;
> >
> > if (desc->irq_data.chip->irq_mask) {
>
> I'm... really not sure what's the best approach here. But basically,
> yes, if we can find a way to properly handle wake and interrupt
> behaviour for drivers with a single mask, that'd be good.
> IRQCHIP_MASK_ON_SUSPEND only seems to be doing half of the work, since
> it does not cover the disable+wake source case.
>
> Thanks,
I finally got around to studying this patch. This series seems okay to
me. The underlying problem is really that the hardware IRQ enabled
state is out of sync with what Linux thinks. This happens during
suspend because Linux thinks the irq is disabled, but due to the
hardware constraints on this platform, the interrupt has to be enabled
for it to be a wake source. So the mtk driver re-enables the
interrupt, and then has to find a way to get back in sync with Linux's
IRQ mask state.
One possible approach is mentioned above by Stephen: stop calling
disable_irq in the cros EC driver. Then both linux and mtk agree the
interrupt is enabled at suspend time. I think this ran into other
problems though, where the EC gets its interrupt but is unable to
silence it because the underlying SPI bus is still suspended.
The other approach, taken here, is to mask the interrupt when it first
comes in, getting Linux and mtk back in agreement that yes, the
interrupt is masked. Outside of enlightening the generic IRQ core
about these types of interrupts that need to get re-enabled to be wake
sources, this seems like a reasonable approach.
-Evan
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH 2/2] pinctrl: mediatek: Update cur_mask in mask/mask ops
2019-05-30 17:12 ` Evan Green
(?)
@ 2019-05-31 8:05 ` Chuanjia Liu
-1 siblings, 0 replies; 50+ messages in thread
From: Chuanjia Liu @ 2019-05-31 8:05 UTC (permalink / raw)
To: Evan Green
Cc: Nicolas Boichat, Stephen Boyd,
moderated list:ARM/Mediatek SoC support, Sean Wang,
Linus Walleij, Matthias Brugger, linux-gpio,
linux-arm Mailing List, lkml, Thomas Gleixner
On Thu, 2019-05-30 at 10:12 -0700, Evan Green wrote:
> On Wed, May 15, 2019 at 1:05 AM Nicolas Boichat <drinkcat@chromium.org> wrote:
> >
> > On Wed, May 15, 2019 at 4:14 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > Quoting Nicolas Boichat (2019-05-13 18:37:58)
> > > > On Tue, May 14, 2019 at 6:29 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > > > >
> > > > > Quoting Nicolas Boichat (2019-04-28 20:55:15)
> > > > > > During suspend/resume, mtk_eint_mask may be called while
> > > > > > wake_mask is active. For example, this happens if a wake-source
> > > > > > with an active interrupt handler wakes the system:
> > > > > > irq/pm.c:irq_pm_check_wakeup would disable the interrupt, so
> > > > > > that it can be handled later on in the resume flow.
> > > > > >
> > > > > > However, this may happen before mtk_eint_do_resume is called:
> > > > > > in this case, wake_mask is loaded, and cur_mask is restored
> > > > > > from an older copy, re-enabling the interrupt, and causing
> > > > > > an interrupt storm (especially for level interrupts).
> > > > > >
> > > > > > Instead, we just record mask/unmask changes in cur_mask. This
> > > > > > also avoids the need to read the current mask in eint_do_suspend,
> > > > > > and we can remove mtk_eint_chip_read_mask function.
> > > > > >
> > > > > > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> > > > >
> > > > > It looks an awful lot like you should just use IRQCHIP_MASK_ON_SUSPEND
> > > > > here. Isn't that what's happening? All non-wake irqs should be masked at
> > > > > the hardware level so they can't cause a wakeup during suspend and on
> > > > > resume they can be unmasked?
> > > >
> > > > No, this is for an line that has both wake and interrupt enabled. To
> > > > reword the commit message above:
> > >
> > > Is my understanding correct that there isn't a different "wake up"
> > > register that can be written to cause a GPIO to be configured to wake
> > > the system from suspend? The only way to do so is to leave the GPIO
> > > unmasked in the hardware by having EINT_EN[irq] = 1? And thus any
> > > interrupts that we don't want to wake us up during suspend should be
> > > masked in the hardware?
> >
> > Yes, that's my understanding as well.
> >
> > And then, what this driver does is to emulate the behaviour of a
> > controller that would actually have separate irq and wake enable
> > registers.
> >
> > > If that's true, the code here that's trying to keep track of enabled
> > > irqs and wakeup enabled irqs can be replaced with the irqchip flag so
> > > that wakeup irqs are not masked while non-wakeups are masked.
> >
> > Correct, but with the caveat that I don't see anything that definitely
> > requires an interrupt to be enabled to be a wake source. See below...
> >
> > >
> > > > 1. cur_mask[irq] = 1; wake_mask[irq] = 1; EINT_EN[irq] = 1 (interrupt
> > > > enabled at hardware level)
> > > > 2. System suspends, resumes due to that line (at this stage EINT_HW
> > > > == wake_mask)
> > > > 3. irq_pm_check_wakeup is called, and disables the interrupt =>
> > > > EINT_EN[irq] = 0, but we still have cur_mask[irq] = 1
> > > > 4. mtk_eint_do_resume is called, and restores EINT_EN = cur_mask, so
> > > > it reenables EINT_EN[irq] = 1 => interrupt storm.
> > > >
> > > > This patch fixes the issue in step 3. So that the interrupt can be
> > > > re-enabled properly later on, sometimes after mtk_eint_do_resume, when
> > > > the driver is ready to handle it.
> > >
> > > Right, we'd rather not see irqchip drivers working around the genirq
> > > layer to do these things like tracking cur_mask and wake_mask. That
> > > leads to subtle bugs and makes the driver maintain state across the
> > > irqchip callbacks and system suspend/resume.
> > >
> > > >
> > > > Also, IRQCHIP_MASK_ON_SUSPEND does not handle lines that are enabled
> > > > as a wake source, but without interrupt enabled (e.g. cros_ec driver
> > > > does that), which we do want to support.
> > >
> > > Hmm. I thought that even if the irq is disabled by a driver, that would
> > > be a lazy disable so it isn't really masked in the hardware. Then if an
> > > interrupt comes in during suspend on a wake configured irq line, the
> > > hardware will have left it unmasked because IRQCHIP_MASK_ON_SUSPEND in
> > > combination with lazy disable would mean that the line is left unmasked
> > > (ignoring whatever this mediatek driver is doing to mask and unmask in
> > > PM hooks).
> >
> > At the very least, that's not what happens with this system. The
> > interrupt is definitely not kept enabled in suspend, and the system
> > would not wake from an EC interrupt. (see also this series, BTW:
> > https://patchwork.kernel.org/cover/10921121/).
> >
> > > Just reading Documentation/power/suspend-and-interrupts.txt I'm led to
> > > believe that the cros_ec driver shouldn't call disable_irq() on the
> > > interrupt if it wants to wakeup from it:
> > >
> > > "Calling enable_irq_wake() causes suspend_device_irqs() to treat the
> > > given IRQ in a special way. Namely, the IRQ remains enabled, by on the
> > > first interrupt it will be disabled, marked as pending and "suspended"
> > > so that it will be re-enabled by resume_device_irqs() during the
> > > subsequent system resume. Also the PM core is notified about the event
> > > which causes the system suspend in progress to be aborted (that doesn't
> > > have to happen immediately, but at one of the points where the suspend
> > > thread looks for pending wakeup events)."
> >
> > I think this describes the behaviour when you keep both enabled.
> >
> > > I suppose the problem is an irq line disabled in hardware that has
> > > wakeup armed on it? Is this even valid? Shouldn't an irq be enabled for
> > > wakeup to work?
> >
> > I couldn't really find a definite answer, but there are a bunch of
> > examples of other drivers in the kernel:
> > - drivers/extcon/extcon-usb-gpio.c:usb_extcon_suspend
> > - drivers/hid/i2c-hid/i2c-hid.c:i2c_hid_suspend
> > - drivers/mfd/max77843.c:max77843_suspend
> > (not exhaustive, this is quite hard to grep for...)
> >
> > > We could immediately unmask those lines in the hardware when the
> > > set_wake() callback is called. That way the genirq layer can use the
> > > driver to do what it wants with the hardware and the driver can make
> > > sure that set_wake() will always cause the wakeup interrupt to be
> > > delivered to genirq even when software has disabled it.
> > >
> > > But I think that there might be a problem with how genirq understands
> > > the masked state of a line when the wakeup implementation conflates
> > > masked state with wakeup armed state. Consider this call-flow:
> > >
> > > irq masked in hardware, IRQD_IRQ_MASKED is set
> > > enable_irq_wake()
> > > unmask_irq in hardware
> > > IRQD_WAKEUP_ARMED is set
> > > <suspend and wakeup from irq>
> > > handle_level_irq()
> > > mask_ack_irq()
> > > mask_irq()
> > > if (irqd_irq_masked()) -> returns true and skips masking!
> > > if (desc->irq_data.chip->irq_ack)
> > > ...
> > > irq_may_run()
> > > irq_pm_check_wakeup()
> > > irq_disable()
> > > mask_irq() -> does nothing again
> > >
> > > In the above flow, we never mask the irq because we thought it was
> > > already masked when it was disabled, but the irqchip implementation
> > > unmasked it to make wakeup work. Maybe we should always mask the irq if
> > > wakeup is armed and we're trying to call mask_irq()? Looks hacky.
Maybe we can implement irqchip's mask_ack_irq in mediatek driver to
always mask the irq. Then flow will always call it without judgment
IRQD_IRQ_MASKED.
diff --git a/drivers/pinctrl/mediatek/mtk-eint.c
b/drivers/pinctrl/mediatek/mtk-
index f464f8c..9f1aae2 100644
--- a/drivers/pinctrl/mediatek/mtk-eint.c
+++ b/drivers/pinctrl/mediatek/mtk-eint.c
@@ -272,12 +272,19 @@ static void mtk_eint_irq_release_resources(struct
irq_data
gpiochip_unlock_as_irq(gpio_c, gpio_n);
}
+static void mtk_eint_mask_ack(struct irq_data *d)
+{
+ mtk_eint_mask(d);
+ mtk_eint_ack(d);
+}
+
static struct irq_chip mtk_eint_irq_chip = {
.name = "mt-eint",
.irq_disable = mtk_eint_mask,
.irq_mask = mtk_eint_mask,
.irq_unmask = mtk_eint_unmask,
.irq_ack = mtk_eint_ack,
+ .irq_mask_ack = mtk_eint_mask_ack,
.irq_set_type = mtk_eint_set_type,
.irq_set_wake = mtk_eint_irq_set_wake,
.irq_request_resources = mtk_eint_irq_request_resources,
This seems like a small change.
thanks.
> > >
> > > diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> > > index 51128bea3846..20257d528880 100644
> > > --- a/kernel/irq/chip.c
> > > +++ b/kernel/irq/chip.c
> > > @@ -411,7 +411,7 @@ static inline void mask_ack_irq(struct irq_desc *desc)
> > >
> > > void mask_irq(struct irq_desc *desc)
> > > {
> > > - if (irqd_irq_masked(&desc->irq_data))
> > > + if (!irqd_is_wakeup_armed(&desc->irq_data) && irqd_irq_masked(&desc->irq_data))
> > > return;
> > >
> > > if (desc->irq_data.chip->irq_mask) {
> >
> > I'm... really not sure what's the best approach here. But basically,
> > yes, if we can find a way to properly handle wake and interrupt
> > behaviour for drivers with a single mask, that'd be good.
> > IRQCHIP_MASK_ON_SUSPEND only seems to be doing half of the work, since
> > it does not cover the disable+wake source case.
> >
> > Thanks,
>
> I finally got around to studying this patch. This series seems okay to
> me. The underlying problem is really that the hardware IRQ enabled
> state is out of sync with what Linux thinks. This happens during
> suspend because Linux thinks the irq is disabled, but due to the
> hardware constraints on this platform, the interrupt has to be enabled
> for it to be a wake source. So the mtk driver re-enables the
> interrupt, and then has to find a way to get back in sync with Linux's
> IRQ mask state.
>
> One possible approach is mentioned above by Stephen: stop calling
> disable_irq in the cros EC driver. Then both linux and mtk agree the
> interrupt is enabled at suspend time. I think this ran into other
> problems though, where the EC gets its interrupt but is unable to
> silence it because the underlying SPI bus is still suspended.
>
> The other approach, taken here, is to mask the interrupt when it first
> comes in, getting Linux and mtk back in agreement that yes, the
> interrupt is masked. Outside of enlightening the generic IRQ core
> about these types of interrupts that need to get re-enabled to be wake
> sources, this seems like a reasonable approach.
> -Evan
^ permalink raw reply related [flat|nested] 50+ messages in thread* Re: [PATCH 2/2] pinctrl: mediatek: Update cur_mask in mask/mask ops
@ 2019-05-31 8:05 ` Chuanjia Liu
0 siblings, 0 replies; 50+ messages in thread
From: Chuanjia Liu @ 2019-05-31 8:05 UTC (permalink / raw)
To: Evan Green
Cc: Nicolas Boichat, Linus Walleij, Sean Wang, lkml, Stephen Boyd,
linux-gpio, moderated list:ARM/Mediatek SoC support,
Matthias Brugger, Thomas Gleixner, linux-arm Mailing List
On Thu, 2019-05-30 at 10:12 -0700, Evan Green wrote:
> On Wed, May 15, 2019 at 1:05 AM Nicolas Boichat <drinkcat@chromium.org> wrote:
> >
> > On Wed, May 15, 2019 at 4:14 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > Quoting Nicolas Boichat (2019-05-13 18:37:58)
> > > > On Tue, May 14, 2019 at 6:29 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > > > >
> > > > > Quoting Nicolas Boichat (2019-04-28 20:55:15)
> > > > > > During suspend/resume, mtk_eint_mask may be called while
> > > > > > wake_mask is active. For example, this happens if a wake-source
> > > > > > with an active interrupt handler wakes the system:
> > > > > > irq/pm.c:irq_pm_check_wakeup would disable the interrupt, so
> > > > > > that it can be handled later on in the resume flow.
> > > > > >
> > > > > > However, this may happen before mtk_eint_do_resume is called:
> > > > > > in this case, wake_mask is loaded, and cur_mask is restored
> > > > > > from an older copy, re-enabling the interrupt, and causing
> > > > > > an interrupt storm (especially for level interrupts).
> > > > > >
> > > > > > Instead, we just record mask/unmask changes in cur_mask. This
> > > > > > also avoids the need to read the current mask in eint_do_suspend,
> > > > > > and we can remove mtk_eint_chip_read_mask function.
> > > > > >
> > > > > > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> > > > >
> > > > > It looks an awful lot like you should just use IRQCHIP_MASK_ON_SUSPEND
> > > > > here. Isn't that what's happening? All non-wake irqs should be masked at
> > > > > the hardware level so they can't cause a wakeup during suspend and on
> > > > > resume they can be unmasked?
> > > >
> > > > No, this is for an line that has both wake and interrupt enabled. To
> > > > reword the commit message above:
> > >
> > > Is my understanding correct that there isn't a different "wake up"
> > > register that can be written to cause a GPIO to be configured to wake
> > > the system from suspend? The only way to do so is to leave the GPIO
> > > unmasked in the hardware by having EINT_EN[irq] = 1? And thus any
> > > interrupts that we don't want to wake us up during suspend should be
> > > masked in the hardware?
> >
> > Yes, that's my understanding as well.
> >
> > And then, what this driver does is to emulate the behaviour of a
> > controller that would actually have separate irq and wake enable
> > registers.
> >
> > > If that's true, the code here that's trying to keep track of enabled
> > > irqs and wakeup enabled irqs can be replaced with the irqchip flag so
> > > that wakeup irqs are not masked while non-wakeups are masked.
> >
> > Correct, but with the caveat that I don't see anything that definitely
> > requires an interrupt to be enabled to be a wake source. See below...
> >
> > >
> > > > 1. cur_mask[irq] = 1; wake_mask[irq] = 1; EINT_EN[irq] = 1 (interrupt
> > > > enabled at hardware level)
> > > > 2. System suspends, resumes due to that line (at this stage EINT_HW
> > > > == wake_mask)
> > > > 3. irq_pm_check_wakeup is called, and disables the interrupt =>
> > > > EINT_EN[irq] = 0, but we still have cur_mask[irq] = 1
> > > > 4. mtk_eint_do_resume is called, and restores EINT_EN = cur_mask, so
> > > > it reenables EINT_EN[irq] = 1 => interrupt storm.
> > > >
> > > > This patch fixes the issue in step 3. So that the interrupt can be
> > > > re-enabled properly later on, sometimes after mtk_eint_do_resume, when
> > > > the driver is ready to handle it.
> > >
> > > Right, we'd rather not see irqchip drivers working around the genirq
> > > layer to do these things like tracking cur_mask and wake_mask. That
> > > leads to subtle bugs and makes the driver maintain state across the
> > > irqchip callbacks and system suspend/resume.
> > >
> > > >
> > > > Also, IRQCHIP_MASK_ON_SUSPEND does not handle lines that are enabled
> > > > as a wake source, but without interrupt enabled (e.g. cros_ec driver
> > > > does that), which we do want to support.
> > >
> > > Hmm. I thought that even if the irq is disabled by a driver, that would
> > > be a lazy disable so it isn't really masked in the hardware. Then if an
> > > interrupt comes in during suspend on a wake configured irq line, the
> > > hardware will have left it unmasked because IRQCHIP_MASK_ON_SUSPEND in
> > > combination with lazy disable would mean that the line is left unmasked
> > > (ignoring whatever this mediatek driver is doing to mask and unmask in
> > > PM hooks).
> >
> > At the very least, that's not what happens with this system. The
> > interrupt is definitely not kept enabled in suspend, and the system
> > would not wake from an EC interrupt. (see also this series, BTW:
> > https://patchwork.kernel.org/cover/10921121/).
> >
> > > Just reading Documentation/power/suspend-and-interrupts.txt I'm led to
> > > believe that the cros_ec driver shouldn't call disable_irq() on the
> > > interrupt if it wants to wakeup from it:
> > >
> > > "Calling enable_irq_wake() causes suspend_device_irqs() to treat the
> > > given IRQ in a special way. Namely, the IRQ remains enabled, by on the
> > > first interrupt it will be disabled, marked as pending and "suspended"
> > > so that it will be re-enabled by resume_device_irqs() during the
> > > subsequent system resume. Also the PM core is notified about the event
> > > which causes the system suspend in progress to be aborted (that doesn't
> > > have to happen immediately, but at one of the points where the suspend
> > > thread looks for pending wakeup events)."
> >
> > I think this describes the behaviour when you keep both enabled.
> >
> > > I suppose the problem is an irq line disabled in hardware that has
> > > wakeup armed on it? Is this even valid? Shouldn't an irq be enabled for
> > > wakeup to work?
> >
> > I couldn't really find a definite answer, but there are a bunch of
> > examples of other drivers in the kernel:
> > - drivers/extcon/extcon-usb-gpio.c:usb_extcon_suspend
> > - drivers/hid/i2c-hid/i2c-hid.c:i2c_hid_suspend
> > - drivers/mfd/max77843.c:max77843_suspend
> > (not exhaustive, this is quite hard to grep for...)
> >
> > > We could immediately unmask those lines in the hardware when the
> > > set_wake() callback is called. That way the genirq layer can use the
> > > driver to do what it wants with the hardware and the driver can make
> > > sure that set_wake() will always cause the wakeup interrupt to be
> > > delivered to genirq even when software has disabled it.
> > >
> > > But I think that there might be a problem with how genirq understands
> > > the masked state of a line when the wakeup implementation conflates
> > > masked state with wakeup armed state. Consider this call-flow:
> > >
> > > irq masked in hardware, IRQD_IRQ_MASKED is set
> > > enable_irq_wake()
> > > unmask_irq in hardware
> > > IRQD_WAKEUP_ARMED is set
> > > <suspend and wakeup from irq>
> > > handle_level_irq()
> > > mask_ack_irq()
> > > mask_irq()
> > > if (irqd_irq_masked()) -> returns true and skips masking!
> > > if (desc->irq_data.chip->irq_ack)
> > > ...
> > > irq_may_run()
> > > irq_pm_check_wakeup()
> > > irq_disable()
> > > mask_irq() -> does nothing again
> > >
> > > In the above flow, we never mask the irq because we thought it was
> > > already masked when it was disabled, but the irqchip implementation
> > > unmasked it to make wakeup work. Maybe we should always mask the irq if
> > > wakeup is armed and we're trying to call mask_irq()? Looks hacky.
Maybe we can implement irqchip's mask_ack_irq in mediatek driver to
always mask the irq. Then flow will always call it without judgment
IRQD_IRQ_MASKED.
diff --git a/drivers/pinctrl/mediatek/mtk-eint.c
b/drivers/pinctrl/mediatek/mtk-
index f464f8c..9f1aae2 100644
--- a/drivers/pinctrl/mediatek/mtk-eint.c
+++ b/drivers/pinctrl/mediatek/mtk-eint.c
@@ -272,12 +272,19 @@ static void mtk_eint_irq_release_resources(struct
irq_data
gpiochip_unlock_as_irq(gpio_c, gpio_n);
}
+static void mtk_eint_mask_ack(struct irq_data *d)
+{
+ mtk_eint_mask(d);
+ mtk_eint_ack(d);
+}
+
static struct irq_chip mtk_eint_irq_chip = {
.name = "mt-eint",
.irq_disable = mtk_eint_mask,
.irq_mask = mtk_eint_mask,
.irq_unmask = mtk_eint_unmask,
.irq_ack = mtk_eint_ack,
+ .irq_mask_ack = mtk_eint_mask_ack,
.irq_set_type = mtk_eint_set_type,
.irq_set_wake = mtk_eint_irq_set_wake,
.irq_request_resources = mtk_eint_irq_request_resources,
This seems like a small change.
thanks.
> > >
> > > diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> > > index 51128bea3846..20257d528880 100644
> > > --- a/kernel/irq/chip.c
> > > +++ b/kernel/irq/chip.c
> > > @@ -411,7 +411,7 @@ static inline void mask_ack_irq(struct irq_desc *desc)
> > >
> > > void mask_irq(struct irq_desc *desc)
> > > {
> > > - if (irqd_irq_masked(&desc->irq_data))
> > > + if (!irqd_is_wakeup_armed(&desc->irq_data) && irqd_irq_masked(&desc->irq_data))
> > > return;
> > >
> > > if (desc->irq_data.chip->irq_mask) {
> >
> > I'm... really not sure what's the best approach here. But basically,
> > yes, if we can find a way to properly handle wake and interrupt
> > behaviour for drivers with a single mask, that'd be good.
> > IRQCHIP_MASK_ON_SUSPEND only seems to be doing half of the work, since
> > it does not cover the disable+wake source case.
> >
> > Thanks,
>
> I finally got around to studying this patch. This series seems okay to
> me. The underlying problem is really that the hardware IRQ enabled
> state is out of sync with what Linux thinks. This happens during
> suspend because Linux thinks the irq is disabled, but due to the
> hardware constraints on this platform, the interrupt has to be enabled
> for it to be a wake source. So the mtk driver re-enables the
> interrupt, and then has to find a way to get back in sync with Linux's
> IRQ mask state.
>
> One possible approach is mentioned above by Stephen: stop calling
> disable_irq in the cros EC driver. Then both linux and mtk agree the
> interrupt is enabled at suspend time. I think this ran into other
> problems though, where the EC gets its interrupt but is unable to
> silence it because the underlying SPI bus is still suspended.
>
> The other approach, taken here, is to mask the interrupt when it first
> comes in, getting Linux and mtk back in agreement that yes, the
> interrupt is masked. Outside of enlightening the generic IRQ core
> about these types of interrupts that need to get re-enabled to be wake
> sources, this seems like a reasonable approach.
> -Evan
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 50+ messages in thread* Re: [PATCH 2/2] pinctrl: mediatek: Update cur_mask in mask/mask ops
@ 2019-05-31 8:05 ` Chuanjia Liu
0 siblings, 0 replies; 50+ messages in thread
From: Chuanjia Liu @ 2019-05-31 8:05 UTC (permalink / raw)
To: Evan Green
Cc: Nicolas Boichat, Stephen Boyd,
moderated list:ARM/Mediatek SoC support, Sean Wang,
Linus Walleij, Matthias Brugger, linux-gpio,
linux-arm Mailing List, lkml, Thomas Gleixner
On Thu, 2019-05-30 at 10:12 -0700, Evan Green wrote:
> On Wed, May 15, 2019 at 1:05 AM Nicolas Boichat <drinkcat@chromium.org> wrote:
> >
> > On Wed, May 15, 2019 at 4:14 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > Quoting Nicolas Boichat (2019-05-13 18:37:58)
> > > > On Tue, May 14, 2019 at 6:29 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > > > >
> > > > > Quoting Nicolas Boichat (2019-04-28 20:55:15)
> > > > > > During suspend/resume, mtk_eint_mask may be called while
> > > > > > wake_mask is active. For example, this happens if a wake-source
> > > > > > with an active interrupt handler wakes the system:
> > > > > > irq/pm.c:irq_pm_check_wakeup would disable the interrupt, so
> > > > > > that it can be handled later on in the resume flow.
> > > > > >
> > > > > > However, this may happen before mtk_eint_do_resume is called:
> > > > > > in this case, wake_mask is loaded, and cur_mask is restored
> > > > > > from an older copy, re-enabling the interrupt, and causing
> > > > > > an interrupt storm (especially for level interrupts).
> > > > > >
> > > > > > Instead, we just record mask/unmask changes in cur_mask. This
> > > > > > also avoids the need to read the current mask in eint_do_suspend,
> > > > > > and we can remove mtk_eint_chip_read_mask function.
> > > > > >
> > > > > > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> > > > >
> > > > > It looks an awful lot like you should just use IRQCHIP_MASK_ON_SUSPEND
> > > > > here. Isn't that what's happening? All non-wake irqs should be masked at
> > > > > the hardware level so they can't cause a wakeup during suspend and on
> > > > > resume they can be unmasked?
> > > >
> > > > No, this is for an line that has both wake and interrupt enabled. To
> > > > reword the commit message above:
> > >
> > > Is my understanding correct that there isn't a different "wake up"
> > > register that can be written to cause a GPIO to be configured to wake
> > > the system from suspend? The only way to do so is to leave the GPIO
> > > unmasked in the hardware by having EINT_EN[irq] = 1? And thus any
> > > interrupts that we don't want to wake us up during suspend should be
> > > masked in the hardware?
> >
> > Yes, that's my understanding as well.
> >
> > And then, what this driver does is to emulate the behaviour of a
> > controller that would actually have separate irq and wake enable
> > registers.
> >
> > > If that's true, the code here that's trying to keep track of enabled
> > > irqs and wakeup enabled irqs can be replaced with the irqchip flag so
> > > that wakeup irqs are not masked while non-wakeups are masked.
> >
> > Correct, but with the caveat that I don't see anything that definitely
> > requires an interrupt to be enabled to be a wake source. See below...
> >
> > >
> > > > 1. cur_mask[irq] = 1; wake_mask[irq] = 1; EINT_EN[irq] = 1 (interrupt
> > > > enabled at hardware level)
> > > > 2. System suspends, resumes due to that line (at this stage EINT_HW
> > > > == wake_mask)
> > > > 3. irq_pm_check_wakeup is called, and disables the interrupt =>
> > > > EINT_EN[irq] = 0, but we still have cur_mask[irq] = 1
> > > > 4. mtk_eint_do_resume is called, and restores EINT_EN = cur_mask, so
> > > > it reenables EINT_EN[irq] = 1 => interrupt storm.
> > > >
> > > > This patch fixes the issue in step 3. So that the interrupt can be
> > > > re-enabled properly later on, sometimes after mtk_eint_do_resume, when
> > > > the driver is ready to handle it.
> > >
> > > Right, we'd rather not see irqchip drivers working around the genirq
> > > layer to do these things like tracking cur_mask and wake_mask. That
> > > leads to subtle bugs and makes the driver maintain state across the
> > > irqchip callbacks and system suspend/resume.
> > >
> > > >
> > > > Also, IRQCHIP_MASK_ON_SUSPEND does not handle lines that are enabled
> > > > as a wake source, but without interrupt enabled (e.g. cros_ec driver
> > > > does that), which we do want to support.
> > >
> > > Hmm. I thought that even if the irq is disabled by a driver, that would
> > > be a lazy disable so it isn't really masked in the hardware. Then if an
> > > interrupt comes in during suspend on a wake configured irq line, the
> > > hardware will have left it unmasked because IRQCHIP_MASK_ON_SUSPEND in
> > > combination with lazy disable would mean that the line is left unmasked
> > > (ignoring whatever this mediatek driver is doing to mask and unmask in
> > > PM hooks).
> >
> > At the very least, that's not what happens with this system. The
> > interrupt is definitely not kept enabled in suspend, and the system
> > would not wake from an EC interrupt. (see also this series, BTW:
> > https://patchwork.kernel.org/cover/10921121/).
> >
> > > Just reading Documentation/power/suspend-and-interrupts.txt I'm led to
> > > believe that the cros_ec driver shouldn't call disable_irq() on the
> > > interrupt if it wants to wakeup from it:
> > >
> > > "Calling enable_irq_wake() causes suspend_device_irqs() to treat the
> > > given IRQ in a special way. Namely, the IRQ remains enabled, by on the
> > > first interrupt it will be disabled, marked as pending and "suspended"
> > > so that it will be re-enabled by resume_device_irqs() during the
> > > subsequent system resume. Also the PM core is notified about the event
> > > which causes the system suspend in progress to be aborted (that doesn't
> > > have to happen immediately, but at one of the points where the suspend
> > > thread looks for pending wakeup events)."
> >
> > I think this describes the behaviour when you keep both enabled.
> >
> > > I suppose the problem is an irq line disabled in hardware that has
> > > wakeup armed on it? Is this even valid? Shouldn't an irq be enabled for
> > > wakeup to work?
> >
> > I couldn't really find a definite answer, but there are a bunch of
> > examples of other drivers in the kernel:
> > - drivers/extcon/extcon-usb-gpio.c:usb_extcon_suspend
> > - drivers/hid/i2c-hid/i2c-hid.c:i2c_hid_suspend
> > - drivers/mfd/max77843.c:max77843_suspend
> > (not exhaustive, this is quite hard to grep for...)
> >
> > > We could immediately unmask those lines in the hardware when the
> > > set_wake() callback is called. That way the genirq layer can use the
> > > driver to do what it wants with the hardware and the driver can make
> > > sure that set_wake() will always cause the wakeup interrupt to be
> > > delivered to genirq even when software has disabled it.
> > >
> > > But I think that there might be a problem with how genirq understands
> > > the masked state of a line when the wakeup implementation conflates
> > > masked state with wakeup armed state. Consider this call-flow:
> > >
> > > irq masked in hardware, IRQD_IRQ_MASKED is set
> > > enable_irq_wake()
> > > unmask_irq in hardware
> > > IRQD_WAKEUP_ARMED is set
> > > <suspend and wakeup from irq>
> > > handle_level_irq()
> > > mask_ack_irq()
> > > mask_irq()
> > > if (irqd_irq_masked()) -> returns true and skips masking!
> > > if (desc->irq_data.chip->irq_ack)
> > > ...
> > > irq_may_run()
> > > irq_pm_check_wakeup()
> > > irq_disable()
> > > mask_irq() -> does nothing again
> > >
> > > In the above flow, we never mask the irq because we thought it was
> > > already masked when it was disabled, but the irqchip implementation
> > > unmasked it to make wakeup work. Maybe we should always mask the irq if
> > > wakeup is armed and we're trying to call mask_irq()? Looks hacky.
Maybe we can implement irqchip's mask_ack_irq in mediatek driver to
always mask the irq. Then flow will always call it without judgment
IRQD_IRQ_MASKED.
diff --git a/drivers/pinctrl/mediatek/mtk-eint.c
b/drivers/pinctrl/mediatek/mtk-
index f464f8c..9f1aae2 100644
--- a/drivers/pinctrl/mediatek/mtk-eint.c
+++ b/drivers/pinctrl/mediatek/mtk-eint.c
@@ -272,12 +272,19 @@ static void mtk_eint_irq_release_resources(struct
irq_data
gpiochip_unlock_as_irq(gpio_c, gpio_n);
}
+static void mtk_eint_mask_ack(struct irq_data *d)
+{
+ mtk_eint_mask(d);
+ mtk_eint_ack(d);
+}
+
static struct irq_chip mtk_eint_irq_chip = {
.name = "mt-eint",
.irq_disable = mtk_eint_mask,
.irq_mask = mtk_eint_mask,
.irq_unmask = mtk_eint_unmask,
.irq_ack = mtk_eint_ack,
+ .irq_mask_ack = mtk_eint_mask_ack,
.irq_set_type = mtk_eint_set_type,
.irq_set_wake = mtk_eint_irq_set_wake,
.irq_request_resources = mtk_eint_irq_request_resources,
This seems like a small change.
thanks.
> > >
> > > diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> > > index 51128bea3846..20257d528880 100644
> > > --- a/kernel/irq/chip.c
> > > +++ b/kernel/irq/chip.c
> > > @@ -411,7 +411,7 @@ static inline void mask_ack_irq(struct irq_desc *desc)
> > >
> > > void mask_irq(struct irq_desc *desc)
> > > {
> > > - if (irqd_irq_masked(&desc->irq_data))
> > > + if (!irqd_is_wakeup_armed(&desc->irq_data) && irqd_irq_masked(&desc->irq_data))
> > > return;
> > >
> > > if (desc->irq_data.chip->irq_mask) {
> >
> > I'm... really not sure what's the best approach here. But basically,
> > yes, if we can find a way to properly handle wake and interrupt
> > behaviour for drivers with a single mask, that'd be good.
> > IRQCHIP_MASK_ON_SUSPEND only seems to be doing half of the work, since
> > it does not cover the disable+wake source case.
> >
> > Thanks,
>
> I finally got around to studying this patch. This series seems okay to
> me. The underlying problem is really that the hardware IRQ enabled
> state is out of sync with what Linux thinks. This happens during
> suspend because Linux thinks the irq is disabled, but due to the
> hardware constraints on this platform, the interrupt has to be enabled
> for it to be a wake source. So the mtk driver re-enables the
> interrupt, and then has to find a way to get back in sync with Linux's
> IRQ mask state.
>
> One possible approach is mentioned above by Stephen: stop calling
> disable_irq in the cros EC driver. Then both linux and mtk agree the
> interrupt is enabled at suspend time. I think this ran into other
> problems though, where the EC gets its interrupt but is unable to
> silence it because the underlying SPI bus is still suspended.
>
> The other approach, taken here, is to mask the interrupt when it first
> comes in, getting Linux and mtk back in agreement that yes, the
> interrupt is masked. Outside of enlightening the generic IRQ core
> about these types of interrupts that need to get re-enabled to be wake
> sources, this seems like a reasonable approach.
> -Evan
^ permalink raw reply related [flat|nested] 50+ messages in thread* Re: [PATCH 2/2] pinctrl: mediatek: Update cur_mask in mask/mask ops
2019-05-31 8:05 ` Chuanjia Liu
(?)
@ 2019-05-31 17:17 ` Evan Green
-1 siblings, 0 replies; 50+ messages in thread
From: Evan Green @ 2019-05-31 17:17 UTC (permalink / raw)
To: Chuanjia Liu
Cc: Nicolas Boichat, Stephen Boyd,
moderated list:ARM/Mediatek SoC support, Sean Wang,
Linus Walleij, Matthias Brugger, linux-gpio,
linux-arm Mailing List, lkml, Thomas Gleixner
On Fri, May 31, 2019 at 1:06 AM Chuanjia Liu <Chuanjia.Liu@mediatek.com> wrote:
>
> On Thu, 2019-05-30 at 10:12 -0700, Evan Green wrote:
> > On Wed, May 15, 2019 at 1:05 AM Nicolas Boichat <drinkcat@chromium.org> wrote:
> > >
> > > On Wed, May 15, 2019 at 4:14 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > > >
> > > > Quoting Nicolas Boichat (2019-05-13 18:37:58)
> > > > > On Tue, May 14, 2019 at 6:29 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > > > > >
> > > > > > Quoting Nicolas Boichat (2019-04-28 20:55:15)
> > > > > > > During suspend/resume, mtk_eint_mask may be called while
> > > > > > > wake_mask is active. For example, this happens if a wake-source
> > > > > > > with an active interrupt handler wakes the system:
> > > > > > > irq/pm.c:irq_pm_check_wakeup would disable the interrupt, so
> > > > > > > that it can be handled later on in the resume flow.
> > > > > > >
> > > > > > > However, this may happen before mtk_eint_do_resume is called:
> > > > > > > in this case, wake_mask is loaded, and cur_mask is restored
> > > > > > > from an older copy, re-enabling the interrupt, and causing
> > > > > > > an interrupt storm (especially for level interrupts).
> > > > > > >
> > > > > > > Instead, we just record mask/unmask changes in cur_mask. This
> > > > > > > also avoids the need to read the current mask in eint_do_suspend,
> > > > > > > and we can remove mtk_eint_chip_read_mask function.
> > > > > > >
> > > > > > > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> > > > > >
> > > > > > It looks an awful lot like you should just use IRQCHIP_MASK_ON_SUSPEND
> > > > > > here. Isn't that what's happening? All non-wake irqs should be masked at
> > > > > > the hardware level so they can't cause a wakeup during suspend and on
> > > > > > resume they can be unmasked?
> > > > >
> > > > > No, this is for an line that has both wake and interrupt enabled. To
> > > > > reword the commit message above:
> > > >
> > > > Is my understanding correct that there isn't a different "wake up"
> > > > register that can be written to cause a GPIO to be configured to wake
> > > > the system from suspend? The only way to do so is to leave the GPIO
> > > > unmasked in the hardware by having EINT_EN[irq] = 1? And thus any
> > > > interrupts that we don't want to wake us up during suspend should be
> > > > masked in the hardware?
> > >
> > > Yes, that's my understanding as well.
> > >
> > > And then, what this driver does is to emulate the behaviour of a
> > > controller that would actually have separate irq and wake enable
> > > registers.
> > >
> > > > If that's true, the code here that's trying to keep track of enabled
> > > > irqs and wakeup enabled irqs can be replaced with the irqchip flag so
> > > > that wakeup irqs are not masked while non-wakeups are masked.
> > >
> > > Correct, but with the caveat that I don't see anything that definitely
> > > requires an interrupt to be enabled to be a wake source. See below...
> > >
> > > >
> > > > > 1. cur_mask[irq] = 1; wake_mask[irq] = 1; EINT_EN[irq] = 1 (interrupt
> > > > > enabled at hardware level)
> > > > > 2. System suspends, resumes due to that line (at this stage EINT_HW
> > > > > == wake_mask)
> > > > > 3. irq_pm_check_wakeup is called, and disables the interrupt =>
> > > > > EINT_EN[irq] = 0, but we still have cur_mask[irq] = 1
> > > > > 4. mtk_eint_do_resume is called, and restores EINT_EN = cur_mask, so
> > > > > it reenables EINT_EN[irq] = 1 => interrupt storm.
> > > > >
> > > > > This patch fixes the issue in step 3. So that the interrupt can be
> > > > > re-enabled properly later on, sometimes after mtk_eint_do_resume, when
> > > > > the driver is ready to handle it.
> > > >
> > > > Right, we'd rather not see irqchip drivers working around the genirq
> > > > layer to do these things like tracking cur_mask and wake_mask. That
> > > > leads to subtle bugs and makes the driver maintain state across the
> > > > irqchip callbacks and system suspend/resume.
> > > >
> > > > >
> > > > > Also, IRQCHIP_MASK_ON_SUSPEND does not handle lines that are enabled
> > > > > as a wake source, but without interrupt enabled (e.g. cros_ec driver
> > > > > does that), which we do want to support.
> > > >
> > > > Hmm. I thought that even if the irq is disabled by a driver, that would
> > > > be a lazy disable so it isn't really masked in the hardware. Then if an
> > > > interrupt comes in during suspend on a wake configured irq line, the
> > > > hardware will have left it unmasked because IRQCHIP_MASK_ON_SUSPEND in
> > > > combination with lazy disable would mean that the line is left unmasked
> > > > (ignoring whatever this mediatek driver is doing to mask and unmask in
> > > > PM hooks).
> > >
> > > At the very least, that's not what happens with this system. The
> > > interrupt is definitely not kept enabled in suspend, and the system
> > > would not wake from an EC interrupt. (see also this series, BTW:
> > > https://patchwork.kernel.org/cover/10921121/).
> > >
> > > > Just reading Documentation/power/suspend-and-interrupts.txt I'm led to
> > > > believe that the cros_ec driver shouldn't call disable_irq() on the
> > > > interrupt if it wants to wakeup from it:
> > > >
> > > > "Calling enable_irq_wake() causes suspend_device_irqs() to treat the
> > > > given IRQ in a special way. Namely, the IRQ remains enabled, by on the
> > > > first interrupt it will be disabled, marked as pending and "suspended"
> > > > so that it will be re-enabled by resume_device_irqs() during the
> > > > subsequent system resume. Also the PM core is notified about the event
> > > > which causes the system suspend in progress to be aborted (that doesn't
> > > > have to happen immediately, but at one of the points where the suspend
> > > > thread looks for pending wakeup events)."
> > >
> > > I think this describes the behaviour when you keep both enabled.
> > >
> > > > I suppose the problem is an irq line disabled in hardware that has
> > > > wakeup armed on it? Is this even valid? Shouldn't an irq be enabled for
> > > > wakeup to work?
> > >
> > > I couldn't really find a definite answer, but there are a bunch of
> > > examples of other drivers in the kernel:
> > > - drivers/extcon/extcon-usb-gpio.c:usb_extcon_suspend
> > > - drivers/hid/i2c-hid/i2c-hid.c:i2c_hid_suspend
> > > - drivers/mfd/max77843.c:max77843_suspend
> > > (not exhaustive, this is quite hard to grep for...)
> > >
> > > > We could immediately unmask those lines in the hardware when the
> > > > set_wake() callback is called. That way the genirq layer can use the
> > > > driver to do what it wants with the hardware and the driver can make
> > > > sure that set_wake() will always cause the wakeup interrupt to be
> > > > delivered to genirq even when software has disabled it.
> > > >
> > > > But I think that there might be a problem with how genirq understands
> > > > the masked state of a line when the wakeup implementation conflates
> > > > masked state with wakeup armed state. Consider this call-flow:
> > > >
> > > > irq masked in hardware, IRQD_IRQ_MASKED is set
> > > > enable_irq_wake()
> > > > unmask_irq in hardware
> > > > IRQD_WAKEUP_ARMED is set
> > > > <suspend and wakeup from irq>
> > > > handle_level_irq()
> > > > mask_ack_irq()
> > > > mask_irq()
> > > > if (irqd_irq_masked()) -> returns true and skips masking!
> > > > if (desc->irq_data.chip->irq_ack)
> > > > ...
> > > > irq_may_run()
> > > > irq_pm_check_wakeup()
> > > > irq_disable()
> > > > mask_irq() -> does nothing again
> > > >
> > > > In the above flow, we never mask the irq because we thought it was
> > > > already masked when it was disabled, but the irqchip implementation
> > > > unmasked it to make wakeup work. Maybe we should always mask the irq if
> > > > wakeup is armed and we're trying to call mask_irq()? Looks hacky.
> Maybe we can implement irqchip's mask_ack_irq in mediatek driver to
> always mask the irq. Then flow will always call it without judgment
> IRQD_IRQ_MASKED.
>
> diff --git a/drivers/pinctrl/mediatek/mtk-eint.c
> b/drivers/pinctrl/mediatek/mtk-
> index f464f8c..9f1aae2 100644
> --- a/drivers/pinctrl/mediatek/mtk-eint.c
> +++ b/drivers/pinctrl/mediatek/mtk-eint.c
> @@ -272,12 +272,19 @@ static void mtk_eint_irq_release_resources(struct
> irq_data
> gpiochip_unlock_as_irq(gpio_c, gpio_n);
> }
>
> +static void mtk_eint_mask_ack(struct irq_data *d)
> +{
> + mtk_eint_mask(d);
> + mtk_eint_ack(d);
> +}
> +
> static struct irq_chip mtk_eint_irq_chip = {
> .name = "mt-eint",
> .irq_disable = mtk_eint_mask,
> .irq_mask = mtk_eint_mask,
> .irq_unmask = mtk_eint_unmask,
> .irq_ack = mtk_eint_ack,
> + .irq_mask_ack = mtk_eint_mask_ack,
> .irq_set_type = mtk_eint_set_type,
> .irq_set_wake = mtk_eint_irq_set_wake,
> .irq_request_resources = mtk_eint_irq_request_resources,
>
> This seems like a small change.
> thanks.
Does this work? My understanding is that Linux thinks the irq is
_already_ masked, so it short-circuits in the generic IRQ code and
doesn't call mask again.
-Evan
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH 2/2] pinctrl: mediatek: Update cur_mask in mask/mask ops
@ 2019-05-31 17:17 ` Evan Green
0 siblings, 0 replies; 50+ messages in thread
From: Evan Green @ 2019-05-31 17:17 UTC (permalink / raw)
To: Chuanjia Liu
Cc: Nicolas Boichat, Linus Walleij, Sean Wang, lkml, Stephen Boyd,
linux-gpio, moderated list:ARM/Mediatek SoC support,
Matthias Brugger, Thomas Gleixner, linux-arm Mailing List
On Fri, May 31, 2019 at 1:06 AM Chuanjia Liu <Chuanjia.Liu@mediatek.com> wrote:
>
> On Thu, 2019-05-30 at 10:12 -0700, Evan Green wrote:
> > On Wed, May 15, 2019 at 1:05 AM Nicolas Boichat <drinkcat@chromium.org> wrote:
> > >
> > > On Wed, May 15, 2019 at 4:14 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > > >
> > > > Quoting Nicolas Boichat (2019-05-13 18:37:58)
> > > > > On Tue, May 14, 2019 at 6:29 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > > > > >
> > > > > > Quoting Nicolas Boichat (2019-04-28 20:55:15)
> > > > > > > During suspend/resume, mtk_eint_mask may be called while
> > > > > > > wake_mask is active. For example, this happens if a wake-source
> > > > > > > with an active interrupt handler wakes the system:
> > > > > > > irq/pm.c:irq_pm_check_wakeup would disable the interrupt, so
> > > > > > > that it can be handled later on in the resume flow.
> > > > > > >
> > > > > > > However, this may happen before mtk_eint_do_resume is called:
> > > > > > > in this case, wake_mask is loaded, and cur_mask is restored
> > > > > > > from an older copy, re-enabling the interrupt, and causing
> > > > > > > an interrupt storm (especially for level interrupts).
> > > > > > >
> > > > > > > Instead, we just record mask/unmask changes in cur_mask. This
> > > > > > > also avoids the need to read the current mask in eint_do_suspend,
> > > > > > > and we can remove mtk_eint_chip_read_mask function.
> > > > > > >
> > > > > > > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> > > > > >
> > > > > > It looks an awful lot like you should just use IRQCHIP_MASK_ON_SUSPEND
> > > > > > here. Isn't that what's happening? All non-wake irqs should be masked at
> > > > > > the hardware level so they can't cause a wakeup during suspend and on
> > > > > > resume they can be unmasked?
> > > > >
> > > > > No, this is for an line that has both wake and interrupt enabled. To
> > > > > reword the commit message above:
> > > >
> > > > Is my understanding correct that there isn't a different "wake up"
> > > > register that can be written to cause a GPIO to be configured to wake
> > > > the system from suspend? The only way to do so is to leave the GPIO
> > > > unmasked in the hardware by having EINT_EN[irq] = 1? And thus any
> > > > interrupts that we don't want to wake us up during suspend should be
> > > > masked in the hardware?
> > >
> > > Yes, that's my understanding as well.
> > >
> > > And then, what this driver does is to emulate the behaviour of a
> > > controller that would actually have separate irq and wake enable
> > > registers.
> > >
> > > > If that's true, the code here that's trying to keep track of enabled
> > > > irqs and wakeup enabled irqs can be replaced with the irqchip flag so
> > > > that wakeup irqs are not masked while non-wakeups are masked.
> > >
> > > Correct, but with the caveat that I don't see anything that definitely
> > > requires an interrupt to be enabled to be a wake source. See below...
> > >
> > > >
> > > > > 1. cur_mask[irq] = 1; wake_mask[irq] = 1; EINT_EN[irq] = 1 (interrupt
> > > > > enabled at hardware level)
> > > > > 2. System suspends, resumes due to that line (at this stage EINT_HW
> > > > > == wake_mask)
> > > > > 3. irq_pm_check_wakeup is called, and disables the interrupt =>
> > > > > EINT_EN[irq] = 0, but we still have cur_mask[irq] = 1
> > > > > 4. mtk_eint_do_resume is called, and restores EINT_EN = cur_mask, so
> > > > > it reenables EINT_EN[irq] = 1 => interrupt storm.
> > > > >
> > > > > This patch fixes the issue in step 3. So that the interrupt can be
> > > > > re-enabled properly later on, sometimes after mtk_eint_do_resume, when
> > > > > the driver is ready to handle it.
> > > >
> > > > Right, we'd rather not see irqchip drivers working around the genirq
> > > > layer to do these things like tracking cur_mask and wake_mask. That
> > > > leads to subtle bugs and makes the driver maintain state across the
> > > > irqchip callbacks and system suspend/resume.
> > > >
> > > > >
> > > > > Also, IRQCHIP_MASK_ON_SUSPEND does not handle lines that are enabled
> > > > > as a wake source, but without interrupt enabled (e.g. cros_ec driver
> > > > > does that), which we do want to support.
> > > >
> > > > Hmm. I thought that even if the irq is disabled by a driver, that would
> > > > be a lazy disable so it isn't really masked in the hardware. Then if an
> > > > interrupt comes in during suspend on a wake configured irq line, the
> > > > hardware will have left it unmasked because IRQCHIP_MASK_ON_SUSPEND in
> > > > combination with lazy disable would mean that the line is left unmasked
> > > > (ignoring whatever this mediatek driver is doing to mask and unmask in
> > > > PM hooks).
> > >
> > > At the very least, that's not what happens with this system. The
> > > interrupt is definitely not kept enabled in suspend, and the system
> > > would not wake from an EC interrupt. (see also this series, BTW:
> > > https://patchwork.kernel.org/cover/10921121/).
> > >
> > > > Just reading Documentation/power/suspend-and-interrupts.txt I'm led to
> > > > believe that the cros_ec driver shouldn't call disable_irq() on the
> > > > interrupt if it wants to wakeup from it:
> > > >
> > > > "Calling enable_irq_wake() causes suspend_device_irqs() to treat the
> > > > given IRQ in a special way. Namely, the IRQ remains enabled, by on the
> > > > first interrupt it will be disabled, marked as pending and "suspended"
> > > > so that it will be re-enabled by resume_device_irqs() during the
> > > > subsequent system resume. Also the PM core is notified about the event
> > > > which causes the system suspend in progress to be aborted (that doesn't
> > > > have to happen immediately, but at one of the points where the suspend
> > > > thread looks for pending wakeup events)."
> > >
> > > I think this describes the behaviour when you keep both enabled.
> > >
> > > > I suppose the problem is an irq line disabled in hardware that has
> > > > wakeup armed on it? Is this even valid? Shouldn't an irq be enabled for
> > > > wakeup to work?
> > >
> > > I couldn't really find a definite answer, but there are a bunch of
> > > examples of other drivers in the kernel:
> > > - drivers/extcon/extcon-usb-gpio.c:usb_extcon_suspend
> > > - drivers/hid/i2c-hid/i2c-hid.c:i2c_hid_suspend
> > > - drivers/mfd/max77843.c:max77843_suspend
> > > (not exhaustive, this is quite hard to grep for...)
> > >
> > > > We could immediately unmask those lines in the hardware when the
> > > > set_wake() callback is called. That way the genirq layer can use the
> > > > driver to do what it wants with the hardware and the driver can make
> > > > sure that set_wake() will always cause the wakeup interrupt to be
> > > > delivered to genirq even when software has disabled it.
> > > >
> > > > But I think that there might be a problem with how genirq understands
> > > > the masked state of a line when the wakeup implementation conflates
> > > > masked state with wakeup armed state. Consider this call-flow:
> > > >
> > > > irq masked in hardware, IRQD_IRQ_MASKED is set
> > > > enable_irq_wake()
> > > > unmask_irq in hardware
> > > > IRQD_WAKEUP_ARMED is set
> > > > <suspend and wakeup from irq>
> > > > handle_level_irq()
> > > > mask_ack_irq()
> > > > mask_irq()
> > > > if (irqd_irq_masked()) -> returns true and skips masking!
> > > > if (desc->irq_data.chip->irq_ack)
> > > > ...
> > > > irq_may_run()
> > > > irq_pm_check_wakeup()
> > > > irq_disable()
> > > > mask_irq() -> does nothing again
> > > >
> > > > In the above flow, we never mask the irq because we thought it was
> > > > already masked when it was disabled, but the irqchip implementation
> > > > unmasked it to make wakeup work. Maybe we should always mask the irq if
> > > > wakeup is armed and we're trying to call mask_irq()? Looks hacky.
> Maybe we can implement irqchip's mask_ack_irq in mediatek driver to
> always mask the irq. Then flow will always call it without judgment
> IRQD_IRQ_MASKED.
>
> diff --git a/drivers/pinctrl/mediatek/mtk-eint.c
> b/drivers/pinctrl/mediatek/mtk-
> index f464f8c..9f1aae2 100644
> --- a/drivers/pinctrl/mediatek/mtk-eint.c
> +++ b/drivers/pinctrl/mediatek/mtk-eint.c
> @@ -272,12 +272,19 @@ static void mtk_eint_irq_release_resources(struct
> irq_data
> gpiochip_unlock_as_irq(gpio_c, gpio_n);
> }
>
> +static void mtk_eint_mask_ack(struct irq_data *d)
> +{
> + mtk_eint_mask(d);
> + mtk_eint_ack(d);
> +}
> +
> static struct irq_chip mtk_eint_irq_chip = {
> .name = "mt-eint",
> .irq_disable = mtk_eint_mask,
> .irq_mask = mtk_eint_mask,
> .irq_unmask = mtk_eint_unmask,
> .irq_ack = mtk_eint_ack,
> + .irq_mask_ack = mtk_eint_mask_ack,
> .irq_set_type = mtk_eint_set_type,
> .irq_set_wake = mtk_eint_irq_set_wake,
> .irq_request_resources = mtk_eint_irq_request_resources,
>
> This seems like a small change.
> thanks.
Does this work? My understanding is that Linux thinks the irq is
_already_ masked, so it short-circuits in the generic IRQ code and
doesn't call mask again.
-Evan
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH 2/2] pinctrl: mediatek: Update cur_mask in mask/mask ops
@ 2019-05-31 17:17 ` Evan Green
0 siblings, 0 replies; 50+ messages in thread
From: Evan Green @ 2019-05-31 17:17 UTC (permalink / raw)
To: Chuanjia Liu
Cc: Nicolas Boichat, Stephen Boyd,
moderated list:ARM/Mediatek SoC support, Sean Wang,
Linus Walleij, Matthias Brugger, linux-gpio,
linux-arm Mailing List, lkml, Thomas Gleixner
On Fri, May 31, 2019 at 1:06 AM Chuanjia Liu <Chuanjia.Liu@mediatek.com> wrote:
>
> On Thu, 2019-05-30 at 10:12 -0700, Evan Green wrote:
> > On Wed, May 15, 2019 at 1:05 AM Nicolas Boichat <drinkcat@chromium.org> wrote:
> > >
> > > On Wed, May 15, 2019 at 4:14 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > > >
> > > > Quoting Nicolas Boichat (2019-05-13 18:37:58)
> > > > > On Tue, May 14, 2019 at 6:29 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > > > > >
> > > > > > Quoting Nicolas Boichat (2019-04-28 20:55:15)
> > > > > > > During suspend/resume, mtk_eint_mask may be called while
> > > > > > > wake_mask is active. For example, this happens if a wake-source
> > > > > > > with an active interrupt handler wakes the system:
> > > > > > > irq/pm.c:irq_pm_check_wakeup would disable the interrupt, so
> > > > > > > that it can be handled later on in the resume flow.
> > > > > > >
> > > > > > > However, this may happen before mtk_eint_do_resume is called:
> > > > > > > in this case, wake_mask is loaded, and cur_mask is restored
> > > > > > > from an older copy, re-enabling the interrupt, and causing
> > > > > > > an interrupt storm (especially for level interrupts).
> > > > > > >
> > > > > > > Instead, we just record mask/unmask changes in cur_mask. This
> > > > > > > also avoids the need to read the current mask in eint_do_suspend,
> > > > > > > and we can remove mtk_eint_chip_read_mask function.
> > > > > > >
> > > > > > > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> > > > > >
> > > > > > It looks an awful lot like you should just use IRQCHIP_MASK_ON_SUSPEND
> > > > > > here. Isn't that what's happening? All non-wake irqs should be masked at
> > > > > > the hardware level so they can't cause a wakeup during suspend and on
> > > > > > resume they can be unmasked?
> > > > >
> > > > > No, this is for an line that has both wake and interrupt enabled. To
> > > > > reword the commit message above:
> > > >
> > > > Is my understanding correct that there isn't a different "wake up"
> > > > register that can be written to cause a GPIO to be configured to wake
> > > > the system from suspend? The only way to do so is to leave the GPIO
> > > > unmasked in the hardware by having EINT_EN[irq] = 1? And thus any
> > > > interrupts that we don't want to wake us up during suspend should be
> > > > masked in the hardware?
> > >
> > > Yes, that's my understanding as well.
> > >
> > > And then, what this driver does is to emulate the behaviour of a
> > > controller that would actually have separate irq and wake enable
> > > registers.
> > >
> > > > If that's true, the code here that's trying to keep track of enabled
> > > > irqs and wakeup enabled irqs can be replaced with the irqchip flag so
> > > > that wakeup irqs are not masked while non-wakeups are masked.
> > >
> > > Correct, but with the caveat that I don't see anything that definitely
> > > requires an interrupt to be enabled to be a wake source. See below...
> > >
> > > >
> > > > > 1. cur_mask[irq] = 1; wake_mask[irq] = 1; EINT_EN[irq] = 1 (interrupt
> > > > > enabled at hardware level)
> > > > > 2. System suspends, resumes due to that line (at this stage EINT_HW
> > > > > == wake_mask)
> > > > > 3. irq_pm_check_wakeup is called, and disables the interrupt =>
> > > > > EINT_EN[irq] = 0, but we still have cur_mask[irq] = 1
> > > > > 4. mtk_eint_do_resume is called, and restores EINT_EN = cur_mask, so
> > > > > it reenables EINT_EN[irq] = 1 => interrupt storm.
> > > > >
> > > > > This patch fixes the issue in step 3. So that the interrupt can be
> > > > > re-enabled properly later on, sometimes after mtk_eint_do_resume, when
> > > > > the driver is ready to handle it.
> > > >
> > > > Right, we'd rather not see irqchip drivers working around the genirq
> > > > layer to do these things like tracking cur_mask and wake_mask. That
> > > > leads to subtle bugs and makes the driver maintain state across the
> > > > irqchip callbacks and system suspend/resume.
> > > >
> > > > >
> > > > > Also, IRQCHIP_MASK_ON_SUSPEND does not handle lines that are enabled
> > > > > as a wake source, but without interrupt enabled (e.g. cros_ec driver
> > > > > does that), which we do want to support.
> > > >
> > > > Hmm. I thought that even if the irq is disabled by a driver, that would
> > > > be a lazy disable so it isn't really masked in the hardware. Then if an
> > > > interrupt comes in during suspend on a wake configured irq line, the
> > > > hardware will have left it unmasked because IRQCHIP_MASK_ON_SUSPEND in
> > > > combination with lazy disable would mean that the line is left unmasked
> > > > (ignoring whatever this mediatek driver is doing to mask and unmask in
> > > > PM hooks).
> > >
> > > At the very least, that's not what happens with this system. The
> > > interrupt is definitely not kept enabled in suspend, and the system
> > > would not wake from an EC interrupt. (see also this series, BTW:
> > > https://patchwork.kernel.org/cover/10921121/).
> > >
> > > > Just reading Documentation/power/suspend-and-interrupts.txt I'm led to
> > > > believe that the cros_ec driver shouldn't call disable_irq() on the
> > > > interrupt if it wants to wakeup from it:
> > > >
> > > > "Calling enable_irq_wake() causes suspend_device_irqs() to treat the
> > > > given IRQ in a special way. Namely, the IRQ remains enabled, by on the
> > > > first interrupt it will be disabled, marked as pending and "suspended"
> > > > so that it will be re-enabled by resume_device_irqs() during the
> > > > subsequent system resume. Also the PM core is notified about the event
> > > > which causes the system suspend in progress to be aborted (that doesn't
> > > > have to happen immediately, but at one of the points where the suspend
> > > > thread looks for pending wakeup events)."
> > >
> > > I think this describes the behaviour when you keep both enabled.
> > >
> > > > I suppose the problem is an irq line disabled in hardware that has
> > > > wakeup armed on it? Is this even valid? Shouldn't an irq be enabled for
> > > > wakeup to work?
> > >
> > > I couldn't really find a definite answer, but there are a bunch of
> > > examples of other drivers in the kernel:
> > > - drivers/extcon/extcon-usb-gpio.c:usb_extcon_suspend
> > > - drivers/hid/i2c-hid/i2c-hid.c:i2c_hid_suspend
> > > - drivers/mfd/max77843.c:max77843_suspend
> > > (not exhaustive, this is quite hard to grep for...)
> > >
> > > > We could immediately unmask those lines in the hardware when the
> > > > set_wake() callback is called. That way the genirq layer can use the
> > > > driver to do what it wants with the hardware and the driver can make
> > > > sure that set_wake() will always cause the wakeup interrupt to be
> > > > delivered to genirq even when software has disabled it.
> > > >
> > > > But I think that there might be a problem with how genirq understands
> > > > the masked state of a line when the wakeup implementation conflates
> > > > masked state with wakeup armed state. Consider this call-flow:
> > > >
> > > > irq masked in hardware, IRQD_IRQ_MASKED is set
> > > > enable_irq_wake()
> > > > unmask_irq in hardware
> > > > IRQD_WAKEUP_ARMED is set
> > > > <suspend and wakeup from irq>
> > > > handle_level_irq()
> > > > mask_ack_irq()
> > > > mask_irq()
> > > > if (irqd_irq_masked()) -> returns true and skips masking!
> > > > if (desc->irq_data.chip->irq_ack)
> > > > ...
> > > > irq_may_run()
> > > > irq_pm_check_wakeup()
> > > > irq_disable()
> > > > mask_irq() -> does nothing again
> > > >
> > > > In the above flow, we never mask the irq because we thought it was
> > > > already masked when it was disabled, but the irqchip implementation
> > > > unmasked it to make wakeup work. Maybe we should always mask the irq if
> > > > wakeup is armed and we're trying to call mask_irq()? Looks hacky.
> Maybe we can implement irqchip's mask_ack_irq in mediatek driver to
> always mask the irq. Then flow will always call it without judgment
> IRQD_IRQ_MASKED.
>
> diff --git a/drivers/pinctrl/mediatek/mtk-eint.c
> b/drivers/pinctrl/mediatek/mtk-
> index f464f8c..9f1aae2 100644
> --- a/drivers/pinctrl/mediatek/mtk-eint.c
> +++ b/drivers/pinctrl/mediatek/mtk-eint.c
> @@ -272,12 +272,19 @@ static void mtk_eint_irq_release_resources(struct
> irq_data
> gpiochip_unlock_as_irq(gpio_c, gpio_n);
> }
>
> +static void mtk_eint_mask_ack(struct irq_data *d)
> +{
> + mtk_eint_mask(d);
> + mtk_eint_ack(d);
> +}
> +
> static struct irq_chip mtk_eint_irq_chip = {
> .name = "mt-eint",
> .irq_disable = mtk_eint_mask,
> .irq_mask = mtk_eint_mask,
> .irq_unmask = mtk_eint_unmask,
> .irq_ack = mtk_eint_ack,
> + .irq_mask_ack = mtk_eint_mask_ack,
> .irq_set_type = mtk_eint_set_type,
> .irq_set_wake = mtk_eint_irq_set_wake,
> .irq_request_resources = mtk_eint_irq_request_resources,
>
> This seems like a small change.
> thanks.
Does this work? My understanding is that Linux thinks the irq is
_already_ masked, so it short-circuits in the generic IRQ code and
doesn't call mask again.
-Evan
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH 2/2] pinctrl: mediatek: Update cur_mask in mask/mask ops
2019-05-31 17:17 ` Evan Green
(?)
@ 2019-06-03 8:01 ` Chuanjia Liu
-1 siblings, 0 replies; 50+ messages in thread
From: Chuanjia Liu @ 2019-06-03 8:01 UTC (permalink / raw)
To: Evan Green
Cc: Nicolas Boichat, Stephen Boyd,
moderated list:ARM/Mediatek SoC support, Sean Wang,
Linus Walleij, Matthias Brugger, linux-gpio,
linux-arm Mailing List, lkml, Thomas Gleixner
On Fri, 2019-05-31 at 10:17 -0700, Evan Green wrote:
> On Fri, May 31, 2019 at 1:06 AM Chuanjia Liu <Chuanjia.Liu@mediatek.com> wrote:
> >
> > On Thu, 2019-05-30 at 10:12 -0700, Evan Green wrote:
> > > On Wed, May 15, 2019 at 1:05 AM Nicolas Boichat <drinkcat@chromium.org> wrote:
> > > >
> > > > On Wed, May 15, 2019 at 4:14 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > > > >
> > > > > Quoting Nicolas Boichat (2019-05-13 18:37:58)
> > > > > > On Tue, May 14, 2019 at 6:29 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > > > > > >
> > > > > > > Quoting Nicolas Boichat (2019-04-28 20:55:15)
> > > > > > > > During suspend/resume, mtk_eint_mask may be called while
> > > > > > > > wake_mask is active. For example, this happens if a wake-source
> > > > > > > > with an active interrupt handler wakes the system:
> > > > > > > > irq/pm.c:irq_pm_check_wakeup would disable the interrupt, so
> > > > > > > > that it can be handled later on in the resume flow.
> > > > > > > >
> > > > > > > > However, this may happen before mtk_eint_do_resume is called:
> > > > > > > > in this case, wake_mask is loaded, and cur_mask is restored
> > > > > > > > from an older copy, re-enabling the interrupt, and causing
> > > > > > > > an interrupt storm (especially for level interrupts).
> > > > > > > >
> > > > > > > > Instead, we just record mask/unmask changes in cur_mask. This
> > > > > > > > also avoids the need to read the current mask in eint_do_suspend,
> > > > > > > > and we can remove mtk_eint_chip_read_mask function.
> > > > > > > >
> > > > > > > > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> > > > > > >
> > > > > > > It looks an awful lot like you should just use IRQCHIP_MASK_ON_SUSPEND
> > > > > > > here. Isn't that what's happening? All non-wake irqs should be masked at
> > > > > > > the hardware level so they can't cause a wakeup during suspend and on
> > > > > > > resume they can be unmasked?
> > > > > >
> > > > > > No, this is for an line that has both wake and interrupt enabled. To
> > > > > > reword the commit message above:
> > > > >
> > > > > Is my understanding correct that there isn't a different "wake up"
> > > > > register that can be written to cause a GPIO to be configured to wake
> > > > > the system from suspend? The only way to do so is to leave the GPIO
> > > > > unmasked in the hardware by having EINT_EN[irq] = 1? And thus any
> > > > > interrupts that we don't want to wake us up during suspend should be
> > > > > masked in the hardware?
> > > >
> > > > Yes, that's my understanding as well.
> > > >
> > > > And then, what this driver does is to emulate the behaviour of a
> > > > controller that would actually have separate irq and wake enable
> > > > registers.
> > > >
> > > > > If that's true, the code here that's trying to keep track of enabled
> > > > > irqs and wakeup enabled irqs can be replaced with the irqchip flag so
> > > > > that wakeup irqs are not masked while non-wakeups are masked.
> > > >
> > > > Correct, but with the caveat that I don't see anything that definitely
> > > > requires an interrupt to be enabled to be a wake source. See below...
> > > >
> > > > >
> > > > > > 1. cur_mask[irq] = 1; wake_mask[irq] = 1; EINT_EN[irq] = 1 (interrupt
> > > > > > enabled at hardware level)
> > > > > > 2. System suspends, resumes due to that line (at this stage EINT_HW
> > > > > > == wake_mask)
> > > > > > 3. irq_pm_check_wakeup is called, and disables the interrupt =>
> > > > > > EINT_EN[irq] = 0, but we still have cur_mask[irq] = 1
> > > > > > 4. mtk_eint_do_resume is called, and restores EINT_EN = cur_mask, so
> > > > > > it reenables EINT_EN[irq] = 1 => interrupt storm.
> > > > > >
> > > > > > This patch fixes the issue in step 3. So that the interrupt can be
> > > > > > re-enabled properly later on, sometimes after mtk_eint_do_resume, when
> > > > > > the driver is ready to handle it.
> > > > >
> > > > > Right, we'd rather not see irqchip drivers working around the genirq
> > > > > layer to do these things like tracking cur_mask and wake_mask. That
> > > > > leads to subtle bugs and makes the driver maintain state across the
> > > > > irqchip callbacks and system suspend/resume.
> > > > >
> > > > > >
> > > > > > Also, IRQCHIP_MASK_ON_SUSPEND does not handle lines that are enabled
> > > > > > as a wake source, but without interrupt enabled (e.g. cros_ec driver
> > > > > > does that), which we do want to support.
> > > > >
> > > > > Hmm. I thought that even if the irq is disabled by a driver, that would
> > > > > be a lazy disable so it isn't really masked in the hardware. Then if an
> > > > > interrupt comes in during suspend on a wake configured irq line, the
> > > > > hardware will have left it unmasked because IRQCHIP_MASK_ON_SUSPEND in
> > > > > combination with lazy disable would mean that the line is left unmasked
> > > > > (ignoring whatever this mediatek driver is doing to mask and unmask in
> > > > > PM hooks).
> > > >
> > > > At the very least, that's not what happens with this system. The
> > > > interrupt is definitely not kept enabled in suspend, and the system
> > > > would not wake from an EC interrupt. (see also this series, BTW:
> > > > https://patchwork.kernel.org/cover/10921121/).
> > > >
> > > > > Just reading Documentation/power/suspend-and-interrupts.txt I'm led to
> > > > > believe that the cros_ec driver shouldn't call disable_irq() on the
> > > > > interrupt if it wants to wakeup from it:
> > > > >
> > > > > "Calling enable_irq_wake() causes suspend_device_irqs() to treat the
> > > > > given IRQ in a special way. Namely, the IRQ remains enabled, by on the
> > > > > first interrupt it will be disabled, marked as pending and "suspended"
> > > > > so that it will be re-enabled by resume_device_irqs() during the
> > > > > subsequent system resume. Also the PM core is notified about the event
> > > > > which causes the system suspend in progress to be aborted (that doesn't
> > > > > have to happen immediately, but at one of the points where the suspend
> > > > > thread looks for pending wakeup events)."
> > > >
> > > > I think this describes the behaviour when you keep both enabled.
> > > >
> > > > > I suppose the problem is an irq line disabled in hardware that has
> > > > > wakeup armed on it? Is this even valid? Shouldn't an irq be enabled for
> > > > > wakeup to work?
> > > >
> > > > I couldn't really find a definite answer, but there are a bunch of
> > > > examples of other drivers in the kernel:
> > > > - drivers/extcon/extcon-usb-gpio.c:usb_extcon_suspend
> > > > - drivers/hid/i2c-hid/i2c-hid.c:i2c_hid_suspend
> > > > - drivers/mfd/max77843.c:max77843_suspend
> > > > (not exhaustive, this is quite hard to grep for...)
> > > >
> > > > > We could immediately unmask those lines in the hardware when the
> > > > > set_wake() callback is called. That way the genirq layer can use the
> > > > > driver to do what it wants with the hardware and the driver can make
> > > > > sure that set_wake() will always cause the wakeup interrupt to be
> > > > > delivered to genirq even when software has disabled it.
> > > > >
> > > > > But I think that there might be a problem with how genirq understands
> > > > > the masked state of a line when the wakeup implementation conflates
> > > > > masked state with wakeup armed state. Consider this call-flow:
> > > > >
> > > > > irq masked in hardware, IRQD_IRQ_MASKED is set
> > > > > enable_irq_wake()
> > > > > unmask_irq in hardware
> > > > > IRQD_WAKEUP_ARMED is set
> > > > > <suspend and wakeup from irq>
> > > > > handle_level_irq()
> > > > > mask_ack_irq()
> > > > > mask_irq()
> > > > > if (irqd_irq_masked()) -> returns true and skips masking!
> > > > > if (desc->irq_data.chip->irq_ack)
> > > > > ...
> > > > > irq_may_run()
> > > > > irq_pm_check_wakeup()
> > > > > irq_disable()
> > > > > mask_irq() -> does nothing again
> > > > >
> > > > > In the above flow, we never mask the irq because we thought it was
> > > > > already masked when it was disabled, but the irqchip implementation
> > > > > unmasked it to make wakeup work. Maybe we should always mask the irq if
> > > > > wakeup is armed and we're trying to call mask_irq()? Looks hacky.
> > Maybe we can implement irqchip's mask_ack_irq in mediatek driver to
> > always mask the irq. Then flow will always call it without judgment
> > IRQD_IRQ_MASKED.
> >
> > diff --git a/drivers/pinctrl/mediatek/mtk-eint.c
> > b/drivers/pinctrl/mediatek/mtk-
> > index f464f8c..9f1aae2 100644
> > --- a/drivers/pinctrl/mediatek/mtk-eint.c
> > +++ b/drivers/pinctrl/mediatek/mtk-eint.c
> > @@ -272,12 +272,19 @@ static void mtk_eint_irq_release_resources(struct
> > irq_data
> > gpiochip_unlock_as_irq(gpio_c, gpio_n);
> > }
> >
> > +static void mtk_eint_mask_ack(struct irq_data *d)
> > +{
> > + mtk_eint_mask(d);
> > + mtk_eint_ack(d);
> > +}
> > +
> > static struct irq_chip mtk_eint_irq_chip = {
> > .name = "mt-eint",
> > .irq_disable = mtk_eint_mask,
> > .irq_mask = mtk_eint_mask,
> > .irq_unmask = mtk_eint_unmask,
> > .irq_ack = mtk_eint_ack,
> > + .irq_mask_ack = mtk_eint_mask_ack,
> > .irq_set_type = mtk_eint_set_type,
> > .irq_set_wake = mtk_eint_irq_set_wake,
> > .irq_request_resources = mtk_eint_irq_request_resources,
> >
> > This seems like a small change.
> > thanks.
>
> Does this work? My understanding is that Linux thinks the irq is
> _already_ masked, so it short-circuits in the generic IRQ code and
> doesn't call mask again.
> -Evan
Yes, you are right.
The underlying problem is really that the hardware IRQ enabled state is
out of sync with what linux thinks.In resume flow,Linux thinks the irq
is _already_masked, so it short-circuits in the generic IRQ code and
doesn't call mask again.So in step 3 will have a interrupt storm.
But we implement irqchip's mask_ack_irq so that mask_ack_irq() calls
desc->irq_data.chip->irq_mask_ack instead of mask_irq() that needs to
judge IRQD_IRQ_MASKED. This will correctly set cur_mask[irq] = 0 to
sync with kernel state.
Also, this patch can solve the issue of [1/2] in this patchset[1] which
also is the interrupt mask cannot be set correctly due to hardware irq
state not sync kernel.
[1] https://patchwork.kernel.org/patch/10921143/
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH 2/2] pinctrl: mediatek: Update cur_mask in mask/mask ops
@ 2019-06-03 8:01 ` Chuanjia Liu
0 siblings, 0 replies; 50+ messages in thread
From: Chuanjia Liu @ 2019-06-03 8:01 UTC (permalink / raw)
To: Evan Green
Cc: Nicolas Boichat, Linus Walleij, Sean Wang, lkml, Stephen Boyd,
linux-gpio, moderated list:ARM/Mediatek SoC support,
Matthias Brugger, Thomas Gleixner, linux-arm Mailing List
On Fri, 2019-05-31 at 10:17 -0700, Evan Green wrote:
> On Fri, May 31, 2019 at 1:06 AM Chuanjia Liu <Chuanjia.Liu@mediatek.com> wrote:
> >
> > On Thu, 2019-05-30 at 10:12 -0700, Evan Green wrote:
> > > On Wed, May 15, 2019 at 1:05 AM Nicolas Boichat <drinkcat@chromium.org> wrote:
> > > >
> > > > On Wed, May 15, 2019 at 4:14 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > > > >
> > > > > Quoting Nicolas Boichat (2019-05-13 18:37:58)
> > > > > > On Tue, May 14, 2019 at 6:29 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > > > > > >
> > > > > > > Quoting Nicolas Boichat (2019-04-28 20:55:15)
> > > > > > > > During suspend/resume, mtk_eint_mask may be called while
> > > > > > > > wake_mask is active. For example, this happens if a wake-source
> > > > > > > > with an active interrupt handler wakes the system:
> > > > > > > > irq/pm.c:irq_pm_check_wakeup would disable the interrupt, so
> > > > > > > > that it can be handled later on in the resume flow.
> > > > > > > >
> > > > > > > > However, this may happen before mtk_eint_do_resume is called:
> > > > > > > > in this case, wake_mask is loaded, and cur_mask is restored
> > > > > > > > from an older copy, re-enabling the interrupt, and causing
> > > > > > > > an interrupt storm (especially for level interrupts).
> > > > > > > >
> > > > > > > > Instead, we just record mask/unmask changes in cur_mask. This
> > > > > > > > also avoids the need to read the current mask in eint_do_suspend,
> > > > > > > > and we can remove mtk_eint_chip_read_mask function.
> > > > > > > >
> > > > > > > > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> > > > > > >
> > > > > > > It looks an awful lot like you should just use IRQCHIP_MASK_ON_SUSPEND
> > > > > > > here. Isn't that what's happening? All non-wake irqs should be masked at
> > > > > > > the hardware level so they can't cause a wakeup during suspend and on
> > > > > > > resume they can be unmasked?
> > > > > >
> > > > > > No, this is for an line that has both wake and interrupt enabled. To
> > > > > > reword the commit message above:
> > > > >
> > > > > Is my understanding correct that there isn't a different "wake up"
> > > > > register that can be written to cause a GPIO to be configured to wake
> > > > > the system from suspend? The only way to do so is to leave the GPIO
> > > > > unmasked in the hardware by having EINT_EN[irq] = 1? And thus any
> > > > > interrupts that we don't want to wake us up during suspend should be
> > > > > masked in the hardware?
> > > >
> > > > Yes, that's my understanding as well.
> > > >
> > > > And then, what this driver does is to emulate the behaviour of a
> > > > controller that would actually have separate irq and wake enable
> > > > registers.
> > > >
> > > > > If that's true, the code here that's trying to keep track of enabled
> > > > > irqs and wakeup enabled irqs can be replaced with the irqchip flag so
> > > > > that wakeup irqs are not masked while non-wakeups are masked.
> > > >
> > > > Correct, but with the caveat that I don't see anything that definitely
> > > > requires an interrupt to be enabled to be a wake source. See below...
> > > >
> > > > >
> > > > > > 1. cur_mask[irq] = 1; wake_mask[irq] = 1; EINT_EN[irq] = 1 (interrupt
> > > > > > enabled at hardware level)
> > > > > > 2. System suspends, resumes due to that line (at this stage EINT_HW
> > > > > > == wake_mask)
> > > > > > 3. irq_pm_check_wakeup is called, and disables the interrupt =>
> > > > > > EINT_EN[irq] = 0, but we still have cur_mask[irq] = 1
> > > > > > 4. mtk_eint_do_resume is called, and restores EINT_EN = cur_mask, so
> > > > > > it reenables EINT_EN[irq] = 1 => interrupt storm.
> > > > > >
> > > > > > This patch fixes the issue in step 3. So that the interrupt can be
> > > > > > re-enabled properly later on, sometimes after mtk_eint_do_resume, when
> > > > > > the driver is ready to handle it.
> > > > >
> > > > > Right, we'd rather not see irqchip drivers working around the genirq
> > > > > layer to do these things like tracking cur_mask and wake_mask. That
> > > > > leads to subtle bugs and makes the driver maintain state across the
> > > > > irqchip callbacks and system suspend/resume.
> > > > >
> > > > > >
> > > > > > Also, IRQCHIP_MASK_ON_SUSPEND does not handle lines that are enabled
> > > > > > as a wake source, but without interrupt enabled (e.g. cros_ec driver
> > > > > > does that), which we do want to support.
> > > > >
> > > > > Hmm. I thought that even if the irq is disabled by a driver, that would
> > > > > be a lazy disable so it isn't really masked in the hardware. Then if an
> > > > > interrupt comes in during suspend on a wake configured irq line, the
> > > > > hardware will have left it unmasked because IRQCHIP_MASK_ON_SUSPEND in
> > > > > combination with lazy disable would mean that the line is left unmasked
> > > > > (ignoring whatever this mediatek driver is doing to mask and unmask in
> > > > > PM hooks).
> > > >
> > > > At the very least, that's not what happens with this system. The
> > > > interrupt is definitely not kept enabled in suspend, and the system
> > > > would not wake from an EC interrupt. (see also this series, BTW:
> > > > https://patchwork.kernel.org/cover/10921121/).
> > > >
> > > > > Just reading Documentation/power/suspend-and-interrupts.txt I'm led to
> > > > > believe that the cros_ec driver shouldn't call disable_irq() on the
> > > > > interrupt if it wants to wakeup from it:
> > > > >
> > > > > "Calling enable_irq_wake() causes suspend_device_irqs() to treat the
> > > > > given IRQ in a special way. Namely, the IRQ remains enabled, by on the
> > > > > first interrupt it will be disabled, marked as pending and "suspended"
> > > > > so that it will be re-enabled by resume_device_irqs() during the
> > > > > subsequent system resume. Also the PM core is notified about the event
> > > > > which causes the system suspend in progress to be aborted (that doesn't
> > > > > have to happen immediately, but at one of the points where the suspend
> > > > > thread looks for pending wakeup events)."
> > > >
> > > > I think this describes the behaviour when you keep both enabled.
> > > >
> > > > > I suppose the problem is an irq line disabled in hardware that has
> > > > > wakeup armed on it? Is this even valid? Shouldn't an irq be enabled for
> > > > > wakeup to work?
> > > >
> > > > I couldn't really find a definite answer, but there are a bunch of
> > > > examples of other drivers in the kernel:
> > > > - drivers/extcon/extcon-usb-gpio.c:usb_extcon_suspend
> > > > - drivers/hid/i2c-hid/i2c-hid.c:i2c_hid_suspend
> > > > - drivers/mfd/max77843.c:max77843_suspend
> > > > (not exhaustive, this is quite hard to grep for...)
> > > >
> > > > > We could immediately unmask those lines in the hardware when the
> > > > > set_wake() callback is called. That way the genirq layer can use the
> > > > > driver to do what it wants with the hardware and the driver can make
> > > > > sure that set_wake() will always cause the wakeup interrupt to be
> > > > > delivered to genirq even when software has disabled it.
> > > > >
> > > > > But I think that there might be a problem with how genirq understands
> > > > > the masked state of a line when the wakeup implementation conflates
> > > > > masked state with wakeup armed state. Consider this call-flow:
> > > > >
> > > > > irq masked in hardware, IRQD_IRQ_MASKED is set
> > > > > enable_irq_wake()
> > > > > unmask_irq in hardware
> > > > > IRQD_WAKEUP_ARMED is set
> > > > > <suspend and wakeup from irq>
> > > > > handle_level_irq()
> > > > > mask_ack_irq()
> > > > > mask_irq()
> > > > > if (irqd_irq_masked()) -> returns true and skips masking!
> > > > > if (desc->irq_data.chip->irq_ack)
> > > > > ...
> > > > > irq_may_run()
> > > > > irq_pm_check_wakeup()
> > > > > irq_disable()
> > > > > mask_irq() -> does nothing again
> > > > >
> > > > > In the above flow, we never mask the irq because we thought it was
> > > > > already masked when it was disabled, but the irqchip implementation
> > > > > unmasked it to make wakeup work. Maybe we should always mask the irq if
> > > > > wakeup is armed and we're trying to call mask_irq()? Looks hacky.
> > Maybe we can implement irqchip's mask_ack_irq in mediatek driver to
> > always mask the irq. Then flow will always call it without judgment
> > IRQD_IRQ_MASKED.
> >
> > diff --git a/drivers/pinctrl/mediatek/mtk-eint.c
> > b/drivers/pinctrl/mediatek/mtk-
> > index f464f8c..9f1aae2 100644
> > --- a/drivers/pinctrl/mediatek/mtk-eint.c
> > +++ b/drivers/pinctrl/mediatek/mtk-eint.c
> > @@ -272,12 +272,19 @@ static void mtk_eint_irq_release_resources(struct
> > irq_data
> > gpiochip_unlock_as_irq(gpio_c, gpio_n);
> > }
> >
> > +static void mtk_eint_mask_ack(struct irq_data *d)
> > +{
> > + mtk_eint_mask(d);
> > + mtk_eint_ack(d);
> > +}
> > +
> > static struct irq_chip mtk_eint_irq_chip = {
> > .name = "mt-eint",
> > .irq_disable = mtk_eint_mask,
> > .irq_mask = mtk_eint_mask,
> > .irq_unmask = mtk_eint_unmask,
> > .irq_ack = mtk_eint_ack,
> > + .irq_mask_ack = mtk_eint_mask_ack,
> > .irq_set_type = mtk_eint_set_type,
> > .irq_set_wake = mtk_eint_irq_set_wake,
> > .irq_request_resources = mtk_eint_irq_request_resources,
> >
> > This seems like a small change.
> > thanks.
>
> Does this work? My understanding is that Linux thinks the irq is
> _already_ masked, so it short-circuits in the generic IRQ code and
> doesn't call mask again.
> -Evan
Yes, you are right.
The underlying problem is really that the hardware IRQ enabled state is
out of sync with what linux thinks.In resume flow,Linux thinks the irq
is _already_masked, so it short-circuits in the generic IRQ code and
doesn't call mask again.So in step 3 will have a interrupt storm.
But we implement irqchip's mask_ack_irq so that mask_ack_irq() calls
desc->irq_data.chip->irq_mask_ack instead of mask_irq() that needs to
judge IRQD_IRQ_MASKED. This will correctly set cur_mask[irq] = 0 to
sync with kernel state.
Also, this patch can solve the issue of [1/2] in this patchset[1] which
also is the interrupt mask cannot be set correctly due to hardware irq
state not sync kernel.
[1] https://patchwork.kernel.org/patch/10921143/
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH 2/2] pinctrl: mediatek: Update cur_mask in mask/mask ops
@ 2019-06-03 8:01 ` Chuanjia Liu
0 siblings, 0 replies; 50+ messages in thread
From: Chuanjia Liu @ 2019-06-03 8:01 UTC (permalink / raw)
To: Evan Green
Cc: Nicolas Boichat, Stephen Boyd,
moderated list:ARM/Mediatek SoC support, Sean Wang,
Linus Walleij, Matthias Brugger, linux-gpio,
linux-arm Mailing List, lkml, Thomas Gleixner
On Fri, 2019-05-31 at 10:17 -0700, Evan Green wrote:
> On Fri, May 31, 2019 at 1:06 AM Chuanjia Liu <Chuanjia.Liu@mediatek.com> wrote:
> >
> > On Thu, 2019-05-30 at 10:12 -0700, Evan Green wrote:
> > > On Wed, May 15, 2019 at 1:05 AM Nicolas Boichat <drinkcat@chromium.org> wrote:
> > > >
> > > > On Wed, May 15, 2019 at 4:14 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > > > >
> > > > > Quoting Nicolas Boichat (2019-05-13 18:37:58)
> > > > > > On Tue, May 14, 2019 at 6:29 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > > > > > >
> > > > > > > Quoting Nicolas Boichat (2019-04-28 20:55:15)
> > > > > > > > During suspend/resume, mtk_eint_mask may be called while
> > > > > > > > wake_mask is active. For example, this happens if a wake-source
> > > > > > > > with an active interrupt handler wakes the system:
> > > > > > > > irq/pm.c:irq_pm_check_wakeup would disable the interrupt, so
> > > > > > > > that it can be handled later on in the resume flow.
> > > > > > > >
> > > > > > > > However, this may happen before mtk_eint_do_resume is called:
> > > > > > > > in this case, wake_mask is loaded, and cur_mask is restored
> > > > > > > > from an older copy, re-enabling the interrupt, and causing
> > > > > > > > an interrupt storm (especially for level interrupts).
> > > > > > > >
> > > > > > > > Instead, we just record mask/unmask changes in cur_mask. This
> > > > > > > > also avoids the need to read the current mask in eint_do_suspend,
> > > > > > > > and we can remove mtk_eint_chip_read_mask function.
> > > > > > > >
> > > > > > > > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> > > > > > >
> > > > > > > It looks an awful lot like you should just use IRQCHIP_MASK_ON_SUSPEND
> > > > > > > here. Isn't that what's happening? All non-wake irqs should be masked at
> > > > > > > the hardware level so they can't cause a wakeup during suspend and on
> > > > > > > resume they can be unmasked?
> > > > > >
> > > > > > No, this is for an line that has both wake and interrupt enabled. To
> > > > > > reword the commit message above:
> > > > >
> > > > > Is my understanding correct that there isn't a different "wake up"
> > > > > register that can be written to cause a GPIO to be configured to wake
> > > > > the system from suspend? The only way to do so is to leave the GPIO
> > > > > unmasked in the hardware by having EINT_EN[irq] = 1? And thus any
> > > > > interrupts that we don't want to wake us up during suspend should be
> > > > > masked in the hardware?
> > > >
> > > > Yes, that's my understanding as well.
> > > >
> > > > And then, what this driver does is to emulate the behaviour of a
> > > > controller that would actually have separate irq and wake enable
> > > > registers.
> > > >
> > > > > If that's true, the code here that's trying to keep track of enabled
> > > > > irqs and wakeup enabled irqs can be replaced with the irqchip flag so
> > > > > that wakeup irqs are not masked while non-wakeups are masked.
> > > >
> > > > Correct, but with the caveat that I don't see anything that definitely
> > > > requires an interrupt to be enabled to be a wake source. See below...
> > > >
> > > > >
> > > > > > 1. cur_mask[irq] = 1; wake_mask[irq] = 1; EINT_EN[irq] = 1 (interrupt
> > > > > > enabled at hardware level)
> > > > > > 2. System suspends, resumes due to that line (at this stage EINT_HW
> > > > > > == wake_mask)
> > > > > > 3. irq_pm_check_wakeup is called, and disables the interrupt =>
> > > > > > EINT_EN[irq] = 0, but we still have cur_mask[irq] = 1
> > > > > > 4. mtk_eint_do_resume is called, and restores EINT_EN = cur_mask, so
> > > > > > it reenables EINT_EN[irq] = 1 => interrupt storm.
> > > > > >
> > > > > > This patch fixes the issue in step 3. So that the interrupt can be
> > > > > > re-enabled properly later on, sometimes after mtk_eint_do_resume, when
> > > > > > the driver is ready to handle it.
> > > > >
> > > > > Right, we'd rather not see irqchip drivers working around the genirq
> > > > > layer to do these things like tracking cur_mask and wake_mask. That
> > > > > leads to subtle bugs and makes the driver maintain state across the
> > > > > irqchip callbacks and system suspend/resume.
> > > > >
> > > > > >
> > > > > > Also, IRQCHIP_MASK_ON_SUSPEND does not handle lines that are enabled
> > > > > > as a wake source, but without interrupt enabled (e.g. cros_ec driver
> > > > > > does that), which we do want to support.
> > > > >
> > > > > Hmm. I thought that even if the irq is disabled by a driver, that would
> > > > > be a lazy disable so it isn't really masked in the hardware. Then if an
> > > > > interrupt comes in during suspend on a wake configured irq line, the
> > > > > hardware will have left it unmasked because IRQCHIP_MASK_ON_SUSPEND in
> > > > > combination with lazy disable would mean that the line is left unmasked
> > > > > (ignoring whatever this mediatek driver is doing to mask and unmask in
> > > > > PM hooks).
> > > >
> > > > At the very least, that's not what happens with this system. The
> > > > interrupt is definitely not kept enabled in suspend, and the system
> > > > would not wake from an EC interrupt. (see also this series, BTW:
> > > > https://patchwork.kernel.org/cover/10921121/).
> > > >
> > > > > Just reading Documentation/power/suspend-and-interrupts.txt I'm led to
> > > > > believe that the cros_ec driver shouldn't call disable_irq() on the
> > > > > interrupt if it wants to wakeup from it:
> > > > >
> > > > > "Calling enable_irq_wake() causes suspend_device_irqs() to treat the
> > > > > given IRQ in a special way. Namely, the IRQ remains enabled, by on the
> > > > > first interrupt it will be disabled, marked as pending and "suspended"
> > > > > so that it will be re-enabled by resume_device_irqs() during the
> > > > > subsequent system resume. Also the PM core is notified about the event
> > > > > which causes the system suspend in progress to be aborted (that doesn't
> > > > > have to happen immediately, but at one of the points where the suspend
> > > > > thread looks for pending wakeup events)."
> > > >
> > > > I think this describes the behaviour when you keep both enabled.
> > > >
> > > > > I suppose the problem is an irq line disabled in hardware that has
> > > > > wakeup armed on it? Is this even valid? Shouldn't an irq be enabled for
> > > > > wakeup to work?
> > > >
> > > > I couldn't really find a definite answer, but there are a bunch of
> > > > examples of other drivers in the kernel:
> > > > - drivers/extcon/extcon-usb-gpio.c:usb_extcon_suspend
> > > > - drivers/hid/i2c-hid/i2c-hid.c:i2c_hid_suspend
> > > > - drivers/mfd/max77843.c:max77843_suspend
> > > > (not exhaustive, this is quite hard to grep for...)
> > > >
> > > > > We could immediately unmask those lines in the hardware when the
> > > > > set_wake() callback is called. That way the genirq layer can use the
> > > > > driver to do what it wants with the hardware and the driver can make
> > > > > sure that set_wake() will always cause the wakeup interrupt to be
> > > > > delivered to genirq even when software has disabled it.
> > > > >
> > > > > But I think that there might be a problem with how genirq understands
> > > > > the masked state of a line when the wakeup implementation conflates
> > > > > masked state with wakeup armed state. Consider this call-flow:
> > > > >
> > > > > irq masked in hardware, IRQD_IRQ_MASKED is set
> > > > > enable_irq_wake()
> > > > > unmask_irq in hardware
> > > > > IRQD_WAKEUP_ARMED is set
> > > > > <suspend and wakeup from irq>
> > > > > handle_level_irq()
> > > > > mask_ack_irq()
> > > > > mask_irq()
> > > > > if (irqd_irq_masked()) -> returns true and skips masking!
> > > > > if (desc->irq_data.chip->irq_ack)
> > > > > ...
> > > > > irq_may_run()
> > > > > irq_pm_check_wakeup()
> > > > > irq_disable()
> > > > > mask_irq() -> does nothing again
> > > > >
> > > > > In the above flow, we never mask the irq because we thought it was
> > > > > already masked when it was disabled, but the irqchip implementation
> > > > > unmasked it to make wakeup work. Maybe we should always mask the irq if
> > > > > wakeup is armed and we're trying to call mask_irq()? Looks hacky.
> > Maybe we can implement irqchip's mask_ack_irq in mediatek driver to
> > always mask the irq. Then flow will always call it without judgment
> > IRQD_IRQ_MASKED.
> >
> > diff --git a/drivers/pinctrl/mediatek/mtk-eint.c
> > b/drivers/pinctrl/mediatek/mtk-
> > index f464f8c..9f1aae2 100644
> > --- a/drivers/pinctrl/mediatek/mtk-eint.c
> > +++ b/drivers/pinctrl/mediatek/mtk-eint.c
> > @@ -272,12 +272,19 @@ static void mtk_eint_irq_release_resources(struct
> > irq_data
> > gpiochip_unlock_as_irq(gpio_c, gpio_n);
> > }
> >
> > +static void mtk_eint_mask_ack(struct irq_data *d)
> > +{
> > + mtk_eint_mask(d);
> > + mtk_eint_ack(d);
> > +}
> > +
> > static struct irq_chip mtk_eint_irq_chip = {
> > .name = "mt-eint",
> > .irq_disable = mtk_eint_mask,
> > .irq_mask = mtk_eint_mask,
> > .irq_unmask = mtk_eint_unmask,
> > .irq_ack = mtk_eint_ack,
> > + .irq_mask_ack = mtk_eint_mask_ack,
> > .irq_set_type = mtk_eint_set_type,
> > .irq_set_wake = mtk_eint_irq_set_wake,
> > .irq_request_resources = mtk_eint_irq_request_resources,
> >
> > This seems like a small change.
> > thanks.
>
> Does this work? My understanding is that Linux thinks the irq is
> _already_ masked, so it short-circuits in the generic IRQ code and
> doesn't call mask again.
> -Evan
Yes, you are right.
The underlying problem is really that the hardware IRQ enabled state is
out of sync with what linux thinks.In resume flow,Linux thinks the irq
is _already_masked, so it short-circuits in the generic IRQ code and
doesn't call mask again.So in step 3 will have a interrupt storm.
But we implement irqchip's mask_ack_irq so that mask_ack_irq() calls
desc->irq_data.chip->irq_mask_ack instead of mask_irq() that needs to
judge IRQD_IRQ_MASKED. This will correctly set cur_mask[irq] = 0 to
sync with kernel state.
Also, this patch can solve the issue of [1/2] in this patchset[1] which
also is the interrupt mask cannot be set correctly due to hardware irq
state not sync kernel.
[1] https://patchwork.kernel.org/patch/10921143/
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH 2/2] pinctrl: mediatek: Update cur_mask in mask/mask ops
2019-06-03 8:01 ` Chuanjia Liu
(?)
@ 2019-06-04 0:57 ` Nicolas Boichat
-1 siblings, 0 replies; 50+ messages in thread
From: Nicolas Boichat @ 2019-06-04 0:57 UTC (permalink / raw)
To: Chuanjia Liu
Cc: Evan Green, Stephen Boyd,
moderated list:ARM/Mediatek SoC support, Sean Wang,
Linus Walleij, Matthias Brugger, open list:GPIO SUBSYSTEM,
linux-arm Mailing List, lkml, Thomas Gleixner
On Mon, Jun 3, 2019 at 4:01 PM Chuanjia Liu <Chuanjia.Liu@mediatek.com> wrote:
>
> On Fri, 2019-05-31 at 10:17 -0700, Evan Green wrote:
> > On Fri, May 31, 2019 at 1:06 AM Chuanjia Liu <Chuanjia.Liu@mediatek.com> wrote:
> > >
> > > On Thu, 2019-05-30 at 10:12 -0700, Evan Green wrote:
> > > > On Wed, May 15, 2019 at 1:05 AM Nicolas Boichat <drinkcat@chromium.org> wrote:
> > > > >
> > > > > On Wed, May 15, 2019 at 4:14 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > > > > >
> > > > > > Quoting Nicolas Boichat (2019-05-13 18:37:58)
> > > > > > > On Tue, May 14, 2019 at 6:29 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > > > > > > >
> > > > > > > > Quoting Nicolas Boichat (2019-04-28 20:55:15)
> > > > > > > > > During suspend/resume, mtk_eint_mask may be called while
> > > > > > > > > wake_mask is active. For example, this happens if a wake-source
> > > > > > > > > with an active interrupt handler wakes the system:
> > > > > > > > > irq/pm.c:irq_pm_check_wakeup would disable the interrupt, so
> > > > > > > > > that it can be handled later on in the resume flow.
> > > > > > > > >
> > > > > > > > > However, this may happen before mtk_eint_do_resume is called:
> > > > > > > > > in this case, wake_mask is loaded, and cur_mask is restored
> > > > > > > > > from an older copy, re-enabling the interrupt, and causing
> > > > > > > > > an interrupt storm (especially for level interrupts).
> > > > > > > > >
> > > > > > > > > Instead, we just record mask/unmask changes in cur_mask. This
> > > > > > > > > also avoids the need to read the current mask in eint_do_suspend,
> > > > > > > > > and we can remove mtk_eint_chip_read_mask function.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> > > > > > > >
> > > > > > > > It looks an awful lot like you should just use IRQCHIP_MASK_ON_SUSPEND
> > > > > > > > here. Isn't that what's happening? All non-wake irqs should be masked at
> > > > > > > > the hardware level so they can't cause a wakeup during suspend and on
> > > > > > > > resume they can be unmasked?
> > > > > > >
> > > > > > > No, this is for an line that has both wake and interrupt enabled. To
> > > > > > > reword the commit message above:
> > > > > >
> > > > > > Is my understanding correct that there isn't a different "wake up"
> > > > > > register that can be written to cause a GPIO to be configured to wake
> > > > > > the system from suspend? The only way to do so is to leave the GPIO
> > > > > > unmasked in the hardware by having EINT_EN[irq] = 1? And thus any
> > > > > > interrupts that we don't want to wake us up during suspend should be
> > > > > > masked in the hardware?
> > > > >
> > > > > Yes, that's my understanding as well.
> > > > >
> > > > > And then, what this driver does is to emulate the behaviour of a
> > > > > controller that would actually have separate irq and wake enable
> > > > > registers.
> > > > >
> > > > > > If that's true, the code here that's trying to keep track of enabled
> > > > > > irqs and wakeup enabled irqs can be replaced with the irqchip flag so
> > > > > > that wakeup irqs are not masked while non-wakeups are masked.
> > > > >
> > > > > Correct, but with the caveat that I don't see anything that definitely
> > > > > requires an interrupt to be enabled to be a wake source. See below...
> > > > >
> > > > > >
> > > > > > > 1. cur_mask[irq] = 1; wake_mask[irq] = 1; EINT_EN[irq] = 1 (interrupt
> > > > > > > enabled at hardware level)
> > > > > > > 2. System suspends, resumes due to that line (at this stage EINT_HW
> > > > > > > == wake_mask)
> > > > > > > 3. irq_pm_check_wakeup is called, and disables the interrupt =>
> > > > > > > EINT_EN[irq] = 0, but we still have cur_mask[irq] = 1
> > > > > > > 4. mtk_eint_do_resume is called, and restores EINT_EN = cur_mask, so
> > > > > > > it reenables EINT_EN[irq] = 1 => interrupt storm.
> > > > > > >
> > > > > > > This patch fixes the issue in step 3. So that the interrupt can be
> > > > > > > re-enabled properly later on, sometimes after mtk_eint_do_resume, when
> > > > > > > the driver is ready to handle it.
> > > > > >
> > > > > > Right, we'd rather not see irqchip drivers working around the genirq
> > > > > > layer to do these things like tracking cur_mask and wake_mask. That
> > > > > > leads to subtle bugs and makes the driver maintain state across the
> > > > > > irqchip callbacks and system suspend/resume.
> > > > > >
> > > > > > >
> > > > > > > Also, IRQCHIP_MASK_ON_SUSPEND does not handle lines that are enabled
> > > > > > > as a wake source, but without interrupt enabled (e.g. cros_ec driver
> > > > > > > does that), which we do want to support.
> > > > > >
> > > > > > Hmm. I thought that even if the irq is disabled by a driver, that would
> > > > > > be a lazy disable so it isn't really masked in the hardware. Then if an
> > > > > > interrupt comes in during suspend on a wake configured irq line, the
> > > > > > hardware will have left it unmasked because IRQCHIP_MASK_ON_SUSPEND in
> > > > > > combination with lazy disable would mean that the line is left unmasked
> > > > > > (ignoring whatever this mediatek driver is doing to mask and unmask in
> > > > > > PM hooks).
> > > > >
> > > > > At the very least, that's not what happens with this system. The
> > > > > interrupt is definitely not kept enabled in suspend, and the system
> > > > > would not wake from an EC interrupt. (see also this series, BTW:
> > > > > https://patchwork.kernel.org/cover/10921121/).
> > > > >
> > > > > > Just reading Documentation/power/suspend-and-interrupts.txt I'm led to
> > > > > > believe that the cros_ec driver shouldn't call disable_irq() on the
> > > > > > interrupt if it wants to wakeup from it:
> > > > > >
> > > > > > "Calling enable_irq_wake() causes suspend_device_irqs() to treat the
> > > > > > given IRQ in a special way. Namely, the IRQ remains enabled, by on the
> > > > > > first interrupt it will be disabled, marked as pending and "suspended"
> > > > > > so that it will be re-enabled by resume_device_irqs() during the
> > > > > > subsequent system resume. Also the PM core is notified about the event
> > > > > > which causes the system suspend in progress to be aborted (that doesn't
> > > > > > have to happen immediately, but at one of the points where the suspend
> > > > > > thread looks for pending wakeup events)."
> > > > >
> > > > > I think this describes the behaviour when you keep both enabled.
> > > > >
> > > > > > I suppose the problem is an irq line disabled in hardware that has
> > > > > > wakeup armed on it? Is this even valid? Shouldn't an irq be enabled for
> > > > > > wakeup to work?
> > > > >
> > > > > I couldn't really find a definite answer, but there are a bunch of
> > > > > examples of other drivers in the kernel:
> > > > > - drivers/extcon/extcon-usb-gpio.c:usb_extcon_suspend
> > > > > - drivers/hid/i2c-hid/i2c-hid.c:i2c_hid_suspend
> > > > > - drivers/mfd/max77843.c:max77843_suspend
> > > > > (not exhaustive, this is quite hard to grep for...)
> > > > >
> > > > > > We could immediately unmask those lines in the hardware when the
> > > > > > set_wake() callback is called. That way the genirq layer can use the
> > > > > > driver to do what it wants with the hardware and the driver can make
> > > > > > sure that set_wake() will always cause the wakeup interrupt to be
> > > > > > delivered to genirq even when software has disabled it.
> > > > > >
> > > > > > But I think that there might be a problem with how genirq understands
> > > > > > the masked state of a line when the wakeup implementation conflates
> > > > > > masked state with wakeup armed state. Consider this call-flow:
> > > > > >
> > > > > > irq masked in hardware, IRQD_IRQ_MASKED is set
> > > > > > enable_irq_wake()
> > > > > > unmask_irq in hardware
> > > > > > IRQD_WAKEUP_ARMED is set
> > > > > > <suspend and wakeup from irq>
> > > > > > handle_level_irq()
> > > > > > mask_ack_irq()
> > > > > > mask_irq()
> > > > > > if (irqd_irq_masked()) -> returns true and skips masking!
> > > > > > if (desc->irq_data.chip->irq_ack)
> > > > > > ...
> > > > > > irq_may_run()
> > > > > > irq_pm_check_wakeup()
> > > > > > irq_disable()
> > > > > > mask_irq() -> does nothing again
> > > > > >
> > > > > > In the above flow, we never mask the irq because we thought it was
> > > > > > already masked when it was disabled, but the irqchip implementation
> > > > > > unmasked it to make wakeup work. Maybe we should always mask the irq if
> > > > > > wakeup is armed and we're trying to call mask_irq()? Looks hacky.
> > > Maybe we can implement irqchip's mask_ack_irq in mediatek driver to
> > > always mask the irq. Then flow will always call it without judgment
> > > IRQD_IRQ_MASKED.
> > >
> > > diff --git a/drivers/pinctrl/mediatek/mtk-eint.c
> > > b/drivers/pinctrl/mediatek/mtk-
> > > index f464f8c..9f1aae2 100644
> > > --- a/drivers/pinctrl/mediatek/mtk-eint.c
> > > +++ b/drivers/pinctrl/mediatek/mtk-eint.c
> > > @@ -272,12 +272,19 @@ static void mtk_eint_irq_release_resources(struct
> > > irq_data
> > > gpiochip_unlock_as_irq(gpio_c, gpio_n);
> > > }
> > >
> > > +static void mtk_eint_mask_ack(struct irq_data *d)
> > > +{
> > > + mtk_eint_mask(d);
> > > + mtk_eint_ack(d);
> > > +}
> > > +
> > > static struct irq_chip mtk_eint_irq_chip = {
> > > .name = "mt-eint",
> > > .irq_disable = mtk_eint_mask,
> > > .irq_mask = mtk_eint_mask,
> > > .irq_unmask = mtk_eint_unmask,
> > > .irq_ack = mtk_eint_ack,
> > > + .irq_mask_ack = mtk_eint_mask_ack,
> > > .irq_set_type = mtk_eint_set_type,
> > > .irq_set_wake = mtk_eint_irq_set_wake,
> > > .irq_request_resources = mtk_eint_irq_request_resources,
> > >
> > > This seems like a small change.
> > > thanks.
> >
> > Does this work? My understanding is that Linux thinks the irq is
> > _already_ masked, so it short-circuits in the generic IRQ code and
> > doesn't call mask again.
> > -Evan
>
> Yes, you are right.
>
> The underlying problem is really that the hardware IRQ enabled state is
> out of sync with what linux thinks.In resume flow,Linux thinks the irq
> is _already_masked, so it short-circuits in the generic IRQ code and
> doesn't call mask again.So in step 3 will have a interrupt storm.
>
> But we implement irqchip's mask_ack_irq so that mask_ack_irq() calls
> desc->irq_data.chip->irq_mask_ack instead of mask_irq() that needs to
> judge IRQD_IRQ_MASKED. This will correctly set cur_mask[irq] = 0 to
> sync with kernel state.
Oh, I see. I guess that would work as mask_ack_irq does not check for
current masked status before calling irq_chip->irq_mask_ack... But
that's only if irq_chip->irq_mask_ack is defined, else it just calls
mask_irq which does that check.
But if we follow bf22ff45bed ("genirq: Avoid unnecessary low level irq
function calls"), this is actually a bug and we should add that same
check to mask_ack_irq (and since pinctrl-rockchip.c does not implement
irq_mask_ack, it's understandable why the author missed that).
So I'm not sure if this is the right change either...
> Also, this patch can solve the issue of [1/2] in this patchset[1] which
> also is the interrupt mask cannot be set correctly due to hardware irq
> state not sync kernel.
>
> [1] https://patchwork.kernel.org/patch/10921143/
>
>
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH 2/2] pinctrl: mediatek: Update cur_mask in mask/mask ops
@ 2019-06-04 0:57 ` Nicolas Boichat
0 siblings, 0 replies; 50+ messages in thread
From: Nicolas Boichat @ 2019-06-04 0:57 UTC (permalink / raw)
To: Chuanjia Liu
Cc: Linus Walleij, Sean Wang, lkml, Evan Green, Stephen Boyd,
open list:GPIO SUBSYSTEM,
moderated list:ARM/Mediatek SoC support, Matthias Brugger,
Thomas Gleixner, linux-arm Mailing List
On Mon, Jun 3, 2019 at 4:01 PM Chuanjia Liu <Chuanjia.Liu@mediatek.com> wrote:
>
> On Fri, 2019-05-31 at 10:17 -0700, Evan Green wrote:
> > On Fri, May 31, 2019 at 1:06 AM Chuanjia Liu <Chuanjia.Liu@mediatek.com> wrote:
> > >
> > > On Thu, 2019-05-30 at 10:12 -0700, Evan Green wrote:
> > > > On Wed, May 15, 2019 at 1:05 AM Nicolas Boichat <drinkcat@chromium.org> wrote:
> > > > >
> > > > > On Wed, May 15, 2019 at 4:14 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > > > > >
> > > > > > Quoting Nicolas Boichat (2019-05-13 18:37:58)
> > > > > > > On Tue, May 14, 2019 at 6:29 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > > > > > > >
> > > > > > > > Quoting Nicolas Boichat (2019-04-28 20:55:15)
> > > > > > > > > During suspend/resume, mtk_eint_mask may be called while
> > > > > > > > > wake_mask is active. For example, this happens if a wake-source
> > > > > > > > > with an active interrupt handler wakes the system:
> > > > > > > > > irq/pm.c:irq_pm_check_wakeup would disable the interrupt, so
> > > > > > > > > that it can be handled later on in the resume flow.
> > > > > > > > >
> > > > > > > > > However, this may happen before mtk_eint_do_resume is called:
> > > > > > > > > in this case, wake_mask is loaded, and cur_mask is restored
> > > > > > > > > from an older copy, re-enabling the interrupt, and causing
> > > > > > > > > an interrupt storm (especially for level interrupts).
> > > > > > > > >
> > > > > > > > > Instead, we just record mask/unmask changes in cur_mask. This
> > > > > > > > > also avoids the need to read the current mask in eint_do_suspend,
> > > > > > > > > and we can remove mtk_eint_chip_read_mask function.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> > > > > > > >
> > > > > > > > It looks an awful lot like you should just use IRQCHIP_MASK_ON_SUSPEND
> > > > > > > > here. Isn't that what's happening? All non-wake irqs should be masked at
> > > > > > > > the hardware level so they can't cause a wakeup during suspend and on
> > > > > > > > resume they can be unmasked?
> > > > > > >
> > > > > > > No, this is for an line that has both wake and interrupt enabled. To
> > > > > > > reword the commit message above:
> > > > > >
> > > > > > Is my understanding correct that there isn't a different "wake up"
> > > > > > register that can be written to cause a GPIO to be configured to wake
> > > > > > the system from suspend? The only way to do so is to leave the GPIO
> > > > > > unmasked in the hardware by having EINT_EN[irq] = 1? And thus any
> > > > > > interrupts that we don't want to wake us up during suspend should be
> > > > > > masked in the hardware?
> > > > >
> > > > > Yes, that's my understanding as well.
> > > > >
> > > > > And then, what this driver does is to emulate the behaviour of a
> > > > > controller that would actually have separate irq and wake enable
> > > > > registers.
> > > > >
> > > > > > If that's true, the code here that's trying to keep track of enabled
> > > > > > irqs and wakeup enabled irqs can be replaced with the irqchip flag so
> > > > > > that wakeup irqs are not masked while non-wakeups are masked.
> > > > >
> > > > > Correct, but with the caveat that I don't see anything that definitely
> > > > > requires an interrupt to be enabled to be a wake source. See below...
> > > > >
> > > > > >
> > > > > > > 1. cur_mask[irq] = 1; wake_mask[irq] = 1; EINT_EN[irq] = 1 (interrupt
> > > > > > > enabled at hardware level)
> > > > > > > 2. System suspends, resumes due to that line (at this stage EINT_HW
> > > > > > > == wake_mask)
> > > > > > > 3. irq_pm_check_wakeup is called, and disables the interrupt =>
> > > > > > > EINT_EN[irq] = 0, but we still have cur_mask[irq] = 1
> > > > > > > 4. mtk_eint_do_resume is called, and restores EINT_EN = cur_mask, so
> > > > > > > it reenables EINT_EN[irq] = 1 => interrupt storm.
> > > > > > >
> > > > > > > This patch fixes the issue in step 3. So that the interrupt can be
> > > > > > > re-enabled properly later on, sometimes after mtk_eint_do_resume, when
> > > > > > > the driver is ready to handle it.
> > > > > >
> > > > > > Right, we'd rather not see irqchip drivers working around the genirq
> > > > > > layer to do these things like tracking cur_mask and wake_mask. That
> > > > > > leads to subtle bugs and makes the driver maintain state across the
> > > > > > irqchip callbacks and system suspend/resume.
> > > > > >
> > > > > > >
> > > > > > > Also, IRQCHIP_MASK_ON_SUSPEND does not handle lines that are enabled
> > > > > > > as a wake source, but without interrupt enabled (e.g. cros_ec driver
> > > > > > > does that), which we do want to support.
> > > > > >
> > > > > > Hmm. I thought that even if the irq is disabled by a driver, that would
> > > > > > be a lazy disable so it isn't really masked in the hardware. Then if an
> > > > > > interrupt comes in during suspend on a wake configured irq line, the
> > > > > > hardware will have left it unmasked because IRQCHIP_MASK_ON_SUSPEND in
> > > > > > combination with lazy disable would mean that the line is left unmasked
> > > > > > (ignoring whatever this mediatek driver is doing to mask and unmask in
> > > > > > PM hooks).
> > > > >
> > > > > At the very least, that's not what happens with this system. The
> > > > > interrupt is definitely not kept enabled in suspend, and the system
> > > > > would not wake from an EC interrupt. (see also this series, BTW:
> > > > > https://patchwork.kernel.org/cover/10921121/).
> > > > >
> > > > > > Just reading Documentation/power/suspend-and-interrupts.txt I'm led to
> > > > > > believe that the cros_ec driver shouldn't call disable_irq() on the
> > > > > > interrupt if it wants to wakeup from it:
> > > > > >
> > > > > > "Calling enable_irq_wake() causes suspend_device_irqs() to treat the
> > > > > > given IRQ in a special way. Namely, the IRQ remains enabled, by on the
> > > > > > first interrupt it will be disabled, marked as pending and "suspended"
> > > > > > so that it will be re-enabled by resume_device_irqs() during the
> > > > > > subsequent system resume. Also the PM core is notified about the event
> > > > > > which causes the system suspend in progress to be aborted (that doesn't
> > > > > > have to happen immediately, but at one of the points where the suspend
> > > > > > thread looks for pending wakeup events)."
> > > > >
> > > > > I think this describes the behaviour when you keep both enabled.
> > > > >
> > > > > > I suppose the problem is an irq line disabled in hardware that has
> > > > > > wakeup armed on it? Is this even valid? Shouldn't an irq be enabled for
> > > > > > wakeup to work?
> > > > >
> > > > > I couldn't really find a definite answer, but there are a bunch of
> > > > > examples of other drivers in the kernel:
> > > > > - drivers/extcon/extcon-usb-gpio.c:usb_extcon_suspend
> > > > > - drivers/hid/i2c-hid/i2c-hid.c:i2c_hid_suspend
> > > > > - drivers/mfd/max77843.c:max77843_suspend
> > > > > (not exhaustive, this is quite hard to grep for...)
> > > > >
> > > > > > We could immediately unmask those lines in the hardware when the
> > > > > > set_wake() callback is called. That way the genirq layer can use the
> > > > > > driver to do what it wants with the hardware and the driver can make
> > > > > > sure that set_wake() will always cause the wakeup interrupt to be
> > > > > > delivered to genirq even when software has disabled it.
> > > > > >
> > > > > > But I think that there might be a problem with how genirq understands
> > > > > > the masked state of a line when the wakeup implementation conflates
> > > > > > masked state with wakeup armed state. Consider this call-flow:
> > > > > >
> > > > > > irq masked in hardware, IRQD_IRQ_MASKED is set
> > > > > > enable_irq_wake()
> > > > > > unmask_irq in hardware
> > > > > > IRQD_WAKEUP_ARMED is set
> > > > > > <suspend and wakeup from irq>
> > > > > > handle_level_irq()
> > > > > > mask_ack_irq()
> > > > > > mask_irq()
> > > > > > if (irqd_irq_masked()) -> returns true and skips masking!
> > > > > > if (desc->irq_data.chip->irq_ack)
> > > > > > ...
> > > > > > irq_may_run()
> > > > > > irq_pm_check_wakeup()
> > > > > > irq_disable()
> > > > > > mask_irq() -> does nothing again
> > > > > >
> > > > > > In the above flow, we never mask the irq because we thought it was
> > > > > > already masked when it was disabled, but the irqchip implementation
> > > > > > unmasked it to make wakeup work. Maybe we should always mask the irq if
> > > > > > wakeup is armed and we're trying to call mask_irq()? Looks hacky.
> > > Maybe we can implement irqchip's mask_ack_irq in mediatek driver to
> > > always mask the irq. Then flow will always call it without judgment
> > > IRQD_IRQ_MASKED.
> > >
> > > diff --git a/drivers/pinctrl/mediatek/mtk-eint.c
> > > b/drivers/pinctrl/mediatek/mtk-
> > > index f464f8c..9f1aae2 100644
> > > --- a/drivers/pinctrl/mediatek/mtk-eint.c
> > > +++ b/drivers/pinctrl/mediatek/mtk-eint.c
> > > @@ -272,12 +272,19 @@ static void mtk_eint_irq_release_resources(struct
> > > irq_data
> > > gpiochip_unlock_as_irq(gpio_c, gpio_n);
> > > }
> > >
> > > +static void mtk_eint_mask_ack(struct irq_data *d)
> > > +{
> > > + mtk_eint_mask(d);
> > > + mtk_eint_ack(d);
> > > +}
> > > +
> > > static struct irq_chip mtk_eint_irq_chip = {
> > > .name = "mt-eint",
> > > .irq_disable = mtk_eint_mask,
> > > .irq_mask = mtk_eint_mask,
> > > .irq_unmask = mtk_eint_unmask,
> > > .irq_ack = mtk_eint_ack,
> > > + .irq_mask_ack = mtk_eint_mask_ack,
> > > .irq_set_type = mtk_eint_set_type,
> > > .irq_set_wake = mtk_eint_irq_set_wake,
> > > .irq_request_resources = mtk_eint_irq_request_resources,
> > >
> > > This seems like a small change.
> > > thanks.
> >
> > Does this work? My understanding is that Linux thinks the irq is
> > _already_ masked, so it short-circuits in the generic IRQ code and
> > doesn't call mask again.
> > -Evan
>
> Yes, you are right.
>
> The underlying problem is really that the hardware IRQ enabled state is
> out of sync with what linux thinks.In resume flow,Linux thinks the irq
> is _already_masked, so it short-circuits in the generic IRQ code and
> doesn't call mask again.So in step 3 will have a interrupt storm.
>
> But we implement irqchip's mask_ack_irq so that mask_ack_irq() calls
> desc->irq_data.chip->irq_mask_ack instead of mask_irq() that needs to
> judge IRQD_IRQ_MASKED. This will correctly set cur_mask[irq] = 0 to
> sync with kernel state.
Oh, I see. I guess that would work as mask_ack_irq does not check for
current masked status before calling irq_chip->irq_mask_ack... But
that's only if irq_chip->irq_mask_ack is defined, else it just calls
mask_irq which does that check.
But if we follow bf22ff45bed ("genirq: Avoid unnecessary low level irq
function calls"), this is actually a bug and we should add that same
check to mask_ack_irq (and since pinctrl-rockchip.c does not implement
irq_mask_ack, it's understandable why the author missed that).
So I'm not sure if this is the right change either...
> Also, this patch can solve the issue of [1/2] in this patchset[1] which
> also is the interrupt mask cannot be set correctly due to hardware irq
> state not sync kernel.
>
> [1] https://patchwork.kernel.org/patch/10921143/
>
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH 2/2] pinctrl: mediatek: Update cur_mask in mask/mask ops
@ 2019-06-04 0:57 ` Nicolas Boichat
0 siblings, 0 replies; 50+ messages in thread
From: Nicolas Boichat @ 2019-06-04 0:57 UTC (permalink / raw)
To: Chuanjia Liu
Cc: Evan Green, Stephen Boyd,
moderated list:ARM/Mediatek SoC support, Sean Wang,
Linus Walleij, Matthias Brugger, open list:GPIO SUBSYSTEM,
linux-arm Mailing List, lkml, Thomas Gleixner
On Mon, Jun 3, 2019 at 4:01 PM Chuanjia Liu <Chuanjia.Liu@mediatek.com> wrote:
>
> On Fri, 2019-05-31 at 10:17 -0700, Evan Green wrote:
> > On Fri, May 31, 2019 at 1:06 AM Chuanjia Liu <Chuanjia.Liu@mediatek.com> wrote:
> > >
> > > On Thu, 2019-05-30 at 10:12 -0700, Evan Green wrote:
> > > > On Wed, May 15, 2019 at 1:05 AM Nicolas Boichat <drinkcat@chromium.org> wrote:
> > > > >
> > > > > On Wed, May 15, 2019 at 4:14 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > > > > >
> > > > > > Quoting Nicolas Boichat (2019-05-13 18:37:58)
> > > > > > > On Tue, May 14, 2019 at 6:29 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > > > > > > >
> > > > > > > > Quoting Nicolas Boichat (2019-04-28 20:55:15)
> > > > > > > > > During suspend/resume, mtk_eint_mask may be called while
> > > > > > > > > wake_mask is active. For example, this happens if a wake-source
> > > > > > > > > with an active interrupt handler wakes the system:
> > > > > > > > > irq/pm.c:irq_pm_check_wakeup would disable the interrupt, so
> > > > > > > > > that it can be handled later on in the resume flow.
> > > > > > > > >
> > > > > > > > > However, this may happen before mtk_eint_do_resume is called:
> > > > > > > > > in this case, wake_mask is loaded, and cur_mask is restored
> > > > > > > > > from an older copy, re-enabling the interrupt, and causing
> > > > > > > > > an interrupt storm (especially for level interrupts).
> > > > > > > > >
> > > > > > > > > Instead, we just record mask/unmask changes in cur_mask. This
> > > > > > > > > also avoids the need to read the current mask in eint_do_suspend,
> > > > > > > > > and we can remove mtk_eint_chip_read_mask function.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> > > > > > > >
> > > > > > > > It looks an awful lot like you should just use IRQCHIP_MASK_ON_SUSPEND
> > > > > > > > here. Isn't that what's happening? All non-wake irqs should be masked at
> > > > > > > > the hardware level so they can't cause a wakeup during suspend and on
> > > > > > > > resume they can be unmasked?
> > > > > > >
> > > > > > > No, this is for an line that has both wake and interrupt enabled. To
> > > > > > > reword the commit message above:
> > > > > >
> > > > > > Is my understanding correct that there isn't a different "wake up"
> > > > > > register that can be written to cause a GPIO to be configured to wake
> > > > > > the system from suspend? The only way to do so is to leave the GPIO
> > > > > > unmasked in the hardware by having EINT_EN[irq] = 1? And thus any
> > > > > > interrupts that we don't want to wake us up during suspend should be
> > > > > > masked in the hardware?
> > > > >
> > > > > Yes, that's my understanding as well.
> > > > >
> > > > > And then, what this driver does is to emulate the behaviour of a
> > > > > controller that would actually have separate irq and wake enable
> > > > > registers.
> > > > >
> > > > > > If that's true, the code here that's trying to keep track of enabled
> > > > > > irqs and wakeup enabled irqs can be replaced with the irqchip flag so
> > > > > > that wakeup irqs are not masked while non-wakeups are masked.
> > > > >
> > > > > Correct, but with the caveat that I don't see anything that definitely
> > > > > requires an interrupt to be enabled to be a wake source. See below...
> > > > >
> > > > > >
> > > > > > > 1. cur_mask[irq] = 1; wake_mask[irq] = 1; EINT_EN[irq] = 1 (interrupt
> > > > > > > enabled at hardware level)
> > > > > > > 2. System suspends, resumes due to that line (at this stage EINT_HW
> > > > > > > == wake_mask)
> > > > > > > 3. irq_pm_check_wakeup is called, and disables the interrupt =>
> > > > > > > EINT_EN[irq] = 0, but we still have cur_mask[irq] = 1
> > > > > > > 4. mtk_eint_do_resume is called, and restores EINT_EN = cur_mask, so
> > > > > > > it reenables EINT_EN[irq] = 1 => interrupt storm.
> > > > > > >
> > > > > > > This patch fixes the issue in step 3. So that the interrupt can be
> > > > > > > re-enabled properly later on, sometimes after mtk_eint_do_resume, when
> > > > > > > the driver is ready to handle it.
> > > > > >
> > > > > > Right, we'd rather not see irqchip drivers working around the genirq
> > > > > > layer to do these things like tracking cur_mask and wake_mask. That
> > > > > > leads to subtle bugs and makes the driver maintain state across the
> > > > > > irqchip callbacks and system suspend/resume.
> > > > > >
> > > > > > >
> > > > > > > Also, IRQCHIP_MASK_ON_SUSPEND does not handle lines that are enabled
> > > > > > > as a wake source, but without interrupt enabled (e.g. cros_ec driver
> > > > > > > does that), which we do want to support.
> > > > > >
> > > > > > Hmm. I thought that even if the irq is disabled by a driver, that would
> > > > > > be a lazy disable so it isn't really masked in the hardware. Then if an
> > > > > > interrupt comes in during suspend on a wake configured irq line, the
> > > > > > hardware will have left it unmasked because IRQCHIP_MASK_ON_SUSPEND in
> > > > > > combination with lazy disable would mean that the line is left unmasked
> > > > > > (ignoring whatever this mediatek driver is doing to mask and unmask in
> > > > > > PM hooks).
> > > > >
> > > > > At the very least, that's not what happens with this system. The
> > > > > interrupt is definitely not kept enabled in suspend, and the system
> > > > > would not wake from an EC interrupt. (see also this series, BTW:
> > > > > https://patchwork.kernel.org/cover/10921121/).
> > > > >
> > > > > > Just reading Documentation/power/suspend-and-interrupts.txt I'm led to
> > > > > > believe that the cros_ec driver shouldn't call disable_irq() on the
> > > > > > interrupt if it wants to wakeup from it:
> > > > > >
> > > > > > "Calling enable_irq_wake() causes suspend_device_irqs() to treat the
> > > > > > given IRQ in a special way. Namely, the IRQ remains enabled, by on the
> > > > > > first interrupt it will be disabled, marked as pending and "suspended"
> > > > > > so that it will be re-enabled by resume_device_irqs() during the
> > > > > > subsequent system resume. Also the PM core is notified about the event
> > > > > > which causes the system suspend in progress to be aborted (that doesn't
> > > > > > have to happen immediately, but at one of the points where the suspend
> > > > > > thread looks for pending wakeup events)."
> > > > >
> > > > > I think this describes the behaviour when you keep both enabled.
> > > > >
> > > > > > I suppose the problem is an irq line disabled in hardware that has
> > > > > > wakeup armed on it? Is this even valid? Shouldn't an irq be enabled for
> > > > > > wakeup to work?
> > > > >
> > > > > I couldn't really find a definite answer, but there are a bunch of
> > > > > examples of other drivers in the kernel:
> > > > > - drivers/extcon/extcon-usb-gpio.c:usb_extcon_suspend
> > > > > - drivers/hid/i2c-hid/i2c-hid.c:i2c_hid_suspend
> > > > > - drivers/mfd/max77843.c:max77843_suspend
> > > > > (not exhaustive, this is quite hard to grep for...)
> > > > >
> > > > > > We could immediately unmask those lines in the hardware when the
> > > > > > set_wake() callback is called. That way the genirq layer can use the
> > > > > > driver to do what it wants with the hardware and the driver can make
> > > > > > sure that set_wake() will always cause the wakeup interrupt to be
> > > > > > delivered to genirq even when software has disabled it.
> > > > > >
> > > > > > But I think that there might be a problem with how genirq understands
> > > > > > the masked state of a line when the wakeup implementation conflates
> > > > > > masked state with wakeup armed state. Consider this call-flow:
> > > > > >
> > > > > > irq masked in hardware, IRQD_IRQ_MASKED is set
> > > > > > enable_irq_wake()
> > > > > > unmask_irq in hardware
> > > > > > IRQD_WAKEUP_ARMED is set
> > > > > > <suspend and wakeup from irq>
> > > > > > handle_level_irq()
> > > > > > mask_ack_irq()
> > > > > > mask_irq()
> > > > > > if (irqd_irq_masked()) -> returns true and skips masking!
> > > > > > if (desc->irq_data.chip->irq_ack)
> > > > > > ...
> > > > > > irq_may_run()
> > > > > > irq_pm_check_wakeup()
> > > > > > irq_disable()
> > > > > > mask_irq() -> does nothing again
> > > > > >
> > > > > > In the above flow, we never mask the irq because we thought it was
> > > > > > already masked when it was disabled, but the irqchip implementation
> > > > > > unmasked it to make wakeup work. Maybe we should always mask the irq if
> > > > > > wakeup is armed and we're trying to call mask_irq()? Looks hacky.
> > > Maybe we can implement irqchip's mask_ack_irq in mediatek driver to
> > > always mask the irq. Then flow will always call it without judgment
> > > IRQD_IRQ_MASKED.
> > >
> > > diff --git a/drivers/pinctrl/mediatek/mtk-eint.c
> > > b/drivers/pinctrl/mediatek/mtk-
> > > index f464f8c..9f1aae2 100644
> > > --- a/drivers/pinctrl/mediatek/mtk-eint.c
> > > +++ b/drivers/pinctrl/mediatek/mtk-eint.c
> > > @@ -272,12 +272,19 @@ static void mtk_eint_irq_release_resources(struct
> > > irq_data
> > > gpiochip_unlock_as_irq(gpio_c, gpio_n);
> > > }
> > >
> > > +static void mtk_eint_mask_ack(struct irq_data *d)
> > > +{
> > > + mtk_eint_mask(d);
> > > + mtk_eint_ack(d);
> > > +}
> > > +
> > > static struct irq_chip mtk_eint_irq_chip = {
> > > .name = "mt-eint",
> > > .irq_disable = mtk_eint_mask,
> > > .irq_mask = mtk_eint_mask,
> > > .irq_unmask = mtk_eint_unmask,
> > > .irq_ack = mtk_eint_ack,
> > > + .irq_mask_ack = mtk_eint_mask_ack,
> > > .irq_set_type = mtk_eint_set_type,
> > > .irq_set_wake = mtk_eint_irq_set_wake,
> > > .irq_request_resources = mtk_eint_irq_request_resources,
> > >
> > > This seems like a small change.
> > > thanks.
> >
> > Does this work? My understanding is that Linux thinks the irq is
> > _already_ masked, so it short-circuits in the generic IRQ code and
> > doesn't call mask again.
> > -Evan
>
> Yes, you are right.
>
> The underlying problem is really that the hardware IRQ enabled state is
> out of sync with what linux thinks.In resume flow,Linux thinks the irq
> is _already_masked, so it short-circuits in the generic IRQ code and
> doesn't call mask again.So in step 3 will have a interrupt storm.
>
> But we implement irqchip's mask_ack_irq so that mask_ack_irq() calls
> desc->irq_data.chip->irq_mask_ack instead of mask_irq() that needs to
> judge IRQD_IRQ_MASKED. This will correctly set cur_mask[irq] = 0 to
> sync with kernel state.
Oh, I see. I guess that would work as mask_ack_irq does not check for
current masked status before calling irq_chip->irq_mask_ack... But
that's only if irq_chip->irq_mask_ack is defined, else it just calls
mask_irq which does that check.
But if we follow bf22ff45bed ("genirq: Avoid unnecessary low level irq
function calls"), this is actually a bug and we should add that same
check to mask_ack_irq (and since pinctrl-rockchip.c does not implement
irq_mask_ack, it's understandable why the author missed that).
So I'm not sure if this is the right change either...
> Also, this patch can solve the issue of [1/2] in this patchset[1] which
> also is the interrupt mask cannot be set correctly due to hardware irq
> state not sync kernel.
>
> [1] https://patchwork.kernel.org/patch/10921143/
>
>
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH 2/2] pinctrl: mediatek: Update cur_mask in mask/mask ops
2019-06-04 0:57 ` Nicolas Boichat
(?)
@ 2019-06-05 7:11 ` Chuanjia Liu
-1 siblings, 0 replies; 50+ messages in thread
From: Chuanjia Liu @ 2019-06-05 7:11 UTC (permalink / raw)
To: Nicolas Boichat
Cc: Evan Green, Stephen Boyd,
moderated list:ARM/Mediatek SoC support, Sean Wang,
Linus Walleij, Matthias Brugger, open list:GPIO SUBSYSTEM,
linux-arm Mailing List, lkml, Thomas Gleixner
On Tue, 2019-06-04 at 08:57 +0800, Nicolas Boichat wrote:
> On Mon, Jun 3, 2019 at 4:01 PM Chuanjia Liu <Chuanjia.Liu@mediatek.com> wrote:
> >
> > On Fri, 2019-05-31 at 10:17 -0700, Evan Green wrote:
> > > On Fri, May 31, 2019 at 1:06 AM Chuanjia Liu <Chuanjia.Liu@mediatek.com> wrote:
> > > >
> > > > On Thu, 2019-05-30 at 10:12 -0700, Evan Green wrote:
> > > > > On Wed, May 15, 2019 at 1:05 AM Nicolas Boichat <drinkcat@chromium.org> wrote:
> > > > > >
> > > > > > On Wed, May 15, 2019 at 4:14 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > > > > > >
> > > > > > > Quoting Nicolas Boichat (2019-05-13 18:37:58)
> > > > > > > > On Tue, May 14, 2019 at 6:29 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > > > > > > > >
> > > > > > > > > Quoting Nicolas Boichat (2019-04-28 20:55:15)
> > > > > > > > > > During suspend/resume, mtk_eint_mask may be called while
> > > > > > > > > > wake_mask is active. For example, this happens if a wake-source
> > > > > > > > > > with an active interrupt handler wakes the system:
> > > > > > > > > > irq/pm.c:irq_pm_check_wakeup would disable the interrupt, so
> > > > > > > > > > that it can be handled later on in the resume flow.
> > > > > > > > > >
> > > > > > > > > > However, this may happen before mtk_eint_do_resume is called:
> > > > > > > > > > in this case, wake_mask is loaded, and cur_mask is restored
> > > > > > > > > > from an older copy, re-enabling the interrupt, and causing
> > > > > > > > > > an interrupt storm (especially for level interrupts).
> > > > > > > > > >
> > > > > > > > > > Instead, we just record mask/unmask changes in cur_mask. This
> > > > > > > > > > also avoids the need to read the current mask in eint_do_suspend,
> > > > > > > > > > and we can remove mtk_eint_chip_read_mask function.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> > > > > > > > >
> > > > > > > > > It looks an awful lot like you should just use IRQCHIP_MASK_ON_SUSPEND
> > > > > > > > > here. Isn't that what's happening? All non-wake irqs should be masked at
> > > > > > > > > the hardware level so they can't cause a wakeup during suspend and on
> > > > > > > > > resume they can be unmasked?
> > > > > > > >
> > > > > > > > No, this is for an line that has both wake and interrupt enabled. To
> > > > > > > > reword the commit message above:
> > > > > > >
> > > > > > > Is my understanding correct that there isn't a different "wake up"
> > > > > > > register that can be written to cause a GPIO to be configured to wake
> > > > > > > the system from suspend? The only way to do so is to leave the GPIO
> > > > > > > unmasked in the hardware by having EINT_EN[irq] = 1? And thus any
> > > > > > > interrupts that we don't want to wake us up during suspend should be
> > > > > > > masked in the hardware?
> > > > > >
> > > > > > Yes, that's my understanding as well.
> > > > > >
> > > > > > And then, what this driver does is to emulate the behaviour of a
> > > > > > controller that would actually have separate irq and wake enable
> > > > > > registers.
> > > > > >
> > > > > > > If that's true, the code here that's trying to keep track of enabled
> > > > > > > irqs and wakeup enabled irqs can be replaced with the irqchip flag so
> > > > > > > that wakeup irqs are not masked while non-wakeups are masked.
> > > > > >
> > > > > > Correct, but with the caveat that I don't see anything that definitely
> > > > > > requires an interrupt to be enabled to be a wake source. See below...
> > > > > >
> > > > > > >
> > > > > > > > 1. cur_mask[irq] = 1; wake_mask[irq] = 1; EINT_EN[irq] = 1 (interrupt
> > > > > > > > enabled at hardware level)
> > > > > > > > 2. System suspends, resumes due to that line (at this stage EINT_HW
> > > > > > > > == wake_mask)
> > > > > > > > 3. irq_pm_check_wakeup is called, and disables the interrupt =>
> > > > > > > > EINT_EN[irq] = 0, but we still have cur_mask[irq] = 1
> > > > > > > > 4. mtk_eint_do_resume is called, and restores EINT_EN = cur_mask, so
> > > > > > > > it reenables EINT_EN[irq] = 1 => interrupt storm.
> > > > > > > >
> > > > > > > > This patch fixes the issue in step 3. So that the interrupt can be
> > > > > > > > re-enabled properly later on, sometimes after mtk_eint_do_resume, when
> > > > > > > > the driver is ready to handle it.
> > > > > > >
> > > > > > > Right, we'd rather not see irqchip drivers working around the genirq
> > > > > > > layer to do these things like tracking cur_mask and wake_mask. That
> > > > > > > leads to subtle bugs and makes the driver maintain state across the
> > > > > > > irqchip callbacks and system suspend/resume.
> > > > > > >
> > > > > > > >
> > > > > > > > Also, IRQCHIP_MASK_ON_SUSPEND does not handle lines that are enabled
> > > > > > > > as a wake source, but without interrupt enabled (e.g. cros_ec driver
> > > > > > > > does that), which we do want to support.
> > > > > > >
> > > > > > > Hmm. I thought that even if the irq is disabled by a driver, that would
> > > > > > > be a lazy disable so it isn't really masked in the hardware. Then if an
> > > > > > > interrupt comes in during suspend on a wake configured irq line, the
> > > > > > > hardware will have left it unmasked because IRQCHIP_MASK_ON_SUSPEND in
> > > > > > > combination with lazy disable would mean that the line is left unmasked
> > > > > > > (ignoring whatever this mediatek driver is doing to mask and unmask in
> > > > > > > PM hooks).
> > > > > >
> > > > > > At the very least, that's not what happens with this system. The
> > > > > > interrupt is definitely not kept enabled in suspend, and the system
> > > > > > would not wake from an EC interrupt. (see also this series, BTW:
> > > > > > https://patchwork.kernel.org/cover/10921121/).
> > > > > >
> > > > > > > Just reading Documentation/power/suspend-and-interrupts.txt I'm led to
> > > > > > > believe that the cros_ec driver shouldn't call disable_irq() on the
> > > > > > > interrupt if it wants to wakeup from it:
> > > > > > >
> > > > > > > "Calling enable_irq_wake() causes suspend_device_irqs() to treat the
> > > > > > > given IRQ in a special way. Namely, the IRQ remains enabled, by on the
> > > > > > > first interrupt it will be disabled, marked as pending and "suspended"
> > > > > > > so that it will be re-enabled by resume_device_irqs() during the
> > > > > > > subsequent system resume. Also the PM core is notified about the event
> > > > > > > which causes the system suspend in progress to be aborted (that doesn't
> > > > > > > have to happen immediately, but at one of the points where the suspend
> > > > > > > thread looks for pending wakeup events)."
> > > > > >
> > > > > > I think this describes the behaviour when you keep both enabled.
> > > > > >
> > > > > > > I suppose the problem is an irq line disabled in hardware that has
> > > > > > > wakeup armed on it? Is this even valid? Shouldn't an irq be enabled for
> > > > > > > wakeup to work?
> > > > > >
> > > > > > I couldn't really find a definite answer, but there are a bunch of
> > > > > > examples of other drivers in the kernel:
> > > > > > - drivers/extcon/extcon-usb-gpio.c:usb_extcon_suspend
> > > > > > - drivers/hid/i2c-hid/i2c-hid.c:i2c_hid_suspend
> > > > > > - drivers/mfd/max77843.c:max77843_suspend
> > > > > > (not exhaustive, this is quite hard to grep for...)
> > > > > >
> > > > > > > We could immediately unmask those lines in the hardware when the
> > > > > > > set_wake() callback is called. That way the genirq layer can use the
> > > > > > > driver to do what it wants with the hardware and the driver can make
> > > > > > > sure that set_wake() will always cause the wakeup interrupt to be
> > > > > > > delivered to genirq even when software has disabled it.
> > > > > > >
> > > > > > > But I think that there might be a problem with how genirq understands
> > > > > > > the masked state of a line when the wakeup implementation conflates
> > > > > > > masked state with wakeup armed state. Consider this call-flow:
> > > > > > >
> > > > > > > irq masked in hardware, IRQD_IRQ_MASKED is set
> > > > > > > enable_irq_wake()
> > > > > > > unmask_irq in hardware
> > > > > > > IRQD_WAKEUP_ARMED is set
> > > > > > > <suspend and wakeup from irq>
> > > > > > > handle_level_irq()
> > > > > > > mask_ack_irq()
> > > > > > > mask_irq()
> > > > > > > if (irqd_irq_masked()) -> returns true and skips masking!
> > > > > > > if (desc->irq_data.chip->irq_ack)
> > > > > > > ...
> > > > > > > irq_may_run()
> > > > > > > irq_pm_check_wakeup()
> > > > > > > irq_disable()
> > > > > > > mask_irq() -> does nothing again
> > > > > > >
> > > > > > > In the above flow, we never mask the irq because we thought it was
> > > > > > > already masked when it was disabled, but the irqchip implementation
> > > > > > > unmasked it to make wakeup work. Maybe we should always mask the irq if
> > > > > > > wakeup is armed and we're trying to call mask_irq()? Looks hacky.
> > > > Maybe we can implement irqchip's mask_ack_irq in mediatek driver to
> > > > always mask the irq. Then flow will always call it without judgment
> > > > IRQD_IRQ_MASKED.
> > > >
> > > > diff --git a/drivers/pinctrl/mediatek/mtk-eint.c
> > > > b/drivers/pinctrl/mediatek/mtk-
> > > > index f464f8c..9f1aae2 100644
> > > > --- a/drivers/pinctrl/mediatek/mtk-eint.c
> > > > +++ b/drivers/pinctrl/mediatek/mtk-eint.c
> > > > @@ -272,12 +272,19 @@ static void mtk_eint_irq_release_resources(struct
> > > > irq_data
> > > > gpiochip_unlock_as_irq(gpio_c, gpio_n);
> > > > }
> > > >
> > > > +static void mtk_eint_mask_ack(struct irq_data *d)
> > > > +{
> > > > + mtk_eint_mask(d);
> > > > + mtk_eint_ack(d);
> > > > +}
> > > > +
> > > > static struct irq_chip mtk_eint_irq_chip = {
> > > > .name = "mt-eint",
> > > > .irq_disable = mtk_eint_mask,
> > > > .irq_mask = mtk_eint_mask,
> > > > .irq_unmask = mtk_eint_unmask,
> > > > .irq_ack = mtk_eint_ack,
> > > > + .irq_mask_ack = mtk_eint_mask_ack,
> > > > .irq_set_type = mtk_eint_set_type,
> > > > .irq_set_wake = mtk_eint_irq_set_wake,
> > > > .irq_request_resources = mtk_eint_irq_request_resources,
> > > >
> > > > This seems like a small change.
> > > > thanks.
> > >
> > > Does this work? My understanding is that Linux thinks the irq is
> > > _already_ masked, so it short-circuits in the generic IRQ code and
> > > doesn't call mask again.
> > > -Evan
> >
> > Yes, you are right.
> >
> > The underlying problem is really that the hardware IRQ enabled state is
> > out of sync with what linux thinks.In resume flow,Linux thinks the irq
> > is _already_masked, so it short-circuits in the generic IRQ code and
> > doesn't call mask again.So in step 3 will have a interrupt storm.
> >
> > But we implement irqchip's mask_ack_irq so that mask_ack_irq() calls
> > desc->irq_data.chip->irq_mask_ack instead of mask_irq() that needs to
> > judge IRQD_IRQ_MASKED. This will correctly set cur_mask[irq] = 0 to
> > sync with kernel state.
>
> Oh, I see. I guess that would work as mask_ack_irq does not check for
> current masked status before calling irq_chip->irq_mask_ack... But
> that's only if irq_chip->irq_mask_ack is defined, else it just calls
> mask_irq which does that check.
>
yes,so we define irq_chip->irq_mask_ack to avoid check.
> But if we follow bf22ff45bed ("genirq: Avoid unnecessary low level irq
> function calls"), this is actually a bug and we should add that same
> check to mask_ack_irq (and since pinctrl-rockchip.c does not implement
> irq_mask_ack, it's understandable why the author missed that).
>
If we added the same check to mask_ack_irq,it might cause ack_irq to
fail,because ack_irq should be executed regardless of state of the mask.
If we want avoid unnecessary low level irq function calls, we can choose
not define irq_chip->irq_mask_ack.In some application scenarios, taken
here, we can use this to call low level irq function to mask hwirq.
I'm also not sure what's the best approach here.
@sean,could you help comment this issue?
> So I'm not sure if this is the right change either...
>
> > Also, this patch can solve the issue of [1/2] in this patchset[1] which
> > also is the interrupt mask cannot be set correctly due to hardware irq
> > state not sync kernel.
> >
> > [1] https://patchwork.kernel.org/patch/10921143/
> >
> >
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH 2/2] pinctrl: mediatek: Update cur_mask in mask/mask ops
@ 2019-06-05 7:11 ` Chuanjia Liu
0 siblings, 0 replies; 50+ messages in thread
From: Chuanjia Liu @ 2019-06-05 7:11 UTC (permalink / raw)
To: Nicolas Boichat, Sean Wang
Cc: Linus Walleij, Sean Wang, lkml, Evan Green, Stephen Boyd,
open list:GPIO SUBSYSTEM,
moderated list:ARM/Mediatek SoC support, Matthias Brugger,
Thomas Gleixner, linux-arm Mailing List
On Tue, 2019-06-04 at 08:57 +0800, Nicolas Boichat wrote:
> On Mon, Jun 3, 2019 at 4:01 PM Chuanjia Liu <Chuanjia.Liu@mediatek.com> wrote:
> >
> > On Fri, 2019-05-31 at 10:17 -0700, Evan Green wrote:
> > > On Fri, May 31, 2019 at 1:06 AM Chuanjia Liu <Chuanjia.Liu@mediatek.com> wrote:
> > > >
> > > > On Thu, 2019-05-30 at 10:12 -0700, Evan Green wrote:
> > > > > On Wed, May 15, 2019 at 1:05 AM Nicolas Boichat <drinkcat@chromium.org> wrote:
> > > > > >
> > > > > > On Wed, May 15, 2019 at 4:14 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > > > > > >
> > > > > > > Quoting Nicolas Boichat (2019-05-13 18:37:58)
> > > > > > > > On Tue, May 14, 2019 at 6:29 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > > > > > > > >
> > > > > > > > > Quoting Nicolas Boichat (2019-04-28 20:55:15)
> > > > > > > > > > During suspend/resume, mtk_eint_mask may be called while
> > > > > > > > > > wake_mask is active. For example, this happens if a wake-source
> > > > > > > > > > with an active interrupt handler wakes the system:
> > > > > > > > > > irq/pm.c:irq_pm_check_wakeup would disable the interrupt, so
> > > > > > > > > > that it can be handled later on in the resume flow.
> > > > > > > > > >
> > > > > > > > > > However, this may happen before mtk_eint_do_resume is called:
> > > > > > > > > > in this case, wake_mask is loaded, and cur_mask is restored
> > > > > > > > > > from an older copy, re-enabling the interrupt, and causing
> > > > > > > > > > an interrupt storm (especially for level interrupts).
> > > > > > > > > >
> > > > > > > > > > Instead, we just record mask/unmask changes in cur_mask. This
> > > > > > > > > > also avoids the need to read the current mask in eint_do_suspend,
> > > > > > > > > > and we can remove mtk_eint_chip_read_mask function.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> > > > > > > > >
> > > > > > > > > It looks an awful lot like you should just use IRQCHIP_MASK_ON_SUSPEND
> > > > > > > > > here. Isn't that what's happening? All non-wake irqs should be masked at
> > > > > > > > > the hardware level so they can't cause a wakeup during suspend and on
> > > > > > > > > resume they can be unmasked?
> > > > > > > >
> > > > > > > > No, this is for an line that has both wake and interrupt enabled. To
> > > > > > > > reword the commit message above:
> > > > > > >
> > > > > > > Is my understanding correct that there isn't a different "wake up"
> > > > > > > register that can be written to cause a GPIO to be configured to wake
> > > > > > > the system from suspend? The only way to do so is to leave the GPIO
> > > > > > > unmasked in the hardware by having EINT_EN[irq] = 1? And thus any
> > > > > > > interrupts that we don't want to wake us up during suspend should be
> > > > > > > masked in the hardware?
> > > > > >
> > > > > > Yes, that's my understanding as well.
> > > > > >
> > > > > > And then, what this driver does is to emulate the behaviour of a
> > > > > > controller that would actually have separate irq and wake enable
> > > > > > registers.
> > > > > >
> > > > > > > If that's true, the code here that's trying to keep track of enabled
> > > > > > > irqs and wakeup enabled irqs can be replaced with the irqchip flag so
> > > > > > > that wakeup irqs are not masked while non-wakeups are masked.
> > > > > >
> > > > > > Correct, but with the caveat that I don't see anything that definitely
> > > > > > requires an interrupt to be enabled to be a wake source. See below...
> > > > > >
> > > > > > >
> > > > > > > > 1. cur_mask[irq] = 1; wake_mask[irq] = 1; EINT_EN[irq] = 1 (interrupt
> > > > > > > > enabled at hardware level)
> > > > > > > > 2. System suspends, resumes due to that line (at this stage EINT_HW
> > > > > > > > == wake_mask)
> > > > > > > > 3. irq_pm_check_wakeup is called, and disables the interrupt =>
> > > > > > > > EINT_EN[irq] = 0, but we still have cur_mask[irq] = 1
> > > > > > > > 4. mtk_eint_do_resume is called, and restores EINT_EN = cur_mask, so
> > > > > > > > it reenables EINT_EN[irq] = 1 => interrupt storm.
> > > > > > > >
> > > > > > > > This patch fixes the issue in step 3. So that the interrupt can be
> > > > > > > > re-enabled properly later on, sometimes after mtk_eint_do_resume, when
> > > > > > > > the driver is ready to handle it.
> > > > > > >
> > > > > > > Right, we'd rather not see irqchip drivers working around the genirq
> > > > > > > layer to do these things like tracking cur_mask and wake_mask. That
> > > > > > > leads to subtle bugs and makes the driver maintain state across the
> > > > > > > irqchip callbacks and system suspend/resume.
> > > > > > >
> > > > > > > >
> > > > > > > > Also, IRQCHIP_MASK_ON_SUSPEND does not handle lines that are enabled
> > > > > > > > as a wake source, but without interrupt enabled (e.g. cros_ec driver
> > > > > > > > does that), which we do want to support.
> > > > > > >
> > > > > > > Hmm. I thought that even if the irq is disabled by a driver, that would
> > > > > > > be a lazy disable so it isn't really masked in the hardware. Then if an
> > > > > > > interrupt comes in during suspend on a wake configured irq line, the
> > > > > > > hardware will have left it unmasked because IRQCHIP_MASK_ON_SUSPEND in
> > > > > > > combination with lazy disable would mean that the line is left unmasked
> > > > > > > (ignoring whatever this mediatek driver is doing to mask and unmask in
> > > > > > > PM hooks).
> > > > > >
> > > > > > At the very least, that's not what happens with this system. The
> > > > > > interrupt is definitely not kept enabled in suspend, and the system
> > > > > > would not wake from an EC interrupt. (see also this series, BTW:
> > > > > > https://patchwork.kernel.org/cover/10921121/).
> > > > > >
> > > > > > > Just reading Documentation/power/suspend-and-interrupts.txt I'm led to
> > > > > > > believe that the cros_ec driver shouldn't call disable_irq() on the
> > > > > > > interrupt if it wants to wakeup from it:
> > > > > > >
> > > > > > > "Calling enable_irq_wake() causes suspend_device_irqs() to treat the
> > > > > > > given IRQ in a special way. Namely, the IRQ remains enabled, by on the
> > > > > > > first interrupt it will be disabled, marked as pending and "suspended"
> > > > > > > so that it will be re-enabled by resume_device_irqs() during the
> > > > > > > subsequent system resume. Also the PM core is notified about the event
> > > > > > > which causes the system suspend in progress to be aborted (that doesn't
> > > > > > > have to happen immediately, but at one of the points where the suspend
> > > > > > > thread looks for pending wakeup events)."
> > > > > >
> > > > > > I think this describes the behaviour when you keep both enabled.
> > > > > >
> > > > > > > I suppose the problem is an irq line disabled in hardware that has
> > > > > > > wakeup armed on it? Is this even valid? Shouldn't an irq be enabled for
> > > > > > > wakeup to work?
> > > > > >
> > > > > > I couldn't really find a definite answer, but there are a bunch of
> > > > > > examples of other drivers in the kernel:
> > > > > > - drivers/extcon/extcon-usb-gpio.c:usb_extcon_suspend
> > > > > > - drivers/hid/i2c-hid/i2c-hid.c:i2c_hid_suspend
> > > > > > - drivers/mfd/max77843.c:max77843_suspend
> > > > > > (not exhaustive, this is quite hard to grep for...)
> > > > > >
> > > > > > > We could immediately unmask those lines in the hardware when the
> > > > > > > set_wake() callback is called. That way the genirq layer can use the
> > > > > > > driver to do what it wants with the hardware and the driver can make
> > > > > > > sure that set_wake() will always cause the wakeup interrupt to be
> > > > > > > delivered to genirq even when software has disabled it.
> > > > > > >
> > > > > > > But I think that there might be a problem with how genirq understands
> > > > > > > the masked state of a line when the wakeup implementation conflates
> > > > > > > masked state with wakeup armed state. Consider this call-flow:
> > > > > > >
> > > > > > > irq masked in hardware, IRQD_IRQ_MASKED is set
> > > > > > > enable_irq_wake()
> > > > > > > unmask_irq in hardware
> > > > > > > IRQD_WAKEUP_ARMED is set
> > > > > > > <suspend and wakeup from irq>
> > > > > > > handle_level_irq()
> > > > > > > mask_ack_irq()
> > > > > > > mask_irq()
> > > > > > > if (irqd_irq_masked()) -> returns true and skips masking!
> > > > > > > if (desc->irq_data.chip->irq_ack)
> > > > > > > ...
> > > > > > > irq_may_run()
> > > > > > > irq_pm_check_wakeup()
> > > > > > > irq_disable()
> > > > > > > mask_irq() -> does nothing again
> > > > > > >
> > > > > > > In the above flow, we never mask the irq because we thought it was
> > > > > > > already masked when it was disabled, but the irqchip implementation
> > > > > > > unmasked it to make wakeup work. Maybe we should always mask the irq if
> > > > > > > wakeup is armed and we're trying to call mask_irq()? Looks hacky.
> > > > Maybe we can implement irqchip's mask_ack_irq in mediatek driver to
> > > > always mask the irq. Then flow will always call it without judgment
> > > > IRQD_IRQ_MASKED.
> > > >
> > > > diff --git a/drivers/pinctrl/mediatek/mtk-eint.c
> > > > b/drivers/pinctrl/mediatek/mtk-
> > > > index f464f8c..9f1aae2 100644
> > > > --- a/drivers/pinctrl/mediatek/mtk-eint.c
> > > > +++ b/drivers/pinctrl/mediatek/mtk-eint.c
> > > > @@ -272,12 +272,19 @@ static void mtk_eint_irq_release_resources(struct
> > > > irq_data
> > > > gpiochip_unlock_as_irq(gpio_c, gpio_n);
> > > > }
> > > >
> > > > +static void mtk_eint_mask_ack(struct irq_data *d)
> > > > +{
> > > > + mtk_eint_mask(d);
> > > > + mtk_eint_ack(d);
> > > > +}
> > > > +
> > > > static struct irq_chip mtk_eint_irq_chip = {
> > > > .name = "mt-eint",
> > > > .irq_disable = mtk_eint_mask,
> > > > .irq_mask = mtk_eint_mask,
> > > > .irq_unmask = mtk_eint_unmask,
> > > > .irq_ack = mtk_eint_ack,
> > > > + .irq_mask_ack = mtk_eint_mask_ack,
> > > > .irq_set_type = mtk_eint_set_type,
> > > > .irq_set_wake = mtk_eint_irq_set_wake,
> > > > .irq_request_resources = mtk_eint_irq_request_resources,
> > > >
> > > > This seems like a small change.
> > > > thanks.
> > >
> > > Does this work? My understanding is that Linux thinks the irq is
> > > _already_ masked, so it short-circuits in the generic IRQ code and
> > > doesn't call mask again.
> > > -Evan
> >
> > Yes, you are right.
> >
> > The underlying problem is really that the hardware IRQ enabled state is
> > out of sync with what linux thinks.In resume flow,Linux thinks the irq
> > is _already_masked, so it short-circuits in the generic IRQ code and
> > doesn't call mask again.So in step 3 will have a interrupt storm.
> >
> > But we implement irqchip's mask_ack_irq so that mask_ack_irq() calls
> > desc->irq_data.chip->irq_mask_ack instead of mask_irq() that needs to
> > judge IRQD_IRQ_MASKED. This will correctly set cur_mask[irq] = 0 to
> > sync with kernel state.
>
> Oh, I see. I guess that would work as mask_ack_irq does not check for
> current masked status before calling irq_chip->irq_mask_ack... But
> that's only if irq_chip->irq_mask_ack is defined, else it just calls
> mask_irq which does that check.
>
yes,so we define irq_chip->irq_mask_ack to avoid check.
> But if we follow bf22ff45bed ("genirq: Avoid unnecessary low level irq
> function calls"), this is actually a bug and we should add that same
> check to mask_ack_irq (and since pinctrl-rockchip.c does not implement
> irq_mask_ack, it's understandable why the author missed that).
>
If we added the same check to mask_ack_irq,it might cause ack_irq to
fail,because ack_irq should be executed regardless of state of the mask.
If we want avoid unnecessary low level irq function calls, we can choose
not define irq_chip->irq_mask_ack.In some application scenarios, taken
here, we can use this to call low level irq function to mask hwirq.
I'm also not sure what's the best approach here.
@sean,could you help comment this issue?
> So I'm not sure if this is the right change either...
>
> > Also, this patch can solve the issue of [1/2] in this patchset[1] which
> > also is the interrupt mask cannot be set correctly due to hardware irq
> > state not sync kernel.
> >
> > [1] https://patchwork.kernel.org/patch/10921143/
> >
> >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH 2/2] pinctrl: mediatek: Update cur_mask in mask/mask ops
@ 2019-06-05 7:11 ` Chuanjia Liu
0 siblings, 0 replies; 50+ messages in thread
From: Chuanjia Liu @ 2019-06-05 7:11 UTC (permalink / raw)
To: Nicolas Boichat, Sean Wang
Cc: Evan Green, Stephen Boyd,
moderated list:ARM/Mediatek SoC support, Sean Wang,
Linus Walleij, Matthias Brugger, open list:GPIO SUBSYSTEM,
linux-arm Mailing List, lkml, Thomas Gleixner
On Tue, 2019-06-04 at 08:57 +0800, Nicolas Boichat wrote:
> On Mon, Jun 3, 2019 at 4:01 PM Chuanjia Liu <Chuanjia.Liu@mediatek.com> wrote:
> >
> > On Fri, 2019-05-31 at 10:17 -0700, Evan Green wrote:
> > > On Fri, May 31, 2019 at 1:06 AM Chuanjia Liu <Chuanjia.Liu@mediatek.com> wrote:
> > > >
> > > > On Thu, 2019-05-30 at 10:12 -0700, Evan Green wrote:
> > > > > On Wed, May 15, 2019 at 1:05 AM Nicolas Boichat <drinkcat@chromium.org> wrote:
> > > > > >
> > > > > > On Wed, May 15, 2019 at 4:14 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > > > > > >
> > > > > > > Quoting Nicolas Boichat (2019-05-13 18:37:58)
> > > > > > > > On Tue, May 14, 2019 at 6:29 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > > > > > > > >
> > > > > > > > > Quoting Nicolas Boichat (2019-04-28 20:55:15)
> > > > > > > > > > During suspend/resume, mtk_eint_mask may be called while
> > > > > > > > > > wake_mask is active. For example, this happens if a wake-source
> > > > > > > > > > with an active interrupt handler wakes the system:
> > > > > > > > > > irq/pm.c:irq_pm_check_wakeup would disable the interrupt, so
> > > > > > > > > > that it can be handled later on in the resume flow.
> > > > > > > > > >
> > > > > > > > > > However, this may happen before mtk_eint_do_resume is called:
> > > > > > > > > > in this case, wake_mask is loaded, and cur_mask is restored
> > > > > > > > > > from an older copy, re-enabling the interrupt, and causing
> > > > > > > > > > an interrupt storm (especially for level interrupts).
> > > > > > > > > >
> > > > > > > > > > Instead, we just record mask/unmask changes in cur_mask. This
> > > > > > > > > > also avoids the need to read the current mask in eint_do_suspend,
> > > > > > > > > > and we can remove mtk_eint_chip_read_mask function.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> > > > > > > > >
> > > > > > > > > It looks an awful lot like you should just use IRQCHIP_MASK_ON_SUSPEND
> > > > > > > > > here. Isn't that what's happening? All non-wake irqs should be masked at
> > > > > > > > > the hardware level so they can't cause a wakeup during suspend and on
> > > > > > > > > resume they can be unmasked?
> > > > > > > >
> > > > > > > > No, this is for an line that has both wake and interrupt enabled. To
> > > > > > > > reword the commit message above:
> > > > > > >
> > > > > > > Is my understanding correct that there isn't a different "wake up"
> > > > > > > register that can be written to cause a GPIO to be configured to wake
> > > > > > > the system from suspend? The only way to do so is to leave the GPIO
> > > > > > > unmasked in the hardware by having EINT_EN[irq] = 1? And thus any
> > > > > > > interrupts that we don't want to wake us up during suspend should be
> > > > > > > masked in the hardware?
> > > > > >
> > > > > > Yes, that's my understanding as well.
> > > > > >
> > > > > > And then, what this driver does is to emulate the behaviour of a
> > > > > > controller that would actually have separate irq and wake enable
> > > > > > registers.
> > > > > >
> > > > > > > If that's true, the code here that's trying to keep track of enabled
> > > > > > > irqs and wakeup enabled irqs can be replaced with the irqchip flag so
> > > > > > > that wakeup irqs are not masked while non-wakeups are masked.
> > > > > >
> > > > > > Correct, but with the caveat that I don't see anything that definitely
> > > > > > requires an interrupt to be enabled to be a wake source. See below...
> > > > > >
> > > > > > >
> > > > > > > > 1. cur_mask[irq] = 1; wake_mask[irq] = 1; EINT_EN[irq] = 1 (interrupt
> > > > > > > > enabled at hardware level)
> > > > > > > > 2. System suspends, resumes due to that line (at this stage EINT_HW
> > > > > > > > == wake_mask)
> > > > > > > > 3. irq_pm_check_wakeup is called, and disables the interrupt =>
> > > > > > > > EINT_EN[irq] = 0, but we still have cur_mask[irq] = 1
> > > > > > > > 4. mtk_eint_do_resume is called, and restores EINT_EN = cur_mask, so
> > > > > > > > it reenables EINT_EN[irq] = 1 => interrupt storm.
> > > > > > > >
> > > > > > > > This patch fixes the issue in step 3. So that the interrupt can be
> > > > > > > > re-enabled properly later on, sometimes after mtk_eint_do_resume, when
> > > > > > > > the driver is ready to handle it.
> > > > > > >
> > > > > > > Right, we'd rather not see irqchip drivers working around the genirq
> > > > > > > layer to do these things like tracking cur_mask and wake_mask. That
> > > > > > > leads to subtle bugs and makes the driver maintain state across the
> > > > > > > irqchip callbacks and system suspend/resume.
> > > > > > >
> > > > > > > >
> > > > > > > > Also, IRQCHIP_MASK_ON_SUSPEND does not handle lines that are enabled
> > > > > > > > as a wake source, but without interrupt enabled (e.g. cros_ec driver
> > > > > > > > does that), which we do want to support.
> > > > > > >
> > > > > > > Hmm. I thought that even if the irq is disabled by a driver, that would
> > > > > > > be a lazy disable so it isn't really masked in the hardware. Then if an
> > > > > > > interrupt comes in during suspend on a wake configured irq line, the
> > > > > > > hardware will have left it unmasked because IRQCHIP_MASK_ON_SUSPEND in
> > > > > > > combination with lazy disable would mean that the line is left unmasked
> > > > > > > (ignoring whatever this mediatek driver is doing to mask and unmask in
> > > > > > > PM hooks).
> > > > > >
> > > > > > At the very least, that's not what happens with this system. The
> > > > > > interrupt is definitely not kept enabled in suspend, and the system
> > > > > > would not wake from an EC interrupt. (see also this series, BTW:
> > > > > > https://patchwork.kernel.org/cover/10921121/).
> > > > > >
> > > > > > > Just reading Documentation/power/suspend-and-interrupts.txt I'm led to
> > > > > > > believe that the cros_ec driver shouldn't call disable_irq() on the
> > > > > > > interrupt if it wants to wakeup from it:
> > > > > > >
> > > > > > > "Calling enable_irq_wake() causes suspend_device_irqs() to treat the
> > > > > > > given IRQ in a special way. Namely, the IRQ remains enabled, by on the
> > > > > > > first interrupt it will be disabled, marked as pending and "suspended"
> > > > > > > so that it will be re-enabled by resume_device_irqs() during the
> > > > > > > subsequent system resume. Also the PM core is notified about the event
> > > > > > > which causes the system suspend in progress to be aborted (that doesn't
> > > > > > > have to happen immediately, but at one of the points where the suspend
> > > > > > > thread looks for pending wakeup events)."
> > > > > >
> > > > > > I think this describes the behaviour when you keep both enabled.
> > > > > >
> > > > > > > I suppose the problem is an irq line disabled in hardware that has
> > > > > > > wakeup armed on it? Is this even valid? Shouldn't an irq be enabled for
> > > > > > > wakeup to work?
> > > > > >
> > > > > > I couldn't really find a definite answer, but there are a bunch of
> > > > > > examples of other drivers in the kernel:
> > > > > > - drivers/extcon/extcon-usb-gpio.c:usb_extcon_suspend
> > > > > > - drivers/hid/i2c-hid/i2c-hid.c:i2c_hid_suspend
> > > > > > - drivers/mfd/max77843.c:max77843_suspend
> > > > > > (not exhaustive, this is quite hard to grep for...)
> > > > > >
> > > > > > > We could immediately unmask those lines in the hardware when the
> > > > > > > set_wake() callback is called. That way the genirq layer can use the
> > > > > > > driver to do what it wants with the hardware and the driver can make
> > > > > > > sure that set_wake() will always cause the wakeup interrupt to be
> > > > > > > delivered to genirq even when software has disabled it.
> > > > > > >
> > > > > > > But I think that there might be a problem with how genirq understands
> > > > > > > the masked state of a line when the wakeup implementation conflates
> > > > > > > masked state with wakeup armed state. Consider this call-flow:
> > > > > > >
> > > > > > > irq masked in hardware, IRQD_IRQ_MASKED is set
> > > > > > > enable_irq_wake()
> > > > > > > unmask_irq in hardware
> > > > > > > IRQD_WAKEUP_ARMED is set
> > > > > > > <suspend and wakeup from irq>
> > > > > > > handle_level_irq()
> > > > > > > mask_ack_irq()
> > > > > > > mask_irq()
> > > > > > > if (irqd_irq_masked()) -> returns true and skips masking!
> > > > > > > if (desc->irq_data.chip->irq_ack)
> > > > > > > ...
> > > > > > > irq_may_run()
> > > > > > > irq_pm_check_wakeup()
> > > > > > > irq_disable()
> > > > > > > mask_irq() -> does nothing again
> > > > > > >
> > > > > > > In the above flow, we never mask the irq because we thought it was
> > > > > > > already masked when it was disabled, but the irqchip implementation
> > > > > > > unmasked it to make wakeup work. Maybe we should always mask the irq if
> > > > > > > wakeup is armed and we're trying to call mask_irq()? Looks hacky.
> > > > Maybe we can implement irqchip's mask_ack_irq in mediatek driver to
> > > > always mask the irq. Then flow will always call it without judgment
> > > > IRQD_IRQ_MASKED.
> > > >
> > > > diff --git a/drivers/pinctrl/mediatek/mtk-eint.c
> > > > b/drivers/pinctrl/mediatek/mtk-
> > > > index f464f8c..9f1aae2 100644
> > > > --- a/drivers/pinctrl/mediatek/mtk-eint.c
> > > > +++ b/drivers/pinctrl/mediatek/mtk-eint.c
> > > > @@ -272,12 +272,19 @@ static void mtk_eint_irq_release_resources(struct
> > > > irq_data
> > > > gpiochip_unlock_as_irq(gpio_c, gpio_n);
> > > > }
> > > >
> > > > +static void mtk_eint_mask_ack(struct irq_data *d)
> > > > +{
> > > > + mtk_eint_mask(d);
> > > > + mtk_eint_ack(d);
> > > > +}
> > > > +
> > > > static struct irq_chip mtk_eint_irq_chip = {
> > > > .name = "mt-eint",
> > > > .irq_disable = mtk_eint_mask,
> > > > .irq_mask = mtk_eint_mask,
> > > > .irq_unmask = mtk_eint_unmask,
> > > > .irq_ack = mtk_eint_ack,
> > > > + .irq_mask_ack = mtk_eint_mask_ack,
> > > > .irq_set_type = mtk_eint_set_type,
> > > > .irq_set_wake = mtk_eint_irq_set_wake,
> > > > .irq_request_resources = mtk_eint_irq_request_resources,
> > > >
> > > > This seems like a small change.
> > > > thanks.
> > >
> > > Does this work? My understanding is that Linux thinks the irq is
> > > _already_ masked, so it short-circuits in the generic IRQ code and
> > > doesn't call mask again.
> > > -Evan
> >
> > Yes, you are right.
> >
> > The underlying problem is really that the hardware IRQ enabled state is
> > out of sync with what linux thinks.In resume flow,Linux thinks the irq
> > is _already_masked, so it short-circuits in the generic IRQ code and
> > doesn't call mask again.So in step 3 will have a interrupt storm.
> >
> > But we implement irqchip's mask_ack_irq so that mask_ack_irq() calls
> > desc->irq_data.chip->irq_mask_ack instead of mask_irq() that needs to
> > judge IRQD_IRQ_MASKED. This will correctly set cur_mask[irq] = 0 to
> > sync with kernel state.
>
> Oh, I see. I guess that would work as mask_ack_irq does not check for
> current masked status before calling irq_chip->irq_mask_ack... But
> that's only if irq_chip->irq_mask_ack is defined, else it just calls
> mask_irq which does that check.
>
yes,so we define irq_chip->irq_mask_ack to avoid check.
> But if we follow bf22ff45bed ("genirq: Avoid unnecessary low level irq
> function calls"), this is actually a bug and we should add that same
> check to mask_ack_irq (and since pinctrl-rockchip.c does not implement
> irq_mask_ack, it's understandable why the author missed that).
>
If we added the same check to mask_ack_irq,it might cause ack_irq to
fail,because ack_irq should be executed regardless of state of the mask.
If we want avoid unnecessary low level irq function calls, we can choose
not define irq_chip->irq_mask_ack.In some application scenarios, taken
here, we can use this to call low level irq function to mask hwirq.
I'm also not sure what's the best approach here.
@sean,could you help comment this issue?
> So I'm not sure if this is the right change either...
>
> > Also, this patch can solve the issue of [1/2] in this patchset[1] which
> > also is the interrupt mask cannot be set correctly due to hardware irq
> > state not sync kernel.
> >
> > [1] https://patchwork.kernel.org/patch/10921143/
> >
> >
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH 2/2] pinctrl: mediatek: Update cur_mask in mask/mask ops
2019-05-30 17:12 ` Evan Green
(?)
@ 2019-06-10 22:25 ` Stephen Boyd
-1 siblings, 0 replies; 50+ messages in thread
From: Stephen Boyd @ 2019-06-10 22:25 UTC (permalink / raw)
To: Evan Green, Nicolas Boichat
Cc: moderated list:ARM/Mediatek SoC support, Sean Wang,
Linus Walleij, Matthias Brugger, linux-gpio,
linux-arm Mailing List, lkml, Chuanjia Liu, Thomas Gleixner
Quoting Evan Green (2019-05-30 10:12:03)
> On Wed, May 15, 2019 at 1:05 AM Nicolas Boichat <drinkcat@chromium.org> wrote:
> >
> > On Wed, May 15, 2019 at 4:14 AM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > > We could immediately unmask those lines in the hardware when the
> > > set_wake() callback is called. That way the genirq layer can use the
> > > driver to do what it wants with the hardware and the driver can make
> > > sure that set_wake() will always cause the wakeup interrupt to be
> > > delivered to genirq even when software has disabled it.
> > >
> > > But I think that there might be a problem with how genirq understands
> > > the masked state of a line when the wakeup implementation conflates
> > > masked state with wakeup armed state. Consider this call-flow:
> > >
> > > irq masked in hardware, IRQD_IRQ_MASKED is set
> > > enable_irq_wake()
> > > unmask_irq in hardware
> > > IRQD_WAKEUP_ARMED is set
> > > <suspend and wakeup from irq>
> > > handle_level_irq()
> > > mask_ack_irq()
> > > mask_irq()
> > > if (irqd_irq_masked()) -> returns true and skips masking!
> > > if (desc->irq_data.chip->irq_ack)
> > > ...
> > > irq_may_run()
> > > irq_pm_check_wakeup()
> > > irq_disable()
> > > mask_irq() -> does nothing again
> > >
> > > In the above flow, we never mask the irq because we thought it was
> > > already masked when it was disabled, but the irqchip implementation
> > > unmasked it to make wakeup work. Maybe we should always mask the irq if
> > > wakeup is armed and we're trying to call mask_irq()? Looks hacky.
> > >
> > > diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> > > index 51128bea3846..20257d528880 100644
> > > --- a/kernel/irq/chip.c
> > > +++ b/kernel/irq/chip.c
> > > @@ -411,7 +411,7 @@ static inline void mask_ack_irq(struct irq_desc *desc)
> > >
> > > void mask_irq(struct irq_desc *desc)
> > > {
> > > - if (irqd_irq_masked(&desc->irq_data))
> > > + if (!irqd_is_wakeup_armed(&desc->irq_data) && irqd_irq_masked(&desc->irq_data))
> > > return;
> > >
> > > if (desc->irq_data.chip->irq_mask) {
> >
> > I'm... really not sure what's the best approach here. But basically,
> > yes, if we can find a way to properly handle wake and interrupt
> > behaviour for drivers with a single mask, that'd be good.
> > IRQCHIP_MASK_ON_SUSPEND only seems to be doing half of the work, since
> > it does not cover the disable+wake source case.
> >
> > Thanks,
>
> I finally got around to studying this patch. This series seems okay to
> me. The underlying problem is really that the hardware IRQ enabled
> state is out of sync with what Linux thinks. This happens during
> suspend because Linux thinks the irq is disabled, but due to the
> hardware constraints on this platform, the interrupt has to be enabled
> for it to be a wake source. So the mtk driver re-enables the
> interrupt, and then has to find a way to get back in sync with Linux's
> IRQ mask state.
>
> One possible approach is mentioned above by Stephen: stop calling
> disable_irq in the cros EC driver. Then both linux and mtk agree the
> interrupt is enabled at suspend time. I think this ran into other
> problems though, where the EC gets its interrupt but is unable to
> silence it because the underlying SPI bus is still suspended.
Does this happen? I thought the interrupt would only be delivered once
all drivers have resumed from the noirq resume phase. Maybe the SPI
controller needs to resume there instead of in the normal resume path
and then this isn't a problem.
>
> The other approach, taken here, is to mask the interrupt when it first
> comes in, getting Linux and mtk back in agreement that yes, the
> interrupt is masked. Outside of enlightening the generic IRQ core
> about these types of interrupts that need to get re-enabled to be wake
> sources, this seems like a reasonable approach.
I prefer we teach the genirq layer about these types of irqchips. That
way the solution is common and not a one-off fix for Mediatek chips. I'm
sure that this same problem will come up again for another SoC vendor
out there so having a chip flag that does the other half of
IRQCHIP_MASK_ON_SUSPEND would be appropriate.
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH 2/2] pinctrl: mediatek: Update cur_mask in mask/mask ops
@ 2019-06-10 22:25 ` Stephen Boyd
0 siblings, 0 replies; 50+ messages in thread
From: Stephen Boyd @ 2019-06-10 22:25 UTC (permalink / raw)
To: Evan Green, Nicolas Boichat
Cc: Chuanjia Liu, Linus Walleij, Sean Wang, lkml, linux-gpio,
moderated list:ARM/Mediatek SoC support, Matthias Brugger,
Thomas Gleixner, linux-arm Mailing List
Quoting Evan Green (2019-05-30 10:12:03)
> On Wed, May 15, 2019 at 1:05 AM Nicolas Boichat <drinkcat@chromium.org> wrote:
> >
> > On Wed, May 15, 2019 at 4:14 AM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > > We could immediately unmask those lines in the hardware when the
> > > set_wake() callback is called. That way the genirq layer can use the
> > > driver to do what it wants with the hardware and the driver can make
> > > sure that set_wake() will always cause the wakeup interrupt to be
> > > delivered to genirq even when software has disabled it.
> > >
> > > But I think that there might be a problem with how genirq understands
> > > the masked state of a line when the wakeup implementation conflates
> > > masked state with wakeup armed state. Consider this call-flow:
> > >
> > > irq masked in hardware, IRQD_IRQ_MASKED is set
> > > enable_irq_wake()
> > > unmask_irq in hardware
> > > IRQD_WAKEUP_ARMED is set
> > > <suspend and wakeup from irq>
> > > handle_level_irq()
> > > mask_ack_irq()
> > > mask_irq()
> > > if (irqd_irq_masked()) -> returns true and skips masking!
> > > if (desc->irq_data.chip->irq_ack)
> > > ...
> > > irq_may_run()
> > > irq_pm_check_wakeup()
> > > irq_disable()
> > > mask_irq() -> does nothing again
> > >
> > > In the above flow, we never mask the irq because we thought it was
> > > already masked when it was disabled, but the irqchip implementation
> > > unmasked it to make wakeup work. Maybe we should always mask the irq if
> > > wakeup is armed and we're trying to call mask_irq()? Looks hacky.
> > >
> > > diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> > > index 51128bea3846..20257d528880 100644
> > > --- a/kernel/irq/chip.c
> > > +++ b/kernel/irq/chip.c
> > > @@ -411,7 +411,7 @@ static inline void mask_ack_irq(struct irq_desc *desc)
> > >
> > > void mask_irq(struct irq_desc *desc)
> > > {
> > > - if (irqd_irq_masked(&desc->irq_data))
> > > + if (!irqd_is_wakeup_armed(&desc->irq_data) && irqd_irq_masked(&desc->irq_data))
> > > return;
> > >
> > > if (desc->irq_data.chip->irq_mask) {
> >
> > I'm... really not sure what's the best approach here. But basically,
> > yes, if we can find a way to properly handle wake and interrupt
> > behaviour for drivers with a single mask, that'd be good.
> > IRQCHIP_MASK_ON_SUSPEND only seems to be doing half of the work, since
> > it does not cover the disable+wake source case.
> >
> > Thanks,
>
> I finally got around to studying this patch. This series seems okay to
> me. The underlying problem is really that the hardware IRQ enabled
> state is out of sync with what Linux thinks. This happens during
> suspend because Linux thinks the irq is disabled, but due to the
> hardware constraints on this platform, the interrupt has to be enabled
> for it to be a wake source. So the mtk driver re-enables the
> interrupt, and then has to find a way to get back in sync with Linux's
> IRQ mask state.
>
> One possible approach is mentioned above by Stephen: stop calling
> disable_irq in the cros EC driver. Then both linux and mtk agree the
> interrupt is enabled at suspend time. I think this ran into other
> problems though, where the EC gets its interrupt but is unable to
> silence it because the underlying SPI bus is still suspended.
Does this happen? I thought the interrupt would only be delivered once
all drivers have resumed from the noirq resume phase. Maybe the SPI
controller needs to resume there instead of in the normal resume path
and then this isn't a problem.
>
> The other approach, taken here, is to mask the interrupt when it first
> comes in, getting Linux and mtk back in agreement that yes, the
> interrupt is masked. Outside of enlightening the generic IRQ core
> about these types of interrupts that need to get re-enabled to be wake
> sources, this seems like a reasonable approach.
I prefer we teach the genirq layer about these types of irqchips. That
way the solution is common and not a one-off fix for Mediatek chips. I'm
sure that this same problem will come up again for another SoC vendor
out there so having a chip flag that does the other half of
IRQCHIP_MASK_ON_SUSPEND would be appropriate.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH 2/2] pinctrl: mediatek: Update cur_mask in mask/mask ops
@ 2019-06-10 22:25 ` Stephen Boyd
0 siblings, 0 replies; 50+ messages in thread
From: Stephen Boyd @ 2019-06-10 22:25 UTC (permalink / raw)
To: Evan Green, Nicolas Boichat
Cc: moderated list:ARM/Mediatek SoC support, Sean Wang,
Linus Walleij, Matthias Brugger, linux-gpio,
linux-arm Mailing List, lkml, Chuanjia Liu, Thomas Gleixner
Quoting Evan Green (2019-05-30 10:12:03)
> On Wed, May 15, 2019 at 1:05 AM Nicolas Boichat <drinkcat@chromium.org> wrote:
> >
> > On Wed, May 15, 2019 at 4:14 AM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > > We could immediately unmask those lines in the hardware when the
> > > set_wake() callback is called. That way the genirq layer can use the
> > > driver to do what it wants with the hardware and the driver can make
> > > sure that set_wake() will always cause the wakeup interrupt to be
> > > delivered to genirq even when software has disabled it.
> > >
> > > But I think that there might be a problem with how genirq understands
> > > the masked state of a line when the wakeup implementation conflates
> > > masked state with wakeup armed state. Consider this call-flow:
> > >
> > > irq masked in hardware, IRQD_IRQ_MASKED is set
> > > enable_irq_wake()
> > > unmask_irq in hardware
> > > IRQD_WAKEUP_ARMED is set
> > > <suspend and wakeup from irq>
> > > handle_level_irq()
> > > mask_ack_irq()
> > > mask_irq()
> > > if (irqd_irq_masked()) -> returns true and skips masking!
> > > if (desc->irq_data.chip->irq_ack)
> > > ...
> > > irq_may_run()
> > > irq_pm_check_wakeup()
> > > irq_disable()
> > > mask_irq() -> does nothing again
> > >
> > > In the above flow, we never mask the irq because we thought it was
> > > already masked when it was disabled, but the irqchip implementation
> > > unmasked it to make wakeup work. Maybe we should always mask the irq if
> > > wakeup is armed and we're trying to call mask_irq()? Looks hacky.
> > >
> > > diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> > > index 51128bea3846..20257d528880 100644
> > > --- a/kernel/irq/chip.c
> > > +++ b/kernel/irq/chip.c
> > > @@ -411,7 +411,7 @@ static inline void mask_ack_irq(struct irq_desc *desc)
> > >
> > > void mask_irq(struct irq_desc *desc)
> > > {
> > > - if (irqd_irq_masked(&desc->irq_data))
> > > + if (!irqd_is_wakeup_armed(&desc->irq_data) && irqd_irq_masked(&desc->irq_data))
> > > return;
> > >
> > > if (desc->irq_data.chip->irq_mask) {
> >
> > I'm... really not sure what's the best approach here. But basically,
> > yes, if we can find a way to properly handle wake and interrupt
> > behaviour for drivers with a single mask, that'd be good.
> > IRQCHIP_MASK_ON_SUSPEND only seems to be doing half of the work, since
> > it does not cover the disable+wake source case.
> >
> > Thanks,
>
> I finally got around to studying this patch. This series seems okay to
> me. The underlying problem is really that the hardware IRQ enabled
> state is out of sync with what Linux thinks. This happens during
> suspend because Linux thinks the irq is disabled, but due to the
> hardware constraints on this platform, the interrupt has to be enabled
> for it to be a wake source. So the mtk driver re-enables the
> interrupt, and then has to find a way to get back in sync with Linux's
> IRQ mask state.
>
> One possible approach is mentioned above by Stephen: stop calling
> disable_irq in the cros EC driver. Then both linux and mtk agree the
> interrupt is enabled at suspend time. I think this ran into other
> problems though, where the EC gets its interrupt but is unable to
> silence it because the underlying SPI bus is still suspended.
Does this happen? I thought the interrupt would only be delivered once
all drivers have resumed from the noirq resume phase. Maybe the SPI
controller needs to resume there instead of in the normal resume path
and then this isn't a problem.
>
> The other approach, taken here, is to mask the interrupt when it first
> comes in, getting Linux and mtk back in agreement that yes, the
> interrupt is masked. Outside of enlightening the generic IRQ core
> about these types of interrupts that need to get re-enabled to be wake
> sources, this seems like a reasonable approach.
I prefer we teach the genirq layer about these types of irqchips. That
way the solution is common and not a one-off fix for Mediatek chips. I'm
sure that this same problem will come up again for another SoC vendor
out there so having a chip flag that does the other half of
IRQCHIP_MASK_ON_SUSPEND would be appropriate.
^ permalink raw reply [flat|nested] 50+ messages in thread