linux-kernel - [PATCH] gpio: mxc: skip GPIO

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <1541674114-843-1-git-send-email-Anson.Huang@nxp.com>
Date:   Thu, 8 Nov 2018 10:53:38 +0000
From:   Anson Huang <anson.huang@....com>
To:     "linus.walleij@...aro.org" <linus.walleij@...aro.org>,
        "bgolaszewski@...libre.com" <bgolaszewski@...libre.com>,
        "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC:     dl-linux-imx <linux-imx@....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@....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