Re: [PATCH v7 04/13] drm/mediatek: mtk_dsi: Cleanup encoder if reset fails during bind

[sashiko-bot@kernel.org]

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 <angelogioacchino.delregno@collabora.com>

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/20260914115812.22751-1-angelogioacchino.delregno@collabora.com?part=4