Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Unprotected hardware register access in panfrost_gpu_get_latest_flush_id() risks data corruption and bus errors during concurrent GPU resets. - [High] Power leak in panfrost_ioctl_query_timestamp() due to incorrect PM runtime put API. -- commit 8d0ec71108e67c1469d4f0da23df91f3320d12ae Author: Adrián Larumbe <[email protected]> drm/panfrost: Introduce a reset lock So as to avoid accessing HW register while a reset is ongoing, a read/write semaphore that envelopes the reset sequence will help driver entry points avoid racing with it. For now, the only such racy entry point is the ioctl that returns the current GPU timestmap. > For now, the only such racy entry point is the ioctl > that returns the current GPU timestmap. [Severity: High] This is a pre-existing issue, but does panfrost_ioctl_submit() in drivers/gpu/drm/panfrost/panfrost_drv.c also need to hold pfdev->reset.lock? It calls panfrost_gpu_get_latest_flush_id() in drivers/gpu/drm/panfrost/panfrost_gpu.c, which reads the GPU_LATEST_FLUSH_ID hardware register directly: panfrost_ioctl_submit() job->flush_id = panfrost_gpu_get_latest_flush_id(pfdev); flush_id = gpu_read(pfdev, GPU_LATEST_FLUSH_ID); If a user calls panfrost_ioctl_submit() concurrently with a job timeout triggering panfrost_reset(), reading the hardware register during a GPU soft reset could return garbage data or trigger a bus error. A corrupted flush ID could cause the GPU to skip necessary cache flushes. [ ... ] > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c > b/drivers/gpu/drm/panfrost/panfrost_drv.c > index f77780c72a1ae..571a26b841269 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c > @@ -41,9 +41,12 @@ static int panfrost_ioctl_query_timestamp(struct > panfrost_device *pfdev, > if (ret) > return ret; > > - panfrost_cycle_counter_get(pfdev); > - *arg = panfrost_timestamp_read(pfdev); > - panfrost_cycle_counter_put(pfdev); > + /* We should not read timestamp register while the GPU is being reset > */ > + scoped_guard(rwsem_read, &pfdev->reset.lock) { > + panfrost_cycle_counter_get(pfdev); > + *arg = panfrost_timestamp_read(pfdev); > + panfrost_cycle_counter_put(pfdev); > + } > > pm_runtime_put(pfdev->base.dev); [Severity: High] This isn't a bug introduced by this patch, but does calling pm_runtime_put() in panfrost_ioctl_query_timestamp() cause the GPU power to remain on indefinitely? Since the driver uses autosuspend (dev->power.use_autosuspend is true), the PM core's rpm_idle returns -EAGAIN and fails to schedule the suspend timer when pm_runtime_put() is called instead of pm_runtime_put_autosuspend(). Any unprivileged user calling the query IOCTL could trigger this. -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=14
