Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] The GPU reset path acquires the read/write semaphore in shared (read) mode instead of exclusive (write) mode, failing to provide mutual exclusion against driver entry points. --
--- Patch [9]: [PATCH v5 09/11] drm/panfrost: Introduce a reset lock --- commit 4a244ba91270b92c6bb1ef0025f7f540dbdc5eff Author: Adrián Larumbe <[email protected]> drm/panfrost: envelope GPU reset sequence with rw_semaphore This commit introduces a read/write semaphore to prevent driver entry points from accessing hardware registers while a GPU reset is ongoing. The query timestamp ioctl is updated to hold this semaphore during execution. > diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c > b/drivers/gpu/drm/panfrost/panfrost_device.c > index 5b66173c75b9..e0390b6c0d22 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_device.c > +++ b/drivers/gpu/drm/panfrost/panfrost_device.c > @@ -602,6 +602,8 @@ bool panfrost_exception_needs_reset(const struct > panfrost_device *pfdev, > > void panfrost_device_reset(struct panfrost_device *pfdev, bool > enable_job_int) > { > + guard(rwsem_read)(&pfdev->reset.lock); [Severity: High] Does acquiring this lock as a shared reader (rwsem_read) instead of an exclusive writer (rwsem_write) fail to provide mutual exclusion? Since the protected ioctl path in panfrost_ioctl_query_timestamp() also acquires the lock in read mode, won't both paths execute concurrently? This could allow an unprivileged userspace program calling the timestamp ioctl to access hardware registers while the GPU reset is ongoing, which defeats the intended mutual exclusion. > + > panfrost_gpu_soft_reset(pfdev); > panfrost_gpu_power_on(pfdev); > panfrost_mmu_reset(pfdev); [ ... ] > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c > b/drivers/gpu/drm/panfrost/panfrost_drv.c > index 958f1d36ab10..ff23b1a979bb 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) { [Severity: High] As noted above, since both the reset path and this ioctl path acquire the lock in read mode, will this scoped_guard fail to wait for an ongoing reset to complete? > + panfrost_cycle_counter_get(pfdev); > + *arg = panfrost_timestamp_read(pfdev); > + panfrost_cycle_counter_put(pfdev); > + } > > pm_runtime_put(pfdev->base.dev); > return 0; -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=9
