|WARNING| net/ice: add chip temperature to xstats

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

# DPDK Patch Review

## ERRORS

None identified.

## WARNINGS

**1. Missing sensor support query on E830**

**File:** drivers/net/intel/ice/ice_ethdev.c  
**Lines:** +7280-7284 (`ice_temp_sensor_supported()`)

```c
static bool
ice_temp_sensor_supported(struct ice_hw *hw)
{
	return (hw->dev_caps.supported_sensors &
		ICE_SENSOR_SUPPORT_E810_INT_TEMP) != 0;
}
```

The function name and capability flag refer only to E810, but the commit message mentions E830 was not tested. The `ICE_SENSOR_SUPPORT_E810_INT_TEMP` flag may need to be verified or extended to cover E830 devices if they also support temperature reading. The Linux kernel ice driver may use additional checks for different hardware generations.

**Suggested fix:** Verify E830 capability flags or add a note in the documentation that this feature is E810-specific until E830 support is confirmed and tested.

---

**2. Missing release notes entry for when the feature is unavailable**

**File:** doc/guides/rel_notes/release_26_11.rst  
**Lines:** +89

```
+  * Added chip temperature to the extended statistics.
```

The release notes mention the feature was added but don't clarify that it's conditional on firmware support. Users reading the release notes might expect the statistics on all ice devices/firmware versions.

**Suggested fix:** Add clarity:
```
* Added chip temperature to the extended statistics (when supported by firmware).
```

---

**3. Potential error code propagation for future use**

**File:** drivers/net/intel/ice/ice_ethdev.c  
**Lines:** +7308

```c
if (ret != 0) {
	PMD_DRV_LOG(DEBUG, "Failed to read chip temperature: %d", ret);
	memset(ts, 0, sizeof(*ts));
	return;
}
```

The error code from `ice_aq_get_sensor_reading()` is logged but not made visible to the caller or stored anywhere. If future code needs to distinguish between "sensor not supported" vs "sensor read failed", this information is lost. The current implementation is acceptable for the use case (periodic polling with cached values), but could limit future diagnostic features.

**Suggested approach:** Consider whether a failure flag or last-error field in `ice_pf` would be useful for future monitoring or alerting features. Not required for this patch, but worth noting.

---

## INFO

**1. Threshold values are static but read on every cache miss**

The warning/critical/fatal thresholds are firmware configuration and unlikely to change at runtime, yet they're re-read every time the 1-second cache expires. If thermal monitoring frequency becomes a concern, the thresholds could be cached separately at device init and only the current temperature polled in `ice_update_temp_stats()`. This is a minor optimization opportunity, not a defect.

---

**2. Documentation structure could benefit from a note on per-PF behavior**

**File:** doc/guides/nics/ice.rst  
**Lines:** +1024-1044

The documentation explains that "every PF reports it" unlike the kernel driver, which is good. It could additionally note that all PFs on a device will report the same chip temperature (since it's a device-level sensor, not per-port), which may not be obvious to users who expect per-port sensors.

**Optional addition:**
```
Note that the chip temperature is a device-level measurement;
all PFs on the same physical device will report identical values.
```

---

**3. Integer truncation acceptable in this context**

**File:** drivers/net/intel/ice/ice_ethdev.c  
**Lines:** +7315

```c
ts->temp = resp.data.s0f0.temp > 0 ? (uint64_t)resp.data.s0f0.temp : 0;
```

The firmware response type `resp.data.s0f0.temp` is an 8-bit signed value (based on the "whole degrees Celsius" description and typical sensor ranges). The conditional zeroing of negative values is appropriate for the use case (chip temperature below 0degC is not physically meaningful in this context). The explicit cast to `uint64_t` is correct and clear.

---

Review-Result: WARNING