When the ID of an Ethernet PHY is not provided by the 'compatible'
string in the device tree, its actual ID is read via the MDIO bus.
For some PHYs this could be unsafe, since a hard reset may be
necessary to safely access the MDIO registers.

Add a fallback mechanism for these devices: when reading the
ID fails, the reset will be asserted, and the ID read is retried.

Signed-off-by: Buday Csaba <buday.csaba@prolan.hu>
---
V4 -> V5:
 - when fwnode_reset_phy() returns with -EPROBE_DEFER, that error
   is now propagated upstream instead of being discarded.
 - info message removed (it will be a separate commit)
 - 'err' variable changed to 'rc' in fwnode_reset_phy()
 - removed last fwnode_handle_put() in fwnode_reset_phy()
   The cleanup is performed by mdio_device_free().
V3 -> V4: dropped DT property `phy-id-read-needs-reset` to control
	  the reset behaviour, asserting reset as a fallback
	  mechanism instead. Added info level message for resetting.
V2 -> V3: kernel-doc replaced with a comment (fixed warning)
V1 -> V2:
 - renamed fwnode_reset_phy_before_probe() to
   fwnode_reset_phy()
 - added kernel-doc for fwnode_reset_phy()
 - improved error handling in fwnode_reset_phy()
---
 drivers/net/mdio/fwnode_mdio.c | 38 +++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
index ba7091518..9669da2b7 100644
--- a/drivers/net/mdio/fwnode_mdio.c
+++ b/drivers/net/mdio/fwnode_mdio.c
@@ -114,6 +114,34 @@ int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio,
 }
 EXPORT_SYMBOL(fwnode_mdiobus_phy_device_register);
 
+/* 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);
+	device_set_node(&tmpdev->dev, phy_node);
+	rc = mdio_device_register_reset(tmpdev);
+	if (rc) {
+		mdio_device_free(tmpdev);
+		return rc;
+	}
+
+	mdio_device_reset(tmpdev, 1);
+	mdio_device_reset(tmpdev, 0);
+
+	mdio_device_unregister_reset(tmpdev);
+	mdio_device_free(tmpdev);
+
+	return 0;
+}
+
 int fwnode_mdiobus_register_phy(struct mii_bus *bus,
 				struct fwnode_handle *child, u32 addr)
 {
@@ -129,8 +157,16 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
 		return PTR_ERR(mii_ts);
 
 	is_c45 = fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45");
-	if (is_c45 || fwnode_get_phy_id(child, &phy_id))
+	if (is_c45 || fwnode_get_phy_id(child, &phy_id)) {
 		phy = get_phy_device(bus, addr, is_c45);
+		if (IS_ERR(phy)) {
+			rc = fwnode_reset_phy(bus, addr, child);
+			if (rc == -EPROBE_DEFER)
+				goto clean_mii_ts;
+			else if (!rc)
+				phy = get_phy_device(bus, addr, is_c45);
+		}
+	}
 	else
 		phy = phy_device_create(bus, addr, phy_id, 0, NULL);
 	if (IS_ERR(phy)) {
-- 
2.39.5

> +/* 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()?

Also, fwnode_handle_get() returns a handle. Why do you throw it away?
What is the point of this get?

> +	device_set_node(&tmpdev->dev, phy_node);
> +	rc = mdio_device_register_reset(tmpdev);
> +	if (rc) {
> +		mdio_device_free(tmpdev);
> +		return rc;
> +	}
> +
> +	mdio_device_reset(tmpdev, 1);
> +	mdio_device_reset(tmpdev, 0);
> +
> +	mdio_device_unregister_reset(tmpdev);
> +	mdio_device_free(tmpdev);
> +
> +	return 0;
> +}
> +

	Andrew

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);

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.

> > +	device_set_node(&tmpdev->dev, phy_node);
> > +	rc = mdio_device_register_reset(tmpdev);
> > +	if (rc) {
> > +		mdio_device_free(tmpdev);
> > +		return rc;
> > +	}
> > +
> > +	mdio_device_reset(tmpdev, 1);
> > +	mdio_device_reset(tmpdev, 0);
> > +
> > +	mdio_device_unregister_reset(tmpdev);
> > +	mdio_device_free(tmpdev);
> > +
> > +	return 0;
> > +}
> > +
> 
> 	Andrew
> 

Csaba

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.

> 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

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