All of lore.kernel.org
 help / color / mirror / Atom feed
From: Geert Uytterhoeven <geert@linux-m68k.org>
To: "Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	John Stultz <john.stultz@linaro.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Linux-Renesas <linux-renesas-soc@vger.kernel.org>
Subject: Re: [PATCH 1/2] clocksource/drivers/sh_cmt: Fix potential deadlock when calling runtime PM
Date: Mon, 7 Dec 2020 16:57:33 +0100	[thread overview]
Message-ID: <CAMuHMdWeOED=nckbaKEt3QRd5wiCyNZ6OBwk7m4vR15=AT7now@mail.gmail.com> (raw)
In-Reply-To: <20201205021921.1456190-2-niklas.soderlund+renesas@ragnatech.se>

Hi Niklas,

On Sat, Dec 5, 2020 at 3:20 AM Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> 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@ragnatech.se>

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@glider.be>

>                 ret = sh_cmt_enable(ch);
> +       }
>
>         if (ret)
>                 goto out;

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

  reply	other threads:[~2020-12-07 15:58 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-05  2:19 [PATCH 0/2] timekeeping: Fix change_clocksource() for PM and sh_cmt Niklas Söderlund
2020-12-05  2:19 ` [PATCH 1/2] clocksource/drivers/sh_cmt: Fix potential deadlock when calling runtime PM Niklas Söderlund
2020-12-07 15:57   ` Geert Uytterhoeven [this message]
2020-12-12 12:58   ` [tip: timers/core] " tip-bot2 for Niklas Söderlund
2020-12-05  2:19 ` [PATCH 2/2] timekeeping: Allow runtime PM from change_clocksource() Niklas Söderlund

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAMuHMdWeOED=nckbaKEt3QRd5wiCyNZ6OBwk7m4vR15=AT7now@mail.gmail.com' \
    --to=geert@linux-m68k.org \
    --cc=daniel.lezcano@linaro.org \
    --cc=john.stultz@linaro.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-renesas-soc@vger.kernel.org \
    --cc=niklas.soderlund+renesas@ragnatech.se \
    --cc=tglx@linutronix.de \
    --subject='Re: [PATCH 1/2] clocksource/drivers/sh_cmt: Fix potential deadlock when calling runtime PM' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.