Re: [PATCH v3] irqchip/riscv-aplic: fix asynchronous probing

[Vladimir Kondratiev <Vladimir.Kondratiev@mobileye.com>]

On 10/14/26 13:53, Sebastian Andrzej Siewior wrote:
> EXTERNAL EMAIL: Do not click any links or open any attachments unless you trust the sender and know the content is safe.
> 
> On 2026-10-14 11:15:05 [+0300], Vladimir Kondratiev wrote:
>> On 10/13/26 18:27, Sebastian Andrzej Siewior wrote:
>>> EXTERNAL EMAIL: Do not click any links or open any attachments unless you trust the sender and know the content is safe.
>>>
>>> On 2026-10-12 12:12:59 [+0300], Vladimir Kondratiev wrote:
>>>> boolean flag don't protect from race when probing several instances
>>>> asynchronously. On system with multiple APLICs, if probed asynchronously,
>>>> warning triggered as below.
>>>
>>> Why is this async? Doesn't __platform_driver_probe() set it to sync?
>>
>> Yes it does but aplic driver uses
>>
>> builtin_platform_driver(aplic_driver);
>>
>> that takes different route. Easy way to directly proof it: I boot with
>>
>> driver_async_probe=* dyndbg=\"func __driver_attach +p\"
>>
>> to request asynchronous probe and debug prints in the __driver_attach
>> shows:
>> [    0.236483] platform 48760000.interrupt-controller: probing driver riscv-aplic asynchronously
>> [    0.236513] platform 674e0000.interrupt-controller: probing driver riscv-aplic asynchronously
>> [    0.236539] platform 67560000.interrupt-controller: probing driver riscv-aplic asynchronously
> 
> Does "driver_async_probe=*" mark _all_ drivers as safe and you run into
> a problem where a driver isn't safe? Does the "dyndbg=" argument show
> the driver is probed asynchronously if you omit the "driver_async_probe"
> argument?

Not exactly, "driver_async_probe=*" in bootargs request only drivers that has
PROBE_DEFAULT_STRATEGY to be probed asynchronously.

aplic_driver don't set probe_type thus it defaults to PROBE_DEFAULT_STRATEGY,
quote exact definition from kdoc:
 * @PROBE_DEFAULT_STRATEGY: Used by drivers that work equally well
 *	whether probed synchronously or asynchronously.

If driver is no capable of async probing it should use PROBE_FORCE_SYNCHRONOUS.
3-rd option for probe_type is PROBE_PREFER_ASYNCHRONOUS

Direct test shows exactly this: that "dyndbg=" argument prints
"probing driver riscv-aplic asynchronously" if I set "driver_async_probe=*"
and prints nothing, meaning driver probed synchronously,
without "driver_async_probe=*"

> So the question is do we want to let it probe asynchronously or should
> we force it to sync.

I see no reason for PROBE_FORCE_SYNCHRONOUS to prohibit asynchronous probing.
code itself is capable of asynchronous probing. Preference for particular
platform can be specified in the bootargs.
> 
>>> Would you mind providing a coherent description of the problem and
>>> implemented solution instead of slapping console output?
>>
>> Sorry if it was not clear; shall I explain in details why pattern
>>
>> if (!done) {
>> 	do_something();
>> 	done = true;
>> }
>>
>> is not SMP safe? Add detailed explanation to commit message and shorten console output?
> 
> No console output at all. You need to coherently explain the problem
> without adding console output. This is a simple problem, there is no
> need to for a huge commit message here. Also there is no need for SMP
> here as we do have kernel preemption.

Got it. Will do. 

> 
>> Reason for asynchronous probing is simple - to reduce boot time, it is critical for embedded
>> systems. Think of system managing car rear camera and proximity sensors, it should be ready when you
>> start driving, this lefts very short time for the kernel to boot.
> 
> So the driver does not support async probing and you add it and call it
> a fix? I don't mind async probing but it should be declared what the
> actual intention is. And maybe the driver should be marked async in the
> end.

As above, drive declares it is agnostic to probe type, supporting both. So
I do think it is a fix.

>>>> ---
>>>> Changes in v3:
>>>> - drop __ro_after_init attribute due to write to read-only page concern
>>>
>>> What concern? That __ro_after_init can stay. It is built-in only.
>> See comment to v2 by sachiko-bot$kernel.org here
>> https://lore.kernel.org/all/sashiko-outbox-167856@kernel.org/
>> quote:
>> ---cut---
>> [Severity: High]
>>
>> Will this trigger a fatal page fault if the probe is deferred?
>>
> …
>> Platform driver probes can be deferred (for instance, waiting for an MSI
>>
>> domain) and run in a workqueue after the init phase is done.
> …
>> and indeed, aplic_msi_setup() can return -EPROBE_DEFER so concern is valid and
>> existed in original code, it is not related to asynchronous probing
> 
> I would not expect the MSI domain to be probed after kernel booting via
> a module. This is the scenario where this would explode.
> And if you consider this a problem you should address it and denote it
> in the commit description as such and not silently changing it as part
> of another change without any explanation.

OK, will add explanation for this.
This driver can't be a module, it is "builtin_platform_driver(aplic_driver);"
but still probe may be deferred if of node has "msi-parent", see
aplic_msi_setup() - it can return -EPROBE_DEFER;

Thanks, Vladimir