Re: [PATCH] gpio: omap: Silence lockdep "Invalid wait context"

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Mon, Nov 25, 2024 at 9:05 AM A. Sverdlin
<alexander.sverdlin@xxxxxxxxxxx> wrote:
>
> From: Alexander Sverdlin <alexander.sverdlin@xxxxxxxxxxx>
>
> 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
>
> Problematic spin_lock_irqsave(&enable_lock, ...) is being called by
> clk_enable()/clk_disable() in omap2_set_gpio_debounce() and
> omap_clear_gpio_debounce().
>
> For omap2_set_gpio_debounce() it's possible to move
> raw_spin_lock_irqsave(&bank->lock, ...) inside omap2_set_gpio_debounce()
> so that the locks nest as follows:
>
>   clk_enable(bank->dbck)
>   raw_spin_lock_irqsave(&bank->lock, ...)
>   raw_spin_unlock_irqrestore()
>   clk_disable()
>
> Two call-sites of omap_clear_gpio_debounce() are more convoluted, but one
> can take the advantage of the nesting nature of clk_enable()/clk_disable(),
> so that the inner clk_disable() becomes lockless:
>
>   clk_enable(bank->dbck)                <-- only to clk_enable_lock()
>   raw_spin_lock_irqsave(&bank->lock, ...)
>   omap_clear_gpio_debounce()
>     clk_disable()                       <-- becomes lockless
>   raw_spin_unlock_irqrestore()
>   clk_disable()
>
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: 4dbada2be460 ("gpio: omap: use raw locks for locking")
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@xxxxxxxxxxx>
> ---
>  drivers/gpio/gpio-omap.c | 35 ++++++++++++++++++++++++++++++-----
>  1 file changed, 30 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 7ad4534054962..f9e502aa57753 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -181,6 +181,7 @@ 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;
> @@ -196,13 +197,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
> @@ -217,6 +223,9 @@ static int omap2_set_gpio_debounce(struct gpio_bank *bank, unsigned offset,
>                 bank->context.debounce_en = val;
>         }
>
> +       raw_spin_unlock_irqrestore(&bank->lock, flags);
> +       clk_disable(bank->dbck);
> +

This part looks pretty clear to me.

>         return 0;
>  }
>
> @@ -647,6 +656,13 @@ static void omap_gpio_irq_shutdown(struct irq_data *d)
>         unsigned long flags;
>         unsigned offset = d->hwirq;
>
> +       /*
> +        * Enable the clock here so that the nested clk_disable() in the
> +        * following omap_clear_gpio_debounce() is lockless
> +        */
> +       if (bank->dbck_flag)
> +               clk_enable(bank->dbck);
> +

But this looks like a functional change. You effectively bump the
clock enable count but don't add a corresponding clk_disable() in the
affected path. Is the clock ever actually disabled then?

Am I not getting something?

Bart

>         raw_spin_lock_irqsave(&bank->lock, flags);
>         bank->irq_usage &= ~(BIT(offset));
>         omap_set_gpio_triggering(bank, offset, IRQ_TYPE_NONE);
> @@ -656,6 +672,9 @@ static void omap_gpio_irq_shutdown(struct irq_data *d)
>                 omap_clear_gpio_debounce(bank, offset);
>         omap_disable_gpio_module(bank, offset);
>         raw_spin_unlock_irqrestore(&bank->lock, flags);
> +
> +       if (bank->dbck_flag)
> +               clk_disable(bank->dbck);
>  }
>
>  static void omap_gpio_irq_bus_lock(struct irq_data *data)
> @@ -827,6 +846,13 @@ static void omap_gpio_free(struct gpio_chip *chip, unsigned offset)
>         struct gpio_bank *bank = gpiochip_get_data(chip);
>         unsigned long flags;
>
> +       /*
> +        * Enable the clock here so that the nested clk_disable() in the
> +        * following omap_clear_gpio_debounce() is lockless
> +        */
> +       if (bank->dbck_flag)
> +               clk_enable(bank->dbck);
> +
>         raw_spin_lock_irqsave(&bank->lock, flags);
>         bank->mod_usage &= ~(BIT(offset));
>         if (!LINE_USED(bank->irq_usage, offset)) {
> @@ -836,6 +862,9 @@ static void omap_gpio_free(struct gpio_chip *chip, unsigned offset)
>         omap_disable_gpio_module(bank, offset);
>         raw_spin_unlock_irqrestore(&bank->lock, flags);
>
> +       if (bank->dbck_flag)
> +               clk_disable(bank->dbck);
> +
>         pm_runtime_put(chip->parent);
>  }
>
> @@ -913,15 +942,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)",
> --
> 2.47.0
>