Re: [PATCH v14 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:
- [Medium] drm/panfrost: data race on perfcnt->state drops reset notifications
- [Medium] drm/panfrost: Race between delayed perfcnt interrupt and dump re-initialization
- [Low] drm/panfrost: cleanup subsystem violation mixing goto and scoped_guard
--

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

drm/panfrost: Fix races between perfcnt and reset sequence

Formerly, the reset sequence would race with panfrost_mmu_as_put()
when tearing down a perfcnt session. On top of that, poking GPU
registers to program a perfcnt session or obtaining a dump might lead to
undefined behaviour when done at the same time a reset was ongoing.

> 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

[ ... ]

> -static int panfrost_perfcnt_dump_locked(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;
> +
> +		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);

[Severity: Medium]
Is there a race condition between reinit_completion() and clearing pending
interrupts?

If a previous dump request timed out, a delayed interrupt could still be
raised by the hardware.

Since panfrost_perfcnt_dump_locked() executes without disabling hardirqs,
could a concurrent interrupt handler execute right after reinit_completion()
but before GPU_INT_CLEAR?

This would spuriously signal the completion and cause the subsequent wait to
return immediately while the hardware is still writing counter data.

> +		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;
> -	else if (ret > 0)
> -		ret = 0;
>  
> -	return ret;
> +	/* We need to restore perfcnt state so the next successful dump can relay it to UM */
> +	if (ret < 0) {
> +		perfcnt->state |= *state;

[Severity: Medium]
Does this unprotected read-modify-write drop reset notifications?

When panfrost_perfcnt_dump_locked() exits early with an error, the code has
already released the reset read semaphore. Concurrently, a reset might
happen and panfrost_perfcnt_reset() could write the interrupted flag to
perfcnt->state:

    perfcnt->state |= PANFROST_PERFCNT_SESSION_INTERRUPTED_BY_RESET;

Since the assignment here performs a non-atomic read-modify-write without
holding the semaphore, won't it clobber the reset flags written by the
reset sequence, hiding them from userspace?

> +		*state = 0;
> +		return ret;
> +	}

[ ... ]

> +	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 this goto violate the cleanup and guard subsystem rules?

The kernel documentation for the cleanup subsystem explicitly forbids mixing
traditional goto-based error handling and scope-based guard() declarations
in the same function. Mixing them creates confusing ownership semantics and
can lead to LIFO ordering bugs on scope exit.

>  
> -	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);
>  
> -	perfcnt->user = user;
> -
>  	return 0;
>  
>  err_vunmap:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009-claude-fixes-v14-0-cb743b3b973b@collabora.com?part=15