Message ID | 20150619170654.GA4937@linutronix.de (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On 6/19/2015 10:06 AM, Sebastian Andrzej Siewior wrote: > This patch converts gpio_bank.lock from a spin_lock into a > raw_spin_lock. The call path is to access this lock is always under a > raw_spin_lock, for instance > - __setup_irq() holds &desc->lock with irq off > + __irq_set_trigger() > + omap_gpio_irq_type() > > - handle_level_irq() (runs with irqs off therefore raw locks) > + mask_ack_irq() > + omap_gpio_mask_irq() > > This fixes the obvious backtrace on -RT. However the locking vs context > is not and this is not limited to -RT: > - omap_gpio_irq_type() is called with IRQ off and has an conditional > call to pm_runtime_get_sync() which may sleep. Either it may happen or > it may not happen but pm_runtime_get_sync() should not be called with > irqs off. > > - omap_gpio_debounce() is holding the lock with IRQs off. > + omap2_set_gpio_debounce() > + clk_prepare_enable() > + clk_prepare() this one might sleep. > The number of users of gpiod_set_debounce() / gpio_set_debounce() > looks low but still this is not good. > > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > --- Should be safe to do it. Acked-by: Santosh Shilimkar <ssantosh@kernel.org> -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in
On Fri, Jun 19, 2015 at 7:42 PM, santosh shilimkar <santosh.shilimkar@oracle.com> wrote: > On 6/19/2015 10:06 AM, Sebastian Andrzej Siewior wrote: >> >> This patch converts gpio_bank.lock from a spin_lock into a >> raw_spin_lock. The call path is to access this lock is always under a >> raw_spin_lock, for instance >> - __setup_irq() holds &desc->lock with irq off >> + __irq_set_trigger() >> + omap_gpio_irq_type() >> >> - handle_level_irq() (runs with irqs off therefore raw locks) >> + mask_ack_irq() >> + omap_gpio_mask_irq() >> >> This fixes the obvious backtrace on -RT. However the locking vs context >> is not and this is not limited to -RT: >> - omap_gpio_irq_type() is called with IRQ off and has an conditional >> call to pm_runtime_get_sync() which may sleep. Either it may happen or >> it may not happen but pm_runtime_get_sync() should not be called with >> irqs off. >> >> - omap_gpio_debounce() is holding the lock with IRQs off. >> + omap2_set_gpio_debounce() >> + clk_prepare_enable() >> + clk_prepare() this one might sleep. >> The number of users of gpiod_set_debounce() / gpio_set_debounce() >> looks low but still this is not good. >> >> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> >> --- > > Should be safe to do it. > Acked-by: Santosh Shilimkar <ssantosh@kernel.org> > Answered on the wrong thread, sorry for the noise. Acked-by: Javier Martinez Canillas <javier@dowhile0.org> Best regards, Javier -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in
* Javier Martinez Canillas <javier@dowhile0.org> [150619 14:57]: > On Fri, Jun 19, 2015 at 7:42 PM, santosh shilimkar > <santosh.shilimkar@oracle.com> wrote: > > On 6/19/2015 10:06 AM, Sebastian Andrzej Siewior wrote: > >> > >> This patch converts gpio_bank.lock from a spin_lock into a > >> raw_spin_lock. The call path is to access this lock is always under a > >> raw_spin_lock, for instance > >> - __setup_irq() holds &desc->lock with irq off > >> + __irq_set_trigger() > >> + omap_gpio_irq_type() > >> > >> - handle_level_irq() (runs with irqs off therefore raw locks) > >> + mask_ack_irq() > >> + omap_gpio_mask_irq() > >> > >> This fixes the obvious backtrace on -RT. However the locking vs context > >> is not and this is not limited to -RT: > >> - omap_gpio_irq_type() is called with IRQ off and has an conditional > >> call to pm_runtime_get_sync() which may sleep. Either it may happen or > >> it may not happen but pm_runtime_get_sync() should not be called with > >> irqs off. > >> > >> - omap_gpio_debounce() is holding the lock with IRQs off. > >> + omap2_set_gpio_debounce() > >> + clk_prepare_enable() > >> + clk_prepare() this one might sleep. > >> The number of users of gpiod_set_debounce() / gpio_set_debounce() > >> looks low but still this is not good. > >> > >> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > >> --- > > > > Should be safe to do it. > > Acked-by: Santosh Shilimkar <ssantosh@kernel.org> > > > > Answered on the wrong thread, sorry for the noise. > > Acked-by: Javier Martinez Canillas <javier@dowhile0.org> Seems OK but looks like this needs updating against current Linux next to apply though. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in
Hi Sebastian, All, On 06/22/2015 10:08 AM, Tony Lindgren wrote: > * Javier Martinez Canillas <javier@dowhile0.org> [150619 14:57]: >> On Fri, Jun 19, 2015 at 7:42 PM, santosh shilimkar >> <santosh.shilimkar@oracle.com> wrote: >>> On 6/19/2015 10:06 AM, Sebastian Andrzej Siewior wrote: >>>> >>>> This patch converts gpio_bank.lock from a spin_lock into a >>>> raw_spin_lock. The call path is to access this lock is always under a >>>> raw_spin_lock, for instance >>>> - __setup_irq() holds &desc->lock with irq off >>>> + __irq_set_trigger() >>>> + omap_gpio_irq_type() >>>> >>>> - handle_level_irq() (runs with irqs off therefore raw locks) >>>> + mask_ack_irq() >>>> + omap_gpio_mask_irq() >>>> >>>> This fixes the obvious backtrace on -RT. However the locking vs context >>>> is not and this is not limited to -RT: >>>> - omap_gpio_irq_type() is called with IRQ off and has an conditional >>>> call to pm_runtime_get_sync() which may sleep. Either it may happen or >>>> it may not happen but pm_runtime_get_sync() should not be called with >>>> irqs off. >>>> >>>> - omap_gpio_debounce() is holding the lock with IRQs off. >>>> + omap2_set_gpio_debounce() >>>> + clk_prepare_enable() >>>> + clk_prepare() this one might sleep. >>>> The number of users of gpiod_set_debounce() / gpio_set_debounce() >>>> looks low but still this is not good. >>>> >>>> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> >>>> --- >>> >>> Should be safe to do it. >>> Acked-by: Santosh Shilimkar <ssantosh@kernel.org> >>> >> >> Answered on the wrong thread, sorry for the noise. >> >> Acked-by: Javier Martinez Canillas <javier@dowhile0.org> > > Seems OK but looks like this needs updating against current Linux next > to apply though. > I've tested this patch on TI 3.14-rt kernel, but I still can see bunch of "inconsistent lock state" warnings related to the PM runtime (as expected and as you mentioned in commit log actually). The problem here is that OMAPs code implemented using PM runtime based approach and PM runtime is used everywhere. We don't see warnings form other drivers just because their HW IRQ handlers forced to be threaded, but that's not the case for OMAP GPIO :( PM runtime for OMAP GPIO is implemented in irq_safe manner and it works for non-RT case, but the main question here - How can we proceed for -RT case? May be you have some thought? From my side what I've tried or thought about: 1) OMAP HW IRQ handler can be transformed to threaded IRQ on -RT. Can it be acceptable? 2) I've tried to use irq_chip->irq_bus_lock/irq_bus_sync_unlock() callbacks. It helps a bit, but there are two problems: - These callbacks are not expected to fail; - PM runtime calls are still present in omap_gpio_irq_handler(). Hypothetically, they can be removed, but we'd need to ensure that OMAP GPIO HW is powered and ready to process IRQs all the time while GPIO bank IRQ is in use. Not trivial thing to do taking into account multi-SoC support, suspend, cpuidle :( [code provided just to illustrate idea] static void omap_gpio_irq_bus_lock(struct irq_data *data) { struct gpio_bank *bank = irq_data_get_irq_chip_data(data); if (!BANK_USED(bank)) pm_runtime_get_sync(bank->dev); } static void gpio_irq_bus_sync_unlock(struct irq_data *data) { struct gpio_bank *bank = irq_data_get_irq_chip_data(data); if (!BANK_USED(bank)) pm_runtime_put(bank->dev); } I would be appreciated for any comments on this (1 or 2 or ..), thanks.
On 06/30/2015 12:55 PM, Grygorii Strashko wrote: > Hi Sebastian, All, Hi Grygorii, > The problem here is that OMAPs code implemented using PM runtime based > approach and PM runtime is used everywhere. We don't see warnings form > other drivers just because their HW IRQ handlers forced to be threaded, but > that's not the case for OMAP GPIO :( Only for the case where it is used as an interrupt controller. So the primary interrupt controller is not using RPM then (or else you would have the same problem). > PM runtime for OMAP GPIO is implemented in irq_safe manner and it works for > non-RT case, but the main question here - How can we proceed for -RT case? > > May be you have some thought? If I remember it properly, you must not sleep but you do so on wakeup. At least you take spinlocks (spinlocks not raw_spinlocks). One question is why do you need (or why is it okay) to put a device to sleep (via RPM) if the device is used as an interrupt controller? From what I understand, if the GPIO controller is really sleeping you won't receive any interrupts. So I think you shouldn't put a GPIO controller to sleep if it is active. If you avoid this, there should be nothing calling you from IRQ-context on -RT. >>From my side what I've tried or thought about: > 1) OMAP HW IRQ handler can be transformed to threaded IRQ on -RT. > Can it be acceptable? It could. In theory. It would work, too. Not sure you really want this. You lose the cascaded handler that you have now. The obvious change would be that you see another IRQ used in /proc/interrupts. This is harmless :) However each time an IRQ (on the GPIO side) arrives you have a hardirq which is defered into thread context. Then it wakes another thread (the GPIO-IRQ-handler). So this adds a little of overhead on your call path. Since you won't have IRQF_NO_THREAD flag on any user, this should remain the only price you pay. Also I try not grow the -RT queue. I sent patches upstream where possible. That means for this I would like to have this addressed upstream as there is no joy carrying this patch (that means please don't do a -RT only fix). > 2) I've tried to use irq_chip->irq_bus_lock/irq_bus_sync_unlock() callbacks. > It helps a bit, but there are two problems: > - These callbacks are not expected to fail; > - PM runtime calls are still present in omap_gpio_irq_handler(). > Hypothetically, they can be removed, but we'd need to ensure that OMAP GPIO HW > is powered and ready to process IRQs all the time while GPIO bank IRQ is in use. > Not trivial thing to do taking into account multi-SoC support, suspend, cpuidle :( What I still don't understand how can a GPIO create an interrupt if it is sleeping? I know CAN and a few others peripherals can do such things. Is this also the case for the GPIO? Or is the problem more that the CPU can't properly enter suspend/ cpuidle if the GPIO bank remains active? > [code provided just to illustrate idea] > static void omap_gpio_irq_bus_lock(struct irq_data *data) > { > struct gpio_bank *bank = irq_data_get_irq_chip_data(data); > > if (!BANK_USED(bank)) > pm_runtime_get_sync(bank->dev); > } I don't really like the !BANK_USED(bank) check because it is either in use or not. Things like that may hide the pm_runtime_get_sync() call and explode with debug-off. > I would be appreciated for any comments on this (1 or 2 or ..), thanks. Another way would be to turn the lock into a raw lock so it won't block on -RT. However each time you hold the lock in process context you block a possible -RT task from becoming run-able. Also this could include larger hacker across the RPM code depending on how this lock issue is solved. Either way: the longer you hold the lock the longer you block the -RT task and the important part is the worst-case scenario. Sebastian -- 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
* Sebastian Andrzej Siewior <bigeasy@linutronix.de> [150630 09:39]: > On 06/30/2015 12:55 PM, Grygorii Strashko wrote: > > > > May be you have some thought? > > If I remember it properly, you must not sleep but you do so on wakeup. > At least you take spinlocks (spinlocks not raw_spinlocks). One question > is why do you need (or why is it okay) to put a device to sleep (via > RPM) if the device is used as an interrupt controller? From what I > understand, if the GPIO controller is really sleeping you won't receive > any interrupts. Not quite so. In many cases the GPIO interrupt just provides a wake-up event for a device that's in deeper idle state. Such as a touch screen interrupt. And the interrupt works just fine, just it's latency can be really bad :) The first GPIO bank is always on, the others need to use dedicated IO chain wake-up interrupts with pinctrl-single. > So I think you shouldn't put a GPIO controller to sleep if it is > active. If you avoid this, there should be nothing calling you from > IRQ-context on -RT. That seems like a workaround though.. It seems we're better off making things work with threaded IRQ (and removing runtime PM from the IRQ path). For the GPIO banks requiring the use of a dedicated wakeirq, his is mostly already done with recent commit 4990d4fe327b ("PM / Wakeirq: Add automated device wake IRQ handling") where the wakeirq handling is a completely separate threaded IRQ handler. The obvious solution seems to make GPIO interrupt handling all happen in a bottom half. That should allow removing the pm_runtime_irq_safe, which is not good to have in general. > >>From my side what I've tried or thought about: > > 1) OMAP HW IRQ handler can be transformed to threaded IRQ on -RT. > > Can it be acceptable? > > It could. In theory. It would work, too. Not sure you really want this. > You lose the cascaded handler that you have now. The obvious change > would be that you see another IRQ used in /proc/interrupts. This is > harmless :) However each time an IRQ (on the GPIO side) arrives you > have a hardirq which is defered into thread context. Then it wakes > another thread (the GPIO-IRQ-handler). So this adds a little of > overhead on your call path. Since you won't have IRQF_NO_THREAD flag on > any user, this should remain the only price you pay. This should be OK for most cases as the GPIO interrupt devices are typically on some external bus like I2C or GPMC. The hurting case would be bitbanging GPIO devices, like the CBUS I2C driver. If something faster is needed, we could allow configuring GPIO banks in a different way where the non-threaded banks don't allow idling the bank? Regards, Tony -- 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
* Tony Lindgren <tony@atomide.com> [150701 00:34]: > > This should be OK for most cases as the GPIO interrupt devices are > typically on some external bus like I2C or GPMC. The hurting case > would be bitbanging GPIO devices, like the CBUS I2C driver. Thinking about it the CBUS I2C driver does not need interrupts for the bitbanging part either :) So I think we can just make it threaded IRQ for GPIO. Tony -- 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
Hi Tony, Sebastian, On 07/01/2015 02:29 PM, Tony Lindgren wrote: > * Tony Lindgren <tony@atomide.com> [150701 00:34]: >> >> This should be OK for most cases as the GPIO interrupt devices are >> typically on some external bus like I2C or GPMC. The hurting case >> would be bitbanging GPIO devices, like the CBUS I2C driver. > > Thinking about it the CBUS I2C driver does not need interrupts > for the bitbanging part either :) > > So I think we can just make it threaded IRQ for GPIO. > Thanks for your comments, I'll try to do patch to make GPIO IRQ threaded (may be as option).
--- a/drivers/gpio/gpio-omap.c +++ b/drivers/gpio/gpio-omap.c @@ -57,7 +57,7 @@ struct gpio_bank { u32 saved_datain; u32 level_mask; u32 toggle_mask; - spinlock_t lock; + raw_spinlock_t lock; struct gpio_chip chip; struct clk *dbck; u32 mod_usage; @@ -498,14 +498,14 @@ static int omap_gpio_irq_type(struct irq (type & (IRQ_TYPE_LEVEL_LOW|IRQ_TYPE_LEVEL_HIGH))) return -EINVAL; - spin_lock_irqsave(&bank->lock, flags); + raw_spin_lock_irqsave(&bank->lock, flags); retval = omap_set_gpio_triggering(bank, offset, type); omap_gpio_init_irq(bank, offset); if (!omap_gpio_is_input(bank, offset)) { - spin_unlock_irqrestore(&bank->lock, flags); + raw_spin_unlock_irqrestore(&bank->lock, flags); return -EINVAL; } - spin_unlock_irqrestore(&bank->lock, flags); + raw_spin_unlock_irqrestore(&bank->lock, flags); if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH)) __irq_set_handler_locked(d->irq, handle_level_irq); @@ -626,14 +626,14 @@ static int omap_set_gpio_wakeup(struct g return -EINVAL; } - spin_lock_irqsave(&bank->lock, flags); + raw_spin_lock_irqsave(&bank->lock, flags); if (enable) bank->context.wake_en |= gpio_bit; else bank->context.wake_en &= ~gpio_bit; writel_relaxed(bank->context.wake_en, bank->base + bank->regs->wkup_en); - spin_unlock_irqrestore(&bank->lock, flags); + raw_spin_unlock_irqrestore(&bank->lock, flags); return 0; } @@ -668,7 +668,7 @@ static int omap_gpio_request(struct gpio if (!BANK_USED(bank)) pm_runtime_get_sync(bank->dev); - spin_lock_irqsave(&bank->lock, flags); + raw_spin_lock_irqsave(&bank->lock, flags); /* Set trigger to none. You need to enable the desired trigger with * request_irq() or set_irq_type(). Only do this if the IRQ line has * not already been requested. @@ -678,7 +678,7 @@ static int omap_gpio_request(struct gpio omap_enable_gpio_module(bank, offset); } bank->mod_usage |= BIT(offset); - spin_unlock_irqrestore(&bank->lock, flags); + raw_spin_unlock_irqrestore(&bank->lock, flags); return 0; } @@ -688,11 +688,11 @@ static void omap_gpio_free(struct gpio_c struct gpio_bank *bank = container_of(chip, struct gpio_bank, chip); unsigned long flags; - spin_lock_irqsave(&bank->lock, flags); + raw_spin_lock_irqsave(&bank->lock, flags); bank->mod_usage &= ~(BIT(offset)); omap_disable_gpio_module(bank, offset); omap_reset_gpio(bank, offset); - spin_unlock_irqrestore(&bank->lock, flags); + raw_spin_unlock_irqrestore(&bank->lock, flags); /* * If this is the last gpio to be freed in the bank, @@ -794,9 +794,9 @@ static unsigned int omap_gpio_irq_startu if (!BANK_USED(bank)) pm_runtime_get_sync(bank->dev); - spin_lock_irqsave(&bank->lock, flags); + raw_spin_lock_irqsave(&bank->lock, flags); omap_gpio_init_irq(bank, offset); - spin_unlock_irqrestore(&bank->lock, flags); + raw_spin_unlock_irqrestore(&bank->lock, flags); omap_gpio_unmask_irq(d); return 0; @@ -808,11 +808,11 @@ static void omap_gpio_irq_shutdown(struc unsigned long flags; unsigned offset = d->hwirq; - spin_lock_irqsave(&bank->lock, flags); + raw_spin_lock_irqsave(&bank->lock, flags); bank->irq_usage &= ~(BIT(offset)); omap_disable_gpio_module(bank, offset); omap_reset_gpio(bank, offset); - spin_unlock_irqrestore(&bank->lock, flags); + raw_spin_unlock_irqrestore(&bank->lock, flags); /* * If this is the last IRQ to be freed in the bank, @@ -836,10 +836,10 @@ static void omap_gpio_mask_irq(struct ir unsigned offset = d->hwirq; unsigned long flags; - spin_lock_irqsave(&bank->lock, flags); + raw_spin_lock_irqsave(&bank->lock, flags); omap_set_gpio_irqenable(bank, offset, 0); omap_set_gpio_triggering(bank, offset, IRQ_TYPE_NONE); - spin_unlock_irqrestore(&bank->lock, flags); + raw_spin_unlock_irqrestore(&bank->lock, flags); } static void omap_gpio_unmask_irq(struct irq_data *d) @@ -849,7 +849,7 @@ static void omap_gpio_unmask_irq(struct u32 trigger = irqd_get_trigger_type(d); unsigned long flags; - spin_lock_irqsave(&bank->lock, flags); + raw_spin_lock_irqsave(&bank->lock, flags); if (trigger) omap_set_gpio_triggering(bank, offset, trigger); @@ -861,7 +861,7 @@ static void omap_gpio_unmask_irq(struct } omap_set_gpio_irqenable(bank, offset, 1); - spin_unlock_irqrestore(&bank->lock, flags); + raw_spin_unlock_irqrestore(&bank->lock, flags); } /*---------------------------------------------------------------------*/ @@ -874,9 +874,9 @@ static int omap_mpuio_suspend_noirq(stru OMAP_MPUIO_GPIO_MASKIT / bank->stride; unsigned long flags; - spin_lock_irqsave(&bank->lock, flags); + raw_spin_lock_irqsave(&bank->lock, flags); writel_relaxed(0xffff & ~bank->context.wake_en, mask_reg); - spin_unlock_irqrestore(&bank->lock, flags); + raw_spin_unlock_irqrestore(&bank->lock, flags); return 0; } @@ -889,9 +889,9 @@ static int omap_mpuio_resume_noirq(struc OMAP_MPUIO_GPIO_MASKIT / bank->stride; unsigned long flags; - spin_lock_irqsave(&bank->lock, flags); + raw_spin_lock_irqsave(&bank->lock, flags); writel_relaxed(bank->context.wake_en, mask_reg); - spin_unlock_irqrestore(&bank->lock, flags); + raw_spin_unlock_irqrestore(&bank->lock, flags); return 0; } @@ -937,9 +937,9 @@ static int omap_gpio_get_direction(struc bank = container_of(chip, struct gpio_bank, chip); reg = bank->base + bank->regs->direction; - spin_lock_irqsave(&bank->lock, flags); + raw_spin_lock_irqsave(&bank->lock, flags); dir = !!(readl_relaxed(reg) & BIT(offset)); - spin_unlock_irqrestore(&bank->lock, flags); + raw_spin_unlock_irqrestore(&bank->lock, flags); return dir; } @@ -949,9 +949,9 @@ static int omap_gpio_input(struct gpio_c unsigned long flags; bank = container_of(chip, struct gpio_bank, chip); - spin_lock_irqsave(&bank->lock, flags); + raw_spin_lock_irqsave(&bank->lock, flags); omap_set_gpio_direction(bank, offset, 1); - spin_unlock_irqrestore(&bank->lock, flags); + raw_spin_unlock_irqrestore(&bank->lock, flags); return 0; } @@ -973,10 +973,10 @@ static int omap_gpio_output(struct gpio_ unsigned long flags; bank = container_of(chip, struct gpio_bank, chip); - spin_lock_irqsave(&bank->lock, flags); + raw_spin_lock_irqsave(&bank->lock, flags); bank->set_dataout(bank, offset, value); omap_set_gpio_direction(bank, offset, 0); - spin_unlock_irqrestore(&bank->lock, flags); + raw_spin_unlock_irqrestore(&bank->lock, flags); return 0; } @@ -988,9 +988,9 @@ static int omap_gpio_debounce(struct gpi bank = container_of(chip, struct gpio_bank, chip); - spin_lock_irqsave(&bank->lock, flags); + raw_spin_lock_irqsave(&bank->lock, flags); omap2_set_gpio_debounce(bank, offset, debounce); - spin_unlock_irqrestore(&bank->lock, flags); + raw_spin_unlock_irqrestore(&bank->lock, flags); return 0; } @@ -1001,9 +1001,9 @@ static void omap_gpio_set(struct gpio_ch unsigned long flags; bank = container_of(chip, struct gpio_bank, chip); - spin_lock_irqsave(&bank->lock, flags); + raw_spin_lock_irqsave(&bank->lock, flags); bank->set_dataout(bank, offset, value); - spin_unlock_irqrestore(&bank->lock, flags); + raw_spin_unlock_irqrestore(&bank->lock, flags); } /*---------------------------------------------------------------------*/ @@ -1199,7 +1199,7 @@ static int omap_gpio_probe(struct platfo else bank->set_dataout = omap_set_gpio_dataout_mask; - spin_lock_init(&bank->lock); + raw_spin_lock_init(&bank->lock); /* Static mapping, never released */ res = platform_get_resource(pdev, IORESOURCE_MEM, 0); @@ -1246,7 +1246,7 @@ static int omap_gpio_runtime_suspend(str unsigned long flags; u32 wake_low, wake_hi; - spin_lock_irqsave(&bank->lock, flags); + raw_spin_lock_irqsave(&bank->lock, flags); /* * Only edges can generate a wakeup event to the PRCM. @@ -1299,7 +1299,7 @@ static int omap_gpio_runtime_suspend(str bank->get_context_loss_count(bank->dev); omap_gpio_dbck_disable(bank); - spin_unlock_irqrestore(&bank->lock, flags); + raw_spin_unlock_irqrestore(&bank->lock, flags); return 0; } @@ -1314,7 +1314,7 @@ static int omap_gpio_runtime_resume(stru unsigned long flags; int c; - spin_lock_irqsave(&bank->lock, flags); + raw_spin_lock_irqsave(&bank->lock, flags); /* * On the first resume during the probe, the context has not @@ -1350,14 +1350,14 @@ static int omap_gpio_runtime_resume(stru if (c != bank->context_loss_count) { omap_gpio_restore_context(bank); } else { - spin_unlock_irqrestore(&bank->lock, flags); + raw_spin_unlock_irqrestore(&bank->lock, flags); return 0; } } } if (!bank->workaround_enabled) { - spin_unlock_irqrestore(&bank->lock, flags); + raw_spin_unlock_irqrestore(&bank->lock, flags); return 0; } @@ -1412,7 +1412,7 @@ static int omap_gpio_runtime_resume(stru } bank->workaround_enabled = false; - spin_unlock_irqrestore(&bank->lock, flags); + raw_spin_unlock_irqrestore(&bank->lock, flags); return 0; }
This patch converts gpio_bank.lock from a spin_lock into a raw_spin_lock. The call path is to access this lock is always under a raw_spin_lock, for instance - __setup_irq() holds &desc->lock with irq off + __irq_set_trigger() + omap_gpio_irq_type() - handle_level_irq() (runs with irqs off therefore raw locks) + mask_ack_irq() + omap_gpio_mask_irq() This fixes the obvious backtrace on -RT. However the locking vs context is not and this is not limited to -RT: - omap_gpio_irq_type() is called with IRQ off and has an conditional call to pm_runtime_get_sync() which may sleep. Either it may happen or it may not happen but pm_runtime_get_sync() should not be called with irqs off. - omap_gpio_debounce() is holding the lock with IRQs off. + omap2_set_gpio_debounce() + clk_prepare_enable() + clk_prepare() this one might sleep. The number of users of gpiod_set_debounce() / gpio_set_debounce() looks low but still this is not good. Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> --- drivers/gpio/gpio-omap.c | 78 +++++++++++++++++++++++------------------------ 1 file changed, 39 insertions(+), 39 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in