Removing the CPU-port support, As stated in #334, it is not needed to
manual add the CPU-port to any command. Because the CPU-port should be
added automatilly when CPU-port is needed.
Refactor cmd_parse_port() because port number is only 1 byte.
This simplifies the parsing code.
Refactor cmd_parse_port_separator() to ensure the return size is correct.
cmd_parse_port_separator() parse the full port number and checks that the
number end with a NUL of space.
Now this function can also be used to parse the last argument number
because this ends with a NUL.
Also refactor code.
Measured against the generic leds_setup() output on the board: eight of
the 21 stock values are identical to what leds_setup() computes from
the machine's led_mux table, and eight more revert to the generic
values with no change in LED behaviour in any tested link state (copper
2.5G, SFP 2.5G, SFP 10G). Keep the five with a measurable effect: 6528
selects blue over green at 10G, 6540/6548 carry the SFP and copper LED
set behaviour, 65dc enables the LED outputs, and PIN_MUX_0 routes the
blue pin.
Verified from a clean boot on a SWTG018AS-V2.1.0 board: copper solid
green with activity blink at 2.5G on two ports, SFP green at 2.5G,
SFP blue at 10G, LEDs off on link down.
machine_custom_init() runs once at boot, but its code and any tables it
uses were compiled into machine.c and so into the common bank. On the
SWTG018AS-V2.1.0 variant, whose init carries a 21-entry LED register
table, that overflows bank 0 by 0x66 bytes and main no longer links for
MACHINE_PCB_SWTG018AS_V2_1_0; any machine whose init grows can hit the
same wall. SDCC segment pragmas apply file-wide, so the hooks move to a
new machine_init.c compiled into BANK2, and the prototype becomes
banked. machine_check also compiles the new file per machine so the
hooks keep CI syntax coverage.
The I2C controller transfers up to sixteen bytes per transaction and
page_impl.c already used that for sfp_send_data(), while sfp_read_reg()
asked for one byte and every caller looped. Reading a module therefore
cost one address phase per byte: 87 transactions when a module is
inserted, 52 for the sfp command, 36 for the vendor block in status.json.
sfp_read_block() replaces sfp_read_reg() and the callers that already
wanted a run of registers ask for it once: the vendor fields as three
16 byte pages, the diagnostics as one transfer, rate and encoding
together. That drops the three paths above to 8, 6 and 3 transactions,
and sfp_send_data() loses its copy of the transfer.
The vendor loops now run over 16..63 rather than 20..59 so the page base
is a multiple of sixteen and the index into the buffer is a single AND.
The four extra bytes at each end are read and discarded. The diagnostics
read asks for 16 bytes rather than the 15 it uses, because 16 is a width
the shipped firmware already exercises and 15 is not.
The device address, the bus selection and the start bit go into the
control register in one write now that the memory address is written
first, so a transfer touches that register once instead of three times.
The register reads take their result from the SFRs directly rather than
through the sfr_data mirror. The result is a bool and the destination is
sfp_buf, so a caller that cares about a failed transfer looks at the
return value instead of a flag.
Every caller gives up on the first failed read rather than carrying a
flag to the end, which is why the module read moved out of handle_sfp
into a function of its own. A module whose read fails is left marked as
absent, so the next poll retries it instead of configuring the SerDes
from bytes that never arrived.
BANK1 -194 bytes, BANK2 +382, common segment +44, xdata +15 for the
buffer, and one byte more of internal RAM free than before the series.
Built for all 25 machine definitions on sdcc 4.5.0; the tightest common
segment is 98 bytes free on SWTG024AS_V2_0, against 54 before this
series.
CONFIG_UPLOAD_BUF at 2560 capped a configuration upload at about 2.2K
while the config sector holds 4K. Size the buffer as CONFIG_LEN plus
room for the multipart framing so the whole sector is usable.
config_take() wrote the payload without checking it against the sector:
safe before only because the buffer could not hold an oversized one.
Reject a payload that does not fit CONFIG_LEN, terminator included,
instead of writing past the erased sector.
send_config() streams a configuration larger than the TCP output buffer
through the cont_addr/cont_len continuation, but set cont_addr to the
offset within the config instead of a flash address, so the tail was read
from code space. The file server sets cont_addr absolute; do the same by
adding CONFIG_START.
The bug was unreachable while CONFIG_UPLOAD_BUF capped uploads below
TCP_OUTBUF_SIZE, but a stored config near the full sector exposes it.
sfp_read_reg() sat in rtlplayground.c, so it occupied the common 16 KB
window that every bank shares, even though nothing outside the SFP paths
calls it. That window is the tightest resource in the image:
SWTG024AS_V2_0 and SWTG024AS_A_2_0_1_5C_1SFP had 54 bytes left in it.
rtl837x_pins.c is already in BANK2 and already holds the I2C bus helpers
this function calls, so the transfer belongs there. The function moves
verbatim and becomes __banked; the prototype in rtl837x_common.h says so,
which is what keeps the callers in BANK1 and BANK2 honest.
No behaviour change. The common segment gains 200 bytes on every machine:
159 to 359 free on SWTGW218AS, 54 to 254 on the two variants above.
BANK1 +6 bytes, BANK2 +336. Built on sdcc 4.5.0.
Saving the configuration works in Chrome and fails in Firefox, and the
difference is only how each browser splits the request. The handler
erased the config sector as soon as the request started and then parsed
the multipart body one TCP segment at a time, which requires every
boundary and every part header to fall inside a single segment. Firefox
splits inside a part header, so the parser lost its place and the
erased sector was left holding a truncated body or nothing at all. A
single-burst scripted post lost the whole body the same way.
The configuration is limited to two kilobytes, so the whole request body
now accumulates in xdata and is parsed only after the closing boundary
has arrived. The parts are walked in one pass, the part carrying a
filename is written to a freshly erased sector, and the client receives
a 200 instead of the previous silent close. No segmentation can confuse
this, since the parser only ever sees a complete body.
Locating the closing boundary first also bounds the walk over the parts,
since none can lie beyond it, so the length of the buffer is no longer
the bound and the test for the two trailing dashes is unnecessary.
The walk matches at offsets inside a buffer that is not terminated, so
neither existing helper fits: strcmp() goes on to compare the byte after
the match and is_word_x() demands a separator there. Add strstart() and
strstart_x() for that case, and use strlen_x() for the boundary length.
The firmware upload path still streams, since a megabyte cannot be
buffered, and is untouched.
The bounded waits were silent: on timeout the code went straight back to
its caller, and handle_rx() then read a frame the DMA may never have
delivered, which is worse than waiting longer.
Each of the three transfers now says so on the console, the two RX ones
report failure to handle_rx(), and handle_rx() acknowledges the packet
and gives up on it rather than parsing whatever is in the buffer. The
guard variable moved to the top of its function, so the block that held
it and its indentation are gone.
The comment said the ASIC never completes a TX when the egress port is in a
non-forwarding MSTP state. The guard is worth keeping either way, since an
unbounded spin in the DMA wait takes the whole main loop down, but the
mechanism is more than the evidence supports and the DMA into the TX ring
has no business knowing the egress port at all.
The variable lives in rtlplayground.c, so it is declared in rtl837x_common.h
with the others there, and it follows the naming of the rest.
The header carried comments on the externs that the definitions in
rtl837x_stp.c repeat, sometimes differently, which is one place too many to
keep in step. What only the header had, the value ranges and what the
designated arrays hold, moved to the definitions; the rest is gone. Function
declarations lost their comments too.
The status printer only prints, so its running commentary went. A define
replaces the bare 33 in stp_in(), and the note on the loop check is down to
what applies at that line.
A blocked port does pass a received BPDU up to the CPU. The evidence is in
logicog's capture of a looped pair: the port the loop check had already
blocked kept reporting a BPDU age of zero seconds across dumps taken more
than a forward delay apart, and that counter is only cleared in stp_in().
That is also what makes the loop latch work, since the port that stays
forwarding has to go on hearing the blocked one.
The tag flag comment said the source address is not learned on the egress
port. doc/CpuPort.md defines it as not learning the source address from the
frame at all, which is the narrower claim to make. The dangling heading
above it described a field that is documented there too.
802.1D puts the port's own address in the source field and the bridge
address only in the Bridge Identifier. We used the bridge address for
both, and on this hardware that costs the management path.
Measured on a SWTGW218AS: the ASIC learns the source address of a frame
addressed to 01:80:c2:00:00:00, and the bridge's own address is not
exempt. A BPDU that leaves a blocked port and comes back on a forwarding
one therefore moves the management address off the CPU port, and frames
for it are then sent down that port instead of to the CPU. Traffic
between other stations is unaffected, which is what makes it look like
the CPU port has been blocked.
The derived address keeps the bridge address and sets the locally
administered bit, so it differs from the bridge address in the first
octet for any globally assigned OUI, with the port number in the low
nibble of the last octet. Nothing here reads the source address of a
received BPDU; the loop check compares the Bridge Identifier.
Declaring port at the top of stp_in() and stp_parse() does the same job
without a block that is not indented like one. The static xdata copy in
stp_in() went with it, it was only ever written.
The argument count check in stp_parse() that lost its comment guards
against cmd_compare(4, ..) reading a stale word from the previous command
line, because cmd_words_b is not cleared between commands.
tcpip_output() splices the 802.1Q tag in right behind the source address,
which is exactly where the ASIC expects the RTL tag of a frame the CPU
addressed to a port itself. The tag then ends up behind the VLAN tag, the
ASIC does not find it, and the frame goes out flooded with the 0x8899
header still on it instead of being sent to the port that was asked for.
Whether a frame is CPU-tagged is a property of the frame, so decide it
here from the ether-type rather than having every sender of such a frame
clear management_vlan around its tcpip_output() call. stp_cnf_send() did
that, and no longer has to.
The comments they replace are gone with them. Two things the comments
carried that the names do not:
Accepting version >= 2 rather than == 2 is deliberate. 802.1D-2004 14.4
has an RSTP bridge accept a higher Protocol Version and treat it as RST,
and MSTP sends version 3 type 2 with a prefix identical to an RST BPDU
for exactly that reason, so insisting on == 2 would make us blind to
every MST bridge on the segment.
In the TCN branch stp_cnf_send() transmits by itself, so uip_len is
cleared afterwards to keep handle_rx() from sending the frame twice.
Worth keeping out of the code but on record: RTL_TAG_KEEP is deliberately
not set here. On an LLC/802.3 frame the ASIC drops the frame outright with
that flag, while on ethertype frames such as LACP it works fine.
The port number comes out of a received BPDU, so the range check is
there to keep a forged frame from naming a port this module does not
manage - including the CPU port, which would cost us the management
path. Nothing outside min_port..max_port would ever release the block
either, because stp_timers() only counts down the ports it walks.
The three xdata bytes and the reused stp_we_root scratch are gone; the
compiler needs a register for an index either way. Frees four bytes of
xdata and 29 bytes of code.
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.
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.
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 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.
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.
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.
"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.
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.
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.
René van Dorst
bloqaudio
logicog
feelfree69
d00f
donbernhardo
d00f