| Message ID | 20210615154401.1274322-1-ciorneiioana@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] mdio: mdiobus: setup of_node for the MDIO device | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | fail | 1 blamed authors not CCed: grant.likely@arm.com; 1 maintainers not CCed: grant.likely@arm.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 7 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
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
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
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.
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
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.
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
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. :(
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
> 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
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
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.
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
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;