|WARNING| [13/13] net/mlx5: allow multiport auxiliary device probe

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-10-26

I'll analyze these patches against the DPDK coding standards and contribution guidelines. Let me review each patch systematically.

## Overall Series Assessment

This series refactors mlx5 driver probing to support multiple rte_devices per mlx5_common_device, enabling scenarios like nested switches and ConnectX socket direct configurations.

---

## PATCH 01/13: common/mlx5: rework IB port number query

### Findings

**Info:**
The patch extracts cache fetch/update logic into separate functions (`mlx5_dev_info_get_portnum`, `mlx5_dev_info_set_portnum`) and allows calling `mlx5_nl_portnum()` without cache. The logic is sound and no correctness issues are detected.

---

## PATCH 02/13: common/mlx5: allow ifindex query without cache

### Findings

**Info:**
Adds `goto query_port_info` label and NULL checks for `dev_info` to allow usage without cache. The error handling is correct.

---

## PATCH 03/13: common/mlx5: separate netdev info from switch info

### Findings

**Info:**
Refactors `mlx5_nl_switch_info()` to call new `mlx5_nl_netdev_info_get()` function. The parsing logic in `mlx5_nl_netdev_info_cb()` is moved from the old switch_info callback. No correctness issues detected.

---

## PATCH 04/13: common/mlx5: query parent device over Netlink

### Findings

**Info:**
Extends `mlx5_nl_netdev_info_get()` to parse `IFLA_PARENT_DEV_NAME` and `IFLA_PARENT_DEV_BUS_NAME`. Buffer size checks are present (`payload_len < MLX5_PARENT_DEV_NAME_MAX`). No issues.

---

## PATCH 05/13: common/mlx5: add check for auxiliary device

### Findings

**Info:**
Adds `mlx5_dev_is_auxiliary()` helper. Simple string comparison. No issues.

---

## PATCH 06/13: common/mlx5: gather associated devices

### Findings

#### Error: Resource leak on netlink socket in `mlx5_os_fill_associated_devices()`

```c
nl_rdma = mlx5_nl_init(NETLINK_RDMA, 0);
if (nl_rdma < 0) {
    ret = -rte_errno;
    goto release_return;
}

portnum = mlx5_nl_portnum(nl_rdma, ibdev->name, NULL);
if (portnum == 0) {
    ret = -rte_errno;
    goto release_return;  /* ERROR: nl_rdma not closed */
}
if (portnum == 1) {
    ret = 0;
    goto release_return;  /* ERROR: nl_rdma not closed */
}

nl_route = mlx5_nl_init(NETLINK_ROUTE, 0);
if (nl_route < 0) {
    ret = -rte_errno;
    goto release_return;  /* ERROR: nl_rdma not closed */
}
```

After `nl_rdma` is successfully opened, the early exits at `portnum == 0` and `portnum == 1` jump to `release_return` without closing `nl_rdma`. The `release_return` label only closes if `>= 0`, but `nl_rdma` is already open and will leak.

**Fix:** All exits after successful `mlx5_nl_init(NETLINK_RDMA, ...)` must ensure `nl_rdma` is closed.

---

## PATCH 07/13: common/mlx5: always use EAL device for probing

### Findings

**Info:**
Adds `dev` parameter to `mlx5_class_driver_probe_t` and checks `probe_associated` flag. The logic to iterate `associated_devices` and match by `mlx5_dev_matches()` is correct.

---

## PATCH 08/13: common/mlx5: query devlink port for netdevs

### Findings

**Info:**
Adds `IFLA_DEVLINK_PORT` parsing in `mlx5_nl_netdev_info_cb()` via nested `parse_ifla_devlink_port()`. Uses `nla_ok()` / `nla_next()` wrappers. Buffer size checks are present. No issues.

---

## PATCH 09/13: common/mlx5: get devlink port by port index

### Findings

**Info:**
Adds `mlx5_nl_devlink_port_info_get()` which constructs a netlink message for `DEVLINK_CMD_PORT_GET`. The buffer for the request is allocated with `calloc()` and freed before return. No leak detected. The `__rte_unused` removal is intentional.

---

## PATCH 10/13: common/mlx5: add check for managed device

### Findings

**Info:**
Adds `mlx5_dev_is_managed()` helper. Simple string comparison on `dev->driver->name`. No issues.

---

## PATCH 11/13: net/mlx5: add sibling device iteration macro

### Findings

**Info:**
Replaces `MLX5_ETH_FOREACH_DEV` with new `MLX5_ETH_FOREACH_SIBLING` macro that iterates by common device instead of EAL device. The `mlx5_eth_find_next_sibling()` function checks `priv->sh->cdev` for equality. No correctness issues.

---

## PATCH 12/13: net/mlx5: allow associated devices

### Findings

#### Error: Missing switch case in `mlx5_nl_check_switch_info()`

```c
case MLX5_PHYS_PORT_NAME_TYPE_VIRTUAL:
    switch_info->master = parent_dev_match;
    switch_info->representor = !parent_dev_match;
    break;
```

This case was added, but the function signature changed to add `parent_dev_match` parameter. The logic depends on `parent_dev_match` being correctly set by the caller. In `mlx5_nl_switch_info()`:

```c
if (info->name_type == MLX5_PHYS_PORT_NAME_TYPE_VIRTUAL) {
    if (!(netdev_info.flags & MLX5_NETDEV_INFO_FLAG_PARENT_DEV_NAME) ||
        !(netdev_info.flags & MLX5_NETDEV_INFO_FLAG_PARENT_DEV_BUS_NAME)) {
        DRV_LOG(ERR, "ifindex %u cannot get parent dev", ifindex);
        rte_errno = EINVAL;
        return -rte_errno;
    }

    strlcpy(info->parent_dev_bus_name, netdev_info.parent_dev_bus_name,
        sizeof(info->parent_dev_bus_name));
    strlcpy(info->parent_dev_name, netdev_info.parent_dev_name,
        sizeof(info->parent_dev_name));

    parent_dev_match = mlx5_dev_matches(cdev->dev,
                        netdev_info.parent_dev_bus_name,
                        netdev_info.parent_dev_name);
} else {
    /* Irrelevant for other port flavours. */
    parent_dev_match = false;
}

mlx5_nl_check_switch_info(parent_dev_match, num_vf_set, info);
```

The logic appears correct: `parent_dev_match` is initialized on the VIRTUAL path before calling `mlx5_nl_check_switch_info()`. No issue here.

#### Warning: `check_virtual_port()` hardcodes NETLINK_GENERIC socket init failure as DEBUG

```c
static void
check_virtual_port(struct mlx5_netdev_info *netdev_info,
           struct mlx5_switch_info *switch_info)
{
    ...
    nl = mlx5_nl_init(NETLINK_GENERIC, 0);
    if (nl < 0) {
        DRV_LOG(DEBUG, "Unable to open NETLINK_GENERIC socket");
        return;
    }
    ...
}
```

Silent return on netlink failure means the port type stays `NOTSET` and later logic may fail silently. If this is expected (old kernel), this is acceptable. If not, consider logging at INFO or WARNING level.

**Decision:** This is a pre-existing pattern in mlx5 (similar to `mlx5_nl_init(NETLINK_RDMA, ...)` failures logged at DEBUG). Acceptable.

---

## PATCH 13/13: net/mlx5: allow multiport auxiliary device probe

### Findings

#### Error: `mlx5_auxiliary_get_ifindex()` changed return type from `int` to `unsigned int` without updating callers

```c
/* Old signature */
int mlx5_auxiliary_get_ifindex(const char *sf_name);

/* New signature */
unsigned int mlx5_auxiliary_get_ifindex(const char *sf_name);
```

Old code:
```c
ret = mlx5_auxiliary_get_ifindex(dev->name);
if (ret < 0) {
    DRV_LOG(ERR, "failed to get ethdev ifindex: %s", dev->name);
    return ret;
}
spawn.ifindex = ret;
```

New code:
```c
unsigned int
mlx5_auxiliary_get_ifindex(const char *sf_name)
{
    char if_name[IF_NAMESIZE] = { 0 };
    unsigned int ifindex;

    if (mlx5_auxiliary_get_child_name(sf_name, "/net",
                      if_name, sizeof(if_name)) != 0)
        return 0;

    ifindex = if_nametoindex(if_name);
    if (ifindex == 0)
        rte_errno = errno;
    return ifindex;
}
```

The function now returns `0` on error and sets `rte_errno`. This is correct. The callers have been updated to check `if (ifindex == 0)`. No error here.

---

## Summary of Findings

### Errors

**PATCH 06/13:**
1. Resource leak: `nl_rdma` socket not closed on early exit paths (`portnum == 0`, `portnum == 1`, `nl_route < 0`) in `mlx5_os_fill_associated_devices()`.

### Warnings

None.

### Info

- Pre-existing code: PATCH 12 logs NETLINK_GENERIC failure at DEBUG level, which is consistent with existing mlx5 practices for optional netlink operations.

---

Review-Result: ERROR