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

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 <[email protected]>

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/[email protected]?part=1