Re: [PATCH v4 1/3] drm/bridge: samsung-dsim: enable MFLUSH_VS for Exynos 7870 DSIM

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unchecked NULL pointer from `of_device_get_match_data()` leads to kernel panic in probe.
- [High] Hardware polling loop for lane stop states terminates prematurely and lacks a time-bound delay.
- [High] Macro `samsung_dsim_hw_is_exynos()` incorrectly evaluates to false for `DSIM_TYPE_EXYNOS7870`.
--

--- Patch [1]: [PATCH v4 1/3] drm/bridge: samsung-dsim: enable MFLUSH_VS for Exynos 7870 DSIM ---
commit 6317440f4c2cdec241fc86381b097531928140af
Author: Kaustabh Chakraborty <kauschluss@disroot.org>

drm/bridge: samsung-dsim: enable MFLUSH_VS for Exynos 7870 DSIM

Commit a36c533ad3e1 ("drm/bridge: samsung-dsim: Always flush display
FIFO on vsync pulse") intends to enable FIFO flushing at v-sync pulse by
not setting the active-low MFLUSH_VS bit. This commit enables the bit
for Exynos 7870 since it is active-high on this hardware.

Link: https://github.com/samsungexynos7870/android_kernel_samsung_exynos7870/blob/a3762bb1761ae/drivers/video/fbdev/exynos/decon_7870/dsim_reg_7870.c#L699 [1]

> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index e2fc69fc51b60..6260c091c3df8 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -1089,6 +1089,13 @@ static int samsung_dsim_init_link(struct samsung_dsim *dsi)
>  			reg |= DSIM_HBP_DISABLE_MODE;
>  		if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_NO_HSA)
>  			reg |= DSIM_HSA_DISABLE_MODE;
> +
> +		/*
> +		 * For some hardware types, DSIM_MFLUSH_VS bit needs to be
> +		 * enabled as its active-high.
> +		 */
> +		if (dsi->plat_data->hw_type == DSIM_TYPE_EXYNOS7870)
> +			reg |= DSIM_MFLUSH_VS;
>  	}
>  
>  	if (dsi->mode_flags & MIPI_DSI_MODE_NO_EOT_PACKET)

[Severity: High]
This isn't a bug introduced by this patch, but since Exynos 7870 support is
being actively modified here, does the samsung_dsim_hw_is_exynos() macro need
updating to include DSIM_TYPE_EXYNOS7870?

Currently, it evaluates to false for Exynos 7870 because the macro strictly
checks up to DSIM_TYPE_EXYNOS5433:

include/drm/bridge/samsung-dsim.h:
#define samsung_dsim_hw_is_exynos(hw) \
	((hw) >= DSIM_TYPE_EXYNOS3250 && (hw) <= DSIM_TYPE_EXYNOS5433)

This causes the driver to erroneously execute the i.MX early initialization
path in samsung_dsim_atomic_pre_enable() and incorrectly set the
DSIM_NON_CONTINUOUS_CLKLANE bit further down in samsung_dsim_init_link().

[ ... ]

> 	/* Check clock and data lane state are stop state */
> 	timeout = 100;
> 	do {
> 		if (timeout-- == 0) {
> 			dev_err(dsi->dev, "waiting for bus lanes timed out\n");
> 			return -EFAULT;
> 		}
> 
> 		if (driver_data->has_legacy_status_reg)
> 			reg = samsung_dsim_read(dsi, DSIM_STATUS_REG);
> 		else
> 			reg = samsung_dsim_read(dsi, DSIM_DPHY_STATUS_REG);
> 		if ((reg & DSIM_STOP_STATE_DAT(lanes_mask))
> 		    != DSIM_STOP_STATE_DAT(lanes_mask))
> 			continue;
> 	} while (!(reg & (DSIM_STOP_STATE_CLK | DSIM_TX_READY_HS_CLK)));

[Severity: High]
This is a pre-existing issue, but can this hardware polling loop terminate
prematurely?

If the clock lane happens to be ready but the data lanes are not, the
continue statement jumps to the loop condition evaluation. The condition
evaluates to false, causing the loop to exit successfully instead of waiting
for the data lanes.

Also, should this loop include a time-bound delay (like ndelay) to prevent
premature aborts on faster processors?

[ ... ]

[Severity: High]
This is a pre-existing issue elsewhere in the file, but can we hit a NULL
pointer dereference in samsung_dsim_probe() if the driver is bound via sysfs?

If a platform device is manually created and the driver is bound via sysfs
(e.g., via "echo samsung-dsim > bind") without an associated device tree node,
of_device_get_match_data() returns NULL:

samsung_dsim_probe() {
    ...
    dsi->plat_data = of_device_get_match_data(dev);
    dsi->driver_data = samsung_dsim_types[dsi->plat_data->hw_type];
    ...
}

Does this need a NULL check before dereferencing dsi->plat_data?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260809-exynos-dsim-fixes-v4-0-ca67c51e61b7@disroot.org?part=1