Re: [PATCH v9 06/16] drm/panfrost: Fix PM refcnt and autosuspend issues at device probe/remove

[sashiko-bot@kernel.org]

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 <adrian.larumbe@collabora.com>

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/20260912-claude-fixes-v9-0-e588feaa61ef@collabora.com?part=6