Re: [PATCH v4 20/20] net/txgbe: fix to enable Tx desc check

[Stephen Hemminger <stephen@networkplumber.org>]

On Mon, 11 May 2026 18:36:02 +0800
Zaiyu Wang <zaiyuwang@trustnetic.com> wrote:

> Now lib security is enabled by default, and cannot be disabled if the
> driver is intended to be used. So Tdm_desc_chk is always unable to enable.
> Remove this restriction, and just enable the corresponding queue check.
> 
> Fixes: 0eabdfcd4af4 ("net/txgbe: enable Tx descriptor error interrupt")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Zaiyu Wang <zaiyuwang@trustnetic.com>
> ---

Minor AI stuff here:


# Review of DPDK Patch: net/txgbe: fix to enable Tx desc check

## Summary
This patch modifies the Tx descriptor check logic in the txgbe driver to work correctly when security features are enabled. The change moves descriptor check setup from a global per-device operation to a per-queue operation.

## ERRORS

None found.

## WARNINGS

### 1. Bit manipulation should use RTE_BIT32() macro
**File:** drivers/net/txgbe/txgbe_rxtx.c  
**Lines:** 4774, 4775

The code uses `BIT()` macro for bitmask generation. DPDK coding standards prefer `RTE_BIT32()` or `RTE_BIT64()` for clarity and portability.

**Current code:**
```c
wr32m(hw, TXGBE_TDM_DESC_CHK(txq->reg_idx / 32),
      BIT(txq->reg_idx % 32), BIT(txq->reg_idx % 32));
```

**Suggested fix:**
```c
wr32m(hw, TXGBE_TDM_DESC_CHK(txq->reg_idx / 32),
      RTE_BIT32(txq->reg_idx % 32), RTE_BIT32(txq->reg_idx % 32));
```

Verify that `BIT()` is defined in the driver's headers. If it's a legacy macro, consider using the DPDK standard `RTE_BIT32()` for consistency.

### 2. Conditional compilation without corresponding documentation update
**Context:** The patch changes the logic for when descriptor checks are enabled but there's no indication of release notes or documentation updates.

The original code disabled all descriptor checks when `RTE_LIB_SECURITY` was not defined. The new code enables descriptor checks per-queue unless the queue is using IPsec. This is a functional behavior change that should be documented.

**Suggested action:**
- Update release notes to describe the change in Tx descriptor check behavior
- Document that descriptor checks are now enabled per-queue based on IPsec usage rather than globally based on library compilation

## INFO

### 1. Potential logic optimization
The same bitmask value `BIT(txq->reg_idx % 32)` is used twice in the `wr32m()` call. Consider extracting it to a local variable for clarity:

```c
uint32_t queue_bit = BIT(txq->reg_idx % 32);
wr32m(hw, TXGBE_TDM_DESC_CHK(txq->reg_idx / 32), queue_bit, queue_bit);
```

This makes the intent clearer (setting the bit to 1) and slightly improves readability.

### 2. Parentheses around `using_ipsec` unnecessary
**Line:** 4772

```c
if (!(txq->using_ipsec))
```

Could be written as:
```c
if (!txq->using_ipsec)
```

The extra parentheses around `using_ipsec` are unnecessary since `!` has higher precedence than most operators.

---

## OVERALL ASSESSMENT

The patch correctly addresses the issue where descriptor checks could not be enabled when the security library was compiled in. The logic change from global enable/disable to per-queue configuration based on IPsec usage is sound.

**Key improvements made:**
- Moves descriptor check setup into the per-queue loop
- Only disables checks for queues actually using IPsec
- Removes the blanket disable when security library is present

**Correctness:** No bugs detected. The resource management and control flow are correct.

**Style compliance:** Minor issues with macro usage; otherwise compliant.