Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] Forced overwrite of PLL reference counter in PHY enable/disable conflicts with VCO prepare/unprepare, leading to skipped hardware initialization and PLL lock failures. - [Medium] Data race when reading `pll_enable_cnt` outside of its protecting spinlock in `dsi_pll_enable_pll_bias`. Pre-existing issues: - [High] Dangling pointer in the global `pll_7nm_list` array leads to a Use-After-Free if device probe fails. -- --- Patch [1]: [PATCH] Revert "drm/msm: dsi: fix PLL init in bonded mode" --- commit 04f56183f63de02b770cafb9bbcc705bedd10f12 Author: Dmitry Baryshkov <[email protected]> Revert "drm/msm: dsi: fix PLL init in bonded mode" This commit reverts commit 93c97bc8d85d which fixed an issue with DSI bonded mode but broke non-bonded usecases. The clock divider was being programmed incorrectly in non-bonded mode, resulting in the wrong display mode being selected. Closes: https://lore.kernel.org/r/[email protected] > diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c > b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c > index 984a66085dfbf..5d805a797abdb 100644 > --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c > +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c > @@ -426,8 +426,11 @@ static void dsi_pll_enable_pll_bias(struct dsi_pll_7nm > *pll) > u32 data; > > spin_lock_irqsave(&pll->pll_enable_lock, flags); > - pll->pll_enable_cnt++; > - WARN_ON(pll->pll_enable_cnt == INT_MAX); > + if (pll->pll_enable_cnt++) { > + spin_unlock_irqrestore(&pll->pll_enable_lock, flags); > + WARN_ON(pll->pll_enable_cnt == INT_MAX); [Severity: Medium] Can reading pll_enable_cnt outside of pll_enable_lock cause a data race? If dsi_7nm_phy_enable() or dsi_7nm_phy_disable() concurrently forces pll_enable_cnt to 1 or 0, this lockless read of pll->pll_enable_cnt in the WARN_ON statement could result in undefined behavior or spurious warnings. > + return; > + } > > data = readl(pll->phy->base + REG_DSI_7nm_PHY_CMN_CTRL_0); > data |= DSI_7nm_PHY_CMN_CTRL_0_PLL_SHUTDOWNB; [ ... ] > @@ -873,6 +876,7 @@ static int dsi_pll_7nm_init(struct msm_dsi_phy *phy) > spin_lock_init(&pll_7nm->pll_enable_lock); > > pll_7nm->phy = phy; > + phy->pll_data = pll_7nm; [Severity: High] This is a pre-existing issue, but can this lead to a Use-After-Free if the device probe fails? In dsi_pll_7nm_init(), the newly allocated pll_7nm is assigned to the global pll_7nm_list[phy->id] before this section. If pll_7nm_register() or a subsequent probe operation fails, the devm framework frees the pll_7nm struct, but the dangling pointer remains in pll_7nm_list. If another bonded PHY probes successfully, could it assign this dangling pointer to its pll_7nm->slave and dereference it during dsi_pll_enable_pll_bias(), causing memory corruption? > > ret = pll_7nm_register(pll_7nm, phy->provided_clocks->hws); > if (ret) { [ ... ] > @@ -1084,10 +1090,13 @@ static int dsi_7nm_phy_enable(struct msm_dsi_phy *phy, > glbl_rescode_bot_ctrl = 0x3c; > } > > + spin_lock_irqsave(&pll->pll_enable_lock, flags); > + pll->pll_enable_cnt = 1; [Severity: High] Does forcing pll_enable_cnt to 1 here conflict with the increment in dsi_pll_enable_pll_bias()? During display bringup, dsi_7nm_phy_enable() is called and sets pll_enable_cnt to 1. When clk_prepare() subsequently calls dsi_pll_enable_pll_bias(), the condition: if (pll->pll_enable_cnt++) { spin_unlock_irqrestore(&pll->pll_enable_lock, flags); ... return; } will evaluate to true, causing the function to return early and skip programming REG_DSI_7nm_PHY_PLL_SYSTEM_MUXES. Will this cause the PLL to fail to lock and break the display? > /* de-assert digital and pll power down */ > data = DSI_7nm_PHY_CMN_CTRL_0_DIGTOP_PWRDN_B | > DSI_7nm_PHY_CMN_CTRL_0_PLL_SHUTDOWNB; > writel(data, base + REG_DSI_7nm_PHY_CMN_CTRL_0); > + spin_unlock_irqrestore(&pll->pll_enable_lock, flags); > > /* Assert PLL core reset */ > writel(0x00, base + REG_DSI_7nm_PHY_CMN_PLL_CNTRL); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20260712-msm-revert-dsi-pll-fix-v1-1-40122689e...@oss.qualcomm.com?part=1
