[PATCH net-next] mdio: mdiobus: setup of_node for the MDIO device

[PATCH net-next] mdio: mdiobus: setup of_node for the MDIO device

[Ioana Ciornei]

From: Ioana Ciornei <ioana.ciornei@nxp.com>

By mistake, the of_node of the MDIO device was not setup in the patch
linked below. As a consequence, any PHY driver that depends on the
of_node in its probe callback was not be able to successfully finish its
probe on a PHY, thus the Generic PHY driver was used instead.

Fix this by actually setting up the of_node.

Fixes: bc1bee3b87ee ("net: mdiobus: Introduce fwnode_mdiobus_register_phy()")
Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
 drivers/net/mdio/fwnode_mdio.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
index e96766da8de4..283ddb1185bd 100644
--- a/drivers/net/mdio/fwnode_mdio.c
+++ b/drivers/net/mdio/fwnode_mdio.c
@@ -65,6 +65,7 @@ int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio,
 	 * can be looked up later
 	 */
 	fwnode_handle_get(child);
+	phy->mdio.dev.of_node = to_of_node(child);
 	phy->mdio.dev.fwnode = child;
 
 	/* All data is now stored in the phy struct;
-- 
2.31.1

Re: [PATCH net-next] mdio: mdiobus: setup of_node for the MDIO device

[Andrew Lunn]

On Tue, Jun 15, 2021 at 06:44:01PM +0300, Ioana Ciornei wrote:
> From: Ioana Ciornei <ioana.ciornei@nxp.com>
> 
> By mistake, the of_node of the MDIO device was not setup in the patch
> linked below. As a consequence, any PHY driver that depends on the
> of_node in its probe callback was not be able to successfully finish its
> probe on a PHY

Do you mean the PHY driver was looking for things like RGMII delays,
skew values etc?

If the PHY driver fails to load because of missing OF properties, i
guess this means the PHY driver will also fail in an ACPI system?

      Andrew

Re: [PATCH net-next] mdio: mdiobus: setup of_node for the MDIO device

[Ioana Ciornei]

On Tue, Jun 15, 2021 at 06:19:26PM +0200, Andrew Lunn wrote:
> On Tue, Jun 15, 2021 at 06:44:01PM +0300, Ioana Ciornei wrote:
> > From: Ioana Ciornei <ioana.ciornei@nxp.com>
> > 
> > By mistake, the of_node of the MDIO device was not setup in the patch
> > linked below. As a consequence, any PHY driver that depends on the
> > of_node in its probe callback was not be able to successfully finish its
> > probe on a PHY
> 
> Do you mean the PHY driver was looking for things like RGMII delays,
> skew values etc?

In my case, the VSC8514 PHY driver was looking for the led modes
(vsc8531,led-%d-mode").

> 
> If the PHY driver fails to load because of missing OF properties, i
> guess this means the PHY driver will also fail in an ACPI system?
> 

Yes, it will.

The PHY drivers were not changed to use the fwnode_* calls
instead of the of_* ones. Unfortunately, I cannot test this with ACPI
since the boards that have this PHY (that I have access to) do not
support ACPI yet.

Ioana

Re: [PATCH net-next] mdio: mdiobus: setup of_node for the MDIO device

[Russell King (Oracle)]

On Tue, Jun 15, 2021 at 06:44:01PM +0300, Ioana Ciornei wrote:
> From: Ioana Ciornei <ioana.ciornei@nxp.com>
> 
> By mistake, the of_node of the MDIO device was not setup in the patch
> linked below. As a consequence, any PHY driver that depends on the
> of_node in its probe callback was not be able to successfully finish its
> probe on a PHY, thus the Generic PHY driver was used instead.
> 
> Fix this by actually setting up the of_node.
> 
> Fixes: bc1bee3b87ee ("net: mdiobus: Introduce fwnode_mdiobus_register_phy()")
> Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> ---
>  drivers/net/mdio/fwnode_mdio.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
> index e96766da8de4..283ddb1185bd 100644
> --- a/drivers/net/mdio/fwnode_mdio.c
> +++ b/drivers/net/mdio/fwnode_mdio.c
> @@ -65,6 +65,7 @@ int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio,
>  	 * can be looked up later
>  	 */
>  	fwnode_handle_get(child);
> +	phy->mdio.dev.of_node = to_of_node(child);
>  	phy->mdio.dev.fwnode = child;

Yes, this is something that was missed, but let's first look at what
other places to when setting up a device:

        pdev->dev.fwnode = pdevinfo->fwnode;
        pdev->dev.of_node = of_node_get(to_of_node(pdev->dev.fwnode));
        pdev->dev.of_node_reused = pdevinfo->of_node_reused;

        dev->dev.of_node = of_node_get(np);
        dev->dev.fwnode = &np->fwnode;

        dev->dev.of_node = of_node_get(node);
        dev->dev.fwnode = &node->fwnode;

That seems to be pretty clear that an of_node_get() is also needed.

Thanks.

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

Re: [PATCH net-next] mdio: mdiobus: setup of_node for the MDIO device

[Ioana Ciornei]

On Tue, Jun 15, 2021 at 06:13:31PM +0100, Russell King (Oracle) wrote:
> On Tue, Jun 15, 2021 at 06:44:01PM +0300, Ioana Ciornei wrote:
> > From: Ioana Ciornei <ioana.ciornei@nxp.com>
> > 
> > By mistake, the of_node of the MDIO device was not setup in the patch
> > linked below. As a consequence, any PHY driver that depends on the
> > of_node in its probe callback was not be able to successfully finish its
> > probe on a PHY, thus the Generic PHY driver was used instead.
> > 
> > Fix this by actually setting up the of_node.
> > 
> > Fixes: bc1bee3b87ee ("net: mdiobus: Introduce fwnode_mdiobus_register_phy()")
> > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> > ---
> >  drivers/net/mdio/fwnode_mdio.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
> > index e96766da8de4..283ddb1185bd 100644
> > --- a/drivers/net/mdio/fwnode_mdio.c
> > +++ b/drivers/net/mdio/fwnode_mdio.c
> > @@ -65,6 +65,7 @@ int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio,
> >  	 * can be looked up later
> >  	 */
> >  	fwnode_handle_get(child);
> > +	phy->mdio.dev.of_node = to_of_node(child);
> >  	phy->mdio.dev.fwnode = child;
> 
> Yes, this is something that was missed, but let's first look at what
> other places to when setting up a device:
> 
>         pdev->dev.fwnode = pdevinfo->fwnode;
>         pdev->dev.of_node = of_node_get(to_of_node(pdev->dev.fwnode));
>         pdev->dev.of_node_reused = pdevinfo->of_node_reused;
> 
>         dev->dev.of_node = of_node_get(np);
>         dev->dev.fwnode = &np->fwnode;
> 
>         dev->dev.of_node = of_node_get(node);
>         dev->dev.fwnode = &node->fwnode;
> 
> That seems to be pretty clear that an of_node_get() is also needed.
> 

I'm not convinced that an of_node_get() is needed besides the
fwnode_handle_get() call that's already there.

The fwnode_handle_get() will call the get callback for that particular
fwnode_handle. When we are in the OF case, the of_fwnode_get() will be
invoked which in turn does of_node_get().

Am I missing something here?

Ioana

Re: [PATCH net-next] mdio: mdiobus: setup of_node for the MDIO device

[Russell King (Oracle)]

On Tue, Jun 15, 2021 at 08:24:44PM +0300, Ioana Ciornei wrote:
> On Tue, Jun 15, 2021 at 06:13:31PM +0100, Russell King (Oracle) wrote:
> > On Tue, Jun 15, 2021 at 06:44:01PM +0300, Ioana Ciornei wrote:
> > > From: Ioana Ciornei <ioana.ciornei@nxp.com>
> > > 
> > > By mistake, the of_node of the MDIO device was not setup in the patch
> > > linked below. As a consequence, any PHY driver that depends on the
> > > of_node in its probe callback was not be able to successfully finish its
> > > probe on a PHY, thus the Generic PHY driver was used instead.
> > > 
> > > Fix this by actually setting up the of_node.
> > > 
> > > Fixes: bc1bee3b87ee ("net: mdiobus: Introduce fwnode_mdiobus_register_phy()")
> > > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> > > ---
> > >  drivers/net/mdio/fwnode_mdio.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
> > > index e96766da8de4..283ddb1185bd 100644
> > > --- a/drivers/net/mdio/fwnode_mdio.c
> > > +++ b/drivers/net/mdio/fwnode_mdio.c
> > > @@ -65,6 +65,7 @@ int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio,
> > >  	 * can be looked up later
> > >  	 */
> > >  	fwnode_handle_get(child);
> > > +	phy->mdio.dev.of_node = to_of_node(child);
> > >  	phy->mdio.dev.fwnode = child;
> > 
> > Yes, this is something that was missed, but let's first look at what
> > other places to when setting up a device:
> > 
> >         pdev->dev.fwnode = pdevinfo->fwnode;
> >         pdev->dev.of_node = of_node_get(to_of_node(pdev->dev.fwnode));
> >         pdev->dev.of_node_reused = pdevinfo->of_node_reused;
> > 
> >         dev->dev.of_node = of_node_get(np);
> >         dev->dev.fwnode = &np->fwnode;
> > 
> >         dev->dev.of_node = of_node_get(node);
> >         dev->dev.fwnode = &node->fwnode;
> > 
> > That seems to be pretty clear that an of_node_get() is also needed.
> > 
> 
> I'm not convinced that an of_node_get() is needed besides the
> fwnode_handle_get() call that's already there.
> 
> The fwnode_handle_get() will call the get callback for that particular
> fwnode_handle. When we are in the OF case, the of_fwnode_get() will be
> invoked which in turn does of_node_get().
> 
> Am I missing something here?

Hmm, I think you're actually correct - the other places increase the
OF node's refcount and then assign the fwnode, which is effectively
what will be happening here (since fwnode_handle_get() will do that
for us.)

However, there's definitely horrid stuff going on in this file with
refcounting:

fwnode_mdiobus_register_phy():

			phy_device_free(phy);
			fwnode_handle_put(phy->mdio.dev.fwnode);

phy_device_free() drops the refcount on the embedded struct device - it
_could_ free it, so we should be assuming that "phy" is dead at that
point - we should not be dereferencing it after the call.

fwnode_mdiobus_phy_device_register():

	fwnode_handle_get(child);
	phy->mdio.dev.fwnode = child;

	rc = phy_device_register(phy);
	if (rc) {
		fwnode_handle_put(child);
		return rc;

Here, we leave this function having dropped the fwnode refcount, but
we have left a dangling pointer to the fwnode in place. Hopefully,
no one will use that dangling pointer, but this is sloppy.

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

Re: [PATCH net-next] mdio: mdiobus: setup of_node for the MDIO device

[Andrew Lunn]

On Tue, Jun 15, 2021 at 06:13:31PM +0100, Russell King (Oracle) wrote:
> On Tue, Jun 15, 2021 at 06:44:01PM +0300, Ioana Ciornei wrote:
> > From: Ioana Ciornei <ioana.ciornei@nxp.com>
> > 
> > By mistake, the of_node of the MDIO device was not setup in the patch
> > linked below. As a consequence, any PHY driver that depends on the
> > of_node in its probe callback was not be able to successfully finish its
> > probe on a PHY, thus the Generic PHY driver was used instead.
> > 
> > Fix this by actually setting up the of_node.
> > 
> > Fixes: bc1bee3b87ee ("net: mdiobus: Introduce fwnode_mdiobus_register_phy()")
> > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> > ---
> >  drivers/net/mdio/fwnode_mdio.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
> > index e96766da8de4..283ddb1185bd 100644
> > --- a/drivers/net/mdio/fwnode_mdio.c
> > +++ b/drivers/net/mdio/fwnode_mdio.c
> > @@ -65,6 +65,7 @@ int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio,
> >  	 * can be looked up later
> >  	 */
> >  	fwnode_handle_get(child);
> > +	phy->mdio.dev.of_node = to_of_node(child);
> >  	phy->mdio.dev.fwnode = child;
> 
> Yes, this is something that was missed, but let's first look at what
> other places to when setting up a device:
> 
>         pdev->dev.fwnode = pdevinfo->fwnode;
>         pdev->dev.of_node = of_node_get(to_of_node(pdev->dev.fwnode));
>         pdev->dev.of_node_reused = pdevinfo->of_node_reused;
> 
>         dev->dev.of_node = of_node_get(np);
>         dev->dev.fwnode = &np->fwnode;
> 
>         dev->dev.of_node = of_node_get(node);
>         dev->dev.fwnode = &node->fwnode;
> 
> That seems to be pretty clear that an of_node_get() is also needed.

I think it also shows we have very little consistency, and the recent
patchset needs a bit of cleanup. Maybe yet another helper which when
passed a struct device * and a node pointer, it sets both values?

	 Andrew

Re: [PATCH net-next] mdio: mdiobus: setup of_node for the MDIO device

[Russell King (Oracle)]

On Tue, Jun 15, 2021 at 08:31:06PM +0200, Andrew Lunn wrote:
> On Tue, Jun 15, 2021 at 06:13:31PM +0100, Russell King (Oracle) wrote:
> > On Tue, Jun 15, 2021 at 06:44:01PM +0300, Ioana Ciornei wrote:
> > > From: Ioana Ciornei <ioana.ciornei@nxp.com>
> > > 
> > > By mistake, the of_node of the MDIO device was not setup in the patch
> > > linked below. As a consequence, any PHY driver that depends on the
> > > of_node in its probe callback was not be able to successfully finish its
> > > probe on a PHY, thus the Generic PHY driver was used instead.
> > > 
> > > Fix this by actually setting up the of_node.
> > > 
> > > Fixes: bc1bee3b87ee ("net: mdiobus: Introduce fwnode_mdiobus_register_phy()")
> > > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> > > ---
> > >  drivers/net/mdio/fwnode_mdio.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
> > > index e96766da8de4..283ddb1185bd 100644
> > > --- a/drivers/net/mdio/fwnode_mdio.c
> > > +++ b/drivers/net/mdio/fwnode_mdio.c
> > > @@ -65,6 +65,7 @@ int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio,
> > >  	 * can be looked up later
> > >  	 */
> > >  	fwnode_handle_get(child);
> > > +	phy->mdio.dev.of_node = to_of_node(child);
> > >  	phy->mdio.dev.fwnode = child;
> > 
> > Yes, this is something that was missed, but let's first look at what
> > other places to when setting up a device:
> > 
> >         pdev->dev.fwnode = pdevinfo->fwnode;
> >         pdev->dev.of_node = of_node_get(to_of_node(pdev->dev.fwnode));
> >         pdev->dev.of_node_reused = pdevinfo->of_node_reused;
> > 
> >         dev->dev.of_node = of_node_get(np);
> >         dev->dev.fwnode = &np->fwnode;
> > 
> >         dev->dev.of_node = of_node_get(node);
> >         dev->dev.fwnode = &node->fwnode;
> > 
> > That seems to be pretty clear that an of_node_get() is also needed.
> 
> I think it also shows we have very little consistency, and the recent
> patchset needs a bit of cleanup. Maybe yet another helper which when
> passed a struct device * and a node pointer, it sets both values?

I do like that idea - maybe a couple of helpers, one that takes the
of_node for a struct device, and another that takes a fwnode and
does the appropriate stuff.

Note that platform_device_release() does this:

	of_node_put(pa->pdev.dev.of_node);

which is currently fine, because platform devices appear to only
have their DT refcount increased. From what I can tell from looking
at drivers/acpi/arm64/iort.c, ACPI fwnodes don't look like they're
refcounted. Seems we're wading into something of a mess here. :(

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

Re: [PATCH net-next] mdio: mdiobus: setup of_node for the MDIO device

[Ioana Ciornei]

On Tue, Jun 15, 2021 at 10:09:07PM +0100, Russell King (Oracle) wrote:
> On Tue, Jun 15, 2021 at 08:31:06PM +0200, Andrew Lunn wrote:
> > On Tue, Jun 15, 2021 at 06:13:31PM +0100, Russell King (Oracle) wrote:
> > > On Tue, Jun 15, 2021 at 06:44:01PM +0300, Ioana Ciornei wrote:
> > > > From: Ioana Ciornei <ioana.ciornei@nxp.com>
> > > > 
> > > > By mistake, the of_node of the MDIO device was not setup in the patch
> > > > linked below. As a consequence, any PHY driver that depends on the
> > > > of_node in its probe callback was not be able to successfully finish its
> > > > probe on a PHY, thus the Generic PHY driver was used instead.
> > > > 
> > > > Fix this by actually setting up the of_node.
> > > > 
> > > > Fixes: bc1bee3b87ee ("net: mdiobus: Introduce fwnode_mdiobus_register_phy()")
> > > > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> > > > ---
> > > >  drivers/net/mdio/fwnode_mdio.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
> > > > index e96766da8de4..283ddb1185bd 100644
> > > > --- a/drivers/net/mdio/fwnode_mdio.c
> > > > +++ b/drivers/net/mdio/fwnode_mdio.c
> > > > @@ -65,6 +65,7 @@ int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio,
> > > >  	 * can be looked up later
> > > >  	 */
> > > >  	fwnode_handle_get(child);
> > > > +	phy->mdio.dev.of_node = to_of_node(child);
> > > >  	phy->mdio.dev.fwnode = child;
> > > 
> > > Yes, this is something that was missed, but let's first look at what
> > > other places to when setting up a device:
> > > 
> > >         pdev->dev.fwnode = pdevinfo->fwnode;
> > >         pdev->dev.of_node = of_node_get(to_of_node(pdev->dev.fwnode));
> > >         pdev->dev.of_node_reused = pdevinfo->of_node_reused;
> > > 
> > >         dev->dev.of_node = of_node_get(np);
> > >         dev->dev.fwnode = &np->fwnode;
> > > 
> > >         dev->dev.of_node = of_node_get(node);
> > >         dev->dev.fwnode = &node->fwnode;
> > > 
> > > That seems to be pretty clear that an of_node_get() is also needed.
> > 
> > I think it also shows we have very little consistency, and the recent
> > patchset needs a bit of cleanup. Maybe yet another helper which when
> > passed a struct device * and a node pointer, it sets both values?
> 
> I do like that idea - maybe a couple of helpers, one that takes the
> of_node for a struct device, and another that takes a fwnode and
> does the appropriate stuff.
> 

I agree. Some consistency would be needed here.
I'll submit something tomorrow.

On the other hand, I would like to keep this patch as it is and build on
top of it with the helpers that Andrew suggested.

> Note that platform_device_release() does this:
> 
> 	of_node_put(pa->pdev.dev.of_node);
> 
> which is currently fine, because platform devices appear to only
> have their DT refcount increased. From what I can tell from looking
> at drivers/acpi/arm64/iort.c, ACPI fwnodes don't look like they're
> refcounted. Seems we're wading into something of a mess here. :(
> 

The fwnode_operations declared in drivers/acpi/property.c also suggest
the ACPI fwnodes are not refcounted.

Ioana

Re: [PATCH net-next] mdio: mdiobus: setup of_node for the MDIO device

[Andrew Lunn]

> The fwnode_operations declared in drivers/acpi/property.c also suggest
> the ACPI fwnodes are not refcounted.

Is this because ACPI is not dynamic, unlike DT, where you can
add/remove overlays at runtime?

   Andrew

Re: [PATCH net-next] mdio: mdiobus: setup of_node for the MDIO device

[Ioana Ciornei]

On Tue, Jun 15, 2021 at 11:31:47PM +0200, Andrew Lunn wrote:
> > The fwnode_operations declared in drivers/acpi/property.c also suggest
> > the ACPI fwnodes are not refcounted.
> 
> Is this because ACPI is not dynamic, unlike DT, where you can
> add/remove overlays at runtime?
> 

I am really not an expert here but the git history suggests so, yes.

Without the CONFIG_OF_DYNAMIC enabled, the fwnode_handle_get() call is
actually a no-op even in the OF case.

Ioana

Re: [PATCH net-next] mdio: mdiobus: setup of_node for the MDIO device

[Russell King (Oracle)]

On Wed, Jun 16, 2021 at 11:20:52AM +0300, Ioana Ciornei wrote:
> On Tue, Jun 15, 2021 at 11:31:47PM +0200, Andrew Lunn wrote:
> > > The fwnode_operations declared in drivers/acpi/property.c also suggest
> > > the ACPI fwnodes are not refcounted.
> > 
> > Is this because ACPI is not dynamic, unlike DT, where you can
> > add/remove overlays at runtime?
> > 
> 
> I am really not an expert here but the git history suggests so, yes.
> 
> Without the CONFIG_OF_DYNAMIC enabled, the fwnode_handle_get() call is
> actually a no-op even in the OF case.

More accurately, of_node_get() is a no-op if CONFIG_OF_DYNAMIC is
disabled, which in turn makes fwnode_handle_get() also a no-op.

I'm wondering whether we would need two helpers to assign these, or
just a single helper that takes a fwnode and assigns both pointers.
to_of_node() returns NULL if the fwnode is not a DT node, so would
be safe to use even with ACPI.

Then there's the cleanup side when the device is released. I haven't
yet found where we release the reference to the fwnode/of_node when
we release the phy_device. I would have expected it in
phy_device_release() but that does nothing. We could only add that
when we are certain that all users who assign the firmware node to
the phy device has properly refcounted it in the DT case.

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

Re: [PATCH net-next] mdio: mdiobus: setup of_node for the MDIO device

[Ioana Ciornei]

On Wed, Jun 16, 2021 at 10:40:12AM +0100, Russell King (Oracle) wrote:
> On Wed, Jun 16, 2021 at 11:20:52AM +0300, Ioana Ciornei wrote:
> > On Tue, Jun 15, 2021 at 11:31:47PM +0200, Andrew Lunn wrote:
> > > > The fwnode_operations declared in drivers/acpi/property.c also suggest
> > > > the ACPI fwnodes are not refcounted.
> > > 
> > > Is this because ACPI is not dynamic, unlike DT, where you can
> > > add/remove overlays at runtime?
> > > 
> > 
> > I am really not an expert here but the git history suggests so, yes.
> > 
> > Without the CONFIG_OF_DYNAMIC enabled, the fwnode_handle_get() call is
> > actually a no-op even in the OF case.
> 
> More accurately, of_node_get() is a no-op if CONFIG_OF_DYNAMIC is
> disabled, which in turn makes fwnode_handle_get() also a no-op.
> 
> I'm wondering whether we would need two helpers to assign these, or
> just a single helper that takes a fwnode and assigns both pointers.
> to_of_node() returns NULL if the fwnode is not a DT node, so would
> be safe to use even with ACPI.
> 

Yes, I think this approach was exactly what Andrew suggested initially.

> Then there's the cleanup side when the device is released. I haven't
> yet found where we release the reference to the fwnode/of_node when
> we release the phy_device. I would have expected it in
> phy_device_release() but that does nothing.

Looking at the fixed_phy.c use of the refcounts, I would expect that a
call to fwnode_handle_put/of_node_put should be right after a
phy_device_remove() call is made.

	void fixed_phy_unregister(struct phy_device *phy)
	{
		phy_device_remove(phy);
		of_node_put(phy->mdio.dev.of_node);
		fixed_phy_del(phy->mdio.addr);
	}


Now going back to the phy_device.c, the phy_device_remove() call is done
in phy_mdio_device_remove. This is the device_remove callback of any PHY
MDIO device, called when, for example, the MDIO bus is unregistered.

After a first pass through the code, I would expect the refcount to be
released in phy_mdio_device_remove().

> We could only add that
> when we are certain that all users who assign the firmware node to
> the phy device has properly refcounted it in the DT case.
> 

Agree. I think we need a proper mapping of the register/unregister code
paths before any of_node/fwnode_handle put is added.

Ioana