Message-ID: <8efc016a-3390-3bec-d74c-7c215c151b2f@gmail.com>
Date:   Mon, 14 Nov 2016 11:19:05 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Måns Rullgård <mans@...sr.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

On 11/14/2016 11:00 AM, Måns Rullgård wrote:
> 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.

You do a DMA stop, but you don't disable the MAC receiver unlike what
nb8800_stop() does, why is not calling nb8800_mac_rx() necessary here?

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

Differentiate in that when you are called from adjust_link, why bother
checking with netif_running() since you are only configuring the pause
settings when phydev->link is set. Not that this matters much, but
that's something the caller can tell you.

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

No kidding.
-- 
Florian

Powered by blists - more mailing lists