[PATCH v1] drm/kmb: Handle DSI clock enable failures

[PATCH v1] drm/kmb: Handle DSI clock enable failures

[Yuho Choi]

Disable clocks that were enabled before a later DSI clock fails, and
track whether the complete DSI clock set is enabled so teardown does
not disable partially initialized clocks.

Propagate kmb_dsi_clk_init() failures through the display hardware
initialization path instead of continuing with a partially initialized
DSI device.

Fixes: 98521f4d4b4c ("drm/kmb: Mipi DSI part of the display driver")
Fixes: 7f7b96a8a0a1 ("drm/kmb: Add support for KeemBay Display")
Signed-off-by: Yuho Choi <dbgh9129@gmail.com>
---
 drivers/gpu/drm/kmb/kmb_drv.c | 6 +++++-
 drivers/gpu/drm/kmb/kmb_dsi.c | 9 +++++++++
 drivers/gpu/drm/kmb/kmb_dsi.h | 1 +
 3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/kmb/kmb_drv.c b/drivers/gpu/drm/kmb/kmb_drv.c
index 7c2eb1152fc2..fef76d6781b4 100644
--- a/drivers/gpu/drm/kmb/kmb_drv.c
+++ b/drivers/gpu/drm/kmb/kmb_drv.c
@@ -61,6 +61,8 @@ static int kmb_initialize_clocks(struct kmb_drm_private *kmb, struct device *dev
 	drm_info(&kmb->drm, "system clk = %d Mhz", kmb->sys_clk_mhz);
 
 	ret =  kmb_dsi_clk_init(kmb->kmb_dsi);
+	if (ret)
+		return ret;
 
 	/* Set LCD clock to 200 Mhz */
 	clk_set_rate(kmb->kmb_clk.clk_lcd, KMB_LCD_DEFAULT_CLK);
@@ -130,7 +132,9 @@ static int kmb_hw_init(struct drm_device *drm, unsigned long flags)
 		return ret;
 
 	/* Enable display clocks */
-	kmb_initialize_clocks(kmb, &pdev->dev);
+	ret = kmb_initialize_clocks(kmb, &pdev->dev);
+	if (ret)
+		return ret;
 
 	/* Register irqs here - section 17.3 in databook
 	 * lists LCD at 79 and 82 for MIPI under MSS CPU -
diff --git a/drivers/gpu/drm/kmb/kmb_dsi.c b/drivers/gpu/drm/kmb/kmb_dsi.c
index 59d0e856392f..07dc4e324426 100644
--- a/drivers/gpu/drm/kmb/kmb_dsi.c
+++ b/drivers/gpu/drm/kmb/kmb_dsi.c
@@ -175,9 +175,14 @@ mipi_hs_freq_range[MIPI_DPHY_DEFAULT_BIT_RATES] = {
 
 static void kmb_dsi_clk_disable(struct kmb_dsi *kmb_dsi)
 {
+	if (!kmb_dsi->dsi_clk_enabled)
+		return;
+
 	clk_disable_unprepare(kmb_dsi->clk_mipi);
 	clk_disable_unprepare(kmb_dsi->clk_mipi_ecfg);
 	clk_disable_unprepare(kmb_dsi->clk_mipi_cfg);
+
+	kmb_dsi->dsi_clk_enabled = false;
 }
 
 void kmb_dsi_host_unregister(struct kmb_dsi *kmb_dsi)
@@ -1494,15 +1499,19 @@ static int kmb_dsi_clk_enable(struct kmb_dsi *kmb_dsi)
 	ret = clk_prepare_enable(kmb_dsi->clk_mipi_ecfg);
 	if (ret) {
 		dev_err(dev, "Failed to enable MIPI_ECFG clock: %d\n", ret);
+		clk_disable_unprepare(kmb_dsi->clk_mipi);
 		return ret;
 	}
 
 	ret = clk_prepare_enable(kmb_dsi->clk_mipi_cfg);
 	if (ret) {
 		dev_err(dev, "Failed to enable MIPI_CFG clock: %d\n", ret);
+		clk_disable_unprepare(kmb_dsi->clk_mipi_ecfg);
+		clk_disable_unprepare(kmb_dsi->clk_mipi);
 		return ret;
 	}
 
+	kmb_dsi->dsi_clk_enabled = true;
 	dev_info(dev, "SUCCESS : enabled MIPI clocks\n");
 	return 0;
 }
diff --git a/drivers/gpu/drm/kmb/kmb_dsi.h b/drivers/gpu/drm/kmb/kmb_dsi.h
index 0faadeb9f1dd..8fafcf0d2a96 100644
--- a/drivers/gpu/drm/kmb/kmb_dsi.h
+++ b/drivers/gpu/drm/kmb/kmb_dsi.h
@@ -76,6 +76,7 @@ struct kmb_dsi {
 	struct clk *clk_mipi_ecfg;
 	struct clk *clk_mipi_cfg;
 	int sys_clk_mhz;
+	bool dsi_clk_enabled;
 };
 
 /* DPHY Tx test codes */
-- 
2.43.0

Re: [PATCH v1] drm/kmb: Handle DSI clock enable failures

[sashiko-bot]

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

Pre-existing issues:
- [High] Kernel panic due to ERR_PTR dereference when kmb_dsi_init() fails.
- [High] Singleton dsi_host prevents re-registration on unbind/rebind.
- [High] Severe device reference and devres leaks involving the DSI platform device.
--

--- Patch [1]: [PATCH v1] drm/kmb: Handle DSI clock enable failures ---
commit 91e06039c775a24291e2b65a4018e13b758801ff
Author: Yuho Choi <dbgh9129@gmail.com>

drm/kmb: Handle DSI clock enable failures
    
Disable clocks that were enabled before a later DSI clock fails, and
track whether the complete DSI clock set is enabled so teardown does
not disable partially initialized clocks. Propagate kmb_dsi_clk_init()
failures through the display hardware initialization path instead of
continuing with a partially initialized DSI device.

> diff --git a/drivers/gpu/drm/kmb/kmb_drv.c b/drivers/gpu/drm/kmb/kmb_drv.c
> index 7c2eb1152fc2f..fef76d6781b43 100644
> --- a/drivers/gpu/drm/kmb/kmb_drv.c
> +++ b/drivers/gpu/drm/kmb/kmb_drv.c
> @@ -61,6 +61,8 @@ static int kmb_initialize_clocks(struct kmb_drm_private *kmb, struct device *dev
>  	drm_info(&kmb->drm, "system clk = %d Mhz", kmb->sys_clk_mhz);
>  
>  	ret =  kmb_dsi_clk_init(kmb->kmb_dsi);
> +	if (ret)
> +		return ret;
>  
>  	/* Set LCD clock to 200 Mhz */
>  	clk_set_rate(kmb->kmb_clk.clk_lcd, KMB_LCD_DEFAULT_CLK);

[ ... ]

> diff --git a/drivers/gpu/drm/kmb/kmb_dsi.c b/drivers/gpu/drm/kmb/kmb_dsi.c
> index 59d0e856392fc..07dc4e324426d 100644
> --- a/drivers/gpu/drm/kmb/kmb_dsi.c
> +++ b/drivers/gpu/drm/kmb/kmb_dsi.c
> @@ -175,9 +175,14 @@ mipi_hs_freq_range[MIPI_DPHY_DEFAULT_BIT_RATES] = {
>  
>  static void kmb_dsi_clk_disable(struct kmb_dsi *kmb_dsi)
>  {
> +	if (!kmb_dsi->dsi_clk_enabled)
> +		return;

[Severity: High]
This isn't a bug introduced by this patch, but will this cause a kernel
panic if kmb_dsi_init() fails?

Looking at kmb_probe(), if DSI initialization fails, it returns an
ERR_PTR which is stored in kmb->kmb_dsi:

drivers/gpu/drm/kmb/kmb_drv.c:kmb_probe() {
    ...
    kmb->kmb_dsi = kmb_dsi_init(dsi_pdev);
    if (IS_ERR(kmb->kmb_dsi)) {
        drm_err(&kmb->drm, "failed to initialize DSI\n");
        ret = PTR_ERR(kmb->kmb_dsi);
        goto err_free1;
    }

The error handler then jumps to err_free1:

err_free1:
    dev_set_drvdata(dev, NULL);
    kmb_dsi_host_unregister(kmb->kmb_dsi);
}

Then kmb_dsi_host_unregister() calls kmb_dsi_clk_disable() where
kmb_dsi is dereferenced. Can we add an IS_ERR_OR_NULL() check here,
or avoid calling unregister on the error pointer in the probe cleanup path?

[Severity: High]
This is a pre-existing issue, but does unbinding and rebinding the driver
permanently break the display?

Looking at kmb_dsi_host_bridge_init(), it registers a static global
dsi_host:

drivers/gpu/drm/kmb/kmb_dsi.c:kmb_dsi_host_bridge_init() {
    if (!dsi_host) {
        dsi_host = kzalloc_obj(*dsi_host);
        ... 
        mipi_dsi_host_register(dsi_host);
    }
}

During unbind, kmb_dsi_host_unregister() unregisters the host, but the
static pointer is never cleared. When the driver is rebound,
kmb_dsi_host_bridge_init() sees dsi_host is not NULL and skips
registration, leaving the host unregistered and causing subsequent
operations to fail. Is there a missing reset of this global variable?

[Severity: High]
This is another pre-existing issue, but does kmb_probe() permanently leak
memory and device references if probe is deferred?

kmb_probe() looks up the DSI platform device and takes references without
releasing them on error or unbind paths:

drivers/gpu/drm/kmb/kmb_drv.c:kmb_probe() {
    dsi_pdev = of_find_device_by_node(dsi_node);
    ...
    ret = kmb_dsi_host_bridge_init(get_device(&dsi_pdev->dev));
}

Furthermore, it allocates managed resources against this foreign DSI
device:

drivers/gpu/drm/kmb/kmb_dsi.c:kmb_dsi_init() {
    struct device *dev = get_device(&pdev->dev);
    kmb_dsi = devm_kzalloc(dev, sizeof(*kmb_dsi), GFP_KERNEL);
    ...
}

Because the foreign DSI device is never unbound when the DRM driver defers
or fails, do these devres resources and device references permanently
leak on every probe deferral?

>  	clk_disable_unprepare(kmb_dsi->clk_mipi);
>  	clk_disable_unprepare(kmb_dsi->clk_mipi_ecfg);
>  	clk_disable_unprepare(kmb_dsi->clk_mipi_cfg);
> +
> +	kmb_dsi->dsi_clk_enabled = false;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260802002512.486369-1-dbgh9129@gmail.com?part=1