Re: [PATCH v12 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: perfcnt state flag lost when dump ioctl is interrupted by signal
- [Low] drm/panfrost: goto mixing with scoped_guard in panfrost_perfcnt_enable_locked
--

commit fdd119adb59d54bffaacf2ad45f065092d48a919
Author: Adrián Larumbe <adrian.larumbe@collabora.com>
Subject: drm/panfrost: Fix races between perfcnt and reset sequence

This commit uses the reset r/w semaphore when disabling and re-enabling
perfcnt configuration, and in the enable/dump ioctls where device registers
are being accessed. It also expands the DRM uAPI for the perfcnt dump
operation so that user space is aware of hardware resets. Finally, it
bumps the DRM driver minor number.

> diff --git a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> index b3f71d7fd82a9..13521e078ce76 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
[ ... ]
> @@ -58,43 +70,161 @@ void panfrost_perfcnt_sample_done(struct panfrost_device *pfdev)
[ ... ]
> +static int panfrost_perfcnt_dump_locked(struct panfrost_device *pfdev, u32 *state)
>  {
> -	u64 gpuva;
> +	struct panfrost_perfcnt *perfcnt = pfdev->perfcnt;
> +	u64 gpuva = perfcnt->mapping->mmnode.start << PAGE_SHIFT;
>  	int ret;
>  
> -	reinit_completion(&pfdev->perfcnt->dump_comp);
> -	gpuva = pfdev->perfcnt->mapping->mmnode.start << PAGE_SHIFT;
> -	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);
> +	scoped_guard(rwsem_read, &pfdev->reset.lock) {
> +		*state = perfcnt->state;
> +		if (perfcnt->state & PANFROST_PERFCNT_SESSION_DEAD)
> +			return -EIO;
> +
> +		perfcnt->state = 0;

[Severity: Medium]
If wait_for_completion_interruptible_timeout() is interrupted by a signal, it
returns -ERESTARTSYS. The syscall will be restarted, but the internal state
(perfcnt->state) has been cleared here and is never restored on the error
path.

Does this mean the restarted ioctl will read a clean state and lose the reset
flag that was present before the wait?

> +
> +		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));
> +
> +	/* A reset might come through in the gap between the completion returning and the following
> +	 * check, but because no sample was produced, we don't care to relay the state back to UM.
> +	 */
>  	if (!ret)
> -		ret = -ETIMEDOUT;
> -	else if (ret > 0)
> +		return -ETIMEDOUT;
> +
> +	scoped_guard(rwsem_read, &pfdev->reset.lock) {
> +		*state |= perfcnt->state;
> +
> +		/* UM must re-enable their session before requesting new dumps. */
> +		if (perfcnt->state & PANFROST_PERFCNT_SESSION_DEAD)
> +			return -EIO;
> +
> +		/* If we faced a reset during our SAMPLE, the user needs to try again. */
> +		if (perfcnt->state & PANFROST_PERFCNT_SESSION_INTERRUPTED_BY_RESET)
> +			return -EAGAIN;
> +
> +		/* Only when we know no re-eanble or re-dump is required, we can afford
> +		 * to reset the internal state. Otherwise it must be kept so that later
> +		 * ioctls know about error situations in this DUMP and work around it.
> +		 */
> +		perfcnt->state = 0;
> +	}
> +
> +	if (ret > 0)
>  		ret = 0;
>  
>  	return ret;
>  }
[ ... ]
> @@ -122,54 +252,30 @@ static int panfrost_perfcnt_enable_locked(struct panfrost_device *pfdev,
[ ... ]
>  	panfrost_gem_internal_set_label(&bo->base, "Perfcnt sample buffer");
>  
> -	/*
> -	 * 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);
[ ... ]
> +	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);
>  
> -		ret = panfrost_perfcnt_hw_enable(pfdev);
> +		ret = panfrost_perfcnt_hw_enable(pfdev);
> +		if (ret)
> +			goto err_vunmap;

[Severity: Low]
Are we mixing goto-based cleanup with scope-based cleanup helpers here?

The Linux kernel cleanup subsystem guidelines explicitly forbid mixing
goto-based error handling with __free() or guard() cleanups in the same
function to prevent ownership confusion.

Should this function be updated to use either scope-based cleanups
consistently, or traditional goto-based cleanups?

>  
> -	if (panfrost_has_hw_issue(pfdev, HW_ISSUE_8186))
> -		gpu_write(pfdev, GPU_PRFCNT_TILER_EN, 0xffffffff);
> +		perfcnt->user = user;
> +		perfcnt->state = 0;
> +	}
>  
>  	/* The BO ref is retained by the mapping. */
>  	drm_gem_object_put(&bo->base);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260929-claude-fixes-v12-0-62beb08de207@collabora.com?part=15