Re: [PATCH v3 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] The return value of wait_for_completion_timeout() is evaluated incorrectly, treating a timeout as success and success as an error.
- [High] The DSIM_INT_PLL_STABLE interrupt is permanently masked in DSIM_INTMSK_REG and will never fire.
- [Medium] Using `wait_for_completion_timeout` with a 1-jiffy timeout can expire prematurely, violating hardware timing requirements.

Pre-existing issues:
- [High] Global static clk_bulk_data structures are shared among driver instances, leading to pointer overwrites and potential Use-After-Free.
--

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

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

Stabilizing PLL needs to be waited for. This is done using a loop,
checking the PLL_STABLE bit in the status register. DSIM fires an
interrupt when the PLL is stabilized. Rely on this functionality for
stabilization wait, getting rid of the implicit loop.

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 +850,26 @@ 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. Calcutate 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 = 100;
> +	fin = clk_get_rate(dsi->driver_data->clk_data[0].clk) / HZ_PER_MHZ;

[Severity: High]
This is a pre-existing issue, but does accessing the global static
clk_bulk_data structures here risk a use-after-free or pointer overwrite
in multi-DSI systems?

The driver defines global static arrays that dsi->driver_data->clk_data points
to, and devm_clk_bulk_get() modifies this shared array in-place, overwriting
the .clk pointers. In multi-DSI systems, a second instance's probe will
overwrite the clock pointers belonging to the first. If an instance is unbound,
the pointers become dangling.

Could this dereference of the potentially overwritten or dangling clock pointer
cause issues during PLL setup?

> +	if (fin)
> +		timeout = max(dsi->driver_data->reg_values[PLL_TIMER] / fin,
> +			      timeout);
> +
> +	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 (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 (wait_for_completion_timeout(&dsi->pll_stabilized,
> +					usecs_to_jiffies(timeout))) {

[Severity: Medium]
Since the timeout is dynamically calculated with a lower bound of 100
microseconds, usecs_to_jiffies(100) will evaluate to 1 jiffy on most
systems.

Does a 1-jiffy timeout mean 'timeout on the next timer tick', which could
happen almost immediately depending on timer alignment?

Could this cause the wait to expire prematurely before the hardware PLL
has actually stabilized, leading to regressions?

[Severity: High]
Does this code incorrectly handle the return value of
wait_for_completion_timeout()?

wait_for_completion_timeout() returns 0 if the wait timed out, and > 0 if
it was successful. Evaluating the return value directly in the if condition
treats > 0 as true, causing a successful wait to erroneously execute the
error path and return 0 (failure).

Conversely, if it times out, it returns 0, evaluating to false, and
incorrectly returns success.

> +		dev_err(dsi->dev, "PLL failed to stabilize\n");
> +		return 0;
> +	}
>  
>  	dsi->hs_clock = fout;

[ ... ]

> @@ -1596,8 +1604,12 @@ static irqreturn_t samsung_dsim_irq(int irq, void *dev_id)
>  		return IRQ_HANDLED;
>  	}
>  
> -	if (!(status & (DSIM_INT_RX_DONE | DSIM_INT_SFR_FIFO_EMPTY |
> -			DSIM_INT_PLL_STABLE)))
> +	if (status & DSIM_INT_PLL_STABLE) {
> +		complete(&dsi->pll_stabilized);
> +		return IRQ_HANDLED;
> +	}

[Severity: High]
Will the DSIM_INT_PLL_STABLE interrupt ever fire?

Looking earlier in samsung_dsim_irq(), when DSIM_INT_SW_RST_RELEASE is
handled, the hardware interrupt mask is rewritten using:

    unsigned long mask = ~(DSIM_INT_RX_DONE |
                           DSIM_INT_SFR_FIFO_EMPTY |
                           DSIM_INT_SFR_HDR_FIFO_EMPTY |
                           DSIM_INT_RX_ECC_ERR |
                           DSIM_INT_SW_RST_RELEASE);
    samsung_dsim_write(dsi, DSIM_INTMSK_REG, mask);

Since DSIM_INT_PLL_STABLE is omitted from this unmasking list, its bit
remains 1 due to the bitwise NOT, which actively masks (disables) it in the
hardware.

Because the interrupt is permanently masked in DSIM_INTMSK_REG, will the wait
for completion always time out?

> +
> +	if (!(status & (DSIM_INT_RX_DONE | DSIM_INT_SFR_FIFO_EMPTY)))
>  		return IRQ_HANDLED;
>  
>  	if (samsung_dsim_transfer_finish(dsi))

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