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 <[email protected]> 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_5yz3aep4uuzfjmuuzy9nfdu13v...@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/[email protected]?part=2
