* [PATCH net-next] net: phylink: propagate phy_attach_direct() return code
@ 2019-12-12 17:16 Russell King
2019-12-15 4:27 ` Jakub Kicinski
0 siblings, 1 reply; 3+ messages in thread
From: Russell King @ 2019-12-12 17:16 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, Heiner Kallweit
Cc: Milind Parab, David S. Miller, netdev
of_phy_attach() hides the return value of phy_attach_direct(), forcing
us to return a "generic" ENODEV error code that is indistinguishable
from the lack-of-phy-property case.
Switch to using of_phy_find_device() to find the PHY device, and then
propagating any phy_attach_direct() error back to the caller.
Link: https://lore.kernel.org/lkml/20191210113829.GT25745@shell.armlinux.org.uk
Signed-off-by: Russell King <rmk+kernel@armlinux•org.uk>
---
drivers/net/phy/phylink.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index f7c660bf99d1..8d20cf3ba0b7 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -859,14 +859,17 @@ int phylink_of_phy_connect(struct phylink *pl, struct device_node *dn,
return 0;
}
- phy_dev = of_phy_attach(pl->netdev, phy_node, flags,
- pl->link_interface);
+ phy_dev = of_phy_find_device(phy_node);
/* We're done with the phy_node handle */
of_node_put(phy_node);
-
if (!phy_dev)
return -ENODEV;
+ ret = phy_attach_direct(pl->netdev, phy_dev, flags,
+ pl->link_interface);
+ if (ret)
+ return ret;
+
ret = phylink_bringup_phy(pl, phy_dev, pl->link_config.interface);
if (ret)
phy_detach(phy_dev);
--
2.20.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH net-next] net: phylink: propagate phy_attach_direct() return code
2019-12-12 17:16 [PATCH net-next] net: phylink: propagate phy_attach_direct() return code Russell King
@ 2019-12-15 4:27 ` Jakub Kicinski
2019-12-15 7:43 ` Russell King - ARM Linux admin
0 siblings, 1 reply; 3+ messages in thread
From: Jakub Kicinski @ 2019-12-15 4:27 UTC (permalink / raw)
To: Russell King
Cc: Andrew Lunn, Florian Fainelli, Heiner Kallweit, Milind Parab,
David S. Miller, netdev
On Thu, 12 Dec 2019 17:16:12 +0000, Russell King wrote:
> of_phy_attach() hides the return value of phy_attach_direct(), forcing
> us to return a "generic" ENODEV error code that is indistinguishable
> from the lack-of-phy-property case.
>
> Switch to using of_phy_find_device() to find the PHY device, and then
> propagating any phy_attach_direct() error back to the caller.
>
> Link: https://lore.kernel.org/lkml/20191210113829.GT25745@shell.armlinux.org.uk
> Signed-off-by: Russell King <rmk+kernel@armlinux•org.uk>
Applied thanks, the ref counting is not entirely obvious to a layman.
In your reply to Milind you said he can immediately of_node_put()
because the phy_dev is never deferenced in his code, but here it looks
like it is actually - the reference used to be given up after attach is
done, now its given up before attach_direct is called.
But I don't know how the refcounting here works, so applied, and on the
off chance the code is wrong follow up will be fine.
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index f7c660bf99d1..8d20cf3ba0b7 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -859,14 +859,17 @@ int phylink_of_phy_connect(struct phylink *pl, struct device_node *dn,
> return 0;
> }
>
> - phy_dev = of_phy_attach(pl->netdev, phy_node, flags,
> - pl->link_interface);
> + phy_dev = of_phy_find_device(phy_node);
> /* We're done with the phy_node handle */
> of_node_put(phy_node);
> -
> if (!phy_dev)
> return -ENODEV;
>
> + ret = phy_attach_direct(pl->netdev, phy_dev, flags,
> + pl->link_interface);
> + if (ret)
> + return ret;
> +
> ret = phylink_bringup_phy(pl, phy_dev, pl->link_config.interface);
> if (ret)
> phy_detach(phy_dev);
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH net-next] net: phylink: propagate phy_attach_direct() return code
2019-12-15 4:27 ` Jakub Kicinski
@ 2019-12-15 7:43 ` Russell King - ARM Linux admin
0 siblings, 0 replies; 3+ messages in thread
From: Russell King - ARM Linux admin @ 2019-12-15 7:43 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Andrew Lunn, Florian Fainelli, Heiner Kallweit, Milind Parab,
David S. Miller, netdev
On Sat, Dec 14, 2019 at 08:27:45PM -0800, Jakub Kicinski wrote:
> On Thu, 12 Dec 2019 17:16:12 +0000, Russell King wrote:
> > of_phy_attach() hides the return value of phy_attach_direct(), forcing
> > us to return a "generic" ENODEV error code that is indistinguishable
> > from the lack-of-phy-property case.
> >
> > Switch to using of_phy_find_device() to find the PHY device, and then
> > propagating any phy_attach_direct() error back to the caller.
> >
> > Link: https://lore.kernel.org/lkml/20191210113829.GT25745@shell.armlinux.org.uk
> > Signed-off-by: Russell King <rmk+kernel@armlinux•org.uk>
>
> Applied thanks, the ref counting is not entirely obvious to a layman.
> In your reply to Milind you said he can immediately of_node_put()
> because the phy_dev is never deferenced in his code, but here it looks
> like it is actually - the reference used to be given up after attach is
> done, now its given up before attach_direct is called.
The refcount is on the device_node - once we've got the phy device
itself (or failed to) we're not using the device_node anymore, so
we can put it directly after the of_phy_find_device() call.
>
> But I don't know how the refcounting here works, so applied, and on the
> off chance the code is wrong follow up will be fine.
>
> > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > index f7c660bf99d1..8d20cf3ba0b7 100644
> > --- a/drivers/net/phy/phylink.c
> > +++ b/drivers/net/phy/phylink.c
> > @@ -859,14 +859,17 @@ int phylink_of_phy_connect(struct phylink *pl, struct device_node *dn,
> > return 0;
> > }
> >
> > - phy_dev = of_phy_attach(pl->netdev, phy_node, flags,
> > - pl->link_interface);
> > + phy_dev = of_phy_find_device(phy_node);
> > /* We're done with the phy_node handle */
> > of_node_put(phy_node);
> > -
> > if (!phy_dev)
> > return -ENODEV;
> >
> > + ret = phy_attach_direct(pl->netdev, phy_dev, flags,
> > + pl->link_interface);
> > + if (ret)
> > + return ret;
> > +
> > ret = phylink_bringup_phy(pl, phy_dev, pl->link_config.interface);
> > if (ret)
> > phy_detach(phy_dev);
>
>
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-12-15 7:43 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-12-12 17:16 [PATCH net-next] net: phylink: propagate phy_attach_direct() return code Russell King
2019-12-15 4:27 ` Jakub Kicinski
2019-12-15 7:43 ` Russell King - ARM Linux admin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox