Re: [PATCH net-next v4 06/15] net: dsa: vsc73xx: add port_stp_state_set function

On Tue, Feb 13, 2024 at 11:03:19PM +0100, Pawel Dembicki wrote:

This isn't a fully functional implementation of 802.1D, but
port_stp_state_set is required for a future tag8021q operations.

This implementation handles properly all states, but vsc73xx doesn't
forward STP packets.

Reviewed-by: Linus Walleij <redacted>
Signed-off-by: Pawel Dembicki <redacted>
---
v4:
  - fully reworked port_stp_state_set

A "fully reworked" patch isn't exactly compatible with keeping attached
Reviewed-by tags attached to the patch. It implies Linus didn't actually
review the code you are submitting now.

Why have you chosen the name "bool configure" exactly? It is not
intuitive. Could you just pass the STP state of the port as an argument?

+{
+	struct dsa_port *dp = dsa_to_port(ds, port);
+	struct vsc73xx *vsc = ds->priv;
+	int i;
+
+	if (configure) {
+		u16 mask = BIT(CPU_PORT);
+
+		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
+				    VSC73XX_SRCMASKS + CPU_PORT, BIT(port), BIT(port));
+
+		if (dp->bridge) {
+			struct dsa_port *other_dp;
+
+			dsa_switch_for_each_user_port(other_dp, ds) {
+				if (other_dp->bridge == dp->bridge &&
+				    other_dp->index != port &&
+				    other_dp->stp_state == BR_STATE_FORWARDING) {
+					int other_port = other_dp->index;
+
+					mask |= BIT(other_port);
+					vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
+							    VSC73XX_SRCMASKS + other_port,
+							    BIT(port), BIT(port));
+				}
+			}
+		}
+
+		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_SRCMASKS + port,
+				    VSC73XX_SRCMASKS_PORTS_MASK, mask);
+	} else {
+		for (i = 0; i < vsc->ds->num_ports; i++) {

Try to refrain from doing plain integer iterations through ports when
there are holes in the port indices. Use dsa_switch_for_each_available_port()
which skips the unused/absent ports, and consequently you're not
touching unwanted registers.

+			if (i == port)
+				vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
+						    VSC73XX_SRCMASKS + i,
+						    VSC73XX_SRCMASKS_PORTS_MASK, 0);
+			else
+				vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
+						    VSC73XX_SRCMASKS + i, BIT(port), 0);
+		}
+	}

To save on indentation, I would simplify the big if () else () block
like this.

	if (state != BR_STATE_FORWARDING) {
		/* Ports that aren't in the forwarding state must not
		 * forward packets anywhere.
		 */
		vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
				    VSC73XX_SRCMASKS + port,
				    VSC73XX_SRCMASKS_PORTS_MASK, 0);

		dsa_switch_for_each_available_port(other_dp, ds) {
			if (dp == other_dp)
				continue;

			vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
					    VSC73XX_SRCMASKS + other_dp->index,
					    BIT(port), 0);
		}

		return;
	}

	/* Forwarding ports must forward to the CPU and to other ports
	 * in the same bridge
	 */
	vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
			    VSC73XX_SRCMASKS + CPU_PORT, BIT(port), BIT(port));

	mask = BIT(CPU_PORT);

	if (dp->bridge) {
		struct dsa_port *other_dp;

		dsa_switch_for_each_user_port(other_dp, ds) {
			if (other_dp->bridge == dp->bridge &&
			    other_dp->index != port &&
			    other_dp->stp_state == BR_STATE_FORWARDING) {
				int other_port = other_dp->index;

				mask |= BIT(other_port);
				vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
						    VSC73XX_SRCMASKS + other_port,
						    BIT(port), BIT(port));
			}
		}
	}

	vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_SRCMASKS + port,
			    VSC73XX_SRCMASKS_PORTS_MASK, mask);

+}
+
+/* FIXME: STP frames aren't forwarded at this moment. BPDU frames are
+ * forwarded only from and to PI/SI interface. For more info see chapter
+ * 2.7.1 (CPU Forwarding) in datasheet.
+ * This function is required for tag8021q operations.

Please keep the name consistent at "tag_8021q".

+ */
+

nitpick: remove the blank line.

+static void vsc73xx_port_stp_state_set(struct dsa_switch *ds, int port, u8 state)
+{
+	struct vsc73xx *vsc = ds->priv;
+	u32 val;
+
+	val = (state == BR_STATE_BLOCKING || state == BR_STATE_DISABLED) ?
+	      0 : BIT(port);
+	vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
+			    VSC73XX_RECVMASK, BIT(port), val);
+
+	val = (state == BR_STATE_LEARNING || state == BR_STATE_FORWARDING) ?
+	      BIT(port) : 0;
+	vsc73xx_update_bits(vsc, VSC73XX_BLOCK_ANALYZER, 0,
+			    VSC73XX_LEARNMASK, BIT(port), val);

I am extremely particular about this. Please also implement .port_pre_bridge_flags()
and .port_bridge_flags(), and treat the BR_LEARNING flag. The idea is
that standalone ports are in the BR_STATE_FORWARDING state (it's a DSA
API thing), but learning should _not_ be enabled (it breaks things when
one port sees traffic originated from the other). So you need to set the
bit in VSC73XX_LEARNMASK only if the port is in the BR_STATE_LEARNING ||
BR_STATE_FORWARDING, _and_ the BR_LEARNING flag is set.

This should be relatively easy to do. If you're not going to do it now
because of patch set size constraints, at least leave a big FIXME in the
code or in the commit message.