Re: [PATCH v2 13/15] phy: starfive: Add jh7110-inno-hdmi-phy driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing cleanup on the clk_set_rate error path in probe leaves a dangling clock provider.
- [High] Clock operations incorrectly modify analog PHY state without synchronization, risking state corruption and power leaks.
- [Medium] Hardware PHY resources are left powered on if starfive_hdmi_phy_power_on() fails.
--

commit 7c46f9e86f09391b8c542e940da1457e36661874
Author: Michal Wilczynski <m.wilczynski@samsung.com>

phy: starfive: Add jh7110-inno-hdmi-phy driver

This commit adds the HDMI PHY driver for the StarFive JH7110. It provides
the variable pixel clock from the Pre-PLL and the PHY operations for the
Post-PLL and analog blocks.

> diff --git a/drivers/phy/starfive/phy-jh7110-inno-hdmi.c b/drivers/phy/starfive/phy-jh7110-inno-hdmi.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/phy/starfive/phy-jh7110-inno-hdmi.c
[ ... ]
> +static int starfive_hdmi_phy_clk_set_rate(struct clk_hw *hw, unsigned long rate,
> +					  unsigned long parent_rate)
> +{
[ ... ]
> +	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);
> +	inno_write(inno, STF_INNO_RX_CONTROL, STF_INNO_RX_ENABLE);

[Severity: High]
Does this incorrectly modify analog PHY state during clock operations?

Since clk_set_rate() is unconditionally called during starfive_hdmi_phy_probe(),
these analog blocks are powered on immediately at boot, which could cause a
permanent power leak until the power_off callback happens to be called.

Furthermore, do these state modifications race with the PHY callbacks? The
common clock framework callbacks execute under the CCF prepare_lock, while PHY
callbacks execute under the phy->mutex lock. A concurrent clk_set_rate() and
starfive_hdmi_phy_power_on() could race on hardware registers and shared driver
state like inno->pixclock.

[ ... ]
> +static int starfive_hdmi_phy_power_on(struct phy *phy)
> +{
[ ... ]
> +	inno_write(inno, STF_INNO_POST_PLL_DIV_1, reg_1aa_value);
> +
> +	/* Wait for post PLL lock */
> +	ret = inno_poll(inno, STF_INNO_POST_PLL_LOCK_STATUS, v,
> +			v & STF_INNO_POST_PLL_LOCK, 1000, 100000);
> +	if (ret) {
> +		dev_err(inno->dev, "Post-PLL locking failed\n");
> +		return ret;
> +	}

[Severity: Medium]
Are the hardware PHY resources left powered on if the Post-PLL lock times out?

If inno_poll() fails, the function returns immediately. The PHY core framework
does not automatically call the power_off callback if the power_on callback
fails, because the power_count remains 0.

Does this error path need to explicitly undo the register writes that enabled
the BIAS, RX, and Post-PLL blocks earlier in this function?

[ ... ]
> +static int starfive_hdmi_phy_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	ret = clk_set_rate(inno->phyclk, 297000000);
> +	if (ret) {
> +		dev_err(dev, "Failed to set default rate: %d\n", ret);
> +		goto err_disable_refoclk;
> +	}
[ ... ]
> +err_del_clk_provider:
> +	of_clk_del_provider(dev->of_node);
> +err_disable_refoclk:
> +	clk_disable_unprepare(inno->refoclk);
> +	return ret;
> +}

[Severity: High]
Does this error path leave a dangling clock provider in the global list?

If clk_set_rate() fails, the code jumps to err_disable_refoclk, bypassing
the required of_clk_del_provider() cleanup. The devres framework will then
unregister and free inno->phyclk, but the device node will remain in the
system-wide of_clk_providers list pointing to freed memory. Any subsequent
clock lookup could trigger a use-after-free crash.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828-jh7110-clean-send-v2-0-331680c8b9d1@samsung.com?part=13

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy