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.
The lookup tables live in BANK1, so the routine switches the code bank to
reach them with movc and never switches it back. That is the contract the
__naked declaration already implied: same bank or common segment only.
The name now says it.
crc16_bank1 is the routine unchanged. crc16 becomes a __banked wrapper in
BANK1, so a caller from another bank has its own bank put back by
__sdcc_banked_ret. The two existing call sites take the direct path and
pay nothing.
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.
The RTL tag carries a 4-bit port number for received frames, and the two
places that turn it into a mask bit take the whole byte:
entry.pmask |= (1L << (IGMP_I->rtl_tag.pmask >> 8));
The comment beside it already says only the low four bits count, but nothing
takes them. rtl837x_stp.c:424 does the same extraction and does mask:
stp_scratch = ((uint8_t)HTONS(STP_I->rtl_tag.pmask)) & 0x0f;
With a count above 15 the result is not a bit at all. SDCC narrows the 1L to
16 bits and emits the shift as a djnz loop, so a count of 200 spins the loop
200 times and leaves zero behind: the join adds no port and the leave clears
none, neither saying anything. It also spends those iterations inside packet
handling.
Mask to four bits and shift a uint16_t, which is the width the surrounding
code works in. Six bytes of BANK1.
Worth saying that this does not make snooping work on its own. struct
igmp_pkt has no vlan_tag field while struct eth_in describes the same frame
with one, so every IP and IGMP field below rtl_tag is read four bytes early.
The 100M branch of phy_set_speed() was missing an else. A port set to 100M
half wrote 0x1481, walked into the next if, found duplex was not 1, took its
else and overwrote the register with 0x1581 straight away, so it advertised
100BASE-TX half and full both. That is what asking for both is meant to do,
and forcing half is something people do to match a partner that cannot
negotiate, so the link comes up looking fine and misbehaves under load.
Bit 7 is 100BASE-TX half and bit 8 is full, which makes 0x1481, 0x1501 and
0x1581 half, full and both, the same shape as 0x1421, 0x1441 and 0x1461 in
the 10M branch above.
Both branches now choose the advertisement into a variable and write it once,
and they compare against the PHY_DUPLEX_* names instead of 0, 1 and whatever
is left. Same values reach the PHY; BANK2 loses 92 bytes.
The 100M branch of phy_set_speed() is missing an else. A port set to 100M
half writes 0x1481, falls into the next if, finds duplex is not 1 and takes
its else, and immediately overwrites the register with 0x1581. The port
ends up advertising 100BASE-TX half and full, which is what "both" is
supposed to mean.
The 10M branch directly above has the chain the 100M one meant to have, and
the values line up with it: bit 7 is 100BASE-TX half, bit 8 is full, so
0x1481, 0x1501 and 0x1581 are half, full and both, exactly as 0x1421,
0x1441 and 0x1461 are for 10M.
Forcing half duplex is something people do to match a link partner that
cannot negotiate, so quietly advertising full as well is the wrong way for
this to fail.
Not tried on hardware: every port on the switch here has a link up, and
setting the speed restarts negotiation.
isr_ext2() ends with PCON |= 1, putting the core to sleep from inside an
interrupt handler.
The DW8051 leaves idle when any enabled interrupt is activated, so the core
does not stay down for long. It naps until the next enabled interrupt request
turns up, with the 200 Hz system tick putting a ceiling of 5 ms on that, and
then carries on.
The sleep buys nothing either way, because the instruction after it is the
return. While it lasts the handler holds the priority latch, so everything at
its level or below waits, and that includes the tick, the console and the NIC
receive interrupt.
The handler still clears its flag, which is what it has to do. The bit numbers
beside the two masks were wrong: 0xef clears bit 4 and 0xdf clears bit 5,
not 7 and 6.
isr_ext2() ends with PCON |= 1, putting the core to sleep from inside an
interrupt handler.
The DW8051 leaves idle when any enabled interrupt is activated, so the core
does not stay down for long. It naps until the next enabled interrupt request
turns up, with the 200 Hz system tick putting a ceiling of 5 ms on that, and
then carries on.
The sleep buys nothing either way, because the instruction after it is the
return. While it lasts the handler holds the priority latch, so everything at
its level or below waits, and that includes the tick, the console and the NIC
receive interrupt.
The handler still clears its flag, which is what it has to do. The two wrong
comments next to the masks go as well: 0xef clears bit 4 and 0xdf clears bit 5,
not 7 and 6, and the masks already say that without help.
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).
isr_ext2() ends with PCON |= 1. On the DW8051 any enabled interrupt request
ends idle, so the handler sleeps until the next request arrives, the system
tick at the latest, and only then returns. Everything at the same priority
waits with it, and the instruction after the idle is RETI, so the sleep buys
nothing.
The handler still clears its flag, which is what it has to do. The other four
uses of PCON |= 1 are in task context and are left alone.
Option 58 is optional, and dhcp_state.renewal is only ever written by
parse_opts(), so a server that leaves it out keeps it at zero. The ACK
handler then sets dhcp_timer to zero, and the callback treats a zero timer
in LEASING as time to renew, so it sends a REQUEST on the next pass.
That REQUEST does set a 30 second timeout, but the ACK that answers it
comes back through the same path and zeroes the timer again, so the two
sides trade REQUEST and ACK as fast as the round trip allows, with the
console printing the whole lease each time round.
RFC 2131 has the client renew at half the lease when the server does not
say, so use that, and fall back to a fixed value if there is no lease
either. Both only fill in a value that is missing: a server that asks for a
short renewal still gets exactly what it asked for, which a blanket lower
bound would have overridden.
Not reproduced here, that needs a server configured without the option.
dhcp_start() never looked at whether DHCP was already running, and "ip dhcp"
calls it without stopping first, so a second start took a fresh slot from
uip_udp_conns and left the previous one bound.
The leak is the smaller half of it. dhcp_callback() decides whether a call
is for it by comparing the local port, not the connection, and every
leftover socket still holds port 68, so each one runs the state machine on
every poll. The timers live in one shared dhcp_state, so ticks count down
once per socket: two starts and the retransmit and renewal timing runs at
twice the intended rate.
Stop the old one first. dhcp_stop() already does exactly that, and the
state test keeps it away from a connection that was never created.
uip_init() does not set uip_conn, and httpd_init() runs right after it, so
the assignment went through a pointer nothing had assigned yet.
It has stayed invisible because the startup code does clear xdata: uip_conn
is NULL rather than garbage, appstate sits 28 bytes into struct uip_conn, and
the byte therefore lands at xdata 0x1C. With the current map that is inside
the command line buffer, at an offset nothing reads while the line is empty,
and cmd_editor_init() runs later anyway.
Nothing is broken today, but it is a write to an address that has nothing to
do with the state being set, and it moves the moment the struct grows or the
map shifts. Address the connection directly.
Leaving SFP ports out of the table is deliberate for now, and being able to
force a module's speed belongs in a feature of its own rather than arriving
inside a fix, so that half goes again.
What remains is the off-by-one: createPortTable() read pState[i] where the
array is filled by portNum - 1, so every row started out showing the next
port's link speed until the first refresh corrected it. The debug line above
it goes too; it printed a third index, pState[i-2].
The id was built and checked with HTONS(), which casts to 16 bits, so the
top half of the 32-bit transaction id was thrown away and the rest byte
swapped. The id is opaque: the client only has to receive its own value
back, so there is no byte order to honour and no reason to narrow it.
Assign and compare the field directly, so the full 32-bit id round-trips.
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.
The EX0..EX3 handlers each emitted a single debug character through
write_char(), which busy-waits on the serial TX-empty flag that is set by
the serial interrupt. If one of these handlers runs while a byte is being
transmitted, it waits for a flag that the equal-or-lower-priority serial
interrupt cannot deliver, hanging the handler with the system tick stopped.
The characters were only ever debug markers, so drop them; the handlers now
just acknowledge the interrupt. No interrupt service routine should call the
blocking console path.
uip_arp_timer() was defined but never called, so ARP entries were only
refreshed on new learning and only removed under table pressure. A cached
entry for a peer whose MAC later changes (a gateway failover, a replaced
device) is then used until it is evicted, and traffic to anything reached
through it is sent to the old MAC and lost, while on-link traffic is
unaffected.
uIP expects the timer roughly every ten seconds; drive it from the
once-per-second housekeeping in idle().
The guard read `if (!DHCP_P->tid == HTONS(...))`. Unary `!` binds tighter
than `==`, so this evaluated `(!tid) == HTONS(id)`, i.e. `(tid ? 0 : 1) ==
HTONS(id)`, which is false for any non-zero id and never returns. The reply
was therefore accepted regardless of its transaction id, so a broadcast
reply meant for another client on the segment was parsed as ours.
Compare the field directly. The id is stored network-order in the packet
and host-order in the state, matching the HTONS() used when the request is
built, so `DHCP_P->tid != HTONS(dhcp_state.transaction_id)` is the intended
test.
The port table skipped every SFP port outright, so on a SWTGW218AS the
uplink simply was not there. Of the seven columns only Set Speed is
meaningless for a module: the name, the link speed and the devices behind
the port all are worth showing, and the name in particular was accepted on
another page and then never displayed anywhere. The MTU table further down
the same page has always drawn the port, so the page disagreed with itself.
Give an SFP port its row and leave the three control cells empty. Whether
speed and disable should work for a module is a separate question and
untested, so this does not offer a button for it.
The link state was read one port too far. main.js fills pState by
portNum - 1, updatePortTable() reads pState[i-1], and createPortTable()
read pState[i], so every row started out showing the next port's link
speed until the first refresh corrected it. On the last copper port that
meant showing the uplink's speed. Feeding the real status.json of a nine
port switch through both versions:
expected 7 7 7 1 7 1 7 7 6
before 7 7 1 7 1 7 7 6
after 7 7 7 1 7 1 7 7 6
Dropping the skip also removes a trap rather than papering over it.
updatePortTable() addresses rows as tbl.rows[i], which only lined up with
port i because the SFP port happens to be last on this board. With a
module in the middle every row after it would have been off by one.
René van Dorst
gnacho
Rob Emery
logicog
eraiza0816
d00f