netdev - Re: [PATCH net-next v5 3/4] net: mdio: reset PHY before attempting to access registers in fwnode_mdiobus_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: <aQMG1A7nPzpoaShr@debianbuilder>
Date: Thu, 30 Oct 2025 07:33:56 +0100
From: Buday Csaba <buday.csaba@...lan.hu>
To: Andrew Lunn <andrew@...n.ch>
CC: Heiner Kallweit <hkallweit1@...il.com>, Russell King
	<linux@...linux.org.uk>, "David S. Miller" <davem@...emloft.net>, "Eric
 Dumazet" <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
	<pabeni@...hat.com>, Philipp Zabel <p.zabel@...gutronix.de>,
	<netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next v5 3/4] net: mdio: reset PHY before attempting
 to access registers in fwnode_mdiobus_register_phy

On Wed, Oct 29, 2025 at 04:15:27PM +0100, Andrew Lunn wrote:
> On Wed, Oct 29, 2025 at 03:15:27PM +0100, Buday Csaba wrote:
> > On Wed, Oct 29, 2025 at 02:20:14PM +0100, Andrew Lunn wrote:
> > > > +/* Hard-reset a PHY before registration */
> > > > +static int fwnode_reset_phy(struct mii_bus *bus, u32 addr,
> > > > +			    struct fwnode_handle *phy_node)
> > > > +{
> > > > +	struct mdio_device *tmpdev;
> > > > +	int rc;
> > > > +
> > > > +	tmpdev = mdio_device_create(bus, addr);
> > > > +	if (IS_ERR(tmpdev))
> > > > +		return PTR_ERR(tmpdev);
> > > > +
> > > > +	fwnode_handle_get(phy_node);
> > > 
> > > You add a _get() here. Where is the corresponding _put()?
> > 
> > When mdio_device_free() is called, it eventually invokes
> > mdio_device_release(). There is the corresponding _put(), that will
> > release the reference. I also verified this with a stack trace.
> > 
> > > 
> > > Also, fwnode_handle_get() returns a handle. Why do you throw it away?
> > > What is the point of this get?
> > >
> > 
> > I copied this initialization stub from of_mdiobus_register_device()
> > in of_mdio.c. The same pattern is used there:
> > 
> > 	fwnode_handle_get(fwnode);
> > 	device_set_node(&mdiodev->dev, fwnode);
> 
> This looks broken, but i'm not sure...
> 
> static int of_mdiobus_register_device(struct mii_bus *mdio,
> 				      struct device_node *child, u32 addr)
> {
> 	struct fwnode_handle *fwnode = of_fwnode_handle(child);
> 	struct mdio_device *mdiodev;
> 	int rc;
> 
> 	mdiodev = mdio_device_create(mdio, addr);
> 	if (IS_ERR(mdiodev))
> 		return PTR_ERR(mdiodev);
> 
> 	/* Associate the OF node with the device structure so it
> 	 * can be looked up later.
> 	 */
> 	fwnode_handle_get(fwnode);
> 	device_set_node(&mdiodev->dev, fwnode);
> 
> 	/* All data is now stored in the mdiodev struct; register it. */
> 	rc = mdio_device_register(mdiodev);
> 	if (rc) {
> 		device_set_node(&mdiodev->dev, NULL);
> 		fwnode_handle_put(fwnode);
> 		mdio_device_free(mdiodev);
> 
> In this error handling, it appears the fwnode is put() and then the
> mdiodev freed. I assume that results in a call to
> mdio_device_release() which does a second put() on fwnode.
> 
> That is why code like this should look symmetric. If the put() is in
> free, the get() should be in the create.

I totally agree with that, but I have nothing to do with that code.
It did also confuse me at first, that is why my earlier versions also had
a put(), just not in the error handling path.

> 
> > It is kind of awkward that we need to half-establish a device, just
> > to assert the reset, but I could not think of any better solution, that
> > does not lead to a large amount of code duplication.
> 
> And this is another argument against this approach. What does the
> documentation say about what you can do with a half-established
> device?
> 
> 	Andrew
> 

But that device never actually leaves fwnode_reset_phy(). It is contained.
That was the whole point of the first patch: to avoid code duplication
as much as possible.
In order to assert and deassert the reset, you have many things to set up:
read DT properties, claim the GPIO or the reset controller (which is only
possible for a device, is it not?), then perform the actual
assertion/deassertion.
These functions currently exist for an mdio device, why not use them?

After these patches fwnode_reset_phy() is reasonably structured, at least
I think so. The temporary device is created, reset properties initialized,
reset performed, then cleaned up on exit.

I understand your concerns about the functionality itself. Yes, it may be
better handled by the driver, and it is just to gain a little convenience.
But that is more of a philosophical argument. If I send a next version,
I can not address that. I can only address technical concerns. If your
opinion is, that this feature is not wanted, then there is no point in
sending a next version.
I personally think that autodetection works reasonably well for most of the
devices, so expanding this set a little bit is a nice thing.
But that decision is ultimately for you -maintainers- to make.

I also think that the documentation should reflect clearly when and why
specifying the PHY ID in the DT is necessary, and wheter it is preferred
or not. I would be happy to make such a patch, once the decision is made.

That would have have saved us a lot of development time, and I imagine
there are others in the same shoes.

Thank you for the thorough review!
Csaba