Messages in this thread | | | From | Milind Parab <> | Subject | RE: [PATCH 1/3] net: macb: fix for fixed-link mode | Date | Wed, 11 Dec 2019 08:21:57 +0000 |
| |
>> >> + 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@armlinux.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@armlinux.org.uk> >--- >
| |