netdev - Re: [net-next PATCH v8 3/4] net: dsa: Add Airoha AN8855 5-Port Gigabit DSA Switch driver

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: <Z1AfM5PzlTN19hF3@shell.armlinux.org.uk>
Date: Wed, 4 Dec 2024 09:21:55 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Maxime Chevallier <maxime.chevallier@...tlin.com>
Cc: Christian Marangi <ansuelsmth@...il.com>, Andrew Lunn <andrew@...n.ch>,
	Florian Fainelli <f.fainelli@...il.com>,
	Vladimir Oltean <olteanv@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Matthias Brugger <matthias.bgg@...il.com>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
	linux-arm-kernel@...ts.infradead.org,
	linux-mediatek@...ts.infradead.org, netdev@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	upstream@...oha.com
Subject: Re: [net-next PATCH v8 3/4] net: dsa: Add Airoha AN8855 5-Port
 Gigabit DSA Switch driver

On Wed, Dec 04, 2024 at 10:09:22AM +0100, Maxime Chevallier wrote:
> Hello Christian,
> 
> On Wed,  4 Dec 2024 08:24:10 +0100
> Christian Marangi <ansuelsmth@...il.com> wrote:
> 
> > +static void
> > +an8855_phylink_mac_config(struct phylink_config *config, unsigned int mode,
> > +			  const struct phylink_link_state *state)
> > +{
> > +	struct dsa_port *dp = dsa_phylink_to_port(config);
> > +	struct dsa_switch *ds = dp->ds;
> > +	struct an8855_priv *priv;
> > +	int port = dp->index;
> > +
> > +	priv = ds->priv;
> > +
> > +	if (port != 5) {
> > +		if (port > 5)
> > +			dev_err(ds->dev, "unsupported port: %d", port);
> > +		return;
> > +	}
> 
> Looks like the above condition can be simplified to :
> 
> 	if (port > 5)
> 		dev_err(...);

Not quite, since if port is 0..4, the function returns at this point.
If >5, then it prints an error and returns. So:

	if (port > 5)
		dev_err(...);
	if (port != 5)
		return;

However, net/dsa/dsa.c::dsa_switch_parse_ports_of() already validates
that the port number will not exceed ds->num_ports, so if that is set
appropriately, the phylink functions will never be called with a port
number larger than this, and checking it is redundant.

So I think this should just be:

	if (port != 5)
		return;

> > +	case 5:
> > +		phy_interface_set_rgmii(config->supported_interfaces);
> > +		__set_bit(PHY_INTERFACE_MODE_SGMII,
> > +			  config->supported_interfaces);
> > +		__set_bit(PHY_INTERFACE_MODE_2500BASEX,
> > +			  config->supported_interfaces);
> > +		break;
> > +	}
> > +
> > +	config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
> > +				   MAC_10 | MAC_100 | MAC_1000FD;
> 
> For port 5, you may also add the MAC_2500FD capability as it supports
> 2500BASEX ?

Agreed, that's needed.

> > +		case SPEED_5000:
> > +			reg |= AN8855_PMCR_FORCE_SPEED_5000;
> > +			break;
> 
> There's no mention of any mode that can give support for the 5000Mbps
> speed, is it a leftover from previous work on the driver ?

Or maybe it's because 5GBASE-R wasn't a phy interface type when the
driver was originally developed (iirc it's a relatively recent
addition.)

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