|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

## ERRORS

### Error 1: Use of rte_memory_order_seq_cst without justification
**File:** `drivers/net/mlx5/linux/mlx5_lsc_intr.c`
**Lines:** 41-42, 47

```c
return rte_atomic_load_explicit(&ctx->cb_in_progress, rte_memory_order_seq_cst);
...
rte_atomic_store_explicit(&ctx->cb_in_progress, val, rte_memory_order_seq_cst);
```

Sequential consistency is the strongest and most expensive memory ordering.
Here, the flag is used to prevent cleanup from freeing the device while the callback is running.
This is a simple producer-consumer pattern where `acquire`/`release` ordering suffices:
the store in `lsc_intr_cb_set_in_progress(true)` publishes the "callback is running" state,
and the load in `mlx5_lsc_intr_cleanup()` consumes it.

**Fix:** Use `rte_memory_order_release` for the store and `rte_memory_order_acquire` for the load:

```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_acquire);
}

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_release);
}
```

### Error 2: Missing release notes for significant API/behavior change
**File:** `doc/guides/rel_notes/release_*.rst` (file not present in patch)

This patch changes the LSC interrupt implementation from netlink-based events to polling.
This is a significant behavior change affecting the mlx5 driver and could impact applications
relying on LSC interrupt semantics.
The current release notes must document:
- The change from netlink to polling for LSC interrupts
- The rationale (netlink linkwatch delays)
- Any user-visible behavior changes (timing, responsiveness)

**Fix:** Add an entry to the current release notes under "New Features" or "Driver Changes" for `net/mlx5`.

### Error 3: Inverted return value interpretation in mlx5_link_update
**File:** `drivers/net/mlx5/linux/mlx5_ethdev_os.c`
**Line:** 578

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

`rte_eth_linkstatus_set()` returns 0 when the link status **changed**, and -1 when unchanged.
The code inverts this: it returns 1 (true) when `rte_eth_linkstatus_set()` returns 0,
but the caller expects 0 on success.
The previous code correctly returned the truthiness of the change:

```c
ret = !!memcmp(&dev->data->dev_link, &dev_link, sizeof(struct rte_eth_link));
```

returns 1 when changed, 0 when unchanged, matching the API contract for `eth_link_update`.

**Fix:** Remove the inversion:

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

---

## WARNINGS

### Warning 1: Global state for mlx5_lsc_intr_ctx
**File:** `drivers/net/mlx5/linux/mlx5_lsc_intr.c`
**Line:** 33-36

```c
static struct mlx5_lsc_intr_ctx lsc_intr_ctx = {
	.period_us = LINK_POLL_DEFAULT_PERIOD_US,
	.lock = RTE_SPINLOCK_INITIALIZER,
};
```

The LSC interrupt context is a single global instance shared across all mlx5 ports.
While this works, it couples all ports to a single polling period and alarm callback.
If different mlx5 devices have vastly different link state change characteristics,
they cannot be tuned independently.

**Consideration:** If this becomes a performance or flexibility issue in the future,
consider per-device or per-shared-context instances instead of a single global.
For now, acceptable given the current use case.

---

## INFORMATIONAL

### Info 1: Static spinlock in global instance
**File:** `drivers/net/mlx5/linux/mlx5_lsc_intr.c`
**Line:** 35

```c
.lock = RTE_SPINLOCK_INITIALIZER,
```

`lsc_intr_ctx` is a static variable in process-private memory (not shared memory),
so using a plain pthread spinlock without `PTHREAD_PROCESS_SHARED` is correct.
This is not an issue.

### Info 2: Bitset usage for polled_ports
**File:** `drivers/net/mlx5/linux/mlx5_lsc_intr.c`
**Line:** 27

```c
RTE_BITSET_DECLARE(polled_ports, POLLED_PORTS_MAX);
```

Using `RTE_BITSET` for tracking which ports are polling is appropriate.
The iteration with `RTE_BITSET_FOREACH_SET` (line 150) is concise and efficient.

### Info 3: Removal of netlink-based LSC interrupt handling
**Files:** `drivers/common/mlx5/linux/mlx5_nl.c`, `drivers/net/mlx5/linux/mlx5_ethdev_os.c`, `drivers/net/mlx5/linux/mlx5_os.c`, `drivers/net/mlx5/mlx5.c`, `drivers/net/mlx5/mlx5.h`

The patch removes the netlink socket creation, `mlx5_dev_interrupt_handler_nl()`,
and all related `nl_ih_port_id` tracking.
The deleted code (`mlx5_nl_parse_link_status_update()`, `mlx5_dev_interrupt_nl_cb()`,
`mlx5_link_update_bond()`) is no longer needed with the polling approach.
Removal is clean and complete.

### Info 4: Windows stub implementation
**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;
}
```

The Windows implementation correctly returns `-ENOTSUP` for LSC interrupts,
matching the platform's lack of support for this feature.

### Info 5: Adaptive polling period
**File:** `drivers/net/mlx5/linux/mlx5_lsc_intr.c`
**Lines:** 64-105

The `adjust_period_and_calc_delay()` function dynamically adjusts the polling period
based on how long link status queries take.
This is a sensible optimization to balance responsiveness and CPU overhead.

### Info 6: ETHTOOL_GLINK for direct carrier query
**File:** `drivers/net/mlx5/linux/mlx5_ethdev_os.c`
**Lines:** 320-351

```c
static int
mlx5_link_carrier_get(struct rte_eth_dev *dev)
{
	struct ethtool_value ecmd = {
		.cmd = ETHTOOL_GLINK,
	};
	...
}
```

The commit message and code comments correctly explain that `ETHTOOL_GLINK`
reads the carrier state directly and flushes pending link watch events,
avoiding the ~1 second delay from the kernel's linkwatch mechanism.
This is the core improvement of the patch.

### Info 7: rte_thread_is_intr() check in cleanup
**File:** `drivers/net/mlx5/linux/mlx5_lsc_intr.c`
**Line:** 298

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

The check prevents deadlock if `mlx5_lsc_intr_cleanup()` is called from within
the interrupt thread (e.g., during a callback).
The comment (lines 290-295) explains this clearly.

---

## SUMMARY

The patch changes mlx5 LSC interrupt handling from netlink events to polling via `ETHTOOL_GLINK`.
The implementation is mostly sound, but has three critical issues:

1. **Memory ordering is too strong** -- use `acquire`/`release` instead of `seq_cst`
2. **Missing release notes** for this significant behavior change
3. **Return value inversion** in `mlx5_link_update()` breaks the API contract

The removal of netlink code is clean, the polling infrastructure is well-structured,
and the adaptive period adjustment is a nice touch.

Review-Result: ERROR