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