On Wed, Mar 16, 2022 at 10:15:18AM +0100, Tobias Waldekranz wrote:

quoted

quoted

+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.