genphy_read_status() vs. 1000bT Pause capability

genphy_read_status() vs. 1000bT Pause capability

[Benjamin Herrenschmidt]

Hi !

I noticed that flow control isn't being enabled on a system I'm
working on by default. I've tracked it down to two things:

 - The realtec.c PHY driver doesn't have Pause or Asym_Pause in
its exposed capabilities. This is in part because PHY_GBIT_FEATURES
does not include SUPPORTED_Pause and SUPPORTED_Asym_Pause. Is there
a specific reason for that ?

 - After I've hacked the above, I get in genphy_read_status():

lpa=c1e1 lpagb=3800 adv=de1 common_adv=1e1 common_adv_gb=800

So we have negociated 1000bT full duplex. LPA_PAUSE's aren't set
but I was under the impression that in Gigabit mode, LPA bit 0x80
which *is* set, meant ADVERTISE_1000XPAUSE which is the pause
capability isn't it ? Or am I confusing with something else ?
This seems to be how mii_adv_to_ethtool_adv_x() decodes them
but that function is not called by genphy_read_status()...

Now it's been a while since I hacked network drivers and back then
everybody did their own salad with gigabit PHYs so it's very possible
that I missed something here.

Should we update genphy_read_status() to grab the pause details
from mii_adv_to_ethtool_adv_x() when in 1000bT mode ?

Thanks !

Cheers,
Ben.

Re: genphy_read_status() vs. 1000bT Pause capability

[Andrew Lunn]

On Tue, Mar 28, 2017 at 11:49:50AM +1100, Benjamin Herrenschmidt wrote:
> Hi !
> 
> I noticed that flow control isn't being enabled on a system I'm
> working on by default. I've tracked it down to two things:
> 
>  - The realtec.c PHY driver doesn't have Pause or Asym_Pause in
> its exposed capabilities. This is in part because PHY_GBIT_FEATURES
> does not include SUPPORTED_Pause and SUPPORTED_Asym_Pause. Is there
> a specific reason for that ?

Hi Ben

It is worth reading Documentation/networking/phy.txt

The MAC should set SUPPORTED_Pause and SUPPORTED_Asym_Pause if the MAC
supports these features. The PHY will then negotiate them.

	 Andrew

Re: genphy_read_status() vs. 1000bT Pause capability

[Benjamin Herrenschmidt]

On Tue, 2017-03-28 at 03:09 +0200, Andrew Lunn wrote:
> On Tue, Mar 28, 2017 at 11:49:50AM +1100, Benjamin Herrenschmidt wrote:
> > Hi !
> > 
> > I noticed that flow control isn't being enabled on a system I'm
> > working on by default. I've tracked it down to two things:
> > 
> >  - The realtec.c PHY driver doesn't have Pause or Asym_Pause in
> > its exposed capabilities. This is in part because PHY_GBIT_FEATURES
> > does not include SUPPORTED_Pause and SUPPORTED_Asym_Pause. Is there
> > a specific reason for that ?
> 
> Hi Ben
> 
> It is worth reading Documentation/networking/phy.txt
> 
> The MAC should set SUPPORTED_Pause and SUPPORTED_Asym_Pause if the MAC
> supports these features. The PHY will then negotiate them.

Ok. I had added them but hit the other issue with the 1000bT  style
pause.

Cheers,
Ben.

Re: genphy_read_status() vs. 1000bT Pause capability

[Benjamin Herrenschmidt]

On Tue, 2017-03-28 at 13:28 +1100, Benjamin Herrenschmidt wrote:
> 
> > Hi Ben
> > 
> > It is worth reading Documentation/networking/phy.txt
> > 
> > The MAC should set SUPPORTED_Pause and SUPPORTED_Asym_Pause if the MAC
> > supports these features. The PHY will then negotiate them.
> 
Haha ! The OpenBMC kernel is still at 4.7 which was still saying you
should only clear bits in there :-) I think that's what I initially
read.

Thanks for the pointer.

Doesn't fix my other problem with Pause in 1000bT land. Do you know if
that way of reflecting the pause capability by hijacking the old
LPA bits is widely implemented enough that we should put it in
genphy_read_status() ?

Cheers,
Ben.

Re: genphy_read_status() vs. 1000bT Pause capability

[Florian Fainelli]

Hi Ben,

On 03/27/2017 07:55 PM, Benjamin Herrenschmidt wrote:
> On Tue, 2017-03-28 at 13:28 +1100, Benjamin Herrenschmidt wrote:
>>
>>> Hi Ben
>>>
>>> It is worth reading Documentation/networking/phy.txt
>>>
>>> The MAC should set SUPPORTED_Pause and SUPPORTED_Asym_Pause if the MAC
>>> supports these features. The PHY will then negotiate them.
>>
> Haha ! The OpenBMC kernel is still at 4.7 which was still saying you
> should only clear bits in there :-) I think that's what I initially
> read.
> 
> Thanks for the pointer.
> 
> Doesn't fix my other problem with Pause in 1000bT land. Do you know if
> that way of reflecting the pause capability by hijacking the old
> LPA bits is widely implemented enough that we should put it in
> genphy_read_status() ?

Not sure I follow you here? The link partner pause capability is
reflected in phydev->pause and phydev->asym_pause (yes, these are
terrible names) when the link is established. An Ethernet driver is
still supposed to reconcile the locally advertised pause parameters
(auto negotiated, or manually configured) from ethtool_{get,set}_param
and then decide what to do in return (typically advertise or not the
support for pause frames).

The plan is eventually to provide better helper function for PHYLIB
aware Ethernet MAC drivers such that given the local pause settings of
the driver (resolved via ethtool), advertisement and auto-(re)negotation
works as expected, essentially providing something generic ala
tg3_set_pauseparam().

Have not gotten the time to get there yet so if you, or Russell beat me
to it, I'd happily review such patches.
-- 
Florian

Re: genphy_read_status() vs. 1000bT Pause capability

[Benjamin Herrenschmidt]

On Mon, 2017-03-27 at 21:14 -0700, Florian Fainelli wrote:
> > Doesn't fix my other problem with Pause in 1000bT land. Do you know if
> > that way of reflecting the pause capability by hijacking the old
> > LPA bits is widely implemented enough that we should put it in
> > genphy_read_status() ?
> 
> Not sure I follow you here? The link partner pause capability is
> reflected in phydev->pause and phydev->asym_pause (yes, these are
> terrible names) when the link is established. 

Right. The problem is that they aren't for some gigabit links :-)

Basically, in my setup with a PHY which uses genphy_read_status()
(like most of them), I never get those advertised despite the fact
that, I *think* they are supported by the other end (even after fixing
my side of the advertising).

I added a printk inside genphy_read_status() to inspect the result
of the negociation, and this is what I read:

lpa=c1e1 lpagb=3800 adv=de1 common_adv=1e1 common_adv_gb=800

As you can see, LPA doesn't have the Pause bits. *However* it does
have bit 0x80 which can mean ADVERTISE_100HALF, but according to
our own mii.h can also mean ADVERTISE_1000XPAUSE. Similarily it 
has bit 0x100 which can mean ADVERTISE_100FULL but also can mean
ADVERTISE_1000XPSE_ASYM.

In fact we appear to have two functions to interpret them as
such inn the non-uapi mii.h:

	ethtool_adv_to_mii_adv_x
	mii_adv_to_ethtool_adv_x

However they aren't used much in the tree and not at all by the
"genphy" code.

So my question is... when we observe that we have a 1000 link
established, should we use these to "interpret" the LPA bits as
above ?

As it is, we never seem to advertise the capability because we never
decode the above (tried with 2 different PHYs, a Realtek and a
Broadcom) while my Cisco switches, I think, do support Pause.

Hence my question ... how "standard" is the re-use of the LPA bits
for these alternate meanings in 1000bT and should we update genphy
to perform that decoding ?

(I'm trying to download the 802.3 document referenced in the phy.txt
to see if it says anything about it but it's taking forever for some
reason).

Cheers,
Ben.

Re: genphy_read_status() vs. 1000bT Pause capability

[Benjamin Herrenschmidt]

On Tue, 2017-03-28 at 16:17 +1100, Benjamin Herrenschmidt wrote:
> Hence my question ... how "standard" is the re-use of the LPA bits
> for these alternate meanings in 1000bT and should we update genphy
> to perform that decoding ?

And btw, I'm happy to provide patches if we agree on the approach :-)

Cheers,
Ben.

Re: genphy_read_status() vs. 1000bT Pause capability

[Russell King - ARM Linux]

On Tue, Mar 28, 2017 at 04:17:18PM +1100, Benjamin Herrenschmidt wrote:
> I added a printk inside genphy_read_status() to inspect the result
> of the negociation, and this is what I read:
> 
> lpa=c1e1 lpagb=3800 adv=de1 common_adv=1e1 common_adv_gb=800
> 
> As you can see, LPA doesn't have the Pause bits. *However* it does
> have bit 0x80 which can mean ADVERTISE_100HALF, but according to
> our own mii.h can also mean ADVERTISE_1000XPAUSE. Similarily it 
> has bit 0x100 which can mean ADVERTISE_100FULL but also can mean
> ADVERTISE_1000XPSE_ASYM.

The 1000X definitions are for 1000BaseX (fiber), not for 1000BaseT
(copper).

I have a working setup here with gigabit pause.  The pause bits come
from bits 11 and 10, just like for older copper PHYs.  Your link
parter is indicating that it has no pause capability.  That means
you can't use pause.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: genphy_read_status() vs. 1000bT Pause capability

[Benjamin Herrenschmidt]

On Tue, 2017-03-28 at 10:42 +0100, Russell King - ARM Linux wrote:
> The 1000X definitions are for 1000BaseX (fiber), not for 1000BaseT
> (copper).
> 
> I have a working setup here with gigabit pause.  The pause bits come
> from bits 11 and 10, just like for older copper PHYs.  Your link
> parter is indicating that it has no pause capability.  That means
> you can't use pause.

Interesting. I tried and it worked :-) Could be something funny in
the config of our switches.

Thanks for the explanation.

Cheers,
Ben.

Re: genphy_read_status() vs. 1000bT Pause capability

[Russell King - ARM Linux]

On Tue, Mar 28, 2017 at 10:11:24PM +1100, Benjamin Herrenschmidt wrote:
> On Tue, 2017-03-28 at 10:42 +0100, Russell King - ARM Linux wrote:
> > The 1000X definitions are for 1000BaseX (fiber), not for 1000BaseT
> > (copper).
> > 
> > I have a working setup here with gigabit pause.  The pause bits come
> > from bits 11 and 10, just like for older copper PHYs.  Your link
> > parter is indicating that it has no pause capability.  That means
> > you can't use pause.
> 
> Interesting. I tried and it worked :-) Could be something funny in
> the config of our switches.

It could be that the switch supports pause frames, but it's not
advertised because there's some corner cases that it doesn't work.  I
know that SolidRun have run into switches that corrupt ethernet frames
when pause is used.  (I don't know off hand which they are, but I could
ask the question.)

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: genphy_read_status() vs. 1000bT Pause capability

[Benjamin Herrenschmidt]

On Tue, 2017-03-28 at 12:31 +0100, Russell King - ARM Linux wrote:
> > Interesting. I tried and it worked :-) Could be something funny in
> > the config of our switches.
> 
> It could be that the switch supports pause frames, but it's not
> advertised because there's some corner cases that it doesn't work.  I
> know that SolidRun have run into switches that corrupt ethernet frames
> when pause is used.  (I don't know off hand which they are, but I could
> ask the question.)

I think this is Cisco gear, I'll check with our lab guy today. In the
meantime I'll test my driver back to back with some other machines, I
should find something that does pause eventually :-)

It's handy when you have a gigabit MAC on a 400Mhz ARM9 to be able to
throttle the peer despite all the other problems with Pause :-)

Cheers,
Ben.