Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Using `wait_for_completion_timeout()` with a short, sub-jiffy timeout leads to premature expiration and spurious display initialization failures. - [High] An early return in the interrupt handler causes `DSIM_INT_PLL_STABLE` events to be silently discarded if they coalesce with `DSIM_INT_SW_RST_RELEASE`. --
--- Patch [2]: [PATCH v5 2/3] drm/bridge: samsung-dsim: use DSIM interrupt to wait for PLL stability --- commit 9e7e8160e685bf48c532c28ad86a19b0799f53ce Author: Kaustabh Chakraborty <[email protected]> drm/bridge: samsung-dsim: use DSIM interrupt to wait for PLL stability This commit replaces a busy-wait polling loop for checking PLL stability with an interrupt-driven completion mechanism. Link: https://lore.kernel.org/r/caaqkjzmlmbwdvzrb5+xb_5yz3aep4uuzfjmuuzy9nfdu13v...@mail.gmail.com > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c > b/drivers/gpu/drm/bridge/samsung-dsim.c > index 6260c091c3df8..216a7cc9d5c55 100644 > --- 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; > + 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] Does this code risk premature timeout expiration? When timeout_us is 100, usecs_to_jiffies(100) translates to 1 jiffy on standard kernel configurations (e.g., HZ=1000 or lower). Passing a 1-jiffy timeout to wait_for_completion_timeout() only guarantees waiting until the next timer tick, which can expire almost immediately depending on tick alignment. This might cause spurious failures to initialize the display PLL, since the robust 3000-iteration polling loop has been replaced with a single register read after this wait. > 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; > + } > + } > > dsi->hs_clock = fout; > [ ... ] > @@ -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; > } > > - 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); [Severity: High] Can DSIM_INT_PLL_STABLE events be silently discarded here if they coalesce with DSIM_INT_SW_RST_RELEASE? If both DSIM_INT_SW_RST_RELEASE and DSIM_INT_PLL_STABLE are set simultaneously in status (which may happen if reset and PLL initialization run concurrently on platforms where wait_for_reset is false), the code inside the if (status & DSIM_INT_SW_RST_RELEASE) block executes and returns IRQ_HANDLED early. Since the interrupts are already cleared in hardware by the samsung_dsim_write(dsi, DSIM_INTSRC_REG, status) call earlier in this function, complete(&dsi->pll_stabilized) would be skipped entirely, potentially causing the waiter to time out. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
