Date:   Thu, 17 Nov 2016 22:17:06 +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 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?

Oh, right.  That's because the RXC_CR register (where the flow control
bits are) can't be modified when the RCR_EN bit (rx dma enable) is set.
The MAC core register controlled by nb8800_mac_rx() doesn't matter here.
There is no way of changing the flow control setting without briefly
stopping rx dma.

None of this should be relevant here though since everything should be
all set up before dma is enabled the first time.

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

netif_running() can be true or false independently of the link state.

-- 
Måns Rullgård

Powered by blists - more mailing lists