Re: [PATCH 3/6] drm/nouveau: add GSP hwmon support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Legacy (pre-GSP) hardware exposes duplicate dummy temperature and power sensor attributes.
- [High] Unconditional 2-second timeout delay during driver initialization if the GPU supports only thermal or only power telemetry.
- [Medium] The hwmon sysfs read callbacks fail intermittently with -EAGAIN when racing with GSP telemetry updates.
- [Medium] The GSP 'temp1' label incorrectly reports 'GPU core' instead of 'gpu', leaving the new label definition as dead code.
--

--- Patch [3]: [PATCH 3/6] drm/nouveau: add GSP hwmon support ---
commit ceccedcbd9d07d5becab288795d358c7f8e9d71b
Author: Mohamed Ahmed <mohamedahmedegypt2001@gmail.com>

drm/nouveau: add GSP hwmon support

This commit wires up the RUSD telemetry to provide hwmon device support
for Turing through Blackwell boards. It adds temperature and power readout
channels, aiming to leave legacy therm/volt/iccsense paths unchanged.

> diff --git a/drivers/gpu/drm/nouveau/nouveau_hwmon.c b/drivers/gpu/drm/nouveau/nouveau_hwmon.c
> --- a/drivers/gpu/drm/nouveau/nouveau_hwmon.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_hwmon.c

[ ... ]

> @@ -243,52 +250,144 @@ nouveau_chip_is_visible(const void *data, u32 attr, int channel)

[ ... ]

> +static umode_t
> +nouveau_power_is_visible(const void *data, u32 attr, int channel)
> +{

[ ... ]

> +	if (nvkm_gsp_rusd(gsp))
> +		return nouveau_rusd_power_is_visible(gsp, attr, channel);
> +	else {
> +		if (!iccsense || !iccsense->data_valid || list_empty(&iccsense->rails))
> +			return 0;
> +
> +		switch (attr) {
> +		case hwmon_power_input:
> +			return 0444;

[Severity: High]
Does this inadvertently expose dummy power sensors on pre-GSP hardware?

The hwmon core iterates over the newly added channels in nouveau_info, but
this legacy fallback branch in nouveau_power_is_visible() does not check the
channel index. Because it unconditionally returns 0444 for hwmon_power_input,
it advertises support for channels 1 and 2 on legacy boards, duplicating
channel 0.

A similar issue appears in nouveau_temp_is_visible() which returns 0444 for
hwmon_temp_input regardless of the channel parameter.

> @@ -368,47 +471,67 @@ nouveau_temp_read(struct device *dev, u32 attr, int channel, long *val)

[ ... ]

> +	if (nvkm_gsp_rusd(gsp)) {

[ ... ]

> +		ret = nvkm_gsp_rusd_read(gsp, channel ? NVKM_GSP_RUSD_TEMP_HBM :
> +							NVKM_GSP_RUSD_TEMP_GPU, &value);
> +		if (ret)
> +			return ret;

[Severity: Medium]
Could this cause sporadic sensor read failures in userspace?

When userspace reads a sysfs attribute like temp1_input during a GSP write
window, nvkm_gsp_rusd_read() returns -EAGAIN. Since there is no retry loop
here in nouveau_temp_read() (unlike the one implemented in nouveau_rusd_ok()
for initialization), does this directly fail the userspace read and cause
monitoring tools to report errors?

This pattern also appears in nouveau_rusd_power_read().

> @@ -597,6 +770,8 @@ nouveau_is_visible(const void *data, enum hwmon_sensor_types type, u32 attr,
>  }
>  
>  static const char input_label[] = "GPU core";
> +static const char * const nouveau_temp_labels[] = { "gpu", "vram" };
> +static const char * const nouveau_power_labels[] = { "gpu", "board", "vram" };
>  
>  static int
>  nouveau_read_string(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> @@ -607,6 +782,18 @@ nouveau_read_string(struct device *dev, enum hwmon_sensor_types type, u32 attr,
>  		return 0;
>  	}
>  
> +	if (type == hwmon_temp && attr == hwmon_temp_label &&
> +	    channel < ARRAY_SIZE(nouveau_temp_labels)) {
> +		*buf = nouveau_temp_labels[channel];
> +		return 0;
> +	}

[Severity: Medium]
Is the new label for temp1 unreachable for GSP boards?

Earlier in nouveau_read_string(), there is a check for channel == 0:

	if (type == hwmon_temp && attr == hwmon_temp_label && channel == 0) {
		*buf = input_label;
		return 0;
	}

Because that check intercepts channel 0 and returns "GPU core", does it
prevent the execution of this new block and leave nouveau_temp_labels[0]
("gpu") as dead code?

> @@ -667,13 +854,39 @@ nouveau_hwmon_init(struct drm_device *dev)

[ ... ]

> +		for (;;) {
> +			thermal = nouveau_rusd_ok(gsp, NVKM_GSP_RUSD_TEMP_GPU);
> +			power = nouveau_rusd_ok(gsp, NVKM_GSP_RUSD_POWER_GPU) ||
> +				nouveau_rusd_ok(gsp, NVKM_GSP_RUSD_POWER_GPU_AVG) ||
> +				nouveau_rusd_ok(gsp, NVKM_GSP_RUSD_POWER_CAP);
> +			if ((thermal && power) || !time_before(jiffies, timeout))
> +				break;
> +			msleep(20);
> +		}

[Severity: High]
Will this loop introduce a noticeable boot delay on configurations that only
support one of these telemetry features?

The loop condition requires both thermal and power telemetry to become valid
to break early. If a GPU SKU natively supports only thermal or only power
sensors, the missing feature will constantly fail the check. Will this
unnecessarily block driver probe for the full timeout duration (typically
2 seconds) before proceeding?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714211429.238355-1-mohamedahmedegypt2001@gmail.com?part=3