KolibriOS/kolibrios
18
28 Fork 71
Code Issues 155 Pull Requests 71 Actions Projects 2 Releases Wiki Activity

drivers/network/bcm57xx: Fix MAC_TX_LENGTHS and other #709

Open
igorsh wants to merge 5 commits from igorsh/kolibrios:bcm57xx into bcm57xx
pull from: igorsh/kolibrios:bcm57xx
merge into: KolibriOS:bcm57xx
Conversation 4 Commits 5 Files Changed 2
+338 -25
igorsh commented 2026-09-20 15:35:54 +00:00
Contributor
Copy link
Copy source

Root cause: MAC_TX_LENGTHS was defined as 0x046c, which is the receive
MAC status register. The inter packet gap went into a status register
and the real transmit lengths register at 0x0464 was never programmed,
so the send engine stalled as soon as the peer's window let it keep
more than a handful of frames in flight.

4 commits on bcm57xx:

  • write the inter packet gap to the transmit lengths
    register. One line, the actual fix.
  • correct the MAC status bit positions: CFG_CHANGED and
    SYNC_CHANGED were off by one bit.
  • only report a link down when a readable PHY confirms it.
    An unreadable PHY is not evidence, and a false verdict here is
    destructive: the stack never re-resolves a route for a socket it
    already has, so a connection with data in flight is over for good.
  • keep the send ring bookkeeping consistent: believe the
    chip's consumer index only inside the span actually handed over,
    restart the ring on the chip's index when reclaiming, and count a
    frame refused for a full ring as an overrun, not a drop.
Root cause: MAC_TX_LENGTHS was defined as 0x046c, which is the receive MAC status register. The inter packet gap went into a status register and the real transmit lengths register at 0x0464 was never programmed, so the send engine stalled as soon as the peer's window let it keep more than a handful of frames in flight. 4 commits on bcm57xx: * write the inter packet gap to the transmit lengths register. One line, the actual fix. * correct the MAC status bit positions: CFG_CHANGED and SYNC_CHANGED were off by one bit. * only report a link down when a readable PHY confirms it. An unreadable PHY is not evidence, and a false verdict here is destructive: the stack never re-resolves a route for a socket it already has, so a connection with data in flight is over for good. * keep the send ring bookkeeping consistent: believe the chip's consumer index only inside the span actually handed over, restart the ring on the chip's index when reclaiming, and count a frame refused for a full ring as an overrun, not a drop.
CODEOWNERS rules requested review from system 2026-09-20 15:35:55 +00:00
CODEOWNERS rules requested review from network 2026-09-20 15:35:55 +00:00
Burer requested changes 2026-09-23 08:34:26 +00:00
Dismissed
Burer left a comment
Owner
Copy link
Copy source

1. The link-down confirmation runs inside the interrupt handler.

check_link is called from int_handler on a MACSTAT_LINK_CHANGED attention, and the new .down path does

        mov     ecx, LINK_CONFIRM_US        ; 10000
        call    delay_us

delay_us is a busy wait — one MMIO read per microsecond, by its own comment — so this spins ~10 ms in interrupt context, blocking the scheduler and every other device's interrupt for that whole time, and hammering the PCI bus while it does. It is not once per outage either: when the confirm withdraws the verdict the state stays up, so a flapping port pays the 10 ms on every attention.

The confirmation itself is a good idea; it just cannot happen here. Options, roughly in order of preference: let the next link attention be the confirmation (record a "down pending" state and decide on the following one), move the recheck to a timer or worker context, or — if it has to stay inline — drop it to a few hundred microseconds and accept the weaker evidence.

2. dbg_worthy is called with the counter in the wrong register.

  .stale:
        inc     [ebx + device.txstale_count]
        mov     edx, [ebx + device.txstale_count]
        call    dbg_worthy

dbg_worthy reads eax (cmp eax, DBG_EVENTS), and every other call site in the file loads eax. Here eax still holds the chip's consumer index, so the rate limiting keys off a ring slot number: with DBG_EVENTS = 16 and a 512-slot ring, a stale consumer at slot 100 is silently dropped forever while one at slot 256 prints on every single occurrence. txstale_count is incremented and never read. The message wants eax for its own %u, so:

        inc     [ebx + device.txstale_count]
        push    eax
        mov     eax, [ebx + device.txstale_count]
        call    dbg_worthy
        pop     eax
        jc      .done

Smaller notes:

  • MACSTAT_TXSTAT_OFLOW = 0x08000000 — tg3 calls this bit TXSTAT_UNDERRUN; 0x04000000 is the RX overrun. Since the point of the commit is getting the definitions exactly right, the name is worth matching.
  • tx_reclaim_all now trusts status_block.tx_cons_idx unconditionally — the very field tx_clean was just taught to distrust. It is defensible because the send engine is stopped there and the index is final, but that is the reason the new comment should give.
  • Stray ;; line in the check_link header comment, and a doubled empty ; line in the tx_clean one.
  • The branch still carries netcfg: show Broadcom for vendor id 14E4, already merged as #708, plus a merge commit. Rebasing would leave the four commits that are actually this PR.

Also, please, cleanup comments and remove redundant info from them, if possible.

**1. The link-down confirmation runs inside the interrupt handler.** `check_link` is called from `int_handler` on a `MACSTAT_LINK_CHANGED` attention, and the new `.down` path does ```asm mov ecx, LINK_CONFIRM_US ; 10000 call delay_us ``` `delay_us` is a busy wait — one MMIO read per microsecond, by its own comment — so this spins ~10 ms in interrupt context, blocking the scheduler and every other device's interrupt for that whole time, and hammering the PCI bus while it does. It is not once per outage either: when the confirm withdraws the verdict the state stays up, so a flapping port pays the 10 ms on every attention. The confirmation itself is a good idea; it just cannot happen here. Options, roughly in order of preference: let the next link attention be the confirmation (record a "down pending" state and decide on the following one), move the recheck to a timer or worker context, or — if it has to stay inline — drop it to a few hundred microseconds and accept the weaker evidence. **2. `dbg_worthy` is called with the counter in the wrong register.** ```asm .stale: inc [ebx + device.txstale_count] mov edx, [ebx + device.txstale_count] call dbg_worthy ``` `dbg_worthy` reads **eax** (`cmp eax, DBG_EVENTS`), and every other call site in the file loads eax. Here eax still holds the chip's consumer index, so the rate limiting keys off a ring slot number: with `DBG_EVENTS = 16` and a 512-slot ring, a stale consumer at slot 100 is silently dropped forever while one at slot 256 prints on every single occurrence. `txstale_count` is incremented and never read. The message wants eax for its own `%u`, so: ```asm inc [ebx + device.txstale_count] push eax mov eax, [ebx + device.txstale_count] call dbg_worthy pop eax jc .done ``` Smaller notes: - `MACSTAT_TXSTAT_OFLOW = 0x08000000` — tg3 calls this bit `TXSTAT_UNDERRUN`; 0x04000000 is the RX overrun. Since the point of the commit is getting the definitions exactly right, the name is worth matching. - `tx_reclaim_all` now trusts `status_block.tx_cons_idx` unconditionally — the very field `tx_clean` was just taught to distrust. It is defensible because the send engine is stopped there and the index is final, but that is the reason the new comment should give. - Stray `;;` line in the `check_link` header comment, and a doubled empty `;` line in the `tx_clean` one. - The branch still carries `netcfg: show Broadcom for vendor id 14E4`, already merged as #708, plus a merge commit. Rebasing would leave the four commits that are actually this PR. Also, please, cleanup comments and remove redundant info from them, if possible.
Leency commented 2026-09-24 09:59:27 +00:00
Contributor
Copy link
Copy source

Jeffrey has already rewritten this driver from scratch #683

Jeffrey has already rewritten this driver from scratch https://git.kolibrios.org/KolibriOS/kolibrios/pulls/683
igorsh added 2 commits 2026-09-24 18:15:48 +00:00
bcm57xx: write the inter packet gap to the transmit lengths register a574e8f1a6 
MAC_TX_LENGTHS named 0x046c, which is the receive MAC status register.
The transmit lengths register is 0x0464 (if_bgereg.h: BGE_TX_LENGTHS
0x0464, BGE_RX_MODE 0x0468, BGE_RX_STS 0x046c), so bring-up wrote the
inter packet gap 0x2620 into a status register and left the lengths
register at whatever reset had put there.

Assisted-by: ZCode:deepseek-flash
bcm57xx: correct the MAC status bit positions and name the underrun bit fcdaf45caf 
CFG_CHANGED and SYNC_CHANGED were defined one bit low (0x4 and 0x8
instead of 0x8 and 0x10), so MACSTAT_STICKY never cleared SYNC_CHANGED
and the driver's picture of which attention bits are pending was never
quite the chip's. The rest of the table is filled in while here.

TXSTAT_OFLOW was one of those additions and carried the wrong name:
0x08000000 is the send engine underrunning, the receive overrun is
0x04000000, which the table already calls RXSTAT_OFLOW.

Nothing observed changes from this - the driver only ever tests
LINK_CHANGED, which both versions mask - but a register table that
disagrees with the reference is a trap for the next reader.

Assisted-by: ZCode:deepseek-flash
igorsh force-pushed bcm57xx from 089f7dc750 to 0c13f332d6 2026-09-24 18:15:48 +00:00 Compare
igorsh commented 2026-09-25 15:41:37 +00:00
Author
Contributor
Copy link
Copy source

Jeffrey has already rewritten this driver from scratch #683

Я туда и шлю PR в bcm57xx

> Jeffrey has already rewritten this driver from scratch https://git.kolibrios.org/KolibriOS/kolibrios/pulls/683 Я туда и шлю PR в [bcm57xx](https://git.kolibrios.org/KolibriOS/kolibrios/src/branch/bcm57xx)
👍 1
Burer approved these changes 2026-09-26 14:26:39 +00:00
Burer left a comment
Owner
Copy link
Copy source

The two earlier findings are properly fixed: the confirmation no longer busy-waits
in the interrupt handler, and dbg_worthy now gets its counter in eax. One new
issue in link_poll.

The stall moved rather than went away. link_poll holds spin_lock_irqsave
across the whole check_link call:

        spin_lock_irqsave
        ...
  .decide:
        mov     dword [ebx + device.link_suspect], 3
        call    check_link
  .done:
        spin_unlock_irqrestore

check_link does two or three phy_reads, and each one can spin

        mov     ecx, 5000
  .wait:
        mov     eax, [esi + MI_COMM]

up to 5000 register reads — about a microsecond apiece by this driver's own
reckoning, so ~5 ms per read. When the MI interface answers normally this is
microseconds and invisible. When it does not — the exact case this mechanism
exists for — it is ~10 ms with interrupts disabled, ~15 ms if AUX_STAT times out
too. Narrower than the old 10 ms in the interrupt handler, since it needs a dead
MI rather than any link event, but the same class of problem.

Two fixes, either works:

  • Hold the lock only around the link_suspect read and write, and sample the PHY
    outside it. link_poll would take the lock, read the ladder, drop it, call
    check_link, then take it again to commit — check_link already tolerates
    running unlocked, since the interrupt path calls it that way.
  • Or cut the MI timeout. 5000 iterations is very generous for an MDIO
    transaction; a few hundred would bound the worst case to well under a
    millisecond and still never trip on healthy hardware.
The two earlier findings are properly fixed: the confirmation no longer busy-waits in the interrupt handler, and `dbg_worthy` now gets its counter in eax. One new issue in `link_poll`. **The stall moved rather than went away.** `link_poll` holds `spin_lock_irqsave` across the whole `check_link` call: ```asm spin_lock_irqsave ... .decide: mov dword [ebx + device.link_suspect], 3 call check_link .done: spin_unlock_irqrestore ``` `check_link` does two or three `phy_read`s, and each one can spin ```asm mov ecx, 5000 .wait: mov eax, [esi + MI_COMM] ``` up to 5000 register reads — about a microsecond apiece by this driver's own reckoning, so ~5 ms per read. When the MI interface answers normally this is microseconds and invisible. When it does not — the exact case this mechanism exists for — it is ~10 ms with interrupts disabled, ~15 ms if AUX_STAT times out too. Narrower than the old 10 ms in the interrupt handler, since it needs a dead MI rather than any link event, but the same class of problem. Two fixes, either works: - Hold the lock only around the `link_suspect` read and write, and sample the PHY outside it. `link_poll` would take the lock, read the ladder, drop it, call `check_link`, then take it again to commit — `check_link` already tolerates running unlocked, since the interrupt path calls it that way. - Or cut the MI timeout. 5000 iterations is very generous for an MDIO transaction; a few hundred would bound the worst case to well under a millisecond and still never trip on healthy hardware.
igorsh added 3 commits 2026-09-29 17:56:37 +00:00
bcm57xx: only report a link down when a readable PHY confirms it bc18e3587c 
A link-down verdict is not a status update on this system, it is
destructive: the stack stops being able to route for the sockets it
already has and never re-resolves them, so a connection with data in
flight is over for good. This driver handed one out on the strength of a
single sample: check_link sent a failed PHY read to .fallback and, if the
MI status word did not have its link bit set there - it is the
auto-poller's view of the PHY, so it is stale exactly when the MI
interface is what broke - declared the carrier gone. A send stall was
then read as a link event, the reclaim threw the ring away and the box
lost every connection it had, which is part of why this took so long to
pin down.

An unreadable PHY is now treated as what it is, an absence of
evidence: the state is left alone and the next sample decides. MI_STS may
keep a link up, never declare one down, the way bge_link_upd uses it. A
down verdict that does come from a readable PHY needs a second sample a
whole poll interval later, and that second sample is taken by a timer, in
thread context: a busy wait inside the interrupt handler costs the machine
about ten milliseconds with interrupts off, on every edge of a flapping
port, and buys a filter too short to be worth it - the link status bit
latches low, so the second BMSR read in check_link is already the live
one. A verdict reached from an interrupt would also drag NetFree and
NetLinkChanged in there with it.

The poll ticks every LINK_POLL_HS and does nothing at all unless a
sample said the carrier is gone, so an idle link costs one memory read
per tick. It also decides the verdict, so the reclaim and the stack
notification happen in thread context. Interrupts stay off across the
tick, which is what makes the deciding sample unambiguous: ladder step 3
exists only for the span of the poll's own check_link call, so a sample
that sees it is that call's and no attention's. The guard is affordable
because phy_read bounds the sample - an MI interface that has stopped
answering gets a fixed number of reads before it gives up, so a tick can
hold the machine for about a millisecond at worst, and only while a
suspicion is pending. A withdrawal - the carrier is back before the
second sample - is counted and likewise resolved there. If the timer could not be armed at all a
single readable sample has to do, and that is logged.

Every branch logs the evidence it decided on - both BMSR reads, MI_STS,
MAC_STS, the transmit MAC's own status and the two samples the verdict was
built from - so a verdict can be explained afterwards.

Assisted-by: ZCode:deepseek-flash
bcm57xx: keep the send ring bookkeeping consistent 63ea8a0cb8 
Three ways the ring could disagree with the chip:

tx_clean walks from tx_cons to the consumer index in the status block and
believed that index whatever it was. A stale status block, or a ring
reclaimed behind the chip's back, can leave it on a slot that was never
queued, and walking to it frees buffers the send engine is still working
from: NetFree hands a buffer back at the head of the pool, so the next
allocation, transmit or receive, gets that memory and the chip reads
whatever landed in it. The index is now only believed while it points
inside the span actually handed over, tx_cons..tx_prod, and the case is
counted and logged when it is not.

tx_reclaim_all gave up on the whole ring by setting tx_cons = tx_prod,
which left the chip's own consumer index where it was. The two ends of
the ring then disagreed, tx_clean saw a consumer it could not justify and
the ring stayed full for good. It now restarts the ring at the chip's
consumer index, sets both of ours to that and publishes the same
producer, so the driver and the chip agree again.

A frame refused because the ring was full was counted as dropped, the
same counter the buffer pool running dry uses. A full ring is an overrun,
which is how the other drivers in this tree count it, so it goes to
packets_tx_ovr, and packets_tx_drop is left meaning a frame really was
thrown away.

Holding the newest descriptors back was tried as well, 32 of them, in
case the chip's index runs ahead of its DMA: it changed nothing, so there
is no lag here.

Assisted-by: ZCode:deepseek-flash
bcm57xx: bound how long an MI transaction is waited for c01ce770ce 
phy_read waits for MICOMM_BUSY to clear by reading MI_COMM back to back,
up to 5000 times. A bound in reads is not a bound in time - tg3 allows
5000 tries for the same wait, with a 10 us delay between them, so fifty
milliseconds - but neither is a problem while the interface answers. What
matters is the ceiling when it does not.

That ceiling is now 1000 reads. A frame is 64 bit times, about 26 us at
the standard 2.5 MHz MI clock, and the reads go out back to back, so a
thousand of them is a few hundred microseconds even on a PCI Express
card, an order of magnitude above the transaction they are waiting for.
What the bound buys is the worst case: check_link reads the PHY two or
three times, and from a link attention those reads happen in the
interrupt handler with interrupts off, so an interface that has stopped
answering used to cost ten to fifteen milliseconds there. It now costs
about one.

The MI interface stopping to answer has not been observed; this is the
ceiling being set deliberately rather than inherited.

Assisted-by: ZCode:deepseek-flash
igorsh force-pushed bcm57xx from 0c13f332d6 to c01ce770ce 2026-09-29 17:56:37 +00:00 Compare
You are not authorized to merge this pull request.
This pull request can be merged automatically.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u https://git.kolibrios.org/igorsh/kolibrios bcm57xx:igorsh-bcm57xx
git checkout igorsh-bcm57xx
Sign in to join this conversation.
Reviewers
No Reviewers
Burer
KolibriOS/system
KolibriOS/network
Labels
Clear labels
Influence/Text/TYPO
AI
Eolite
FS
GSoC
This issue or PR in the Google Source of Code program
 Good First PR
The issue is suitable to beginners
 HLL
HardwareTested
IRCC
Influence/Settings
Lang/C
Lang/FASM
Pay for the code
Paid task
 Subsystem/API
Subsystem/Audio
infinity service, audio drivers, midi, speacker, audio programs
 Subsystem/Graphics
vesa, vga, framebuffer, cursors, blitter, and video drivers
 Subsystem/IPC and events
pipes, signals, events, shared memory
 Subsystem/Memory
virt and phys memory allocators, malloc and other
 Subsystem/Network
Subsystem/Services(daemon)
userspace and kernel(for example: serial) services
 Subsystem/Taskmanager
process, threads, run apps, scheduler
 Subsystem/VFS
drivers from filesystem, fs api, blkdev, programs that work with the file system
 Subsystem/Window
windows, skins, buttons, mouse and keyboard code for windows (not the base code)
Category
Applications
Category
Drivers
Category
General
Category
Kernel
Category
Libraries
Kind
Breaking
Breaking change that won't be backward compatible
Kind
Bug
Something is not working
Kind
Build
Kind
Documentation
Documentation changes
Kind
Enhancement
Improve existing functionality
Kind
Feature
New functionality
Kind
Security
This is security issue
Kind
Testing
Issue or pull request related to testing
PR
Ready to merge
Pull request is ready for merge
PR
Conflicts
PR conflicts with main
PR
Dependent
This PR is dependent on another PR
PR
Request changes
Changes requested in pull request
PR
Review required
Priority
Critical
1
The priority is critical
Priority
High
2
The priority is high
Priority
Medium
3
The priority is medium
Priority
Low
4
The priority is low
Reviewed
Confirmed
Issue has been confirmed
Reviewed
Duplicate
This issue or pull request already exists
Reviewed
Invalid
Invalid issue
Reviewed
Won't Fix
This issue won't be fixed
Status
Abandoned
Somebody has started to work on this but abandoned work
Status
Blocked
Something is blocking this issue or pull request
Status
Need More Info
Feedback is required to reproduce issue or to continue work
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
ace-dent (Andrew) b00bl1k (Alexey Ryabov) bad_Dr3dd0x Burer Doczom (Mikhail Frolov) dunkaist (Ivan B) Egor00f (Егор Ярин) hidnplayr IgorA kolibrios-bot Bot mxlgv (Max Logaev) ramenu raTaHoa90 (Сергеев Данила) rgimad sdongles (Serhii Sakhno) Sweetbread (Gleb Zaharov)
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: KolibriOS/kolibrios#709