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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/panfrost: unnormalized positive return value causes ioctl to skip copy_to_user
- [High] drm/panfrost: missing state reset causes permanent DoS for perfcnt after SESSION_DEAD
- [Low] drm/panfrost: mixing goto and scoped_guard violates cleanup subsystem rules
--

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

drm/panfrost: Fix races between perfcnt and reset sequence

This commit uses the reset r/w semaphore to govern access to the hardware
at reset time, preventing races with tearing down perfcnt sessions or
ongoing hardware resets. It also expands the DRM uAPI for the perfcnt
DUMP operation to notify userspace if a reset occurred.

> diff --git a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> index b3f71d7fd82a9..91983d450c246 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
[ ... ]
> @@ -58,25 +69,100 @@ void panfrost_perfcnt_sample_done(struct panfrost_device *pfdev)
[ ... ]
> +static int panfrost_perfcnt_dump_locked(struct panfrost_device *pfdev, u32 *state)
> +{
> +	struct panfrost_perfcnt *perfcnt = pfdev->perfcnt;
> +	u64 gpuva = perfcnt->mapping->mmnode.start << PAGE_SHIFT;
> +	int ret;
> +
> +	scoped_guard(rwsem_read, &pfdev->reset.lock) {
> +		*state = perfcnt->state;
> +		if (perfcnt->state & PANFROST_PERFCNT_SESSION_DEAD)
> +			return -EIO;
> +
> +		perfcnt->state = 0;

[Severity: High]
Does this permanently lock out the perfcnt session if it dies?

If PANFROST_PERFCNT_SESSION_DEAD is set, the code returns -EIO here before
reaching `perfcnt->state = 0`. Since panfrost_perfcnt_enable_locked() also
does not clear `perfcnt->state` when starting a new session, won't this
leave the dead state stuck permanently and fail all future dump attempts?

[ ... ]
> +	/*
> +	 * 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)
> -		ret = 0;
> +		return -ETIMEDOUT;

[Severity: High]
Will this return the remaining jiffies as a positive value on success?

Since the previous code `else if (ret > 0) ret = 0;` was removed, a successful
completion will now return a positive number. When this propagates back up to
the caller:

panfrost_ioctl_perfcnt_dump()
    ret = panfrost_perfcnt_dump_locked(pfdev, &req->state);
    if (ret)
        goto out;

won't this positive return value be treated as an error by the `if (ret)`
check? If so, this skips the copy_to_user() call entirely and prevents
userspace from receiving the requested performance counter data.

[ ... ]
> @@ -87,9 +173,8 @@ static int panfrost_perfcnt_enable_locked(struct panfrost_device *pfdev,
[ ... ]
> +	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]
Is it intended to use a goto statement inside a scoped_guard block?

The cleanup subsystem guidelines indicate that goto-based error handling
and scope-based cleanup helpers shouldn't be mixed in the same function
to avoid confusing ownership semantics. Should this be refactored to avoid
jumping out of the scope to the err_vunmap label?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924-claude-fixes-v10-0-755929b3cc19@collabora.com?part=15