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 <[email protected]> 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/[email protected]?part=16
