linux-kernel - Re: [PATCH net-next v7 09/10] net: phy: smsc: use phy_read_poll

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [day] [month] [year] [list]
Message-ID: <20200602163022.GA6169@nuc8i5>
Date:   Wed, 3 Jun 2020 00:30:22 +0800
From:   Dejin Zheng <zhengdejin5@...il.com>
To:     Kevin Groeneveld <kgroeneveld@...il.com>
Cc:     andrew@...n.ch, f.fainelli@...il.com, hkallweit1@...il.com,
        linux@...linux.org.uk, David Miller <davem@...emloft.net>,
        corbet@....net, tglx@...utronix.de, gregkh@...uxfoundation.org,
        allison@...utok.net, mchehab+samsung@...nel.org,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
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@...il.com> wrote:
> >
> > use phy_read_poll_timeout() to replace the poll codes for
> > simplify lan87xx_read_status() function.
> >
> > Suggested-by: Andrew Lunn <andrew@...n.ch>
> > Reviewed-by: Florian Fainelli <f.fainelli@...il.com>
> > Signed-off-by: Dejin Zheng <zhengdejin5@...il.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