On Mon, 11 May 2026 18:36:02 +0800
Zaiyu Wang <[email protected]> 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: [email protected]
>
> Signed-off-by: Zaiyu Wang <[email protected]>
> ---
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.
