Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] Hardware initialization is bypassed on new perfcnt sessions, breaking performance counters. - [High] Unconditional MMU AS reference put causes reference count underflow. - [Medium] Mixing goto-based error handling with scoped_guard violates the kernel cleanup API. --
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. 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..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; > - [ ... ] > + 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 code inadvertently skip hardware initialization for new perfcnt sessions? Earlier in panfrost_perfcnt_enable_locked(), the function returns early if perfcnt->user is not NULL. Because reset_failed is only set during a reset while a session is already active, reset_failed is guaranteed to be false when creating a new session. Consequently, panfrost_perfcnt_hw_enable() is never called here, the MMU address space is not acquired, and hardware performance counters are never enabled. [Severity: Medium] Is it safe to mix goto-based error handling with scoped_guard() blocks here? According to the kernel cleanup subsystem guidelines, the usage of "goto" and cleanup helpers should never be mixed in the same function to prevent confusing ownership semantics and potential resource leak bugs. The jump to err_vunmap directly exits the scoped_guard block, breaking this rule. > + 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] Can this unconditional call to panfrost_mmu_as_put() cause a reference count underflow regression? If panfrost_perfcnt_hw_enable() was skipped during initialization as noted above, or if it failed to re-acquire the AS during reset recovery, the perfcnt session does not actually hold an MMU AS reference. When a userspace client closes the perfcnt session, panfrost_mmu_as_put() is called unconditionally, which will drive the mmu->as_count reference count to -1 and trigger a kernel warning. > + perfcnt->user = NULL; > + } > > - perfcnt->user = NULL; > drm_gem_vunmap(&perfcnt->mapping->obj->base.base, &map); > perfcnt->buf = NULL; > panfrost_gem_close(&perfcnt->mapping->obj->base.base, file_priv); > - panfrost_mmu_as_put(pfdev, perfcnt->mapping->mmu); > panfrost_gem_mapping_put(perfcnt->mapping); > perfcnt->mapping = NULL; > pm_runtime_put_autosuspend(pfdev->base.dev); -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=15
