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 for Android: free password hash cracker in your pocket

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

Message-ID: <2105036.YKUYFuaPT4@fedora.fritz.box>
Date: Thu, 02 Jan 2025 15:01:46 +0100
From: Francesco Valla <francesco@...la.it>
To: Andrew Lunn <andrew@...n.ch>
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 Thursday, 2 January 2025 at 14:52:18 Andrew Lunn <andrew@...n.ch> 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.
> 

Ok, will do.

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

It probably isn't, but considering the way phylib works currently (with the
genphy filling up for missing drivers etc.) I'm not sure if/how this can work.
I need to think about it.

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

For me it's a reduction of ~170ms, which currently accounts for roughly the 25%
of the time spent before starting userspace (660ms vs 490ms, give or take a
couple of milliseconds). That's due to the large reset time required by the PHYs
to initialize, so I expect it would be much lower on most of the systems.
But, I've done much more to shave much less time in the past, so I think it is
at least worth investigating.

Thank you!

Regards,
Francesco

> 
> 	Andrew
>