Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider: - [High] PHY provider is registered before probe finishes, exposing it to early concurrent access and potential Use-After-Free (UAF) on probe failure. - [High] Lack of driver-level synchronization between CCF and PHY frameworks leads to concurrent hardware state corruption. - [Medium] The MPLL (main link PLL) is powered on during PHY initialization but is never powered off in the power down sequence. - [Medium] Missing hardware state cleanup on error paths in `k3_inno_dp_phy_power_on` leaves the PHY powered on. - [Medium] Rate configuration functions unconditionally power on the PLLs, overriding the logical PHY power state. - [Medium] PLL programming functions do not poll for hardware lock before proceeding. --
--- Patch [5]: [PATCH RESEND 05/17] phy: spacemit: add Innosilicon DP TX PHY driver --- commit 31db74eae7dc59ddc61373dbe3e2c6c1e6966ccd Author: Cody Kang <[email protected]> Subject: phy: spacemit: add Innosilicon DP TX PHY driver This commit introduces the SpacemiT K3 Innosilicon DisplayPort PHY driver. It integrates the PHY as a child of the DP controller, registering a PHY provider and exposing the pixel PLL as a clock provider. > diff --git a/drivers/phy/spacemit/phy-k3-inno-dp.c > b/drivers/phy/spacemit/phy-k3-inno-dp.c > new file mode 100644 > index 0000000000000..9dd62edb7a5af > --- /dev/null > +++ b/drivers/phy/spacemit/phy-k3-inno-dp.c > @@ -0,0 +1,948 @@ [ ... ] > +static int k3_inno_dp_phy_power_on(struct phy *phy) > +{ > + struct k3_inno_dp_phy *p = phy_get_drvdata(phy); > + u32 lane_en; > + u32 val; > + int ret; [ ... ] > + ret = regmap_read_poll_timeout(p->regmap, DPTX_ANA_MPLL, val, > + val & DPTX_ANA_MPLL_LOCKED, > + 2000, 10 * 1000); > + if (ret) { > + dev_err(p->dev, "DP PHY core PLL lock timed out\n"); > + return ret; > + } [Severity: Medium] If the MPLL or PREPLL fails to lock within 10ms, this immediately returns an error without restoring the power down bits or XMIT_EN. Since the generic PHY framework does not automatically call phy_power_off() when phy_power_on() fails, could this leave the hardware running in an inconsistent state and leak power? > + > + ret = regmap_read_poll_timeout(p->regmap, DPTX_ANA_PREPLL, val, > + val & DPTX_ANA_PREPLL_LOCKED, > + 2000, 10 * 1000); > + if (ret) > + dev_err(p->dev, "DP PHY pixel PLL lock timed out\n"); > + > + return ret; > +} > + > +static int k3_inno_dp_phy_power_off(struct phy *phy) > +{ > + struct k3_inno_dp_phy *p = phy_get_drvdata(phy); > + > + regmap_write_bits(p->regmap, DPTX_PHY_CTRL, DPTX_PHY_CTRL_XMIT_EN, 0); > + usleep_range(2000, 4000); > + > + regmap_write_bits(p->regmap, DPTX_ANA_PREPLL, DPTX_ANA_PREPLL_PD, > + DPTX_ANA_PREPLL_PD); > + usleep_range(2000, 4000); > + > + return 0; > +} [Severity: Medium] Does this power down sequence omit the MPLL? The k3_inno_dp_phy_power_on() function powers up the MPLL by clearing DPTX_ANA_MPLL_PD, but this power_off function does not appear to re-assert it. Could this leave the main link PLL running indefinitely when the DP controller is suspended or torn down? [ ... ] > +static void k3_inno_dp_phy_program_mpll(struct k3_inno_dp_phy *p, > + const struct k3_inno_dp_mpll_cfg *cfg) > +{ > + struct regmap *rm = p->regmap; [ ... ] > + regmap_write_bits(rm, DPTX_ANA_PREPLL_CTRL, DPTX_ANA_PREPLL_LOCK_BYPEN, > + FIELD_PREP(DPTX_ANA_PREPLL_LOCK_BYPEN, 1)); > + > + regmap_write_bits(rm, DPTX_ANA_MPLL, DPTX_ANA_MPLL_PD, > + FIELD_PREP(DPTX_ANA_MPLL_PD, 0)); [Severity: Medium] Does unconditionally writing 0 to DPTX_ANA_MPLL_PD override the logical power state of the PHY? If a DRM driver configures the link rate (calling phy_configure()) while the PHY is logically powered off, this will force the PLL to run and desynchronize the hardware from the PHY framework's power state. > + usleep_range(2000, 4000); > +} [Severity: Medium] Is it safe to rely on a fixed 2-4ms sleep here instead of polling DPTX_ANA_MPLL_LOCKED? The power_on() function polls the locked bit for up to 10ms. A fixed delay here might proceed with an unstable clock if the hardware takes longer to lock, potentially causing display failures or link training issues. [ ... ] > +static void k3_inno_dp_program_prepll(struct k3_inno_dp_phy *p, > + const struct k3_inno_dp_prepll_cfg *cfg) > +{ > + regmap_write_bits(p->regmap, DPTX_ANA_PREPLL, DPTX_ANA_PREPLL_PD, > + FIELD_PREP(DPTX_ANA_PREPLL_PD, 1)); > + usleep_range(2000, 4000); [Severity: High] Is a driver-level lock needed to synchronize access to the PHY registers between the CCF and PHY frameworks? Since this driver exposes both a clock (via CCF) and a PHY that share the same hardware registers, concurrent calls to clk_set_rate() and phy_power_on() can interleave. If clk_set_rate() begins this multi-step programming sequence, a concurrent phy_power_on() could clear the PD bit while the clock is still being configured, corrupting the hardware state. [ ... ] > + regmap_write_bits(p->regmap, DPTX_ANA_PREPLL, > + DPTX_ANA_PREPLL_PCLK_NORMAL, > + FIELD_PREP(DPTX_ANA_PREPLL_PCLK_NORMAL, 1)); > + usleep_range(2000, 4000); > + > + regmap_write_bits(p->regmap, DPTX_ANA_PREPLL, DPTX_ANA_PREPLL_PD, > + FIELD_PREP(DPTX_ANA_PREPLL_PD, 0)); [Severity: Medium] Similar to the MPLL configuration, does unconditionally clearing DPTX_ANA_PREPLL_PD here override the PHY power state if the clock rate is set while the PHY is logically off? > + usleep_range(2000, 4000); > +} [Severity: Medium] Is it safe to rely on a fixed delay here instead of polling the DPTX_ANA_PREPLL_LOCKED bit, which could lead to proceeding with an unstable pixel clock? [ ... ] > +static int k3_inno_dp_phy_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct k3_inno_dp_phy *p; > + struct phy_provider *provider; > + struct clk_init_data init = {}; > + int ret; [ ... ] > + provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate); > + if (IS_ERR(provider)) > + return dev_err_probe(dev, PTR_ERR(provider), > + "failed to register PHY provider\n"); > + > + /* PHY takes a 24 MHz reference clock */ > + p->ref_clk_khz = 24000; > + > + /* The APMU pixel-clock mux references this clock by name. */ > + of_property_read_string(dev->of_node, "clock-output-names", &init.name); > + if (!init.name) > + init.name = dev_name(dev); > + init.ops = &k3_inno_dp_pxclk_ops; > + init.parent_names = NULL; > + init.num_parents = 0; > + init.flags = 0; > + p->pxclk_hw.init = &init; > + > + ret = devm_clk_hw_register(dev, &p->pxclk_hw); > + if (ret) > + return dev_err_probe(dev, ret, > + "failed to register pixel clock\n"); [Severity: High] Can the PHY provider be accessed by consumers before probe completes successfully? The PHY provider is registered via devm_of_phy_provider_register() before the pixel clock is registered. If devm_clk_hw_register() fails, probe will return an error and devres will free the driver context struct. If a consumer driver acquires a reference to the PHY during this race window, it could retain the reference after probe fails, leading to a use-after-free when it attempts PHY operations on the freed pointer. -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5
