Re: [PATCH v4 04/20] net/ngbe: fix VF promiscuous and allmulticast

[Stephen Hemminger <stephen@networkplumber.org>]

On Mon, 11 May 2026 18:35:46 +0800
Zaiyu Wang <zaiyuwang@trustnetic.com> 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: stable@dpdk.org
> 
> Signed-off-by: Zaiyu Wang <zaiyuwang@trustnetic.com>
> ---

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).