From eab927f52a8dcd7aa240708bd7f948c6fb533dec Mon Sep 17 00:00:00 2001 From: d00f <8052722+DrDoof@users.noreply.github.com> Date: Sun, 6 Sep 2026 15:41:59 +0200 Subject: [PATCH] stp: name the port count instead of spelling out ten Review of #397 pointed at the bare 10, which shows up in fifteen places. It is the width of the port field in the MSTP register, so it is the CPU port index plus one and it gets a name. The web page counted to ten as well when listing a group's members. It uses numPorts now, like the rest of that page does. The aggregation section of the doc no longer names the OEM firmware or calls a group a trunk, and it says plainly that the hardware keeps no per-group state, so the firmware writes the members itself. Generated code is unchanged: the only difference in the assembly is the version string. --- doc/stp.md | 26 +++++++++++++++----------- rtl837x_stp.c | 30 +++++++++++++++--------------- rtl837x_stp.h | 4 +++- 3 files changed, 33 insertions(+), 27 deletions(-) diff --git a/doc/stp.md b/doc/stp.md index c6ff6598..8b6780c5 100644 --- a/doc/stp.md +++ b/doc/stp.md @@ -60,23 +60,27 @@ state. ## Link aggregation -A LAG is a port to the protocol, the way the vendor firmware presents it as -Trk1 to Trk4. The four groups are entities alongside the nine physical ports: -they carry their own path cost, priority, edge and guard settings, they hold -their own timers, and they appear as their own rows on the Spanning Tree page. +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 the physical ports in the STP status. A group +carries its own path cost, priority, edge and guard settings, and gets its own +row on the Spanning Tree page. ``` stp lag 1 cost 10000 # the group decides, not its members stp lag 1 edge off ``` -Membership comes from the aggregation registers the `lag` command writes, re-read -once a second, so a group changing under LACP is picked up without any -coordination between the two. A member port is not an STP port of its own: -`stp port ` on a member says which group to configure instead. State -changes are written to every member in one register write, since the hardware -has no per-group state, and BPDUs are transmitted through the lowest -member while carrying the group's own port id. Losing one member of a live +LAGs are configured via the `lag` command. Once a port is a member of a LAG it +can no longer be configured individually for STP, so `stp port ` on a member +tells you which group to configure instead. Membership is re-read from the +aggregation registers once a second, so a group changing under LACP is picked +up without any coordination between the two. + +The switch hardware does not handle STP state for a LAG as a whole. State +changes for the members have to be done by the firmware, updating every member +port, which is possible using a single register write. BPDUs go out through the +lowest member and carry the group's own port id. Losing one member of a live LAG is not a topology change; the logical port only goes down with its last link. diff --git a/rtl837x_stp.c b/rtl837x_stp.c index 20f530d8..78a4cfd3 100644 --- a/rtl837x_stp.c +++ b/rtl837x_stp.c @@ -47,7 +47,7 @@ __xdata uint8_t stp_rstp; /* 1 = RST BPDUs, 0 = legacy Config BPDUs */ __xdata uint8_t stp_txhold; /* BPDUs per port per second */ -__xdata uint8_t stp_ent_of[10]; +__xdata uint8_t stp_ent_of[STP_PORTS]; __xdata uint16_t stp_lag_mask[STP_LAG_COUNT]; __xdata uint8_t stp_ss_i; __xdata uint8_t stp_map_dirty; @@ -248,9 +248,9 @@ static void stp_status(void) stp_st_of = stp_i; if (stp_i >= STP_LAG_BASE) { stp_st_of = 0; - while (stp_st_of < 10 && !((stp_lag_mask[stp_i - STP_LAG_BASE] >> stp_st_of) & 1)) + while (stp_st_of < STP_PORTS && !((stp_lag_mask[stp_i - STP_LAG_BASE] >> stp_st_of) & 1)) stp_st_of++; - if (stp_st_of >= 10) + if (stp_st_of >= STP_PORTS) stp_st_of = 0; } print_field(stp_state_txt, (sfr_data[3 - (stp_st_of >> 2)] >> ((stp_st_of << 1) & 0x7)) & 0x3, 5); @@ -300,14 +300,14 @@ int8_t cmpBytes(__xdata uint8_t *m1, __xdata uint8_t *m2, uint8_t n) __reentrant static void stp_lag_map(void) { - for (stp_ss_i = 0; stp_ss_i < 10; stp_ss_i++) + for (stp_ss_i = 0; stp_ss_i < STP_PORTS; stp_ss_i++) stp_ent_of[stp_ss_i] = stp_ss_i; for (stp_scratch = 0; stp_scratch < STP_LAG_COUNT; stp_scratch++) { stp_scratch16 = port_lag_members_get(stp_scratch); 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++) + for (stp_ss_i = 0; stp_ss_i < STP_PORTS; stp_ss_i++) if ((stp_lag_mask[stp_scratch] >> stp_ss_i) & 1) stp_ent_of[stp_ss_i] = STP_LAG_BASE + stp_scratch; } @@ -336,7 +336,7 @@ static uint8_t stp_state_port(uint8_t e) __reentrant { if (e < STP_LAG_BASE) return e; - for (stp_ss_i = 0; stp_ss_i < 10; stp_ss_i++) + for (stp_ss_i = 0; stp_ss_i < STP_PORTS; stp_ss_i++) if ((stp_lag_mask[e - STP_LAG_BASE] >> stp_ss_i) & 1) return stp_ss_i; return 0; @@ -345,7 +345,7 @@ static uint8_t stp_state_port(uint8_t e) __reentrant static void stp_state_bits(uint8_t port, uint8_t state) __reentrant { - for (stp_ss_i = 0; stp_ss_i < 10; stp_ss_i++) + for (stp_ss_i = 0; stp_ss_i < STP_PORTS; stp_ss_i++) if (stp_ent_has(port, stp_ss_i)) sfr_data[3 - (stp_ss_i >> 2)] |= (uint8_t)(state << ((stp_ss_i << 1) & 0x7)); } @@ -355,7 +355,7 @@ static void stp_state_bits(uint8_t port, uint8_t state) __reentrant static void stp_state_set(uint8_t port, uint8_t state) __reentrant { reg_read_m(RTL837X_MSTP_STATES); - for (stp_ss_i = 0; stp_ss_i < 10; stp_ss_i++) { + for (stp_ss_i = 0; stp_ss_i < STP_PORTS; stp_ss_i++) { if (!stp_ent_has(port, stp_ss_i)) continue; stp_scratch = 3 - (stp_ss_i >> 2); @@ -466,9 +466,9 @@ void stp_cnf_send(uint8_t port) __reentrant STP_O->rtl_tag.flags = HTONS(RTL_TAG_LEARN_DIS); if (port >= STP_LAG_BASE) { stp_scratch = 0; - while (stp_scratch < 10 && !stp_ent_has(port, stp_scratch)) + while (stp_scratch < STP_PORTS && !stp_ent_has(port, stp_scratch)) stp_scratch++; - if (stp_scratch >= 10) { + if (stp_scratch >= STP_PORTS) { stp_tx_flags_extra = 0; return; } @@ -739,9 +739,9 @@ void stp_timers(void) __banked } if (stp_link_now != stp_link_prev) { for (stp_i = 0; stp_i < STP_ENTITIES; stp_i++) { - if (stp_i < 10 && (stp_i < machine.min_port || stp_i > machine.max_port)) + if (stp_i < STP_PORTS && (stp_i < machine.min_port || stp_i > machine.max_port)) continue; - if (stp_i < 10 && stp_ent_of[stp_i] != stp_i) + if (stp_i < STP_PORTS && stp_ent_of[stp_i] != stp_i) continue; if (!(stp_pflags[stp_i] & STP_PF_ENABLED)) continue; @@ -834,7 +834,7 @@ void stp_defaults(void) __banked stp_fwddelay_s = 15; stp_rstp = 1; stp_txhold = 6; - for (stp_i = 0; stp_i < 10; stp_i++) + for (stp_i = 0; stp_i < STP_PORTS; stp_i++) stp_ent_of[stp_i] = stp_i; for (stp_i = 0; stp_i < STP_LAG_COUNT; stp_i++) stp_lag_mask[stp_i] = 0; @@ -896,11 +896,11 @@ void stp_setup(void) __banked stp_tx_count[stp_i] = 0; 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)) + if (stp_i < STP_PORTS && (stp_i < machine.min_port || stp_i > machine.max_port)) continue; if (stp_i >= STP_LAG_BASE && !stp_lag_mask[stp_i - STP_LAG_BASE]) continue; - if (stp_i < 10 && stp_ent_of[stp_i] != stp_i) + if (stp_i < STP_PORTS && stp_ent_of[stp_i] != stp_i) continue; if (!(stp_pflags[stp_i] & STP_PF_ENABLED) || (stp_pflags[stp_i] & STP_PF_ADMEDGE)) { diff --git a/rtl837x_stp.h b/rtl837x_stp.h index 75289a5d..b547f5d8 100644 --- a/rtl837x_stp.h +++ b/rtl837x_stp.h @@ -2,6 +2,7 @@ #define _RTL837X_STP_H_ #include +#include "rtl837x_common.h" void stp_in(void) __banked; void stp_setup(void) __banked; void stp_timers(void) __banked; @@ -12,7 +13,8 @@ void stp_defaults(void) __banked; /* Tick rate of stp_timers(), also used by the web UI. */ #define STP_HZ 50 -#define STP_LAG_BASE 10 +#define STP_PORTS (CPU_PORT + 1) +#define STP_LAG_BASE STP_PORTS #define STP_LAG_COUNT 4 #define STP_ENTITIES (STP_LAG_BASE + STP_LAG_COUNT)