|WARNING| net/ice: add explicit control of link auto-negotiation

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## ERRORS

### 1. Boolean comparison style violation
**File:** `drivers/net/intel/ice/ice_ethdev.c`
**Line:** `const bool link_autoneg = !ad->devargs.auto_neg_disable ||`

The comparison `(conf->link_speeds & RTE_ETH_LINK_SPEED_FIXED) == 0` should be written explicitly as `!= 0` per DPDK coding standards. DPDK requires explicit comparison against 0 rather than relying on truthiness for non-boolean integer types.

**Suggested fix:**
```c
const bool link_autoneg = !ad->devargs.auto_neg_disable ||
		(conf->link_speeds & RTE_ETH_LINK_SPEED_FIXED) != 0;
```

Note: The negation `!ad->devargs.auto_neg_disable` is correct because `auto_neg_disable` is a `bool` type, and direct boolean usage is acceptable for actual `bool` variables.

### 2. Missing error check
**File:** `drivers/net/intel/ice/ice_ethdev.c`
**Line:** Starting at line 2543

The code checks `ret` after the `ICE_LINK_STATE_ON_CLOSE` parsing but then calls `rte_kvargs_process()` for `ICE_AUTO_NEG_DISABLE_ARG` without assigning or checking its return value before the `bail:` label.

**Current code:**
```c
ret = rte_kvargs_process(kvlist, ICE_LINK_STATE_ON_CLOSE,
			 &parse_link_state_on_close, &ad->devargs.link_state_on_close);
if (ret)
	goto bail;

ret = rte_kvargs_process(kvlist, ICE_AUTO_NEG_DISABLE_ARG,
			 &parse_bool, &ad->devargs.auto_neg_disable);

bail:
rte_kvargs_free(kvlist);
```

The second `rte_kvargs_process()` call assigns to `ret`, but there is no check between the assignment and the `bail:` label. If parsing fails, `ret` will contain an error code that gets returned, but there's a logical inconsistency: the first call uses `if (ret) goto bail;` immediately, while the second does not. This pattern is error-prone and inconsistent.

**Suggested fix:**
```c
ret = rte_kvargs_process(kvlist, ICE_LINK_STATE_ON_CLOSE,
			 &parse_link_state_on_close, &ad->devargs.link_state_on_close);
if (ret)
	goto bail;

ret = rte_kvargs_process(kvlist, ICE_AUTO_NEG_DISABLE_ARG,
			 &parse_bool, &ad->devargs.auto_neg_disable);
if (ret)
	goto bail;

bail:
rte_kvargs_free(kvlist);
```

---

## WARNINGS

None identified.

---

## INFO

### Documentation style note
**File:** `doc/guides/nics/ice.rst`

The documentation adds the new `auto_neg_disable` option, which is good. The formatting and structure follow the existing pattern in the file. The explanation is clear about when the option applies (fixed speed only) and what it does.

One minor observation: The option description says "Control hardware link negotiation when a fixed speed is requested with `RTE_ETH_LINK_SPEED_FIXED`." This is accurate and well-explained.

---

Review-Result: ERROR