Re: [PATCH 2/2] clocksource/drivers/nxp-timer: Add the System Timer Module for the s32g platform

[Krzysztof Kozlowski <krzk@kernel.org>]

On 25/03/2025 19:38, Daniel Lezcano wrote:
> 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.


Yes, that's what I meant, although indeed good point that clocksource is
way too early to be async. In that case this part is up to you, maybe my
suggestion was not correct.


Best regards,
Krzysztof