Re: [PATCH] fbdev: omapfb: panel-dsi-cm: initialize lock before registering display

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] List corruption and Use-After-Free due to missing display unregistration on probe error paths.
- [High] Premature device registration exposes the display to consumers before initialization is complete, risking uninitialized resource access.
- [High] Use-After-Free during driver removal due to non-synchronous cancellation of delayed work.
- [High] Race condition between TE interrupt handler and timeout work causing double bus unlock.
--

--- Patch [1]: [PATCH] fbdev: omapfb: panel-dsi-cm: initialize lock before registering display ---
commit 62bd817117921c28fba7141ddc3cf344ad0e4b1e
Author: Runyu Xiao <runyu.xiao@seu.edu.cn>

    fbdev: omapfb: panel-dsi-cm: initialize lock before registering display
    
    dsicm_probe() registers the display before initializing ddata->lock.
    Once omapdss_register_display() publishes the display, another consumer
    can reach a dsicm callback that takes this mutex while it is still
    uninitialized.
    
    Initialize the mutex before registering the display so the published
    callbacks always see a valid lock.

> diff --git a/drivers/video/fbdev/omap2/omapfb/displays/panel-dsi-cm.c b/drivers/video/fbdev/omap2/omapfb/displays/panel-dsi-cm.c
> index 5e7963b4aa93c..8d1dcd4371139 100644
> --- a/drivers/video/fbdev/omap2/omapfb/displays/panel-dsi-cm.c
> +++ b/drivers/video/fbdev/omap2/omapfb/displays/panel-dsi-cm.c
> @@ -1150,14 +1150,14 @@ static int dsicm_probe(struct platform_device *pdev)
>  	dssdev->caps = OMAP_DSS_DISPLAY_CAP_MANUAL_UPDATE |
>  		OMAP_DSS_DISPLAY_CAP_TEAR_ELIM;
>  
> +	mutex_init(&ddata->lock);
> +
>  	r = omapdss_register_display(dssdev);
>  	if (r) {
>  		dev_err(dev, "Failed to register panel\n");
>  		goto err_reg;
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but does publishing the display
globally here expose it to consumers before initialization is complete?

If a consumer like omapfb discovers the display immediately and calls
dsicm_update() or dsicm_disable(), could they trigger cancel_delayed_work()
or schedule_delayed_work() on ulps_work and te_timeout_work before those
works are initialized later in the probe function?

>  
> -	mutex_init(&ddata->lock);
> -
>  	atomic_set(&ddata->do_update, 0);
>  
>  	ddata->reset_gpio = devm_gpiod_get(&pdev->dev, "reset", GPIOD_OUT_LOW);

[Severity: High]
This is a pre-existing issue, but does the error path for this and subsequent
initializations leak the registered display?

If devm_gpiod_get() fails:

	if (r) {
		dev_err(&pdev->dev, "Failed to request reset gpio: %d\n", r);
		return r;
	}

the probe function returns the error directly, but omapdss_register_display()
has already added dssdev->panel_list to the global panel_list. Since
omapdss_unregister_display() is never called on these error paths, will this
leave a dangling pointer in the global list that could cause a panic if
traversed?

[Severity: High]
This isn't a bug introduced by this patch, but does driver removal have a
use-after-free risk?

In dsicm_remove():

	dsicm_cancel_ulps_work(ddata);

This uses cancel_delayed_work() for ulps_work, which does not wait for
currently executing work, and te_timeout_work cancellation appears to
be omitted entirely.

When remove() returns, devres frees the ddata structure. If either work item
is running or subsequently fires, will it dereference the freed ddata?

[Severity: High]
This is a pre-existing issue, but is there a race condition between the TE
interrupt handler and the timeout work?

In dsicm_te_timeout_work_callback():

	atomic_set(&ddata->do_update, 0);
	in->ops.dsi->bus_unlock(in);

If this executes concurrently with dsicm_te_isr(), could the ISR successfully
claim the update via atomic_cmpxchg() before the timeout work zeroes it?

If both paths then handle the frame, would this lead to an immediate
uncoordinated bus_unlock() from the timeout work and a subsequent
bus_unlock() from the frame-done callback?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818135318.1957283-1-runyu.xiao@seu.edu.cn?part=1