Re: [PATCH v4 18/20] phy: starfive: Add jh7110-inno-hdmi-phy driver

[sashiko-bot@kernel.org]

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 <m.wilczynski@samsung.com>

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/20260915-jh7110-clean-send-v4-0-f0e4fd6f2cc8@samsung.com?part=18