Re: [PATCH v2] drm/arm/komeda: fix error handling for clk_prepare_enable() and callers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] Skipping drm_mode_config_helper_resume() upon error leaks the atomic suspend state and breaks future system suspends.

Pre-existing issues:
- [High] `clk_prepare_enable()` return value is ignored in `komeda_dev_create()` and `komeda_dev_destroy()`, leading to hardware access without clocks.
- [High] `pm_runtime_get_sync()` return value is ignored in `komeda_crtc_atomic_enable()`, leading to unclocked hardware access.
- [High] Ignoring IOMMU connection failure in `komeda_dev_resume()` allows IOMMU bypass.
- [High] `komeda_crtc_prepare()` inadvertently ignores `clk_prepare_enable()` failures due to variable overwrite.
--

commit c3f7a8cea64f30e941bea95c0ba39f80812bdd7f
Author: Gustavo Kenji Mendonça Kaneko <kaneko.dev@pm.me>

drm/arm/komeda: fix error handling for clk_prepare_enable() and callers

komeda_dev_resume() calls clk_prepare_enable() without checking the
return value. If the clock fails to enable, the function returns 0
(success) while IRQs are enabled and IOMMU is connected on potentially
unclocked hardware, causing undefined behavior on resume.

> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_dev.c b/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
> index 5ba62e637a616..9aad1d1d28ec0 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
> @@ -313,7 +313,11 @@ void komeda_dev_destroy(struct komeda_dev *mdev)
>  
>  int komeda_dev_resume(struct komeda_dev *mdev)
>  {
> -	clk_prepare_enable(mdev->aclk);
> +	int err;
> +
> +	err = clk_prepare_enable(mdev->aclk);
> +	if (err)
> +		return err;
>  
>  	mdev->funcs->enable_irq(mdev);
>  

[Severity: High]
This is a pre-existing issue, but does ignoring the connect_iommu() failure
later in komeda_dev_resume() allow the display controller to bypass the
IOMMU?

If mdev->funcs->connect_iommu(mdev) fails, the function prints an error
but still returns 0 (success). The PM framework considers the resume
successful, and the display controller pipelines are programmed with
IOVAs.

Because the IOMMU translation buffer unit failed to connect, it might
bypass translation and treat the programmed IOVAs as raw physical
addresses, potentially allowing unintended memory access.

> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> index 4bb5f250e95ef..67fffab018ae8 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
[ ... ]
> @@ -140,11 +143,12 @@ static int __maybe_unused komeda_pm_suspend(struct device *dev)
>  static int __maybe_unused komeda_pm_resume(struct device *dev)
>  {
>  	struct komeda_drv *mdrv = dev_get_drvdata(dev);
> +	int err = 0;
>  
>  	if (!pm_runtime_status_suspended(dev))
> -		komeda_dev_resume(mdrv->mdev);
> +		err = komeda_dev_resume(mdrv->mdev);
>  
> -	return drm_mode_config_helper_resume(&mdrv->kms->base);
> +	return err ? err : drm_mode_config_helper_resume(&mdrv->kms->base);
>  }

[Severity: High]
If komeda_dev_resume() fails, will skipping drm_mode_config_helper_resume()
leak the atomic suspend state?

By returning early, dev->mode_config.suspend_state is left allocated and
non-NULL. When the system attempts to suspend again,
drm_mode_config_helper_suspend() hits a WARN_ON for the suspend_state
check and returns -EINVAL, which permanently prevents the system from
entering a suspended state again. The error path should explicitly drop
the reference to the atomic state to prevent a memory leak and state
corruption.

[Severity: High]
This isn't a bug introduced by this patch, but does komeda_dev_destroy()
ignore the clock enable failure?

drivers/gpu/drm/arm/display/komeda/komeda_dev.c:komeda_dev_destroy() {
        ...
        if (mdev->aclk)
            clk_prepare_enable(mdev->aclk);
        ...
}

Similarly in komeda_dev_create(), clk_prepare_enable() is called without
checking the return value. If it fails, the subsequent call to
komeda_identify() accesses unclocked hardware registers, which can lead
to a synchronous external abort.

In komeda_dev_destroy(), the unconditional call to clk_disable_unprepare()
on a failed clock could underflow the clock's reference count.

[Severity: High]
This isn't a bug introduced by this patch, but does komeda_crtc_prepare()
inadvertently ignore clk_prepare_enable() failures?

drivers/gpu/drm/arm/display/komeda/komeda_crtc.c:komeda_crtc_prepare() {
        err = clk_prepare_enable(mdev->aclk);
        if (err)
            drm_err(drm, "failed to enable aclk.\n");
        
        err = clk_set_rate(master->pxlclk, mode->crtc_clock * 1000);
        ...
}

The return value of clk_prepare_enable() is stored in err, but err is
immediately overwritten by clk_set_rate(). If the pixel clock
operations succeed, the function returns 0 even though the main engine
clock failed to enable, allowing the display pipeline to proceed and
access unclocked hardware.

[Severity: High]
This is a pre-existing issue, but does komeda_crtc_atomic_enable() ignore
the pm_runtime_get_sync() return value?

drivers/gpu/drm/arm/display/komeda/komeda_crtc.c:
komeda_crtc_atomic_enable() {
        ...
        pm_runtime_get_sync(crtc->dev->dev);
        komeda_crtc_prepare(to_kcrtc(crtc));
        ...
}

If the device fails to resume, the driver still proceeds to call
komeda_crtc_prepare() and komeda_crtc_do_flush(), which access hardware
registers. Accessing registers while the device is suspended or
unclocked can cause a bus fault. Ignoring the error also leaves the
runtime PM usage counter unbalanced.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609130828.1066038-1-kaneko.dev@pm.me?part=1