gpio: omap: Fix bad device access with setup_irq()

PATCH New Tony Lindgren 6 participants 8 messages Jan 16, 2015 – Jan 21, 2015

Subsystems (2)

GPIO OMAP GPIO driver

lore

Share & tools

lore

Tony Lindgren

author

2015-01-16 22:50 UTC #0 lore raw

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

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 <[email protected]>

Cc Felipe Balbi <[email protected]>

Cc Javier Martinez Canillas <[email protected]>

Cc Kevin Hilman <[email protected]>

Cc Linus Walleij <[email protected]>

Cc Russell King - ARM Linux <[email protected]>

Cc Santosh Shilimkar <[email protected]>

Signed-off-by Tony Lindgren <[email protected]>

---
 drivers/gpio/gpio-omap.c | 39 +++++++++++++++++++++++++++++++++------
 1 file changed, 33 insertions(+), 6 deletions(-)

drivers/gpio/gpio-omap.c +33 -6

index 30646cf..f476ae2 100644
@@ -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,

Signature · 1 line

2.1.4

Felipe Balbi

2015-01-16 22:55 UTC #1 lore raw

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

Quoted 21 lines · 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.

Quoted 8 lines

Reported-by Russell King <[email protected]>
Cc Felipe Balbi <[email protected]>
Cc Javier Martinez Canillas <[email protected]>
Cc Kevin Hilman <[email protected]>
Cc Linus Walleij <[email protected]>
Cc Russell King - ARM Linux <[email protected]>
Cc Santosh Shilimkar <[email protected]>
Signed-off-by Tony Lindgren <[email protected]>

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 <[email protected]>

Signature · 8 lines

balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150116/d7a4db5f/attachment.sig>

santosh shilimkar

2015-01-17 00:19 UTC #2 lore raw

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

In reply to Tony Lindgren

Quoted 5 lines · 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():

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]

Quoted 24 lines

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 <[email protected]>
Cc Felipe Balbi <[email protected]>
Cc Javier Martinez Canillas <[email protected]>
Cc Kevin Hilman <[email protected]>
Cc Linus Walleij <[email protected]>
Cc Russell King - ARM Linux <[email protected]>
Cc Santosh Shilimkar <[email protected]>
Signed-off-by Tony Lindgren <[email protected]>

Quoted 4 lines

---
  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

Tony Lindgren

author

2015-01-17 01:00 UTC #3 lore raw

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

Quoted 9 lines · santosh shilimkar

Quoted 5 lines · 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():
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.

Quoted 31 lines

One old thread on possibly similar issue is here[1]
Quoted 24 lines
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 <[email protected]>
Cc Felipe Balbi <[email protected]>
Cc Javier Martinez Canillas <[email protected]>
Cc Kevin Hilman <[email protected]>
Cc Linus Walleij <[email protected]>
Cc Russell King - ARM Linux <[email protected]>
Cc Santosh Shilimkar <[email protected]>
Signed-off-by Tony Lindgren <[email protected]>
Quoted 4 lines
---
 drivers/gpio/gpio-omap.c | 39 +++++++++++++++++++++++++++++++++------
 1 file changed, 33 insertions(+), 6 deletions(-)

Quoted 3 lines

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

Quoted 2 lines

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

santosh.shilimkar at oracle.com

2015-01-19 19:05 UTC #4 lore raw

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

Quoted 18 lines · Tony Lindgren

Quoted 9 lines · santosh shilimkar
Quoted 5 lines · 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():
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:

Quoted 8 lines

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

Russell King - ARM Linux

2015-01-17 10:10 UTC #5 lore raw

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

In reply to santosh shilimkar

Quoted 9 lines · santosh shilimkar

Quoted 5 lines · 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():
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.

Signature · 2 lines

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

Linus Walleij

2015-01-21 16:41 UTC #6 lore raw

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

In reply to Tony Lindgren

Quoted 21 lines · 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.

Quoted 8 lines

Reported-by Russell King <[email protected]>
Cc Felipe Balbi <[email protected]>
Cc Javier Martinez Canillas <[email protected]>
Cc Kevin Hilman <[email protected]>
Cc Linus Walleij <[email protected]>
Cc Russell King - ARM Linux <[email protected]>
Cc Santosh Shilimkar <[email protected]>
Signed-off-by Tony Lindgren <[email protected]>

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

Yours,
Linus Walleij

Javier Martinez Canillas

2015-01-21 21:39 UTC #7 lore raw

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

In reply to Tony Lindgren

Hello Tony,

Quoted 15 lines · 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)
...

Quoted 2 lines

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 <[email protected]>

Quoted 4 lines

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