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

["Russell King (Oracle)" <linux@armlinux.org.uk>]

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!