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