Re: [PATCH v3 17/19] phy: starfive: Add jh7110-inno-hdmi-phy driver

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Unconditional register access in `clk_set_rate` and `probe` will cause a CPU wedge if the HDMI system clock is gated.
- [High] `phy_power_off` unconditionally disables shared analog blocks (BIAS and RX), breaking the pixel clock output if `hdmi_pclk` is still running.
- [Medium] `clk_set_rate` unconditionally powers on the Pre-PLL and analog blocks, violating CCF semantics and leaking power.
- [Medium] Data race on state variables and unprotected register sequences between Clock and PHY callbacks.
--

commit 08ad435ba48340f7f566c8d19a6a67d1faa2ca9d
Author: Michal Wilczynski [off-list ref]
Subject: phy: starfive: Add jh7110-inno-hdmi-phy driver

This commit adds the HDMI PHY driver for the StarFive JH7110. It registers 
as a clock provider for the pre-PLL pixel clock and a PHY provider for the 
post-PLL analog blocks.

[ ... ]

+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;

[ ... ]

+	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 register access execute blindly without checking if the clock is
prepared? 

The commit message mentions that unclocked accesses wedge the bus hard enough
that the CPU stops taking NMIs. If clk_set_rate() is called while the clock is
unprepared, could this cause the described system crash?

[Severity: Medium]
Additionally, could this unconditionally power on the shared analog blocks
and violate the Common Clock Framework semantics? 

CCF allows clk_set_rate() to be called on an unprepared clock, which should
merely update the configuration. If the clock remains unprepared, do these
blocks remain powered on and unnecessarily leak power?

+
+	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]
Does this also force the Pre-PLL hardware into a powered-up and running state
without checking if the clock is prepared, potentially leaking power?

+
+	inno->pixclock = rate;
+	inno->tmdsclock = cfg->tmdsclock;

[Severity: Medium]
Is it safe to modify this shared state under the CCF's prepare_lock?

The PHY framework might concurrently modify these variables under phy->mutex in
starfive_hdmi_phy_power_down(). Could this data race cause inconsistent
internal state, or allow the multi-register hardware configuration sequences
to dangerously interleave?

+
+	return 0;
+}

[ ... ]

+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);

[Severity: High]
Does this unconditionally disable the shared BIAS and RX analog blocks, even
if the hdmi_pclk clock provider is still running? 

If the PHY is powered off, but the VOUT subsystem still expects the pixel
clock to be active, will this abruptly kill the clock signal behind the clock
framework's back?

+
+	/* 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 appears to be the other side of the data race mentioned above. Does
modifying this state under the PHY mutex race with the CCF reading or
writing it via clk_set_rate() or clk_recalc_rate()?

+}

[ ... ]

+static int starfive_hdmi_phy_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device *parent = dev->parent;

[ ... ]

+	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);
+	if (ret) {
+		dev_err(dev, "Failed to set default rate: %d\n", ret);
+		goto err_del_clk_provider;
+	}

[Severity: High]
Does this synchronous clk_set_rate() call hit the PHY registers before the
sibling controller is guaranteed to have enabled the system clock?

Because the PHY driver can probe before the bridge driver, could this happen
while the clock is gated, triggering the system crash (CPU NMI stall)
described in the commit message?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904-jh7110-clean-send-v3-0-484f9ae72715@samsung.com?part=17

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