|WARNING| net/ixgbe: fix repeated Rx packet buffer shrink for FDIR

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Summary
This patch fixes a bug where RXPBSIZE (Rx packet buffer size) is repeatedly reduced when Flow Director rules are added/removed. The core issue is that clearing the last FDIR rule reset the mode to NONE, causing the next flow create to subtract the buffer size again.

## Errors

**1. Missing initialization in hardware reset path**

The patch sets `info->rx_pb_reduced = false` in `ixgbe_dev_start()` but does not initialize it in `ixgbe_dev_reset()`. After a device reset (via `rte_eth_dev_reset()`), the flag would retain its old value while the hardware state has been cleared, causing the buffer reduction to be skipped incorrectly.

**Suggested fix:**
```c
/* In ixgbe_dev_reset() after calling ixgbe_dev_stop() */
struct ixgbe_hw_fdir_info *info =
    IXGBE_DEV_PRIVATE_TO_FDIR_INFO(adapter);
info->rx_pb_reduced = false;
```

**2. Resource leak on FDIR configure failure**

In `ixgbe_dev_start()`, if `ixgbe_fdir_configure()` fails after successfully reducing RXPBSIZE (setting `info->rx_pb_reduced = true`), the function jumps to `error:` label. The RXPBSIZE register is not restored, but the device remains in a partially-configured state. On the next `dev_start` attempt, the buffer will not be reduced again (flag is true), leaving RXPBSIZE permanently smaller than intended.

**Suggested fix:**
Add a cleanup path that restores RXPBSIZE if configure fails:
```c
if (fdir_conf->mode != RTE_FDIR_MODE_NONE) {
    err = ixgbe_fdir_configure(adapter, fdir_conf, &info->mask);
    if (err) {
        /* Restore RXPBSIZE if we reduced it */
        if (info->rx_pb_reduced) {
            uint32_t fdirctrl = IXGBE_READ_REG(hw, IXGBE_FDIRCTRL);
            uint32_t pbsize = (1 << (PBALLOC_SIZE_SHIFT +
                (fdirctrl & FDIRCTRL_PBALLOC_MASK)));
            IXGBE_WRITE_REG(hw, IXGBE_RXPBSIZE(0),
                (IXGBE_READ_REG(hw, IXGBE_RXPBSIZE(0)) + pbsize));
            info->rx_pb_reduced = false;
        }
        goto error;
    }
}
```

## Warnings

**1. Missing release notes update**

This is a bug fix that changes driver behavior (prevents RXPBSIZE corruption) and affects packet buffer sizing. It should be documented in the release notes under "net/ixgbe" fixes.

**2. Inconsistent boolean type usage**

The new field `rx_pb_reduced` uses lowercase `false`/`true` (C99 `<stdbool.h>`) while the adjacent `mask_added` field uses uppercase `FALSE` (DPDK legacy style). For consistency within the structure, consider using the same convention as the existing field, or as a later cleanup convert both to `bool`/`true`/`false`.

Current code:
```c
fdir_info->mask_added = FALSE;
fdir_info->rx_pb_reduced = false;
```

More consistent:
```c
fdir_info->mask_added = FALSE;
fdir_info->rx_pb_reduced = FALSE;
```

## Info

**1. Comment clarity**

The comment in `ixgbe_fdir_configure()` could be more explicit about *why* re-configure must not subtract again: "a later re-configure (e.g. after a failed filter program)" doesn't mention that dev_start can be called multiple times without an intervening reset.

Suggested rewording:
```c
/*
 * Before enabling Flow Director, the Rx Packet Buffer size
 * must be reduced. The new value is the current size minus
 * flow director memory usage size. Subtract only once per
 * hardware reset; dev_start may be called multiple times
 * (e.g., after failed filter program, link down/up).
 */
```

**2. Root cause note**

The commit message mentions the proximate cause (mode reset to NONE) but not the deeper issue: `dev_start` can be called multiple times without `dev_stop` in error recovery paths. This context would help future maintainers understand why the flag is necessary rather than relying solely on `fdir_conf->mode`.

---

## Positive Observations

- The fix correctly addresses the root cause (stateless RXPBSIZE reduction)
- Boolean flag is the appropriate solution (avoids recalculation)
- Flag placement in existing `ixgbe_hw_fdir_info` structure is logical
- Initialization in `ixgbe_fdir_filter_init()` ensures clean state at device probe