Re: [PATCH 0/9] net/sfc: SFN7xxx deprecation and Medford fixes

[Stephen Hemminger <stephen@networkplumber.org>]

On Tue, 30 Dec 2025 01:35:18 +0400
Ivan Malov <ivan.malov@arknetworks.am> 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 <ivan.malov@arknetworks.am> (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: stable@dpdk.org`? 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: stable@dpdk.org` tags to patch 0013 if X4 support was added in a prior release and this constitutes a fix for that support.