Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 17 Jun 2020 21:24:50 +0100
From:   Russell King - ARM Linux admin <linux@armlinux.org.uk>
To:     Andrew Lunn <andrew@lunn.ch>
Cc:     Dejin Zheng <zhengdejin5@gmail.com>, f.fainelli@gmail.com,
        hkallweit1@gmail.com, davem@davemloft.net, kuba@kernel.org,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        Kevin Groeneveld <kgroeneveld@gmail.com>
Subject: Re: [PATCH net v1] net: phy: smsc: fix printing too many logs
Message-ID: <20200617202450.GX1551@shell.armlinux.org.uk>
References: <20200617153340.17371-1-zhengdejin5@gmail.com>
 <20200617161925.GE205574@lunn.ch>
 <20200617175039.GA18631@nuc8i5>
 <20200617184334.GA240559@lunn.ch>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200617184334.GA240559@lunn.ch>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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!