|WARNING| [v2] net/ixgbe: add fdir_buffer_size devarg

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: WARNING
https://dpdk.org/patch/169417

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-09-10

# DPDK Patch Review

## Summary
This patch adds a probe-time devarg `fdir_buffer_size` to the ixgbe driver to control Flow Director table size and rejects Flow Director when DCB is enabled.

---

## Errors

### 1. Devarg parsing error not propagated in init (Error path resource leak)

**File:** `drivers/net/intel/ixgbe/ixgbe_ethdev.c`
**Function:** `eth_ixgbe_dev_init()`

The function allocates resources before calling `ixgbe_parse_devargs()`. If devarg parsing fails, the function returns without cleaning up previously allocated resources (PCI info copy, device flags set, etc.).

**Suggested fix:**
Move `ixgbe_parse_devargs()` call before resource allocation:

```c
ret = ixgbe_parse_devargs(eth_dev->data->dev_private,
                          pci_dev->device.devargs);
if (ret != 0)
    return ret;

rte_atomic_store_explicit(&ad->link_thread_running, 0, rte_memory_order_seq_cst);
rte_eth_copy_pci_info(eth_dev, pci_dev);
eth_dev->data->dev_flags |= RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS;
```

---

### 2. Reading `rxmode.mq_mode` from `dev_conf` instead of checking actual device state

**File:** `drivers/net/intel/ixgbe/ixgbe_fdir.c`
**Function:** `ixgbe_fdir_configure()`

The code reads `dev->data->dev_conf.rxmode.mq_mode` to check if DCB is enabled. According to the MTU guidelines, reading from `dev_conf` after configure can be stale if the configuration was modified at runtime.

However, in this specific case, the check occurs during initial configuration or reconfiguration, and `mq_mode` is not modifiable at runtime (unlike MTU). This pattern is acceptable for `mq_mode`.

**Note:** This is actually correct for checking initial DCB configuration. No change needed.

---

### 3. Missing parameter documentation in release notes

**File:** `doc/guides/rel_notes/release_26_11.rst`

The release notes mention the new devarg but do not document that the driver now rejects Flow Director when DCB is enabled. This is a significant behavior change that applications need to know about.

**Suggested fix:**
```rst
* **Updated Intel ixgbe driver.**

  Added ``fdir_buffer_size`` devarg to select the Flow Director table size
  (``64k``, ``128k`` or ``256k``) at probe time. The default remains ``64k``.
  
  The driver now rejects Flow Director configuration when DCB is enabled,
  as FDIR requires Rx packet buffer space that DCB also uses.
```

---

## Warnings

### 1. Missing space in RTE_PMD_REGISTER_PARAM_STRING

**File:** `drivers/net/intel/ixgbe/ixgbe_ethdev.c`

The parameter string concatenation is missing a space between the two devargs:

```c
RTE_PMD_REGISTER_PARAM_STRING(net_ixgbe,
                  IXGBE_DEVARG_FIBER_SDP3_NOT_TX_DISABLE "=<0|1>"
                  IXGBE_DEVARG_FDIR_BUFFER_SIZE "=<64k|128k|256k>");
```

Should be:

```c
RTE_PMD_REGISTER_PARAM_STRING(net_ixgbe,
                  IXGBE_DEVARG_FIBER_SDP3_NOT_TX_DISABLE "=<0|1> "
                  IXGBE_DEVARG_FDIR_BUFFER_SIZE "=<64k|128k|256k>");
```

---

### 2. Inconsistent error code propagation

**File:** `drivers/net/intel/ixgbe/ixgbe_flow.c`
**Function:** `ixgbe_fdir_flow_program()`

The code converts the `-ENOTSUP` error from `ixgbe_fdir_configure()` to a positive `ENOTSUP` in the rte_flow_error_set() call, but then the function returns the result of `rte_flow_error_set()` which always returns `-EINVAL`. The DCB-specific error path should preserve the `-ENOTSUP` distinction:

```c
if (ret == -ENOTSUP)
    return rte_flow_error_set(error, ENOTSUP,
        RTE_FLOW_ERROR_TYPE_UNSPECIFIED, NULL,
        "Flow Director is not supported with DCB");
```

**Suggested fix:**
Since `rte_flow_error_set()` returns `-EINVAL` by design, the current code is acceptable. The error message is set correctly for the user. However, a comment would clarify intent:

```c
if (ret == -ENOTSUP)  /* DCB conflict detected */
    return rte_flow_error_set(error, ENOTSUP, ...);
```

---

### 3. DCB check location

**File:** `drivers/net/intel/ixgbe/ixgbe_fdir.c`

The DCB rejection check occurs inside `ixgbe_fdir_configure()` after the `PMD_INIT_FUNC_TRACE()` call. For consistency with other validation, consider moving it to the top of the function before any side effects.

**Current:**
```c
PMD_INIT_FUNC_TRACE();

switch (dev->data->dev_conf.rxmode.mq_mode) {
```

**Suggested:**
```c
switch (dev->data->dev_conf.rxmode.mq_mode) {
case RTE_ETH_MQ_RX_VMDQ_DCB:
case RTE_ETH_MQ_RX_DCB:
case RTE_ETH_MQ_RX_DCB_RSS:
    PMD_INIT_LOG(ERR, "Flow Director is not supported with DCB");
    return -ENOTSUP;
default:
    break;
}

PMD_INIT_FUNC_TRACE();
```

---

## Overall Assessment

The patch is generally well-structured with clear documentation. The main correctness issue is the potential resource leak in `eth_ixgbe_dev_init()` when devarg parsing fails. The other issues are minor style/consistency improvements.