netdev - Re: [PATCH net v1] net: phy: smsc: fix printing too many logs

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>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200617202450.GX1551@shell.armlinux.org.uk>
Date:   Wed, 17 Jun 2020 21:24:50 +0100
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Dejin Zheng <zhengdejin5@...il.com>, f.fainelli@...il.com,
        hkallweit1@...il.com, davem@...emloft.net, kuba@...nel.org,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        Kevin Groeneveld <kgroeneveld@...il.com>
Subject: Re: [PATCH net v1] net: phy: smsc: fix printing too many logs

On Wed, Jun 17, 2020 at 08:43:34PM +0200, Andrew Lunn wrote:
> You have explained what the change does. But not why it is
> needed. What exactly is happening. To me, the key thing is
> understanding why we get -110, and why it is not an actual error we
> should be reporting as an error. That is what needs explaining.

The patch author really ought to be explaining this... but let me
have a go.  It's worth pointing out that the comments in the file
aren't good English either, so don't really describe what is going
on.

When this PHY is in EDPD mode, it doesn't always detect a connected
cable.  The workaround for it involves, when the link is down, and
at each read_status() call:

- disable EDPD mode, forcing the PHY out of low-power mode
- waiting 640ms to see if we have any energy detected from the media
- re-enable entry to EDPD mode

This is presumably enough to allow the PHY to notice that a cable is
connected, and resume normal operations to negotiate with the partner.

The problem is that when no media is detected, the 640ms wait times
out (as it should, we don't want to wait forever) and the kernel
prints a warning.

This bug was introduced by an inappropriate conversion of:

                /* 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;
-               }

to phy_read_poll_timeout() in the belief that it was "cleaning up"
the code, but it actually results in a functional change of printing
an error at the end of the 640ms window which wasn't there before.
The patch that does this even states that it's about "simplifying"
the code, yet it introduced a bug by doing so - that being the
extra kernel log message.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!