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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d5bbf98e-7dff-436e-9759-0d809072202f@lunn.ch>
Date: Thu, 2 Jan 2025 14:52:18 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Francesco Valla <francesco@...la.it>
Cc: "Russell King (Oracle)" <linux@...linux.org.uk>,
	Andrew Lunn <andrew+netdev@...n.ch>,
	Heiner Kallweit <hkallweit1@...il.com>, netdev@...r.kernel.org
Subject: Re: [PATCH] net: phy: don't issue a module request if a driver is
 available

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?

	Andrew