Re: [PATCH] gpio: omap: use raw locks for locking
On Mon, 27 Jul 2015, Sebastian Andrzej Siewior wrote: On 07/27/2015 02:50 PM, Linus Walleij wrote: Patch applied. thanks. Now this question appear in my head: Is drivers/gpio full of stuff that will not work with the -RT kernel, and is this a change that should be done mutatis mutandis on all the GPIO drivers? I described two call paths where you need a rawlock_t. If your gpio driver uses irq_chip_generic then you a rawlock here and things should be fine. In general: If your gpio controller acts as an interrupts controller (that is via chained handler) then you need the raw-locks if you need any locking (if you have a write 1 to mask/unmask/enable/disable register then you probably don't need any locking here at all). If the gpio controller does not act as an interrupt controller than the spinlock_t type should be enough. If your gpio-interrupt controller requests its interrupt via requested_threaded_irq() then it should do handle_nested_irq() and a mutex is probably used for locking. Using request_irq() with 0 flags is kind of broken. It works in IRQ-context and delivers the interrupts with generic_handle_irq() and this one passes it the handler (like handle_edge_irq() / handle_level_irq()) which takes a raw_lock. Now, if you boot the vanilla kernel with threadedirq then the irq-handler runs in threaded context and you can't take a spinlock here anymore. So I think you should use here IRQF_NO_THREAD here (and the raw lock type). I added tglx on Cc: to back up because it is Monday. Indeed it was monday. It's an RT only problem. On mainline spinlock resovles to raw_spinlock. Now there is the story what breaks in RT: 1) irq controller specific locks which are taken in the irq chip callbacks need to be raw_spinlocks because these functions are called with irq_desc-lock helds and interrupts disabled. If that irq chip lock is not raw you rightfully get a backtrace. raw_spinlock_irq(desc-lock); chip-callback() spin_lock(chip_private_lock); might_sleep() triggers 2) locks which are taken in chained interrupt handlers need to be raw on RT. These handlers run in hard interrupt context so again might_sleep() rightfully complains. Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gpio: omap: use raw locks for locking
On Tue, Jul 21, 2015 at 6:26 PM, Sebastian Andrzej Siewior bige...@linutronix.de 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. Acked-by: Javier Martinez Canillas jav...@dowhile0.org Acked-by: Santosh Shilimkar ssant...@kernel.org Signed-off-by: Sebastian Andrzej Siewior bige...@linutronix.de Patch applied. Now this question appear in my head: Is drivers/gpio full of stuff that will not work with the -RT kernel, and is this a change that should be done mutatis mutandis on all the GPIO drivers? Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gpio: omap: use raw locks for locking
On 07/27/2015 02:50 PM, Linus Walleij wrote: Patch applied. thanks. Now this question appear in my head: Is drivers/gpio full of stuff that will not work with the -RT kernel, and is this a change that should be done mutatis mutandis on all the GPIO drivers? I described two call paths where you need a rawlock_t. If your gpio driver uses irq_chip_generic then you a rawlock here and things should be fine. In general: If your gpio controller acts as an interrupts controller (that is via chained handler) then you need the raw-locks if you need any locking (if you have a write 1 to mask/unmask/enable/disable register then you probably don't need any locking here at all). If the gpio controller does not act as an interrupt controller than the spinlock_t type should be enough. If your gpio-interrupt controller requests its interrupt via requested_threaded_irq() then it should do handle_nested_irq() and a mutex is probably used for locking. Using request_irq() with 0 flags is kind of broken. It works in IRQ-context and delivers the interrupts with generic_handle_irq() and this one passes it the handler (like handle_edge_irq() / handle_level_irq()) which takes a raw_lock. Now, if you boot the vanilla kernel with threadedirq then the irq-handler runs in threaded context and you can't take a spinlock here anymore. So I think you should use here IRQF_NO_THREAD here (and the raw lock type). I added tglx on Cc: to back up because it is Monday. Yours, Linus Walleij Sebastian -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] gpio: omap: use raw locks for locking
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.
Acked-by: Javier Martinez Canillas jav...@dowhile0.org
Acked-by: Santosh Shilimkar ssant...@kernel.org
Signed-off-by: Sebastian Andrzej Siewior bige...@linutronix.de
---
drivers/gpio/gpio-omap.c | 82
1 file changed, 41 insertions(+), 41 deletions(-)
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 61a731ff9a07..f56de8de9c55 100644
--- 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,19 +498,19 @@ static int omap_gpio_irq_type(struct irq_data *d,
unsigned type)
if (!BANK_USED(bank))
pm_runtime_get_sync(bank-dev);
- spin_lock_irqsave(bank-lock, flags);
+ raw_spin_lock_irqsave(bank-lock, flags);
retval = omap_set_gpio_triggering(bank, offset, type);
if (retval) {
- spin_unlock_irqrestore(bank-lock, flags);
+ raw_spin_unlock_irqrestore(bank-lock, flags);
goto error;
}
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);
retval = -EINVAL;
goto error;
}
- 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);
@@ -636,14 +636,14 @@ static int omap_set_gpio_wakeup(struct gpio_bank *bank,
unsigned offset,
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;
}
@@ -669,10 +669,10 @@ static int omap_gpio_request(struct gpio_chip *chip,
unsigned offset)
if (!BANK_USED(bank))
pm_runtime_get_sync(bank-dev);
- spin_lock_irqsave(bank-lock, flags);
+ raw_spin_lock_irqsave(bank-lock, flags);
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;
}
@@ -682,14 +682,14 @@ static void omap_gpio_free(struct gpio_chip *chip,
unsigned offset)
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));
if (!LINE_USED(bank-irq_usage, offset)) {
omap_set_gpio_direction(bank, offset, 1);
omap_clear_gpio_debounce(bank, offset);
}
omap_disable_gpio_module(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,
@@ -791,7 +791,7 @@ static unsigned int omap_gpio_irq_startup(struct irq_data
*d)
if (!BANK_USED(bank))
pm_runtime_get_sync(bank-dev);
- spin_lock_irqsave(bank-lock, flags);
+ raw_spin_lock_irqsave(bank-lock, flags);
if (!LINE_USED(bank-mod_usage, offset))
omap_set_gpio_direction(bank, offset, 1);
@@ -800,12 +800,12 @@ static unsigned int omap_gpio_irq_startup(struct irq_data
*d)
omap_enable_gpio_module(bank, offset);
Re: [PATCH] gpio: omap: use raw locks for locking
On 06/30/2015 08:45 AM, Linus Walleij wrote: This doesn't apply to the current development tree. it has been pointed out to me. Can you rebase this patch in Torvalds' HEAD, add Javier's ACK and resend? I will do it once I'm done roasted :) Please put me on the To: line, I have no time to read all mail that I'm only CC on. noted. This might been the problem while it hasn't been applied the first time I posted it. Yours, Linus Walleij Sebastian -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gpio: omap: use raw locks for locking
On Thu, Feb 12, 2015 at 5:10 PM, Sebastian Andrzej Siewior bige...@linutronix.de wrote: This patch converts gpio_bank.lock from a spin_lock into a raw_spin_lock. The call path 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 I noticed two cases where it looks wrong 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 in an atomic section. - 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 bige...@linutronix.de This doesn't apply to the current development tree. Can you rebase this patch in Torvalds' HEAD, add Javier's ACK and resend? Please put me on the To: line, I have no time to read all mail that I'm only CC on. Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gpio: omap: use raw locks for locking
Hello Sebastian, On Thu, Feb 12, 2015 at 5:10 PM, Sebastian Andrzej Siewior bige...@linutronix.de wrote: This patch converts gpio_bank.lock from a spin_lock into a raw_spin_lock. The call path 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 I noticed two cases where it looks wrong 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 in an atomic section. - 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 bige...@linutronix.de --- I had already acked the first time you posted this [0] but you didn't carry the tag so here goes again: Acked-by: Javier Martinez Canillas jav...@dowhile0.org Best regards, Javier [0]: https://patchwork.ozlabs.org/patch/439249/ -- To unsubscribe from this list: send the line unsubscribe linux-omap in
Re: [PATCH] gpio: omap: use raw locks for locking
On 02/16/2015 09:54 AM, Javier Martinez Canillas wrote: Hello Sebastian, Hello Javier, Right, those are bugs regardless of PREEMPT_RT or not as you said. I'll add it to my TODO list, thanks for finding those. Thanks. Acked-by: Javier Martinez Canillas jav...@dowhile0.org Best regards, Javier Sebastian -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gpio: omap: use raw locks for locking
Hello Sebastian, Thanks a lot for your patch. On Thu, Feb 12, 2015 at 5:10 PM, Sebastian Andrzej Siewior bige...@linutronix.de wrote: This patch converts gpio_bank.lock from a spin_lock into a raw_spin_lock. The call path 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() Agreed that raw_spin_lock should be used instead of spin_lock since afaiu those are converted to a rt-mutex in PREEMPT_RT and so they might sleep. This fixes the obvious backtrace on -RT. However I noticed two cases where it looks wrong 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 in an atomic section. - 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. Right, those are bugs regardless of PREEMPT_RT or not as you said. I'll add it to my TODO list, thanks for finding those. Signed-off-by: Sebastian Andrzej Siewior bige...@linutronix.de --- drivers/gpio/gpio-omap.c | 78 1 file changed, 39 insertions(+), 39 deletions(-) Acked-by: Javier Martinez Canillas jav...@dowhile0.org Best regards, Javier -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] gpio: omap: use raw locks for locking
This patch converts gpio_bank.lock from a spin_lock into a
raw_spin_lock. The call path 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 I noticed two cases
where it looks wrong 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 in an
atomic section.
- 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 bige...@linutronix.de
---
drivers/gpio/gpio-omap.c | 78
1 file changed, 39 insertions(+), 39 deletions(-)
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index f476ae2eb0b3..27e835f4c39d 100644
--- 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;
@@ -515,15 +515,15 @@ static int omap_gpio_irq_type(struct irq_data *d,
unsigned type)
(type (IRQ_TYPE_LEVEL_LOW|IRQ_TYPE_LEVEL_HIGH)))
return -EINVAL;
- spin_lock_irqsave(bank-lock, flags);
+ raw_spin_lock_irqsave(bank-lock, flags);
offset = GPIO_INDEX(bank, gpio);
retval = omap_set_gpio_triggering(bank, offset, type);
omap_gpio_init_irq(bank, gpio, offset);
if (!omap_gpio_is_input(bank, BIT(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);
@@ -641,14 +641,14 @@ static int omap_set_gpio_wakeup(struct gpio_bank *bank,
int gpio, int enable)
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;
}
@@ -683,7 +683,7 @@ static int omap_gpio_request(struct gpio_chip *chip,
unsigned offset)
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.
@@ -693,7 +693,7 @@ static int omap_gpio_request(struct gpio_chip *chip,
unsigned offset)
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;
}
@@ -703,11 +703,11 @@ static void omap_gpio_free(struct gpio_chip *chip,
unsigned offset)
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, bank-chip.base + 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,
@@ -810,9 +810,9 @@ static unsigned int omap_gpio_irq_startup(struct irq_data
*d)
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, gpio, offset);
- spin_unlock_irqrestore(bank-lock, flags);
+ raw_spin_unlock_irqrestore(bank-lock, flags);
omap_gpio_unmask_irq(d);
return 0;
@@ -825,12 +825,12 @@ static void omap_gpio_irq_shutdown(struct irq_data *d)
