|WARNING| [2/2] net/mlx5: reset promiscuous modes on device close

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-07-31

# DPDK Patch Review

## Patch 1/2: net/mlx5: fix promiscuous inconsistency on port start

### Errors

None.

### Warnings

1. **Missing error handling in mlx5_dev_start()**
   
   The calls to `mlx5_os_set_promisc()` and `mlx5_os_set_allmulti()` can fail (both return `int`), but the return values are ignored. If these calls fail, the port state remains inconsistent between DPDK and the kernel.
   
   ```c
   /* Current code ignores return values */
   mlx5_os_set_promisc(dev, dev->data->promiscuous);
   mlx5_os_set_allmulti(dev, dev->data->all_multicast);
   
   /* Should check and handle errors */
   ret = mlx5_os_set_promisc(dev, dev->data->promiscuous);
   if (ret != 0) {
       DRV_LOG(ERR, "port %u failed to set promiscuous mode", 
               dev->data->port_id);
       goto error;
   }
   ```

2. **New API functions not marked as experimental**
   
   `mlx5_nl_get_promisc()` and `mlx5_nl_get_allmulti()` are new internal API functions but lack the `__rte_internal` tag in their declarations in `mlx5_nl.h`. The `RTE_EXPORT_INTERNAL_SYMBOL` macro is correctly used in the `.c` file, but the header should also have the attribute.
   
   In `drivers/common/mlx5/linux/mlx5_nl.h`:
   ```c
   /* Current */
   bool mlx5_nl_get_promisc(int nlsk_fd, unsigned int iface_idx);
   bool mlx5_nl_get_allmulti(int nlsk_fd, unsigned int iface_idx);
   
   /* Should be */
   __rte_internal
   bool mlx5_nl_get_promisc(int nlsk_fd, unsigned int iface_idx);
   __rte_internal
   bool mlx5_nl_get_allmulti(int nlsk_fd, unsigned int iface_idx);
   ```

3. **Implicit comparison on boolean return**
   
   The comparisons `!= dev->data->promiscuous` and `!= dev->data->all_multicast` compare a `bool` return value against an `int` field. While this works, it would be clearer to either cast the field to `bool` or use explicit `!= 0` if the field is intentionally `int`.
   
   ```c
   /* Current */
   if (mlx5_os_get_promisc(dev) != dev->data->promiscuous)
   
   /* Clearer if data fields are int */
   if (mlx5_os_get_promisc(dev) != (dev->data->promiscuous != 0))
   ```

### Info

1. **Refactoring opportunity for code reuse**
   
   The new `mlx5_nl_link_info()` helper nicely refactors the duplicated RTM_GETLINK request pattern. The `mlx5_nl_get_mtu_bounds()` conversion demonstrates good cleanup.

---

## Patch 2/2: net/mlx5: reset promiscuous modes on device close

### Errors

None.

### Warnings

1. **Missing error handling in mlx5_dev_close()**
   
   Similar to Patch 1, the calls to `mlx5_os_set_promisc()` and `mlx5_os_set_allmulti()` can fail, but the return values are ignored. In a close path, failure to reset the kernel state leaves the netdevice in promiscuous mode, which is the bug this patch aims to prevent.
   
   ```c
   /* Current code ignores return values */
   mlx5_os_set_promisc(dev, 0);
   mlx5_os_set_allmulti(dev, 0);
   
   /* Should log errors at minimum */
   if (mlx5_os_set_promisc(dev, 0) != 0)
       DRV_LOG(WARNING, "port %u failed to reset promiscuous mode",
               dev->data->port_id);
   if (mlx5_os_set_allmulti(dev, 0) != 0)
       DRV_LOG(WARNING, "port %u failed to reset all multicast mode",
               dev->data->port_id);
   ```

2. **Missing braces for single-statement conditionals**
   
   Both `if` blocks contain only a single compound statement (the `DRV_LOG` call followed by the set call), but the DPDK style omits braces only for truly single statements. Each block here has two statements (log + set), so braces should be used.
   
   ```c
   /* Current */
   if (mlx5_os_get_promisc(dev)) {
       DRV_LOG(DEBUG, "port %u resetting promiscuous mode", dev->data->port_id);
       mlx5_os_set_promisc(dev, 0);
   }
   
   /* This is correct - two statements require braces */
   ```
   
   Actually, on review, the braces ARE present in the patch. Disregard this item.

### Info

1. **Placement of reset logic**
   
   The reset logic is placed after freeing RSS resources but before flushing MAC addresses. This ordering appears reasonable -- resetting promiscuous mode before flushing MACs ensures the netdevice is in a clean state before MAC cleanup.

---

## General Observations

1. **Documentation**: The new functions `mlx5_os_get_promisc()` and `mlx5_os_get_allmulti()` have Doxygen comments, which is good. The Windows stub implementations correctly document their behavior (always returning false).

2. **Code consistency**: Both patches apply the same pattern (check kernel state, sync if needed) symmetrically to promisc and allmulti modes, which is good for maintainability.

3. **Platform portability**: The Windows stubs are appropriate -- the comment indicating no API exists for querying the states documents the limitation clearly.

---

## Summary

The patches correctly address the promiscuous mode inconsistency issue by querying kernel state on start and resetting it on close. The main concern is the missing error handling for the set operations in both start and close paths. The new internal API functions should have `__rte_internal` tags in the header declarations to match the export macros in the implementation.