linux-kernel - Re: [PATCH net-next v2 1/1] net: mdio: reset PHY before attempting to access ID register

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] [day] [month] [year] [list]

Message-ID: <aTBeVTlsElGXUCSN@shell.armlinux.org.uk>
Date: Wed, 3 Dec 2025 15:59:17 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Andrew Lunn <andrew@...n.ch>
Cc: Buday Csaba <buday.csaba@...lan.hu>,
	Heiner Kallweit <hkallweit1@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v2 1/1] net: mdio: reset PHY before attempting
 to access ID register

On Tue, Dec 02, 2025 at 02:46:42PM +0100, Andrew Lunn wrote:
> > @@ -13,9 +13,12 @@
> >  #include <linux/phy.h>
> >  #include <linux/pse-pd/pse.h>
> >  
> > +#include "../phy/mdio-private.h"
> 
> Relative imports are generally not allowed. It sometimes suggests
> layering violations.
> 
> I'm not convinced the complexity and ugliness of partially registering
> a device in order to be able to reset it, is worth the effort when we
> have a simpler solution of just using the ID values in DT.
> 
> Give the merge window, i will mark this as change request for the
> moment.

It seems to me that PHY clocks and resets are a never-ending source of
problems, so I'm wondering whether we need to consider a radically
different solution, rather than keeping trying to put sticky plasters
over this in various different forms.

__mdiobus_register() is a path where deferred probing is possible,
and this is responsible for doing the bus scan, which is one source
of the problems.

If we add a new ops structure for firmware methods:

struct mii_bus_fw_ops {
	int (*init)(struct mii_bus *bus);
	int (*prescan)(struct mii_bus *bus);
	void (*postscan)(struct mii_bus *bus);
	void (*release)(struct mii_bus *bus);
};

This would be provided when we have e.g. a MII bus described in DT.

The init() method would be called by __mdiobus_register() before:

        err = device_register(&bus->dev);

and this method would:
- walk the sub-nodes of the bus, looking for reset and clock resources.
- it gets the reset and clock resources that are found.
- any that return deferred probe can safely propagate that error code.

__mdiobus_register() would then do its reset stuff, and then before
doing the scans, call the prescan() method. This will:

- walk the resources found in the init() method, noting the initial
  state of resets, then enabling clocks and finally releasing any
  resets.

During the scan, if a device is probed, then we need to consider how
to handle a potential updated reset state. Unlike clocks, which are
nestable, resets aren't. We don't want to re-assert a reset signal
if the PHY driver has specifically asked for it to be deasserted in
its probe() method.

After the bus scans have completed, then the postscan() op would be
called, and this will:

- walk the resources again, restoring the reset state (note the
  comment above concerning PHY driver probe()) and disabling
  any clocks (if the clock has already been "got" by something, e.g.
  a driver that probed as a result of successful scan, this doesn't
  affect it.)

The release() method would be called whenever we are unregistering
the bus (after device_del(&bus->dev) in __mdiobus_register() and
mdiobus_unregister()).

Maybe we should also consider whether phylib should have functions
for PHY drivers to call to control these resources that it has
obtained for each PHY device too?

Yes, this is a bigger change, but I think it should put a stop to
the dribble of problems that seem to be cropping up around the bus
scanning. It can be easily extended for other resources (e.g.
regulators) that are needed to scan and probe, and should also
avoid the need for relative-path includes.

If there are buses that we need to actively assert the reset, then
I feel that this should itself be a firmware property of the device,
as there are likely platforms that come up with the PHY already
released from reset and in a functional state, and causing their
link to drop by forcing a reset of the PHY could be undesirable.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!