|WARNING| net/mlx5: move to polling for LSC interrupts

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review: net/mlx5: move to polling for LSC interrupts

## ERRORS

**1. Resource leak on error path in `mlx5_dev_start()`**

File: `drivers/net/mlx5/mlx5_trigger.c`

```c
if (dev->data->dev_conf.intr_conf.lsc) {
    ret = mlx5_lsc_intr_setup(dev);
    if (ret < 0) {
        DRV_LOG(INFO, "port %u failed configure LSC interrupts, starting without",
            dev->data->port_id);
        dev->data->dev_conf.intr_conf.lsc = 0;
    }
}
```

When `mlx5_lsc_intr_setup()` fails, the function continues and may allocate further resources (setting `ih_port_id`, `devx_ih_port_id`, enabling datapath), but the error path on line 1728 (`error:`) does not undo the partial state left by `mlx5_lsc_intr_setup()`. If a later allocation in `mlx5_dev_start()` fails and the code jumps to the `error:` label, any partial state from `mlx5_lsc_intr_setup()` is not cleaned up.

The pattern should either propagate the error immediately (returning after the failure) or ensure cleanup is called on all subsequent error paths. Currently, the code logs INFO and continues, but this is a correctness risk if later error paths do not know about the partial LSC state.

**2. Missing verification that `mlx5_link_carrier_get()` is not silently added to an already-deprecated code path**

File: `drivers/net/mlx5/linux/mlx5_ethdev_os.c`

The patch introduces `mlx5_link_carrier_get()` and calls it from `mlx5_link_update_unlocked_gset()` and `mlx5_link_update_unlocked_gs()`. The names `_gset` and `_gs` suggest legacy ethtool APIs (`ETHTOOL_GSET` and `ETHTOOL_GLINKSETTINGS`). If these are fallback paths for old kernels, then adding new `ETHTOOL_GLINK` dependency makes the fallback path no longer work on kernels that lack `ETHTOOL_GLINK`.

The commit message states `ETHTOOL_GLINK` is available since kernel 4.14, but does not verify that the `_gset` and `_gs` fallback paths are only reached on kernels >= 4.14. If these fallbacks are used on older kernels, `ETHTOOL_GLINK` will fail, producing a false link down or error.

This is a correctness bug: silently breaking the fallback without verifying kernel version or ethtool availability.

**3. `lsc_intr_cb_in_progress()` uses `rte_memory_order_seq_cst` unnecessarily**

File: `drivers/net/mlx5/linux/mlx5_lsc_intr.c`

```c
static inline bool
lsc_intr_cb_in_progress(struct mlx5_lsc_intr_ctx *ctx)
{
    return rte_atomic_load_explicit(&ctx->cb_in_progress, rte_memory_order_seq_cst);
}

static inline void
lsc_intr_cb_set_in_progress(struct mlx5_lsc_intr_ctx *ctx, bool val)
{
    rte_atomic_store_explicit(&ctx->cb_in_progress, val, rte_memory_order_seq_cst);
}
```

Sequential consistency is the strongest and slowest ordering. For a simple boolean flag guarding cleanup (no other data dependencies), `rte_memory_order_acquire` (load) and `rte_memory_order_release` (store) are sufficient and more performant.

The load should use `acquire` to ensure the reader sees all prior writes by the thread that set the flag, and the store should use `release` to ensure all prior writes by the callback thread are visible to the cleanup thread.

## WARNINGS

**1. `adjust_period_and_calc_delay()` may return delay larger than expected**

File: `drivers/net/mlx5/linux/mlx5_lsc_intr.c`

```c
if (ctx->period_us == LINK_POLL_MAXIMUM_PERIOD_US) {
    /*
     * If at maximum period and querying link still takes too long, then default to
     * half of the maximum period so other interrupt thread users are not starved.
     */
    if (delta_us > ctx->period_us / 2)
        return LINK_POLL_MAXIMUM_PERIOD_US / 2;
}
```

This returns the period itself (half of maximum), not the delay until the next poll. The caller expects `period_us - delta_us` (time remaining until next poll should start). Returning a fixed period instead of `(period - delta_us)` may cause the next poll to be scheduled too late or too early.

If `delta_us` is large, this should either return a very small delay (to re-poll soon) or adjust the period and then calculate the correct remaining delay.

**2. `mlx5_link_update()` return value inverted**

File: `drivers/net/mlx5/linux/mlx5_ethdev_os.c`

```c
return rte_eth_linkstatus_set(dev, &dev_link) == 0;
```

`rte_eth_linkstatus_set()` returns `-1` if the link status changed, `0` if it did not change. The `mlx5_link_update()` API (per `rte_eth_dev_ops.link_update`) should return `0` on success (changed or not) and negative on error, OR some drivers interpret the return as "changed" (non-zero) vs "not changed" (zero).

This line inverts the return: if `rte_eth_linkstatus_set()` returns `-1` (link changed), the function returns `0` (false), and if it returns `0` (no change), the function returns `1` (true). This is backwards.

The correct pattern is either:
- Return the value from `rte_eth_linkstatus_set()` directly (if non-zero means change), or
- Return `0` always (if errors are already handled)

Returning `(ret == 0)` makes no sense.

**3. `lsc_intr_check_links()` overwrites `dev->data->dev_link` without holding a lock**

File: `drivers/net/mlx5/linux/mlx5_lsc_intr.c`

```c
dev_link = dev->data->dev_link;
if (dev_link.link_status == ret) {
    devs[i] = NULL;
    continue;
}
dev_link.link_status = ret;
dev->data->dev_link = dev_link;
```

`dev->data->dev_link` is shared state read by control plane operations (link query, MTU set, configure) and the datapath (some PMDs check link status). Writing it from a polling thread without synchronization races with `rte_eth_link_get_nowait()` and similar calls.

The correct pattern is to use `rte_eth_linkstatus_set()`, which handles locking and atomic updates. This function does a read-modify-write without protection.

**4. `rte_eal_alarm_set()` failure in `mlx5_lsc_intr_cb()` leaves polling stopped without notifying application**

File: `drivers/net/mlx5/linux/mlx5_lsc_intr.c`

```c
if (reschedule) {
    ret = rte_eal_alarm_set(delay_us, mlx5_lsc_intr_cb, ctx);
    if (ret < 0) {
        DRV_LOG(ERR, "rescheduling link polling callback failed (%s)",
            rte_strerror(-ret));
        lsc_intr_disarm(ctx);
    }
}
```

If `rte_eal_alarm_set()` fails, polling stops permanently (until the device is stopped and restarted). The application has LSC interrupts enabled in its config, but receives no further link change events. This is a silent failure.

The code should either retry (with backoff), or log at CRIT level, or send a device error event to the application.

**5. `mlx5_lsc_intr_cleanup()` busy-waits with `rte_pause()` instead of using proper synchronization**

File: `drivers/net/mlx5/linux/mlx5_lsc_intr.c`

```c
while (lsc_intr_cb_in_progress(ctx) && !rte_thread_is_intr())
    rte_pause();
```

Busy-waiting is acceptable for a very short wait (a few microseconds), but the callback may run for hundreds of microseconds (gathering devices, checking links, sending notifications). This burns CPU.

A better pattern is a completion mechanism (e.g., a semaphore or condition variable where the callback posts on exit), or tolerating a brief window where the callback may still be running (acceptable if the device removal path does not free memory immediately).

**6. Windows stub returns `-ENOTSUP` for `mlx5_lsc_intr_setup()` but does not set `rte_errno`**

File: `drivers/net/mlx5/windows/mlx5_lsc_intr.c`

```c
int
mlx5_lsc_intr_setup(struct rte_eth_dev *dev)
{
    DRV_LOG(ERR, "port %u does not support LSC interrupts", dev->data->port_id);
    return -ENOTSUP;
}
```

DPDK convention for functions returning negative errno is that they also set `rte_errno` to the positive errno value. The caller in `mlx5_trigger.c` passes the return value to `rte_strerror(-ret)`, which will work, but other callers may expect `rte_errno` to be set.

Add `rte_errno = ENOTSUP;` before returning.

## INFO

**1. Pre-existing: `mlx5_link_carrier_get()` does not validate `dev` argument**

All the link update functions in `mlx5_ethdev_os.c` call `mlx5_ifreq()` without first checking if `dev` or `dev->data` is NULL. This is a pre-existing pattern, but `mlx5_link_carrier_get()` (new in this patch) inherits the same lack of defensive checks. This is acceptable if callers guarantee `dev` is valid, but worth noting for future hardening.

**2. Comment says "Block logical LSC events" but code was removed**

File: `drivers/net/mlx5/linux/mlx5_ethdev_os.c`

The deleted code in `mlx5_dev_interrupt_nl_cb()` had a comment:
```c
/* Block logical LSC events. */
uint16_t prev_status = dev->data->dev_link.link_status;
```

This comment is cryptic and is now gone. If "blocking logical LSC events" referred to suppressing duplicate notifications (checking `prev_status != new_status`), the new code in `lsc_intr_check_links()` performs the same check. No issue, just an observation about comment clarity.

**3. `adjust_period_and_calc_delay()` time calculation**

The function adjusts polling period based on callback duration, which is clever, but the adaptive algorithm (`delta_us > period/2` -> double period, `delta_us < period/4` -> halve period) may oscillate if callback time varies. Consider hysteresis or averaging over several iterations. This is a design trade-off, not a bug.

**4. `mlx5_lsc_intr_gather_devs()` could be more efficient**

The function iterates the bitset and copies pointers into an array, then the caller iterates the array. For small numbers of devices, this is fine, but a pattern where the callback operates on the bitset directly (using `RTE_BITSET_FOREACH_SET`) would avoid the intermediate array. Not a defect, just a performance note.

## Summary

The patch migrates from netlink-based LSC notification to polling via `ETHTOOL_GLINK`, which is a reasonable architectural change. However, it introduces several correctness bugs: error path resource leaks, inverted return value in `mlx5_link_update()`, unsynchronized write to `dev->data->dev_link`, and a busy-wait cleanup pattern. The strongest memory ordering (`seq_cst`) is also used unnecessarily.

Review-Result: ERROR