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@xxxxxxxxxx; bgolaszewski@xxxxxxxxxxxx;
> linux-gpio@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Cc: dl-linux-imx <linux-imx@xxxxxxx>
> 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@xxxxxxx>
> ---
>  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