Re: [PATCH v6 16/16] 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 1 potential issue(s) to consider:
- [High] Hardware performance counter initialization is skipped on new sessions due to an incorrect condition, leading to an MMU address space reference count underflow and resource leak.
--

commit 036d049b8b6f60dc9ba18742b07e01b404ef96c3
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. This commit introduces a read/write
semaphore to govern access to the hardware at reset time, and expands the
DRM uAPI for the perfcnt DUMP operation.

> diff --git a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> index b3f71d7fd82a9..7dcf27add7ae7 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
[ ... ]
> @@ -132,44 +206,20 @@ static int panfrost_perfcnt_enable_locked(struct panfrost_device *pfdev,
>  	gpu_write(pfdev, GPU_INT_CLEAR, GPU_IRQ_PERFCNT_SAMPLE_COMPLETED);
>  	gpu_write(pfdev, GPU_CMD, GPU_CMD_PERFCNT_CLEAR);
>  
> -	ret = panfrost_mmu_as_get(pfdev, perfcnt->mapping->mmu);
> -	if (ret < 0)
> -		goto err_vunmap;
> -
[ ... ]
> -	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) {
> +		if (perfcnt->reset_failed) {
> +			ret = panfrost_perfcnt_hw_enable(pfdev);
> +			if (ret)
> +				goto err_vunmap;
> +		}

[Severity: High]
Does this skip the initial hardware enablement on new sessions?  Since
perfcnt->reset_failed would be false initially, it seems
panfrost_perfcnt_hw_enable() is bypassed entirely, which also means we miss
taking the MMU AS reference via panfrost_mmu_as_get().

> +		perfcnt->reset_happened = false;
> +		perfcnt->reset_failed = false;
> +		perfcnt->user = user;
> +	}
>  
>  	/* The BO ref is retained by the mapping. */
>  	drm_gem_object_put(&bo->base);
[ ... ]
> @@ -195,13 +245,15 @@ static int panfrost_perfcnt_disable_locked(struct panfrost_device *pfdev,
>  	if (user != perfcnt->user)
>  		return -EINVAL;
>  
> -	panfrost_perfcnt_hw_disable(pfdev);
> +	scoped_guard(rwsem_read, &pfdev->reset.lock) {
> +		panfrost_perfcnt_hw_disable(pfdev);
> +		panfrost_mmu_as_put(pfdev, perfcnt->mapping->mmu);

[Severity: High]
If panfrost_perfcnt_hw_enable() was skipped in
panfrost_perfcnt_enable_locked(), will this unconditionally drop a reference
that was never acquired?  It looks like this could underflow the AS
reference count, which would prevent subsequent allocations since a negative
reference count isn't considered free.

> +		perfcnt->user = NULL;
> +	}
>  
> -	perfcnt->user = NULL;
>  	drm_gem_vunmap(&perfcnt->mapping->obj->base.base, &map);
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826-claude-fixes-v6-0-78119af409e7@collabora.com?part=16