Re: [PATCH v4 net-next 10/15] net: dsa: Validate hardware support for MST

[Vladimir Oltean]

On Wed, Mar 16, 2022 at 10:15:18AM +0100, Tobias Waldekranz wrote:
> >> +int dsa_port_mst_enable(struct dsa_port *dp, bool on,
> >> +			struct netlink_ext_ack *extack)
> >> +{
> >> +	if (!on)
> >> +		return 0;
> >> +
> >> +	if (!dsa_port_supports_mst(dp)) {
> >> +		NL_SET_ERR_MSG_MOD(extack, "Hardware does not support MST");
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >
> > Experimenting a bit... maybe this looks tidier? We make the "if" condition
> > have the same basic structure as the previous "if (br_mst_enabled(br) &&
> > !dsa_port_supports_mst(dp))", albeit transformed using De Morgan's rules.
> >
> > {
> > 	if (!on || dsa_port_supports_mst(dp))
> > 		return 0;
> >
> > 	NL_SET_ERR_MSG_MOD(extack, "Hardware does not support MST");
> > 	return -EINVAL;
> > }
> 
> I initially had it like this. It looks tidier, yes - but to me the
> intent is less obvious when reading it. How about:
> 
> {
> 	if (on && !dsa_port_supports_mst(dp)) {
> 		NL_SET_ERR_MSG_MOD(extack, "Hardware does not support MST");
> 		return -EINVAL;
> 	}
> 
> 	return 0;
> }

Yes, let's go with this.