Re: [PATCH v15 15/15] drm/panfrost: Fix races between perfcnt and reset sequence

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] drm/panfrost: data race on perfcnt->state during dump error path
- [Low] cleanup: mixing goto and scoped_guard in panfrost_perfcnt_enable_locked
--

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

drm/panfrost: Fix races between perfcnt and reset sequence

This commit updates panfrost's performance counter session logic to use
the reset read/write semaphore when accessing hardware registers. It also
expands the perfcnt dump ioctl to notify userspace if a reset occurred,
rendering previous counter states invalid.

> diff --git a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> index b3f71d7fd82a9..a9def452d5ef9 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
[ ... ]
> @@ -58,27 +70,130 @@ void panfrost_perfcnt_sample_done(struct panfrost_device *pfdev)
>  		complete(&pfdev->perfcnt->dump_comp);
>  }
>  
> -static int panfrost_perfcnt_dump_locked(struct panfrost_device *pfdev)
> +static int panfrost_perfcnt_dump_locked(struct panfrost_device *pfdev, u32 *state)
>  {
[ ... ]
> +	scoped_guard(rwsem_read, &pfdev->reset.lock) {
> +		*state = perfcnt->state;
> +		if (perfcnt->state & PANFROST_PERFCNT_SESSION_DEAD)
> +			return -EIO;
> +
> +		perfcnt->state = 0;
> +
> +		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. Besides, a legitimate reset might be issued during the wait.
> +	 */
>  	ret = wait_for_completion_interruptible_timeout(&pfdev->perfcnt->dump_comp,
>  							msecs_to_jiffies(1000));
> +
>  	if (!ret)
>  		ret = -ETIMEDOUT;
>  
> -	return ret;
> +	/* We need to restore perfcnt state so the next successful dump can relay it to UM */
> +	if (ret < 0) {
> +		perfcnt->state |= *state;
> +		*state = 0;
> +		return ret;
> +	}

[Severity: Medium]
Is there a potential data race on perfcnt->state here?

The dump wait function releases pfdev->reset.lock to allow concurrent GPU
resets. If the wait is interrupted or times out (ret < 0), it performs a
read-modify-write on perfcnt->state without re-acquiring the lock.

Concurrently, if a GPU fault occurs, panfrost_device_reset() will run and
invoke panfrost_perfcnt_reset(), which modifies the same state:

panfrost_perfcnt_reset()
   perfcnt->state |= PANFROST_PERFCNT_SESSION_INTERRUPTED_BY_RESET;

Since the error path in panfrost_perfcnt_dump_locked() modifies
perfcnt->state without synchronization against this reset path, the
PANFROST_PERFCNT_SESSION_INTERRUPTED_BY_RESET flag might be silently
overwritten and lost. Would it be safer to wrap this restoration block inside
a scoped_guard to ensure the flag from the reset thread is preserved?

[ ... ]
> @@ -122,54 +252,30 @@ static int panfrost_perfcnt_enable_locked(struct panfrost_device *pfdev,
>  	ret = drm_gem_vmap(&bo->base, &map);
>  	if (ret)
>  		goto err_put_mapping;
> +
>  	perfcnt->buf = map.vaddr;
> +	perfcnt->counterset = counterset;
>  
>  	panfrost_gem_internal_set_label(&bo->base, "Perfcnt sample buffer");
>  
[ ... ]
> +	scoped_guard(rwsem_read, &pfdev->reset.lock) {
> +		/*
> +		 * 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);
>  
> -	gpu_write(pfdev, GPU_PERFCNT_CFG, cfg);
> +		ret = panfrost_perfcnt_hw_enable(pfdev);
> +		if (ret)
> +			goto err_vunmap;

[Severity: Low]
Does jumping out of a scoped_guard with a goto statement mix cleanup patterns?

The kernel cleanup guidelines note that usage of "goto" and cleanup helpers
(like scoped_guard) should not be mixed in the same function. Doing so can
create confusing ownership semantics and increase the risk of leaks during
future modifications.

Would it be better to either convert all resources in this function to use
scope-based cleanup, or stick to traditional locking (down_read / up_read) to
match the existing goto-based error handling?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012-claude-fixes-v15-0-bc0a6c498553@collabora.com?part=15