RE: [PATCH] net: ethernet: fec: Add missing SPEED_

[Andy Duan]

From: Heiner Kallweit <hkallweit1@xxxxxxxxx> Sent: 2018å10æ19æ 4:41
> On 18.10.2018 22:10, Florian Fainelli wrote:
> > On 10/18/2018 12:59 PM, LABBE Corentin wrote:
> >> On Thu, Oct 18, 2018 at 12:38:32PM -0700, Florian Fainelli wrote:
> >>> On 10/18/2018 12:16 PM, LABBE Corentin wrote:
> >>>> On Thu, Oct 18, 2018 at 11:55:49AM -0700, Florian Fainelli wrote:
> >>>>> On 10/18/2018 11:47 AM, LABBE Corentin wrote:
> >>>>>> On Thu, Oct 18, 2018 at 11:39:24AM -0700, Florian Fainelli
> wrote:
> >>>>>>> On 10/18/2018 08:05 AM, Corentin Labbe wrote:
> >>>>>>>> Since commit 58056c1e1b0e ("net: ethernet: Use
> phy_set_max_speed() to limit advertised speed"), the fec driver is unable
> to get any link.
> >>>>>>>> This is due to missing SPEED_.
> >>>>>>>
> >>>>>>> But SPEED_1000 is defined in include/uapi/linux/ethtool.h as
> >>>>>>> 1000, so surely this would amount to the same code paths being
> >>>>>>> taken or am I missing something here?
> >>>>>>
> >>>>>> The bisect session pointed your patch, reverting it fix the issue.
> >>>>>> BUT since the fix seemed trivial I sent the patch without more test
> then compile it.
> >>>>>> Sorry, I have just found some minutes ago that it didnt fix the
> issue.
> >>>>>>
> >>>>>> But your patch is still the cause for sure.
> >>>>>>
> >>>>>
> >>>>> What you are writing is really lowering the confidence level,
> >>>>> first Andrew is the author of that patch, and second "just
> >>>>> compiling" and pretending this fixes a problem when it does not is
> >>>>> not quite what I would expect.
> >>>>>
> >>>>> I don't have a problem helping you find the solution or the right
> >>>>> fix though, even if it is not my patch, but please get the author
> >>>>> and actual problem right so we can move forward in confidence,
> thanks!
> >>>>
> >>>> Sorry again, I wanted to acknoledge my error but I did it too fast and
> late.
> >>>> And sorry to have confound you with Andrew.
> >>>
> >>> No worries, here to help, let us know what your bisection points to.
> >>> THanks
> >>
> >> I have added printing of phydev->supported My working kernel (on top
> >> of 58056c1e1b0e + revert patch) got:
> >> [    5.550838] fec_enet_mii_probe 2ff (gbit features)
> >> [    5.555848] fec_enet_mii_probe 2ef (without 1000baseT_Half)
> >> [    5.561620] fec_enet_mii_probe 22ef final (after pause)
> >> [    5.566914] Micrel KSZ9021 Gigabit PHY 2188000.ethernet-1:06:
> attached PHY driver [Micrel KSZ9021 Gigabit PHY]
> (mii_bus:phy_addr=2188000.ethernet-1:06, irq=POLL)
> >> [    8.730751] fec 2188000.ethernet eth0: Link is Up - 1Gbps/Full -
> flow control rx/tx
> >> [    8.788311] Sending DHCP requests ., OK
> >> [    8.832357] IP-Config: Got DHCP answer from 192.168.66.1, my
> address is 192.168.66.58
> >>
> >> the non-working kernel (next-20181015)
> >> [    7.308917] fec_enet_mii_probe 62ff after phy_set_max_speed
> >> [    7.314545] fec_enet_mii_probe 62ef after
> phy_remove_link_mode
> >> [    7.320418] fec_enet_mii_probe 62ef after pause
> >> and then no link
> >>
> >> So it seems that phy_set_max_speed adds bit 14
> >> (ETHTOOL_LINK_MODE_Asym_Pause_BIT)
> >
> > It's not masking it so it must be coming from phy_probe().
> >
> See df8ed346d4a8 ("net: phy: fix flag masking in __set_phy_supported").
> phy_set_max_speed() used to (unintentionally) mask the pause bits and it
> seems that the fec driver used this bug as a feature.
> 
> >>
> >> I have patched by adding:
> >> phy_remove_link_mode(phy_dev,
> ETHTOOL_LINK_MODE_Asym_Pause_BIT);
> 
> Instead of programmatically removing the feature bit it should be possible
> to do this in the PHY driver configuration. See also this part of
> phy_probe().
> 
> 	if (phydrv->features & (SUPPORTED_Pause |
> SUPPORTED_Asym_Pause)) {
> 		phydev->supported &= ~(SUPPORTED_Pause |
> SUPPORTED_Asym_Pause);
> 		phydev->supported |= phydrv->features &
> 				     (SUPPORTED_Pause | SUPPORTED_Asym_Pause);
> 	} else {
> 		phydev->supported |= SUPPORTED_Pause |
> SUPPORTED_Asym_Pause;
> 	}

The ksz9021 phy driver don't set Pause feature,  then the phylib enable "SUPPORTED_Pause" and " SUPPORTED_Asym_Pause" in both.
Micrel.c:
        .phy_id         = PHY_ID_KSZ9021,
        .phy_id_mask    = 0x000ffffe,
        .name           = "Micrel KSZ9021 Gigabit PHY",
        .features       = PHY_GBIT_FEATURES,

From @LABBE Corentin debug,  it seem ksz9021 cannot advertise Pause and Asym pause in both, otherwise it cannot link up.
From ksz9021 datasheet description as below,  Symmetric & Asymmetric PAUSE is for local device,  I don't understand its mean.  
4.11:10 Pause
[0,0] = No PAUSE
[1,0] = Asymmetric PAUSE (link partner)
[0,1] = Symmetric PAUSE
[1,1] = Symmetric & Asymmetric PAUSE (local device)

@ LABBE Corentin, you can try this:
Micrel.c:
        .phy_id         = PHY_ID_KSZ9021,
        .phy_id_mask    = 0x000ffffe,
        .name           = "Micrel KSZ9021 Gigabit PHY",
        .features       = PHY_GBIT_FEATURES | SUPPORTED_Pause,


In fact,  I test net tree without any change with AR8031 and there has no link problem.


Andy