diff mbox

gpio: omap: use raw locks for locking

Message ID 1437496011-11486-1-git-send-email-bigeasy@linutronix.de
State New
Headers show

Commit Message

Sebastian Andrzej Siewior July 21, 2015, 4:26 p.m. UTC 
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(-)

Comments

Linus Walleij July 27, 2015, 12:50 p.m. UTC | #1 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" 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 July 27, 2015, 1:25 p.m. UTC | #2 
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-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thomas Gleixner July 28, 2015, 12:22 p.m. UTC | #3 
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-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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