| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAMuHMdVKZn-YfBxvwA1wgjksvzZK=NPzaoTCPRur_Z=AneLA6w@mail.gmail.com>
Date: Tue, 23 Sep 2025 16:56:05 +0200
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Niklas Söderlund <niklas.soderlund+renesas@...natech.se>
Cc: Daniel Lezcano <daniel.lezcano@...aro.org>, Thomas Gleixner <tglx@...utronix.de>,
linux-kernel@...r.kernel.org, linux-renesas-soc@...r.kernel.org
Subject: Re: [PATCH v2 2/2] clocksource/drivers/sh_cmt: Do not power down
channels used for events
Hi Niklas,
On Wed, 10 Sept 2025 at 16:27, Niklas Söderlund
<niklas.soderlund+renesas@...natech.se> wrote:
> The CMT do runtime PM and call clk_enable()/clk_disable() when a new
> clock event is register and the CMT is not already started. This is not
> always possible as a spinlock is also needed to sync the internals of
> the CMT. Running with PROVE_LOCKING uncovers one such issue.
>
> =============================
> [ BUG: Invalid wait context ]
> 6.17.0-rc3-arm64-renesas-03071-gb3c4f4122b28-dirty #21 Not tainted
> -----------------------------
> swapper/1/0 is trying to lock:
> ffff00000898d180 (&dev->power.lock){-...}-{3:3}, at: __pm_runtime_resume+0x38/0x88
> ccree e6601000.crypto: ARM CryptoCell 630P Driver: HW version 0xAF400001/0xDCC63000, Driver version 5.0
> other info that might help us debug this:
> ccree e6601000.crypto: ARM ccree device initialized
> context-{5:5}
> 2 locks held by swapper/1/0:
> #0: ffff80008173c298 (tick_broadcast_lock){-...}-{2:2}, at: __tick_broadcast_oneshot_control+0xa4/0x3a8
> #1: ffff0000089a5858 (&ch->lock){....}-{2:2}
> usbcore: registered new interface driver usbhid
> , at: sh_cmt_start+0x30/0x364
> stack backtrace:
> CPU: 1 UID: 0 PID: 0 Comm: swapper/1 Not tainted 6.17.0-rc3-arm64-renesas-03071-gb3c4f4122b28-dirty #21 PREEMPT
> Hardware name: Renesas Salvator-X 2nd version board based on r8a77965 (DT)
> Call trace:
> show_stack+0x14/0x1c (C)
> dump_stack_lvl+0x6c/0x90
> dump_stack+0x14/0x1c
> __lock_acquire+0x904/0x1584
> lock_acquire+0x220/0x34c
> _raw_spin_lock_irqsave+0x58/0x80
> __pm_runtime_resume+0x38/0x88
> sh_cmt_start+0x54/0x364
> sh_cmt_clock_event_set_oneshot+0x64/0xb8
> clockevents_switch_state+0xfc/0x13c
> tick_broadcast_set_event+0x30/0xa4
> __tick_broadcast_oneshot_control+0x1e0/0x3a8
> tick_broadcast_oneshot_control+0x30/0x40
> cpuidle_enter_state+0x40c/0x680
> cpuidle_enter+0x30/0x40
> do_idle+0x1f4/0x26c
> cpu_startup_entry+0x34/0x40
> secondary_start_kernel+0x11c/0x13c
> __secondary_switched+0x74/0x78
>
> Fix this by unconditionally powering on and enabling the needed clocks
> for all CMT channels which are used for clock events. Do this before
> registering any clock source or event to avid having to take the
> spin lock at probe time.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@...natech.se>
> ---
> * Changes since v1
> - Move the unconditional power on case before registering any clock
> source or event to avoid having to use the spinlock to synchronize the
> powerup sequence in probe.
Thanks for your patch, which is now commit cfbc0f1d24030ff9
("clocksource/drivers/sh_cmt: Do not power down channels used for
events") in clockevents/timers/drivers/next.
Unfortunately this commit introduces an s2ram regression on e.g.
Atmark Techo Armadillo-800EVA with R-Mobile A1: the system wakes
up immediately. There is no evidence of a wake-up event shown in
/sys/kernel/debug/wakeup_sources. This happens with or without
console_suspend enabled.
Reverting this commit fixes the issue. I suspect the system wakes up
because the periodic clock event fires, and causes an interrupt.
> --- a/drivers/clocksource/sh_cmt.c
> +++ b/drivers/clocksource/sh_cmt.c
> @@ -5,6 +5,7 @@
> * Copyright (C) 2008 Magnus Damm
> */
>
> +#include <linux/cleanup.h>
> #include <linux/clk.h>
> #include <linux/clockchips.h>
> #include <linux/clocksource.h>
> @@ -623,51 +624,6 @@ static void sh_cmt_stop_clocksource(struct sh_cmt_channel *ch)
> pm_runtime_put(&ch->cmt->pdev->dev);
> }
>
> -static int sh_cmt_start_clockevent(struct sh_cmt_channel *ch)
> -{
> - int ret = 0;
> - unsigned long flags;
> -
> - raw_spin_lock_irqsave(&ch->lock, flags);
> -
> - if (!(ch->flags & (FLAG_CLOCKEVENT | FLAG_CLOCKSOURCE))) {
> - pm_runtime_get_sync(&ch->cmt->pdev->dev);
> - ret = sh_cmt_enable(ch);
> - }
> -
> - if (ret)
> - goto out;
> -
> - ch->flags |= FLAG_CLOCKEVENT;
> - out:
> - raw_spin_unlock_irqrestore(&ch->lock, flags);
> -
> - return ret;
> -}
> -
> -static void sh_cmt_stop_clockevent(struct sh_cmt_channel *ch)
> -{
> - unsigned long flags;
> - unsigned long f;
> -
> - raw_spin_lock_irqsave(&ch->lock, flags);
> -
> - f = ch->flags & (FLAG_CLOCKEVENT | FLAG_CLOCKSOURCE);
> -
> - ch->flags &= ~FLAG_CLOCKEVENT;
> -
> - if (f && !(ch->flags & (FLAG_CLOCKEVENT | FLAG_CLOCKSOURCE))) {
> - sh_cmt_disable(ch);
> - pm_runtime_put(&ch->cmt->pdev->dev);
> - }
> -
> - /* adjust the timeout to maximum if only clocksource left */
> - if (ch->flags & FLAG_CLOCKSOURCE)
> - __sh_cmt_set_next(ch, ch->max_match_value);
> -
> - raw_spin_unlock_irqrestore(&ch->lock, flags);
> -}
> -
> static struct sh_cmt_channel *cs_to_sh_cmt(struct clocksource *cs)
> {
> return container_of(cs, struct sh_cmt_channel, cs);
> @@ -774,19 +730,30 @@ static struct sh_cmt_channel *ced_to_sh_cmt(struct clock_event_device *ced)
>
> static void sh_cmt_clock_event_start(struct sh_cmt_channel *ch, int periodic)
> {
> - sh_cmt_start_clockevent(ch);
> -
> if (periodic)
> sh_cmt_set_next(ch, ((ch->cmt->rate + HZ/2) / HZ) - 1);
> else
> sh_cmt_set_next(ch, ch->max_match_value);
> }
>
> +static void sh_cmt_clock_event_stop(struct sh_cmt_channel *ch)
> +{
> + unsigned long flags;
> +
> + raw_spin_lock_irqsave(&ch->lock, flags);
> +
> + /* adjust the timeout to maximum if only clocksource left */
> + if (ch->flags & FLAG_CLOCKSOURCE)
> + __sh_cmt_set_next(ch, ch->max_match_value);
> +
> + raw_spin_unlock_irqrestore(&ch->lock, flags);
> +}
> +
> static int sh_cmt_clock_event_shutdown(struct clock_event_device *ced)
> {
> struct sh_cmt_channel *ch = ced_to_sh_cmt(ced);
>
> - sh_cmt_stop_clockevent(ch);
> + sh_cmt_clock_event_stop(ch);
> return 0;
> }
>
> @@ -797,7 +764,7 @@ static int sh_cmt_clock_event_set_state(struct clock_event_device *ced,
>
> /* deal with old setting first */
> if (clockevent_state_oneshot(ced) || clockevent_state_periodic(ced))
> - sh_cmt_stop_clockevent(ch);
> + sh_cmt_clock_event_stop(ch);
>
> dev_info(&ch->cmt->pdev->dev, "ch%u: used for %s clock events\n",
> ch->index, periodic ? "periodic" : "oneshot");
> @@ -968,6 +935,32 @@ static int sh_cmt_setup_channel(struct sh_cmt_channel *ch, unsigned int index,
> ch->match_value = ch->max_match_value;
> raw_spin_lock_init(&ch->lock);
>
> + if (clockevent) {
> + /*
> + * To support clock events the CMT must always be ready to
> + * accept new events, start it and leave no way for it to be
> + * turned off.
> + *
> + * This is OK as we can't never unregister a CMT device. If this
> + * is fixed in future an equal unconditional shutdown is needed.
> + *
> + * We don't need to hold the channel lock here as we have not
> + * yet register any clock source or event so there is no
> + * possible race. And holding the spinlock at probe time
> + * produces lockdep warnings.
> + */
> + pm_runtime_get_sync(&ch->cmt->pdev->dev);
> + ret = sh_cmt_enable(ch);
> + if (ret)
> + return ret;
> +
> + /*
> + * Flag that the channel is used as a clock event, it's not
> + * allowed to be powered off!
> + */
> + ch->flags |= FLAG_CLOCKEVENT;
> + }
> +
> ret = sh_cmt_register(ch, dev_name(&cmt->pdev->dev),
> clockevent, clocksource);
> if (ret) {
> --
> 2.51.0
>
--
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-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
Powered by blists - more mailing lists