| Message ID | 20210901225053.1205571-3-vladimir.oltean@nxp.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Make the PHY library stop being so greedy when binding the generic PHY driver | expand |
On Thu, Sep 02, 2021 at 01:50:52AM +0300, Vladimir Oltean wrote: > DSA supports connecting to a phy-handle, and has a fallback to a non-OF > based method of connecting to an internal PHY on the switch's own MDIO > bus, if no phy-handle and no fixed-link nodes were present. > > The -ENODEV error code from the first attempt (phylink_of_phy_connect) > is what triggers the second attempt (phylink_connect_phy). > > However, when the first attempt returns a different error code than > -ENODEV, this results in an unbalance of calls to phylink_create and > phylink_destroy by the time we exit the function. The phylink instance > has leaked. > > There are many other error codes that can be returned by > phylink_of_phy_connect. For example, phylink_validate returns -EINVAL. > So this is a practical issue too. > > Fixes: aab9c4067d23 ("net: dsa: Plug in PHYLINK support") > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > --- > I know, I will send this bug fix to "net" too, this is provided just for > testing purposes, and for the completeness of the patch set. Probably should have been the first patch of the set. This looks absolutely correct to me. Please send for the net tree. Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> Thanks.
On 9/1/2021 3:50 PM, Vladimir Oltean wrote: > DSA supports connecting to a phy-handle, and has a fallback to a non-OF > based method of connecting to an internal PHY on the switch's own MDIO > bus, if no phy-handle and no fixed-link nodes were present. > > The -ENODEV error code from the first attempt (phylink_of_phy_connect) > is what triggers the second attempt (phylink_connect_phy). > > However, when the first attempt returns a different error code than > -ENODEV, this results in an unbalance of calls to phylink_create and > phylink_destroy by the time we exit the function. The phylink instance > has leaked. > > There are many other error codes that can be returned by > phylink_of_phy_connect. For example, phylink_validate returns -EINVAL. > So this is a practical issue too. > > Fixes: aab9c4067d23 ("net: dsa: Plug in PHYLINK support") > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
diff --git a/net/dsa/slave.c b/net/dsa/slave.c index a7a114b9cb77..8a395290267c 100644 --- a/net/dsa/slave.c +++ b/net/dsa/slave.c @@ -1856,13 +1856,11 @@ static int dsa_slave_phy_setup(struct net_device *slave_dev) * use the switch internal MDIO bus instead */ ret = dsa_slave_phy_connect(slave_dev, dp->index, phy_flags); - if (ret) { - netdev_err(slave_dev, - "failed to connect to port %d: %d\n", - dp->index, ret); - phylink_destroy(dp->pl); - return ret; - } + } + if (ret) { + netdev_err(slave_dev, "failed to connect to PHY: %pe\n", + ERR_PTR(ret)); + phylink_destroy(dp->pl); } return ret;
DSA supports connecting to a phy-handle, and has a fallback to a non-OF based method of connecting to an internal PHY on the switch's own MDIO bus, if no phy-handle and no fixed-link nodes were present. The -ENODEV error code from the first attempt (phylink_of_phy_connect) is what triggers the second attempt (phylink_connect_phy). However, when the first attempt returns a different error code than -ENODEV, this results in an unbalance of calls to phylink_create and phylink_destroy by the time we exit the function. The phylink instance has leaked. There are many other error codes that can be returned by phylink_of_phy_connect. For example, phylink_validate returns -EINVAL. So this is a practical issue too. Fixes: aab9c4067d23 ("net: dsa: Plug in PHYLINK support") Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- I know, I will send this bug fix to "net" too, this is provided just for testing purposes, and for the completeness of the patch set. net/dsa/slave.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)