Re: [PATCH 1/2] clocksource/drivers/sh_cmt: Fix potential deadlock when calling runtime PM

[Geert Uytterhoeven]

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