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

[Milind Parab]

>> >> +		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@xxxxxxxxxxxxxxx>
>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@xxxxxxxxxxxxxxx>
>---
>