* [PATCH] gpio: omap: use raw locks for locking
@ 2015-07-21 16:26 Sebastian Andrzej Siewior
2015-07-27 12:50 ` Linus Walleij
0 siblings, 1 reply; 10+ messages in thread
From: Sebastian Andrzej Siewior @ 2015-07-21 16:26 UTC (permalink / raw)
To: linus.walleij
Cc: javier, linux-gpio, ssantosh, tony, khilman, gnurou, linux-omap,
Sebastian Andrzej Siewior
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 <javier@dowhile0.org>
Acked-by: Santosh Shilimkar <ssantosh@kernel.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@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);
bank->irq_usage |= BIT(offset);
- spin_unlock_irqrestore(&bank->lock, flags);
+ raw_spin_unlock_irqrestore(&bank->lock, flags);
omap_gpio_unmask_irq(d);
return 0;
err:
- spin_unlock_irqrestore(&bank->lock, flags);
+ raw_spin_unlock_irqrestore(&bank->lock, flags);
if (!BANK_USED(bank))
pm_runtime_put(bank->dev);
return -EINVAL;
@@ -817,7 +817,7 @@ static void omap_gpio_irq_shutdown(struct irq_data *d)
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_set_gpio_irqenable(bank, offset, 0);
omap_clear_gpio_irqstatus(bank, offset);
@@ -825,7 +825,7 @@ static void omap_gpio_irq_shutdown(struct irq_data *d)
if (!LINE_USED(bank->mod_usage, offset))
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 IRQ to be freed in the bank,
@@ -849,10 +849,10 @@ static void omap_gpio_mask_irq(struct irq_data *d)
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)
@@ -862,7 +862,7 @@ static void omap_gpio_unmask_irq(struct irq_data *d)
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);
@@ -874,7 +874,7 @@ static void omap_gpio_unmask_irq(struct irq_data *d)
}
omap_set_gpio_irqenable(bank, offset, 1);
- spin_unlock_irqrestore(&bank->lock, flags);
+ raw_spin_unlock_irqrestore(&bank->lock, flags);
}
/*---------------------------------------------------------------------*/
@@ -887,9 +887,9 @@ static int omap_mpuio_suspend_noirq(struct device *dev)
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;
}
@@ -902,9 +902,9 @@ static int omap_mpuio_resume_noirq(struct device *dev)
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;
}
@@ -950,9 +950,9 @@ static int omap_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
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;
}
@@ -962,9 +962,9 @@ static int omap_gpio_input(struct gpio_chip *chip, unsigned offset)
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;
}
@@ -986,10 +986,10 @@ static int omap_gpio_output(struct gpio_chip *chip, unsigned offset, int value)
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;
}
@@ -1001,9 +1001,9 @@ static int omap_gpio_debounce(struct gpio_chip *chip, unsigned offset,
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;
}
@@ -1014,9 +1014,9 @@ static void omap_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
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);
}
/*---------------------------------------------------------------------*/
@@ -1213,7 +1213,7 @@ static int omap_gpio_probe(struct platform_device *pdev)
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);
@@ -1271,7 +1271,7 @@ static int omap_gpio_runtime_suspend(struct device *dev)
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.
@@ -1324,7 +1324,7 @@ static int omap_gpio_runtime_suspend(struct device *dev)
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;
}
@@ -1339,7 +1339,7 @@ static int omap_gpio_runtime_resume(struct device *dev)
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
@@ -1375,14 +1375,14 @@ static int omap_gpio_runtime_resume(struct device *dev)
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;
}
@@ -1437,7 +1437,7 @@ static int omap_gpio_runtime_resume(struct device *dev)
}
bank->workaround_enabled = false;
- spin_unlock_irqrestore(&bank->lock, flags);
+ raw_spin_unlock_irqrestore(&bank->lock, flags);
return 0;
}
--
2.1.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] gpio: omap: use raw locks for locking
2015-07-21 16:26 [PATCH] gpio: omap: use raw locks for locking Sebastian Andrzej Siewior
@ 2015-07-27 12:50 ` Linus Walleij
2015-07-27 13:25 ` Sebastian Andrzej Siewior
0 siblings, 1 reply; 10+ messages in thread
From: Linus Walleij @ 2015-07-27 12:50 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Javier Martinez Canillas, linux-gpio, Santosh Shilimkar,
Tony Lindgren, ext Kevin Hilman, Alexandre Courbot, Linux-OMAP
On Tue, Jul 21, 2015 at 6:26 PM, Sebastian Andrzej Siewior
<bigeasy@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 <javier@dowhile0.org>
> Acked-by: Santosh Shilimkar <ssantosh@kernel.org>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] gpio: omap: use raw locks for locking
2015-07-27 12:50 ` Linus Walleij
@ 2015-07-27 13:25 ` Sebastian Andrzej Siewior
2015-07-28 12:22 ` Thomas Gleixner
0 siblings, 1 reply; 10+ messages in thread
From: Sebastian Andrzej Siewior @ 2015-07-27 13:25 UTC (permalink / raw)
To: Linus Walleij
Cc: Javier Martinez Canillas, linux-gpio, Santosh Shilimkar,
Tony Lindgren, ext Kevin Hilman, Alexandre Courbot, Linux-OMAP,
'Thomas Gleixner'
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] gpio: omap: use raw locks for locking
2015-07-27 13:25 ` Sebastian Andrzej Siewior
@ 2015-07-28 12:22 ` Thomas Gleixner
0 siblings, 0 replies; 10+ messages in thread
From: Thomas Gleixner @ 2015-07-28 12:22 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Linus Walleij, Javier Martinez Canillas, linux-gpio,
Santosh Shilimkar, Tony Lindgren, ext Kevin Hilman,
Alexandre Courbot, Linux-OMAP
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] gpio: omap: use raw locks for locking
@ 2015-02-12 16:10 Sebastian Andrzej Siewior
2015-02-16 8:54 ` Javier Martinez Canillas
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Sebastian Andrzej Siewior @ 2015-02-12 16:10 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: Santosh Shilimkar, Kevin Hilman, Linus Walleij,
Alexandre Courbot, linux-omap, linux-gpio,
Sebastian Andrzej Siewior
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 <bigeasy@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)
unsigned long flags;
unsigned offset = GPIO_INDEX(bank, gpio);
- spin_lock_irqsave(&bank->lock, flags);
+ raw_spin_lock_irqsave(&bank->lock, flags);
gpiochip_unlock_as_irq(&bank->chip, offset);
bank->irq_usage &= ~(BIT(offset));
omap_disable_gpio_module(bank, offset);
omap_reset_gpio(bank, gpio);
- 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,
@@ -854,10 +854,10 @@ static void omap_gpio_mask_irq(struct irq_data *d)
unsigned int gpio = omap_irq_to_gpio(bank, d->hwirq);
unsigned long flags;
- spin_lock_irqsave(&bank->lock, flags);
+ raw_spin_lock_irqsave(&bank->lock, flags);
omap_set_gpio_irqenable(bank, gpio, 0);
omap_set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), 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)
@@ -868,7 +868,7 @@ static void omap_gpio_unmask_irq(struct irq_data *d)
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, GPIO_INDEX(bank, gpio), trigger);
@@ -880,7 +880,7 @@ static void omap_gpio_unmask_irq(struct irq_data *d)
}
omap_set_gpio_irqenable(bank, gpio, 1);
- spin_unlock_irqrestore(&bank->lock, flags);
+ raw_spin_unlock_irqrestore(&bank->lock, flags);
}
/*---------------------------------------------------------------------*/
@@ -893,9 +893,9 @@ static int omap_mpuio_suspend_noirq(struct device *dev)
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;
}
@@ -908,9 +908,9 @@ static int omap_mpuio_resume_noirq(struct device *dev)
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;
}
@@ -956,9 +956,9 @@ static int omap_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
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;
}
@@ -968,9 +968,9 @@ static int omap_gpio_input(struct gpio_chip *chip, unsigned offset)
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;
}
@@ -994,10 +994,10 @@ static int omap_gpio_output(struct gpio_chip *chip, unsigned offset, int value)
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;
}
@@ -1009,9 +1009,9 @@ static int omap_gpio_debounce(struct gpio_chip *chip, unsigned offset,
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;
}
@@ -1022,9 +1022,9 @@ static void omap_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
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);
}
/*---------------------------------------------------------------------*/
@@ -1250,7 +1250,7 @@ static int omap_gpio_probe(struct platform_device *pdev)
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);
@@ -1297,7 +1297,7 @@ static int omap_gpio_runtime_suspend(struct device *dev)
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.
@@ -1350,7 +1350,7 @@ static int omap_gpio_runtime_suspend(struct device *dev)
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;
}
@@ -1365,7 +1365,7 @@ static int omap_gpio_runtime_resume(struct device *dev)
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
@@ -1401,14 +1401,14 @@ static int omap_gpio_runtime_resume(struct device *dev)
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;
}
@@ -1463,7 +1463,7 @@ static int omap_gpio_runtime_resume(struct device *dev)
}
bank->workaround_enabled = false;
- spin_unlock_irqrestore(&bank->lock, flags);
+ raw_spin_unlock_irqrestore(&bank->lock, flags);
return 0;
}
--
2.1.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] gpio: omap: use raw locks for locking
2015-02-12 16:10 Sebastian Andrzej Siewior
@ 2015-02-16 8:54 ` Javier Martinez Canillas
2015-02-25 9:44 ` Sebastian Andrzej Siewior
2015-06-19 21:52 ` Javier Martinez Canillas
2015-06-30 6:45 ` Linus Walleij
2 siblings, 1 reply; 10+ messages in thread
From: Javier Martinez Canillas @ 2015-02-16 8:54 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Santosh Shilimkar, Kevin Hilman, Linus Walleij,
Alexandre Courbot, linux-omap, Linux GPIO List
Hello Sebastian,
Thanks a lot for your patch.
On Thu, Feb 12, 2015 at 5:10 PM, Sebastian Andrzej Siewior
<bigeasy@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 <bigeasy@linutronix.de>
> ---
> drivers/gpio/gpio-omap.c | 78 ++++++++++++++++++++++++------------------------
> 1 file changed, 39 insertions(+), 39 deletions(-)
>
Acked-by: Javier Martinez Canillas <javier@dowhile0.org>
Best regards,
Javier
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] gpio: omap: use raw locks for locking
2015-02-16 8:54 ` Javier Martinez Canillas
@ 2015-02-25 9:44 ` Sebastian Andrzej Siewior
0 siblings, 0 replies; 10+ messages in thread
From: Sebastian Andrzej Siewior @ 2015-02-25 9:44 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: Santosh Shilimkar, Kevin Hilman, Linus Walleij,
Alexandre Courbot, linux-omap, Linux GPIO List
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 <javier@dowhile0.org>
>
> Best regards,
> Javier
Sebastian
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] gpio: omap: use raw locks for locking
2015-02-12 16:10 Sebastian Andrzej Siewior
2015-02-16 8:54 ` Javier Martinez Canillas
@ 2015-06-19 21:52 ` Javier Martinez Canillas
2015-06-30 6:45 ` Linus Walleij
2 siblings, 0 replies; 10+ messages in thread
From: Javier Martinez Canillas @ 2015-06-19 21:52 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Santosh Shilimkar, Kevin Hilman, Linus Walleij,
Alexandre Courbot, linux-omap, Linux GPIO List
Hello Sebastian,
On Thu, Feb 12, 2015 at 5:10 PM, Sebastian Andrzej Siewior
<bigeasy@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 <bigeasy@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 <javier@dowhile0.org>
Best regards,
Javier
[0]: https://patchwork.ozlabs.org/patch/439249/
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] gpio: omap: use raw locks for locking
2015-02-12 16:10 Sebastian Andrzej Siewior
2015-02-16 8:54 ` Javier Martinez Canillas
2015-06-19 21:52 ` Javier Martinez Canillas
@ 2015-06-30 6:45 ` Linus Walleij
2015-06-30 16:39 ` Sebastian Andrzej Siewior
2 siblings, 1 reply; 10+ messages in thread
From: Linus Walleij @ 2015-06-30 6:45 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Javier Martinez Canillas, Santosh Shilimkar, Kevin Hilman,
Alexandre Courbot, Linux-OMAP, linux-gpio
On Thu, Feb 12, 2015 at 5:10 PM, Sebastian Andrzej Siewior
<bigeasy@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 <bigeasy@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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] gpio: omap: use raw locks for locking
2015-06-30 6:45 ` Linus Walleij
@ 2015-06-30 16:39 ` Sebastian Andrzej Siewior
0 siblings, 0 replies; 10+ messages in thread
From: Sebastian Andrzej Siewior @ 2015-06-30 16:39 UTC (permalink / raw)
To: Linus Walleij
Cc: Javier Martinez Canillas, Santosh Shilimkar, Kevin Hilman,
Alexandre Courbot, Linux-OMAP, linux-gpio
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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2015-07-28 12:22 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-07-21 16:26 [PATCH] gpio: omap: use raw locks for locking Sebastian Andrzej Siewior
2015-07-27 12:50 ` Linus Walleij
2015-07-27 13:25 ` Sebastian Andrzej Siewior
2015-07-28 12:22 ` Thomas Gleixner
-- strict thread matches above, loose matches on Subject: below --
2015-02-12 16:10 Sebastian Andrzej Siewior
2015-02-16 8:54 ` Javier Martinez Canillas
2015-02-25 9:44 ` Sebastian Andrzej Siewior
2015-06-19 21:52 ` Javier Martinez Canillas
2015-06-30 6:45 ` Linus Walleij
2015-06-30 16:39 ` Sebastian Andrzej Siewior
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.