[PATCH] gpio: mxc: skip GPIO_IMR restore in noirq resume

[PATCH] gpio: mxc: skip GPIO_IMR restore in noirq resume

[Anson Huang]

During the time window of noirq suspend and noirq resume,
if a GPIO pin is enabled as system wakeup source, the
corresponding GPIO line's IRQ will be unmasked, and GPIO bank
will NOT lost power, when GPIO irq arrives, generic irq handler
will mask it until GPIO driver is ready to handle it, but in GPIO
noirq resume callback, current implementation will restore the IMR
register using the value saved in previous noirq suspend callback
which is unmasked, so this GPIO line with IRQ pending will be
unmasked again after GPIO IMR regitster is restored, ARM
will be triggered to handle this IRQ again, because of the
change of commit bf22ff45bed6 ("genirq: Avoid unnecessary low
level irq function calls"), the mask_irq function will check
the status of irq_data to decide whether to mask the irq,
in this scenario, since the GPIO IRQ is being masked before
GPIO noirq resume, IRQD_IRQ_MASKED flag is set, so the second
time GPIO IRQ triggered by restoring GPIO IMR register, the
irq_mask callback will be skipped and cause ARM busy handling
the GPIO IRQ come all the way and no response to user anymore.

To make the scenario easy to understand, I take GPIO1_0 for
example when it is used as wake up source:

1. GPIO1_0 is enabled as wakeup source, it will be unmasked;
2. In noirq suspend, GPIO driver saves GPIO1_0's mask state in
   IMR register as "unmasked";
3. System go into suspend;
4. GPIO1_0 IRQ arrives, system wakes up;
5. Before noirq resume phase, ARM handles the GPIO1_0 IRQ, set
   IRQD_IRQ_MASKED flag and call GPIO irq_mask callback to mask
   it, as GPIO driver is NOT ready to handle it, now GPIO1_0
   IRQ is masked;
6. In GPIO noirq resume, GPIO driver restores GPIO registers,
   GPIO1_0 IRQ will be restored to unmask state again and system
   IRQ triggered;
7. ARM handles GPIO1_0 IRQ again, this time the GPIO1_0 will NOT be
   masked since its irq_data already has IRQD_IRQ_MASKED flag set;
8. GPIO1_0 IRQ keeps coming, ARM will be busy handling it but always
   skip the irq_mask operation, system no response.

Although this issue is exposed by commit bf22ff45bed6 ("genirq: Avoid
unnecessary low level irq function calls"), but actually we should
skip the GPIO IMR register restore, as the IMR register is NOT
atomic during the time window of GPIO noirq suspend and noirq resume,
it could be changed by ARM if there is GPIO IRQ pending at this window.

For the scenario of GPIO bank lost power, that means no GPIO pin is
enabled as wakeup source, all GPIO IRQs will be masked and it is
same as the reset value when GPIO bank power ON, so no issue for
this case.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
 drivers/gpio/gpio-mxc.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
index 5c69516..3d74ad7 100644
--- a/drivers/gpio/gpio-mxc.c
+++ b/drivers/gpio/gpio-mxc.c
@@ -531,7 +531,6 @@ static void mxc_gpio_save_regs(struct mxc_gpio_port *port)
 
 	port->gpio_saved_reg.icr1 = readl(port->base + GPIO_ICR1);
 	port->gpio_saved_reg.icr2 = readl(port->base + GPIO_ICR2);
-	port->gpio_saved_reg.imr = readl(port->base + GPIO_IMR);
 	port->gpio_saved_reg.gdir = readl(port->base + GPIO_GDIR);
 	port->gpio_saved_reg.edge_sel = readl(port->base + GPIO_EDGE_SEL);
 	port->gpio_saved_reg.dr = readl(port->base + GPIO_DR);
@@ -544,7 +543,6 @@ static void mxc_gpio_restore_regs(struct mxc_gpio_port *port)
 
 	writel(port->gpio_saved_reg.icr1, port->base + GPIO_ICR1);
 	writel(port->gpio_saved_reg.icr2, port->base + GPIO_ICR2);
-	writel(port->gpio_saved_reg.imr, port->base + GPIO_IMR);
 	writel(port->gpio_saved_reg.gdir, port->base + GPIO_GDIR);
 	writel(port->gpio_saved_reg.edge_sel, port->base + GPIO_EDGE_SEL);
 	writel(port->gpio_saved_reg.dr, port->base + GPIO_DR);
-- 
2.7.4

RE: [PATCH] gpio: mxc: skip GPIO_IMR restore in noirq resume

[Anson Huang]

Please ignore this patch, as it can NOT completely fix the issue of the case when GPIO IRQ coming during the noirq suspend/resume phase, the correct solution should be to save/restore the GPIO registers when local irq is off, so move the GPIO noirq suspend/resume to syscore phase, I have send out another patch "[PATCH] gpio: mxc: move gpio noirq suspend/resume to syscore phase", please review this patch, thanks!

Best Regards!
Anson Huang

> -----Original Message-----
> From: Anson Huang
> Sent: 2018年11月8日 18:54
> To: linus.walleij@linaro.org; bgolaszewski@baylibre.com;
> linux-gpio@vger.kernel.org; linux-kernel@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>
> Subject: [PATCH] gpio: mxc: skip GPIO_IMR restore in noirq resume
> 
> During the time window of noirq suspend and noirq resume, if a GPIO pin is
> enabled as system wakeup source, the corresponding GPIO line's IRQ will be
> unmasked, and GPIO bank will NOT lost power, when GPIO irq arrives, generic
> irq handler will mask it until GPIO driver is ready to handle it, but in GPIO noirq
> resume callback, current implementation will restore the IMR register using
> the value saved in previous noirq suspend callback which is unmasked, so this
> GPIO line with IRQ pending will be unmasked again after GPIO IMR regitster is
> restored, ARM will be triggered to handle this IRQ again, because of the change
> of commit bf22ff45bed6 ("genirq: Avoid unnecessary low level irq function
> calls"), the mask_irq function will check the status of irq_data to decide
> whether to mask the irq, in this scenario, since the GPIO IRQ is being masked
> before GPIO noirq resume, IRQD_IRQ_MASKED flag is set, so the second time
> GPIO IRQ triggered by restoring GPIO IMR register, the irq_mask callback will
> be skipped and cause ARM busy handling the GPIO IRQ come all the way and
> no response to user anymore.
> 
> To make the scenario easy to understand, I take GPIO1_0 for example when it
> is used as wake up source:
> 
> 1. GPIO1_0 is enabled as wakeup source, it will be unmasked; 2. In noirq
> suspend, GPIO driver saves GPIO1_0's mask state in
>    IMR register as "unmasked";
> 3. System go into suspend;
> 4. GPIO1_0 IRQ arrives, system wakes up; 5. Before noirq resume phase, ARM
> handles the GPIO1_0 IRQ, set
>    IRQD_IRQ_MASKED flag and call GPIO irq_mask callback to mask
>    it, as GPIO driver is NOT ready to handle it, now GPIO1_0
>    IRQ is masked;
> 6. In GPIO noirq resume, GPIO driver restores GPIO registers,
>    GPIO1_0 IRQ will be restored to unmask state again and system
>    IRQ triggered;
> 7. ARM handles GPIO1_0 IRQ again, this time the GPIO1_0 will NOT be
>    masked since its irq_data already has IRQD_IRQ_MASKED flag set; 8.
> GPIO1_0 IRQ keeps coming, ARM will be busy handling it but always
>    skip the irq_mask operation, system no response.
> 
> Although this issue is exposed by commit bf22ff45bed6 ("genirq: Avoid
> unnecessary low level irq function calls"), but actually we should skip the GPIO
> IMR register restore, as the IMR register is NOT atomic during the time window
> of GPIO noirq suspend and noirq resume, it could be changed by ARM if there
> is GPIO IRQ pending at this window.
> 
> For the scenario of GPIO bank lost power, that means no GPIO pin is enabled
> as wakeup source, all GPIO IRQs will be masked and it is same as the reset
> value when GPIO bank power ON, so no issue for this case.
> 
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
>  drivers/gpio/gpio-mxc.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c index
> 5c69516..3d74ad7 100644
> --- a/drivers/gpio/gpio-mxc.c
> +++ b/drivers/gpio/gpio-mxc.c
> @@ -531,7 +531,6 @@ static void mxc_gpio_save_regs(struct mxc_gpio_port
> *port)
> 
>  	port->gpio_saved_reg.icr1 = readl(port->base + GPIO_ICR1);
>  	port->gpio_saved_reg.icr2 = readl(port->base + GPIO_ICR2);
> -	port->gpio_saved_reg.imr = readl(port->base + GPIO_IMR);
>  	port->gpio_saved_reg.gdir = readl(port->base + GPIO_GDIR);
>  	port->gpio_saved_reg.edge_sel = readl(port->base + GPIO_EDGE_SEL);
>  	port->gpio_saved_reg.dr = readl(port->base + GPIO_DR); @@ -544,7
> +543,6 @@ static void mxc_gpio_restore_regs(struct mxc_gpio_port *port)
> 
>  	writel(port->gpio_saved_reg.icr1, port->base + GPIO_ICR1);
>  	writel(port->gpio_saved_reg.icr2, port->base + GPIO_ICR2);
> -	writel(port->gpio_saved_reg.imr, port->base + GPIO_IMR);
>  	writel(port->gpio_saved_reg.gdir, port->base + GPIO_GDIR);
>  	writel(port->gpio_saved_reg.edge_sel, port->base + GPIO_EDGE_SEL);
>  	writel(port->gpio_saved_reg.dr, port->base + GPIO_DR);
> --
> 2.7.4