[PATCH 1/1] gpio: omap: Fix bad device access with setup_irq()

[PATCH 1/1] gpio: omap: Fix bad device access with setup_irq()

[Tony Lindgren]

Similar to omap_gpio_irq_type() let's make sure that the GPIO
is usable as an interrupt if the platform init code did not
call gpio_request(). Otherwise we can get invalid device access
after setup_irq():

WARNING: CPU: 0 PID: 1 at drivers/bus/omap_l3_noc.c:147 l3_interrupt_handler+0x214/0x340()
44000000.ocp:L3 Custom Error: MASTER MPU TARGET L4CFG (Idle): Data Access in Supervisor mode during Functional access
...
[<c05f21e4>] (__irq_svc) from [<c05f1974>] (_raw_spin_unlock_irqrestore+0x34/0x44)
[<c05f1974>] (_raw_spin_unlock_irqrestore) from [<c00914a8>] (__setup_irq+0x244/0x530)
[<c00914a8>] (__setup_irq) from [<c00917d4>] (setup_irq+0x40/0x8c)
[<c00917d4>] (setup_irq) from [<c0039c8c>] (omap_system_dma_probe+0x1d4/0x2b4)
[<c0039c8c>] (omap_system_dma_probe) from [<c03b2200>] (platform_drv_probe+0x44/0xa4)
...

We can fix this the same way omap_gpio_irq_type() is handling it.

Note that the long term solution is to change the gpio-omap driver
to handle the banks as separate driver instances. This will allow
us to rely on just runtime PM for tracking the bank specific state.

Reported-by: Russell King <rmk+kernel@arm.linux.org.uk>
Cc: Felipe Balbi <balbi@ti.com>
Cc: Javier Martinez Canillas <javier@dowhile0.org>
Cc: Kevin Hilman <khilman@kernel.org>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Russell King - ARM Linux <linux@arm.linux.org.uk>
Cc: Santosh Shilimkar <ssantosh@kernel.org>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/gpio/gpio-omap.c | 39 +++++++++++++++++++++++++++++++++------
 1 file changed, 33 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 30646cf..f476ae2 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -88,6 +88,8 @@ struct gpio_bank {
 #define BANK_USED(bank) (bank->mod_usage || bank->irq_usage)
 #define LINE_USED(line, offset) (line & (BIT(offset)))
 
+static void omap_gpio_unmask_irq(struct irq_data *d);
+
 static int omap_irq_to_gpio(struct gpio_bank *bank, unsigned int gpio_irq)
 {
 	return bank->chip.base + gpio_irq;
@@ -477,6 +479,16 @@ static int omap_gpio_is_input(struct gpio_bank *bank, int mask)
 	return readl_relaxed(reg) & mask;
 }
 
+static void omap_gpio_init_irq(struct gpio_bank *bank, unsigned gpio,
+			       unsigned offset)
+{
+	if (!LINE_USED(bank->mod_usage, offset)) {
+		omap_enable_gpio_module(bank, offset);
+		omap_set_gpio_direction(bank, offset, 1);
+	}
+	bank->irq_usage |= BIT(GPIO_INDEX(bank, gpio));
+}
+
 static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
 {
 	struct gpio_bank *bank = omap_irq_data_get_bank(d);
@@ -506,15 +518,11 @@ static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
 	spin_lock_irqsave(&bank->lock, flags);
 	offset = GPIO_INDEX(bank, gpio);
 	retval = omap_set_gpio_triggering(bank, offset, type);
-	if (!LINE_USED(bank->mod_usage, offset)) {
-		omap_enable_gpio_module(bank, offset);
-		omap_set_gpio_direction(bank, offset, 1);
-	} else if (!omap_gpio_is_input(bank, BIT(offset))) {
+	omap_gpio_init_irq(bank, gpio, offset);
+	if (!omap_gpio_is_input(bank, BIT(offset))) {
 		spin_unlock_irqrestore(&bank->lock, flags);
 		return -EINVAL;
 	}
-
-	bank->irq_usage |= BIT(GPIO_INDEX(bank, gpio));
 	spin_unlock_irqrestore(&bank->lock, flags);
 
 	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH))
@@ -792,6 +800,24 @@ exit:
 	pm_runtime_put(bank->dev);
 }
 
+static unsigned int omap_gpio_irq_startup(struct irq_data *d)
+{
+	struct gpio_bank *bank = omap_irq_data_get_bank(d);
+	unsigned int gpio = omap_irq_to_gpio(bank, d->hwirq);
+	unsigned long flags;
+	unsigned offset = GPIO_INDEX(bank, gpio);
+
+	if (!BANK_USED(bank))
+		pm_runtime_get_sync(bank->dev);
+
+	spin_lock_irqsave(&bank->lock, flags);
+	omap_gpio_init_irq(bank, gpio, offset);
+	spin_unlock_irqrestore(&bank->lock, flags);
+	omap_gpio_unmask_irq(d);
+
+	return 0;
+}
+
 static void omap_gpio_irq_shutdown(struct irq_data *d)
 {
 	struct gpio_bank *bank = omap_irq_data_get_bank(d);
@@ -1181,6 +1207,7 @@ static int omap_gpio_probe(struct platform_device *pdev)
 	if (!irqc)
 		return -ENOMEM;
 
+	irqc->irq_startup = omap_gpio_irq_startup,
 	irqc->irq_shutdown = omap_gpio_irq_shutdown,
 	irqc->irq_ack = omap_gpio_ack_irq,
 	irqc->irq_mask = omap_gpio_mask_irq,
-- 
2.1.4

Re: [PATCH 1/1] gpio: omap: Fix bad device access with setup_irq()

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1914 bytes --]

On Fri, Jan 16, 2015 at 02:50:50PM -0800, Tony Lindgren wrote:
> Similar to omap_gpio_irq_type() let's make sure that the GPIO
> is usable as an interrupt if the platform init code did not
> call gpio_request(). Otherwise we can get invalid device access
> after setup_irq():
> 
> WARNING: CPU: 0 PID: 1 at drivers/bus/omap_l3_noc.c:147 l3_interrupt_handler+0x214/0x340()
> 44000000.ocp:L3 Custom Error: MASTER MPU TARGET L4CFG (Idle): Data Access in Supervisor mode during Functional access
> ...
> [<c05f21e4>] (__irq_svc) from [<c05f1974>] (_raw_spin_unlock_irqrestore+0x34/0x44)
> [<c05f1974>] (_raw_spin_unlock_irqrestore) from [<c00914a8>] (__setup_irq+0x244/0x530)
> [<c00914a8>] (__setup_irq) from [<c00917d4>] (setup_irq+0x40/0x8c)
> [<c00917d4>] (setup_irq) from [<c0039c8c>] (omap_system_dma_probe+0x1d4/0x2b4)
> [<c0039c8c>] (omap_system_dma_probe) from [<c03b2200>] (platform_drv_probe+0x44/0xa4)
> ...
> 
> We can fix this the same way omap_gpio_irq_type() is handling it.
> 
> Note that the long term solution is to change the gpio-omap driver
> to handle the banks as separate driver instances. This will allow
> us to rely on just runtime PM for tracking the bank specific state.
> 
> Reported-by: Russell King <rmk+kernel@arm.linux.org.uk>
> Cc: Felipe Balbi <balbi@ti.com>
> Cc: Javier Martinez Canillas <javier@dowhile0.org>
> Cc: Kevin Hilman <khilman@kernel.org>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Russell King - ARM Linux <linux@arm.linux.org.uk>
> Cc: Santosh Shilimkar <ssantosh@kernel.org>
> Signed-off-by: Tony Lindgren <tony@atomide.com>

Tony sent me this patch privately for testing, I've boot tested with
AM437x IDK, AM437x SK and BBB with CONFIG_PREEMPT.

Here are the logs:

http://hastebin.com/vabenibema
http://hastebin.com/weyukexuzi
http://hastebin.com/siceyiwite

Tested-by: Felipe Balbi <balbi@ti.com>

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/1] gpio: omap: Fix bad device access with setup_irq()

[santosh shilimkar]

On 1/16/2015 2:50 PM, Tony Lindgren wrote:
> Similar to omap_gpio_irq_type() let's make sure that the GPIO
> is usable as an interrupt if the platform init code did not
> call gpio_request(). Otherwise we can get invalid device access
> after setup_irq():
>
I let Linus W comment on it but IIRC we chewed this issue last
time and the conclusion was the gpio_request() must have to be called
directly or indirectly in case of irq line.

One old thread on possibly similar issue is here[1]


> WARNING: CPU: 0 PID: 1 at drivers/bus/omap_l3_noc.c:147 l3_interrupt_handler+0x214/0x340()
> 44000000.ocp:L3 Custom Error: MASTER MPU TARGET L4CFG (Idle): Data Access in Supervisor mode during Functional access
> ...
> [<c05f21e4>] (__irq_svc) from [<c05f1974>] (_raw_spin_unlock_irqrestore+0x34/0x44)
> [<c05f1974>] (_raw_spin_unlock_irqrestore) from [<c00914a8>] (__setup_irq+0x244/0x530)
> [<c00914a8>] (__setup_irq) from [<c00917d4>] (setup_irq+0x40/0x8c)
> [<c00917d4>] (setup_irq) from [<c0039c8c>] (omap_system_dma_probe+0x1d4/0x2b4)
> [<c0039c8c>] (omap_system_dma_probe) from [<c03b2200>] (platform_drv_probe+0x44/0xa4)
> ...
>
> We can fix this the same way omap_gpio_irq_type() is handling it.
>
> Note that the long term solution is to change the gpio-omap driver
> to handle the banks as separate driver instances. This will allow
> us to rely on just runtime PM for tracking the bank specific state.
>
> Reported-by: Russell King <rmk+kernel@arm.linux.org.uk>
> Cc: Felipe Balbi <balbi@ti.com>
> Cc: Javier Martinez Canillas <javier@dowhile0.org>
> Cc: Kevin Hilman <khilman@kernel.org>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Russell King - ARM Linux <linux@arm.linux.org.uk>
> Cc: Santosh Shilimkar <ssantosh@kernel.org>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>   drivers/gpio/gpio-omap.c | 39 +++++++++++++++++++++++++++++++++------
>   1 file changed, 33 insertions(+), 6 deletions(-)
>
Is it really OMAP specific issue ? On OMAP, clocks needs to enabled for 
GPIO's to work which is what the init is doing but I believe the same
should apply to other GPIO controllers as well.

regards,
Santosh

[1] https://lkml.org/lkml/2013/8/27/509

Re: [PATCH 1/1] gpio: omap: Fix bad device access with setup_irq()

[Tony Lindgren]

* santosh shilimkar <santosh.shilimkar@oracle.com> [150116 16:23]:
> On 1/16/2015 2:50 PM, Tony Lindgren wrote:
> >Similar to omap_gpio_irq_type() let's make sure that the GPIO
> >is usable as an interrupt if the platform init code did not
> >call gpio_request(). Otherwise we can get invalid device access
> >after setup_irq():
> >
> I let Linus W comment on it but IIRC we chewed this issue last
> time and the conclusion was the gpio_request() must have to be called
> directly or indirectly in case of irq line.

This is a corner case where the error is triggered by a wrong,
non-GPIO IRQ so gpio_request() will never be called before setup_irq()
unlike for any legacy platform code.

The legacy and DT cases we're already handling in the gpio-omap.c
driver a while back with:

2f56e0a57ff1 ("gpio/omap: use gpiolib API to mark a GPIO used as an IRQ")
fac7fa162a19 ("gpio/omap: auto-setup a GPIO when used as an IRQ")
fa365e4d7290 ("gpio/omap: maintain GPIO and IRQ usage separately")

And most of that the bank specific hacks we can get rid of by making
the driver multple instances as that allows replacing BANK_USED
with just runtime PM.
 
> One old thread on possibly similar issue is here[1]
> 
> 
> >WARNING: CPU: 0 PID: 1 at drivers/bus/omap_l3_noc.c:147 l3_interrupt_handler+0x214/0x340()
> >44000000.ocp:L3 Custom Error: MASTER MPU TARGET L4CFG (Idle): Data Access in Supervisor mode during Functional access
> >...
> >[<c05f21e4>] (__irq_svc) from [<c05f1974>] (_raw_spin_unlock_irqrestore+0x34/0x44)
> >[<c05f1974>] (_raw_spin_unlock_irqrestore) from [<c00914a8>] (__setup_irq+0x244/0x530)
> >[<c00914a8>] (__setup_irq) from [<c00917d4>] (setup_irq+0x40/0x8c)
> >[<c00917d4>] (setup_irq) from [<c0039c8c>] (omap_system_dma_probe+0x1d4/0x2b4)
> >[<c0039c8c>] (omap_system_dma_probe) from [<c03b2200>] (platform_drv_probe+0x44/0xa4)
> >...
> >
> >We can fix this the same way omap_gpio_irq_type() is handling it.
> >
> >Note that the long term solution is to change the gpio-omap driver
> >to handle the banks as separate driver instances. This will allow
> >us to rely on just runtime PM for tracking the bank specific state.
> >
> >Reported-by: Russell King <rmk+kernel@arm.linux.org.uk>
> >Cc: Felipe Balbi <balbi@ti.com>
> >Cc: Javier Martinez Canillas <javier@dowhile0.org>
> >Cc: Kevin Hilman <khilman@kernel.org>
> >Cc: Linus Walleij <linus.walleij@linaro.org>
> >Cc: Russell King - ARM Linux <linux@arm.linux.org.uk>
> >Cc: Santosh Shilimkar <ssantosh@kernel.org>
> >Signed-off-by: Tony Lindgren <tony@atomide.com>
> >---
> >  drivers/gpio/gpio-omap.c | 39 +++++++++++++++++++++++++++++++++------
> >  1 file changed, 33 insertions(+), 6 deletions(-)
> >
> Is it really OMAP specific issue ? On OMAP, clocks needs to enabled for
> GPIO's to work which is what the init is doing but I believe the same
> should apply to other GPIO controllers as well.

In the long run it should be handled by the generic GPIO code IMO.

I doubt that's doable for the -rc series though. Probably only a
few platforms have hit PM related issues like this. And actually
the omap specific hacks become really minimal if we make the driver
have a separate instance for each GPIO bank.

Regards,

Tony
 
> [1] https://lkml.org/lkml/2013/8/27/509
> 

Re: [PATCH 1/1] gpio: omap: Fix bad device access with setup_irq()

[Russell King - ARM Linux]

On Fri, Jan 16, 2015 at 04:19:41PM -0800, santosh shilimkar wrote:
> On 1/16/2015 2:50 PM, Tony Lindgren wrote:
> >Similar to omap_gpio_irq_type() let's make sure that the GPIO
> >is usable as an interrupt if the platform init code did not
> >call gpio_request(). Otherwise we can get invalid device access
> >after setup_irq():
> >
> I let Linus W comment on it but IIRC we chewed this issue last
> time and the conclusion was the gpio_request() must have to be called
> directly or indirectly in case of irq line.

That's really not the issue here.

The issue is that it's possible to claim the interrupt, and then the
driver goes wrong - not only does it attempt in that case to access
hardware which is runtime suspended, if the interrupt is subsequently
freed, it will then do a pm_runtime_put().

The interrupt code is wrong there, plain and simple.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 1/1] gpio: omap: Fix bad device access with setup_irq()

[santosh.shilimkar]

On 1/16/15 5:00 PM, Tony Lindgren wrote:
> * santosh shilimkar <santosh.shilimkar@oracle.com> [150116 16:23]:
>> On 1/16/2015 2:50 PM, Tony Lindgren wrote:
>>> Similar to omap_gpio_irq_type() let's make sure that the GPIO
>>> is usable as an interrupt if the platform init code did not
>>> call gpio_request(). Otherwise we can get invalid device access
>>> after setup_irq():
>>>
>> I let Linus W comment on it but IIRC we chewed this issue last
>> time and the conclusion was the gpio_request() must have to be called
>> directly or indirectly in case of irq line.
>
> This is a corner case where the error is triggered by a wrong,
> non-GPIO IRQ so gpio_request() will never be called before setup_irq()
> unlike for any legacy platform code.
>
> The legacy and DT cases we're already handling in the gpio-omap.c
> driver a while back with:
>
> 2f56e0a57ff1 ("gpio/omap: use gpiolib API to mark a GPIO used as an IRQ")
> fac7fa162a19 ("gpio/omap: auto-setup a GPIO when used as an IRQ")
> fa365e4d7290 ("gpio/omap: maintain GPIO and IRQ usage separately")
>
> And most of that the bank specific hacks we can get rid of by making
> the driver multple instances as that allows replacing BANK_USED
> with just runtime PM.
>
Right. Thanks for expanding it.

Regards,
Santosh

Re: [PATCH 1/1] gpio: omap: Fix bad device access with setup_irq()

[Linus Walleij]

On Fri, Jan 16, 2015 at 11:50 PM, Tony Lindgren <tony@atomide.com> wrote:

> Similar to omap_gpio_irq_type() let's make sure that the GPIO
> is usable as an interrupt if the platform init code did not
> call gpio_request(). Otherwise we can get invalid device access
> after setup_irq():
>
> WARNING: CPU: 0 PID: 1 at drivers/bus/omap_l3_noc.c:147 l3_interrupt_handler+0x214/0x340()
> 44000000.ocp:L3 Custom Error: MASTER MPU TARGET L4CFG (Idle): Data Access in Supervisor mode during Functional access
> ...
> [<c05f21e4>] (__irq_svc) from [<c05f1974>] (_raw_spin_unlock_irqrestore+0x34/0x44)
> [<c05f1974>] (_raw_spin_unlock_irqrestore) from [<c00914a8>] (__setup_irq+0x244/0x530)
> [<c00914a8>] (__setup_irq) from [<c00917d4>] (setup_irq+0x40/0x8c)
> [<c00917d4>] (setup_irq) from [<c0039c8c>] (omap_system_dma_probe+0x1d4/0x2b4)
> [<c0039c8c>] (omap_system_dma_probe) from [<c03b2200>] (platform_drv_probe+0x44/0xa4)
> ...
>
> We can fix this the same way omap_gpio_irq_type() is handling it.
>
> Note that the long term solution is to change the gpio-omap driver
> to handle the banks as separate driver instances. This will allow
> us to rely on just runtime PM for tracking the bank specific state.
>
> Reported-by: Russell King <rmk+kernel@arm.linux.org.uk>
> Cc: Felipe Balbi <balbi@ti.com>
> Cc: Javier Martinez Canillas <javier@dowhile0.org>
> Cc: Kevin Hilman <khilman@kernel.org>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Russell King - ARM Linux <linux@arm.linux.org.uk>
> Cc: Santosh Shilimkar <ssantosh@kernel.org>
> Signed-off-by: Tony Lindgren <tony@atomide.com>

Patch applied for fixes with Felipe's Tested-by.

Yours,
Linus Walleij

Re: [PATCH 1/1] gpio: omap: Fix bad device access with setup_irq()

[Javier Martinez Canillas]

Hello Tony,

On Fri, Jan 16, 2015 at 11:50 PM, Tony Lindgren <tony@atomide.com> wrote:
> Similar to omap_gpio_irq_type() let's make sure that the GPIO
> is usable as an interrupt if the platform init code did not
> call gpio_request(). Otherwise we can get invalid device access
> after setup_irq():
>
> WARNING: CPU: 0 PID: 1 at drivers/bus/omap_l3_noc.c:147 l3_interrupt_handler+0x214/0x340()
> 44000000.ocp:L3 Custom Error: MASTER MPU TARGET L4CFG (Idle): Data Access in Supervisor mode during Functional access
> ...
> [<c05f21e4>] (__irq_svc) from [<c05f1974>] (_raw_spin_unlock_irqrestore+0x34/0x44)
> [<c05f1974>] (_raw_spin_unlock_irqrestore) from [<c00914a8>] (__setup_irq+0x244/0x530)
> [<c00914a8>] (__setup_irq) from [<c00917d4>] (setup_irq+0x40/0x8c)
> [<c00917d4>] (setup_irq) from [<c0039c8c>] (omap_system_dma_probe+0x1d4/0x2b4)
> [<c0039c8c>] (omap_system_dma_probe) from [<c03b2200>] (platform_drv_probe+0x44/0xa4)
> ...
>
> We can fix this the same way omap_gpio_irq_type() is handling it.
>

I see that Linus already picked this patch but fwiw:

Acked-by: Javier Martinez Canillas <javier@dowhile0.org>

> Note that the long term solution is to change the gpio-omap driver
> to handle the banks as separate driver instances. This will allow
> us to rely on just runtime PM for tracking the bank specific state.
>

Agreed.

Best regards,
Javier