Re: [PATCH 0/2] ethdev: fix SFF-8472 external Rx power calibration

[Stephen Hemminger <stephen@networkplumber.org>]

On Tue, 29 Sep 2026 19:22:08 +0200
Roman Khromenok <roma55592@yandex.ru> wrote:

> The SFF-8472 decoder does not apply the external Rx power calibration
> as defined by the specification: the fourth order polynomial is reduced
> to RX_PWR(0) + x * (RX_PWR(1) + RX_PWR(2) + RX_PWR(3)) and RX_PWR(4)
> is never used. The code came from ethtool sfpdiag.c.
> 
> This was pointed out by Stephen during the review of the module EEPROM
> decoding series; this series is based on it (dpdk-next-net).
> 
> Patch 1 fixes the formula and is intended for stable.
> It also clamps the result to the 16-bit range, as the polynomial
> easily overflows it with unexpected coefficients and converting
> an out of range double to an integer is undefined behavior.
> Patch 2 adds a unit test with all coefficients set.
> 
> Note: the calibration formula 1 (Tx bias, Tx power, temperature,
> voltage) has the same out of range conversion issue; it is left
> for a separate patch.
> 
> Depends-on: series-39436 ("ethdev: add API to decode module EEPROM")
> 
> Roman Khromenok (2):
>   ethdev: fix SFF-8472 external Rx power calibration
>   test: check SFF-8472 external Rx power calibration
> 
>  app/test/test_ethdev_module_eeprom.c | 37 ++++++++++++++++++++++++++++
>  lib/ethdev/sff_8472.c                | 27 +++++++++++++-------
>  2 files changed, 55 insertions(+), 9 deletions(-)
> 

Looks ok as is, applied to next-net.

Detailed AI review had a bunch of feedback about range checking.

Review: [PATCH 0/2] ethdev: fix SFF-8472 external Rx power calibration

Patch 1 applies to main on its own and builds with -Dwerror=true, so
it backports as is. Patch 2 needs test_ethdev_module_eeprom.c from
the module EEPROM decoding API series.

The test input decodes to "0.1560 mW / -8.07 dBm" with the fix and
"0.0017 mW / -27.70 dBm" without it, so the test catches the bug.


Patch 1/2 ethdev: fix SFF-8472 external Rx power calibration

Info
- The final conversion truncates:

      sd->rx_power[i] = rx_power;

  Coefficients that are not exact in binary land just below the
  integer. RX_PWR(1) = 0.7 is stored as 0.69999999, so x = 1000
  decodes to 699 (0.0699 mW) instead of 700. rx_power is known
  positive in that branch, so round instead:

      sd->rx_power[i] = rx_power + 0.5;

  The UINT16_MAX check still holds, anything below 65535 rounds to
  at most 65535.

- Pre-existing, not introduced by this patch: formula 1 just above
  has the same out of range float to integer conversion the commit
  message describes.

      sd->bias_cur[i]    *= A2_OFFSET_TO_SLP(SFF_A2_CAL_TXI_SLP);

  The slope is a double up to 255.996, so a large reading leaves
  the uint16_t range. Same for tx_power and sfp_voltage, and for
  sfp_temp (int16_t) in both directions. Worth a follow up patch
  with the same clamping.


Patch 2/2 test: check SFF-8472 external Rx power calibration

Info
- The test covers the polynomial but neither clamp branch. Two more
  cases would cover them:

    x = 10, RX_PWR(1) = -1.0     -> "0.0000 mW / -inf dBm"
    x = 65535, RX_PWR(4) = 1.0   -> "6.5535 mW / 8.16 dBm"