| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <VI1PR0402MB360027E31680BD74F8514A5FFFF90@VI1PR0402MB3600.eurprd04.prod.outlook.com>
Date: Fri, 19 Oct 2018 07:07:12 +0000
From: Andy Duan <fugang.duan@....com>
To: Heiner Kallweit <hkallweit1@...il.com>,
Florian Fainelli <f.fainelli@...il.com>,
LABBE Corentin <clabbe@...libre.com>
CC: "andrew@...n.ch" <andrew@...n.ch>,
"davem@...emloft.net" <davem@...emloft.net>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH] net: ethernet: fec: Add missing SPEED_
From: Heiner Kallweit <hkallweit1@...il.com> 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
Powered by blists - more mailing lists