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 <[email protected]> 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/[email protected]?part=3
