Re: [PATCH v4 2/3] drm/bridge: samsung-dsim: use DSIM interrupt to wait for PLL stability

[sashiko-bot@kernel.org]

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

New issues:
- [High] An early return in the interrupt handler permanently drops the newly added `DSIM_INT_PLL_STABLE` interrupt on platforms that do not wait for a reset completion.
- [High] The jiffy-based timeout for waiting on the PLL stabilization completion can expire prematurely, leading to spurious initialization failures.

Pre-existing issues:
- [High] `devm_clk_bulk_get()` overwrites a statically allocated global clock data array, breaking multi-instance platforms.
- [High] The return value of `samsung_dsim_enable_clock()` is ignored in `samsung_dsim_init()`, causing silent hardware initialization failures.
--

--- Patch [2]: [PATCH v4 2/3] drm/bridge: samsung-dsim: use DSIM interrupt to wait for PLL stability ---
commit 2d251655e36dcc7e05ab4e2b1dc184bbb561aeb9
Author: Kaustabh Chakraborty <kauschluss@disroot.org>

drm/bridge: samsung-dsim: use DSIM interrupt to wait for PLL stability

This commit transitions the PLL stabilization wait in the samsung-dsim driver
from a busy loop to an interrupt-driven completion. It calculates a dynamic
timeout and updates the IRQ handler to complete the wait when the PLL is
stable.

Link: https://lore.kernel.org/r/CAAQKjZMLMbwDVZRb5+Xb_5yz3AEP4uuzFJMuuZy9NFDu13VU5w@mail.gmail.com

> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
[ ... ]
> @@ -849,19 +854,33 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi,
>  	if (dsi->swap_dn_dp_data)
>  		reg |= DSIM_PLL_DPDNSWAP_DAT;
>  
> +	/*
> +	 * The PLL_TIMER value is the product of the timeout delay and the APB
> +	 * bus clock rate. Calculate the timeout delay on-the-fly here.
> +	 * It is assumed that the bus clock is the first clock in the provided
> +	 * bulk clock data.
> +	 */
> +	timeout_us = 100;
> +	bus_clk_mhz = clk_get_rate(dsi->driver_data->clk_data[0].clk) / HZ_PER_MHZ;

[Severity: High]
This is a pre-existing issue, but does using dsi->driver_data->clk_data here
expose a data race on multi-instance setups? In samsung_dsim_probe(),
devm_clk_bulk_get() writes the acquired clock pointers directly into the
shared global static array referenced by driver_data. On multi-instance
platforms, a second instance overwrites the clocks of the first, and unbinding
either instance causes clk_bulk_put() to free the other instance's clocks,
leading to a double-free on the second unbind.

> +	if (bus_clk_mhz)
> +		timeout_us = max(driver_data->reg_values[PLL_TIMER] / bus_clk_mhz,
> +				 timeout_us);
> +
> +	reinit_completion(&dsi->pll_stabilized);
>  	samsung_dsim_write(dsi, DSIM_PLLCTRL_REG, reg);
>  
> -	timeout = 3000;
> -	do {
> -		if (timeout-- == 0) {
> -			dev_err(dsi->dev, "PLL failed to stabilize\n");
> -			return 0;
> -		}
> +	if (!wait_for_completion_timeout(&dsi->pll_stabilized,
> +					 usecs_to_jiffies(timeout_us))) {

[Severity: High]
Could this jiffy-based timeout expire prematurely and lead to spurious
initialization failures? The dynamically calculated timeout_us is around
100-300us, which evaluates to 1 jiffy via usecs_to_jiffies(). A call to
wait_for_completion_timeout(..., 1) sleeps until the next timer tick, which
can happen in less than 1us. If the timeout expires prematurely, the fallback
code checks the status register exactly once, and if the PLL hasn't stabilized
in that narrow window, it aborts.

>  		if (driver_data->has_legacy_status_reg)
>  			reg = samsung_dsim_read(dsi, DSIM_STATUS_REG);
>  		else
>  			reg = samsung_dsim_read(dsi, DSIM_LINK_STATUS_REG);
> -	} while ((reg & BIT(driver_data->pll_stable_bit)) == 0);
> +
> +		if (!(reg & BIT(driver_data->pll_stable_bit))) {
> +			dev_err(dsi->dev, "PLL failed to stabilize\n");
> +			return 0;
> +		}
> +	}
[ ... ]
> @@ -1586,7 +1605,8 @@ static irqreturn_t samsung_dsim_irq(int irq, void *dev_id)
>  	samsung_dsim_write(dsi, DSIM_INTSRC_REG, status);
>  
>  	if (status & DSIM_INT_SW_RST_RELEASE) {
> -		unsigned long mask = ~(DSIM_INT_RX_DONE |
> +		unsigned long mask = ~(DSIM_INT_PLL_STABLE |
> +				       DSIM_INT_RX_DONE |
>  				       DSIM_INT_SFR_FIFO_EMPTY |
>  				       DSIM_INT_SFR_HDR_FIFO_EMPTY |
>  				       DSIM_INT_RX_ECC_ERR |
> @@ -1596,8 +1616,10 @@ static irqreturn_t samsung_dsim_irq(int irq, void *dev_id)
>  		return IRQ_HANDLED;
>  	}

[Severity: High]
Does this unconditional early return drop the newly added DSIM_INT_PLL_STABLE
interrupt? On platforms where wait_for_reset is false, samsung_dsim_init()
triggers a software reset and immediately enables the clock without waiting.
Both the DSIM_INT_SW_RST_RELEASE and DSIM_INT_PLL_STABLE interrupts can fire
concurrently. If coalesced, the handler clears both hardware interrupts,
processes the reset, and executes this early return in samsung_dsim_irq(),
skipping the DSIM_INT_PLL_STABLE check entirely.

[ ... ]
> @@ -1635,9 +1657,9 @@ static int samsung_dsim_init(struct samsung_dsim *dsi)
>  	if (driver_data->reg_values[RESET_TYPE] == DSIM_FUNCRST)
>  		samsung_dsim_enable_lane(dsi, BIT(dsi->lanes) - 1);
>  
> -	samsung_dsim_enable_clock(dsi);
>  	if (driver_data->wait_for_reset)
>  		samsung_dsim_wait_for_reset(dsi);
> +	samsung_dsim_enable_clock(dsi);

[Severity: High]
This is a pre-existing issue, but should the return value of
samsung_dsim_enable_clock() be checked here? If samsung_dsim_set_pll() fails
and returns 0, the ignored -EFAULT allows the driver to continue
initialization with a broken clock state, computing a byte_clock of 0, writing
0s to PHY timing registers, and leaving the hardware in an unusable state.

>  	samsung_dsim_set_phy_ctrl(dsi);
>  	samsung_dsim_init_link(dsi);

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