Message-ID: <Z3fJQEVV4ACpvP3L@ryzen>
Date: Fri, 3 Jan 2025 12:25:52 +0100
From: Niklas Cassel <cassel@...nel.org>
To: Andrew Lunn <andrew@...n.ch>
Cc: Francesco Valla <francesco@...la.it>,
	"Russell King (Oracle)" <linux@...linux.org.uk>,
	Andrew Lunn <andrew+netdev@...n.ch>,
	Heiner Kallweit <hkallweit1@...il.com>, netdev@...r.kernel.org,
	Anand Moon <linux.amoon@...il.com>
Subject: 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:
> > On Thursday, 2 January 2025 at 12:06:15 Russell King (Oracle) <linux@...linux.org.uk> wrote:
> > > On Thu, Jan 02, 2025 at 12:51:22AM +0100, Francesco Valla wrote:
> > > > 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.
> 
> > 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/
> > 
> > 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

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

Powered by blists - more mailing lists