|WARNING| [v1, 1/1] net/nbl: fix multicast reception in promiscuous mode

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: WARNING
https://dpdk.org/patch/165212

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-06-09

# DPDK Patch Review

## Patch: net/nbl: fix multicast reception in promiscuous mode

### Summary
This patch fixes multicast packet reception when promiscuous mode is enabled in the NBL PMD. The fix adds calls to `cfg_multi_mcast()` to enable/disable multicast reception alongside promiscuous mode configuration.

---

## Findings

### Errors

None.

### Warnings

**1. Missing resource cleanup on error path in `nbl_disp_cfg_multi_mcast()`**

In `nbl_dispatch.c`, function `nbl_disp_cfg_multi_mcast()`:
```c
static int nbl_disp_cfg_multi_mcast(void *priv, u16 vsi, u16 enable)
{
	struct nbl_dispatch_mgt *disp_mgt = (struct nbl_dispatch_mgt *)priv;
	struct nbl_resource_ops *res_ops = NBL_DISP_MGT_TO_RES_OPS(disp_mgt);
	int ret = 0;

	if (enable)
		ret = NBL_OPS_CALL(res_ops->add_multi_mcast,
				   (NBL_DISP_MGT_TO_RES_PRIV(disp_mgt), vsi));
	else
		NBL_OPS_CALL(res_ops->del_multi_mcast, (NBL_DISP_MGT_TO_RES_PRIV(disp_mgt), vsi));

	return ret;
}
```

If `add_multi_mcast()` fails (returns non-zero), the function returns the error but leaves multicast state inconsistent. The callers (`nbl_promiscuous_enable`/`nbl_promiscuous_disable`) do not check this return value and continue to set promiscuous mode anyway.

**Suggested fix:**
```c
/* In nbl_dev.c, nbl_promiscuous_enable(): */
int ret;

ret = disp_ops->cfg_multi_mcast(NBL_DEV_MGT_TO_DISP_PRIV(dev_mgt),
				dev_mgt->net_dev->vsi_id, 1);
if (ret != 0)
	return ret;

ret = disp_ops->set_promisc_mode(NBL_DEV_MGT_TO_DISP_PRIV(dev_mgt),
				 dev_mgt->net_dev->vsi_id, 1);
if (ret != 0) {
	/* Rollback multicast config on error */
	disp_ops->cfg_multi_mcast(NBL_DEV_MGT_TO_DISP_PRIV(dev_mgt),
				  dev_mgt->net_dev->vsi_id, 0);
	return ret;
}

dev_mgt->net_dev->promisc = 1;
return 0;
```

Apply similar error handling to `nbl_promiscuous_disable()`.

**2. Missing release notes update**

This is a bug fix for multicast reception that affects user-visible behavior. The fix should be documented in the current release notes (`doc/guides/rel_notes/release_XX_YY.rst`) under "Fixed Issues" for the net/nbl PMD section.

**Suggested addition:**
```rst
* **net/nbl: Fixed multicast reception in promiscuous mode.**

  Fixed an issue where enabling promiscuous mode on NBL PMD prevented
  multicast packet reception due to hardware not forwarding multicast frames.
```

### Info

**1. Function pointer NULL check consideration**

The patch adds calls to `disp_ops->cfg_multi_mcast()` without NULL checks. While this follows the existing pattern in the code (other `disp_ops` calls also lack NULL checks), if this is a newly added operation, verify that all relevant dispatch operation tables are initialized with this function pointer. Missing initialization would cause a NULL dereference at runtime.

**2. Order of operations in stop path**

In `nbl_dev_leonis_stop()`, the new `cfg_multi_mcast(..., 0)` call is placed after `del_multi_rule()` and `del_macvlan()`. Consider whether multicast should be disabled before or after deleting MAC/VLAN filters, depending on hardware behavior. The current order appears reasonable (filters first, then mode) but verify against hardware requirements.

---

## Compliance Summary

**Code Style:**  Compliant
- Indentation, brace style, and naming conventions follow DPDK standards
- No forbidden tokens detected
- No trailing whitespace

**Structure:** Partial compliance
- Missing error handling for `cfg_multi_mcast()` return value
- Missing release notes update

**Correctness:** Minor concern
- Needs error path cleanup to maintain state consistency
- Runtime behavior depends on proper function pointer initialization

---

## Recommendation

Address the error handling in Warning #1 before merging. Add release notes documentation (Warning #2). Verify that `add_multi_mcast` and `del_multi_mcast` function pointers are properly initialized in all relevant operation tables to prevent NULL dereferences.