On Thu, 15 Jan 2026 15:40:34 +0100 [email protected] wrote: > From: Martin Spinler <[email protected]> > > This patchset mainly cleans up the code and prepare it for another > quite large rework. Also it resolves some unpleasant behavior. > > --- > v2: > * Fixed coding style issues > > Martin Spinler (6): > net/nfb: use constant values for max Rx/Tx queues count > net/nfb: fix bad pointer access in queue stats > net/nfb: update timestamp calculation to meaningful value > net/nfb: use process private variable for internal data > net/nfb: release allocated resources correctly > net/nfb: stop only started queues in fail path > > doc/guides/nics/nfb.rst | 6 +- > drivers/net/nfb/nfb.h | 5 ++ > drivers/net/nfb/nfb_ethdev.c | 165 ++++++++++++++++++++--------------- > drivers/net/nfb/nfb_rx.c | 2 +- > drivers/net/nfb/nfb_rx.h | 13 +-- > drivers/net/nfb/nfb_rxmode.c | 12 +-- > drivers/net/nfb/nfb_stats.c | 46 +++++----- > drivers/net/nfb/nfb_tx.c | 2 +- > 8 files changed, 142 insertions(+), 109 deletions(-) > > -- > 2.52.0 >
Looks ok but AI code review spotted somethings that need to be addressed. --- ## DPDK NFB Driver Patchset Review **Series:** net/nfb cleanup (6 patches, v2) **Submitter:** Martin Spinler <[email protected]> --- ### Patch 1/6: `net/nfb: use constant values for max Rx/Tx queues count` | Check | Status | Notes | |-------|--------|-------| | Subject length | ✅ PASS | 51 chars (≤60) | | Subject format | ✅ PASS | Lowercase after colon, no trailing period | | Component prefix | ✅ PASS | `net/nfb:` is correct | | Signed-off-by | ✅ PASS | Present with valid name/email | | Fixes tag | ✅ PASS | `Fixes: 6435f9a0ac22` (12-char SHA) | | Cc: stable | ✅ PASS | Present | | Tag order | ✅ PASS | Fixes before Cc, blank line before Signed-off-by | | Body doesn't start with "It" | ✅ PASS | Starts with "The" | **Code Review:** - Clean addition of `max_rx_queues` and `max_tx_queues` to `pmd_internals` structure - Proper initialization from hardware counts instead of mutable `nb_*_queues` **Issue (Warning):** The log message at line 543 still references `data->nb_rx_queues` / `data->nb_tx_queues` which will be 0 at that point since they're no longer initialized. Should log `internals->max_rx_queues` and `internals->max_tx_queues` instead. --- ### Patch 2/6: `net/nfb: fix bad pointer access in queue stats` | Check | Status | Notes | |-------|--------|-------| | Subject length | ✅ PASS | 47 chars | | Subject format | ✅ PASS | Correct format | | Signed-off-by | ✅ PASS | Present | | Fixes tag | ✅ PASS | Present with 12-char SHA | | Cc: stable | ✅ PASS | Present | | Body doesn't start with "It" | ✅ PASS | Starts with "The" | **Code Review:** - Correctly fixes the pointer dereference pattern from `rx_queue[i].rx_pkts` to `rx_queue->rx_pkts` - The original code treated an array of pointers as an array of structures — this is a legitimate bug fix **Issue (Warning):** Still missing NULL pointer checks before dereferencing `dev->data->rx_queues[i]` and `dev->data->tx_queues[i]`. The commit message mentions the pointer can be NULL, but the fix doesn't add the NULL check. --- ### Patch 3/6: `net/nfb: update timestamp calculation to meaningful value` | Check | Status | Notes | |-------|--------|-------| | Subject length | ✅ PASS | 55 chars | | Subject format | ✅ PASS | Correct format | | Signed-off-by | ✅ PASS | Present | | Fixes tag | ⚠️ **MISSING** | This is a bug fix (non-monotonic timestamp) | | Cc: stable | ⚠️ **MISSING** | Should be present for bug fixes | | Body doesn't start with "It" | ✅ PASS | Starts with "The" | | Doc + code atomic | ✅ PASS | Both `nfb.rst` and `nfb_rx.h` updated together | **Recommendation:** Add: ``` Fixes: 6435f9a0ac22 ("net/nfb: add new netcope driver") Cc: [email protected] ``` --- ### Patch 4/6: `net/nfb: use process private variable for internal data` | Check | Status | Notes | |-------|--------|-------| | Subject length | ✅ PASS | 56 chars | | Subject format | ✅ PASS | Correct format | | Signed-off-by | ✅ PASS | Present | | Fixes tag | ⚠️ **MISSING** | This fixes multi-process support | | Cc: stable | ⚠️ **MISSING** | Should be present for fixes | **Code Issues:** 1. **Error (Style):** Line 148 changes a Doxygen comment to broken format: ```c -/** +/* * ``` This breaks Doxygen documentation parsing. Should remain `/**` or be changed to `/*` (without the space after asterisk). 2. **Warning:** The new `struct pmd_priv` only contains `max_rx_queues` and `max_tx_queues` as `int` types. These should likely be `uint16_t` to match the DPDK convention for queue counts. --- ### Patch 5/6: `net/nfb: release allocated resources correctly` | Check | Status | Notes | |-------|--------|-------| | Subject length | ✅ PASS | 49 chars | | Subject format | ✅ PASS | Correct format | | Signed-off-by | ✅ PASS | Present | | Fixes tag | ⚠️ **MISSING** | This fixes resource leaks | | Cc: stable | ⚠️ **MISSING** | Should be present | **Code Review:** - Good refactoring of cleanup paths using `goto` error handling - Properly separates primary vs secondary process cleanup **Issue (Warning):** Forward declaration added at line 21: ```c static int nfb_eth_dev_uninit(struct rte_eth_dev *dev); ``` This is fine, but the blank line before it creates two consecutive blank lines which is non-standard. --- ### Patch 6/6: `net/nfb: stop only started queues in fail path` | Check | Status | Notes | |-------|--------|-------| | Subject length | ✅ PASS | 47 chars | | Subject format | ✅ PASS | Correct format | | Signed-off-by | ✅ PASS | Present | | Fixes tag | ⚠️ **MISSING** | This is a bug fix | | Cc: stable | ⚠️ **MISSING** | Should be present | **Code Review:** - Correctly uses reverse iteration to only stop queues that were successfully started - Loop logic is correct: `for (; i > 0; i--)` then `stop(dev, i - 1)` --- ## Summary ### Errors (Must Fix) | Patch | Issue | |-------|-------| | 4/6 | Broken Doxygen comment: `/* *` should be `/**` or `/*` | ### Warnings (Should Fix) | Patch | Issue | |-------|-------| | 1/6 | Log message references uninitialized `data->nb_*_queues` | | 2/6 | Missing NULL checks for queue pointers (mentioned in commit but not implemented) | | 3/6 | Missing `Fixes:` tag and `Cc: [email protected]` | | 4/6 | Missing `Fixes:` tag and `Cc: [email protected]`; consider `uint16_t` for queue counts | | 5/6 | Missing `Fixes:` tag and `Cc: [email protected]`; extra blank line before forward declaration | | 6/6 | Missing `Fixes:` tag and `Cc: [email protected]` | ### Info (Consider) - The series is well-structured with logical patch ordering - Good that patches 1 and 2 have proper `Fixes:` tags — patches 3-6 should follow the same pattern - Documentation is updated atomically with code in patch 3 ### Verdict **Needs revision.** The main blocker is the broken Doxygen comment in patch 4/6. Additionally, patches 3-6 should have `Fixes:` tags and `Cc: [email protected]` since they all fix bugs in the original driver.
