Re: [PATCH] net: phy: don't issue a module request if a driver is available

On Thu, Jan 02, 2025 at 02:52:18PM +0100, Andrew Lunn wrote:

On Thu, Jan 02, 2025 at 02:26:58PM +0100, Francesco Valla wrote:

quoted

On Thursday, 2 January 2025 at 12:06:15 Russell King (Oracle) [off-list ref] wrote:

quoted

On Thu, Jan 02, 2025 at 12:51:22AM +0100, Francesco Valla wrote:

quoted

Whenever a new PHY device is created, request_module() is called
unconditionally, without checking if a driver for the new PHY is already
available (either built-in or from a previous probe). This conflicts
with async probing of the underlying MDIO bus and always throws a
warning (because if a driver is loaded it _might_ cause a deadlock, if
in turn it calls async_synchronize_full()).

Why aren't any of the phylib maintainers seeing this warning? Where does
the warning come from?

I'm not sure. For me, it was pretty easy to trigger.

Please include the information how you triggered it into the commit
message.

quoted

This is expected, as request_module() is not meant to be called from an async
context:

https://lore.kernel.org/lkml/20130118221227.GG24579@htj.dyndns.org/ (local)

It should be noted that:
 - the davincio_mdio device is a child of the am65-cpsw-nuss device
 - the am65-cpsw-nuss driver is NOT marked with neither PROBE_PREFER_ASYNCHRONOUS
   nor PROBE_FORCE_SYNCHRONOUS and the behavior is being triggered specifying
   driver_async_probe=am65-cpsw-nuss on the command line.

So the phylib core is currently async probe incompatible. The whole
module loading story is a bit shaky in phylib, so we need to be very
careful with any changes, or you are going to break stuff, in
interesting ways, with it first appearing to work, because the
fallback genphy is used rather than the specific PHY driver, but then
breaking when genphy is not sufficient.

Please think about this as a generic problem with async probe. Is this
really specific to phylib? Should some or all of the solution to the
problem be moved into the driver core? Could we maybe first try an
async probe using the existing drivers, and then fall back to a sync
probe which can load additional drivers?

One other question, how much speadup do you get with async probe of
PHYs? Is it really worth the effort?

I'm trying to enable async probe for my PCIe controller (pcie-dw-rockchip),
which on the radxa rock5b has a RTL8125 NIC connected to it.

By enabling async probe for the PCIe driver I get the same splat as Francesco.

Looking at the prints, it is trying to load a module for PHY ID: 0x1cc840
This PHY ID is defined in: drivers/net/phy/realtek.c.

Looking at my .config I have:
CONFIG_REALTEK_PHY=y

So this is not built as a module, so I am a bit surprised to see this
splat (since the driver is built as built-in).


I think it would be nice if the phylib core could be fixed so that
it does not try to load modules for drivers which are built as built-in.


Also see this old thread that tries to enable async probe by default on
DT systems:
https://lore.kernel.org/linux-kernel//d5796286-ec24-511a-5910-5673f8ea8b10@samsung.com/T/#u (local)

AFAICT, it seems that the phylib core is one of the biggest blockers from
being able to enable async probe by default on DT systems.


Kind regards,
Niklas