Skip to content

Make BCM84891-based RollBall SFP-10G-T modules work, and stop them costing 1.7 load average - #203

Open
meehien wants to merge 8 commits into
frank-w:7.1-mainfrom
meehien:bpi/sfp-rollball-bcm8489x
Open

Make BCM84891-based RollBall SFP-10G-T modules work, and stop them costing 1.7 load average#203
meehien wants to merge 8 commits into
frank-w:7.1-mainfrom
meehien:bpi/sfp-rollball-bcm8489x

Conversation

@meehien

@meehien meehien commented Jul 30, 2026

Copy link
Copy Markdown

Two problems with copper SFP+ modules on the R4/R4 Pro cages, addressed
together because they are the same modules.

They didn't come up. An "OEM SFP-10G-T" carrying a Broadcom BCM84891
reliably failed to attach. Three separate causes: the module leaves its
access-denial window before its identifier registers are stable, so the
probe reads zeros or a half-0xffff ID that matches no driver and lands the
SFP state machine in the terminal SFP_S_FAIL; the RollBall mailbox
intermittently misses its 200 ms completion budget, injecting 0xffff words
into whatever is being read; bcm8489x_config_init() rejected anything but
USXGMII, which is not how phylink attaches a module PHY; and the driver
declares .inband_caps without .config_inband, so phylink_major_config()
takes -EOPNOTSUPP as fatal and holds the link down forever with a
perfectly negotiated copper side.

They cost a lot of load. Every MDIO access on these modules goes through
the RollBall I2C mailbox, measured at ~377 ms per register read at steady
state (kprobe histogram, n=130, 371–384 ms). With msleep(20) before every
poll in TASK_UNINTERRUPTIBLE, plus 8 MMD reads per read_status, plus a
polled PCS blocking on phydev->lock, an otherwise 98%-idle box sat at ~1.7
load average. The last four patches take the steady-state poll from 8 reads
to 1, move the poll to TASK_IDLE with exponential backoff on a wall-clock
deadline, and skip ~2 s of ability discovery per attach for a family whose
abilities are constant.

The transport changes affect both cages — lsfp's AQR113C is on RollBall
too on this hardware, confirmed via /proc/<pid>/stack.

meehien added 8 commits July 30, 2026 20:25
@meehien
net: phy: sfp: retry PHY probe when c45 identifiers are implausible
03c1088 
RollBall SFP modules gate PHY register access for up to 25 seconds after
insertion, returning 0xffff for all reads.  get_phy_c45_ids() detects
this (devices-in-package reads as all-ones) and returns -ENODEV, which
the SFP state machine handles with its 25 x 1s retry loop
(R_PHY_RETRY / phy_t_retry set by sfp_fixup_rollball()).

Some of these modules (observed with an "OEM SFP-10G-T" containing a
Broadcom BCM84891) leave that window before their identifier registers
are stable, in two forms.  Devices-in-package reads a plausible value
while MII_PHYSID1/2 in all MMDs still read zero; or a RollBall command
that times out mid-probe returns 0xffff for one 16-bit half of an
identifier while the other half reads real data, giving a PMA ID of
0xffff5081 where the true value is 0x35905081.

get_phy_c45_ids() has no validity check for either case, so a phydev is
created with identifiers that no driver can match - phy_bus_match()
only skips 0xffffffff.  phy->drv is NULL, phylink_sfp_connect_phy()
rejects the PHY, and sfp_add_phy() fails with -EINVAL:

  mtk_soc_eth 15100000.ethernet lsfp: PHY i2c:sfp2:11 (id 0x00000000)
    has no driver loaded
  sfp sfp2: sfp_add_phy failed: -EINVAL

Unlike -ENODEV, this error path sends the state machine to SFP_S_FAIL,
which is terminal until the module is physically re-seated.

Validate the identifiers in sfp_sm_probe_phy(): an identifier is only
plausible if it is non-zero and neither 16-bit half reads 0xffff.  If no
MMD carries a plausible identifier, treat the PHY as not ready and
return -ENODEV so the existing retry logic applies.  Also log the c45
identifiers when sfp_add_phy() fails, so a genuinely unsupported PHY
model is diagnosable from dmesg.

A legitimate PHY ID with a 16-bit half of exactly 0xffff is not known to
exist; if one ever appears, it would only cause probe retries for that
module, diagnosable via the retry log message.

Signed-off-by: Mihai Ordean <research@mihaiordean.com>
@meehien
net: mdio: mdio-i2c: rollball: retry command on completion timeout
cd0b12e 
The RollBall MDIO-over-I2C protocol issues a command and polls the
command register for ROLLBALL_CMD_DONE, with a budget of 10 x 20 ms.
When the poll times out, i2c_mii_read_rollball() returns 0xffff --
indistinguishable from register data.

Some modules (observed: "OEM SFP-10G-T" with a Broadcom BCM84891)
intermittently take longer than the ~200 ms budget to complete a
command, well past the initial 25 s access-denial window. Each
timed-out read then injects a spurious 0xffff word into whatever the
caller is reading. During PHY probe this corrupts the C45 identifiers
(e.g. devs_in_pkg 0xc000ffff, PMA ID 0xffff5081 where the true values
are 0xc000009b and 0x35905081), so no PHY driver matches and
sfp_add_phy() fails with -EINVAL, leaving the SFP state machine in
SFP_S_FAIL until the module is re-seated. The same mechanism can
corrupt reads and silently drop writes at runtime after a successful
probe.

Re-issue the command up to 3 times when the completion poll times
out, in both the read and write paths. Re-issuing a read of the same
devad/reg is idempotent, so a late completion of a previous attempt
still yields data for the correct register. The 0xffff-on-timeout
semantic is preserved once retries are exhausted, keeping the
existing behaviour for the initial access-denial window (where
get_phy_c45_ids() maps all-ones to -ENODEV and the SFP state machine
retries the whole probe).

Signed-off-by: Mihai Ordean <research@mihaiordean.com>
@meehien
net: mdio-i2c: rollball: idle-sleep backoff poll within a wall-clock …
dc99f24 
…budget

i2c_rollball_mii_poll() slept msleep(20) before every read and capped the
wait with an iteration count. Two problems were tangled in that one loop:

  - Load: the sleeps are TASK_UNINTERRUPTIBLE, so the entire time spent
    waiting on the module's MCU counts toward the load average with the
    CPU idle. On an OEM SFP-10G-T (BCM84891) the MCU was measured taking
    ~377 ms per register read at steady state (kprobe delta histogram,
    n=130, tight cluster 371-384 ms), so one read_status (8 MMD reads)
    occupies ~3 s of every ~4 s phylib poll cycle: 0.75 load per module,
    1.5 with two modules, on an otherwise idle box.

  - Wake window: right after insertion the module's mailbox genuinely
    stalls for hundreds of ms, so the poll needs a generous budget there
    (this is what the previous iteration-count bump was really buying).

Poll on usleep_range_idle() sleeps with exponential backoff (2-4 ms
first, doubling to a 32-64 ms cap) bounded by a ~1 s wall-clock
deadline. Fast completions return in a couple of ms instead of 20. Slow
commands complete in a bounded number of iterations: a ~380 ms command
costs ~15 status polls (each four I2C transfers for the page
save/set/read/restore dance, plus an uninterruptible wait on the I2C
controller) versus ~100+ with a flat 2-4 ms interval, keeping the
residual D-state in the I2C core small. The cap is only reached ~30 ms
into a command, so fast modules never pay it. Wake-window reads still
get the same ~1 s budget as the previous iteration-count bump (combined
with the command-retry logic still ~3 s worst case).
usleep_range_idle() sleeps as TASK_IDLE, so the poll no longer inflates
the load average while it waits on hardware with the CPU free.

Supersedes the fixed iteration-count budget bump and the earlier
flat-interval idle-poll rework.

Signed-off-by: Mihai Ordean <research@mihaiordean.com>
@meehien
net: phy: bcm84881: accept BCM84891 attached over 10GBASE-R/SGMII/250…
a9c2713 
…0BASE-X

bcm8489x_config_init() rejects any attachment other than USXGMII with
-ENODEV, reflecting the board-soldered configuration it was written
for. The BCM84891 is however also the PHY inside various RollBall
"OEM SFP-10G-T" copper SFP+ modules, where phylink selects 10GBASE-R
as the host interface (with SGMII/2500BASE-X rate switching), exactly
as for the BCM84881. In that configuration a successful PHY probe
still fails at attach time:

  sfp sfp2: PHY c45 ids: PMA 0x35905081 PCS ... devs_in_pkg ...
  sfp sfp2: sfp_add_phy failed: -ENODEV

with the -ENODEV originating from phy_init_hw() ->
bcm8489x_config_init().

Accept the same interface trio as the BCM84881 in addition to
USXGMII, and populate possible_interfaces with all four so phylink
can rate-switch on copper linkdowns. The LPOWER clearing is kept
unconditional; it is harmless on module-hosted PHYs.

Signed-off-by: Mihai Ordean <research@mihaiordean.com>
@meehien
net: phy: bcm84881: implement config_inband
406a539 
The bcm84881 driver declares .inband_caps, unconditionally returning
LINK_INBAND_DISABLE, but does not implement .config_inband. When a
BCM84881-family PHY is used with a phylink MAC in inband mode (e.g.
any SFP module PHY, or managed = "in-band-status" in DT), phylink's
negotiation resolves to outband operation and sets
phy_ib_mode = LINK_INBAND_DISABLE, then calls phy_config_inband() to
apply it. With no .config_inband method this returns -EOPNOTSUPP,
which phylink_major_config() treats as a fatal configuration error:

  mtk_soc_eth 15100000.ethernet lsfp: phy_config_inband: -EOPNOTSUPP

major_config_failed then forces the link down in phylink_resolve()
and holds it down across reconfiguration attempts. The result is a
permanently dead link with a perfectly negotiated copper side
(observed with a BCM84891-based "OEM SFP-10G-T" module on mt7988:
copper AN completes at 10G, carrier never comes up).

Implement config_inband: the PHY does not generate inband signalling
in any mode, so it is permanently in the "disabled" state and a
request to disable inband succeeds as a no-op. Any other request
cannot be satisfied and returns -EINVAL (phylink will not request
one, given the capabilities this driver reports).

Signed-off-by: Mihai Ordean <research@mihaiordean.com>
@meehien
net: phy: bcm84881: skip re-reading negotiated parameters while link …
469eabb 
…is up

bcm84881_read_status() reads 8 MMD registers on every poll: three to
determine link/autoneg state, and five (LPA base page, 10GBT status,
C22 STAT1000, mdix, host interface mode) to resolve the negotiated
parameters. The latter five cannot change while the link is up: any
change requires a renegotiation, which is observable in the first
three reads as a link drop, an autoneg restart or autoneg-complete
deasserting.

On modules where every MDIO access goes through a RollBall
MDIO-over-I2C mailbox this matters a lot: an OEM SFP-10G-T (BCM84891)
was measured taking ~377 ms per register read at steady state, so one
read_status occupies ~3 s -- all of it holding phydev->lock. With the
MAC PCS in polled mode (pcs->poll, e.g. mtk_eth USXGMII), phylink's
1 Hz PCS poll then blocks in phylink_resolve() on phy->lock for up to
3 s in TASK_UNINTERRUPTIBLE on every tick, inflating the load average
by ~0.6 per port and delaying ethtool/link operations, while the
effective link poll rate drops to 0.25 Hz.

Cache the resolved state: when the previous poll left the link up and
autoneg complete, and the three state reads show it still is, return
with the previously resolved parameters intact. Steady state drops
from 8 reads to 3 (~1.1 s on the module above), and any renegotiation
still invalidates the cache on the very next poll.

Signed-off-by: Mihai Ordean <research@mihaiordean.com>
@meehien
net: phy: bcm84881: single latched-status read per poll while link is up
9ba63e8 
With the negotiated-parameter cache in place, bcm84881_read_status()
still performs three MMD reads per poll (AN CTRL1, AN STAT1, C22
shadow BMSR) just to confirm an unchanged link. Apply the same
polling-mode logic genphy_c45_read_link() uses: the AN link status
bit is latched low, so in polling mode a single read is authoritative
-- reading 1 means the link has been continuously up since the last
poll, and autoneg-complete deasserts across any renegotiation. When
the previous poll left the link up and resolved, read AN STAT1 alone
and return if it still shows link up and autoneg complete.

If the fast read shows anything else, fall through to the full
evaluation, reusing the already-read value: the latched status has
been consumed, and re-reading the register would miss a momentary
link drop that the pre-existing code reports. An autoneg restart
issued between polls drops the latched link status, so the CTRL1
restart check in the full path is still reached in that case.

On an OEM SFP-10G-T (BCM84891, RollBall MDIO-over-I2C, ~377 ms per
register read) this takes the steady-state poll from three reads
(~1.1 s) to one (~0.38 s), cutting the time phydev->lock is held per
phylib poll cycle from ~53% to ~27% and correspondingly the time
phylink_resolve() blocks on phy->lock under a polled PCS (pcs->poll).

Signed-off-by: Mihai Ordean <research@mihaiordean.com>
@meehien
net: phy: bcm84881: use static feature set for BCM8489x
63f3955 
bcm84881_get_features() calls genphy_c45_pma_read_abilities(), which
walks PMA STAT2/EXTABLE/10GBT ability registers to discover link mode
support. For the BCM8489x family these are constants, but on SFP
modules using the RollBall MDIO-over-I2C mailbox each register access
was measured at ~377 ms (OEM SFP-10G-T, BCM84891), so ability
discovery alone adds ~2 s to every PHY attach -- paid on every module
insertion and every interface up on top of an already slow probe.

Set the known feature set (100BASE-T half/full, 1000/2500/5000/
10000BASE-T full, autoneg -- matching what ability discovery reports
on this hardware) directly for the BCM84891/BCM84892 entries. EEE
abilities may vary with firmware, so those are still read from the
device. BCM84881 keeps the discovery path unchanged.

Signed-off-by: Mihai Ordean <research@mihaiordean.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@meehien