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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 10 Dec 2019 09:14:13 +0000
From:   Milind Parab <mparab@...ence.com>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>
CC:     "nicolas.nerre@...rochip.com" <nicolas.nerre@...rochip.com>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "antoine.tenart@...tlin.com" <antoine.tenart@...tlin.com>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "hkallweit1@...il.com" <hkallweit1@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Dhananjay Vilasrao Kangude <dkangude@...ence.com>,
        "a.fatoum@...gutronix.de" <a.fatoum@...gutronix.de>,
        "brad.mouring@...com" <brad.mouring@...com>,
        Parshuram Raju Thombare <pthombar@...ence.com>
Subject: RE: [PATCH 1/3] net: macb: fix for fixed-link mode

>> 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@...ence.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