Message-ID: <20250910142151.GB3518415@ragnatech.se>
Date: Wed, 10 Sep 2025 16:21:51 +0200
From: Niklas Söderlund <niklas.soderlund+renesas@...natech.se>
To: Geert Uytterhoeven <geert@...ux-m68k.org>
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: [RFC/PATCH 0/2] clocksource/drivers/sh_cmt: Improve clock event
 design

Hi Geert,

On 2025-09-09 14:17:16 +0200, Geert Uytterhoeven wrote:
> Hi Niklas,
> 
> On Sat, 30 Aug 2025 at 23:10, Niklas Söderlund
> <niklas.soderlund+renesas@...natech.se> wrote:
> > This RFC/PATCH tries to address an issue with the Renesas CMT driver
> > design. The driver do PM and clock handling in struct clock_event_device
> > callbacks. This leads to LOCKDEP warnings and I think hints at a larger
> > 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
> >
> > This series tries to address this by instead doing PM and clock
> > management at probe time, and leaving them on for the CMT channels that
> > are used as clock events. The CMT design is a bit messy as channels can
> > be used both as clock sources and events. And the design to do the
> > housekeeping for clock sources seems to be valid and is kept.
> >
> > This is posted as an RFC as there is one other driver in-tree that
> > suffers form similar issues. I intend to try and refactor that away too,
> > but would like to try and get some feedback first.
> >
> > The work is tested on R-Car M3N.
> >
> > Niklas Söderlund (2):
> >   clocksource/drivers/sh_cmt: Split start/stop of clock source and
> >     events
> >   clocksource/drivers/sh_cmt: Do not power down channels used for events
> 
> Thanks for your series!
> 
> I can confirm this fixes the issue on e.g. R-Car Gen3, where the CMT
> can be used for clock events or as a clock source, so
> Tested-by: Geert Uytterhoeven <geert+renesas@...der.be>

Thanks for testing this.

> 
> On R-Mobile A1, the CMT is also used for periodic clock events.
> Your series seems to fix one invalid context, and expose a "new" one
> (lockdep shuts down after the first report):

Interesting find. Indeed this patch solves on issue just to uncover the 
next one. I have reworked the order of when the CMT is enabled in the 
probe call path to also solve this new uncovered issue and will post a 
v2.

Thanks for finding it, and for allowing me access to a R-Mobile A1 so I 
could fix it.

Please note when you test v2 on R-Mobile A1 you will still hit a splat 
"BUG: Invalid wait context" as fixing all CMT issues moves on and 
exposes issue with TMU (a different driver)... I intend to fix them too 
as soon as this CMT solution is acked as I suspect TMU have very similar 
issue as I try to fix in CMT with this work.

> 
>     sh_cmt e6138000.timer: ch0: used for clock events
>     sh_cmt e6138000.timer: ch0: used for periodic clock events
> 
>     =============================
>     [ BUG: Invalid wait context ]
>     6.17.0-rc5-armadillo-06055-g57d9d685e295 #872 Not tainted
>     -----------------------------
>     swapper/0/1 is trying to lock:
>     c0e5ae28 (enable_lock){....}-{3:3}, at: clk_enable_lock+0x38/0xc4
>     other info that might help us debug this:
>     context-{5:5}
>     2 locks held by swapper/0/1:
>      #0: c218e488 (&dev->mutex){....}-{4:4}, at: __driver_attach+0xd8/0xf8
>      #1: c2201038 (&ch->lock){....}-{2:2}, at: sh_cmt_probe+0x598/0x780
>     stack backtrace:
>     CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted
> 6.17.0-rc5-armadillo-06055-g57d9d685e295 #872 NONE
>     Hardware name: Generic R8A7740 (Flattened Device Tree)
>     Call trace:
>      unwind_backtrace from show_stack+0x10/0x14
>      show_stack from dump_stack_lvl+0x44/0x74
>      dump_stack_lvl from __lock_acquire+0x41c/0x17e8
>      __lock_acquire from lock_acquire+0x208/0x28c
>      lock_acquire from _raw_spin_lock_irqsave+0x50/0x64
> 
>    - _raw_spin_lock_irqsave from __pm_runtime_resume+0x54/0x80
>    - __pm_runtime_resume from sh_cmt_start+0x60/0x26c
>    - sh_cmt_start from sh_cmt_clock_event_set_state+0x60/0xe8
>    - sh_cmt_clock_event_set_state from clockevents_switch_state+0x90/0x138
>    - clockevents_switch_state from clockevents_register_device+0x78/0xe8
>    - clockevents_register_device from sh_cmt_probe+0x588/0x700
>    + _raw_spin_lock_irqsave from clk_enable_lock+0x38/0xc4
>    + clk_enable_lock from clk_core_enable_lock+0xc/0x2c
>    + clk_core_enable_lock from sh_cmt_enable+0x28/0x1c8
>    + sh_cmt_enable from sh_cmt_probe+0x5a4/0x780
> 
>      sh_cmt_probe from platform_probe+0x58/0x90
>      platform_probe from really_probe+0x128/0x28c
>      really_probe from __driver_probe_device+0x16c/0x18c
>      __driver_probe_device from driver_probe_device+0x2c/0xa8
>      driver_probe_device from __driver_attach+0xe4/0xf8
>      __driver_attach from bus_for_each_dev+0x84/0xc8
>      bus_for_each_dev from bus_add_driver+0xd0/0x1d8
>      bus_add_driver from driver_register+0xb8/0x100
>      driver_register from do_one_initcall+0x74/0x1cc
>      do_one_initcall from kernel_init_freeable+0x224/0x294
>      kernel_init_freeable from kernel_init+0x14/0x12c
>      kernel_init from ret_from_fork+0x14/0x28
>     Exception stack(0xe0851fb0 to 0xe0851ff8)
>     1fa0:                                     00000000 00000000
> 00000000 00000000
>     1fc0: 00000000 00000000 00000000 00000000 00000000 00000000
> 00000000 00000000
>     1fe0: 00000000 00000000 00000000 00000000 00000013 00000000
> 
> 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

-- 
Kind Regards,
Niklas Söderlund

Powered by blists - more mailing lists