On Mon, 11 May 2026 18:35:46 +0800
Zaiyu Wang <[email protected]> wrote:
> The configuration of allmulti and promiscuous modes conflicts
> together. For instance, if we enable promiscuous mode, then enable and
> disable allmulti, then the promiscuous mode is wrongly disabled.
>
> To resolve this, the following changes are made:
> - do nothing when we set/unset allmulti if promiscuous mode is on
> - restore the proper mode (none or allmulti) when we disable
> promiscuous mode
>
> Fixes: 7744e90805b5 ("net/ngbe: add promiscuous and allmulticast ops for VF
> device")
> Cc: [email protected]
>
> Signed-off-by: Zaiyu Wang <[email protected]>
> ---
Worth mentioning, but IMHO this is case of AI being overly pedantic.
Patch is OK as is.
# Review of DPDK Patch: net/ngbe: fix VF promiscuous and allmulticast
## Summary
This patch fixes a mode conflict issue in the ngbe VF driver between
promiscuous and allmulticast modes. The overall approach is correct: it
prevents mode conflicts by making allmulticast operations no-ops when
promiscuous mode is active, and restores the appropriate mode when disabling
promiscuous.
## Errors
None identified.
## Warnings
### 1. Implicit comparison in existing code (line 1226)
**Issue:** The existing code uses `if (dev->data->promiscuous == 1)` which
mixes explicit and implicit styles. The patch changes this to `if
(dev->data->promiscuous)` which is implicit.
**Why it matters:** DPDK coding standards require explicit comparisons for
integers (`!= 0` or `== 0`).
**Suggested fix:**
```c
/* Line 1226 - be consistent with explicit comparison */
if (dev->data->promiscuous != 0)
return 0;
/* Line 1250 - same issue */
if (dev->data->promiscuous != 0)
return 0;
```
Note: While the patch changes `== 1` to implicit, the proper fix is to use `!=
0` since `promiscuous` is an integer field, not a boolean type.
### 2. Error propagation pattern inconsistency
**Issue:** The function `ngbevf_dev_promiscuous_disable()` returns 0 early when
allmulticast check causes mode selection, but this early return bypasses the
switch statement's error handling for the actual mode update call.
**Context:** This is not a bug in the patch itself—the early returns in
`ngbevf_dev_allmulticast_enable()` and `ngbevf_dev_allmulticast_disable()` when
promiscuous mode is active are correct (those are intentional no-ops). The mode
variable change in `ngbevf_dev_promiscuous_disable()` correctly passes through
to the existing switch statement for error handling.
No change needed—this comment is for clarity only.
## Info
### Code structure observation
The logic correctly handles the mode transitions:
- When disabling promiscuous mode, it checks `all_multicast` flag and sets the
appropriate target mode (ALLMULTI if enabled, NONE otherwise)
- When enabling/disabling allmulticast while promiscuous is active, it returns
early as a no-op (correct, since promiscuous overrides allmulticast)
This is a clean fix for the mode conflict issue described in the commit message.
---
## Final Recommendation
Address the implicit comparison style issue (Warning #1) to comply with DPDK
coding standards. Change `if (dev->data->promiscuous)` to `if
(dev->data->promiscuous != 0)` in both locations (lines 1226 and 1250).
