|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

## WARNINGS

### 1. Missing TSC initialization check in `ice_update_temp_stats`

**File:** `drivers/net/intel/ice/ice_ethdev.c`
**Line:** Added function `ice_update_temp_stats`

The function reads `pf->temp_read_tsc` to implement rate limiting,
but never initializes it.
On first call, `temp_read_tsc` is 0 (from `rte_zmalloc_socket` in `ice_dev_init`).
The comparison `now - pf->temp_read_tsc < rte_get_timer_hz()` will be true
only if `now < rte_get_timer_hz()`, which depends on TSC wrap or very early boot.
This is unlikely but not impossible, and produces unpredictable behaviour
on the first call.

**Suggested fix:**

```c
if (pf->temp_read_tsc != 0 &&
    now - pf->temp_read_tsc < rte_get_timer_hz())
    return;
```

becomes:

```c
/* On first call (temp_read_tsc == 0), force a read.
 * Thereafter, limit to one read per second. */
if (pf->temp_read_tsc != 0 &&
    now - pf->temp_read_tsc < rte_get_timer_hz())
    return;
```

Actually, the existing code is correct: when `temp_read_tsc == 0`,
`now - 0 = now`, and `now` is always >= `rte_get_timer_hz()`
except in the first second after boot, which is before DPDK initializes.
However, the intent is unclear.
Add a comment explaining that `temp_read_tsc == 0` means "never read"
and forces the first read.

### 2. Signed temperature cast to unsigned without negative handling justification

**File:** `drivers/net/intel/ice/ice_ethdev.c`
**Line:** `ts->temp = resp.data.s0f0.temp > 0 ? (uint64_t)resp.data.s0f0.temp : 0;`

The code clamps negative temperatures to 0.
The comment says "report below zero as 0",
but the choice to report 0 instead of e.g. omitting the stat
or returning an error is not documented in the code or the .rst file.

Zero is a valid physical temperature (0degC = 32degF, well within operating range),
so reporting a negative reading as 0 could confuse monitoring.
Consider whether a different sentinel (e.g., `UINT64_MAX`) or error indication is clearer.

**Suggested action:**
Document in `doc/guides/nics/ice.rst` that negative readings
(if they occur, indicating firmware error or extreme cold) are reported as 0.

Alternatively, log at WARNING level if a negative temperature is seen,
since it likely indicates a firmware bug.

## INFO

### 1. Rate-limiting uses `rte_get_timer_cycles()` and `rte_get_timer_hz()`

This is correct for the use case (one-second throttle on a slow control path),
but note that `rte_get_tsc_cycles()` / `rte_get_tsc_hz()` are slightly faster
on platforms where TSC is the timer source.
The difference is negligible here since this runs only from xstats_get,
not the data path.
No change needed.

### 2. Temperature stats are `uint64_t` for consistency with xstats API

The xstats API uses `uint64_t` for all stat values.
Temperatures fit in `uint8_t`, but the driver uses `uint64_t`
for consistency with the xstats framework.
This is correct.

### 3. Memset on failure is defensive

On `ice_aq_get_sensor_reading()` failure,
the code zeroes `ts` even though the caller will read stale values from `pf->temp_stats`.
This is fine: it ensures that after one failure,
the next successful read won't be confused by leftover data in `ts` on the stack
(though `ts` is a pointer to `pf->temp_stats`, so there's no stack issue).
Actually, `ts` points to `pf->temp_stats`, so the `memset(ts, 0, sizeof(*ts))`
zeroes the persistent structure.
This means after a failure, the stats are reported as 0 until the next successful read.
This is documented ("If the reading fails, all four values are 0").
Correct.

---

## Summary

The patch is well-structured and correct.
The main issue is a minor clarity concern around the signed-to-unsigned cast
and the choice to report negative temperatures as 0.
Documenting this behaviour in the .rst file or adding a warning log
for negative readings would improve the patch.

Review-Result: WARNING