Re: [PATCH v7 16/17] drm/panfrost: Fix races between perfcnt and reset sequence

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Hardware reset notifications are permanently lost on interrupted ioctl waits.
- [High] Unprotected GPU register writes to GPU_CMD race with device reset.
- [Low] Mixing goto-based error handling with scoped_guard() cleanup in the same function.

Pre-existing issues:
- [High] Spurious WARN_ON due to an off-by-one boundary check in panfrost_mmu_as_get.
--

commit f478a9bf05ba523f5eb295d8a3b40bf995a8444e
Author: Adrián Larumbe <adrian.larumbe@collabora.com>

drm/panfrost: Fix races between perfcnt and reset sequence

This commit updates the reset sequence to use the reset r/w semaphore to
govern hardware access. It also expands the DRM uAPI for the perfcnt dump
operation to inform userspace when a reset occurs, which resets the counters
back to zero.

> diff --git a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> --- a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
[ ... ]
> @@ -58,25 +63,107 @@ void panfrost_perfcnt_sample_done(struct panfrost_device *pfdev)
[ ... ]
> +static int panfrost_perfcnt_dump_locked(struct panfrost_device *pfdev,
> +					u64 *reset_happened)
> +{
[ ... ]
> +dump_retry:
> +	retry = false;
> +	scoped_guard(rwsem_read, &pfdev->reset.lock) {
> +		*reset_happened = perfcnt->reset_happened;
> +		perfcnt->reset_happened = false;

[Severity: High]
If this wait is interrupted by a signal, 
wait_for_completion_interruptible_timeout() returns -ERESTARTSYS. Because
we already cleared perfcnt->reset_happened to false above, when the syscall
is automatically restarted, will it see the cleared flag and falsely report
hw_reset = 0 to userspace?

Could this cause userspace to permanently lose notifications about hardware 
resets?

> +		if (perfcnt->reset_failed) {
> +			ret = panfrost_perfcnt_hw_enable(pfdev);
> +			if (ret)
> +				return ret;
> +			perfcnt->reset_failed = false;
> +		}
> +
> +		reinit_completion(&pfdev->perfcnt->dump_comp);
> +
> +		gpu_write(pfdev, GPU_PERFCNT_BASE_LO, lower_32_bits(gpuva));
> +		gpu_write(pfdev, GPU_PERFCNT_BASE_HI, upper_32_bits(gpuva));
> +		gpu_write(pfdev, GPU_INT_CLEAR, GPU_IRQ_CLEAN_CACHES_COMPLETED |
> +						GPU_IRQ_PERFCNT_SAMPLE_COMPLETED);
> +		gpu_write(pfdev, GPU_CMD, GPU_CMD_PERFCNT_SAMPLE);
> +	}
> +
> +	/*
> +	 * Here we release the reset semaphore because perfcnt should not get in the way
> +	 * of a HW reset. The downside is in the gap between the completion is flagged
> +	 * by the GPU IRQ handler and the sempahore taken once again, yet another reset
> +	 * could happen, which would lead to a valid sample being discarded. This is
> +	 * a solution of compromise between ensuring synchronisation with the reset
> +	 * sequence and letting it happen as quickly as possible for jobs to be rerun.
> +	 */
> +
>  	ret = wait_for_completion_interruptible_timeout(&pfdev->perfcnt->dump_comp,
>  							msecs_to_jiffies(1000));
[ ... ]
> @@ -132,44 +220,19 @@ static int panfrost_perfcnt_enable_locked(struct panfrost_device *pfdev,
[ ... ]
>  	/*
>  	 * Clear the counters to start from a fresh state.
>  	 */
>  	gpu_write(pfdev, GPU_INT_CLEAR, GPU_IRQ_PERFCNT_SAMPLE_COMPLETED);
>  	gpu_write(pfdev, GPU_CMD, GPU_CMD_PERFCNT_CLEAR);

[Severity: High]
Are these register writes exposed to the exact race condition this patch
intends to fix? 

Executing gpu_write(pfdev, GPU_CMD, GPU_CMD_PERFCNT_CLEAR) without holding 
pfdev->reset.lock means a concurrent soft reset (which writes 
GPU_CMD_SOFT_RESET) could race on the GPU_CMD register.

> -	ret = panfrost_mmu_as_get(pfdev, perfcnt->mapping->mmu);
> -	if (ret < 0)
> -		goto err_vunmap;
> -
[ ... ]
> +	scoped_guard(rwsem_read, &pfdev->reset.lock) {
> +		ret = panfrost_perfcnt_hw_enable(pfdev);

[Severity: High]
This is a pre-existing issue, but does panfrost_mmu_as_get() contain an 
off-by-one boundary check that could trigger a spurious kernel warning? 

Looking at panfrost_mmu_as_get() in drivers/gpu/drm/panfrost/panfrost_mmu.c:

        /*
         * AS can be retained by active jobs or a perfcnt context,
         * hence the '+ 1' here.
         */
        WARN_ON(en >= (NUM_JOB_SLOTS + 1));

If a user process submits jobs to all 3 available slots (NUM_JOB_SLOTS = 3)
and simultaneously enables a performance counter session, there are 4
references. atomic_inc_return() returns 4, satisfying 4 >= 4, which
erroneously triggers the WARN_ON.

> +		if (ret)
> +			goto err_vunmap;

[Severity: Low]
Does mixing goto with scoped_guard() here violate the subsystem cleanup
guidelines? 

Jumping to a manual cleanup label from inside a scoped_guard creates 
confusing ownership semantics.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828-claude-fixes-v7-0-72a13b2c125d@collabora.com?part=16