netdev - Re: [PATCH net-next 2/3] net: dsa: allow DSA switch drivers to provide their own phylink mac ops

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: <ZhVs41dODkA/B7JH@shell.armlinux.org.uk>
Date: Tue, 9 Apr 2024 17:29:23 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Vladimir Oltean <olteanv@...il.com>
Cc: Andrew Lunn <andrew@...n.ch>, "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Florian Fainelli <f.fainelli@...il.com>,
	Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
	Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net-next 2/3] net: dsa: allow DSA switch drivers to
 provide their own phylink mac ops

On Tue, Apr 09, 2024 at 06:33:46PM +0300, Vladimir Oltean wrote:
> On Tue, Apr 09, 2024 at 03:37:31PM +0300, Vladimir Oltean wrote:
> > On Mon, Apr 08, 2024 at 12:19:25PM +0100, Russell King (Oracle) wrote:
> > > +static void dsa_shared_port_link_down(struct dsa_port *dp)
> > > +{
> > > +	struct dsa_switch *ds = dp->ds;
> > > +
> > > +	if (ds->phylink_mac_ops) {
> > > +		if (ds->phylink_mac_ops->mac_link_down)
> > > +			ds->phylink_mac_ops->mac_link_down(&dp->pl_config,
> > > +							   MLO_AN_FIXED,
> > > +							   PHY_INTERFACE_MODE_NA);
> > > +	} else {
> > > +		if (ds->ops->phylink_mac_link_down)
> > > +			ds->ops->phylink_mac_link_down(ds, dp->index,
> > > +				MLO_AN_FIXED, PHY_INTERFACE_MODE_NA);
> > > +	}
> > > +}
> > 
> > Please roll this other change into the patch when respinning:
> > 
> > else {
> > 	if { }
> > }
> > 
> > becomes
> > 
> > else if {}

This would destroy the symmetry that I think aids readability. I did
consider it at the time and decided against it.

> Something like this:
> 
> static void dsa_shared_port_link_down(struct dsa_port *dp)
> {
> 	struct dsa_switch *ds = dp->ds;
> 
> 	if (ds->phylink_mac_ops && ds->phylink_mac_ops->mac_link_down) {
> 		ds->phylink_mac_ops->mac_link_down(&dp->pl_config, MLO_AN_FIXED,
> 						   PHY_INTERFACE_MODE_NA);
> 	} else if (ds->ops->phylink_mac_link_down) {
> 		ds->ops->phylink_mac_link_down(ds, dp->index, MLO_AN_FIXED,
> 					       PHY_INTERFACE_MODE_NA);
> 	}

This changes the logic - it allows driver authors to provide the
MAC operations, omit the mac_link_down() op _and_ an
ops->phylink_mac_link_down() function. This could lead to buggy
drivers since this will only happen in this path and none of the
others.

I want this to be an "either provide phylink_mac_ops, and thus
none of the phylink_mac_* ops in dsa_switch_ops will be called" or
"don't provide phylink_mac_ops and the phylink_mac_* ops in
dsa_switch_ops will be called". It's then completely clear cut
that it's one or the other, whereas the code above makes it
unclear.

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