RE: [PATCH 1/3] net: macb: fix for fixed-link mode

From: Milind Parab
Date: Tue Dec 10 2019 - 04:14:50 EST


>> 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@xxxxxxxxxxx>
>> ---
>> 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