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

["Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>]

Hi Daniel,

On 2025-09-24 11:19:21 +0200, Daniel Lezcano wrote:
> 
> 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@ragnatech.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@ragnatech.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.
> 
> 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 ?

If it's not too late I think we should drop it. The issue this work 
tries to solve is a lockdep blurb which highlights a design issue in the 
driver. But the driver have function properly in the past. So I think 
it's better I work on solving the blurb without any regressions.

Thanks for checking.

> 
> 
> -- 
> <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

-- 
Kind Regards,
Niklas Söderlund