Messages in this thread | | | Date | Wed, 3 Jun 2020 00:30:22 +0800 | From | Dejin Zheng <> | Subject | 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 <zhengdejin5@gmail.com> wrote: > > > > 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 <zhengdejin5@gmail.com> > > --- > > 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) > > 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) > > 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?
> > /* 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
| |