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.
