Message-ID: <ff213c29-2a17-4776-a575-35e398e560ec@linaro.org>
Date: Tue, 25 Mar 2025 19:38:12 +0100
From: Daniel Lezcano <daniel.lezcano@...aro.org>
To: Krzysztof Kozlowski <krzk@...nel.org>, tglx@...utronix.de
Cc: linux-kernel@...r.kernel.org, Thomas Fossati <thomas.fossati@...aro.org>,
 Larisa Grigore <Larisa.Grigore@....com>,
 Ghennadi Procopciuc <ghennadi.procopciuc@....com>,
 Maxime Coquelin <mcoquelin.stm32@...il.com>,
 Alexandre Torgue <alexandre.torgue@...s.st.com>,
 "moderated list:ARM/STM32 ARCHITECTURE"
 <linux-stm32@...md-mailman.stormreply.com>,
 "moderated list:ARM/STM32 ARCHITECTURE"
 <linux-arm-kernel@...ts.infradead.org>, dl-S32 <S32@....com>
Subject: Re: [PATCH 2/2] clocksource/drivers/nxp-timer: Add the System Timer
 Module for the s32g platform

On 25/03/2025 13:30, Krzysztof Kozlowski wrote:
> On 25/03/2025 13:23, Daniel Lezcano wrote:

[ ... ]

>>>> +	if (!stm_instances->clocksource && (stm_instances->features & STM_CLKSRC)) {
>>>> +
>>>> +		/*
>>>> +		 * First probed STM will be a clocksource
>>>> +		 */
>>>> +		ret = nxp_stm_clocksource_init(dev, name, base, clk);
>>>> +		if (ret)
>>>> +			return ret;
>>>> +		stm_instances->clocksource++;
>>>
>>> That's racy. Devices can be brought async, ideally. This should be
>>> rather idr or probably entire structure protected with a mutex.
>>
>> Mmh, interesting. I never had to think about this problem before
>>
>> Do you know at what moment the probing is parallelized ?
> 
> You don't have PROBE_PREFER_ASYNCHRONOUS, so currently this will be
> still sync, but I don't think we want it to be that way forever. I think
> new drivers should not rely on implicit sync, because converting it
> later to async will be difficult. It's easier to design it now or even
> choose async explicitly (after testing).

I gave a try and sometimes I reach the warnings below. I suspect the 
underlying code in the time framework is not yet ready for that.

Even if it could be a good candidate for parallelizing the boot, this 
driver should stay sync ATM. Except if someone has the willing to dig 
into the core framework to find out the race when switching the 
clockevent. I think a thread is setting a timer while we are switching 
the driver.

IMO, this core framework is too sensitive for this kind of change now.

Alternatively, I can put anyway the lock which is harmless for the sync 
code but making the driver race free. The async flag can be put later.


[    2.066807] ------------[ cut here ]------------
[    2.073190] SPI driver st-magn-spi has no spi_device_id for 
st,lsm9ds1-magn
[    2.077841] Current state: 0
[    2.077866] WARNING: CPU: 0 PID: 0 at kernel/time/clockevents.c:319 
clockevents_program_event+0x124/0x12c
[    2.084972] SPI driver st-magn-spi has no spi_device_id for 
st,lsm303c-magn
[    2.087876] CPU: 0 UID: 0 PID: 0 Comm: swapper/0 Not tainted 
6.14.0-rc4-00009-g1418f8fd0e24-dirty #163
[    2.087889] Hardware name: NXP S32G2 Reference Design Board 2 
(S32G-VNP-RDB2) (DT)
[    2.121868] pstate: 600000c5 (nZCv daIF -PAN -UAO -TCO -DIT -SSBS 
BTYPE=--)
[    2.128959] pc : clockevents_program_event+0x124/0x12c
[    2.134196] lr : clockevents_program_event+0x124/0x12c
[    2.139437] sp : ffff800080003e50
[    2.142808] x29: ffff800080003e50 x28: ffff00085f899538 x27: 
ffff00085f899578
[    2.150092] x26: ffff00085f8995b8 x25: ffff00085f89948c x24: 
7fffffffffffffff
[    2.157368] x23: 0000000000000003 x22: 000000007233f8db x21: 
0000000000000000
[    2.164643] x20: 000000007270e000 x19: ffff0008001286c0 x18: 
00000000ffffffff
[    2.171918] x17: ffff8007dd240000 x16: ffff800080000000 x15: 
ffff8000800039f0
[    2.179195] x14: 0000000000000000 x13: ffff800082a864f8 x12: 
0000000000000381
[    2.186470] x11: 000000000000012b x10: ffff800082ade4f8 x9 : 
ffff800082a864f8
[    2.193746] x8 : 00000000ffffefff x7 : ffff800082ade4f8 x6 : 
80000000fffff000
[    2.201023] x5 : 0000000000000000 x4 : 0000000000000000 x3 : 
00000000ffffffff
[    2.208297] x2 : 0000000000000000 x1 : 0000000000000000 x0 : 
ffff800082a78080
[    2.215574] Call trace:
[    2.218063]  clockevents_program_event+0x124/0x12c (P)
[    2.223304]  tick_program_event+0x50/0x9c
[    2.227392]  hrtimer_interrupt+0x120/0x254
[    2.231571]  nxp_stm_clockevent_interrupt+0x8c/0x9c
[    2.236545]  __handle_irq_event_percpu+0x48/0x13c
[    2.241345]  handle_irq_event+0x4c/0xa8
[    2.245256]  handle_fasteoi_irq+0xa4/0x230
[    2.249431]  handle_irq_desc+0x40/0x58
[    2.253254]  generic_handle_domain_irq+0x1c/0x28
[    2.257961]  gic_handle_irq+0x4c/0x118
[    2.261783]  call_on_irq_stack+0x24/0x4c
[    2.265784]  do_interrupt_handler+0x80/0x84
[    2.270049]  el1_interrupt+0x34/0x68
[    2.273696]  el1h_64_irq_handler+0x18/0x24
[    2.277873]  el1h_64_irq+0x6c/0x70
[    2.281340]  default_idle_call+0x28/0x3c (P)
[    2.285693]  do_idle+0x1f8/0x250
[    2.288988]  cpu_startup_entry+0x34/0x3c
[    2.292988]  kernel_init+0x0/0x1d4
[    2.296454]  start_kernel+0x5e4/0x72c
[    2.300188]  __primary_switched+0x88/0x90
[    2.304283] ---[ end trace 0000000000000000 ]---
[    2.309891] hw perfevents: enabled with armv8_cortex_a53 PMU driver, 
7 (0,8000003f) counters available
[    2.320001]  cs_system_cfg: CoreSight Configuration manager initialised
[    2.327712] gnss: GNSS driver registered with major 506
[    2.340793] GACT probability NOT on
[    2.344401] Mirror/redirect action on
[    2.348835] IPVS: Registered protocols ()
[    2.352946] IPVS: Connection hash table configured (size=4096, 
memory=32Kbytes)
[    2.360566] IPVS: ipvs loaded.
[    2.363904] NET: Registered PF_INET6 protocol family

Or this one:

[    2.369946] ------------[ cut here ]------------
[    2.370044] Segment Routing with IPv6
[    2.374649] Current state: 0
[    2.374668] WARNING: CPU: 0 PID: 0 at kernel/time/clockevents.c:125 
clockevents_switch_state+0x10c/0x114
[    2.378462] In-situ OAM (IOAM) with IPv6
[    2.381328] CPU: 0 UID: 0 PID: 0 Comm: swapper/0 Tainted: G        W 
         6.14.0-rc4-00009-g1418f8fd0e24-dirty #163
[    2.381341] Tainted: [W]=WARN
[    2.391115] sit: IPv6, IPv4 and MPLS over IPv4 tunneling driver
[    2.394964] Hardware name: NXP S32G2 Reference Design Board 2 
(S32G-VNP-RDB2) (DT)
[    2.394971] pstate: 600000c5 (nZCv daIF -PAN -UAO -TCO -DIT -SSBS 
BTYPE=--)
[    2.394980] pc : clockevents_switch_state+0x10c/0x114
[    2.406609] NET: Registered PF_PACKET protocol family
[    2.408957] lr : clockevents_switch_state+0x10c/0x114
[    2.408971] sp : ffff800080003cf0
[    2.408975] x29: ffff800080003cf0
[    2.415066] Bridge firewalling registered
[    2.422692]  x28: ffff00085f89fc40 x27: 0000000000000001
[    2.422704] x26: ffff800082a6f0e0 x25: ffff80008265c400 x24: 
ffff00085f89fbc0
[    2.468656] x23: 0000000000000001 x22: ffff00085f899480 x21: 
0000000000000001
[    2.475931] x20: 0000000000000004 x19: ffff0008001286c0 x18: 
00000000ffffffff
[    2.483208] x17: ffff8007dd240000 x16: ffff800080000000 x15: 
ffff800080003890
[    2.490484] x14: 0000000000000000 x13: ffff800082a864f8 x12: 
0000000000000420
[    2.497763] x11: 0000000000000160 x10: ffff800082ade4f8 x9 : 
ffff800082a864f8
[    2.505038] x8 : 00000000ffffefff x7 : ffff800082ade4f8 x6 : 
80000000fffff000
[    2.512314] x5 : 0000000000000000 x4 : 0000000000000000 x3 : 
00000000ffffffff
[    2.519588] x2 : 0000000000000000 x1 : 0000000000000000 x0 : 
ffff800082a78080
[    2.526865] Call trace:
[    2.529352]  clockevents_switch_state+0x10c/0x114 (P)
[    2.534505]  tick_program_event+0x6c/0x9c
[    2.538590]  __remove_hrtimer+0xb0/0xb4
[    2.542506]  hrtimer_try_to_cancel.part.0+0xc8/0xd0
[    2.547478]  hrtimer_try_to_cancel+0x6c/0x78
[    2.551832]  task_contending+0xd4/0x13c
[    2.555746]  enqueue_dl_entity+0x214/0x500
[    2.559925]  dl_server_start+0x50/0x138
[    2.563835]  enqueue_task_fair+0x1dc/0x5e0
[    2.568012]  activate_task+0x4c/0x90
[    2.571660]  ttwu_do_activate.isra.0+0x58/0x138
[    2.576281]  sched_ttwu_pending+0xa4/0x128
[    2.580459]  __flush_smp_call_function_queue+0xf0/0x224
[    2.585790]  generic_smp_call_function_single_interrupt+0x14/0x20
[    2.592002]  ipi_handler+0x134/0x168
[    2.595650]  handle_percpu_devid_irq+0x80/0x120
[    2.600269]  handle_irq_desc+0x40/0x58
[    2.604091]  generic_handle_domain_irq+0x1c/0x28
[    2.608798]  gic_handle_irq+0x4c/0x118
[    2.612618]  call_on_irq_stack+0x24/0x4c
[    2.616617]  do_interrupt_handler+0x80/0x84
[    2.620881]  el1_interrupt+0x34/0x68
[    2.624526]  el1h_64_irq_handler+0x18/0x24
[    2.628700]  el1h_64_irq+0x6c/0x70
[    2.632166]  default_idle_call+0x28/0x3c (P)
[    2.636520]  do_idle+0x1f8/0x250
[    2.639812]  cpu_startup_entry+0x38/0x3c
[    2.643814]  kernel_init+0x0/0x1d4
[    2.647281]  start_kernel+0x5e4/0x72c
[    2.651014]  __primary_switched+0x88/0x90
[    2.655107] ---[ end trace 0000000000000000 ]---


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

Powered by blists - more mailing lists