Follow-up to #494. Storm control (#455) was merged after it and brings
three objects in code space without const: the extern declaration of
machine in rtl837x_storm.c and the name tables storm_types and
storm_names. SDCC 4.6.0 reports them as warnings 356 and 357, which
fail the build with CI=1 (--Werror). SDCC 4.5, which the CI uses, does
not report them.
The declarations now take the form used in the rest of the tree, as in
mime_strings in httpd.c. The images for KP_9000_6XHML_X2 and
KP_9000_9XHML_X_V3_1 are byte-identical before and after.
Storm control used meters 0 to 39, the same meters the ACL hands out,
so a storm limit and an acl meter could overwrite each other. It now
uses 24 + port * 4 + type, meters 24 to 63.
parse_storm() is no longer reentrant and uses plain locals, looks the
type up in a while loop and returns straight from the one-line cases.
storm_show() uses plain locals too.
The documentation says how IGMP decides between mcast and umcast, how
to get from a meter index to its limit, where the IFG bit of a meter
is, and what an ingress bandwidth limit on the same port does to the
storm limit.
The name branch read the fourth word without checking that the line had
one, and the tokenizer does not reset unused word slots, so "port 5 name"
took the name from wherever the previous command's fourth word had been:
"port 5 name hello" followed by "port 5 name" gave "hello" again, and
after a longer line it gave the tail of that line. The web UI never sends
the form, the console can. With no argument the name is now empty, which
also gives the console a way to remove a name.
The receive interrupt was wired up but never used: EX1 was enabled, the
handler re-enabled itself and did nothing, and the mask bit for it in the
NIC stayed clear, so idle() read the receive buffer register on every
pass to find out whether a packet had arrived, and slept until the next
tick when none had. A packet that arrived during that sleep waited for
the tick: 2.2 ms on average, 5 ms at worst, for every frame the CPU
handles.
Now the handler latches rx_irq and masks itself, and idle() sleeps
unless a receive, a link change, a console command or the tick is
pending. On rx_irq the loop drains the buffer, clears the status bit and
re-enables EX1, in that order: the NIC holds the line as long as the
status bit is set, and the bit does not clear while the buffer still
holds a frame, so a frame that lands between the last read and EX1 = 1
raises the interrupt again rather than getting lost. The other order,
clearing before draining, left the line asserted and produced an
interrupt on every pass. The mask is written after nic_setup(), which
resets it.
Measured on an RTL8373 board with the same counters in both images,
clocked by the tick counter, traffic from a host behind an LACP trunk:
```
polling interrupt
idle, register ops/s 788 79
25 pps, RTT avg/max 2.19/4.96 0.44/0.75 ms
500 pps, RTT avg/max 2.78/5.06 0.39/5.40 ms
500 pps, loss 0.08% 0.08%
```
The handler compiles to clr EX1 / setb rx_irq / reti.
The timer interrupt now raises a tick flag too, so idle() no longer works
out TICKS_DUE twice per pass, and cmd_available moves from xdata to a
bit. The four wake-up flags sit in the low bits of one byte at 0x20, so
the sleep test is a single read of that byte. tx_buf_empty deliberately
stays out: it is 1 when there is nothing to do, so including it would
keep the sleep test from ever firing.
Two per-port MAC status registers that nothing in the firmware reads:
0x6408 says which end is master on a BASE-T link, 0x63FC and 0x6400 say
whether pause was negotiated in each direction. port <n> show prints
both at the end.
The read sits in rtl837x_port.c rather than phy_show(), because these
are MAC registers and phy_show() is skipped for SFP ports. Master/slave
keys off machine.is_sfp[] so it stays off the fibre ports, where the
notion does not apply.
Bit N of each register is chip port N, which is not how
RTL837X_REG_LINKS_STS indexes its bytes a few lines above in the same
header.
The counters, the phase hooks in idle() and the "health" command are now
behind HEALTH, set from the command line with HEALTH=1. Without it the
hooks are empty macros and the command does not exist, so a default build
carries none of it: on SWTGW218AS that is 1065 bytes of bank 2, 101 of the
common segment and 28 of xdata, plus seven banked calls per main-loop pass.
It is a developer's tool. Reading the dump takes knowing the main loop, and
a user who needs it for a report can build with the flag on, which is how
the kernel treats its lock and latency instrumentation as well.
The tcp slot dump printed the slot index behind `if (UIP_CONNS > 1)`, and
with one connection configured that is a constant-false condition around
live code: sdcc reported two "unreachable code" warnings for the body and
one "conditional flow changed by optimizer" for the loop that could only
run once.
Print the single slot directly and state the assumption as a build-time
check, so a build that raises UIP_CONNS fails loudly here instead of
quietly labelling every line as slot zero. Builds warning-free again.
Every probe of a misbehaving switch so far measured it from the
outside. This puts the witnesses inside.
Each idle() pass increments a loop counter and stamps a checkpoint
after every phase: link polling, SFP, RX, TX, STP and the command
dispatch. A phase spanning two or more system ticks counts as slow and
its worst time is kept, in 5 ms units. handle_rx counts frames, so the
dump separates a flooded CPU from a stalled loop.
At boot the free stack area is painted, preserving the interrupt state
around the painting, and the dump reports how much of the pattern
survives next to the live stack pointer. The TCP connection table is
printed whole: state, timers, retransmissions, ports and peer, next to
the httpd transfer state, so a permanently occupied slot names its
occupant. With more than one slot configured every slot is listed.
Everything is raw hex; two dumps a known time apart give the rates.
The implementation sits in cmd_parser next to the other command
handlers and needs no storage beyond its counters.
Building with SDCC 4.6.0 emits tons of warnings e.g.:
cmd_parser.c:31: warning 356: object in read-only code space should be const
cmd_parser.c:36: warning 357: pointer to object in read-only code space should be pointer to const
These of course become fatal when building with --Werror.
Comparing resultant binaries, it seems that a single string is shifted
(deduplication?) but the numbers emitted at the end of the build are
otherwise identical.
parse_eee() sniffs word 2 for a trailing 'g' or 'm' to tell a speed from
a port number, and advances idx past the digits to do so. The port branch
then hands that advanced idx to cmd_parse_port_separator(), which expects
the first character of the number and finds the separator instead, so it
returns 0 and the command is rejected. Every other caller passes
cmd_words_b[].
The result is that no eee command carrying a port number works, in any
argument order, and the stored per-port lines a saved configuration
replays are silently dropped.
Reported by tofurky in issue #459, along with this fix.
The chip has a pair of thermal meters and nothing was reading them. This adds a
`temp` command that prints the current reading next to the one taken at power-on.
The result register at 0x340 splits a 19 bit value across two fields: bits 18:3
live in the low 16 bits, bits 2:0 in bits 18:16, with the sign at bit 18, degrees
in 17:10 and a fraction below. The nice part of that layout is that the degrees
fall out of a shift of the low half, so the conversion stays 16 bit.
The meter comes out of reset latched: the register keeps whatever was measured
when the board powered up and then never moves. Four samples 40 s apart came back
identical down to the raw ADC, and 0x340 matched the power-on register at 0x350.
Clearing bit 2 of the control register at 0x338 during init is what makes it live.
Measured on an SWTGW218AS: 58.7 to 59.5 C while running, with the power-on
register still showing the 49.2 C it started from. For what it is worth, the
scale lines up three ways: the threshold register reads -40 and +125, the slope
from warm-up works out at 4450 LSB/C, and the slope between the two meters at
4388 LSB/C.
We only read the meter that is up after boot. The other sits in reset with no
coefficients, so it would just repeat its power-on value.
Costs 231 bytes in BANK2.
The helper landed between that comment and the function it describes, so the
comment now reads as if it belonged to name_char(). Moving the helper up puts
the two back together. The generated code is unchanged.
execute_config() replays the saved configuration line by line into
cmd_buffer. A line that does not fit, or one cmd_tokenize() rejects,
ended the replay right there: everything below it was silently left
unapplied, while the file itself and the editor showing it stayed
complete. On a switch here, a 130-byte line placed after the last real
setting was enough to lose every line that followed it on the next boot.
Report the offending line and carry on with the next one. The skip flag
lives across the flash page refill, so a long line that crosses a page
boundary is not resumed halfway and run as a truncated command.
The console, POST /cmd and the saved config all go through the command
parser, so one filter there covers every way a name can be set. The
hostname already did this inline; port and VLAN names now do it too.
The SFP strings still need escaping on the way out: they come from the
module's EEPROM, not from a command.
The messages said Error: themselves, or usage:, or nothing at all.
cmd_error() prints the prefix now, so every rejected command answers the
same way and the strings only carry what is specific to them.
Two messages that started with Error or refusing are reworded so the
prefix does not read twice. The syslog usage text becomes one string
instead of three calls.
run_cmd_body() answers POST /cmd with 400 when err_status is set, but
only the tokenizer set it. Every "Error:", "Invalid ..." and "Unknown
command" from the parsers went back as 200, so the web page reported the
command as applied, and save_cmd put the rejected line into the command
history, from where Save to flash wrote it into the startup config.
Set ERR_INVALID_ARGUMENT on every path that rejects a command: the err:
labels of the parsers, the parsers that print their own error, the
stp parser and the unknown command fallback. Warnings that still apply
the command, such as a full VLAN name table, are left alone.
Record a command in the history only when it was accepted, and let
execute_commands() report the first failure of a multi-line body, since
cmd_tokenize() clears err_status again for every following line.
The session timeout was a compile-time constant (200 s), so changing it
meant rebuilding the firmware. Make it a variable that keeps the same
default and add a "session <seconds>" command, so it can be set from
the console or from the startup configuration like the other settings.
Measured on KP_9000_9XHML_X_V3_1: ROM +180 bytes (BANK1 +17, BANK2
+161) and xdata +2 bytes; the internal RAM map is unchanged.
sfp.c takes handle_sfp, sfp_module_read, sfp_apply_quirks, sfp_get_info,
sfp_read_field, sfp_print_info, setup_sfp_gpio, the quirk table and the module
state that goes with them. Everything exported is __banked: handle_sfp() and
sfp_print_info() are reached from cmd_parser.c in BANK2, the rest from the
common area, and both are bank boundaries.
The quirk table needed checking rather than assuming. Its entries are __code
pointers, and a __code pointer carries no bank, so passing one to a function in
a different bank hands it an offset read through the wrong window. That is safe
here because strcmp() sits in the common segment at 0x51f and is reached with a
plain lcall, so the bank register still says 3 while it runs. Verified in the
built image: the table at 0x35580 holds 0x563c and 0x5644, which resolve to
"QSFPTEK" and "QT-SFP+-T" inside BANK3.
Common segment 13393 -> 8432 bytes, leaving 7492 free rather than 2531. BANK3
holds 5710, BANK1 is unchanged and BANK2 grows by 31 for the trampolines.
All 34 machines in machine.h build and link, and the same walk over the
generated assembly finds no plain lcall crossing a bank boundary.
parse_mac() now writes to the global mac_parse_result[6] and takes only the
index, like the other parsers (ip[4]). This drops the local __xdata buffer
and the now-unused hexval() helper.
Build (KP_9000_9XHML_X_V3_1): ROM 110302 -> 110193 bytes, XSEG and internal
RAM unchanged.
parse_mac now accepts only a strict aa:bb:cc:dd:ee:ff (or '-' separators)
form: each octet is exactly two hex digits, the 5 separators are a single
consistent character, and nothing is allowed after the 6th octet. This
rejects the malformed values the reviewer flagged (repeated separators,
missing digits, trailing characters). A new inline hexval() helper does the
digit-to-value conversion and can be reused by future commands.
print_mac and print_ip are marked __banked to match cmd_parser(), which
lives in codeseg BANK2 and is itself __banked, so callers across the code
bank work correctly. cmd_parser.h is updated with the new prototypes and
the __banked qualifier on print_ip.
Verified with make MACHINE=KP_9000_9XHML_X_V3_1: the build links cleanly
and the internal-RAM (DSEG) layout is unchanged against main; XSEG is
unchanged and ROM grows by 184 bytes. A host-side test confirms parse_mac
rejects the two malformed inputs from the review.
A VLAN name is copied into vlan_names with nothing checking vlan_ptr
against VLAN_NAMES_SIZE. The only limit is the 128 byte command line, so
enough named VLANs walk straight off the end of a 1 kB table.
What sits immediately behind it makes this worse than a plain overflow: at
0x048A comes vlan_ptr itself, the index driving the write, then port_names
and cmd_buffer. Once the index is corrupted the following writes land
wherever it now points, and the configuration is replayed at boot, so a
config file that overflows does it again on every start.
Measure the name first and refuse it if the three id digits, the name, its
separating space and the terminator would not fit. The check goes before
vlan_name_remove() so a name that does not fit cannot take the existing one
with it, and the VLAN still gets its ports: dropping the whole line would
lose port membership during a config replay, which is worse than losing a
name.
The copy walks a pointer instead of indexing vlan_names[vlan_ptr++] each time,
and the word offset comes out of cmd_words_b once. Writing the bound as
VLAN_NAMES_SIZE - 5 folds the constant at compile time instead of adding it on
every call. BANK2 ends up 25 bytes smaller than it was before the check
existed.
Add a `mac <aa:bb:cc:dd:ee:ff>` console command (and a bare `mac` to show
the current address) that overrides the management MAC at runtime. The
address is validated (unicast, globally administered, non-zero OUI) and
the static L2 management entry is re-registered so the change applies
without a reboot. Because it is a normal command, a line in the startup
config persists it across boots: execute_config() runs it before the final
static L2 entry and the derived hostname are set up.
Also point the keepLink KP-9000-9XHML-X V3.1 machine definition at the
factory MAC area (0x1FC000, same location other machines use) so a valid
factory MAC is picked up at boot instead of falling back to a locally
administered address.
The MAC parsing/printing lives in parse_mac()/print_mac() helpers next to
parse_ip()/print_ip() so future commands can reuse them, and a memcmp()
helper joins the project's own memcpy()/memset() (no libc is linked).
Fold the "group with no ports" case into the argument-count guard
(cmd_words_len < 3) instead of a separate check, and accept only "d" for
delete, dropping the longer "delete" spelling. Update the serial-console
docs to match.
Two small things about the same command.
The usage line offered "lag <1-4> [port]...", which leaves out "lag show" even
though that is the only way to see the groups, so the one subcommand a reader
most needs is the one the error does not mention.
And a group number with no ports after it quietly wiped the group. That is a
reasonable thing to want and an unreasonable thing to do by accident, as it is
one fat-fingered return away from a bare "lag 1". It now takes "lag 1 delete"
or "lag 1 d", and a bare group number prints the usage instead.
Nothing changes for a saved configuration: "lag 1 5 6" is what gets written and
what gets replayed, and neither the old bare form nor the new delete matches
the pattern that decides what is persisted.
The CPU's own MAC exists in the L2 table only as a learned entry,
created whenever the CPU transmits, and it ages out like any other
dynamic entry. Unknown unicast is not flooded to the CPU port, so on an
idle switch (no web session, no management traffic) the entry expires
after the aging time and frames for the management IP no longer reach
the CPU: pings die until the next CPU transmission (an ARP reply)
re-teaches the entry, then die again. Real management sessions keep the
entry alive continuously, which is why this is only visible on idle
switches.
Install a static entry for the management MAC on the CPU port after the
startup config has been replayed, so it lands in the final management
VLAN, and move it when the management VLAN is changed at runtime.
Entry mechanics, established on hardware: a table write with read-method
0 hashes the key in TBL_DATA_IN and places the entry itself (a search
returns index 0 on a miss, there is no software-visible insertion slot).
The age field in the third data word must be non-zero for the entry to
be visible to lookup at all, and the static flag (bit 0 of byte 1 of
that word) stops the aging engine from decrementing it: with the flag a
full-age entry survives untouched where an identical unflagged entry
ages to invisible within the aging period. With no management VLAN
configured the entry lives in VLAN 1.
The pointer is masked when it advances over the command text, then
incremented once more for the newline without masking, so it can come to
rest one past the end of the ring.
Both readers walk from the pointer with their own index masked every
step, so an index that sits outside the ring is never reached and the
walk does not end. `history` on the console spins there, and so does
`/cmd_log`, which the web page fetches whenever settings are saved: it
fills the transmit buffer and keeps going past it.
The member mask is filled with a loop that stops one short, so the
highest front panel port never gets its bit and stays isolated from the
port that was just opened up. Every other loop over the port range in
the tree treats machine.max_port as the last port rather than the one
past it.
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.
Hiro
DrDoof
gnacho
René van Dorst
feelfree69
Matt Merhar
bloqaudio
logicog