Hello.
On 04/29/2016 02:45 PM, Marek Vasut wrote:
First of all, thank you for the patch!
You beat me to it (and not only me). :-)
Heh, hacking at night has it's perks :)
I know, I'm just supposed to work on different things. :-)
On 4/29/2016 4:09 AM, Marek Vasut wrote:
Since commit b74766a0a0feeef5c779709cc5d109451c0d5b17 in linux-next,
( phylib: don't return NULL from get_phy_device() ), phy_get_device()
scripts/checkpatch.pl now enforces certain commit citing style, yours
doesn't quite match it.
Ha, I didn't know that checkpatch can now warn about this too, nice. Is
that in next already ? I just tried checkpatch and it doesn't warn about it.
It's been merged long ago. Actually, I've just run this script from the
current Linus' tree on your patch, here's the results:
ERROR: Please use git commit description style 'commit <12+ chars of sha1>
("<title line>")' - ie: 'commit fatal: bad o ("cc5d109451c0d5b17")'
#4:
Since commit b74766a0a0feeef5c779709cc5d109451c0d5b17 in linux-next,
WARNING: 'immediatelly' may be misspelled - perhaps 'immediately'?
#23:
'if (IS_ERR(phydev))' condition and the loop exits immediatelly if the
total: 1 errors, 1 warnings, 0 checks, 8 lines checked
Anyway, regarding this format, do you want V2 ? Originally, I had the
full commit info in the message, but that was just taking space and
it is not the commit which is important in the message, so I trimmed
it down.
I'll let DaveM decide.
will return ERR_PTR(-ENODEV) instead of NULL if the PHY device ID is
all ones.
This causes problem with stmmac driver and likely some other drivers
which call mdiobus_register(). I triggered this bug on SoCFPGA MCVEVK
board with linux-next 20160427 and 20160428. In case of the stmmac, if
there is no PHY node specified in the DT for the stmmac block, the stmmac
driver ( drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c function
stmmac_mdio_register() ) will call mdiobus_register() , which will
register the MDIO bus and probe for the PHY.
The mdiobus_register() resp. __mdiobus_register() iterates over all of
the addresses on the MDIO bus and calls mdiobus_scan() for each of them,
which invokes get_phy_device(). Before the aforementioned patch, the
mdiobus_scan() would return NULL if no PHY was found on a given address
and mdiobus_register() would continue and try the next PHY address. Now,
mdiobus_scan() returns ERR_PTR(-ENODEV), which is caught by the
'if (IS_ERR(phydev))' condition and the loop exits immediatelly if the
PHY address does not contain PHY.
Repair this by explicitly checking for the ERR_PTR(-ENODEV) and if this
error comes around, continue with the next PHY address.
Signed-off-by: Marek Vasut <ma...@denx.de>
Cc: Arnd Bergmann <a...@arndb.de>
Cc: David S. Miller <da...@davemloft.net>
Cc: Dinh Nguyen <dingu...@opensource.altera.com>
Cc: Florian Fainelli <f.faine...@gmail.com>
Cc: Sergei Shtylyov <sergei.shtyl...@cogentembedded.com>
Reviewed-by: Sergei Shtylyov <sergei.shtyl...@cogentembedded.com>
---
drivers/net/phy/mdio_bus.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
NOTE: I don't quite like this explicit check , but I don't have better
idea now.
It's fine. I was going to do just the same :-)
OK, I'm glad I'm not alone on this one :)
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 499003ee..388f992 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -333,7 +333,7 @@ int __mdiobus_register(struct mii_bus *bus, struct
module *owner)
struct phy_device *phydev;
phydev = mdiobus_scan(bus, i);
- if (IS_ERR(phydev)) {
+ if (IS_ERR(phydev) && (PTR_ERR(phydev) != -ENODEV)) {
Parens around the second operand of && are not really needed though...
While I agree, I also prefer to make things obvious when reading the
code by adding the parenthesis. It's a matter of taste I think. Just let
me know if I should spin V2 without them :)
Again, let DaveM decide. But I don't think the code being patched uses
parens in such cases... I wouldn't, for sure.
Thanks for the review!
You've saved me some work (but I still need to analyze the other call paths).
[...]
MBR, Sergei
