Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] clk: starfive: Unsafe hardware access in .set_rate causes CPU stall during probe - [Medium] clk: starfive: Pre-PLL is unconditionally powered on in .set_rate causing a power leak - [Medium] clk: starfive: Resource lifecycle inversion with refoclk and devres clock provider - [Medium] clk: starfive: Data race on cached clock state variables between CCF and PHY frameworks --
commit 5469b7909aa9bb8690ee9fbfe18ef57f4c2d5820 Author: Michal Wilczynski <[email protected]> phy: starfive: Add jh7110-inno-hdmi-phy driver Add the HDMI PHY driver for the StarFive JH7110. > diff --git a/drivers/phy/starfive/phy-jh7110-inno-hdmi.c > b/drivers/phy/starfive/phy-jh7110-inno-hdmi.c > new file mode 100644 > index 0000000000000..2575cb5ed624a > --- /dev/null > +++ b/drivers/phy/starfive/phy-jh7110-inno-hdmi.c > @@ -0,0 +1,579 @@ [ ... ] > +static int starfive_hdmi_phy_clk_set_rate(struct clk_hw *hw, unsigned long > rate, > + unsigned long parent_rate) > +{ > + struct starfive_hdmi_phy *inno = to_starfive_hdmi_phy(hw); > + const struct inno_hdmi_phy_pre_pll_config *cfg; > + > + /* > + * The JH7110 only drives 8bpc, so the TMDS clock always matches the > + * pixel clock. > + */ > + cfg = inno_hdmi_phy_pre_pll_lookup(&inno->pre_pll, rate, rate); > + if (IS_ERR(cfg)) > + return PTR_ERR(cfg); > + > + dev_dbg(inno->dev, "%s rate %lu tmdsclk %lu\n", > + __func__, rate, cfg->tmdsclock); > + > + if (inno->pixclock == rate && inno->tmdsclock == cfg->tmdsclock) > + return 0; > + > + inno_update_bits(inno, STF_INNO_BIAS_CONTROL, > + STF_INNO_BIAS_ENABLE, STF_INNO_BIAS_ENABLE); [Severity: High] Does this execute an unclocked hardware access if the clock is gated? The commit message notes that accessing the PHY registers while the HDMI controller's system clock is gated wedges the CPU. Because starfive_hdmi_phy_probe() unconditionally calls clk_set_rate() before the sibling HDMI bridge driver has bound and enabled the shared system clock, the hardware is gated. Could a clk_hw_is_prepared(hw) check be added here similarly to how it was used in .recalc_rate? > + inno_write(inno, STF_INNO_RX_CONTROL, STF_INNO_RX_ENABLE); > + > + inno_hdmi_phy_pre_pll_power_down(&inno->pre_pll, true); > + inno_hdmi_phy_pre_pll_configure(&inno->pre_pll, cfg); > + inno_hdmi_phy_pre_pll_power_down(&inno->pre_pll, false); [Severity: Medium] Will the Pre-PLL remain powered on indefinitely if the clock isn't prepared? Since .set_rate turns the Pre-PLL on, and the driver omits the .is_prepared callback in starfive_hdmi_phy_clk_ops, the CCF relies on the software prepare count (which is 0). When clk_disable_unused() runs, it skips unprepared clocks with a 0 count, meaning .unprepare() won't be invoked to turn the PLL off. > + > + inno->pixclock = rate; > + inno->tmdsclock = cfg->tmdsclock; [Severity: Medium] Could there be a data race on inno->pixclock and inno->tmdsclock between the CCF and PHY frameworks? The CCF callbacks (such as .set_rate and .recalc_rate) read and write inno->pixclock and inno->tmdsclock under the CCF core's prepare_lock. Concurrently, the PHY framework operations (such as .power_off calling starfive_hdmi_phy_power_down()) write to these exact same variables under the PHY framework's internal phy->mutex. If userspace triggers a sysfs clock summary read (triggering .recalc_rate) concurrently with a DRM display modeset or power down sequence, could .recalc_rate overwrite the 0 set by power_down with stale hardware values? This might prevent a subsequent set_rate from applying the correct settings. > + > + return 0; > +} > + > +static const struct clk_ops starfive_hdmi_phy_clk_ops = { > + .prepare = starfive_hdmi_phy_clk_prepare, > + .unprepare = starfive_hdmi_phy_clk_unprepare, > + .recalc_rate = starfive_hdmi_phy_clk_recalc_rate, > + .determine_rate = starfive_hdmi_phy_clk_determine_rate, > + .set_rate = starfive_hdmi_phy_clk_set_rate, > +}; [Severity: Medium] The omission of the .is_prepared callback here is what causes clk_disable_unused() to assume the clock is already physically disabled, leading to the power leak mentioned in .set_rate. > + > +static void starfive_hdmi_phy_power_down(struct starfive_hdmi_phy *inno) > +{ > + inno_write(inno, STF_INNO_TMDS_CONTROL, 0x00); > + inno_write(inno, STF_INNO_SERIALIER_CONTROL, 0x00); > + inno_write(inno, STF_INNO_LDO_CONTROL, 0x00); > + inno_update_bits(inno, STF_INNO_BIAS_CONTROL, > + STF_INNO_BIAS_ENABLE, 0x00); > + inno_write(inno, STF_INNO_RX_CONTROL, 0x00); > + > + /* Power down Post-PLL */ > + inno_update_bits(inno, STF_INNO_POST_PLL_DIV_1, > + STF_INNO_POST_PLL_POWER_DOWN, > + STF_INNO_POST_PLL_POWER_DOWN); > + > + inno->tmdsclock = 0; > + inno->pixclock = 0; [Severity: Medium] This is where the concurrent writes from the PHY framework happen, as noted in the previous comment about the data race with the CCF callbacks. > +} [ ... ] > +static int starfive_hdmi_phy_probe(struct platform_device *pdev) > +{ [ ... ] > + ret = of_clk_add_provider(dev->of_node, of_clk_src_simple_get, > inno->phyclk); > + if (ret) { > + dev_err(dev, "Failed to add clock provider: %d\n", ret); > + goto err_disable_refoclk; > + } > + > + ret = clk_set_rate(inno->phyclk, 297000000); [Severity: High] This unconditionally triggers a rate change during probe, which leads to the unsafe hardware access in .set_rate described earlier, since the sibling bridge driver is not guaranteed to have enabled the shared system clock yet. > + if (ret) { > + dev_err(dev, "Failed to set default rate: %d\n", ret); > + goto err_del_clk_provider; > + } > + > + /* Create and register the PHY provider */ > + inno->phy = devm_phy_create(inno->dev, NULL, &starfive_hdmi_phy_ops); > + if (IS_ERR(inno->phy)) { > + ret = PTR_ERR(inno->phy); > + dev_err(inno->dev, "failed to create HDMI PHY: %d\n", ret); > + goto err_del_clk_provider; > + } > + > + phy_set_drvdata(inno->phy, inno); > + > + phy_provider = devm_of_phy_provider_register(inno->dev, > + of_phy_simple_xlate); > + ret = PTR_ERR_OR_ZERO(phy_provider); > + if (ret) > + goto err_del_clk_provider; > + > + return 0; > + > +err_del_clk_provider: > + of_clk_del_provider(dev->of_node); > +err_disable_refoclk: > + clk_disable_unprepare(inno->refoclk); > + return ret; [Severity: Medium] Is it safe to manually disable the reference clock here before devres unregisters the clock and PHY providers? The clock and PHY providers are registered using devm_clk_register() and devm_of_phy_provider_register(). Disabling refoclk manually here (and in remove) creates a window where operations could execute with a disabled parent clock if a consumer triggers a clock or PHY operation before devres cleans up the providers. Would it be better to bind the refoclk enable state to devres via devm_clk_get_enabled() or devm_add_action_or_reset()? > +} > + > +static void starfive_hdmi_phy_remove(struct platform_device *pdev) > +{ > + struct starfive_hdmi_phy *inno = platform_get_drvdata(pdev); > + > + of_clk_del_provider(pdev->dev.of_node); > + clk_disable_unprepare(inno->refoclk); [Severity: Medium] As in the probe error path, manually disabling the reference clock before devres unregisters the providers creates a use-after-disable window. > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=18
