On Tue, 30 Dec 2025 01:35:18 +0400 Ivan Malov <[email protected]> wrote:
> The series provides various fixes, primarily for Medford4 support. > It also fixes a legacy flow control bug; due to hardware constraints > in the fix, SFN7xxx NICs are now marked as deprecated. > > Andy Moreton (1): > common/sfc_efx/base: count Rx TRUNC ERR as CRC errors on X4 > > Ivan Malov (8): > doc: remove support for AMD Solarflare SFN7xxx family boards > common/sfc_efx/base: fix flow control setting on legacy MCDI > common/sfc_efx/base: fix indication of requestable FEC flags > common/sfc_efx/base: define mask for pause mode capabilities > net/sfc: avoid speed reset when setting FEC in started state > net/sfc: rework the capability check that is done on FEC set > net/sfc: drop AUTO from FEC capabilities and fix the comment > net/sfc: fix reporting status of autonegotiation to the user > > doc/guides/nics/sfc_efx.rst | 14 --- > doc/guides/rel_notes/release_26_03.rst | 4 + > drivers/common/sfc_efx/base/ef10_ev.c | 41 +++++-- > drivers/common/sfc_efx/base/ef10_mac.c | 35 ++++-- > drivers/common/sfc_efx/base/ef10_nic.c | 57 +++++----- > drivers/common/sfc_efx/base/efx.h | 1 + > drivers/common/sfc_efx/base/efx_np.c | 2 +- > drivers/net/sfc/sfc.h | 2 +- > drivers/net/sfc/sfc_ethdev.c | 141 +++++++++++++++---------- > drivers/net/sfc/sfc_ev.c | 7 +- > drivers/net/sfc/sfc_port.c | 8 +- > drivers/net/sfc/sfc_repr.c | 2 +- > 12 files changed, 193 insertions(+), 121 deletions(-) > AI code review spotted one minor thing but was overall quite happy. # DPDK Solarflare Driver Patch Series Review **Patches Reviewed:** 0009-0017 (9 patches) **Author:** Ivan Malov <[email protected]> (with contribution from Andy Moreton) **Review Date:** January 13, 2026 --- ## Overall Assessment This patch series is **generally well-prepared** and follows DPDK coding standards. The patches address a mix of documentation updates, bug fixes in the sfc_efx base library, and improvements to FEC/flow control handling in the net/sfc driver. Most patches are suitable for merging with only minor observations. --- ## Patch-by-Patch Review ### Patch 0009: doc: remove support for AMD Solarflare SFN7xxx family boards **Status:** ✅ Ready | Check | Result | |-------|--------| | Subject ≤60 chars | ✅ 59 characters | | Subject format | ✅ Correct `doc:` prefix, lowercase | | Body format | ✅ Doesn't start with "It", proper wrapping | | Signed-off-by | ✅ Present | | Release notes | ✅ Updated in `release_26_03.rst` | **Observations:** Clean documentation removal patch. Properly updates both the NIC guide and release notes. --- ### Patch 0010: common/sfc_efx/base: fix flow control setting on legacy MCDI **Status:** ✅ Ready | Check | Result | |-------|--------| | Subject ≤60 chars | ✅ 60 characters (at limit) | | Subject format | ✅ Correct prefix, lowercase | | Fixes tag | ✅ `e7cd430c864f` with correct format | | Cc: stable | ✅ Present | | Tag order | ✅ Fixes, Cc, blank line, Signed-off-by, Reviewed-by | **Code Review:** - Uses explicit comparison `!= B_FALSE` (line 46) — acceptable, though `== B_TRUE` might be slightly clearer - Properly adds new fail label and updates error handling - Switch statement formatting is correct --- ### Patch 0011: common/sfc_efx/base: fix indication of requestable FEC flags **Status:** ✅ Ready | Check | Result | |-------|--------| | Subject ≤60 chars | ✅ 59 characters | | Fixes tag | ✅ `a90549f527eb` present | | Cc: stable | ✅ Present | | Tag order | ✅ Correct | **Code Review:** - Refactors code to move FEC capability bits initialization to `ef10_nic_probe()` - Clean removal of `goto skip_phy_props` pattern - Uses explicit comparison `== B_FALSE` --- ### Patch 0012: common/sfc_efx/base: define mask for pause mode capabilities **Status:** ✅ Ready | Check | Result | |-------|--------| | Subject ≤60 chars | ✅ 58 characters | | Subject format | ✅ Correct | | Signed-off-by | ✅ Present | **Observations:** Simple macro addition. No Fixes tag is appropriate since this is a new feature/enhancement required by subsequent patches. --- ### Patch 0013: common/sfc_efx/base: count Rx TRUNC ERR as CRC errors on X4 **Status:** ⚠️ Minor Question | Check | Result | |-------|--------| | Subject ≤60 chars | ✅ 57 characters | | Subject format | ✅ Uses correct `Rx` capitalization | | Signed-off-by | ✅ Two signers (appropriate for co-authored work) | **Observations:** - **Question:** Should this have a `Fixes:` tag and `Cc: [email protected]`? The commit message describes a workaround for X4 hardware behavior, which could be considered a bug fix for X4 support. If the X4 family was added in a specific commit, a Fixes tag referencing that commit would be appropriate. - Code correctly duplicates the X4-specific handling in both `ef10_ev_rx_packed_stream` and `ef10_ev_rx` functions --- ### Patch 0014: net/sfc: avoid speed reset when setting FEC in started state **Status:** ✅ Ready | Check | Result | |-------|--------| | Subject ≤60 chars | ✅ 58 characters | | Fixes tag | ✅ `5efa8fc1cfc0` present | | Cc: stable | ✅ Present | | Dependencies | ✅ Uses `EFX_PHY_CAP_PAUSE_MASK` from patch 0012 | **Code Review:** - Correctly preserves pause capabilities while updating FEC settings - Proper dependency ordering within the series --- ### Patch 0015: net/sfc: rework the capability check that is done on FEC set **Status:** ✅ Ready | Check | Result | |-------|--------| | Subject ≤60 chars | ✅ 58 characters | | Fixes tag | ✅ `5efa8fc1cfc0` present | | Cc: stable | ✅ Present | | New includes | ✅ `<stdbool.h>` and `<rte_bitops.h>` added | **Code Review:** - Uses `bool` type appropriately - `if (!supported)` at line 156 is acceptable for actual bool types per DPDK guidelines - Uses `rte_popcount32()` — good use of DPDK utility function - Commit message includes helpful testpmd reproduction steps --- ### Patch 0016: net/sfc: drop AUTO from FEC capabilities and fix the comment **Status:** ✅ Ready | Check | Result | |-------|--------| | Subject ≤60 chars | ✅ 58 characters | | Fixes tags | ✅ Two Fixes tags (both appropriate) | | Cc: stable | ✅ Present | | Tag order | ✅ Correct | **Observations:** - Multiple Fixes tags are appropriate when a single patch addresses regressions from multiple commits - Updated comments improve code clarity --- ### Patch 0017: net/sfc: fix reporting status of autonegotiation to the user **Status:** ✅ Ready | Check | Result | |-------|--------| | Subject ≤60 chars | ✅ 58 characters | | Fixes tag | ✅ `886f8d8a05bf` present | | Cc: stable | ✅ Present | **Code Review:** - Function signature change is propagated to all call sites - Uses explicit comparison `!= 0` (good practice) - Comment in `sfc_ev.c` appropriately documents a potential race condition and explains the trade-off --- ## Summary | Severity | Count | Details | |----------|-------|---------| | **Error** | 0 | None found | | **Warning** | 1 | Patch 0013 may benefit from Fixes/Cc:stable tags | | **Info** | 0 | — | ### Series Dependencies The patches have proper internal dependencies: - Patch 0014 depends on patch 0012 (`EFX_PHY_CAP_PAUSE_MASK`) - The series should be applied in order ### Recommendation **The series is suitable for merging.** Consider adding `Fixes:` and `Cc: [email protected]` tags to patch 0013 if X4 support was added in a prior release and this constitutes a fix for that support.
