Re: [PATCH v11 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: resource leak in panfrost_perfcnt_enable_locked due to missing early return
- [Medium] drm/panfrost: data race on perfcnt->state in panfrost_perfcnt_enable_locked
- [Low] drm/panfrost: cleanup helper mixing in panfrost_perfcnt_enable_locked
--

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

This commit modifies the panfrost driver to fix races between perfcnt
teardown and the reset sequence. It introduces the use of a reset
read/write semaphore during perfcnt configuration and modifies the
uAPI to report reset states.

> diff --git a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> index b3f71d7fd82a9..96c3c41cd269e 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c

[ ... ]

>  static int panfrost_perfcnt_enable_locked(struct panfrost_device *pfdev,
>  					  struct drm_file *file_priv,
>  					  unsigned int counterset)
>  {
>  	struct panfrost_file_priv *user = file_priv->driver_priv;
>  	struct panfrost_perfcnt *perfcnt = pfdev->perfcnt;
> -	struct iosys_map map;
>  	struct drm_gem_shmem_object *bo;
> -	u32 cfg, as;
> +	struct iosys_map map;
>  	int ret;
>  
> -	if (user == perfcnt->user)
> -		return 0;
> -	else if (perfcnt->user)
> -		return -EBUSY;
> +	if (perfcnt->user) {
> +		if (perfcnt->user != user)
> +			return -EBUSY;
> +
> +		if (perfcnt->state & PANFROST_PERFCNT_SESSION_DEAD)
> +			drm_WARN_ON(&pfdev->base,
> +				    panfrost_perfcnt_disable_locked(pfdev, file_priv));
> +	}

[Severity: High]
Could this missing early return sequence cause a resource leak?

When panfrost_perfcnt_enable_locked() is called on an already active session,
if the session is not dead, the function falls through to resource allocation
because there is no early return. This could lead to a memory leak, an
unbalanced power management reference count, and dangling pointers by
allocating a new GEM object and mapping it without freeing the old session.

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

This function reads perfcnt->state before establishing the reset.lock via
scoped_guard. A concurrent GPU reset might be calling panfrost_perfcnt_reset()
and updating perfcnt->state. This could result in missing a concurrently set
PANFROST_PERFCNT_SESSION_DEAD flag, preventing a dead session from being
restarted properly.

>  
>  	ret = pm_runtime_get_sync(pfdev->base.dev);

[ ... ]

>  	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]
Are we mixing goto-based cleanup with scoped_guard declarations here?

The subsystem guide forbids mixing goto-based cleanup labels with scoped_guard
in the same function. Using goto err_vunmap inside the scoped_guard block
could violate ownership semantics and lead to subtle bugs.

>  
> -	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);

[ ... ]

> +void panfrost_perfcnt_reset(struct panfrost_device *pfdev)
> +{
> +	struct panfrost_perfcnt *perfcnt = pfdev->perfcnt;
> +
> +	if (drm_WARN_ON(&pfdev->base, !perfcnt))
> +		return;
> +
> +	lockdep_assert_held(&pfdev->reset.lock);
> +
> +	if (!perfcnt->user)
> +		return;
> +
> +	/* All active AS are released during the MMU post_reset. */
> +	perfcnt->owns_as_ref = false;
> +	perfcnt->state |= PANFROST_PERFCNT_SESSION_INTERRUPTED_BY_RESET;
> +	if (panfrost_perfcnt_hw_enable(pfdev))
> +		perfcnt->state |= PANFROST_PERFCNT_SESSION_DEAD;

[Severity: Medium]
This is the concurrent write to perfcnt->state that could race with the
unlocked read in panfrost_perfcnt_enable_locked() mentioned above.

> +
> +	/* Unblock pending sample requests. */
> +	complete(&perfcnt->dump_comp);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260925-claude-fixes-v11-0-0dbf5a58e7ce@collabora.com?part=15