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