Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Medium] drm/panfrost: data race on perfcnt->state drops reset notifications - [Medium] drm/panfrost: Race between delayed perfcnt interrupt and dump re-initialization - [Low] drm/panfrost: cleanup subsystem violation mixing goto and scoped_guard --
commit a3c7a0f93b13c20ffb612831ca0de295364a95de 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..a9def452d5ef9 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c > +++ b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c [ ... ] > -static int panfrost_perfcnt_dump_locked(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; > + > + 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); [Severity: Medium] Is there a race condition between reinit_completion() and clearing pending interrupts? If a previous dump request timed out, a delayed interrupt could still be raised by the hardware. Since panfrost_perfcnt_dump_locked() executes without disabling hardirqs, could a concurrent interrupt handler execute right after reinit_completion() but before GPU_INT_CLEAR? This would spuriously signal the completion and cause the subsequent wait to return immediately while the hardware is still writing counter data. > + 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)); > + > if (!ret) > ret = -ETIMEDOUT; > - else if (ret > 0) > - ret = 0; > > - return ret; > + /* We need to restore perfcnt state so the next successful dump can > relay it to UM */ > + if (ret < 0) { > + perfcnt->state |= *state; [Severity: Medium] Does this unprotected read-modify-write drop reset notifications? When panfrost_perfcnt_dump_locked() exits early with an error, the code has already released the reset read semaphore. Concurrently, a reset might happen and panfrost_perfcnt_reset() could write the interrupted flag to perfcnt->state: perfcnt->state |= PANFROST_PERFCNT_SESSION_INTERRUPTED_BY_RESET; Since the assignment here performs a non-atomic read-modify-write without holding the semaphore, won't it clobber the reset flags written by the reset sequence, hiding them from userspace? > + *state = 0; > + return ret; > + } [ ... ] > + 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] Does this goto violate the cleanup and guard subsystem rules? The kernel documentation for the cleanup subsystem explicitly forbids mixing traditional goto-based error handling and scope-based guard() declarations in the same function. Mixing them creates confusing ownership semantics and can lead to LIFO ordering bugs on scope exit. > > - 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); > > - perfcnt->user = user; > - > return 0; > > err_vunmap: -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=15
