Re: [PATCH net-next v7 09/10] net: phy: smsc: use phy_read_poll_timeout() to simplify the code

On Mon, Jun 01, 2020 at 02:58:21PM -0400, Kevin Groeneveld wrote:

On Mon, Mar 23, 2020 at 11:10 AM Dejin Zheng [off-list ref] wrote:

quoted

use phy_read_poll_timeout() to replace the poll codes for
simplify lan87xx_read_status() function.

Suggested-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Signed-off-by: Dejin Zheng <redacted>
---
v6 -> v7:
        - adapt to a newly added parameter sleep_before_read.
v5 -> v6:
        - no changed.
v4 -> v5:
        - add msleep before phy_read_poll_timeout() to keep the
          code more similar
v3 -> v4:
        - add this patch by Andrew's suggestion. Thanks Andrew!

 drivers/net/phy/smsc.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
index b73298250793..93da7d3d0954 100644
--- a/drivers/net/phy/smsc.c
+++ b/drivers/net/phy/smsc.c

@@ -112,8 +112,6 @@ static int lan87xx_read_status(struct phy_device

*phydev)

quoted

        int err = genphy_read_status(phydev);

        if (!phydev->link && priv->energy_enable) {
-               int i;
-
                /* Disable EDPD to wake up PHY */
                int rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS);
                if (rc < 0)

@@ -125,15 +123,11 @@ static int lan87xx_read_status(struct phy_device

*phydev)

quoted

                        return rc;

                /* Wait max 640 ms to detect energy */
-               for (i = 0; i < 64; i++) {
-                       /* Sleep to allow link test pulses to be sent */
-                       msleep(10);
-                       rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS);
-                       if (rc < 0)
-                               return rc;
-                       if (rc & MII_LAN83C185_ENERGYON)
-                               break;
-               }
+               phy_read_poll_timeout(phydev, MII_LAN83C185_CTRL_STATUS,
+                                     rc & MII_LAN83C185_ENERGYON, 10000,
+                                     640000, true);
+               if (rc < 0)
+                       return rc;

                /* Re-enable EDPD */
                rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS);
--
2.25.0

This patch causes the kernel log to be spammed with the following when
Ethernet cable is not connected:
SMSC LAN8710/LAN8720 2188000.ethernet-1:00: lan87xx_read_status failed: -110

Kevin, I am very sorry for the trouble caused by my patch. 

It still seems to work but I think that is only a fluke.

The "if (rc < 0)" is not actually checking the return value of
phy_read_poll_timeout but is looking at the value of the register read.  I
don't think rc will ever be negative in this case.  If you change the code
to "rc = phy_read_poll_timeout(...)" so that it actually checks the error
then the function will behave differently than before.  The original code
would only return an error if phy_read returned an error.  On a timeout it
just continued.  So the "if" could be changed to "if (rc < 0 && rc !=
-ETIMEDOUT)".  But you will still get the extra messages in the log that
were not there before.

Yes, My patch did change the original behavior. It will not have error message
whether it is timeout or phy_read fails, but my patch changed it and will
print some error messages. It is my mistake. I'm so sorry for that.
How do you think of the following fix?

quoted

                /* Wait max 640 ms to detect energy */
-               for (i = 0; i < 64; i++) {
-                       /* Sleep to allow link test pulses to be sent */
-                       msleep(10);
-                       rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS);
-                       if (rc < 0)
-                               return rc;
-                       if (rc & MII_LAN83C185_ENERGYON)
-                               break;
-               }
+               phy_read_poll_timeout(phydev, MII_LAN83C185_CTRL_STATUS,
+                                     rc & MII_LAN83C185_ENERGYON, 10000,
+                                     640000, true);
+               if (rc < 0)
+                       return rc;

		ret = read_poll_timeout(phy_read, rc, rc & MII_LAN83C185_ENERGYON || rc < 0,                                                         
		       		10000, 640000, true, phydev, MII_LAN83C185_CTRL_STATUS);
		if (!ret && rc < 0)
			return rc;
BR,
Dejin

Kevin