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.
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 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.
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.
d00f
feelfree69
Matt Merhar
René van Dorst
bloqaudio
gnacho
logicog