Re: [PATCH v3 13/13] drm/panfrost: Bump driver minor to reflect new DUMP IOCTL req field

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Infinite loop in `panfrost_perfcnt_dump_locked` due to unsigned integer underflow.
- [High] Race condition bypassing wait for completion in `panfrost_perfcnt_dump_locked`.
- [High] Dead code in reset recovery during performance counter enablement leads to spurious timeouts.
- [High] Ignored return value of `panfrost_perfcnt_hw_enable()` leads to address space reference count underflow.
- [Medium] Stale `reset_happened` flag masks timeout errors in subsequent performance counter sessions.
- [Medium] Race condition in `panfrost_perfcnt_dump_locked()` fails to notify userspace of hardware resets.
--

--- Patch [13]: [PATCH v3 13/13] drm/panfrost: Bump driver minor to reflect new DUMP IOCTL req field ---
commit a7210878c461c0a2b1d591ac0e3e2de27fb4740f
Author: Adrián Larumbe <adrian.larumbe@collabora.com>

drm/panfrost: Bump driver minor to reflect new DUMP IOCTL req field
    
Expose expanded PERFCNT_DUMP IOCTL interface to user mode.

> diff --git a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> index 7020c0192e189..a6f1b0b961fcd 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
[ ... ]
> -static int panfrost_perfcnt_dump_locked(struct panfrost_device *pfdev)
> +static int panfrost_perfcnt_dump_locked(struct panfrost_device *pfdev,
> +					u64 *reset_happened)
>  {
> +	struct panfrost_perfcnt *perfcnt = pfdev->perfcnt;
> +	u64 retries = PERFCNT_DUMP_MAX_RETRIES;
> +	bool previous_reset;
>  	u64 gpuva;
>  	int ret;
>  
>  	reinit_completion(&pfdev->perfcnt->dump_comp);
>  	gpuva = pfdev->perfcnt->mapping->mmnode.start << PAGE_SHIFT;
> +	previous_reset = atomic_read(&perfcnt->reset_happened);
> +
> +dump_retry:

[Severity: High]
Does this sequence have a race condition if a GPU reset occurs during the dump?

The reinit_completion() is called outside the dump_retry loop.

If a hardware sample completion IRQ fires (waking the wait and consuming the
completion so done=0), and a GPU reset occurs immediately after but before
atomic_cmpxchg(), the reset handler calls complete(&dump_comp) (done=1).

The code then jumps to dump_retry because of the reset.

Will the second wait_for_completion immediately consume the pending completion
(done=1) and return without actually waiting for the new sample commands to
finish?

> +	scoped_guard(rwsem_read, &pfdev->reset.lock) {
> +		gpu_write(pfdev, GPU_PERFCNT_BASE_LO, lower_32_bits(gpuva));
[ ... ]
> +	}
> +
>  	ret = wait_for_completion_interruptible_timeout(&pfdev->perfcnt->dump_comp,
>  							msecs_to_jiffies(1000));
> +	if (retries == PERFCNT_DUMP_MAX_RETRIES)
> +		*reset_happened = previous_reset || atomic_read(&perfcnt->reset_happened);
> +
> +	if (atomic_cmpxchg(&perfcnt->reset_happened, 1, 0)) {

[Severity: Medium]
Can this miss notifying userspace of a hardware reset?

If a GPU reset occurs precisely between the atomic_read() and
atomic_cmpxchg(&perfcnt->reset_happened, 1, 0), *reset_happened is assigned 0.

The atomic_cmpxchg() then succeeds, decrements retries, and loops to
dump_retry. On the subsequent successful iteration, the condition
(retries == PERFCNT_DUMP_MAX_RETRIES) is false, so *reset_happened is never
updated to true.

Will this break userspace counter accumulation logic by causing it to aggregate
post-reset small counter values as if they were continuous?

> +		if (--retries >= 0)
> +			goto dump_retry;

[Severity: High]
Is it possible for this to enter an infinite loop?

The retries variable is declared as a u64.

When it reaches 0, --retries will underflow to UINT64_MAX, making the condition
>= 0 always evaluate to true.

If resets continuously occur, could this bypass the PERFCNT_DUMP_MAX_RETRIES
limit and cause a soft lockup?

> +		ret = -EBUSY;
> +	} else {
[ ... ]
> @@ -117,57 +191,37 @@ static int panfrost_perfcnt_enable_locked(struct panfrost_device *pfdev,
[ ... ]
> +	ret = wait_for_completion_timeout(&pfdev->perfcnt->dump_comp,
>  					  msecs_to_jiffies(1000));
> -	if (!ret) {
> +	if (!ret && !atomic_read(&perfcnt->reset_happened)) {
>  		ret = -ETIMEDOUT;
>  		goto err_vunmap;
>  	}

[Severity: High]
Is this reset recovery path dead code?

In panfrost_perfcnt_enable_locked(), the code waits for cache cleaning and
expects perfcnt->reset_happened to be set if a reset occurs during the wait.
However, panfrost_perfcnt_reset() checks if (!perfcnt->user) and returns early.

Because perfcnt->user is only assigned after the wait completes, any reset
during the wait causes the reset handler to return without setting
reset_happened.

Does this mean the wait will just time out and return -ETIMEDOUT instead of
recovering?

[ ... ]
> -	gpu_write(pfdev, GPU_PERFCNT_CFG, cfg);
> -
> -	if (panfrost_has_hw_issue(pfdev, HW_ISSUE_8186))
> -		gpu_write(pfdev, GPU_PRFCNT_TILER_EN, 0xffffffff);
> +	scoped_guard(rwsem_read, &pfdev->reset.lock) {
> +		perfcnt->counterset = counterset;
> +		perfcnt->user = user;
> +		atomic_set(&perfcnt->reset_happened, 0);
> +		panfrost_perfcnt_hw_enable(pfdev);
> +	}

[Severity: High]
Is it safe to ignore the return value of panfrost_perfcnt_hw_enable()?

If panfrost_mmu_as_get() fails inside the helper, the hardware is not enabled
and the AS refcount is not incremented. However, perfcnt->user is still set
and the ioctl returns success.

Upon closing the session, panfrost_perfcnt_disable_locked() unconditionally
calls panfrost_mmu_as_put().

Will this cause an MMU address space reference count underflow?

[ ... ]
> @@ -193,18 +247,15 @@ static int panfrost_perfcnt_disable_locked(struct panfrost_device *pfdev,
>  	if (user != perfcnt->user)
>  		return -EINVAL;
>  
> -	gpu_write(pfdev, GPU_PRFCNT_JM_EN, 0x0);
[ ... ]
> +	scoped_guard(rwsem_read, &pfdev->reset.lock) {
> +		panfrost_mmu_as_put(pfdev, perfcnt->mapping->mmu);
> +		panfrost_perfcnt_gpu_disable(pfdev);
> +		perfcnt->user = NULL;
> +	}

[Severity: Medium]
Does tearing down the session here without clearing the reset_happened atomic
flag cause issues for future sessions?

If a GPU reset occurs during a performance counter session, the flag is set
to 1.

If the session is then closed without calling PERFCNT_DUMP (which normally
clears it), the flag remains 1.

When a new session is started via panfrost_perfcnt_enable_locked(), will the
stale flag cause the wait timeout condition !ret &&
!atomic_read(&perfcnt->reset_happened) to evaluate to false, masking any
legitimate timeouts?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724-claude-fixes-v3-0-542af61abd0f@collabora.com?part=13