Re: [PATCH v3 6/6] drm/panel: Use devm_mipi_dsi_attach in assorted panels

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

New issues:
- [Low] Unused local variable 'dev' left behind after removing error handling.

Pre-existing issues:
- [High] Missing drm_panel_remove() on devm_mipi_dsi_attach() failure causes Use-After-Free.
- [High] DCS commands sent over a detached DSI bus during st7701 cleanup.
--

--- Patch [6]: [PATCH v3 6/6] drm/panel: Use devm_mipi_dsi_attach in assorted panels ---
commit 7405393b6171044f855c59f559f223d4f0b22c36
Author: Osama Abdelkader <[email protected]>

drm/panel: Use devm_mipi_dsi_attach in assorted panels

This commit converts multiple panel drivers to use the managed
devm_mipi_dsi_attach helper. It drops the corresponding manual detach
calls from the remove paths of these drivers.

> diff --git a/drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c b/drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c
> index db006576d7046..63359416944fe 100644
> --- a/drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c
> +++ b/drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c
[ ... ]
> @@ -264,7 +264,7 @@ static int tm5p5_nt35596_probe(struct mipi_dsi_device *dsi)
>  
>  	drm_panel_add(&ctx->panel);
>  
> -	ret = mipi_dsi_attach(dsi);
> +	ret = devm_mipi_dsi_attach(&dsi->dev, dsi);
>  	if (ret < 0) {

[Severity: High]
This isn't a bug introduced by this patch, but if the attach fails here,
does this return the error code without calling drm_panel_remove()?

Because the panel struct was allocated using devm_drm_panel_alloc(), devres
will automatically free the memory upon probe failure. However, the panel
would remain linked in the global panel_list, which could cause a
use-after-free when the list is iterated.

This pattern appears to be present in multiple drivers modified in this
patch (like panel-lg-sw43408.c, panel-summit.c, and panel-tdo-tl070wsh30.c).

>  		dev_err(dev, "Failed to attach to DSI host: %d\n", ret);
>  		return ret;
>  	}

[ ... ]
> diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7701.c b/drivers/gpu/drm/panel/panel-sitronix-st7701.c
> index f16e0de1ea606..b45e2e683fe9f 100644
> --- a/drivers/gpu/drm/panel/panel-sitronix-st7701.c
> +++ b/drivers/gpu/drm/panel/panel-sitronix-st7701.c
[ ... ]
> @@ -1347,7 +1347,7 @@ static int st7701_dsi_probe(struct mipi_dsi_device *dsi)
>  	dsi->format = st7701->desc->format;
>  	dsi->lanes = st7701->desc->lanes;
>  
> -	err = mipi_dsi_attach(dsi);
> +	err = devm_mipi_dsi_attach(&dsi->dev, dsi);

[Severity: High]
This is a pre-existing issue, but does this lead to DCS commands being
sent over a detached DSI bus during cleanup?

The st7701_probe() function registers the st7701_cleanup() devres action
which eventually calls drm_panel_unprepare() and sends a sleep command.
Because devm_mipi_dsi_attach() is called after st7701_probe(), the detach
devres action is registered last.

Since devres executes in LIFO order, the bus will be detached before
st7701_cleanup() attempts to send the sleep command.

>  	if (err)
>  		return dev_err_probe(&dsi->dev, err, "Failed to init MIPI DSI\n");

[ ... ]
> diff --git a/drivers/gpu/drm/panel/panel-synaptics-r63353.c b/drivers/gpu/drm/panel/panel-synaptics-r63353.c
> index 3a74d48753d9c..ea3fbd0c8662e 100644
> --- a/drivers/gpu/drm/panel/panel-synaptics-r63353.c
> +++ b/drivers/gpu/drm/panel/panel-synaptics-r63353.c
[ ... ]
> @@ -281,12 +281,6 @@ static void r63353_panel_remove(struct mipi_dsi_device *dsi)
>  {
>  	struct r63353_panel *rpanel = mipi_dsi_get_drvdata(dsi);
>  	struct device *dev = &dsi->dev;
> -	int ret;
> -
> -	ret = mipi_dsi_detach(dsi);
> -	if (ret < 0)
> -		dev_err(dev, "Failed to detach from host (%d)\n", ret);
> -

[Severity: Low]
With the error logging removed, is the dev variable now unused and safe
to remove from r63353_panel_remove() to avoid a compiler warning?

>  	drm_panel_remove(&rpanel->base);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6