netdev - Re: [PATCH net-next] net: dsa: mark parsed interface mode for legacy switch drivers

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]

Date: Tue, 8 Aug 2023 16:52:15 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>,
	Florian Fainelli <f.fainelli@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org
Subject: Re: [PATCH net-next] net: dsa: mark parsed interface mode for legacy
 switch drivers

On Tue, Aug 08, 2023 at 01:57:43PM +0100, Russell King (Oracle) wrote:
> Thanks for the r-b.
> 
> At risk of delaying this patch through further discussion... so I'll
> say now that we're going off into discussions about future changes.
> 
> I believe all DSA drivers that provide .phylink_get_caps fill in the
> .mac_capabilities member, which leaves just a few drivers that do not,
> which are:
> 
> $ git grep -l dsa_switch_ops.*= drivers/net/dsa/ | xargs grep -L '\.phylink_get_caps'
> drivers/net/dsa/dsa_loop.c
> drivers/net/dsa/mv88e6060.c
> drivers/net/dsa/realtek/rtl8366rb.c
> drivers/net/dsa/vitesse-vsc73xx-core.c
> 
> I've floated the idea to Linus W and Arinc about setting
> .mac_capabilities in the non-phylink_get_caps path as well, suggesting:

Not sure what you mean by "in the non-phylink_get_caps path" (what is
that other path). Don't you mean that we should implement phylink_get_caps()
for these drivers, to have a unified code flow for everyone?

> 
> 	MAC_1000 | MAC_100 | MAC_10 | MAC_SYM_PAUSE | MAC_ASYM_PAUSE
> 
> support more than 1G speeds. I think the only exception to that may
> be dsa_loop, but as I think that makes use of the old fixed-link
> software emulated PHYs, I believe that would be limited to max. 1G
> as well.

I don't believe that dsa_loop makes use of fixed-link at all. Its user
ports use phy/gmii mode through the non-OF-based dsa_slave_phy_connect()
to the ds->slave_mii_bus, and the CPU port goes through the non-OF code
path ("else" block) here (because dsa_loop_bdinfo.c _is_ non-OF-based):

dsa_port_setup:
	case DSA_PORT_TYPE_CPU:
		if (dp->dn) {
			err = dsa_shared_port_link_register_of(dp);
			if (err)
				break;
			dsa_port_link_registered = true;
		} else {
			dev_warn(ds->dev,
				 "skipping link registration for CPU port %d\n",
				 dp->index);
		}

> If we did set .mac_capabilities, then dsa_port_phylink_validate() would
> always call phylink_generic_validate() for all DSA drivers, and at that
> point, we don't need dsa_port_phylink_validate() anymore as it provides
> nothing that isn't already done inside phylink.
> 
> Once dsa_port_phylink_validate() is gone, then I believe there are no
> drivers populating the .validate method in phylink_mac_ops, which
> then means there is the possibility to remove that method.

Assuming I understand correctly, I agree it would be beneficial for
mv88e6060, rtl8366rb and vsc73xx to populate mac_capabilities and
supported_interfaces.