diff mbox series

[v2] gpio: omap: Silence lockdep "Invalid wait context"

Message ID 20241127184506.962756-1-alexander.sverdlin@siemens.com (mailing list archive)
State New
Headers show
Series [v2] gpio: omap: Silence lockdep "Invalid wait context" | expand

Commit Message

Sverdlin, Alexander Nov. 27, 2024, 6:45 p.m. UTC 
From: Alexander Sverdlin <alexander.sverdlin@siemens.com>

The problem apparetly has been known since the conversion to
raw_spin_lock() (commit 4dbada2be460
("gpio: omap: use raw locks for locking")).

Symptom:

[ BUG: Invalid wait context ]
5.10.214
-----------------------------
swapper/1 is trying to lock:
(enable_lock){....}-{3:3}, at: clk_enable_lock
other info that might help us debug this:
context-{5:5}
2 locks held by swapper/1:
 #0: (&dev->mutex){....}-{4:4}, at: device_driver_attach
 #1: (&bank->lock){....}-{2:2}, at: omap_gpio_set_config
stack backtrace:
CPU: 0 PID: 1 Comm: swapper Not tainted 5.10.214
Hardware name: Generic AM33XX (Flattened Device Tree)
unwind_backtrace
show_stack
__lock_acquire
lock_acquire.part.0
_raw_spin_lock_irqsave
clk_enable_lock
clk_enable
omap_gpio_set_config
gpio_keys_setup_key
gpio_keys_probe
platform_drv_probe
really_probe
driver_probe_device
device_driver_attach
__driver_attach
bus_for_each_dev
bus_add_driver
driver_register
do_one_initcall
do_initcalls
kernel_init_freeable
kernel_init

Reorder clk_enable()/clk_disable() calls in a way that they always happen
outside of raw_spin_lock'ed sections.

Cc: stable@vger.kernel.org
Fixes: 4dbada2be460 ("gpio: omap: use raw locks for locking")
Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
---
Changelog:
v2: complete rework, I've totally missed the fact
    clk_enable()/clk_disable() cannot avoid clk_enable_lock() even if the
    clock is enabled; I had to extract all clk_*() calls out of
    raw_spin_lock'ed sections

 drivers/gpio/gpio-omap.c | 114 +++++++++++++++++++++++++++++----------
 1 file changed, 87 insertions(+), 27 deletions(-)

Comments

Bartosz Golaszewski Dec. 3, 2024, 4:41 p.m. UTC | #1 
On Wed, Nov 27, 2024 at 7:45 PM A. Sverdlin
<alexander.sverdlin@siemens.com> wrote:
>
> From: Alexander Sverdlin <alexander.sverdlin@siemens.com>
>
> The problem apparetly has been known since the conversion to
> raw_spin_lock() (commit 4dbada2be460
> ("gpio: omap: use raw locks for locking")).
>
> Symptom:
>
> [ BUG: Invalid wait context ]
> 5.10.214
> -----------------------------
> swapper/1 is trying to lock:
> (enable_lock){....}-{3:3}, at: clk_enable_lock
> other info that might help us debug this:
> context-{5:5}
> 2 locks held by swapper/1:
>  #0: (&dev->mutex){....}-{4:4}, at: device_driver_attach
>  #1: (&bank->lock){....}-{2:2}, at: omap_gpio_set_config
> stack backtrace:
> CPU: 0 PID: 1 Comm: swapper Not tainted 5.10.214
> Hardware name: Generic AM33XX (Flattened Device Tree)
> unwind_backtrace
> show_stack
> __lock_acquire
> lock_acquire.part.0
> _raw_spin_lock_irqsave
> clk_enable_lock
> clk_enable
> omap_gpio_set_config
> gpio_keys_setup_key
> gpio_keys_probe
> platform_drv_probe
> really_probe
> driver_probe_device
> device_driver_attach
> __driver_attach
> bus_for_each_dev
> bus_add_driver
> driver_register
> do_one_initcall
> do_initcalls
> kernel_init_freeable
> kernel_init
>
> Reorder clk_enable()/clk_disable() calls in a way that they always happen
> outside of raw_spin_lock'ed sections.
>
> Cc: stable@vger.kernel.org
> Fixes: 4dbada2be460 ("gpio: omap: use raw locks for locking")
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> ---
> Changelog:
> v2: complete rework, I've totally missed the fact
>     clk_enable()/clk_disable() cannot avoid clk_enable_lock() even if the
>     clock is enabled; I had to extract all clk_*() calls out of
>     raw_spin_lock'ed sections
>

This looks so much worse now. :(

I refuse to believe this is the only way to fix it.

Would it be possible to wrap the logic that disables the clock
depending on the return value of omap_gpio_dbck_enable() in some
abstraction layer? Basing the behavior on the boolean return value of
a function named omap_gpio_dbck_enable() makes very little sense when
looking at it now and it will make even less a year from now.

Could we add functions like omap_gpio_dbck_clk_enable() and
omap_gpio_dbck_clk_disable() plus some state in the driver data set by
omap_gpio_dbck_enable() which would be then read by
omap_gpio_dbck_clk_disable() in order to determine whether to disable
the clock or keep it going?

Bart
diff mbox series

Patch

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 7ad4534054962..3e6c8dec7cacc 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -140,18 +140,22 @@  static void omap_set_gpio_dataout_mask(struct gpio_bank *bank, unsigned offset,
 					      BIT(offset), enable);
 }
 
-static inline void omap_gpio_dbck_enable(struct gpio_bank *bank)
+/* Returns true if debounce clock has to be enabled by the caller */
+static inline bool omap_gpio_dbck_enable(struct gpio_bank *bank)
 {
 	if (bank->dbck_enable_mask && !bank->dbck_enabled) {
-		clk_enable(bank->dbck);
 		bank->dbck_enabled = true;
-
 		writel_relaxed(bank->dbck_enable_mask,
 			     bank->base + bank->regs->debounce_en);
+
+		return true;
 	}
+
+	return false;
 }
 
-static inline void omap_gpio_dbck_disable(struct gpio_bank *bank)
+/* Returns true if debounce clock has to be disabled by the caller */
+static inline bool omap_gpio_dbck_disable(struct gpio_bank *bank)
 {
 	if (bank->dbck_enable_mask && bank->dbck_enabled) {
 		/*
@@ -160,10 +164,12 @@  static inline void omap_gpio_dbck_disable(struct gpio_bank *bank)
 		 * to detect events and generate interrupts at least on OMAP3.
 		 */
 		writel_relaxed(0, bank->base + bank->regs->debounce_en);
-
-		clk_disable(bank->dbck);
 		bank->dbck_enabled = false;
+
+		return true;
 	}
+
+	return false;
 }
 
 /**
@@ -181,9 +187,11 @@  static inline void omap_gpio_dbck_disable(struct gpio_bank *bank)
 static int omap2_set_gpio_debounce(struct gpio_bank *bank, unsigned offset,
 				   unsigned debounce)
 {
+	unsigned long		flags;
 	u32			val;
 	u32			l;
 	bool			enable = !!debounce;
+	bool			enable_clk;
 
 	if (!bank->dbck_flag)
 		return -ENOTSUPP;
@@ -196,13 +204,18 @@  static int omap2_set_gpio_debounce(struct gpio_bank *bank, unsigned offset,
 
 	l = BIT(offset);
 
+	/*
+	 * Ordering is important here: clk_enable() calls spin_lock_irqsave(),
+	 * therefore it must be outside of the following raw_spin_lock_irqsave()
+	 */
 	clk_enable(bank->dbck);
+	raw_spin_lock_irqsave(&bank->lock, flags);
+
 	writel_relaxed(debounce, bank->base + bank->regs->debounce);
 
 	val = omap_gpio_rmw(bank->base + bank->regs->debounce_en, l, enable);
 	bank->dbck_enable_mask = val;
 
-	clk_disable(bank->dbck);
 	/*
 	 * Enable debounce clock per module.
 	 * This call is mandatory because in omap_gpio_request() when
@@ -211,12 +224,16 @@  static int omap2_set_gpio_debounce(struct gpio_bank *bank, unsigned offset,
 	 * used within _gpio_dbck_enable() is still not initialized at
 	 * that point. Therefore we have to enable dbck here.
 	 */
-	omap_gpio_dbck_enable(bank);
+	enable_clk = omap_gpio_dbck_enable(bank);
 	if (bank->dbck_enable_mask) {
 		bank->context.debounce = debounce;
 		bank->context.debounce_en = val;
 	}
 
+	raw_spin_unlock_irqrestore(&bank->lock, flags);
+	if (!enable_clk)
+		clk_disable(bank->dbck);
+
 	return 0;
 }
 
@@ -229,16 +246,19 @@  static int omap2_set_gpio_debounce(struct gpio_bank *bank, unsigned offset,
  * this is the only gpio in this bank using debounce, then clear the debounce
  * time too. The debounce clock will also be disabled when calling this function
  * if this is the only gpio in the bank using debounce.
+ *
+ * Return: true if bank->bdck clock has to be disabled by the caller,
+ *         false otherwise
  */
-static void omap_clear_gpio_debounce(struct gpio_bank *bank, unsigned offset)
+static bool omap_clear_gpio_debounce(struct gpio_bank *bank, unsigned int offset)
 {
 	u32 gpio_bit = BIT(offset);
 
 	if (!bank->dbck_flag)
-		return;
+		return false;
 
 	if (!(bank->dbck_enable_mask & gpio_bit))
-		return;
+		return false;
 
 	bank->dbck_enable_mask &= ~gpio_bit;
 	bank->context.debounce_en &= ~gpio_bit;
@@ -249,9 +269,12 @@  static void omap_clear_gpio_debounce(struct gpio_bank *bank, unsigned offset)
 		bank->context.debounce = 0;
 		writel_relaxed(bank->context.debounce, bank->base +
 			     bank->regs->debounce);
-		clk_disable(bank->dbck);
 		bank->dbck_enabled = false;
+
+		return true;
 	}
+
+	return false;
 }
 
 /*
@@ -646,6 +669,7 @@  static void omap_gpio_irq_shutdown(struct irq_data *d)
 	struct gpio_bank *bank = omap_irq_data_get_bank(d);
 	unsigned long flags;
 	unsigned offset = d->hwirq;
+	bool disable_clk = false;
 
 	raw_spin_lock_irqsave(&bank->lock, flags);
 	bank->irq_usage &= ~(BIT(offset));
@@ -653,9 +677,17 @@  static void omap_gpio_irq_shutdown(struct irq_data *d)
 	omap_clear_gpio_irqstatus(bank, offset);
 	omap_set_gpio_irqenable(bank, offset, 0);
 	if (!LINE_USED(bank->mod_usage, offset))
-		omap_clear_gpio_debounce(bank, offset);
+		disable_clk = omap_clear_gpio_debounce(bank, offset);
 	omap_disable_gpio_module(bank, offset);
 	raw_spin_unlock_irqrestore(&bank->lock, flags);
+
+	/*
+	 * This has to be done outside of bank->lock'ed section, because
+	 * spin_lock_irqsave(&enable_lock, ...) <= clk_enable_lock()
+	 * cannot be called from raw_spin_lock'ed section.
+	 */
+	if (disable_clk)
+		clk_disable(bank->dbck);
 }
 
 static void omap_gpio_irq_bus_lock(struct irq_data *data)
@@ -825,17 +857,26 @@  static int omap_gpio_request(struct gpio_chip *chip, unsigned offset)
 static void omap_gpio_free(struct gpio_chip *chip, unsigned offset)
 {
 	struct gpio_bank *bank = gpiochip_get_data(chip);
+	bool disable_clk = false;
 	unsigned long 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);
+		disable_clk = omap_clear_gpio_debounce(bank, offset);
 	}
 	omap_disable_gpio_module(bank, offset);
 	raw_spin_unlock_irqrestore(&bank->lock, flags);
 
+	/*
+	 * This has to be done outside of bank->lock'ed section, because
+	 * spin_lock_irqsave(&enable_lock, ...) <= clk_enable_lock()
+	 * cannot be called from raw_spin_lock'ed section.
+	 */
+	if (disable_clk)
+		clk_disable(bank->dbck);
+
 	pm_runtime_put(chip->parent);
 }
 
@@ -913,15 +954,11 @@  static int omap_gpio_debounce(struct gpio_chip *chip, unsigned offset,
 			      unsigned debounce)
 {
 	struct gpio_bank *bank;
-	unsigned long flags;
 	int ret;
 
 	bank = gpiochip_get_data(chip);
 
-	raw_spin_lock_irqsave(&bank->lock, flags);
 	ret = omap2_set_gpio_debounce(bank, offset, debounce);
-	raw_spin_unlock_irqrestore(&bank->lock, flags);
-
 	if (ret)
 		dev_info(chip->parent,
 			 "Could not set line %u debounce to %u microseconds (%d)",
@@ -1131,7 +1168,8 @@  static void omap_gpio_restore_context(struct gpio_bank *bank)
 	writel_relaxed(bank->context.irqenable2, base + regs->irqenable2);
 }
 
-static void omap_gpio_idle(struct gpio_bank *bank, bool may_lose_context)
+/* Returns true if debounce clock has to be disabled by the caller */
+static bool omap_gpio_idle(struct gpio_bank *bank, bool may_lose_context)
 {
 	struct device *dev = bank->chip.parent;
 	void __iomem *base = bank->base;
@@ -1175,13 +1213,15 @@  static void omap_gpio_idle(struct gpio_bank *bank, bool may_lose_context)
 		bank->context_loss_count =
 				bank->get_context_loss_count(dev);
 
-	omap_gpio_dbck_disable(bank);
+	return omap_gpio_dbck_disable(bank);
 }
 
-static void omap_gpio_unidle(struct gpio_bank *bank)
+/* Returns true if debounce clock has to be enabled by the caller */
+static bool omap_gpio_unidle(struct gpio_bank *bank)
 {
 	struct device *dev = bank->chip.parent;
 	u32 l = 0, gen, gen0, gen1;
+	bool enable_clk;
 	int c;
 
 	/*
@@ -1197,7 +1237,7 @@  static void omap_gpio_unidle(struct gpio_bank *bank)
 				bank->get_context_loss_count(dev);
 	}
 
-	omap_gpio_dbck_enable(bank);
+	enable_clk = omap_gpio_dbck_enable(bank);
 
 	if (bank->loses_context) {
 		if (!bank->get_context_loss_count) {
@@ -1207,7 +1247,7 @@  static void omap_gpio_unidle(struct gpio_bank *bank)
 			if (c != bank->context_loss_count) {
 				omap_gpio_restore_context(bank);
 			} else {
-				return;
+				return enable_clk;
 			}
 		}
 	} else {
@@ -1267,6 +1307,8 @@  static void omap_gpio_unidle(struct gpio_bank *bank)
 		writel_relaxed(old0, bank->base + bank->regs->leveldetect0);
 		writel_relaxed(old1, bank->base + bank->regs->leveldetect1);
 	}
+
+	return enable_clk;
 }
 
 static int gpio_omap_cpu_notifier(struct notifier_block *nb,
@@ -1276,6 +1318,8 @@  static int gpio_omap_cpu_notifier(struct notifier_block *nb,
 	unsigned long flags;
 	int ret = NOTIFY_OK;
 	u32 isr, mask;
+	bool enable_clk = false;
+	bool disable_clk = false;
 
 	bank = container_of(nb, struct gpio_bank, nb);
 
@@ -1291,17 +1335,23 @@  static int gpio_omap_cpu_notifier(struct notifier_block *nb,
 			ret = NOTIFY_BAD;
 			break;
 		}
-		omap_gpio_idle(bank, true);
+		disable_clk = omap_gpio_idle(bank, true);
 		break;
 	case CPU_CLUSTER_PM_ENTER_FAILED:
 	case CPU_CLUSTER_PM_EXIT:
-		omap_gpio_unidle(bank);
+		enable_clk = omap_gpio_unidle(bank);
 		break;
 	}
 
 out_unlock:
 	raw_spin_unlock_irqrestore(&bank->lock, flags);
 
+	/* This has to happen outside of raw_spin_lock'ed section */
+	if (enable_clk)
+		clk_enable(bank->dbck);
+	if (disable_clk)
+		clk_disable(bank->dbck);
+
 	return ret;
 }
 
@@ -1503,12 +1553,17 @@  static int __maybe_unused omap_gpio_runtime_suspend(struct device *dev)
 {
 	struct gpio_bank *bank = dev_get_drvdata(dev);
 	unsigned long flags;
+	bool disable_clk;
 
 	raw_spin_lock_irqsave(&bank->lock, flags);
-	omap_gpio_idle(bank, true);
+	disable_clk = omap_gpio_idle(bank, true);
 	bank->is_suspended = true;
 	raw_spin_unlock_irqrestore(&bank->lock, flags);
 
+	/* This has to happen outside of raw_spin_lock'ed section */
+	if (disable_clk)
+		clk_disable(bank->dbck);
+
 	return 0;
 }
 
@@ -1516,12 +1571,17 @@  static int __maybe_unused omap_gpio_runtime_resume(struct device *dev)
 {
 	struct gpio_bank *bank = dev_get_drvdata(dev);
 	unsigned long flags;
+	bool enable_clk;
 
 	raw_spin_lock_irqsave(&bank->lock, flags);
-	omap_gpio_unidle(bank);
+	enable_clk = omap_gpio_unidle(bank);
 	bank->is_suspended = false;
 	raw_spin_unlock_irqrestore(&bank->lock, flags);
 
+	/* This has to happen outside of raw_spin_lock'ed section */
+	if (enable_clk)
+		clk_enable(bank->dbck);
+
 	return 0;
 }