Hi Niklas,
On Sat, Dec 5, 2020 at 3:20 AM Niklas Söderlund
<niklas.soderlund+renesas@xxxxxxxxxxxx> wrote:
> The ch->lock is used to protect the whole enable() and read() of
> sh_cmt's implementation of struct clocksource. The enable()
> implementation calls pm_runtime_get_sync() which may result in the clock
> source to be read() triggering a cyclic lockdep warning for the
> ch->lock.
>
> The sh_cmt driver implement its own balancing of calls to
> sh_cmt_{enable,disable}() with flags in sh_cmt_{start,stop}(). It does
> this to deal with that start and stop are shared between the clock
> source and clock event providers. While this could be improved on
> verifying corner cases based on any substantial rework on all devices
> this driver supports might prove hard.
>
> As a first step separate the PM handling for clock event and clock
> source. Always put/get the device when enabling/disabling the clock
> source but keep the clock event logic unchanged. This allows the sh_cmt
> implementation of struct clocksource to call PM without holding the
> ch->lock and avoiding the deadlock.
>
> Triggering and log of the deadlock warning,
>
> # echo e60f0000.timer > /sys/devices/system/clocksource/clocksource0/current_clocksource
> [ 46.948370] ======================================================
> [ 46.954730] WARNING: possible circular locking dependency detected
[...]
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
Thanks for looking into this!
> --- a/drivers/clocksource/sh_cmt.c
> +++ b/drivers/clocksource/sh_cmt.c
> @@ -319,7 +319,6 @@ static int sh_cmt_enable(struct sh_cmt_channel *ch)
> {
> int k, ret;
>
> - pm_runtime_get_sync(&ch->cmt->pdev->dev);
> dev_pm_syscore_device(&ch->cmt->pdev->dev, true);
>
> /* enable clock */
> @@ -394,7 +393,6 @@ static void sh_cmt_disable(struct sh_cmt_channel *ch)
> clk_disable(ch->cmt->clk);
>
> dev_pm_syscore_device(&ch->cmt->pdev->dev, false);
> - pm_runtime_put(&ch->cmt->pdev->dev);
> }
>
> /* private flags */
> @@ -562,10 +560,16 @@ static int sh_cmt_start(struct sh_cmt_channel *ch, unsigned long flag)
> int ret = 0;
> unsigned long flags;
>
> + if (flag & FLAG_CLOCKSOURCE)
> + pm_runtime_get_sync(&ch->cmt->pdev->dev);
> +
> raw_spin_lock_irqsave(&ch->lock, flags);
>
> - if (!(ch->flags & (FLAG_CLOCKEVENT | FLAG_CLOCKSOURCE)))
> + if (!(ch->flags & (FLAG_CLOCKEVENT | FLAG_CLOCKSOURCE))) {
> + if (flag & FLAG_CLOCKEVENT)
> + pm_runtime_get_sync(&ch->cmt->pdev->dev);
This change emphasizes the (pre-existing) issue with clock events:
pm_runtime_get_sync() is called while holding a spinlock, leading to the
following splat on r8a7740/armadillo:
sh_cmt e6138000.timer: ch0: used for periodic clock events
=============================
[ BUG: Invalid wait context ]
5.10.0-rc5-armadillo-00566-g8eaa6103691d-dirty #225 Not tainted
-----------------------------
swapper/1 is trying to lock:
c254cd2c (&dev->power.lock){....}-{3:3}, at: __pm_runtime_resume+0x54/0x80
other info that might help us debug this:
context-{5:5}
3 locks held by swapper/1:
#0: c254ccd0 (&dev->mutex){....}-{4:4}, at: device_driver_attach+0x18/0x5c
#1: c0bed230 (clockevents_lock){....}-{2:2}, at:
clockevents_register_device+0x5c/0x10c
#2: c26a5038 (&ch->lock){....}-{2:2}, at: sh_cmt_start+0x18/0x1b0
As this is a pre-existing issue:
Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
> ret = sh_cmt_enable(ch);
> + }
>
> if (ret)
> goto out;
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
