stp_enabled is a flag, so say bool. cmpBytes() returns a comparison
result, so say int8_t. The three busy waits this branch adds read the
status straight out of the SFR instead of copying four bytes to xdata
first.
The hand rolled loop was here because atoi_short() reported "no digits"
but not overflow, so 65540 would have wrapped to 4 and opened a
connection to a port nobody asked for. #326 gave it the same bound this
loop was carrying, so the reason is gone.
Zero still covers all three ways to be wrong, except that "no digits"
and overflow now come back as a zero return and port zero as a zero
value, so both are tested.
Three xdata scratch variables go with the loop.
The loop tested the digit range and the overflow bound with separate ifs,
on the strength of a comment saying the joined form makes sdcc park a
boolean in the bit area, and that an image carrying LACP and STP as well
is one bit short of needing a third byte of it. Measured on this branch
that is not what happens: the joined form links, the bit segment is
unchanged at fourteen bits overall and nine in cmd_parser, and BANK2
comes out ten bytes smaller.
The claim was about an image this branch does not build, and no branch
currently carries LACP, STP and this parser at once, so there is nothing
left to check it against. A justification nobody can verify, for a shape
nobody would otherwise write, is worse than no comment at all.
Behaviour is unchanged. The two forms agree on every input from 0 to
99999 and on the edges: empty, non digits, trailing junk, leading zeros,
65535 and 65536.
The note about atoi_short() stays, because that one still reports "no
digits" without reporting overflow. #326 replaces it with a version that
does both, and this should move onto it once that lands.
The port was frozen at 514 in the call that opens the connection, so a
collector listening anywhere else was simply out of reach, and plenty do,
because 514 is privileged and containerised collectors usually get a high
port instead. Working around it meant putting a proxy in front, which then
rewrites the source address and hides which device actually sent the line.
Add `syslog port <n>`. The default stays 514, so nothing changes for an
existing configuration. Setting it tears the connection down and builds a
new one, the same dance the address already does, because uip_udp_new()
freezes the port into the connection.
The value is parsed here rather than through atoi_short(), which reports
"no digits" but not overflow: 65540 would quietly wrap to 4 and we would
open a connection to a port nobody asked for. Checking before each multiply
keeps it in 16-bit arithmetic. Zero then covers all three ways to be wrong -
no digits, past 65535, or an explicit "0", which is not a port either.
Whitelist the new command in config.js as well, next to `syslog ip`, or the
line is dropped without a word when the configuration is saved and the
setting quietly fails to survive a reboot. The dedup entry has to precede
the general `^syslog\b` one, otherwise it would deduplicate against
`syslog on`.
itoa_short() is new: itoa() only handles a byte and print_short() prints
hex, so there was no way to show a port as a decimal number. Kept separate
from itoa() rather than widening it, because every existing caller passes a
byte and would start paying for 16-bit divisions it does not need.
The scratch variables for the parser live in xdata. Locals there would take
internal RAM, and this image has none left: data ends at 0x77, the overlay
occupies 0x78-0x7f and the stack starts at 0x80. Measured: no module's DSEG
grew.
cmd_parser.c conflicted twice: main rewrote the pvid command around the
new port-separator and atoi helpers, next to the line where this branch
delegates "stp" to stp_parse(). Both belong, so the STP delegation keeps
main's pvid body.
The linker caught what the merge could not see: main changed atoi_byte()
to return the digit count and leave the value in atoi_results_u8, where
it used to return non-zero on failure and write through a pointer. Note
the sense is inverted, so the three calls in rtl837x_stp.c are adjusted
rather than just re-arranged.
Only httpd.c conflicted: main added the buffered configuration upload
next to the pointers this branch had moved into xdata to free internal
RAM. Both belong, so the new globals sit above declarations that keep
their storage class.
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.
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.
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.
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)
The single-digit arm of the ingress parser guards with p - '1' > 9,
which no digit can satisfy: the largest, '9', gives 8. The digit that
needed rejecting is '0', which gives -1 and indexes one byte before
phys_to_log_port, so "ingress 0 t" reads out of bounds and applies the
ingress mode to whatever port number that byte happens to contain.
Ports are 1-based, so reject anything below '1'; values above '9' are
already excluded by the isnumber check before this.
parse_isolate() accepted a two digit port and mapped it to logical port 9,
the CPU port, while port_isolate() and port_isolation_get() both refuse
anything above machine.max_port. Setting the isolation of the CPU port was
therefore declined without a word and reading it always answered no
members, whatever the hardware held.
Bound the port to the front panel, so the command says what it does. The
digit is checked before it indexes phys_to_log_port[], which a non numeric
argument used to read past.
handle_rx() dispatched to igmp_packet_handler() on the destination
address alone, so an ordinary IGMPv3 report off the wire reached the
handler and could write a table entry whether or not anyone had asked
for snooping. The STP branch right above it is gated on stpEnabled;
this brings the IGMP branch in line.
Snooping state lived only in the per-port registers, and the receive
path cannot afford to read one per packet, so the flag shadows it:
igmp_enable() sets it, igmp_setup() clears it, and igmp_setup() runs
from both the boot path and "igmp off".
While here, igmp off becomes an explicit subcommand instead of the
fall-through, and an unrecognised igmp subcommand prints the usage
line rather than silently turning snooping off.
Six bytes of BANK1 and one of xdata, no internal RAM.
lag show has always printed the groups as 1 to 4 while lag <n> took the
number literally, so typing what you saw configured the group beside it.
Both lag and lag hash count from one now, matching how ports are numbered
everywhere else, and reject anything outside 1 to 4. Subtracting '1' makes
0 wrap well past three, so one test covers both ends.
The port argument indexed machine.phys_to_log_port, which holds nine
entries, before it was checked, and a two digit argument reaches 109. It is
bounded before the table is touched rather than after.
port_lag_members_set() and port_lag_hash_set() complained about a group out
of range and then wrote the registers anyway, past the four the groups
occupy. They return instead.
lag hash also read cmd_words_b[1] without checking a word was there, and
now shares the error path parse_lag() already had.
sfp_read_reg() waited for the transfer to finish and then read the output
register whatever the outcome, so an address nothing acknowledged came back
as an ordinary byte and no caller could tell it apart from data. The vendor
SDK looks at bit 1 of the control register for exactly this, and we did
not.
A failure now sets sfp_i2c_fail and the read returns 0xff, which is already
the value sfp_apply_quirks() reads as either a failed transfer or a voltage
the spec does not allow, so that test starts being true when it should be.
The insertion path and the sfp command clear the flag first and say so
afterwards, rather than presenting the bytes as though they came from the
module.
What this deliberately does not do is act on the failure. Skipping
sds_config() when the rate read failed is the obvious next step, but a
module that raises the bit spuriously would then never be configured at
all, which is worse than what happens today, and I have no way to judge how
often the bit is right. That decision belongs with someone holding the
board.
It also leaves the other half of the rewrite alone, reading and writing up
to sixteen bytes per transaction. doc/sfp.md describes only the single byte
path and does not name a length field, and guessing at a register I cannot
test is how the last attempt at this function went wrong.
40 bytes of the common segment, 51 of BANK2 and 1 of xdata, nothing in
BANK1 or internal RAM. Built for SWTGW218AS and KP_9000_6XHML_X2 on sdcc
4.5.0. Not tested on hardware: shorting the clock line, as in #342, should
now print the failure line instead of a plausible looking byte.
The member mask of an aggregation group is decoded by hand in two places,
the lag command and the JSON behind the aggregation page, and every branch
that touches trunks adds another copy.
port_lag_members_get() sits next to port_lag_members_set() and both readers
call it. It answers from the hardware, so it covers a group configured with
lag and one a protocol brought up, without either having to say so.
It reads through reg_read() rather than reg_read_m(), so sfr_data is left
alone. Neither caller looked at it afterwards; both read the hash register
next.
Both handlers turned the first character of the port word into an index
with no check at all. "mtu 0 100" computes '0' - '1' = 255, reads far past
the end of phys_to_log_port[9], and writes the size to whatever register
0x1250 plus that garbage points at. "pvid 0 2" walks the same path into
port_pvid_set(). lag and vlan already validate their port arguments; these
two just did not.
The port now has to be a single digit 1 to 9, which is exactly the range
the mapping table holds. A second digit or a stray letter falls to the
usage message.
"mtu show" also gained the return it was missing: after printing the table
it fell through, derived a port from the word "show" and printed the
garbage byte before the length check stopped it.
"vlan 5000 mgmt" parked the management interface on a VLAN the table
cannot hold, which quietly cuts management off. IDs above 4094 now fall
through to the usage message. 0 still switches the management VLAN off,
which is the documented way to disable it.
"pvid 1 5000" packed 5000 into the 12-bit PVID field and truncated on the
way, so the port ended up with a PVID nobody chose. 0 and anything above
4094 are refused now, matching what the VLAN table can hold. A failed
parse used to be dropped without a word; both cases print the usage line.
The handler threw away atoi_short()'s return and leant on the range test
alone. The range test cannot tell a failed parse from a small number, so
"mtu 1 99999" stopped at the partial 9999 and went through as a number
nobody typed. With the parse result checked, a failure is rejected with
the same message as an out-of-range value.
"mtu 1 0" was accepted. The chip takes it verbatim, the port reports 0x0000
back, and it stops passing frames: on a live 2.5G LAG member the LACPDU
receives moved by 4 in fifteen seconds against 21 on the sibling port, and
the partner went expired. Restoring the size brought both back.
Nothing shorter than a minimum Ethernet frame is a usable maximum, so the
range is now 64 to 16383. The upper end is unchanged and still comes from
the width of the field the value is written into.
This also covers most of #312 by accident: "mtu 1 abc" leaves the parse
result at 0 and now gets rejected on the bound rather than reaching the
register. It does not cover all of it. The handler still ignores what
atoi_short() returns, so "mtu 1 99999" stops on a partial 9999 and goes
through as a number nobody typed.
The GUI is not affected either way, it offers a fixed list of sizes.
vlan_create() and vlan_delete() wrote the ID straight into the table index
register. vlan_get() has refused anything >= 0xfff for a while, so reads were
guarded and writes were not: "vlan 4095 1 2" built an entry that no read path
can see, and IDs above that either miss the table or alias onto another VLAN.
Both writers now enforce the range vlan_get() already did, and parse_vlan()
rejects the same values with the usage message so the CLI says why. The check
sits after the "vlan 0 mgmt" branch, which legitimately takes 0 to switch the
management VLAN off.
Costs nothing in RAM: rtl837x_port.rel stays at DSEG 0, OSEG 5, and the image
still reports 10207 bytes of XDATA in use.
atoi_short() accumulated into a uint16_t without checking, so "vlan 65540"
wrapped to 4 and edited VLAN 4 instead of failing. atoi_byte() had the same
hole with "300" landing on 44. Both now refuse the digit that would push the
value past its type, before it lands.
The partial result is deliberately left alone rather than zeroed. Zeroing
would give the function one tidy rule, every failure leaves 0, but a caller
that ignores the return would then write that 0, and 0 is not a harmless
number everywhere. Set as a port MTU it stops the port taking frames: I put
0 on a live 2.5G LAG member and its LACPDU receives moved by 4 in fifteen
seconds against 21 on the sibling port, with the partner going expired.
Putting the size back recovered both. A wrong number does less damage than
that, and the real fix belongs in the callers that ignore the return anyway.
The test sits inside the loop rather than after it, so no wider accumulator
is needed and the parser stays off the internal RAM budget.
056a30a on the branch in #303 fixes atoi_byte a different way, by widening
the accumulator. Whichever lands first, the other hunk should go.
Typing "hostname" on its own cleared the name: with no argument the copy
loop never ran and the terminating NUL landed at index 0. Report the
current name instead, accept exactly one argument to set it, and reject
anything longer - a name with spaces tokenizes into several words, and
silently keeping only the first one is worse than an error. Walk the
buffer with a pointer, which the compiler codes better than indexing.
Suggested-by: vDorst
d00f
René van Dorst
logicog
d00f
bloqaudio