Message-ID: <20200319112535.GD25745@shell.armlinux.org.uk>
Date:   Thu, 19 Mar 2020 11:25:35 +0000
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Heiner Kallweit <hkallweit1@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        David Miller <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 1/3] net: phy: add and use phy_check_downshift

On Thu, Mar 19, 2020 at 08:30:58AM +0100, Heiner Kallweit wrote:
> On 19.03.2020 00:21, Russell King - ARM Linux admin wrote:
> > On Wed, Mar 18, 2020 at 10:29:01PM +0100, Heiner Kallweit wrote:
> >> So far PHY drivers have to check whether a downshift occurred to be
> >> able to notify the user. To make life of drivers authors a little bit
> >> easier move the downshift notification to phylib. phy_check_downshift()
> >> compares the highest mutually advertised speed with the actual value
> >> of phydev->speed (typically read by the PHY driver from a
> >> vendor-specific register) to detect a downshift.
> > 
> > My personal position on this is that reporting a downshift will be
> > sporadic at best, even when the link has negotiated slower.
> > 
> > The reason for this is that either end can decide to downshift.  If
> > the remote partner downshifts, then the local side has no idea that
> > a downshift occurred, and can't report that the link was downshifted.
> > 
> Right, this warning can't cover the case that remote link partner
> downshifts. In this case however ethtool et al should show the reduced
> link partner advertisement, and therefore provide a hint why speed
> is slow.
> 
> > So, is it actually useful to report these events?
> > 
> To provide an example: A user recently complained that r8169 driver
> makes problems on his system:
> - it takes long time until link comes up
> - link is slow
> With iperf he then found out that displayed speed is 1Gbps but actual
> link speed is 100Mbps. In the end he found that one pin of his network
> port was corroded, therefore the downshift.
> 
> The phase of blaming the driver could have been skipped if he would
> have seen a downshift warning from the very beginning.

This sounds like a good theory to justify it, but it suffers from one
major flaw.

There was indeed a bug - the driver was reporting 1Gbps, whereas the
link was not operating at that speed, but at 100Mbps.  Had that bug
not existed, the kernel would've reported 100Mbps as the speed, and
then your justification in the first paragraph applies - the link
speed is slower than expected.

With that bug in place, this patch does nothing; you're using the same
algorithm to calculate what the speed should be and comparing it with
the same algorithm result reported from phy_resolve_aneg_linkmode().

So, the problem is going to remain.

The only time that this helps is if PHY drivers implement reading a
vendor register to report the actual link speed, and the PHY specific
driver is used.

Also, falling back to the generic PHY driver is going to result in the
same hard-to-debug problem that you refer to above.

So, do we need to print a big fat warning that the generic driver is
being used, and recommend the user uses the tailored driver instead?

There's lots of issues to consider here; it is not as simple as has
been suggested, and I'm not sure that this patch adds particularly
high value by really solving the problem.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Powered by blists - more mailing lists