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 <[email protected]> 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/[email protected]?part=13
