Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] drm/panfrost: unnormalized positive return value causes ioctl to skip copy_to_user - [High] drm/panfrost: missing state reset causes permanent DoS for perfcnt after SESSION_DEAD - [Low] drm/panfrost: mixing goto and scoped_guard violates cleanup subsystem rules --
commit 3965f7ad946ecef84cbe249a17983ab972ee3c12 Author: Adrián Larumbe <[email protected]> drm/panfrost: Fix races between perfcnt and reset sequence This commit uses the reset r/w semaphore to govern access to the hardware at reset time, preventing races with tearing down perfcnt sessions or ongoing hardware resets. It also expands the DRM uAPI for the perfcnt DUMP operation to notify userspace if a reset occurred. > diff --git a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c > b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c > index b3f71d7fd82a9..91983d450c246 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c > +++ b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c [ ... ] > @@ -58,25 +69,100 @@ void panfrost_perfcnt_sample_done(struct panfrost_device > *pfdev) [ ... ] > +static int panfrost_perfcnt_dump_locked(struct panfrost_device *pfdev, u32 > *state) > +{ > + struct panfrost_perfcnt *perfcnt = pfdev->perfcnt; > + u64 gpuva = perfcnt->mapping->mmnode.start << PAGE_SHIFT; > + int ret; > + > + scoped_guard(rwsem_read, &pfdev->reset.lock) { > + *state = perfcnt->state; > + if (perfcnt->state & PANFROST_PERFCNT_SESSION_DEAD) > + return -EIO; > + > + perfcnt->state = 0; [Severity: High] Does this permanently lock out the perfcnt session if it dies? If PANFROST_PERFCNT_SESSION_DEAD is set, the code returns -EIO here before reaching `perfcnt->state = 0`. Since panfrost_perfcnt_enable_locked() also does not clear `perfcnt->state` when starting a new session, won't this leave the dead state stuck permanently and fail all future dump attempts? [ ... ] > + /* > + * 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)); > + > + /* A reset might come through in the gap between the completion > returning and the following > + * check, but because no sample was produced, we don't care to relay > the state back to UM > + */ > if (!ret) > - ret = -ETIMEDOUT; > - else if (ret > 0) > - ret = 0; > + return -ETIMEDOUT; [Severity: High] Will this return the remaining jiffies as a positive value on success? Since the previous code `else if (ret > 0) ret = 0;` was removed, a successful completion will now return a positive number. When this propagates back up to the caller: panfrost_ioctl_perfcnt_dump() ret = panfrost_perfcnt_dump_locked(pfdev, &req->state); if (ret) goto out; won't this positive return value be treated as an error by the `if (ret)` check? If so, this skips the copy_to_user() call entirely and prevents userspace from receiving the requested performance counter data. [ ... ] > @@ -87,9 +173,8 @@ static int panfrost_perfcnt_enable_locked(struct > panfrost_device *pfdev, [ ... ] > + 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] Is it intended to use a goto statement inside a scoped_guard block? The cleanup subsystem guidelines indicate that goto-based error handling and scope-based cleanup helpers shouldn't be mixed in the same function to avoid confusing ownership semantics. Should this be refactored to avoid jumping out of the scope to the err_vunmap label? > > - if (panfrost_has_hw_issue(pfdev, HW_ISSUE_8186)) > - gpu_write(pfdev, GPU_PRFCNT_TILER_EN, 0xffffffff); > + perfcnt->user = user; > + } > > /* The BO ref is retained by the mapping. */ > drm_gem_object_put(&bo->base); -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=15
