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

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

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

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.

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



The aplic_syscore_registered variable retains the __ro_after_init attribute,

which places it in a memory section that becomes strictly read-only after

the kernel initialization phase completes.



Platform driver probes can be deferred (for instance, waiting for an MSI

domain) and run in a workqueue after the init phase is done.



If a late probe calls aplic_syscore_init(), atomic_cmpxchg() will attempt

to write 1 to this read-only memory page, which causes a store fault (or

AMO fault on RISC-V).



Does the __ro_after_init attribute need to be removed here to safely

support late and deferred probing?

---cut---

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

> 
>> diff --git a/drivers/irqchip/irq-riscv-aplic-main.c b/drivers/irqchip/irq-riscv-aplic-main.c
>> index d9afb6ae98cf56d6fd9f858c31a3d23546c3b450..54e1d5602f15a04127e900c0a4983022d49da553 100644
>> --- a/drivers/irqchip/irq-riscv-aplic-main.c
>> +++ b/drivers/irqchip/irq-riscv-aplic-main.c
>> @@ -116,14 +116,12 @@ static struct syscore aplic_syscore = {
>>  	.ops = &aplic_syscore_ops,
>>  };
>>  
>> -static bool aplic_syscore_registered __ro_after_init;
>> +static atomic_t aplic_syscore_registered = ATOMIC_INIT(0);
>>  
>>  static void aplic_syscore_init(void)
>>  {
>> -	if (!aplic_syscore_registered) {
>> +	if (!atomic_cmpxchg(&aplic_syscore_registered, 0, 1))
>>  		register_syscore(&aplic_syscore);
>> -		aplic_syscore_registered = true;
>> -	}
>>  }
>>  
>>  static int aplic_pm_notifier(struct notifier_block *nb, unsigned long action, void *data)
>>Thanks, Vladimir