Re: [PATCH 2/6] drm/nouveau/gsp: add RUSD telemetry support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Missing delay or CPU relax in bounded retry loop causes spurious -EAGAIN on GSP-RM telemetry reads.
- [Medium] Jiffies wrap-around edge case causes telemetry group to be prematurely dropped.
--

--- Patch [2]: [PATCH 2/6] drm/nouveau/gsp: add RUSD telemetry support ---
commit e2f85d24749e78bb60d3f7f4d694be38a59b92af
Author: Mohamed Ahmed <mohamedahmedegypt2001@gmail.com>

drm/nouveau/gsp: add RUSD telemetry support

This commit adds RUSD telemetry support for GSP-RM by allocating a shared-data
buffer and enabling demand-driven polling of sensor data. It tracks group
access and disables polling for groups that have been idle for a timeout period.

> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/rusd.c b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/rusd.c
> new file mode 100644
> index 0000000000000..d20dda07596a0
> --- /dev/null
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/rusd.c
> @@ -0,0 +1,531 @@
[ ... ]
> +static void
> +r570_rusd_poke(struct nvkm_gsp *gsp, enum nvkm_gsp_rusd_group group)
> +{
> +	mutex_lock(&gsp->rusd.mutex);
> +	/* @valid (not gsp->running) is the teardown fence: it is cleared under
> +	 * this mutex at the very start of fini, while gsp->running is still
> +	 * true, so checking it here stops a read from enabling a group and
> +	 * issuing a SET_DATA_POLL to a firmware that is being unloaded.
> +	 */
> +	if (gsp->rusd.valid) {
> +		gsp->rusd.group_access[group] = jiffies;
> +		r570_rusd_set_mask(gsp, gsp->rusd.mask | r570_rusd_group_mask[group]);

[Severity: Medium]
If jiffies happens to be exactly 0 during this assignment, could it cause issues
in the idle worker below?

> +		if (gsp->rusd.mask)
> +			schedule_delayed_work(&gsp->rusd.work,
> +					      msecs_to_jiffies(R570_RUSD_IDLE_MS));
> +	}
> +	mutex_unlock(&gsp->rusd.mutex);
> +}
> +
> +/* Drop the groups that haven't been read within the idle window. */
> +static void
> +r570_rusd_work_drop_idle(struct work_struct *work)
> +{
> +	struct nvkm_gsp *gsp = container_of(to_delayed_work(work),
> +					    struct nvkm_gsp, rusd.work);
> +	unsigned long idle = msecs_to_jiffies(R570_RUSD_IDLE_MS);
> +	u64 mask = 0;
> +	int g;
> +
> +	mutex_lock(&gsp->rusd.mutex);
> +	if (gsp->rusd.valid) {
> +		for (g = 0; g < NVKM_GSP_RUSD_GROUP_COUNT; g++) {
> +			if (gsp->rusd.group_access[g] &&

[Severity: Medium]
When jiffies wraps to 0, this check evaluates to false. 

Will this cause the telemetry group to be prematurely dropped from the poll mask
until the next read cycle?

> +			    time_before(jiffies, gsp->rusd.group_access[g] + idle))
> +				mask |= r570_rusd_group_mask[g];
> +		}
[ ... ]
> +/*
> + * Copy one section out of the shared buffer, untorn. Every section starts
> + * with a u64 timestamp GSP-RM sets to WRITE_IN_PROGRESS while updating it.
> + */
> +static int
> +r570_rusd_read_section(struct nvkm_gsp *gsp, size_t offset, size_t size, void *data)
> +{
> +	const void *sect = gsp->rusd.mem.data + offset;
> +	u64 seq0, seq1;
> +	int retries;
> +
> +	for (retries = 10; retries; retries--) {
> +		seq0 = READ_ONCE(*(const u64 *)sect);
> +		if (seq0 == RUSD_TIMESTAMP_INVALID)
> +			return -ENODATA;
> +		if (seq0 == RUSD_TIMESTAMP_WRITE_IN_PROGRESS)
> +			continue;

[Severity: Medium]
Without a delay or cpu_relax() here, the 10 iterations will exhaust extremely
fast (in tens of nanoseconds). 

Could this cause spurious -EAGAIN errors if the CPU spins through all retries
before the GSP firmware has time to finish its memory write?

> +		/* Sequence counters appear only in sections written by the kernel and
> +		 * never in GSP-polled ones.
> +		 */
> +		if (seq0 >= RUSD_SEQ_START)
> +			return -ENODATA;
> +

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