Re: [PATCH v3] PCI: dw-rockchip: Enable async probe by default

[Robin Murphy <robin.murphy@arm.com>]

On 2026-03-11 9:09 pm, Danilo Krummrich wrote:
> On Wed Mar 11, 2026 at 1:28 PM CET, Manivannan Sadhasivam wrote:
>> On Wed, Mar 11, 2026 at 12:46:03PM +0100, Danilo Krummrich wrote:
>>> On Wed Mar 11, 2026 at 6:24 AM CET, Manivannan Sadhasivam wrote:
>>>> I have a contrary view here. If just a single driver or lib doesn't handle async
>>>> probe, it cannot just force other drivers to not take the advantage of async
>>>> probe. As I said above, enabling async probe easily saves a few hunderd ms or
>>>> even more if there are more than one Root Port or Root Complex in an SoC.
>>>
>>> Then the driver or lib has to be fixed / improved first or the driver core has
>>> to be enabled to deal with a case where PROBE_FORCE_SYNCHRONOUS is requested
>>> from an async path, etc.
>>>
>>> In any case, applying the patch and breaking things (knowingly?) doesn't seem
>>> like the correct approach.
>>>
>>>> I strongly agree with you here that the underlying issue should be fixed. But
>>>> the real impact to end users is not this splat, but not having the boot time
>>>> optimization that this patch brings in. As an end user, one would want their
>>>> systems to boot quickly and they wouldn't bother much about a harmless warning
>>>> splat appearing in the dmesg log.
>>>
>>> You mean quickly booting into a "harmless" potential deadlock condition the
>>> warning splat tries to make people aware of? :)
>>>
>>
>> Hmm, I overlooked the built-as-module part where the deadlock could be possible
>> as indicated by the comment about the WARN_ON_ONCE().
>>
>> But what is the path forward here? Do you want the phylib to fix the
>> request_module() call or fix the driver core instead?
> 
> Here are a few thoughts.
> 
> In general, I think the best would be to get rid of the (affected)
> PROBE_FORCE_SYNCHRONOUS cases.
> 
> Now, I guess this can be pretty hard for a PCI controller driver, as you can't
> really predict what ends up being probed from you async context, i.e. it could
> even be some other bus controller and things could even propagate further.
> 
> Not sure how big of a deal it is in practice though, there are not a lot of
> PROBE_FORCE_SYNCHRONOUS drivers (left), but note that specifying neither
> PROBE_FORCE_SYNCHRONOUS nor PROBE_PREFER_ASYNCHRONOUS currently results in
> synchronous by default.
> 
> (Also, quite some other PCI controller drivers do set PROBE_PREFER_ASYNCHRONOUS
> and apparently got lucky with it.)
> 
>  From a driver-core perspective I think we're rather limited on what we can do;
> we are already in async context at this point and can't magically go back to
> initcall context.
> 
> So, the only thing I can think of is to kick off work on a workqueue, which in
> the end would be the same as the deferred probe handling.

Hmm, in fact, isn't the deferred probe mechanism itself actually quite
appropriate? A suitable calling context isn't the most obvious "resource
provider" to wait for, but ultimately it's still a case of "we don't
have everything we need right now, but it's worth trying again soon".
I may have missed some subtleties, but my instinct is that it could
perhaps be as simple as something like this (completely untested).

Cheers,
Robin.

----->8-----

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index bea8da5f8a3a..3c4a0207ae3f 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -954,6 +954,16 @@ static int __device_attach_driver(struct device_driver *drv, void *_data)
  	if (data->check_async && async_allowed != data->want_async)
  		return 0;
  
+	/*
+	 * Bus drivers may probe asynchronously, but be adding a child device
+	 * whose driver still wants a synchronous probe. In this case, just
+	 * defer it, to be triggered by the parent driver probe succeeding.
+	 */
+	if (!async_allowed && current_is_async()) {
+		driver_deferred_probe_add(dev);
+		return 0;
+	}
+
  	/*
  	 * Ignore errors returned by ->probe so that the next driver can try
  	 * its luck.