Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] drm/panfrost: perfcnt state flag lost when dump ioctl is interrupted by signal - [Low] drm/panfrost: goto mixing with scoped_guard in panfrost_perfcnt_enable_locked --
commit fdd119adb59d54bffaacf2ad45f065092d48a919 Author: Adrián Larumbe <[email protected]> Subject: drm/panfrost: Fix races between perfcnt and reset sequence This commit uses the reset r/w semaphore when disabling and re-enabling perfcnt configuration, and in the enable/dump ioctls where device registers are being accessed. It also expands the DRM uAPI for the perfcnt dump operation so that user space is aware of hardware resets. Finally, it bumps the DRM driver minor number. > diff --git a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c > b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c > index b3f71d7fd82a9..13521e078ce76 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c > +++ b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c [ ... ] > @@ -58,43 +70,161 @@ void panfrost_perfcnt_sample_done(struct panfrost_device > *pfdev) [ ... ] > +static int panfrost_perfcnt_dump_locked(struct panfrost_device *pfdev, u32 > *state) > { > - u64 gpuva; > + struct panfrost_perfcnt *perfcnt = pfdev->perfcnt; > + u64 gpuva = perfcnt->mapping->mmnode.start << PAGE_SHIFT; > int ret; > > - reinit_completion(&pfdev->perfcnt->dump_comp); > - gpuva = pfdev->perfcnt->mapping->mmnode.start << PAGE_SHIFT; > - gpu_write(pfdev, GPU_PERFCNT_BASE_LO, lower_32_bits(gpuva)); > - gpu_write(pfdev, GPU_PERFCNT_BASE_HI, upper_32_bits(gpuva)); > - gpu_write(pfdev, GPU_INT_CLEAR, > - GPU_IRQ_CLEAN_CACHES_COMPLETED | > - GPU_IRQ_PERFCNT_SAMPLE_COMPLETED); > - gpu_write(pfdev, GPU_CMD, GPU_CMD_PERFCNT_SAMPLE); > + scoped_guard(rwsem_read, &pfdev->reset.lock) { > + *state = perfcnt->state; > + if (perfcnt->state & PANFROST_PERFCNT_SESSION_DEAD) > + return -EIO; > + > + perfcnt->state = 0; [Severity: Medium] If wait_for_completion_interruptible_timeout() is interrupted by a signal, it returns -ERESTARTSYS. The syscall will be restarted, but the internal state (perfcnt->state) has been cleared here and is never restored on the error path. Does this mean the restarted ioctl will read a clean state and lose the reset flag that was present before the wait? > + > + reinit_completion(&pfdev->perfcnt->dump_comp); > + > + gpu_write(pfdev, GPU_PERFCNT_BASE_LO, lower_32_bits(gpuva)); > + gpu_write(pfdev, GPU_PERFCNT_BASE_HI, upper_32_bits(gpuva)); > + gpu_write(pfdev, GPU_INT_CLEAR, GPU_IRQ_CLEAN_CACHES_COMPLETED | > + > GPU_IRQ_PERFCNT_SAMPLE_COMPLETED); > + gpu_write(pfdev, GPU_CMD, GPU_CMD_PERFCNT_SAMPLE); > + } > + > + /* > + * 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) > + return -ETIMEDOUT; > + > + scoped_guard(rwsem_read, &pfdev->reset.lock) { > + *state |= perfcnt->state; > + > + /* UM must re-enable their session before requesting new dumps. > */ > + if (perfcnt->state & PANFROST_PERFCNT_SESSION_DEAD) > + return -EIO; > + > + /* If we faced a reset during our SAMPLE, the user needs to try > again. */ > + if (perfcnt->state & > PANFROST_PERFCNT_SESSION_INTERRUPTED_BY_RESET) > + return -EAGAIN; > + > + /* Only when we know no re-eanble or re-dump is required, we > can afford > + * to reset the internal state. Otherwise it must be kept so > that later > + * ioctls know about error situations in this DUMP and work > around it. > + */ > + perfcnt->state = 0; > + } > + > + if (ret > 0) > ret = 0; > > return ret; > } [ ... ] > @@ -122,54 +252,30 @@ static int panfrost_perfcnt_enable_locked(struct > panfrost_device *pfdev, [ ... ] > panfrost_gem_internal_set_label(&bo->base, "Perfcnt sample buffer"); > > - /* > - * 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); [ ... ] > + 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); > > - ret = panfrost_perfcnt_hw_enable(pfdev); > + ret = panfrost_perfcnt_hw_enable(pfdev); > + if (ret) > + goto err_vunmap; [Severity: Low] Are we mixing goto-based cleanup with scope-based cleanup helpers here? The Linux kernel cleanup subsystem guidelines explicitly forbid mixing goto-based error handling with __free() or guard() cleanups in the same function to prevent ownership confusion. Should this function be updated to use either scope-based cleanups consistently, or traditional goto-based cleanups? > > - 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); -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=15
