On Mon, 12 Jan 2026 11:24:34 +0200 Maayan Kashani <[email protected]> wrote:
> This patch series contains bug fixes for the mlx5 PMD, primarily > addressing issues with Hardware Steering (HWS) and flow devarg handling. > > Summary of changes: > > 1. drivers: fix flow devarg handling for future HW > Addresses SWS (Software Steering) deprecation on future hardware > generations (e.g., ConnectX-9). Updates default behavior for > dv_flow_en and allow_duplicate_pattern devargs based on device > capabilities, with proper error handling and user feedback. > > 2. net/mlx5: fix default memzone requirements in HWS > Fixes memzone exhaustion when probing setups with ~1K SFs. The > default HWS sync flow API configuration was allocating unnecessary > rings (flow_transfer_pending/completed) that are only used with > async flow API. This patch removes the unnecessary allocations to > stay within memzone limits. > > 3. net/mlx5: fix internal HWS pattern template creation > Improves PMD initialization time by separating pattern templates > into internal and external categories. Internal templates (created > by PMD) skip expensive validations, while application-provided > templates remain fully validated. > > 4. net/mlx5: fix redundant control rules in promiscuous mode > Removes redundant DMAC and multicast/broadcast control flow rules > when promiscuous mode is enabled, as the device already receives > all traffic in this mode. > > All patches are targeted for stable backport. > > Dariusz Sosnowski (1): > net/mlx5: fix default memzone requirements in HWS > > Maayan Kashani (3): > drivers: fix flow devarg handling for future HW > net/mlx5: fix internal HWS pattern template creation > net/mlx5: fix redundant control rules in promiscuous mode > > doc/guides/nics/mlx5.rst | 11 ++- > drivers/common/mlx5/mlx5_devx_cmds.c | 18 ++++ > drivers/common/mlx5/mlx5_devx_cmds.h | 6 ++ > drivers/common/mlx5/mlx5_prm.h | 14 +++- > drivers/net/mlx5/mlx5.c | 71 +++++++++++++++- > drivers/net/mlx5/mlx5_flow_hw.c | 121 ++++++++++++++++++++------- > drivers/net/mlx5/mlx5_trigger.c | 16 ++-- > 7 files changed, 214 insertions(+), 43 deletions(-) > It looks ok to me, AI code review had some useful feedback. Also, it looks like AGENTS.md doesn't really know what is best for declaration placement. # DPDK Patch Review: mlx5 devargs Series **Series**: [PATCH 1/4] - [PATCH 4/4] mlx5 devargs and flow handling fixes **Submitter**: Maayan Kashani <[email protected]> **Date**: Mon, 12 Jan 2026 **Reviewed against**: AGENTS.md criteria --- ## Series Overview This 4-patch series addresses flow devarg handling for future NVIDIA hardware generations where SWS (Software Steering) is disabled. The patches update defaults, fix memory usage issues, optimize initialization performance, and correct redundant control flow rules. --- ## Patch 1/4: drivers: fix flow devarg handling for future HW ### Commit Message Analysis | Criterion | Status | Notes | |-----------|--------|-------| | Subject ≤60 chars | ✅ PASS | 50 chars: "drivers: fix flow devarg handling for future HW" | | Lowercase after colon | ✅ PASS | "fix flow devarg..." | | Imperative mood | ✅ PASS | "fix" | | No trailing period | ✅ PASS | | | Component prefix | ⚠️ WARNING | Uses "drivers:" but changes span `common/mlx5` and `net/mlx5`. Consider using `common/mlx5` or splitting. | | Body wrap ≤75 chars | ✅ PASS | All lines within limit | | Body doesn't start with "It" | ✅ PASS | Starts with "SWS (software steering)..." | | Signed-off-by present | ✅ PASS | `Signed-off-by: Maayan Kashani <[email protected]>` | | Fixes tag format | ✅ PASS | `Fixes: 1b55eeb7b76f ("common/mlx5: add ConnectX-9 SuperNIC")` - 12-char SHA | | Cc: [email protected] | ✅ PASS | Present | | Tag order | ✅ PASS | Fixes → Cc → blank → Signed-off-by → Acked-by | ### Code Analysis **New functions added:** ```c static bool mlx5_hws_is_supported(struct mlx5_dev_ctx_shared *sh) static bool mlx5_sws_is_any_supported(struct mlx5_dev_ctx_shared *sh) static bool mlx5_kvargs_is_used(struct mlx5_kvargs_ctrl *mkvlist, const char *key) ``` | Criterion | Status | Notes | |-----------|--------|-------| | Line length ≤100 chars | ⚠️ WARNING | Line 295: `if (hca_attr->eswitch_manager && (hca_attr->esw_sw_owner_v2 || hca_attr->esw_sw_owner))` is ~95 chars - acceptable but borderline | | Explicit comparisons | ✅ PASS | Uses `== 0`, `== 1`, `== 2`, `!= NULL` appropriately | | No forbidden tokens | ✅ PASS | | | Naming conventions | ✅ PASS | Functions use lowercase with underscores | | Comment style | ✅ PASS | Multi-line comments follow C style | **Style Issues:** | Issue | Severity | Location | Description | |-------|----------|----------|-------------| | Mixed declaration and code | ⚠️ WARNING | Line 358 | `bool allow_dup_pattern_set = ...` declared mid-block after statements. C90 style prefers declarations at block start. | | Duplicate logic | ℹ️ INFO | Lines 338-341 vs 360-367 | The `allow_duplicate_pattern` handling appears in two places with slightly different logic. Consider consolidating. | **Documentation:** | Criterion | Status | Notes | |-----------|--------|-------| | doc/guides updated | ✅ PASS | Updates `doc/guides/nics/mlx5.rst` for `dv_flow_en` and `allow_duplicate_pattern` | | Docs match code | ✅ PASS | Documentation accurately reflects the new default behavior | ### Patch 1 Verdict: **ACCEPTABLE WITH MINOR ISSUES** - Consider using `net/mlx5:` prefix since that's the primary subsystem being modified - Minor C90 style preference for declarations at block start --- ## Patch 2/4: net/mlx5: fix default memzone requirements in HWS ### Commit Message Analysis | Criterion | Status | Notes | |-----------|--------|-------| | Subject ≤60 chars | ✅ PASS | 51 chars: "net/mlx5: fix default memzone requirements in HWS" | | Component prefix | ✅ PASS | `net/mlx5:` correct | | Lowercase after colon | ✅ PASS | | | Imperative mood | ✅ PASS | "fix" | | Body wrap ≤75 chars | ✅ PASS | | | Signed-off-by | ✅ PASS | | | Fixes tag | ✅ PASS | `Fixes: 27d171b88031 ("net/mlx5: abstract flow action and enable reconfigure")` | | Cc: stable | ✅ PASS | | | Tag order | ✅ PASS | | | References commit | ✅ PASS | Uses `[1] commit d1ac7b6c64d9` notation correctly | **Author note**: Patch is `From: Dariusz Sosnowski` but submitted by Maayan Kashani - this is fine for series coordination. ### Code Analysis **New function:** ```c static int flow_hw_queue_setup_rings(struct rte_eth_dev *dev, uint16_t queue, uint32_t queue_size, bool nt_mode) ``` | Criterion | Status | Notes | |-----------|--------|-------| | Line length ≤100 chars | ✅ PASS | | | NULL checks | ✅ PASS | Uses `== NULL` and `!= NULL` explicitly | | Error handling | ✅ PASS | Returns -ENOMEM on allocation failures | | Assertion usage | ✅ PASS | Uses `MLX5_ASSERT()` appropriately | | Refactoring | ✅ PASS | Good refactoring - extracts ring creation to separate function | **Logic review:** ```c if (ring == NULL) return 0; ``` This early return in `mlx5_hw_pull_flow_transfer_comp()` is safe since rings may not be allocated in sync flow API mode. ```c if (hw_q->flow_transfer_pending != NULL && hw_q->flow_transfer_completed != NULL) mlx5_hw_push_queue(...) ``` Appropriate guard for optional rings. ### Patch 2 Verdict: **GOOD** Clean, well-structured fix that reduces memzone usage significantly for SF-heavy deployments. --- ## Patch 3/4: net/mlx5: skip internal pattern template validation ### Commit Message Analysis | Criterion | Status | Notes | |-----------|--------|-------| | Subject ≤60 chars | ✅ PASS | 53 chars | | Component prefix | ✅ PASS | `net/mlx5:` | | Lowercase | ✅ PASS | | | Signed-off-by | ✅ PASS | Two authors: Gregory Etelson and Maayan Kashani | | Fixes tag | ✅ PASS | `Fixes: a190f25e6a93 ("net/mlx5: improve pattern template validation")` | | Cc: stable | ✅ PASS | | ### Code Analysis | Criterion | Status | Notes | |-----------|--------|-------| | Function signature change | ✅ PASS | Added `bool external` parameter | | Wrapper function | ✅ PASS | `flow_hw_external_pattern_template_create()` wraps with `true` | | Internal calls | ✅ PASS | All internal calls use `false` | | Line length | ✅ PASS | | **Style issue:** | Issue | Severity | Location | Description | |-------|----------|----------|-------------| | Extra blank line | ⚠️ WARNING | Line 925-926 | Double blank line before `const struct mlx5_flow_driver_ops` | The optimization approach is sound - internal PMD-created templates are known-safe and don't need expensive validation. ### Patch 3 Verdict: **GOOD** Minor style nit with extra blank line. --- ## Patch 4/4: net/mlx5: fix redundant control rules in promiscuous mode ### Commit Message Analysis | Criterion | Status | Notes | |-----------|--------|-------| | Subject ≤60 chars | ✅ PASS | 58 chars | | Component prefix | ✅ PASS | `net/mlx5:` | | Imperative mood | ✅ PASS | "fix" | | Body doesn't start with "It" | ⚠️ WARNING | Body starts with "When promiscuous..." but third paragraph starts "This patch makes..." which is discouraged phrasing | | Signed-off-by | ✅ PASS | | | Fixes tag | ✅ PASS | `Fixes: 9fa7c1cddb85 ("net/mlx5: create control flow rules with HWS")` | | Cc: stable | ✅ PASS | | ### Code Analysis | Criterion | Status | Notes | |-----------|--------|-------| | Logic correctness | ✅ PASS | Correctly makes DMAC/multicast rules conditional on non-promiscuous mode | | Brace style | ✅ PASS | Correct K&R style | | Flag handling | ✅ PASS | Clean bitwise OR operations | **Before:** ```c if (dev->data->promiscuous) flags |= MLX5_CTRL_PROMISCUOUS; if (dev->data->all_multicast) flags |= MLX5_CTRL_ALL_MULTICAST; else flags |= MLX5_CTRL_BROADCAST | ...; flags |= MLX5_CTRL_DMAC; ``` **After:** ```c if (dev->data->promiscuous) { flags |= MLX5_CTRL_PROMISCUOUS; } else { if (dev->data->all_multicast) flags |= MLX5_CTRL_ALL_MULTICAST; else flags |= (MLX5_CTRL_BROADCAST | ...); flags |= MLX5_CTRL_DMAC; } ``` The refactored logic correctly avoids setting redundant DMAC and multicast rules when promiscuous mode already accepts all traffic. ### Patch 4 Verdict: **GOOD** --- ## Series-Level Analysis ### Compilation Independence Each patch should compile independently. The series has proper ordering: 1. Patch 1 adds capability detection and devarg handling 2. Patch 2 depends on HWS mode changes from patch 1 3. Patch 3 is independent (optimization) 4. Patch 4 is independent (bug fix) ⚠️ **Potential issue**: Patch 2 references `d1ac7b6c64d9` which appears to be a different commit than patch 1's `1b55eeb7b76f`. The series should be self-contained or clearly document the dependency. ### ABI Considerations | Change | Impact | |--------|--------| | New fields in `mlx5_hca_attr` | Internal structure, no ABI impact | | New static functions | No ABI impact | | Function signature change (`flow_hw_pattern_template_create`) | Static function, no ABI impact | ### Documentation Completeness | Criterion | Status | |-----------|--------| | Release notes needed? | ⚠️ CONSIDER | These are bug fixes, but the behavioral changes (default dv_flow_en) may warrant release note mention | | API documentation | ✅ N/A | No public API changes | --- ## Summary | Patch | Verdict | Blocking Issues | |-------|---------|-----------------| | 1/4 | ✅ Acceptable | Component prefix could be more specific | | 2/4 | ✅ Good | None | | 3/4 | ✅ Good | Extra blank line (minor) | | 4/4 | ✅ Good | None | ### Errors (Must Fix) None ### Warnings (Should Fix) 1. **Patch 1**: Consider `net/mlx5:` prefix instead of generic `drivers:` 2. **Patch 1**: Mixed declaration and code (C90 style) 3. **Patch 3**: Extra blank line at line 925 ### Info (Consider) 1. **Patch 1**: Logic for `allow_duplicate_pattern` appears in two locations 2. **Series**: Consider adding release notes entry for the default behavior change 3. **Patch 2**: Commit reference `[1]` should ideally use Fixes: format or be clear it's not in this series --- ## Recommendation **Series is ready for merge** with optional minor cleanups. The fixes address real issues for future hardware support and scalability with large numbers of SFs. Code quality is good and follows DPDK conventions.
