Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues: - [High] Unbalanced reference count on error path in `mtk_dsi_poweron`. - [High] Data race on `dsi->irq_data` between process and IRQ contexts leading to lost interrupts. - [High] Missing synchronization in `mtk_dsi_host_transfer` allows concurrent DSI commands to corrupt hardware state. - [High] Sleeping function `usleep_range()` called from atomic context. -- commit 911862fd7e3a1163be08c7ed054bcfc694f709a1 Author: AngeloGioacchino Del Regno <[email protected]> drm/mediatek: mtk_dsi: Cleanup encoder if reset fails during bind When binding the dsi component, the .bind() callback initializes the encoder, and then will optionally trigger resets. This patch cleans up the encoder upon reset failure to prevent a use-after-free. > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c > b/drivers/gpu/drm/mediatek/mtk_dsi.c > index 97b8a91874f5b..0b167b4411e59 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c > @@ -1095,7 +1095,13 @@ static int mtk_dsi_bind(struct device *dev, struct > device *master, void *data) [ ... ] While reviewing this patch, I noticed a few pre-existing issues in the same file. drivers/gpu/drm/mediatek/mtk_dsi.c:mtk_dsi_poweron() { ... if (++dsi->refcount != 1) return 0; ret = mipi_dsi_pixel_format_to_bpp(dsi->format); if (ret < 0) { dev_err(dev, "Unknown MIPI DSI format %d\n", dsi->format); return ret; ... } [Severity: High] This is a pre-existing issue, but does this error path fail to decrement the reference count? Since dsi->refcount is incremented at the start of mtk_dsi_poweron(), returning directly here instead of jumping to err_refcount seems to skip the decrement. This could lead to an unbalanced count, causing problems during teardown when mtk_dsi_poweroff() attempts to disable clocks that were never powered up. drivers/gpu/drm/mediatek/mtk_dsi.c:mtk_dsi_irq_data_clear() { ... static void mtk_dsi_irq_data_clear(struct mtk_dsi *dsi, u32 irq_bit) { dsi->irq_data &= ~irq_bit; } ... [Severity: High] This isn't a bug introduced by this patch, but is there a data race on dsi->irq_data between process and IRQ contexts? If a DSI interrupt fires concurrently while mtk_dsi_irq_data_clear() executes in process context during a DSI host transfer, the non-atomic read-modify-write here could overwrite and erase the flag set by the hardware IRQ handler, potentially causing lost interrupts. drivers/gpu/drm/mediatek/mtk_dsi.c:mtk_dsi_host_transfer() { ... static ssize_t mtk_dsi_host_transfer(struct mipi_dsi_host *host, const struct mipi_dsi_msg *msg) { struct mtk_dsi *dsi = host_to_dsi(host); struct drm_device *drm = dsi->bridge.dev; ssize_t recv_cnt; u8 read_data[16]; void *src_addr; u8 irq_flag = CMD_DONE_INT_FLAG; u32 dsi_mode; int ret, i; dsi_mode = readl(dsi->regs + dsi->driver_data->reg_main[DSI_MODE_CTRL]); if (dsi_mode & MODE) { mtk_dsi_stop(dsi); ... } [Severity: High] This is a pre-existing issue, but does mtk_dsi_host_transfer() lack synchronization to protect hardware state? If called concurrently (for example, from a sysfs backlight update while an atomic display modeset is ongoing), the lack of internal locking for the DSI controller's registers could allow concurrent commands to corrupt the hardware state machine and lead to DSI transmission failures. drivers/gpu/drm/mediatek/mtk_dsi.c:mtk_dsi_lane_ready() { ... static void mtk_dsi_lane_ready(struct mtk_dsi *dsi) { if (!dsi->lanes_ready) { dsi->lanes_ready = true; mtk_dsi_rxtx_control(dsi); usleep_range(30, 100); mtk_dsi_reset_dphy(dsi); mtk_dsi_clk_ulp_mode_leave(dsi); mtk_dsi_lane0_ulp_mode_leave(dsi); mtk_dsi_clk_hs_mode(dsi, 0); usleep_range(1000, 3000); ... } [Severity: High] This isn't a bug introduced by this patch, but does this code sleep in an atomic context? It appears drm_atomic_helper_commit_tail() can invoke the bridge atomic pre-enable callback mtk_dsi_bridge_atomic_pre_enable(), which calls mtk_dsi_lane_ready(). Calling usleep_range() from an atomic callback violates DRM subsystem rules and can lead to scheduling while atomic panics or warnings. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
