Re: [PATCH v2 2/2] clocksource/drivers/sh_cmt: Do not power down channels used for events

[Daniel Lezcano]

Hi Geert,

On 23/09/2025 16:56, Geert Uytterhoeven wrote:
> Hi Niklas,
>
> On Wed, 10 Sept 2025 at 16:27, Niklas Söderlund
> <niklas.soderlund+renesas@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> > ---
> > * 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.

I'm about to send a PR.

Shall I drop this change which fixes a lock issue or keep it ?

What has the higher priority ?


--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog