Messages in this thread Patch in this message | | | From | Milind Parab <> | Subject | RE: [PATCH 1/3] net: macb: fix for fixed-link mode | Date | Tue, 10 Dec 2019 09:14:13 +0000 |
| |
>> This patch fix the issue with fixed link. With fixed-link >> device opening fails due to macb_phylink_connect not >> handling fixed-link mode, in which case no MAC-PHY connection >> is needed and phylink_connect return success (0), however >> in current driver attempt is made to search and connect to >> PHY even for fixed-link. >> >> Signed-off-by: Milind Parab <mparab@cadence.com> >> --- >> drivers/net/ethernet/cadence/macb_main.c | 17 ++++++++--------- >> 1 file changed, 8 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c >> index 9c767ee252ac..6b68ef34ab19 100644 >> --- a/drivers/net/ethernet/cadence/macb_main.c >> +++ b/drivers/net/ethernet/cadence/macb_main.c >> @@ -615,17 +615,13 @@ static int macb_phylink_connect(struct macb *bp) >> { >> struct net_device *dev = bp->dev; >> struct phy_device *phydev; >> + struct device_node *dn = bp->pdev->dev.of_node; >> int ret; >> >> - if (bp->pdev->dev.of_node && >> - of_parse_phandle(bp->pdev->dev.of_node, "phy-handle", 0)) { >> - ret = phylink_of_phy_connect(bp->phylink, bp->pdev- >>dev.of_node, >> - 0); >> - if (ret) { >> - netdev_err(dev, "Could not attach PHY (%d)\n", ret); >> - return ret; >> - } >> - } else { >> + if (dn) >> + ret = phylink_of_phy_connect(bp->phylink, dn, 0); >> + >> + if (!dn || (ret && !of_parse_phandle(dn, "phy-handle", 0))) { > >Hi, >If of_parse_phandle() returns non-null, the device_node it returns will >have its reference count increased by one. That reference needs to be >put. >
Okay, as per your suggestion below addition will be okay to store the "phy_node" and then of_node_put(phy_node) on error
phy_node = of_parse_phandle(dn, "phy-handle", 0); if (!dn || (ret && !phy_node)) { phydev = phy_find_first(bp->mii_bus); if (!phydev) { netdev_err(dev, "no PHY found\n"); ret = -ENXIO; goto phylink_connect_err; }
/* attach the mac to the phy */ ret = phylink_connect_phy(bp->phylink, phydev); if (ret) { netdev_err(dev, "Could not attach to PHY (%d)\n", ret); goto phylink_connect_err; } } else if (ret) { netdev_err(dev, "Could not attach PHY (%d)\n", ret); goto phylink_connect_err; }
phylink_start(bp->phylink);
phylink_connect_err: if (phy_node) of_node_put(phy_node);
return ret;
>I assume you're trying to determine whether phylink_of_phy_connect() >failed because of a missing phy-handle rather than of_phy_attach() >failing? Maybe those two failures ought to be distinguished by errno >return value?
Yes, PHY will be scanned only if phylink_of_phy_connect() returns error due to missing "phy-handle". Currently, phylink_of_phy_connect() returns same error for missing "phy-handle" and of_phy_attach() failure.
>of_phy_attach() may fail due to of_phy_find_device() failing to find >the PHY, or phy_attach_direct() failing. We could switch from using >of_phy_attach(), to using of_phy_find_device() directly so we can then >propagate phy_attach_direct()'s error code back, rather than losing it. >That would then leave the case of of_phy_find_device() failure to be >considered in terms of errno return value.
>> phydev = phy_find_first(bp->mii_bus); >> if (!phydev) { >> netdev_err(dev, "no PHY found\n"); >> @@ -638,6 +634,9 @@ static int macb_phylink_connect(struct macb *bp) >> netdev_err(dev, "Could not attach to PHY (%d)\n",ret); >> return ret; >> } >> + } else if (ret) { >> + netdev_err(dev, "Could not attach PHY (%d)\n", ret); >> + return ret; >> } >> >> phylink_start(bp->phylink); >> -- >> 2.17.1 >> >> >--RMK's Patch system: https://urldefense.proofpoint.com/v2/url?u=https- >3A__www.armlinux.org.uk_developer_patches_&d=DwIBAg&c=aUq983L2pue >2FqKFoP6PGHMJQyoJ7kl3s3GZ- >_haXqY&r=BDdk1JtITE_JJ0519WwqU7IKF80Cw1i55lZOGqv2su8&m=blnuaRbic >V2uF6XaoVuWN0U5yR5cFOzUSAs3ZPlxioU&s=rhp71ilc6R4_pmDsY07- >kLPGbhyoyixXoHF0hMGu4Go&e= > >FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps >up > >According to speedtest.net: 11.9Mbps down 500kbps up
| |