| Message ID | 5829D95C.6060509@free.fr |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
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). > > # ethtool -a eth0 > Pause parameters for eth0: > Autonegotiate: off > RX: off > TX: off > > These values make sense considering my changes in the driver. > > Are 100 Mbps switches supposed to support these pause features, > and are they supposed to be able to auto-negotiate them? Yes, switches can support flow control aka pause frames, and unless they are configurable, they typically advertise what their EEPROM has defined for them, so most likely the full auto-negotiated spectrum: 10/100/1000Mbps and support for flow control, but your mileage may vary of course.
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? Right now we know that Mason's patch makes this work, but we do not understand why nor its implications. > >> >> # ethtool -a eth0 >> Pause parameters for eth0: >> Autonegotiate: off >> RX: off >> TX: off >> >> These values make sense considering my changes in the driver. >> >> Are 100 Mbps switches supposed to support these pause features, >> and are they supposed to be able to auto-negotiate them? > > Yes, switches can support flow control aka pause frames, and unless they > are configurable, they typically advertise what their EEPROM has defined > for them, so most likely the full auto-negotiated spectrum: > 10/100/1000Mbps and support for flow control, but your mileage may vary > of course. >
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?
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 Fainelli <f.fainelli@gmail.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.
On 11/14/2016 11:00 AM, Måns Rullgård wrote: > Florian Fainelli <f.fainelli@gmail.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.
On 14/11/2016 19:20, Florian Fainelli wrote: > On 11/14/2016 09:59 AM, Sebastian Frias wrote: > >> 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). Did you note that, without the change under discussion (i.e. with the eth driver as it is upstream), when the board is connected to a 100 Mbps switch, then *nothing* works *systematically (no ping, no DHCP; are there other relevant low-level network tools?). Also, maybe this comment was lost in my own noise: If I manually set the link up, then down, then run udhcpc => then nothing works, as if something is wedged somewhere (a kernel thread gets borked by a race condition?) Could not advertising pause frames result in making such a race condition impossible? (I don't really believe in a race, due to the 100% nature of the problem.) >> 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? Isn't that what ethtool -a eth0 prints? How do I get the link partner information? Just ethtool eth0? Regards.
On 11/14/2016 12:27 PM, Mason wrote: > On 14/11/2016 19:20, Florian Fainelli wrote: > >> On 11/14/2016 09:59 AM, Sebastian Frias wrote: >> >>> 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). > > Did you note that, without the change under discussion (i.e. with > the eth driver as it is upstream), when the board is connected to > a 100 Mbps switch, then *nothing* works *systematically (no ping, > no DHCP; are there other relevant low-level network tools?). No I missed that, way too many emails, really. So how about you compare the register settings that could be (that is, all that could be modified by the PHYLIB adjust_link function) and try to spot where things could go wrong? Any other register that can be influenced by the link speed? It seems like a possible (yet after re-reading, very unlikely) scenario, considering that priv->speed, priv->duplex and priv->link are initially zero-initialized (because nb8800_priv is zero initialized) may not force a correct link transition and a full MAC reconfiguration in nb8800_link_reconfigure() where some of the cached values are used. NB: you will see most drivers initialize the previous link, speed, duplex values to -1, because those are outside of the range of values that PHYLIB would assign to phydev->{link,duplex,speed}, and therefore, this is guaranteed to make the adjust_link callback that tries to minimize these settings to force a transition. > > Also, maybe this comment was lost in my own noise: > > If I manually set the link up, then down, then run udhcpc > => then nothing works, as if something is wedged somewhere > (a kernel thread gets borked by a race condition?) Well then start seriously debugging the problem: firs thing you need to check is is the RUNNING flag set on the interface (which indicates a carrier on?) without that, the networking stack won't even send packets. If it is not set, why is not it set? Did nb8800_mac_config() get called in the first place to configure the MAC wrt. the link settings? When you transmit, do transmit counters increase? That would indicate the TX DMA does its job. When transmission occurs, it is successful or is it reporting errors? If the PHY supports it, can you access PHY counters and look for success/error counters changing? Finally, try to put another golden (working) host and if your switch supports it, configure port mirroring to look at packets. If the switch does not support it, then try different link partners. > > Could not advertising pause frames result in making such a > race condition impossible? (I don't really believe in a race, > due to the 100% nature of the problem.) > >>> 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? > > Isn't that what ethtool -a eth0 prints? No, ethtool -a prints the local pause settings. > How do I get the link partner information? ethtool eth0: # ethtool eth0 Settings for eth0: Supported ports: [ TP MII ] Supported link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full Supported pause frame use: Symmetric Receive-only Supports auto-negotiation: Yes Advertised link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full Advertised pause frame use: No Advertised auto-negotiation: Yes Link partner advertised link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full ^====================== Link partner advertised pause frame use: Symmetric Link partner advertised auto-negotiation: Yes ^======================== Speed: 100Mb/s Duplex: Full Port: MII PHYAD: 1 Transceiver: internal Auto-negotiation: on Supports Wake-on: gs Wake-on: d SecureOn password: 00:00:00:00:00:00 Current message level: 0x00000007 (7) drv probe link Link detected: yes # > Just ethtool eth0? Yes, just that.
On 14/11/2016 22:00, Florian Fainelli wrote:
> No I missed that, way too many emails, really.
Sorry, I was trying to be thorough, but I went overboard.
I'll start a new thread tomorrow, with a smaller CC list
(only those who have participated so far) and I'll try to
remain concise.
Regards.
Hi Florian, On 11/14/2016 10:00 PM, Florian Fainelli wrote: > On 11/14/2016 12:27 PM, Mason wrote: >> On 14/11/2016 19:20, Florian Fainelli wrote: >> >>> On 11/14/2016 09:59 AM, Sebastian Frias wrote: >>> >>>> 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). >> >> Did you note that, without the change under discussion (i.e. with >> the eth driver as it is upstream), when the board is connected to >> a 100 Mbps switch, then *nothing* works *systematically (no ping, >> no DHCP; are there other relevant low-level network tools?). > > No I missed that, way too many emails, really. So how about you compare > the register settings that could be (that is, all that could be modified > by the PHYLIB adjust_link function) and try to spot where things could > go wrong? The registers that make this work or fail are modified in 'nb8800_pause_config()' which is called from 'adjust_link', see below. >Any other register that can be influenced by the link speed? It may not be the link-speed per se, but the way these "pause" parameters are interacting. Indeed, on the 1000 Mbps switch we get (1): "Link partner advertised pause frame use: Symmetric Receive-only" And on the 100 Mbps switch we get (2): "Link partner advertised pause frame use: Symmetric" Since the 100 Mbps is advertising "symmetric", the code in 'drivers/net/ethernet/aurora/nb8800.c:nb8800_pause_config()' enables the flow control bit: nb8800_modl(priv, NB8800_RXC_CR, RCR_FL, priv->pause_tx); If we force this to be disabled, the network works on 100 Mbps. Keep in mind that the same driver (net and phy) work on a different board, could it be just by chance? > > It seems like a possible (yet after re-reading, very unlikely) scenario, > considering that priv->speed, priv->duplex and priv->link are initially > zero-initialized (because nb8800_priv is zero initialized) may not force > a correct link transition and a full MAC reconfiguration in > nb8800_link_reconfigure() where some of the cached values are used. > > NB: you will see most drivers initialize the previous link, Could you suggest another driver to compare this with? It is not easy to know which drivers follow the conventions so that they can be used as examples. >speed, > duplex values to -1, because those are outside of the range of values > that PHYLIB would assign to phydev->{link,duplex,speed}, and therefore, > this is guaranteed to make the adjust_link callback that tries to > minimize these settings to force a transition. > >> >> Also, maybe this comment was lost in my own noise: >> >> If I manually set the link up, then down, then run udhcpc >> => then nothing works, as if something is wedged somewhere >> (a kernel thread gets borked by a race condition?) > > Well then start seriously debugging the problem: firs thing you need to > check is is the RUNNING flag set on the interface (which indicates a > carrier on?) without that, the networking stack won't even send packets. 'ifconfig' reports "eth0: flags=4163<UP,BROADCAST,RUNNING,MULTICAST>" Packets get sent, as the statistics show for example the following values increasing: tx_bytes: 756 tx_frames: 3 However, the statistics show that RX values increase and then stop. Basically, if we do: # ethtool -S eth0 > /tmp/toto # udhcpc (ctrl-C because it won't return because it keeps trying) # ethtool -S eth0 > /tmp/titi # diff /tmp/toto /tmp/titi we can see that the RX values go from 0 to X, but get stuck at X, while the TX values keep increasing: rx_bytes_ok: 887 rx_frames_ok: 8 ... tx_bytes: 1576 tx_frames: 7 Like if the Flow Control is not working properly. However we don't know how to double check it, since the older driver (for kernel 3.4) does not seem to use the feature. > If it is not set, why is not it set? Did nb8800_mac_config() get called > in the first place to configure the MAC wrt. the link settings? > > When you transmit, do transmit counters increase? That would indicate > the TX DMA does its job. When transmission occurs, it is successful or > is it reporting errors? If the PHY supports it, can you access PHY > counters and look for success/error counters changing? Finally, try to > put another golden (working) host and if your switch supports it, > configure port mirroring to look at packets. If the switch does not > support it, then try different link partners. > So far, our limited testing indicates that only switches that advertise: "Link partner advertised pause frame use: Symmetric" are problematic. Would the information presented here give you guys other ideas of what to look for? Are those switches known to give issues? Could the "pause" feature be timing dependent? (since it works on other boards) Best regards, Sebastian ---- (1): ethtool eth0 when connected to 1000 Mbps switch Supported ports: [ TP MII ] Supported link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full 1000baseT/Full Supported pause frame use: Symmetric Supports auto-negotiation: Yes Advertised link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full 1000baseT/Full Advertised pause frame use: Symmetric Advertised auto-negotiation: Yes Link partner advertised link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full 1000baseT/Half 1000baseT/Full Link partner advertised pause frame use: Symmetric Receive-only Link partner advertised auto-negotiation: Yes Speed: 1000Mb/s Duplex: Full Port: MII PHYAD: 4 Transceiver: external Auto-negotiation: on Link detected: yes (2): ethtool eth0 when connected to 100 Mbps switch Supported ports: [ TP MII ] Supported link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full 1000baseT/Full Supported pause frame use: Symmetric Supports auto-negotiation: Yes Advertised link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full 1000baseT/Full Advertised pause frame use: Symmetric Advertised auto-negotiation: Yes Link partner advertised link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full Link partner advertised pause frame use: Symmetric Link partner advertised auto-negotiation: Yes Speed: 100Mb/s Duplex: Full Port: MII PHYAD: 4 Transceiver: external Auto-negotiation: on Link detected: yes >> >> Could not advertising pause frames result in making such a >> race condition impossible? (I don't really believe in a race, >> due to the 100% nature of the problem.) >> >>>> 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? >> >> Isn't that what ethtool -a eth0 prints? > > No, ethtool -a prints the local pause settings. > >> How do I get the link partner information? > > ethtool eth0: > > # ethtool eth0 > Settings for eth0: > Supported ports: [ TP MII ] > Supported link modes: 10baseT/Half 10baseT/Full > 100baseT/Half 100baseT/Full > Supported pause frame use: Symmetric Receive-only > Supports auto-negotiation: Yes > Advertised link modes: 10baseT/Half 10baseT/Full > 100baseT/Half 100baseT/Full > Advertised pause frame use: No > Advertised auto-negotiation: Yes > Link partner advertised link modes: 10baseT/Half 10baseT/Full > 100baseT/Half 100baseT/Full > > ^====================== > > Link partner advertised pause frame use: Symmetric > Link partner advertised auto-negotiation: Yes > > ^======================== > > Speed: 100Mb/s > Duplex: Full > Port: MII > PHYAD: 1 > Transceiver: internal > Auto-negotiation: on > Supports Wake-on: gs > Wake-on: d > SecureOn password: 00:00:00:00:00:00 > Current message level: 0x00000007 (7) > drv probe link > Link detected: yes > # > > >> Just ethtool eth0? > > Yes, just that. >
Florian Fainelli <f.fainelli@gmail.com> writes: > On 11/14/2016 11:00 AM, Måns Rullgård wrote: >> Florian Fainelli <f.fainelli@gmail.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.
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);