stp: treat a link aggregation group as one entity - #325
Closed
DrDoof wants to merge 61 commits into
Closed
Conversation
Contributor
Author
|
Draft on purpose, because of what it sits on. This branches off #314 rather than off main, so the 40 commits and 19 files What it does not depend on is LACP. Membership comes from the trunk membership Once #314 merges I'll rebase onto main and take it out of draft. Eight commits |
This was referenced Aug 15, 2026
DrDoof
force-pushed
the
feat/stp-trk
branch
2 times, most recently
from
August 15, 2026 20:14
05373bf to
c447921
Compare
Contributor
Author
|
Now that #347 is in, the map here calls |
added 21 commits
August 15, 2026 23:15
port_l2mc_set() writes a static L2 multicast entry for a reserved group 01:80:C2:00:00:<last> in a given VLAN with a given member portmask. Slow-protocol frames must reach the management CPU without being flooded to other ports, but the RMA "trap" action cannot deliver to the internal NIC on this hardware - its destination is an external CPU attached to a physical port. The working alternative is to keep the RMA action at "forward" and constrain the egress with a static entry: the forward lookup then hits the entry's member mask instead of the VLAN flood mask. Hardware-verified on a SWTGW218AS in both directions: a mask without the CPU bit stops delivery to the CPU, a CPU-only mask delivers with no port egress. Lookups are IVL, so callers add one entry per VID they care about; rewriting the same MAC+VID replaces the entry in place. Used by the BPDU containment in the next commit; the pending LACP branch adopts it for 01:80:C2:00:00:02 the same way.
With STP enabled the switch is a participating bridge, so BPDUs must be consumed, not relayed - yet the reserved group 01:80:C2:00:00:00 was flooded across the VLAN just like any multicast, leaking every BPDU to all ports (the same defect class as the LACPDU flood addressed in the LACP branch, PR logicog#299). On stp on, write a CPU-only static L2 multicast entry for the BPDU group per VLAN: BPDUs can arrive VLAN-tagged and classify into the tag's VID, so cover every VLAN present in the VLAN table plus every port's PVID for the untagged case. On stp off the same entries are retargeted to all ports + CPU, restoring the previous flood behaviour: an unmanaged switch is expected to be transparent to BPDUs so the surrounding spanning tree can span through it, and dropping them instead would partition that topology. Note: with STP enabled the ports start out blocking, which also stops egress of CPU-originated LACPDUs, so an active LACP aggregate drops until the ports reach forwarding - a pre-existing interaction, not changed here.
The rtl_tag `flags` word (LEARN_DIS, KEEP) and the `pmask` ALLOW-bit semantics are properties of the RTL8_4 CPU tag, not of any one protocol: STP injects BPDUs with LEARN_DIS set and LACP emits slow-protocol frames the same way. Define them once in the shared header, with the HTONS byte-order caveat documented, so every feature that hand-builds a CPU tag frame uses the same constants. (cherry picked from commit 7f905b3)
The always-mapped common area is nearly full (349 bytes free before this change), and the STP state machine that follows does not fit there. Move the module to BANK2 next to the other protocol code; its public entry points are already __banked, and cmpMAC/stp_cnf_send have no callers outside the file.
"stp on" put every port into blocking (stp_setup, port_timers = "10 s") but nothing ever counted those timers down: stp_timers() only sent hello BPDUs. On a network with no other (R)STP bridge - i.e. nobody sends us BPDUs - every port therefore stayed blocking FOREVER and enabling STP took the whole network down until "stp off". - stp_timers(): count port_timers down; when a port's listen period expires with no better root heard, promote it to forwarding in MSTP_STATES (we are the designated bridge on that port). - Calibrate the tick constants to the real stp_timers() rate (~64 Hz: main loop ~256 Hz / (STP_TICK_DIVIDER+1)): TIME_HELLO 0x200->0x80 is an actual 2 s hello, port_timers 0xa00->0x280 an actual 10 s listen period. Measured before the fix, ports converged only after ~40 s. - Move struct bridge into rtl837x_stp.h and export root_bridge/-_cost for the web UI status endpoint. Verified on hardware: "stp on" -> ports report Blocking, after the 10 s listen period all ports promote to Forwarding and LAN connectivity returns; "stp off" restores forwarding immediately. We elect ourselves root (weRoot) with no other bridge present. (cherry picked from commit 8537a15ca254b2122272b20bec7a66426e86df4b)
The "do not block the CPU port" mask 0x0f covers bits 3:0 of MSTP_STATES byte 1, which is ports 8 AND 9 - so stp_setup unconditionally forced port 8 (a real front port, the SFP uplink on SWTGW218AS) into forwarding and it could never be blocked. The CPU port alone is bits 3:2 = 0x0c.
Add a Spanning Tree page: an on/off toggle driving the existing "stp" command over /cmd, and a live status section fed by a new /stp.json endpoint - the elected root bridge (priority + MAC), our path cost, whether we are the root, and the per-port STP state read live from the ASIC's MSTP register (same 2-bit encoding stp_setup() writes). Ports are reported by their physical numbers. Recovered-from: 3132319, 9365c86
…ence
Implements the standard 802.1D-2004/802.1w configuration surface:
Bridge: priority (0-15 x4096), hello time, max age, forward delay,
force-version (RSTP v2 / STP-compatible v0 Config BPDUs), tx hold
count (per-port per-second BPDU budget).
Per port: enable, admin edge (forwarding immediately - no listen gap),
auto edge (forwarding after 3 s of BPDU silence; DEFAULT, so
host-facing ports no longer take the full forward delay),
path cost (0=auto/20000), port priority, BPDU guard (port disabled
on BPDU receipt), root guard (never accept a better root on the
port), BPDU filter (no BPDUs in or out).
Engine additions: root max-age expiry (reclaim the tree when the root goes
silent), root path cost accounting (rx cost + root-port cost, advertised in
our BPDUs), loop detection (our own BPDU coming back blocks the port for a
listen period), topology-change counter, approximated per-port roles
(Root/Designated/Alternate) for diagnostics.
CLI: "stp prio|hello|maxage|fwd|txhold|version ..." and
"stp port <n> on|off|edge|cost|prio|guard|filter ..." (stp_parse, delegated
from cmd_parser); all forms accepted by the startup-config validator so the
whole configuration persists. /stp.json now reports config + status; the
Spanning Tree page exposes everything with immediate-apply controls and live
state/role columns (edit-in-flight guard against the 2 s refresh).
8051 memory: the module moves to code BANK2; internal-RAM pressure from
cross-bank calls resolved by xdata loop iterators/scratch, __reentrant on
the small helpers, and moving httpd's header-pointer globals to xdata.
Verified on hardware (SWTGW218AS): defaults land per standard; priority and
hello change live; admin-edge ports (the LACP bond uplinks) keep the LAN at
0% loss THROUGH "stp on"; auto-edge ports forward after 3 s; a port that
heard real BPDUs (a VM bridge behind physical port 6) correctly declined
auto-edge, sat out the full listen period and became Designated; tc counts
promotions; we win the root election at priority 16384 vs 32768.
(cherry picked from commit 09a34dc6acdc81ab9cab0727d2f4a59c68131a3e)
"stp port 7 edge" (no value) passed the cmd_words_len < 4 check and then cmd_compare(4, ...) read a stale word left over from the PREVIOUS command line - cmd_words_b is not cleared between commands - so the sub-command could randomly match whatever was typed before. Require 5 words for every per-port sub-command that carries an argument (everything except on/off). (cherry picked from commit 1210f4f9257b14c31ad653fc7616ef403a494d28)
Two TX bugs meant our BPDUs NEVER left the switch as valid STP frames - on the wire they appeared as ethertype 0x8899 (the raw Realtek CPU tag) and were flooded to all ports instead of directed. Every earlier root election was a solo act: no other bridge ever saw us. Both are the same bug classes fixed for LACP earlier: - rtl_tag.flags was written raw (0x0020); like every other tag field it must go through HTONS, otherwise the bits land in the wrong byte (0x2000 = EFID), the ASIC fails to parse the tag and floods the frame with the 0x8899 header still attached. - With a management VLAN set, tcpip_output() splices an 802.1Q tag after the SA, again shifting the CPU tag out of the parsed position. BPDUs are link-local and must egress untagged: suppress the VLAN insert per frame, exactly as lacp_send() does. Hardware note discovered while fixing this: RTL_TAG_KEEP on an LLC/802.3 (length-field) frame makes the ASIC drop it entirely - the same flag works fine on ethertype frames (LACP). So BPDUs use LEARN_DIS only. Verified on the wire (tcpdump on the peer): clean "802.3 ... LLC, dsap STP 0x42 ... Rapid STP, bridge-id 8000.<our mac>" at the hello interval, sent directed (no flood), management HTTP unaffected, LAN at 0% loss throughout. (cherry picked from commit 4a41a292a9ab88d4fb05a8481ad28f8ffcfd9bc4)
Enabling STP on a bridge whose management rides an in-band VLAN can cut
off that very management - and not only by our own blocking: on this
network the upstream TP-Link Easy Smart switch's "loop prevention"
reacted to our BPDU hellos by blocking ITS port towards us while our
ASIC was all-forwarding, isolating the whole segment until a power
cycle. Recoverable only by going quiet.
Add a commit-confirm watchdog: while STP is enabled, any HTTP request
re-arms a countdown ("stp failsafe <seconds>", default 180, 0 disables);
if management stays silent for the whole window, STP disables itself,
which also stops BPDU TX so a neighbour's loop protection can release
its block. The web UI polls /stp.json every 2 s, so an open browser
naturally keeps the watchdog re-armed. The trip is reported via
/stp.json (fs, fsT) and as a warning on the Spanning Tree page.
Deliberately not conditioned on our own MSTP port states - the incident
above proves the uplink can be dead while every local port forwards.
Also bound the NIC DMA busy-waits (nic_tx_packet, nic_rx_header,
nic_rx_packet): an unbounded spin on SFR_NIC_CTRL freezes the entire
main loop (timers, HTTP, ARP) if the ASIC ever fails to consume a
transfer; give up after ~65k polls and drop the frame instead.
Hardware-verified end to end: with priority 15 against a live RSTP
bridge the uplink died 6 s after "stp on" and the network recovered BY
ITSELF 66 s later (trip at 45 s + neighbour release), fsT=1, LACP and
LAN intact. Telemetry via syslog-to-edge-port host confirmed the full
chain: countdown 44->4, trip, hello TX stopping at the trip.
(cherry picked from commit 1fa9775156fd6d7ebfdda2382f73430b86601230)
…d info) Bring the Spanning Tree page in line with a typical managed switch's per-port panel. Configuration gains the full-range path cost (raw 0..200000000, 0 = auto, replacing the old 1000x-scaled byte), a point-to-point admin control (auto/on/off), and the priority is now a 0..240 step-16 dropdown. A new status table shows, per port, the Port State, Role, Designated Bridge / Port ID / Cost (learned from received BPDUs, kept per port and aged via the BPDU age), Operational Edge and Operational Point-to-Point. The designated fields fall back to presenting this switch as the segment's designated bridge when no fresh BPDU has been heard (so a quiet port shows our own bridge-id, as the vendor UIs do). /stp.json carries the packed hex fields plus our own MAC for that fallback. Space: reclaim BANK2 for the above by moving rtl837x_pins to HOME and compacting leds_dump into a register-address table (~800B); bandwidth returns to BANK1. No BANK3 - hardware-verified that PSBANK > 2 crashes this SoC at boot (a bricked unit and an SPI-programmer recovery earlier today); a warning to that effect is now in rtl837x_lldp.c. Hardware-verified: cost 200000000 and p2p off round-trip through the CLI and JSON, the status table populates correctly with STP enabled (all ports Forwarding/Designated, oper-edge and oper-p2p True), LACP 3f/3f and the LAN unaffected. (cherry picked from commit 2ec62072f061dc9e78bc821ba1c297cb6819e206)
An RST BPDU body is 36 bytes: the Config-BPDU fields plus a trailing version-1 length octet (zero - there is no version-1 information). Ours was 35 - strict 802.1w parsers treat such a BPDU as malformed and drop it. Add the field, keep legacy Config BPDUs at 35 bytes, and set the 802.3 length accordingly (0x27 with LLC for RST, 0x26 for Config).
Accept legacy Topology Change Notification BPDUs (v0, type 0x80): reply on the ingress port with a Config BPDU carrying Topology Change Acknowledgment so the sender stops repeating, and count the change. Also stop reading fields past the end of short frames: require the header through bpdu_type (33 bytes with the CPU/VLAN prefix) before classifying, and the full 35-byte body before the election logic - truncated or fuzzed BPDUs are dropped instead of parsed as garbage.
Two protocol-correctness fixes on the information we advertise: Only designated ports announce periodically. The root port is where our root information arrives; sending it back there feeds the upstream bridge its own data and makes us look like a competing designated bridge on that segment. Relay the message age instead of always claiming zero. A bridge increments the received age by one second per hop, so downstream neighbours can age the information out; advertising 0 forever made our BPDUs look eternally fresh no matter how stale the root information was. Age stays 0 while we are the root ourselves.
port_l2_forget() flushes the whole table and polls the flush engine without a bound. A topology change only needs to age out the port that changed, and the STP tick cannot afford an unbounded poll: add port_l2_forget_port() with a single-port mask and the same bounded wait the static-entry helper uses.
A port entering forwarding, or being blocked because its own BPDU came back, changes where MAC addresses live - but the counter was bumped and nothing else happened: our forwarding table kept the stale entries and the neighbours were never told. Flush the affected port's dynamic entries (bounded single-port flush) and set the Topology Change flag in our BPDUs for max age + forward delay, so neighbours age their tables out as well. Edge ports are exempt: a host coming or going is not a topology change.
The Topology Change Acknowledgment is staged in a one-shot variable and consumed when the BPDU is built - but stp_cnf_send() can return before that, when the port is filtered/tripped or its tx-hold budget for this second is spent. The flag then survived and was OR-ed into the next BPDU this switch sent, on whatever port that happened to be. Clear it with the frame it belonged to.
The timers assumed stp_timers() runs at 64 Hz. It does not: the main loop idles on the 200 Hz system tick and calls us every fourth pass, i.e. 50 Hz. Measured on hardware - with hello configured to 2 s the BPDUs left the port 2.560 s apart, exactly the 28 % overshoot the wrong constant implies, and every other timer (forward delay, max age, tx-hold refill) was stretched the same way. Move the constant to the header with the arithmetic spelled out, and use it in the status page too, which had the 64 hardcoded and therefore aged the same counters differently than the engine.
"stp port N edge off" cleared only the admin and auto flags, not the operational one - and that is the flag the engine actually consults: it exempts the port from topology changes and lets it skip the listen period. A port therefore stayed an edge port until the next "stp off" / "stp on", silently ignoring the new setting. Clear it with the others, and mark an admin edge operational right away, as stp_setup() does.
Covers enabling it, the bridge and per-port settings, how BPDUs are delivered on this hardware (RMA forward constrained by a CPU-only static L2 entry, since trap-to-CPU targets an external CPU these boards do not have), the timer base, and the management failsafe - enabling STP over the network can block the very port the management VLAN rides on, so that part is spelled out rather than left to be discovered.
The report on the PR is that it will not link for KP_9000_6XHML_X2, with "?ASlink-Error-Could not get N consecutive bytes in internal RAM for area OSEG" five times over. It builds here on sdcc 4.2.0 and 4.5.0 for that same machine and the same commit, so something in the toolchain differs, but the pressure it is complaining about is mine and it costs little to give back. stp_in() is __banked, so its temporaries get exclusive DSEG instead of overlaying with anything else. Recording the designated bridge put four more live values across a memcpy in the middle of it and the register allocator answered with five spill locations. The module went from 5 bytes of DSEG to 12, and from 17 sloc references to 49. Moving that block into a __reentrant helper puts its temporaries on the stack instead. The module now claims no DSEG at all, 5 bytes better than before the recording was added, and the image sits at 95 bytes of DSEG against 101 on main. It costs 170 bytes of BANK2, where there is room.
Two things from the review, both about the console being where you end up when the tree is not what you expected. The six messages that name a port were printing the internal index. On a board whose map is not the identity that is a different number from the one written next to the socket, which is worse than no number at all. They go through machine.log_to_phys_port now, in a small helper that also swallows the newline each of them repeated. "stp status" prints the bridge and root IDs, the root port and path cost, the topology change count, the failsafe setting, and a line per port with state, role and operational edge. Everything it shows is state the module already keeps, apart from the port states, which come from one read of MSTP_STATES. 660 bytes of BANK2, which leaves 3366 free. No internal RAM, no xdata.
Your console session shows the shape of this better than I could have. STP found a loop, blocked the port, unblocked the other side of the pair, and then the failsafe turned STP off. It had been disarmed by your first console command and re-armed by the loop detection itself, so a mechanism that exists to protect against a lockout ended up removing loop protection while a loop was physically present. That is the part that did not make sense, and it wasn't the console. Two changes, both narrowing. Loop detection and root guard no longer arm the window. Those are the protocol doing its job on evidence off the wire. Nothing an operator did needs undoing there, and nobody is waiting to confirm anything. Typing on the serial console no longer disarms it. The failsafe asks one question, whether the operator can still reach management over the network, and serial activity doesn't answer it. It proves somebody is standing at the box, which is the one case where a lockout doesn't matter, and it took the safety net away from a remote operator on behalf of someone not using it. HTTP activity still confirms, because that is the path being measured, and the console in the web interface counts for the same reason. What is left arms on stp on, stp port N on and stp failsafe, each of them an operator choosing something whose outcome the protocol then decides. Gives back 61 bytes of BANK2, 31 of the common area and a byte of xdata.
The window could be armed from the serial console but only ever disarmed by an HTTP request. save_cmd, which gates arming, is cleared only while execute_config() replays the startup config, so every interactive command armed it wherever it was typed, while mgmt_alive, which disarms it, was written in exactly one place, on HTTP traffic. An operator working entirely on the serial console therefore lost STP 180 seconds after enabling it however much they typed, which is what makes the mechanism impossible to test from a console. The documentation described the behaviour that was intended rather than the one that was built, and in both directions: it said a command on the serial console also confirms, and it said a reboot with STP in the startup config disables it again three minutes later. Neither held. The replay path never armed the window at all. Repairing the asymmetry would have kept a mechanism whose premise is contested anyway. A watchdog that switches the protection off in response to silence adds a second failure mode on top of the first: where the network is misconfigured and STP is the thing holding a storm back, restoring forwarding removes the last reason management still answers. Gone with it: the stp failsafe command, the fs and fsT fields of /stp.json, the input and the tripped banner on the Spanning Tree page, the two persistence patterns in config.js, the documentation section, and mgmt_alive itself, which had no other reader. 550 bytes back, 145 of BANK1 and 405 of BANK2, and five of xdata, which is the four counters and mgmt_alive and nothing else. Built for SWTGW218AS and KP_9000_6XHML_X2 on sdcc 4.5.0.
"stp status" has always worked, but the line printed on a bad command listed only on and off, so the one subcommand that shows what the bridge thinks was the one you had to already know about.
802.1D has a port leave the edge state when a BPDU arrives on it. Here the flag was only ever cleared by the loop latch, root guard, a link coming back, "stp on", "stp off" and the edge command itself, so a port that auto-edged during the three seconds of silence after link-up kept the flag for as long as it stayed up, whatever the neighbour sent. Two things read that flag. The status page prints it, so a port talking to a bridge reported edge 1 and there was no way to tell from the output whether a BPDU had ever arrived. More quietly, stp_topology_change() returns early for an edge port, which is right for a real one and wrong for this: a topology change on such a port was neither counted nor propagated, and port_l2_forget_port() never ran, so what was learned behind it stayed in the table. Only the flag is cleared. The port is not pushed back through the listen period, which would take a working link out of forwarding for a forward delay the first time a neighbour speaks.
…he MAC A Bridge Identifier is two priority octets followed by the MAC, compared as one unsigned number. The test here read the first priority octet and then went straight to the MAC, so the system ID extension in between was never looked at and two bridges differing only in it were ranked by MAC instead. The field is stored, sent and printed, just not compared. Ordinary single instance RSTP leaves the extension zero on both sides, which is why this has not shown up. Where it is not zero the ranking is simply wrong: same priority octet, extension 0x0a against 0x00, and the worse bridge wins if its MAC happens to be lower. cmpMAC becomes cmpBytes with a length, since the identifier is eight contiguous bytes in both the packet overlay and root_bridge, and the loop was already doing the right thing for six of them. sdcc lays the struct out with no padding, checked, so the eight byte compare is the standard's rule written directly.
The status output named state, role and edge, none of which separates a port nobody is speaking (R)STP to from a port whose BPDUs we are dropping. Both look the same: forwarding, designated, edge, and the tree rooted at ourselves. stp_in() leaves on eight different conditions, from a short frame through an unexpected LLC header to a disabled port, and none of them says anything. stp_bpdu_age was already maintained for the ageing rules, so this only prints it, in seconds and capped at 255. A column that counts up means nothing is arriving; a column that stays near zero means frames are arriving and any disagreement about the tree is ours. Eighty two bytes of BANK2 and two of xdata, most of it the sixteen bit divide by the tick rate. It comes out of a branch that gives back three hundred and twenty eight, so it is affordable, and printing raw ticks to save it would put the reader back to converting in their head.
The table printed the ASIC's raw two bit state, a 1 or a 2 for the role and
a 1 or a 0 for the edge flag, so reading it meant having the source open
next to the console. The columns carry the words now:
port state role edge bpdu
05 fwd desg yes 255
01 block desg no 21
02 learn desg no 5
03 fwd root no 0
They come from fixed width tables indexed by the same values as before, so
nothing about how any of the three is derived changes, and the columns line
up under the header without a formatter.
The role column still only tells the root port from everything else,
because that is all the state machine tracks. A port sitting in blocking
because a better BPDU arrived on it reads as designated here. Naming the
column makes that visible rather than introducing it.
154 bytes of BANK2, nothing in BANK1, xdata or internal RAM. Built for
SWTGW218AS and KP_9000_6XHML_X2 on sdcc 4.5.0.
The table could say a port was designated and had heard nothing, which is
two different situations wearing the same face: either we are not
announcing on that segment, or we are and nobody is answering. Telling them
apart needed a capture on the far side.
port state role edge tx bpdu
05 fwd desg yes 2a 255
01 block desg no 2a 21
03 fwd root no 00 0
The tx column counts BPDUs actually handed to the hardware, so it moves
only past the enable, filter and tx hold checks in stp_cnf_send(). A
designated port has to show it climbing once per hello time. The root port
never does, because we do not announce back towards the root, so a
neighbour that has taken us as root falls silent in both directions on that
link and the two columns together say exactly that rather than looking like
a fault.
The counter is a byte and wraps at 256. It is meant to be watched moving,
not summed, and it starts again when STP is enabled.
67 bytes of BANK2 and 10 of xdata, nothing in BANK1 or internal RAM. Built
for SWTGW218AS and KP_9000_6XHML_X2 on sdcc 4.5.0.
Byte access instead of 32-bit shifts is how this codebase works everywhere, so the comment explained a house rule at one call site.
The tree structure already crossed the switch by regeneration, but the topology-change information did not: a received TC flag was ignored and a TCN only acknowledged, so bridges behind this one kept stale entries until normal aging. A TC flag in a received BPDU now flushes the other non-edge ports once and arms the transmit window our BPDUs already copy the flag from, refreshed to hello+1 seconds by every further flagged frame so it ends one hello after the neighbour stops, without shortening the longer window a local change arms. A TCN is acknowledged as before and then treated like a local change on that port.
Each member of a trunk ran its own copy of the state machine: both sent hellos with their own port id, held their own timers and reported their own role. On the wire that is two designated ports on one logical link, and a peer is entitled to read it as a loop. The protocol now works on entities rather than ports, the way the vendor firmware presents them: 0 to 8 are the physical ports, 9 is the CPU port, and 10 to 13 are Trk1 to Trk4. A trunk carries its own path cost, priority, edge and guard settings, holds its own timers, transmits one BPDU with its own port id, and shows up as its own row on the Spanning Tree page next to the physical ports. A member port stops being an STP port of its own, and "stp port <n>" on a member answers with the trunk to configure instead. Membership is read from the trunk registers the lag command writes, once at "stp on" and once a second after that, so a group changing under LACP is picked up without any coordination between the two modules. The hardware has no per-trunk state, so a state change is expanded to every member inside one register write, and BPDUs go out through the lowest live member. Link supervision compares masks folded onto entities, so losing one member of a live trunk is not a topology change and the logical port only goes down with its last link. Costs 133 bytes of XDATA: nine per-entity arrays grow by four entries each, plus the port-to-entity map and the membership masks. No internal RAM, the segment sizes of both modules are unchanged. Not yet exercised on hardware. The entity mapping, the state expansion, the transmit member choice and the folded link mask are checked against the masks the lag command produces.
The membership and activity tests were written out at every call site, which made the trunk-aware paths harder to read than the port-only ones they replaced. They become two small helpers instead. No behaviour changes: the helpers hold exactly the expressions the call sites had.
The status page read the port state straight out of MSTP_STATES at the entity index. That register only covers the ten hardware ports, so a trunk entity read past the end of it and always came back disabled, with the role derived from it landing on alternate. The trunk now reports the state of its lowest member, which is the state all of its members share. (cherry picked from commit 4fcd246f0978a1836ab3ed339421980c8edaea37)
The tx hold budget was refilled once a second over the physical port range only. A trunk entity therefore got its six BPDUs from stp_setup and then went quiet for good, which a capture on the peer showed as nothing at all arriving from us after the first second. The refill now covers the whole entity space. (cherry picked from commit 3104f93adac98af6768bba271c05864ee358da95)
The flag byte read MSTP_STATES at the entity index, which for a trunk points past the ten hardware ports and always read as not forwarding. A capture on the peer showed the trunk announcing itself as designated with learning and forwarding clear while it was in fact forwarding, so the neighbour was told the link was unusable. The state now comes from the member whose bits represent the entity, the same way the status page does it. (cherry picked from commit b9d4126642ed9c638380286d451a92b70f4e2148)
A port whose link returns rejoins its trunk through the aggregation code, but the STP entity map is only rebuilt once per second. In that window the returning port is still its own entity, so link supervision gives it a state of its own and a stale BPDU age can send it straight to edge forwarding. When the trunk is forwarding this is invisible. When the trunk is blocked the member opens a path STP meant to keep closed. stp_lag_map() now notices when a trunk membership register differs from the value it read last time, and the one Hz tick reapplies the trunk state to every member. Detection compares the membership masks rather than the map derived from them, so no copy of the previous map is needed. Seen on hardware by pulling and reinserting one cable of a two port group: the returning member logged an edge-port transition of its own before the aggregation code put it back in the trunk.
The JSON the STP page reads listed every physical port in range and then appended a row per non-empty trunk, so a port that belongs to a trunk appeared twice: once on its own and once inside the group. Configuring the standalone row got the command rejected, since the parser answers a member port by naming the trunk to use instead. Seen on hardware with a two port group: ports 7 and 8 kept their own rows next to a Trk2 row carrying the same two ports in its member mask. The entity map already records which entity owns a port, so the loop now skips a physical port that is not its own entity, the same test the console paths use.
The startup config is rebuilt in the browser by matching the command log against a whitelist, and a line matching nothing is dropped without a word. The whitelist knew only "stp port N ...", so every STP setting made on a trunk was thrown away at save time and the switch came back from a restart without it. Seven patterns cover the same subcommands the parser accepts for a trunk, and one more joins the dedup list so a repeated setting overwrites the earlier one rather than piling up. The trunk number is bounded to 1 to 4, the range the parser takes, so a line the switch would refuse never reaches the stored config in the first place. Found by reading the stored config off a switch whose two aggregated ports were still listed there by their own numbers. (cherry picked from commit 303a6c2a8c2e331c84a78862923207be6f0b93b6)
The table walked physical ports, so it listed the members of a trunk with their own role and counters and had no row for the entity at all. It walks entities now, hides members the way the port list already does, and labels a trunk t1 to t4. The ASIC holds no state for a trunk, so the row shows a member's, picked the same way the JSON picks it. stp_state_set() writes every member together, so which one it is does not matter.
The rest of the firmware already calls it a lag: the command, port_lag_members_set(), the LAG_HASH constants, lag.html and doc/link_aggregation.md. This branch was the only place saying trk, which also reads like a VLAN trunk port and means something else there. STP_LAG_BASE, STP_LAG_COUNT and stp_lag_mask follow, the command becomes stp lag <1-4>, the status row is labelled L1 to L4, and the JSON field is lag. The prose follows too. The vendor register names keep their TRK spelling, since that is the datasheet's word and not ours. Kept as its own commit rather than folded into the nine before it, because a rename is easier to check when it stands alone.
port_lag_members_get() is in main now, so the map drops its own copy of the decode. Nothing about the entity mapping changes, only where the two bytes come from.
The configuration table already labels the aggregate Trk1 with its members, but the status table below printed the raw entity number, which for a trunk is a zero. Worse, its cells were created under the trunk key while the refresh looked them up under that zero, so the row never updated at all. Both tables now share the naming and the refresh uses the key the cells were created with.
Same defect as the status table one commit earlier, with a wider blast radius: the controls are created under the trunk key but filled under the raw entity number, and since the trunk sorts first its missing element threw before any row was filled. Every port then showed an empty path cost and a zero priority regardless of what the switch reported.
Contributor
Author
|
Superseded by #397, which is the same idea rebuilt as one commit on current main. This branch predates the trunk/lag rename and 45 of its 61 commits have landed here by other routes since, so rebasing it would have meant re-doing work that is already in. Closing rather than leaving it to rot, but happy to reopen if you would rather review it in this shape. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rebased onto #314, which is where it should have sat from the start. It was carrying its own copy of that branch's history: 32 of its 40 commits were byte identical patches to commits there, which is the real reason the two conflicted in nine files. It is now eight commits on top of #314 and nothing else, so those eight are the whole of what this PR asks you to read.
In order: a group becomes an entity standing beside the physical ports, the entity tests move out of the places that were spelling them out, the state shown for a trunk is taken from one of its members because the hardware holds none for the group itself, the transmit budget is refilled per entity rather than per port, the BPDU flags for a trunk come from a member's hardware state, a change of membership re-applies the entity's state to whatever joined or left, the port list leaves members out, and the saved configuration keeps the trunk form of the commands instead of expanding them to members.
Dropping the duplicated history also dropped what those commits had inherited from before the management failsafe went away. Every single conflict in the rebase was that failsafe trying to come back, and I took the current side each time, so none of it survives here.
One thing this still does not do, and you will see it the moment you run it: the status table added in #314 walks physical ports, so it lists the members of a trunk with their own role and counters and has no row for the entity at all. What that row should say is a design question rather than a mechanical one, so I would rather ask than guess. My inclination is a row per entity with the members hidden, the way the port list already handles them, but say if you would rather see both.
Costs on top of #314: 451 bytes of BANK1, 1472 of BANK2 and 139 of xdata. Built for SWTGW218AS and KP_9000_6XHML_X2 on sdcc 4.5.0. Not run on hardware in this form yet, and the earlier hardware results on the old lineage do not carry over, since the base underneath has moved a long way.