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]

Message-ID: <BY5PR07MB6514DEE80FD0C24F9FD52ED1D35A0@BY5PR07MB6514.namprd07.prod.outlook.com>
Date:   Wed, 11 Dec 2019 08:21:57 +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

>> >> +		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 (phy_node)
>>                 of_node_put(phy_node);
>
>As you're only interested in whether phy-handle exists or not, you
>could do this instead:
>
>	phy_node = of_parse_phandle(dn, "phy-handle", 0);
>	of_node_put(phy_node);
>	if (!dn || (ret && !phy_node)) {
>		...
>
>Yes, it looks a bit weird, but the only thing you're interested in
>here is whether of_parse_phandle() returned NULL or non-NULL. You're
>not interested in dereferencing the pointer.
>
>Some may raise some eye-brows at that, so it may be better to have
>this as a helper:
>
>static bool macb_phy_handle_exists(struct device_node *dn)
>{
>	dn = of_parse_phandle(dn, "phy-handle", 0);
>	of_node_put(dn);
>	return dn != NULL;
>}
>
>and use it as:
>
>	if (!dn || (ret && !macb_phy_handle_exists(dn))) {
>
>which is more obvious what is going on.
>

This is good. I will put this in the revised patch.

>
>>
>>         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.
>
>Here's a patch I quickly knocked up that does this - may not apply to
>the kernel you're using as there's a whole bunch of work I have
>outstanding, but gives the outline idea.  Does this help?
>
>

Yes, this will help. Once available, we will adopt this change.

>8<===
>From: Russell King <rmk+kernel@...linux.org.uk>
>Subject: [PATCH] net: phylink: avoid of_phy_attach()
>
>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.
>
>
>Signed-off-by: Russell King <rmk+kernel@...linux.org.uk>
>---
>