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

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Wed, Nov 27, 2024 at 7:45 PM 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
>
> Reorder clk_enable()/clk_disable() calls in a way that they always happen
> outside of raw_spin_lock'ed sections.
>
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: 4dbada2be460 ("gpio: omap: use raw locks for locking")
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@xxxxxxxxxxx>
> ---
> 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