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

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

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?
So the question is do we want to let it probe asynchronously or should
we force it to sync.

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

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

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

Sebastian