Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

clocksource/drivers/sh_cmt: Always leave device running after probe

The CMT device can be used as both a clocksource and a clockevent
provider. The driver tries to be smart and power itself on and off, as
well as enabling and disabling its clock when it's not in operation.
This behavior is slightly altered if the CMT is used as an early
platform device in which case the device is left powered on after probe,
but the clock is still enabled and disabled at runtime.

This has worked for a long time, but recent improvements in PREEMPT_RT
and PROVE_LOCKING have highlighted an issue. As the CMT registers itself
as a clockevent provider, clockevents_register_device(), it needs to use
raw spinlocks internally as this is the context of which the clockevent
framework interacts with the CMT driver. However in the context of
holding a raw spinlock the CMT driver can't really manage its power
state or clock with calls to pm_runtime_*() and clk_*() as these calls
end up in other platform drivers using regular spinlocks to control
power and clocks.

This mix of spinlock contexts trips a lockdep warning.

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

For non-PREEMPT_RT builds this is not really an issue, but for
PREEMPT_RT builds where normal spinlocks can sleep this might be an
issue. Be cautious and always leave the power and clock running after
probe.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
Link: https://patch.msgid.link/20251016182022.1837417-1-niklas.soderlund+renesas@ragnatech.se

authored by

Niklas Söderlund and committed by
Daniel Lezcano
62524f28 6b38a8b3

+3 -33
+3 -33
drivers/clocksource/sh_cmt.c
··· 355 355 356 356 dev_pm_syscore_device(&ch->cmt->pdev->dev, true); 357 357 358 - /* enable clock */ 359 - ret = clk_enable(ch->cmt->clk); 360 - if (ret) { 361 - dev_err(&ch->cmt->pdev->dev, "ch%u: cannot enable clock\n", 362 - ch->index); 363 - goto err0; 364 - } 365 - 366 358 /* make sure channel is disabled */ 367 359 sh_cmt_start_stop_ch(ch, 0); 368 360 ··· 376 384 if (ret || sh_cmt_read_cmcnt(ch)) { 377 385 dev_err(&ch->cmt->pdev->dev, "ch%u: cannot clear CMCNT\n", 378 386 ch->index); 379 - ret = -ETIMEDOUT; 380 - goto err1; 387 + return -ETIMEDOUT; 381 388 } 382 389 383 390 /* enable channel */ 384 391 sh_cmt_start_stop_ch(ch, 1); 385 392 return 0; 386 - err1: 387 - /* stop clock */ 388 - clk_disable(ch->cmt->clk); 389 - 390 - err0: 391 - return ret; 392 393 } 393 394 394 395 static void sh_cmt_disable(struct sh_cmt_channel *ch) ··· 391 406 392 407 /* disable interrupts in CMT block */ 393 408 sh_cmt_write_cmcsr(ch, 0); 394 - 395 - /* stop clock */ 396 - clk_disable(ch->cmt->clk); 397 409 398 410 dev_pm_syscore_device(&ch->cmt->pdev->dev, false); 399 411 } ··· 565 583 int ret = 0; 566 584 unsigned long flags; 567 585 568 - pm_runtime_get_sync(&ch->cmt->pdev->dev); 569 - 570 586 raw_spin_lock_irqsave(&ch->lock, flags); 571 587 572 588 if (!(ch->flags & (FLAG_CLOCKEVENT | FLAG_CLOCKSOURCE))) ··· 599 619 sh_cmt_disable(ch); 600 620 601 621 raw_spin_unlock_irqrestore(&ch->lock, flags); 602 - 603 - pm_runtime_put(&ch->cmt->pdev->dev); 604 622 } 605 623 606 624 static int sh_cmt_start_clockevent(struct sh_cmt_channel *ch) ··· 608 630 609 631 raw_spin_lock_irqsave(&ch->lock, flags); 610 632 611 - if (!(ch->flags & (FLAG_CLOCKEVENT | FLAG_CLOCKSOURCE))) { 612 - pm_runtime_get_sync(&ch->cmt->pdev->dev); 633 + if (!(ch->flags & (FLAG_CLOCKEVENT | FLAG_CLOCKSOURCE))) 613 634 ret = sh_cmt_enable(ch); 614 - } 615 635 616 636 if (ret) 617 637 goto out; ··· 632 656 633 657 ch->flags &= ~FLAG_CLOCKEVENT; 634 658 635 - if (f && !(ch->flags & (FLAG_CLOCKEVENT | FLAG_CLOCKSOURCE))) { 659 + if (f && !(ch->flags & (FLAG_CLOCKEVENT | FLAG_CLOCKSOURCE))) 636 660 sh_cmt_disable(ch); 637 - pm_runtime_put(&ch->cmt->pdev->dev); 638 - } 639 661 640 662 /* adjust the timeout to maximum if only clocksource left */ 641 663 if (ch->flags & FLAG_CLOCKSOURCE) ··· 1108 1134 mask &= ~(1 << hwidx); 1109 1135 } 1110 1136 1111 - clk_disable(cmt->clk); 1112 - 1113 1137 platform_set_drvdata(pdev, cmt); 1114 1138 1115 1139 return 0; ··· 1155 1183 out: 1156 1184 if (cmt->has_clockevent || cmt->has_clocksource) 1157 1185 pm_runtime_irq_safe(&pdev->dev); 1158 - else 1159 - pm_runtime_idle(&pdev->dev); 1160 1186 1161 1187 return 0; 1162 1188 }