From: "Russell King (Oracle)" <linux@armlinux.org.uk>
To: Ioana Ciornei <ciorneiioana@gmail.com>
Cc: davem@davemloft.net, kuba@kernel.org, netdev@vger.kernel.org,
calvin.johnson@oss.nxp.com, andrew@lunn.ch, hkallweit1@gmail.com,
Ioana Ciornei <ioana.ciornei@nxp.com>
Subject: Re: [PATCH net-next] mdio: mdiobus: setup of_node for the MDIO device
Date: Tue, 15 Jun 2021 19:25:01 +0100 [thread overview]
Message-ID: <20210615182501.GX22278@shell.armlinux.org.uk> (raw)
In-Reply-To: <20210615172444.dirudehe3vzis2kw@skbuf>
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!
next prev parent reply other threads:[~2021-06-15 18:25 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-15 15:44 [PATCH net-next] mdio: mdiobus: setup of_node for the MDIO device Ioana Ciornei
2021-06-15 16:19 ` Andrew Lunn
2021-06-15 16:49 ` Ioana Ciornei
2021-06-15 17:13 ` Russell King (Oracle)
2021-06-15 17:24 ` Ioana Ciornei
2021-06-15 18:25 ` Russell King (Oracle) [this message]
2021-06-15 18:31 ` Andrew Lunn
2021-06-15 21:09 ` Russell King (Oracle)
2021-06-15 21:21 ` Ioana Ciornei
2021-06-15 21:31 ` Andrew Lunn
2021-06-16 8:20 ` Ioana Ciornei
2021-06-16 9:40 ` Russell King (Oracle)
2021-06-16 11:01 ` Ioana Ciornei
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210615182501.GX22278@shell.armlinux.org.uk \
--to=linux@armlinux.org.uk \
--cc=andrew@lunn.ch \
--cc=calvin.johnson@oss.nxp.com \
--cc=ciorneiioana@gmail.com \
--cc=davem@davemloft.net \
--cc=hkallweit1@gmail.com \
--cc=ioana.ciornei@nxp.com \
--cc=kuba@kernel.org \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.