Re: [PATCH v4 09/20] net/txgbe: fix link flow control config for Sapphire

[Stephen Hemminger <stephen@networkplumber.org>]

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

> SP chips have a hardware bug preventing XON flow control support,
> so the driver disables it.
> 
> Fixes: 69ce8c8a4ce3 ("net/txgbe: support flow control")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Zaiyu Wang <zaiyuwang@trustnetic.com>
> ---

Comment vs code mismatch


# Review of Patch 9/20: net/txgbe: fix link flow control config for Sapphire

## Errors

None.

## Warnings

### W1: Conditional logic inverted (line 1042-1043)

The comment states "SP doesn't support xon" but the condition enables XON for `txgbe_mac_aml` and `txgbe_mac_aml40`, which are **not** SP chips based on typical wangxun naming. The logic appears inverted.

**Why this matters**: If SP chips are `txgbe_mac_sp` or similar (not aml/aml40), then XON is being enabled on the wrong hardware, causing the bug the patch claims to fix.

**Suggested fix**: Verify the hardware type constants. If SP chips are a different enum value, the condition should be:
```c
/* SP doesn't support xon */
if (hw->mac.type != txgbe_mac_sp && hw->mac.type != txgbe_mac_sp40)
    fcrtl |= TXGBE_FCWTRLO_XON;
```
Or if the current condition is correct, the comment should read "Only AML supports xon" to match the code.

### W2: Inconsistent capitalization in comment (line 1042)

Comment uses lowercase "xon" while the code uses uppercase `XON` in the macro name `TXGBE_FCWTRLO_XON`.

**Suggested fix**:
```c
/* SP doesn't support XON */
```

## Info

None.

---

**Summary**: The primary concern is the apparent mismatch between the comment and the conditional logic. The patch claims to disable XON for SP chips, but the condition enables it for aml/aml40. This requires verification that either the comment is wrong or the condition should be negated/use different constants.