|WARNING| [v2, 3/3] net/ice: fix adapter stopped on device start error

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## Patch 1/3: net/ice: poll AdminQ if interrupt delivery unavailable

### Errors

None identified.

### Warnings

**Missing alarm cancellation in ice_dev_close**

The new alarm is set in `ice_dev_start()` and cancelled in `ice_dev_stop()`, but there is no cancellation in `ice_dev_close()`.
If the device is closed without first being stopped (which is a valid sequence), the alarm will continue firing on a freed device structure, causing use-after-free.

**Suggested fix:** Add alarm cancellation in `ice_dev_close()`:
```c
static int
ice_dev_close(struct rte_eth_dev *dev)
{
	struct ice_pf *pf = ICE_DEV_PRIVATE_TO_PF(dev->data->dev_private);

	if (pf->use_aq_polling) {
		rte_eal_alarm_cancel(ice_aq_alarm_handler, dev);
		pf->use_aq_polling = false;
	}
	/* ... rest of close ... */
}
```

**Race condition in alarm handler check**

The alarm handler checks `pf->adapter_stopped` without synchronization, and the flag is set by `ice_dev_stop()` without synchronization.
The alarm could fire during or after `ice_dev_stop()` has set `adapter_stopped = true` but before `rte_eal_alarm_cancel()` completes, leading to a race where the alarm re-arms itself after cancellation was requested.

**Suggested fix:** Use atomic operations or document that the race is benign (the next `ice_dev_stop()` will cancel the re-armed alarm).
Given the benign nature (alarm will be cancelled on next stop), this is acceptable as-is but should be noted in a comment:
```c
/* Note: adapter_stopped check without synchronization is intentional.
 * A race where the alarm re-arms after stop begins is benign --
 * the alarm will be cancelled on the next stop or close.
 */
if (!pf->adapter_stopped) {
	ice_handle_aq_msg(dev);
	rte_eal_alarm_set(ICE_AQ_ALARM_INTERVAL, ice_aq_alarm_handler, dev);
}
```

### Info

None.

---

## Patch 2/3: net/i40e: activate alarm if interrupt delivery unavailable

### Errors

None identified.

### Warnings

**Potential alarm leak on device close**

Similar to ice, if `use_aq_polling` is true and the device is closed without being stopped, the alarm will not be cancelled, leading to use-after-free when the alarm fires on a freed device structure.
The existing code only cancels the alarm in polling mode (`intr_conf.rxq == 0`), not in the new fallback case (`use_aq_polling` when interrupt enable failed).

**Suggested fix:** Add alarm cancellation in `i40e_dev_close()` when `use_aq_polling` is true, or ensure `i40e_dev_stop()` is always called before close.
If the latter is guaranteed by the framework, this is acceptable; otherwise, add:
```c
static int
i40e_dev_close(struct rte_eth_dev *dev)
{
	struct i40e_pf *pf = I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private);

	if (pf->use_aq_polling)
		rte_eal_alarm_cancel(i40e_dev_alarm_handler, dev);
	/* ... rest of close ... */
}
```

### Info

None.

---

## Patch 3/3: net/ice: fix adapter stopped on device start error

### Errors

None identified.

### Warnings

None.

### Info

None.

---

## Summary

**Critical issue (Patches 1 & 2):** Both patches introduce alarms that are not cancelled in the device close path, risking use-after-free if the device is closed without being stopped first.
This is a resource leak on the error/close path.

**Recommended action:** Add alarm cancellation in `ice_dev_close()` and `i40e_dev_close()` when `use_aq_polling` is true, or verify and document that `dev_stop()` is always called before `dev_close()` by the framework.