Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues: - [High] Setting PM state to suspended before hardware teardown completes causes bus errors. Pre-existing issues: - [High] Failure of pm_runtime_get_sync() is ignored in panfrost_remove(), which can lead to fatal MMIO accesses. - [High] Concurrent IOCTLs during device removal can cause Use-After-Free or hardware faults. - [High] Interrupt handlers are not synchronized before hardware clocks are disabled and scheduler structures are destroyed. -- commit fc7ca29ac5bbdcbf8ed388bccbba4ac531c7ed14 Author: Adrián Larumbe <[email protected]> drm/panfrost: Fix PM refcnt and autosuspend issues at device probe/remove This patch fixes PM refcnt and autosuspend issues during device probe and removal in the panfrost driver. It ensures the PM usage count is properly managed so the device does not remain powered on forever or increase its refcnt monotonically. > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c > b/drivers/gpu/drm/panfrost/panfrost_drv.c > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c [ ... ] > @@ -866,13 +867,16 @@ static int panfrost_probe(struct platform_device *pdev) > if (err < 0) > goto err_out1; > > + pm_runtime_put_autosuspend(pfdev->base.dev); > > return 0; > > err_out1: > + pm_runtime_dont_use_autosuspend(pfdev->base.dev); > pm_runtime_disable(pfdev->base.dev); > - panfrost_device_fini(pfdev); > + pm_runtime_put_noidle(pfdev->base.dev); > pm_runtime_set_suspended(pfdev->base.dev); > + panfrost_device_fini(pfdev); [Severity: High] Does moving pm_runtime_set_suspended() before panfrost_device_fini() cause bus errors? pm_runtime_set_suspended() internally drops the PM runtime active reference on the PM domains via rpm_put_suppliers(). Because the PM domains may power off asynchronously or synchronously when their usage count drops, they can be disabled while panfrost_device_fini() is still executing. Subsequently, functions like panfrost_jm_fini() and panfrost_mmu_fini() perform direct MMIO accesses on unclocked or powered-down hardware, which causes a synchronous external abort and a kernel panic. Note that this bug is not fixed in the subsequent patches of the series (e.g. Patch 8 just moves the buggy sequence into panfrost_device_fini). > err_out0: > return err; > } > @@ -884,9 +888,11 @@ static void panfrost_remove(struct platform_device *pdev) > drm_dev_unregister(&pfdev->base); [Severity: High] This isn't a bug introduced by this patch, but does drm_dev_unregister() prevent concurrent IOCTLs from open file descriptors? drm_dev_unregister() removes the sysfs node but already-open file descriptors can still execute IOCTLs (e.g. panfrost_ioctl_submit). Because Panfrost does not use drm_dev_unplug() and drm_dev_enter()/exit() in its IOCTL paths, a concurrent IOCTL can proceed, access the DRM scheduler, or write to MMIO while panfrost_device_fini() destroys the schedulers and disables clocks, causing a Use-After-Free or a kernel panic. Note that this missing synchronization remains unfixed at the end of the patch series. > > pm_runtime_get_sync(pfdev->base.dev); [Severity: High] This isn't a bug introduced by this patch, but does ignoring the return value of pm_runtime_get_sync() lead to fatal MMIO accesses? If resuming fails, the device remains unclocked. Execution proceeds unconditionally into panfrost_device_fini(), which performs MMIO writes to the hardware. Writing to MMIO registers when the device is powered off causes a bus fault / SError, resulting in a kernel panic. Although remove() cannot return an error, the code must check the return value and skip hardware accesses if resuming failed. Note that a follow-up patch (Patch 9) wraps this call in a drm_WARN_ON, but it still does not handle the error gracefully by aborting the hardware teardown steps, so the crash still occurs. > + pm_runtime_dont_use_autosuspend(pfdev->base.dev); > + pm_runtime_put_noidle(pfdev->base.dev); > pm_runtime_disable(pfdev->base.dev); > - panfrost_device_fini(pfdev); > pm_runtime_set_suspended(pfdev->base.dev); > + panfrost_device_fini(pfdev); [Severity: High] This isn't a bug introduced by this patch, but does this teardown sequence properly synchronize interrupts? panfrost_device_fini() calls panfrost_mmu_fini() and panfrost_jm_fini(), which mask interrupts via MMIO writes (e.g. mmu_write(pfdev, MMU_INT_MASK, 0)), but they do not call synchronize_irq(). If the IRQ handler thread is running concurrently on another CPU, it will continue executing and attempt to read MMIO registers with clocks disabled or access destroyed scheduler queues, leading to a UAF or kernel panic. This teardown synchronization issue remains unfixed at the end of the patch series. Also, as noted in the panfrost_probe() error path, moving pm_runtime_set_suspended() before panfrost_device_fini() risks disabling PM domains while teardown is still ongoing, leading to bus errors. -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6
