Message-ID: <Y2UbK8/LLJwIZ3st@shell.armlinux.org.uk>
Date:   Fri, 4 Nov 2022 14:01:15 +0000
From:   "Russell King (Oracle)" <linux@...linux.org.uk>
To:     Vladimir Oltean <vladimir.oltean@....com>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        "UNGLinuxDriver@...rochip.com" <UNGLinuxDriver@...rochip.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Sean Anderson <sean.anderson@...o.com>,
        Colin Foster <colin.foster@...advantage.com>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net-next 4/4] net: dsa: remove phylink_validate() method

On Fri, Nov 04, 2022 at 01:32:48PM +0000, Vladimir Oltean wrote:
> On Fri, Nov 04, 2022 at 11:35:08AM +0000, Russell King (Oracle) wrote:
> > On Fri, Nov 04, 2022 at 11:24:44AM +0000, Russell King (Oracle) wrote:
> > > There is one remaining issue that needs to be properly addressed,
> > > which is the bcm_sf2 driver, which is basically buggy. The recent
> > > kernel build bot reports reminded me of this.
> > > 
> > > I've tried talking to Florian about it, and didn't make much progress,
> > > so I'm carrying a patch in my tree which at least makes what is
> > > provided to phylink correct.
> > > 
> > > See
> > > http://git.armlinux.org.uk/cgit/linux-arm.git/commit/?h=net-queue&id=63d77c1f9db167fd74994860a4a899df5c957aab
> > > and all the FIXME comments in there.
> > > 
> > > This driver really needs to be fixed before we kill DSA's
> > > phylink_validate method (although doing so doesn't change anything
> > > in mainline, but will remove my reminder that bcm_sf2 is still
> > > technically broken.)
> > 
> > Here's the corrected patch, along with a bit more commentry about the
> > problems that I had kicking around in another commit.
> 
> The inconsistencies in the sf2 driver seem valid - I don't know why/if
> the hardware doesn't support flow control on MoCA, internal ports and
> (some but not all?!) RGMII modes. I hope Florian can make some clarifications.
> 
> However, I don't exactly understand your choice of fixing this
> inconsistency (by providing a phylink_validate method). Why don't you
> simply set MAC_ASYM_PAUSE | MAC_SYM_PAUSE in config->mac_capabilities
> from within bcm_sf2_sw_get_caps(), only if we know this is an xMII port
> (and not for MoCA and internal PHYs)? Then, phylink_generic_validate()
> would know to exclude the "pause" link modes, right?

bcm_sf2_sw_get_caps() doesn't have visibility of which interface mode
will be used.

In any case, the patch is not meant for merging, it is meant to provoke
discussion about how to fix the driver, identifying the places in the
driver where it is broken and why.

I'd have fixed it if I could see a solution to the problems, but the
driver is rather self-contradictory, which makes identifying what it
actually supports rather impossible.

For example, does the driver actually work if
PHY_INTERFACE_MODE_RGMII_RXID or PHY_INTERFACE_MODE_RGMII_ID are used,
or does it fail because the port mode is set incorrectly in the RGMII
control register? Should these two interface modes be included in the
switch() to set port_mode to EXT_GPHY or should they be dropped from
the list of supported interfaces. If they should be dropped from the
list of supported interfaces, then that makes sense why we only program
pause modes for PHY_INTERFACE_MODE_RGMII and
PHY_INTERFACE_MODE_RGMII_TXID, and not the other two. Then there's
questions whether this is acting as a MAC end of the RGMII link or
the PHY end, so whether it should even be acting upon the delay
settings in the phy interface mode. If it's the MAC end then it ought
to be allowing all and dealing with the pause mode irrespective of
the RGMII mode. If it's acting as the PHY end, then maybe it only
supports the RGMII and RGMII_TXID modes, in which case the other
two don't matter.

There's too much ambiguity in the driver to derive what's actually
going on here, so the only thing I can do is raise the issues and
hope for a resolution.

What I might do is trim the patch down and submit it - mostly as a
patch adding those FIXME comments to the driver, possibly also
making the driver print a big fat warning when it's initialised in
the hope of finding someone who can at least run some tests on the
hardware.

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

Powered by blists - more mailing lists