stp: latch the block on a looped port

Two of our own ports on one segment blocked each other in turn instead
of settling. The guard on the loop path only acted when port_timers[]
had already run out, so a BPDU arriving while the port was blocked did
nothing: the timer expired, the port went forwarding, the loop reopened
and the pair started over. The comment above the code claimed the
opposite - "if the loop persists the BPDUs keep arriving and the port
stays blocked" - but nothing implemented it.

Measured on a SWTGW218AS with a patch cord between two free ports: both
ports blocked, both returned to forwarding one forward delay later, and
the topology-change counter reached 0x51 in 5.5 minutes - 15.6 changes
per minute for as long as the cable was in.

Let the better Port ID decide for both. That port is forwarding by
construction, so it goes on hearing the loop and re-arms the other
port's timer on every BPDU, which is what turns the block into a latch;
the held port only has to keep transmitting, which the send path already
allows in any MSTP state. Nothing here depends on a blocked port still
receiving - that was never established. Having one writer also removes a
race: while both ends decided for themselves, the winner's re-arm could
land in the loser's port_timers[] first, the loser read it as "already
blocked" and skipped its own state change, and the loop stayed open.

802.1D compares the priority before the number and stp_cnf_send() puts
stp_pprio[] on the wire next to it, so compare that first - otherwise
"stp port N prio" would quietly not influence which end of a looped pair
keeps forwarding.

The port number arrives in a frame and our bridge MAC is public in every
BPDU we send, so bound it to the ports this module manages before
indexing anything. Outside that range nothing would release the block
either: stp_timers() walks min_port..max_port and skips ports that are
not STP-enabled, so their port_timers[] never counts down.

Equal Port IDs mean the frame came back on the port it left - a loop
further out, behind an unmanaged switch. There is no pair to choose
from, so that port holds itself down; since it can only re-arm while it
is receiving, that case stays the forward-delay pulse it was before
rather than becoming a real latch.

The work sits in a __reentrant helper on purpose, like the two functions
above it: parameters and locals then live on the stack. Inlined into
stp_in(), which is __banked and whose temporaries cannot be overlaid,
the same code costs two more bytes of DSEG - enough to stop an image
that also carries LACP from linking at all.

Verified on hardware: with the loop in place for 1 h 36 min exactly one
port blocked, the other kept forwarding, and the topology-change counter
moved four times in total - three of them the link event and the
promotion in the first minute.

rtl837x_stp.c

@@ -10,8 +10,9 @@
  * The engine itself stays deliberately simple (no proposal/agreement
  * handshake, no full port-role machine): we elect a root from received BPDUs,
  * promote ports to forwarding after the listen period (or immediately for
- * edge ports), age the root out via max age, and block ports on which we see
+ * edge ports), age the root out via max age, and block on a loop (our own
- * our own BPDU (loop!) or - with root guard - a better root.
+ * BPDU comes back: the worse Port ID of the pair stops forwarding and stays
+ * blocked while it keeps coming back) or - with root guard - a better root.
  */
 
 // #define REGDBG
@@ -102,6 +103,7 @@ __xdata uint8_t  stp_msg_age;		/* message age of the root info we hold, seconds
 __xdata uint16_t stp_tc_while;		/* ticks left to set the TC flag in our BPDUs */
 __xdata uint8_t  stp_i;		/* shared loop iterator (DSEG relief) */
 __xdata uint32_t stp_cost_scratch;
+__xdata uint8_t  stp_loop_peer;		/* the other own port seen on a looped segment */
 
 #define STP_EDGE_DELAY	(3 * STP_HZ)	/* auto-edge: forward after 3 s without BPDU */
 
@@ -200,6 +202,43 @@ static void stp_topology_change(uint8_t port) __reentrant
 }
 
 
+/* Hold one port out of forwarding because a loop was seen on it, and keep
+ * holding it for as long as the caller keeps saying so. The caller is the
+ * port that won the Port ID compare (see stp_in) - a different port than
+ * the one held, except when the frame came back on the port it left.
+ *
+ * Reentrant on purpose, like the two above: parameters and locals then live
+ * on the stack instead of taking internal RAM of their own. Inlined into
+ * stp_in - which is __banked, so its temporaries cannot be overlaid - the
+ * same code costs two more bytes of DSEG, and that is enough to stop the
+ * image with LACP from linking at all. */
+static void stp_loop_hold_peer(uint8_t port) __reentrant
+{
+	/* The port number arrives in a BPDU, so it is somebody else's data,
+	 * and our own bridge MAC is public in every BPDU we send - a forged
+	 * frame can name any port it likes. Bound it to the ports this module
+	 * actually manages, like every other loop here does. Out of that
+	 * range nothing would ever release the block either: stp_timers()
+	 * walks min_port..max_port and skips ports that are not STP-enabled,
+	 * so their port_timers[] never counts down. Naming the CPU port would
+	 * otherwise cost us our own management path. */
+	if (port < machine.min_port || port > machine.max_port)
+		return;
+	if (!(stp_pflags[port] & STP_PF_ENABLED))
+		return;
+	if (stp_pflags[port] & STP_PF_TRIPPED)
+		return;
+	if (!port_timers[port]) {		/* not held down yet */
+		print_string("STP: loop detected, blocking port ");
+		print_byte(port); write_char('\n');
+		stp_state_set(port, 0b01);
+		stp_pflags[port] &= ~STP_PF_OPEREDGE;
+		stp_topology_change(port);
+	}
+	port_timers[port] = (uint16_t)stp_fwddelay_s * STP_HZ;
+}
+
+
 /* Take the bridge back as root of its own tree (initial state / root aged out) */
 static void stp_claim_root(void)
 {
@@ -378,18 +417,52 @@ void stp_in(void) __banked
 	if (stp_rxlen < 64)
 		return;
 
-	/* Our own BPDU coming back at us = a loop in the network. Block the port
+	/* Our own BPDU coming back at us: two of our ports sit on one segment.
-	 * for a listen period; if the loop persists the BPDUs keep arriving and
+	 * Only the one with the worse Port ID has to stop forwarding - 802.1D
-	 * the port stays blocked. */
+	 * calls it a backup port. Blocking both, as we used to, kills a segment
+	 * that can still carry traffic, and worse, leaves nobody forwarding to
+	 * hear the loop: both then time out of blocking together and the pair
+	 * oscillates for as long as the cable is in (measured: a topology change
+	 * every ~4 s).
+	 *
+	 * The port with the better Port ID decides for both and is the only one
+	 * that touches state - the other just drops the frame. One writer is
+	 * what makes this safe: while both were still deciding for themselves,
+	 * the winner's re-arm landed in the loser's port_timers[] first, the
+	 * loser then read it as "already blocked" and skipped its own
+	 * stp_state_set(), and the loop stayed open. Whether that happened came
+	 * down to which frame the switch handed us first.
+	 *
+	 * The winner is forwarding by construction (nothing here ever blocks
+	 * it), so it goes on hearing the loop and re-arms the loser's timer on
+	 * every BPDU - that is what makes the block a latch rather than a
+	 * forward-delay pulse, and it needs no assumption about a blocked port
+	 * still receiving. Pull the cable and the re-arming stops, so the loser
+	 * comes back on its own after a forward delay - and the link
+	 * supervision above gets there first anyway. */
 	if (cmpMAC(STP_I->bridge.mac, uip_ethaddr.addr) == 0) {
-		if (port_timers[port] == 0 && !(stp_pflags[port] & STP_PF_TRIPPED)) {
+		/* Equal means the frame came back on the port it left: a loop
-			print_string("STP: loop detected on port ");
+		 * further out, behind an unmanaged switch. There is no pair to
-			print_byte(port); write_char('\n');
+		 * pick from, so that port holds itself down - and since it can
-			stp_state_set(port, 0b01);
+		 * only re-arm while it is receiving, that case degrades to the
-			port_timers[port] = (uint16_t)stp_fwddelay_s * STP_HZ;
+		 * forward-delay pulse we had before rather than a real latch.
-			stp_pflags[port] &= ~STP_PF_OPEREDGE;
+		 * The peer's number is validated by the callee, not here. */
-			stp_topology_change(port);
+		stp_loop_peer = STP_I->port_id;		/* 1-based, as we send it */
+		if (!stp_loop_peer)
+			return;
+		stp_loop_peer--;
+		/* A Port ID is (priority, number) and priority is compared
+		 * first - stp_cnf_send() puts stp_pprio[] on the wire next to
+		 * the number, so "stp port N prio" has to be able to decide
+		 * which end of a looped pair keeps forwarding. Comparing the
+		 * number alone would quietly ignore it. */
+		if (STP_I->port_prio != stp_pprio[port]) {
+			if (STP_I->port_prio < stp_pprio[port])
+				return;			/* peer is better: it decides */
+		} else if (stp_loop_peer < port) {
+			return;
 		}
+		stp_loop_hold_peer(stp_loop_peer);
 		return;
 	}