|WARNING| [v2,2/2] 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/172320

_AI Code Review_

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

# DPDK Patch Review

## PATCH 1/2: net/mlx5: fix link status query

### Errors

None.

### Warnings

None.

### Info

None.

---

## PATCH 2/2: net/mlx5: move to polling for LSC interrupts

### Errors

**Error: Resource leak on error path**

In `mlx5_lsc_intr_cb()` at `drivers/net/mlx5/linux/mlx5_lsc_intr.c:249`:
```c
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, the function logs an error and disarms the context, but the callback returns without cleaning up the devices that were added via `lsc_intr_dev_add()`. The devices remain in `ctx->polled_ports`, causing subsequent callbacks to attempt to poll devices that may be in an inconsistent state.

**Suggested fix:**
```c
if (ret < 0) {
	DRV_LOG(ERR, "rescheduling link polling callback failed (%s)",
		rte_strerror(-ret));
	/* Clear all polled ports since we cannot reschedule */
	rte_spinlock_lock(&ctx->lock);
	rte_bitset_clear_all(ctx->polled_ports, POLLED_PORTS_MAX);
	ctx->armed = false;
	rte_spinlock_unlock(&ctx->lock);
}
```

**Error: Use-after-free risk in callback waiting loop**

In `mlx5_lsc_intr_cleanup()` at `drivers/net/mlx5/linux/mlx5_lsc_intr.c:289-291`:
```c
while (lsc_intr_cb_in_progress(ctx) && !rte_thread_is_intr())
	rte_pause();
```
The device is removed from `ctx->polled_ports` before this wait loop. If `lsc_intr_cb()` has already gathered the device pointer into its local `to_process` array (line 213), the callback will access the device via `mlx5_link_carrier_get()` and `mlx5_link_update()` after cleanup has proceeded past the wait loop. This is a use-after-free: the device structure may be freed while the callback is still using it.

The race window: (1) callback gathers device pointers, (2) cleanup removes device from bitset and passes the wait loop (because callback hasn't set in_progress yet), (3) device is freed, (4) callback accesses freed device.

**Suggested fix:**
Set `cb_in_progress` before gathering device pointers, not after:
```c
/* In mlx5_lsc_intr_cb */
lsc_intr_cb_set_in_progress(ctx, true);
reschedule = lsc_intr_cb(ctx, &delay_us);
/* Remove the second set-in-progress call after lsc_intr_cb */
```
And in `lsc_intr_cb`, remove the local setting of `cb_in_progress` -- the outer function now owns it.

**Error: Missing bounds check on port_id in callback**

In `lsc_intr_gather_devs()` at `drivers/net/mlx5/linux/mlx5_lsc_intr.c:150-152`:
```c
RTE_BITSET_FOREACH_SET(port_id, ctx->polled_ports, POLLED_PORTS_MAX) {
	out[ret++] = &rte_eth_devices[port_id];
}
```
If `port_id >= RTE_MAX_ETHPORTS`, the access to `rte_eth_devices[port_id]` is out of bounds. While the bitset size is `POLLED_PORTS_MAX` (which equals `RTE_MAX_ETHPORTS`), there is no guarantee that only valid port IDs were added -- a logic error elsewhere could set a bit beyond the valid range.

**Suggested fix:**
```c
RTE_BITSET_FOREACH_SET(port_id, ctx->polled_ports, POLLED_PORTS_MAX) {
	if (port_id >= RTE_MAX_ETHPORTS) {
		DRV_LOG(ERR, "invalid port_id %zd in polled_ports", port_id);
		continue;
	}
	out[ret++] = &rte_eth_devices[port_id];
}
```

### Warnings

**Warning: Spinlock held across logging call**

In `adjust_period_and_calc_delay()` at `drivers/net/mlx5/linux/mlx5_lsc_intr.c:89-91`:
```c
DRV_LOG(DEBUG, "adjusting link polling period: %" PRIu64 " us to %" PRIu64 " us",
	ctx->period_us, new_period);
```
This function is called while holding `ctx->lock` in the caller `lsc_intr_cb()` (via `lsc_intr_gather_devs`). Logging while holding a spinlock can cause delays or deadlock if the log backend acquires locks.

**Suggested fix:** Move the logging outside the locked region:
```c
/* In lsc_intr_cb, after lsc_intr_gather_devs returns */
if (new_period != ctx->period_us)
	DRV_LOG(DEBUG, "adjusting link polling period: %" PRIu64 " us to %" PRIu64 " us",
		old_period, new_period);
```
And pass `old_period` and `new_period` out of `adjust_period_and_calc_delay`.

**Warning: No bounds check on to_process array**

In `lsc_intr_gather_devs()` at `drivers/net/mlx5/linux/mlx5_lsc_intr.c:143-156`:
```c
static int
lsc_intr_gather_devs(struct mlx5_lsc_intr_ctx *ctx,
		     struct rte_eth_dev *out[POLLED_PORTS_MAX])
{
	ssize_t port_id;
	int ret = 0;

	rte_spinlock_lock(&ctx->lock);
	RTE_BITSET_FOREACH_SET(port_id, ctx->polled_ports, POLLED_PORTS_MAX) {
		out[ret++] = &rte_eth_devices[port_id];
	}
	rte_spinlock_unlock(&ctx->lock);

	return ret;
}
```
If more than `POLLED_PORTS_MAX` bits are set in `ctx->polled_ports` (which cannot happen given the bitset size, but the code does not assert this), `ret` could overflow the `out` array. While the bitset API prevents this in practice, defensive programming would add an assertion.

**Suggested fix:**
```c
RTE_BITSET_FOREACH_SET(port_id, ctx->polled_ports, POLLED_PORTS_MAX) {
	MLX5_ASSERT(ret < POLLED_PORTS_MAX);
	out[ret++] = &rte_eth_devices[port_id];
}
```

**Warning: Potential division by zero**

In `adjust_period_and_calc_delay()` at `drivers/net/mlx5/linux/mlx5_lsc_intr.c:78-85`:
```c
if (delta_us > new_period / 2) {
	/* If takes too long, adjust period up. */
	while (new_period < LINK_POLL_MAXIMUM_PERIOD_US && delta_us > new_period / 2)
		new_period = RTE_MIN(new_period * 2, LINK_POLL_MAXIMUM_PERIOD_US);
} else if (delta_us < new_period / 4) {
	/* If polling is fast, adjust period down. */
	while (new_period > LINK_POLL_MINIMUM_PERIOD_US && delta_us < new_period / 4)
		new_period = RTE_MAX(new_period / 2, LINK_POLL_MINIMUM_PERIOD_US);
}
```
If `new_period` is zero (due to corruption or logic error), the divisions `/2` and `/4` would cause division by zero. While `new_period` is initialized to `ctx->period_us` (which is initialized to `LINK_POLL_DEFAULT_PERIOD_US`), defensive code should validate it.

**Suggested fix:**
```c
new_period = ctx->period_us;
if (new_period == 0) {
	DRV_LOG(ERR, "link polling period is zero, resetting to default");
	new_period = LINK_POLL_DEFAULT_PERIOD_US;
}
```

### Info

**Pre-existing: mlx5_link_update return value semantics inverted**

In patch 1, at `drivers/net/mlx5/linux/mlx5_ethdev_os.c:578`:
```c
return rte_eth_linkstatus_set(dev, &dev_link) == 0;
```
`rte_eth_linkstatus_set()` returns -1 when the link status changed, 0 when it did not change. The new code inverts this: `== 0` becomes `true` (1), meaning "no change", but the caller expects the opposite (return 1 when link changed). This makes the callback in patch 2 suppress notifications when the link actually changed.

Note: This issue is in patch 1, which the current review scope includes. However, it is a logic error, not a style or process issue, and would cause incorrect behaviour.

**Suggested fix:**
```c
return rte_eth_linkstatus_set(dev, &dev_link) == -1;
```
Or more idiomatically:
```c
return rte_eth_linkstatus_set(dev, &dev_link) != 0;
```

---

Review-Result: ERROR