Message-ID: <ZZQckTKmEjSbgCDo@shell.armlinux.org.uk>
Date: Tue, 2 Jan 2024 14:24:17 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Vladimir Oltean <vladimir.oltean@....com>
Cc: netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Andrew Lunn <andrew@...n.ch>,
	Heiner Kallweit <hkallweit1@...il.com>
Subject: Re: [PATCH net-next] net: phylink: avoid one unnecessary
 phylink_validate() call during phylink_create()

On Thu, Dec 14, 2023 at 05:18:35PM +0000, Russell King (Oracle) wrote:
> I'll say this for the benefit of the netdev developers - my time is
> currently being spent elsewhere (e.g. ARM64 VCPU hotplug) so I don't
> have a lot of time to review netdev patches. With only a few days
> left to Christmas vacations and my intention not to work over that
> period, this means that I'm unlikely to be able to review changes
> such as this - and they do need review.
> 
> If I get some spare time, then I will review them.
> 
> However, probably best to wait until the new year before sending
> patches that touch phylink - which will involve adding stress on my
> side.

So... getting back to this as I have platforms I can test with in front
of me now... On October 5th, you asked by email what the purpose of
this was. I replied same day, stating the purpose of it, citing mvneta.
I did state in that email about reporting "absolutely nothing" to
userspace, but in fact it's the opposite - without this validate()
call, we report absolutely everything.

With it removed, booting my Armada 388 Clearfog platform with the
ethernet interface not having been brought up, and then running ethtool
on it, this is what I get:

# ethtool eno0
Settings for eno0:
        Supported ports: [ TP    AUI     MII     FIBRE   BNC     Backplane ]
        Supported link modes:   10baseT/Half 10baseT/Full
                                100baseT/Half 100baseT/Full
                                1000baseT/Half 1000baseT/Full
                                10000baseT/Full
                                2500baseX/Full
                                1000baseKX/Full
                                10000baseKX4/Full
                                10000baseKR/Full
                                10000baseR_FEC
                                20000baseMLD2/Full
                                20000baseKR2/Full
                                40000baseKR4/Full
                                40000baseCR4/Full
                                40000baseSR4/Full
                                40000baseLR4/Full
                                56000baseKR4/Full
                                56000baseCR4/Full
                                56000baseSR4/Full
                                56000baseLR4/Full
                                25000baseCR/Full
                                25000baseKR/Full
                                25000baseSR/Full
                                50000baseCR2/Full
                                50000baseKR2/Full
                                100000baseKR4/Full
                                100000baseSR4/Full
                                100000baseCR4/Full
                                100000baseLR4_ER4/Full
                                50000baseSR2/Full
                                1000baseX/Full
                                10000baseCR/Full
                                10000baseSR/Full
                                10000baseLR/Full
                                10000baseLRM/Full
                                10000baseER/Full
                                2500baseT/Full
                                5000baseT/Full
                                50000baseKR/Full
                                50000baseSR/Full
                                50000baseCR/Full
                                50000baseLR_ER_FR/Full
                                50000baseDR/Full
                                100000baseKR2/Full
                                100000baseSR2/Full
                                100000baseCR2/Full
                                100000baseLR2_ER2_FR2/Full
                                100000baseDR2/Full
                                200000baseKR4/Full
                                200000baseSR4/Full
                                200000baseLR4_ER4_FR4/Full
                                200000baseDR4/Full
                                200000baseCR4/Full
                                100baseT1/Full
                                1000baseT1/Full
                                400000baseKR8/Full
                                400000baseSR8/Full
                                400000baseLR8_ER8_FR8/Full
                                400000baseDR8/Full
                                400000baseCR8/Full
                                100000baseKR/Full
                                100000baseSR/Full
                                100000baseLR_ER_FR/Full
                                100000baseCR/Full
                                100000baseDR/Full
                                200000baseKR2/Full
                                200000baseSR2/Full
                                200000baseLR2_ER2_FR2/Full
                                200000baseDR2/Full
                                200000baseCR2/Full
                                400000baseKR4/Full
                                400000baseSR4/Full
                                400000baseLR4_ER4_FR4/Full
                                400000baseDR4/Full
                                400000baseCR4/Full
                                100baseFX/Half 100baseFX/Full
                                10baseT1L/Full
                                800000baseCR8/Full
                                800000baseKR8/Full
                                800000baseDR8/Full
                                800000baseDR8_2/Full
                                800000baseSR8/Full
                                800000baseVR8/Full
                                10baseT1S/Full
                                10baseT1S/Half
                                10baseT1S_P2MP/Half
        Supported pause frame use: Symmetric Receive-only
        Supports auto-negotiation: Yes
        Supported FEC modes: None        RS      BASER   LLRS
        Advertised link modes:  Not reported
        Advertised pause frame use: No
        Advertised auto-negotiation: No
        Advertised FEC modes: Not reported
        Speed: Unknown!
        Duplex: Half
        Auto-negotiation: off
        Port: MII
        PHYAD: 0
        Transceiver: internal
        Supports Wake-on: d
        Wake-on: d
        Link detected: no

Does that look like sensible output for a network interface that can
only do up to 2.5G speeds to you?

The phylink_validate() there serves to restrict the link modes to
something sensible in the case where we are expecting a PHY to be
attached to the interface, but the MAC driver attaches the PHY at
open time, but the network interface has yet to be opened.

So... I completely disagree with removing this phylink_validate()
call - it's there so we report something sane to userspace for
network drivers that attach PHYs at open time before that PHY is
attached.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Powered by blists - more mailing lists