Date:   Mon, 14 Nov 2016 10:42:38 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Sebastian Frias <sf84@...oste.net>, Mason <slash.tmp@...e.fr>,
        Andrew Lunn <andrew@...n.ch>
Cc:     netdev <netdev@...r.kernel.org>, Mans Rullgard <mans@...sr.com>,
        Sergei Shtylyov <sergei.shtylyov@...entembedded.com>,
        Tom Lendacky <thomas.lendacky@....com>,
        Zach Brown <zach.brown@...com>,
        Shaohui Xie <shaohui.xie@....com>,
        Tim Beale <tim.beale@...iedtelesis.co.nz>,
        Brian Hill <brian@...ston-radar.com>,
        Vince Bridgers <vbridgers2013@...il.com>,
        Balakumaran Kannan <kumaran.4353@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Kirill Kapranov <kapranoff@...ox.ru>
Subject: Re: Debugging Ethernet issues

On 11/14/2016 10:20 AM, Florian Fainelli wrote:
> On 11/14/2016 09:59 AM, Sebastian Frias wrote:
>> On 11/14/2016 06:32 PM, Florian Fainelli wrote:
>>> On 11/14/2016 07:33 AM, Mason wrote:
>>>> On 14/11/2016 15:58, Mason wrote:
>>>>
>>>>> nb8800 26000.ethernet eth0: Link is Up - 100Mbps/Full - flow control rx/tx
>>>>> vs
>>>>> nb8800 26000.ethernet eth0: Link is Up - 100Mbps/Full - flow control off
>>>>>
>>>>> I'm not sure whether "flow control" is relevant...
>>>>
>>>> Based on phy_print_status()
>>>> phydev->pause ? "rx/tx" : "off"
>>>> I added the following patch.
>>>>
>>>> diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
>>>> index defc22a15f67..4e758c1cfa4e 100644
>>>> --- a/drivers/net/ethernet/aurora/nb8800.c
>>>> +++ b/drivers/net/ethernet/aurora/nb8800.c
>>>> @@ -667,6 +667,8 @@ static void nb8800_link_reconfigure(struct net_device *dev)
>>>>         struct phy_device *phydev = priv->phydev;
>>>>         int change = 0;
>>>>  
>>>> +       printk("%s from %pf\n", __func__, __builtin_return_address(0));
>>>> +
>>>>         if (phydev->link) {
>>>>                 if (phydev->speed != priv->speed) {
>>>>                         priv->speed = phydev->speed;
>>>> @@ -1274,9 +1276,9 @@ static int nb8800_hw_init(struct net_device *dev)
>>>>         nb8800_writeb(priv, NB8800_PQ2, val & 0xff);
>>>>  
>>>>         /* Auto-negotiate by default */
>>>> -       priv->pause_aneg = true;
>>>> -       priv->pause_rx = true;
>>>> -       priv->pause_tx = true;
>>>> +       priv->pause_aneg = false;
>>>> +       priv->pause_rx = false;
>>>> +       priv->pause_tx = false;
>>>>  
>>>>         nb8800_mc_init(dev, 0);
>>>>  
>>>>
>>>> Connected to 1000 Mbps switch:
>>>>
>>>> # time udhcpc | while read LINE; do date; echo $LINE; done
>>>> Thu Jan  1 00:00:22 UTC 1970
>>>> udhcpc (v1.22.1) started
>>>> Thu Jan  1 00:00:22 UTC 1970
>>>> Sending discover...
>>>> [   24.565346] nb8800_link_reconfigure from phy_state_machine
>>>> Thu Jan  1 00:00:25 UTC 1970
>>>> Sending discover...
>>>> [   26.575402] nb8800_link_reconfigure from phy_state_machine
>>>> [   26.580972] nb8800 26000.ethernet eth0: Link is Up - 1Gbps/Full - flow control rx/tx
>>>> Thu Jan  1 00:00:28 UTC 1970
>>>> Sending discover...
>>>> Thu Jan  1 00:00:29 UTC 1970
>>>> Sending select for 172.27.64.58...
>>>> Thu Jan  1 00:00:29 UTC 1970
>>>> Lease of 172.27.64.58 obtained, lease time 604800
>>>> Thu Jan  1 00:00:29 UTC 1970
>>>> deleting routers
>>>> Thu Jan  1 00:00:29 UTC 1970
>>>> adding dns 172.27.0.17
>>>>
>>>> real    0m7.388s
>>>> user    0m0.040s
>>>> sys     0m0.090s
>>>>
>>>>
>>>>
>>>> Connected to 100 Mbps switch:
>>>>
>>>> # time udhcpc | while read LINE; do date; echo $LINE; done
>>>> Thu Jan  1 00:00:14 UTC 1970
>>>> udhcpc (v1.22.1) started
>>>> Thu Jan  1 00:00:15 UTC 1970
>>>> Sending discover...
>>>> [   16.968621] nb8800_link_reconfigure from phy_state_machine
>>>> [   17.975359] nb8800_link_reconfigure from phy_state_machine
>>>> [   17.980923] nb8800 26000.ethernet eth0: Link is Up - 100Mbps/Full - flow control rx/tx
>>>> Thu Jan  1 00:00:18 UTC 1970
>>>> Sending discover...
>>>> Thu Jan  1 00:00:19 UTC 1970
>>>> Sending select for 172.27.64.58...
>>>> Thu Jan  1 00:00:19 UTC 1970
>>>> Lease of 172.27.64.58 obtained, lease time 604800
>>>> Thu Jan  1 00:00:19 UTC 1970
>>>> deleting routers
>>>> Thu Jan  1 00:00:19 UTC 1970
>>>> adding dns 172.27.0.17
>>>>
>>>> real    0m4.355s
>>>> user    0m0.043s
>>>> sys     0m0.083s
>>>>
>>>
>>> And the time difference is clearly accounted for auto-negotiation time
>>> here, as you can see it takes about 3 seconds for Gigabit Ethernet to
>>> auto-negotiate and that seems completely acceptable and normal to me
>>> since it is a more involved process than lower speeds.
>>>
>>>>
>>>>
>>>> OK, so now it works (by accident?) even on 100 Mbps switch, but it still
>>>> prints "flow control rx/tx"...
>>>
>>> Because your link partner advertises flow control, and that's what
>>> phydev->pause and phydev->asym_pause report (I know it's confusing, but
>>> that's what it is at the moment).
>>
>> Thanks.
>> Could you confirm that Mason's patch is correct and/or that it does not
>> has negative side-effects?
> 
> The patch is not correct nor incorrect per-se, it changes the default
> policy of having pause frames advertised by default to not having them
> advertised by default. This influences both your Ethernet MAC and the
> link partner in that the result is either flow control is enabled
> (before) or it is not (with the patch). There must be something amiss if
> you see packet loss or some kind of problem like that with an early
> exchange such as DHCP. Flow control tend to kick in under higher packet
> rates (at least, that's what you expect).
> 
> 
>>
>> Right now we know that Mason's patch makes this work, but we do not understand
>> why nor its implications.
> 
> You need to understand why, right now, the way this problem is
> presented, you came up with a workaround, not with the root cause or the
> solution. What does your link partner (switch?) reports, that is, what
> is the ethtool output when you have a link up from  your nb8800 adapter?

Actually, nb8800_pause_config() seems to be doing a complete MAC/DMA
reconfiguration when pause frames get auto-negotiated while the link is
UP, and it does not differentiate being called from
ethtool::set_pauseparam or the PHYLIB adjust_link callback (which it
probably should), wondering if there is a not a remote chance you can
get the reply to arrive right when you just got signaled a link UP?
-- 
Florian

Powered by blists - more mailing lists