Re: [PATCH v5 09/11] drm/panfrost: Introduce a reset lock

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