| Message ID | 1421448650-15904-1-git-send-email-tony@atomide.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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>
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* 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 > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
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(-)