netdev - Re: Debugging Ethernet issues

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: <yw1xpolxga3o.fsf@unicorn.mansr.com>
Date:   Mon, 14 Nov 2016 19:00:43 +0000
From:   Måns Rullgård <mans@...sr.com>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     Sebastian Frias <sf84@...oste.net>, Mason <slash.tmp@...e.fr>,
        Andrew Lunn <andrew@...n.ch>, netdev <netdev@...r.kernel.org>,
        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

Florian Fainelli <f.fainelli@...il.com> writes:

> 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);
>>>>>  
>>>>>

[...]

>>>> 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.

I was advised to advertise flow control by default back when I was
working on the driver, and I think it makes sense to do so.

>> 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,

This is due to a silly hardware limitation.  The register containing the
flow control bits can't be written while rx is enabled.

> and it does not differentiate being called from
> ethtool::set_pauseparam or the PHYLIB adjust_link callback (which it
> probably should),

Differentiate how?

> 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?

If you're attempting to send or receive things before you get the link
up notification, you shouldn't expect anything to work reliably.

-- 
Måns Rullgård