stp: treat a link aggregation group as one entity by DrDoof · Pull Request #397 · logicog/RTLPlayground

DrDoof · 2026-09-04T10:06:16Z

STP on this tree has no idea link aggregation exists. Every member of a LAG is its own port to it, so it sees several parallel paths to the same neighbour, does exactly what it is meant to do, and blocks all but one of them. A four-link aggregate ends up carrying one link's worth. The LAG side is fine and the STP side is fine; it is the join between them that is missing.

This puts entities above the physical ports, 10 to 13 for the four groups, so an entity is either a port or a LAG and everything downstream stops having to care which. Membership comes from port_lag_members_get() from #351, re-read once a second, so a group that gains or loses ports is picked up without anything else noticing. stp_ent_apply() is the only place that knows the formula for an entity's state, and it writes that state to every member. Members drop out of the port list in stp status and on the STP page, because they are not entities any more, the group is.

With no LAG configured the tree reduces to what is there today on every path. That was the case I most wanted not to break.

Tested on a SWTGW218AS: a static LAG of two ports, then a cable from one member back to an ordinary port on the same switch, so there is a loop through the aggregate. The LAG goes to blocking and the ordinary port stays forwarding, which is the right way round, the aggregate being entity 12 and losing the tie against port 3. The part I wanted to see was the ASIC rather than the status page: regget 5310 came back 0x000ff577, and both members are blocked in silicon, not only the one with the cable in it. Traffic on the looped pair stayed at BPDU rate throughout.

Saved and rebooted with the loop still in place. It latches from the first second and held over 40 s, register 0x000ff5f7.

This is meant to replace #325, which carries the same idea but as 61 commits from before the trunk/lag rename, 45 of which have since landed here by other routes. Happy to close that one in favour of this, or the other way round if you prefer.

Spanning tree so far saw only physical ports. Two ports aggregated towards the same neighbour look to the protocol like two parallel paths, so it blocks one of them and cuts the group in half. Give each of the four groups an STP entity of its own, numbered above the physical ports. A member port answers to its group: BPDUs arriving on it are accounted to the entity, the entity's state is written to every member together, and its BPDUs carry the state of a live member. Ports outside any group keep behaving exactly as before. Membership comes from the aggregation tables once a second; when it changed, the entity's state is reapplied to the new set of members. The CLI takes lag <1-4> wherever it takes port <n>, the web UI shows a group as one row, and the saved config keeps the lag form of the commands.

logicog · 2026-09-04T12:51:09Z

Good call to close #325. This looks much better. Still needs a bit of reviewing (saw a couple of hard-coded things like the CPU-port or the number of LAGs, already), I will do that and testing later today. Also need to think about how to test the LAG part... Maybe with 2 RTL switches? One can also set up LAGs in Linux, but that is really cumbersome, Want to make sure all lag members can be blocked.

DrDoof · 2026-09-04T17:04:39Z

You don't need a second switch, or Linux, for the blocking part — one switch and one cable does it, and the check you want falls out of a register read.

Take two ports with nothing plugged into them and make a static LAG of them. No LACP needed, lag 2 5 6 is enough: STP only cares that the group exists. Then run a single cable from one of those two members to an ordinary port on the same switch. That is a loop through the aggregate, entirely inside the switch, so nothing else on the network is involved and there is nothing to set up on a host.

Then read the MSTP state register, two bits per port from the LSB, 3 forwarding and 1 blocking:

regget 5310  ->  0x000ff577

  log 4 (phys 5)  BLOCK    <- lag member, this one has the cable
  log 5 (phys 6)  BLOCK    <- lag member, no cable in it at all
  log 2 (phys 3)  forward  <- the ordinary port, stays up

Both members are blocked in silicon with only one of them cabled, which is the thing you wanted to be sure of: the decision is taken once for the entity and written to every member, not to the port the BPDU happened to arrive on. stp status agrees ( L2 block, 03 fwd), but the register is the one I would trust, since it sits on the far side of the whole display path.

I also saved and rebooted with the loop still in place, so it exists from the first second rather than being introduced into an already converged tree. It latches immediately and held over 40 s, register 0x000ff5f7. Traffic on the looped pair stayed at BPDU rate throughout, so the loop is extinguished rather than merely labelled.

On the hard-coded values, you are right on both. The port count is the bare 10 the file already used before this change, and it is really CPU_PORT + 1 from rtl837x_common.h; STP_LAG_BASE is that same number written once more. The LAG count is the worse of the two — 4 appears here, as group > 3 in parse_lag, and again in the LACP branch, with no shared define anywhere.

I would rather not move the code while you are reading it. Say when you are through and I will fold both into named constants, or tell me to keep it inside this file and leave parse_lag for a separate change.

logicog · 2026-09-05T05:44:12Z

I tried creating a LAG via the web-interface, on a 4+2 device. I want to put both STP-ports into LAG 1 (I have one of those dirt-cheap Intel 82599ES dual SFP+ PCIe-cards on the Linux-PC side.). However, this does not work, there is no reaction on the web-page and on the console I see:

> Error: lag <1-4> [port]...

It looks like the web-page sends an incorrectly configured lag command. Indeed, Wireshark says, the command is lag 0 5 6, and 0 is not between 1 and 4.

logicog

Not yet through, as I was testing in parallel and got stuck, see above.

logicog · 2026-09-05T05:14:13Z


+## Link aggregation
+
+A LAG is a port to the protocol, the way the vendor firmware presents it as


Suggestion, just write: A LAG (Link Aggregation Group) is handled like an additional port by the STP protocol with its own timers and states. The switch-devices support up to 4 LAGs which are shown alongside physical ports in the STP status.

I.e.: No reference to OEM firmware, which may change the name or have different names on different versions. Also, mixing trunk and LAG is kind of confusing.

logicog · 2026-09-05T05:22:15Z

+stp lag 1 edge off
+```
+
+Membership comes from the aggregation registers the `lag` command writes, re-read


Suggestion: LAGs are configure via the lag command. Once a port is configured as a member of a LAG, it can no longer be configured individually for STP, e.g. via the stp port <n> command. The switch hardware itself does not handle STP state for a LAG as a whole, instead STP-status changes for the members of a LAG must be handled by the firmware by updating all member ports, which is possible using a single register write.

logicog · 2026-09-05T05:23:12Z


+function members(mask) {
+  const a = [];
+  for (let i = 0; i < 10; i++)


Hard-coded port 10?

logicog · 2026-09-05T07:46:00Z

Created LAG 1 (2 in the web-interface, but later is correctly shown as 1), and then went to STP. This looks like this, now:

I then logged out and back in and got:

which looks OK. (except for Trk).

logicog · 2026-09-05T07:47:55Z

In the above image, Trk(4, 9) is confusing. These are the logical ports. Needs to be translated to LAG1(5, 6).

logicog · 2026-09-05T07:55:04Z

Dissolving the lag works nicely, although the lag command documentation is incorrect, show is missing. And the result from lag 1 is confusing, we should have lag 1 delete/d:

> stp status
STP on, RSTP
bridge  8000/061e04b94f1a
root    8000/061e04b94f1a (this switch)
changes 0x0000
port state role edge tx bpdu
 01  fwd   desg yes  36 255
 02  fwd   desg yes  36 255
 03  fwd   desg yes  36 255
 04  fwd   desg yes  36 255
 L1  fwd   desg yes  36 255 <<<<<<<<<<< LAG from Web.

> lag
Error: lag <1-4> [port]...

> lag 1
port_lag_members_set, lag: 00, members: 0x0000

> STP: link down, port blocking L1
STP: edge port forwarding 05
STP: edge port forwarding 06


> 
tcp: found no unused connections.

> lag 1 5 6
port_lag_members_set, lag: 00, members: 0x0108

> STP: edge port forwarding L1

> stp status
STP on, RSTP
bridge  8000/061e04b94f1a
root    8000/061e04b94f1a (this switch)
changes 0x0001
port state role edge tx bpdu
 01  fwd   desg yes  d5 255
 02  fwd   desg yes  d5 255
 03  block desg yes  d5 255
 04  block desg yes  d5 255
 L1  fwd   desg yes  5e 36

> lag show
LAG status:
 1 member ports: 5 6  (hash: 0x7e)
 2 disabled
 3 disabled
 4 disabled

>

logicog

Just details.

logicog · 2026-09-05T07:37:05Z

+    p.n = p.lag ? "lag " + p.lag : "port " + p.p;
+    p.k = p.lag ? "L" + p.lag : p.p;
+    tr.insertCell().textContent = p.lag
+      ? "Trk" + p.lag + " (" + members(p.mbr) + ")"


LAG or Trunk? Usually a trunk is the central backbone of a network carrying multiple VLANs, and LAG is very specific about combining multiple Links/Ports. I would prefer to stay with LAG, which is also used by Linux and Windows. Also, the menu entry is called LAG, and not "Trunking".

logicog · 2026-09-05T07:42:57Z

    const sr = stat.insertRow();
-    sr.insertCell().textContent = p.p;
+    sr.insertCell().textContent = p.lag
+      ? "Trk" + p.lag + " (" + members(p.mbr) + ")"


logicog · 2026-09-05T07:56:50Z

+function members(mask) {
+  const a = [];
+  for (let i = 0; i < 10; i++)
+    if (mask & (1 << i)) a.push(i + 1);


Here, you need to push machine.log_to_phys_port[i]);

logicog · 2026-09-05T07:58:18Z

+		if (stp_lag_mask[stp_scratch] != stp_scratch16)
+			stp_map_dirty = 1;
+		stp_lag_mask[stp_scratch] = stp_scratch16;
+		for (stp_ss_i = 0; stp_ss_i < 10; stp_ss_i++)


10 hard-coded.

logicog · 2026-09-05T07:58:27Z

+			stp_st_of = 0;
+			while (stp_st_of < 10 && !((stp_lag_mask[stp_i - STP_LAG_BASE] >> stp_st_of) & 1))
+				stp_st_of++;
+			if (stp_st_of >= 10)


10 hardcoded.

logicog · 2026-09-05T08:00:04Z

-	stp_scratch = 3 - (port >> 2);
-	sfr_data[stp_scratch] &= ~(uint8_t)(0b11 << ((port << 1) & 0x7));
-	sfr_data[stp_scratch] |= (uint8_t)(state << ((port << 1) & 0x7));
+	for (stp_ss_i = 0; stp_ss_i < 10; stp_ss_i++) {


logicog · 2026-09-05T08:00:26Z

-	STP_O->rtl_tag.pmask = HTONS(((uint16_t)1) << port);
+	if (port >= STP_LAG_BASE) {
+		stp_scratch = 0;
+		while (stp_scratch < 10 && !stp_ent_has(port, stp_scratch))


logicog · 2026-09-05T08:00:35Z

+		stp_scratch = 0;
+		while (stp_scratch < 10 && !stp_ent_has(port, stp_scratch))
+			stp_scratch++;
+		if (stp_scratch >= 10) {


logicog · 2026-09-05T08:01:03Z

-		if (!(stp_pflags[stp_i] & STP_PF_ENABLED) || (stp_pflags[stp_i] & STP_PF_ADMEDGE)) {
+		port_timers[stp_i] = 0;
+		port_hello[stp_i] = (uint16_t)stp_hello_s * STP_HZ;
+		if (stp_i < 10 && (stp_i < machine.min_port || stp_i > machine.max_port))


logicog · 2026-09-05T08:01:16Z

 	stp_rstp = 1;
 	stp_txhold = 6;
-	for (stp_i = 0; stp_i < 10; stp_i++) {
+	for (stp_i = 0; stp_i < 10; stp_i++)


logicog · 2026-09-05T08:08:32Z

Then read the MSTP state register, two bits per port from the LSB, 3 forwarding and 1 blocking:

I want to test this through with practical examples. It is often surprising what kind of errors and "undocumented hardware features" one finds when actually doing real-life tests. I also do lots of other driver development and skipping this step can become very embarrassing when users start using your code. Just reading the register, we could overlook that other registers are also necessary to be correctly set. Or one would need to first disable a port before taking it out of a LAG or other funny things one would not expect. BTW, the most thorough tester I have ever met is @vDorst ;-)

The members list on the STP page was built straight from the bit index, which happens to be right on a board where logical and physical numbering differ by one and wrong everywhere else. On a 4+2 device a group of ports 5 and 6 read as "Trk (4, 9)", which is the mask spelled out in logical ports. logToPhysPort is already there and already used by the LAG page, the mirror page and the L2 table, so the fix is to use it here too. The label says LAG rather than Trk while we are in there, since that is what everything else calls it now. Reported by logicog, who found it in about a minute on hardware that does not have the mapping I had been testing against.

DrDoof · 2026-09-05T11:28:47Z

You are right about the testing, and this is a good demonstration of why. My register read was true and it was true about one board. regget 5310 tells you what the silicon decided, which is the part I most wanted evidence for, but it says nothing about the layer above it, and that is exactly where you found the bug. On the switch I have, logical and physical numbering differ by one, so the members list came out right by accident and would have kept coming out right forever.

Fixed in b802f6a: the list goes through logToPhysPort now, the same map the LAG page, the mirror page and the L2 table already use. Your Trk (4, 9) is mask 0x0108, so with the translation it reads LAG1 (5, 6). The label says LAG rather than Trk while I was in there, since that is what the rest of the tree calls it.

The other three you hit are not from this branch, so I put them in #399 rather than widen this one:

the web page cannot create the first group at all, because the buttons pass a 0-based index and the parser wants 1 to 4 — that is the lag 0 5 6 Wireshark showed you, and why the page did nothing
show missing from the usage line
a bare lag 1 wiping the group, now lag 1 delete or lag 1 d

I have not clicked through the GUI half of #399 myself for the same reason as above: on my board the numbering bug is invisible. It builds and machine_check is clean, but it wants your hardware.

DrDoof mentioned this pull request Sep 4, 2026

stp: treat a link aggregation group as one entity #325

Closed

logicog reviewed Sep 5, 2026

View reviewed changes

DrDoof mentioned this pull request Sep 5, 2026

lag: fix the web page's group numbering, and the command's syntax line #399

Open


		## Link aggregation

		A LAG is a port to the protocol, the way the vendor firmware presents it as

Conversation

DrDoof commented Sep 4, 2026

Uh oh!

logicog commented Sep 4, 2026

Uh oh!

DrDoof commented Sep 4, 2026

Uh oh!

logicog commented Sep 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

logicog left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

logicog commented Sep 5, 2026

Uh oh!

logicog commented Sep 5, 2026

Uh oh!

logicog commented Sep 5, 2026

Uh oh!

logicog left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

logicog commented Sep 5, 2026

Uh oh!

DrDoof commented Sep 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

logicog commented Sep 5, 2026 •

edited

Loading

logicog left a comment •

edited

Loading