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