diff mbox series

[v2,6/11] drm: ingenic: Use devm_of_reserved_mem_device_init()

Message ID 20260902201640.2024648-7-mukesh.ojha@oss.qualcomm.com (mailing list archive)
State New
Headers
Series drm: Use devm_of_reserved_mem_device_init() |

Commit Message

Mukesh Ojha Sept. 2, 2026, 8:16 p.m. UTC 
Replace the hand-rolled devm wrapper (ingenic_drm_release_rmem +
devm_add_action_or_reset) with the standard
devm_of_reserved_mem_device_init(), letting the device resource
manager handle cleanup automatically.

Reviewed-by: Paul Cercueil <paul@crapouillou.net>
Signed-off-by: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>
---
 drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

Comments

sashiko-bot@kernel.org Sept. 2, 2026, 8:27 p.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Using devres (`devm_*`) allocations in a component bind callback causes resource leaks and rebind failures.
- [High] Hardware clocks are disabled before shutting down the DRM pipeline, causing a bus hang on unbind.
- [High] `component_unbind_all()` is executed from a devres cleanup action without holding `component_mutex`, triggering a kernel warning and data race.
--

commit c7a21593821addf1c793b56266b4eca7b14e1ad2
Author: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>

drm: ingenic: Use devm_of_reserved_mem_device_init()

Replace the hand-rolled devm wrapper (ingenic_drm_release_rmem +
devm_add_action_or_reset) with the standard
devm_of_reserved_mem_device_init(), letting the device resource
manager handle cleanup automatically.

> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index 738a80b2550ff..2c4dd6ed2d413 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> @@ -1060,11 +1060,6 @@ static void ingenic_drm_unbind_all(void *d)
>  	component_unbind_all(priv->dev, &priv->drm);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but is component_unbind_all()
executed from a devres cleanup action without holding component_mutex?

The driver registers ingenic_drm_unbind_all() as a devres action via
devm_add_action_or_reset(). When the platform device is removed, the driver
core executes devres cleanup actions after ->remove() returns.

At this point, the component framework is inactive and component_mutex is not
held, but ingenic_drm_unbind_all() directly calls component_unbind_all(),
which strictly requires this mutex.

Could this trigger a kernel warning (WARN_ON(!mutex_is_locked(&component_mutex))),
and potential list corruption or use-after-free due to concurrent modification
of the component lists without proper serialization?

[ ... ]

> @@ -1127,16 +1122,10 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>  	}
>  
>  	if (IS_ENABLED(CONFIG_OF_RESERVED_MEM)) {
> -		ret = of_reserved_mem_device_init(dev);
> +		ret = devm_of_reserved_mem_device_init(dev);
>  
>  		if (ret && ret != -ENODEV)
>  			dev_warn(dev, "Failed to get reserved memory: %d\n", ret);
> -
> -		if (!ret) {
> -			ret = devm_add_action_or_reset(dev, ingenic_drm_release_rmem, dev);
> -			if (ret)
> -				return ret;
> -		}
>  	}
>  
>  	priv = devm_drm_dev_alloc(dev, &ingenic_drm_driver_data,

[Severity: High]
This is a pre-existing issue, but does using devres (devm_*) allocations in a
component bind callback cause resource leaks and rebind failures?

When the component framework unbinds a device (e.g., if the IPU component is
removed), ingenic_drm_unbind() is called, but it does not trigger devres cleanup
since devres is tied to the platform device, not the component.

When it binds again, ingenic_drm_bind() runs, leaking previous devres
allocations like devm_drm_dev_alloc(). This also causes failures when requesting
exclusive resources again, such as:

ingenic_drm_bind()
    ...
    base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);

This will fail on rebind because the MMIO region remains held by the leaked
previous allocation. Can this be triggered locally by a privileged user
unbinding the IPU component or the display driver via sysfs?


While reviewing this file, I noticed another pre-existing issue:

[Severity: High]
This isn't a bug introduced by this patch, but are hardware clocks disabled
before shutting down the DRM pipeline, causing a bus hang on unbind?

In ingenic_drm_unbind():

    clk_disable_unprepare(priv->pix_clk);

    drm_dev_unregister(&priv->drm);
    drm_atomic_helper_shutdown(&priv->drm);

When the driver is unbound while the display is active, it first disables
lcd_clk and pix_clk. Immediately after, it calls drm_atomic_helper_shutdown(),
which iterates over active CRTCs and planes to disable them.

These disable callbacks (like ingenic_drm_bridge_atomic_disable() and
ingenic_drm_plane_disable()) perform MMIO writes via regmap_update_bits() and
regmap_clear_bits().

Will accessing MMIO registers while the peripheral's clock is gated cause a
system bus hang (hard freeze or kernel panic) when a privileged user unloads
the driver module or unbinds the device via sysfs?
diff mbox series

Patch

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index 738a80b2550f..2c4dd6ed2d41 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -1060,11 +1060,6 @@  static void ingenic_drm_unbind_all(void *d)
 	component_unbind_all(priv->dev, &priv->drm);
 }
 
-static void __maybe_unused ingenic_drm_release_rmem(void *d)
-{
-	of_reserved_mem_device_release(d);
-}
-
 static void ingenic_drm_configure_hwdesc(struct ingenic_drm *priv,
 					 unsigned int hwdesc,
 					 unsigned int next_hwdesc, u32 id)
@@ -1127,16 +1122,10 @@  static int ingenic_drm_bind(struct device *dev, bool has_components)
 	}
 
 	if (IS_ENABLED(CONFIG_OF_RESERVED_MEM)) {
-		ret = of_reserved_mem_device_init(dev);
+		ret = devm_of_reserved_mem_device_init(dev);
 
 		if (ret && ret != -ENODEV)
 			dev_warn(dev, "Failed to get reserved memory: %d\n", ret);
-
-		if (!ret) {
-			ret = devm_add_action_or_reset(dev, ingenic_drm_release_rmem, dev);
-			if (ret)
-				return ret;
-		}
 	}
 
 	priv = devm_drm_dev_alloc(dev, &ingenic_drm_driver_data,