netdev - Re: [PATCH RFC russell-king 3/4] net: phy: marvell10g: change MACTYPE according to phydev->interface

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200812155441.GR1551@shell.armlinux.org.uk>
Date:   Wed, 12 Aug 2020 16:54:41 +0100
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Marek Behún <marek.behun@....cz>,
        Maxime Chevallier <maxime.chevallier@...tlin.com>,
        Baruch Siach <baruch@...s.co.il>,
        Chris Healy <cphealy@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>, netdev@...r.kernel.org
Subject: Re: [PATCH RFC russell-king 3/4] net: phy: marvell10g: change
 MACTYPE according to phydev->interface

On Wed, Aug 12, 2020 at 05:44:36PM +0200, Andrew Lunn wrote:
> > I'm aware of that problem.  I have some experimental patches which add
> > PHY interface mode bitmaps to the MAC, PHY, and SFP module parsing
> > functions.  I have stumbled on some problems though - it's going to be
> > another API change (and people are already whinging about the phylink
> > API changing "too quickly", were too quickly seems to be defined as
> > once in three years), and in some cases, DSA, it's extremely hard to
> > work out how to properly set such a bitmap due to DSA's layered
> > approach.
> 
> Hi Russell
> 
> If DSAs layering is causing real problems, we could rip it out, and
> let the driver directly interact with phylink. I'm not opposed to
> that.

The reason I mentioned it is because I have this unpublished patch
(beware, whitespace damaged due to copy-n-pasted):

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index c1967e08b017..ba32492f3ec0 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1629,6 +1629,12 @@ static int dsa_slave_phy_setup(struct net_device *slave_dev)

        dp->pl_config.dev = &slave_dev->dev;
        dp->pl_config.type = PHYLINK_NETDEV;
+       __set_bit(PHY_INTERFACE_MODE_SGMII,
+                 dp->pl_config.supported_interfaces);
+       __set_bit(PHY_INTERFACE_MODE_2500BASEX,
+                 dp->pl_config.supported_interfaces);
+       __set_bit(PHY_INTERFACE_MODE_1000BASEX,
+                 dp->pl_config.supported_interfaces);

        /* The get_fixed_state callback takes precedence over polling the
         * link GPIO in PHYLINK (see phylink_get_fixed_state).  Only set

Which clearly is a gross hack - this code certainly has no idea about
what interfaces the port itself supports.  How do we get around that
with DSA layering?

We could add yet-another-driver-call down into the DSA driver for it
to fill in that information and keep the current structure.  However,
is that really the best solution - to have lots of fine grained driver
calls?

DSA feels to be very cumbersome and awkward to modify at least to me.
Almost everything seems to be "add another driver call" at the DSA
layer, followed by "add another sub-driver call" at the mv88e6xxx
layer.

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