Re: [PATCH v9 5/5] phy: qualcomm: add MSM8974 HDMI PHY support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Integer truncation when passing vco_freq to write16() writes garbage calibration data
- [High] Concurrent access to PLL registers between clock and PHY frameworks without a shared lock
- [High] User-controllable input triggers a WARN_ON(), enabling a kernel panic DoS
- [Medium] Error path in qcom_hdmi_msm8974_phy_pll_enable() leaks power by failing to restore PD_CTRL0
- [Medium] The read24() helper lacks bitmasks, leading to potential data corruption from upper register bits
--

commit d2fb8e86ab045b987f00fdf674f81b4bc2d4b950
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

phy: qualcomm: add MSM8974 HDMI PHY support
    
Add support for HDMI PHY on Qualcomm MSM8974 / APQ8074 platforms.

> diff --git a/drivers/phy/qualcomm/phy-qcom-hdmi-28hpm.c b/drivers/phy/qualcomm/phy-qcom-hdmi-28hpm.c
> index 720757f8f3931..7d398060b3a3c 100644
> --- a/drivers/phy/qualcomm/phy-qcom-hdmi-28hpm.c
> +++ b/drivers/phy/qualcomm/phy-qcom-hdmi-28hpm.c
[ ... ]
> @@ -31,8 +34,282 @@
>  #define REG_HDMI_8x74_BIST_PATN3				0x00000048
>  #define REG_HDMI_8x74_STATUS					0x0000005c
[ ... ]
> +static inline u32 read24(void __iomem *reg)
> +{
> +	u32 val = readl(reg);
> +
> +	val |= readl(reg + 4) << 8;
> +	val |= readl(reg + 8) << 16;

Can this result in data corruption if the hardware bus returns non-zero upper
bits for these registers?

Unlike write24() which correctly masks its values with & 0xff, the readl()
results are not masked before bit-shifting. If the hardware returns reserved
or uninitialized upper bits, they will bleed into and corrupt the assembled
24-bit value used in qcom_uniphy_recalc().

> +
> +	return val;
> +}
> +
> +static void qcom_uniphy_setup(void __iomem *base, unsigned int ref_freq,
> +			      bool sdm_mode,
> +			      bool ref_freq_mult_2,
> +			      bool dither,
> +			      unsigned int refclk_div,
> +			      unsigned int vco_freq)
> +{
[ ... ]
> +	write16(mult_frac(ref_freq, 5, 1000000), base + UNIPHY_PLL_CAL_CFG8);
> +	write16(vco_freq / 16, base + UNIPHY_PLL_CAL_CFG10);

Will this cause integer truncation and misconfigure the PLL calibration?

The vco_freq parameter can be up to 1.8 GHz, meaning vco_freq / 16 can yield
a value up to ~112.5 million, which requires 27 bits. Since write16() takes a
u16 parameter, the upper 11 bits are silently discarded, and only the lower 16
bits are written to the CFG10 and CFG11 registers.

> +}
> +
> +static unsigned long qcom_uniphy_recalc(void __iomem *base, unsigned long parent_rate)
> +{
> +	unsigned long rate;
> +	u32 refclk_cfg;
> +	u32 dc_offset;
> +	u64 fraq_n;
> +	u32 val;
> +
> +	refclk_cfg = readl(base + UNIPHY_PLL_REFCLK_CFG);
[ ... ]
> +	val = readl(base + UNIPHY_PLL_SDM_CFG0);
> +	if (FIELD_GET(UNIPHY_PLL_SDM_BYP, val)) {
> +		dc_offset = FIELD_GET(UNIPHY_PLL_SDM_BYP_DIV, val);
> +		fraq_n = 0;
> +	} else {
> +		dc_offset = FIELD_GET(UNIPHY_PLL_SDM_DC_OFFSET,
> +				      readl(base + UNIPHY_PLL_SDM_CFG1));
> +		fraq_n = read24(base + UNIPHY_PLL_SDM_CFG2);
> +	}

Does this code race with qcom_hdmi_msm8974_phy_power_on()?

The PHY driver exposes a clock provider, and this recalc_rate callback reads
shared PLL hardware registers protected by the clock framework's prepare_lock.
However, qcom_hdmi_msm8974_phy_power_on() writes to these exact same PLL
registers while holding an unrelated PHY mutex.

Without a shared lock synchronizing these paths, reading from the clk_summary
debugfs file concurrently with a PHY power-on could cause torn reads,
especially since the 24-bit fractional divider is written across three
separate writel() calls.

[ ... ]
> +static int qcom_hdmi_msm8974_phy_pll_set_rate(struct qcom_hdmi_preqmp_phy *hdmi_phy)
> +{
> +	unsigned long long pixclk = hdmi_phy->hdmi_opts.tmds_char_rate;
> +	unsigned long vco_rate;
> +	unsigned int div;
> +	int div_idx = 0;
> +
> +	div_idx = qcom_hdmi_msm8974_phy_find_div(pixclk);
> +	if (WARN_ON(div_idx < 0))
> +		return div_idx;

Is it safe to use a WARN_ON() for user-controllable input?

The requested pixel clock (hdmi_opts.tmds_char_rate) is influenced by DRM
modesetting or an external monitor's EDID. An unprivileged user could trigger
this path by requesting an unsupported display mode, intentionally hitting
this WARN_ON().

On systems configured with panic_on_warn=1, this would crash the kernel,
potentially leading to a local denial of service.

[ ... ]
> +static int qcom_hdmi_msm8974_phy_pll_enable(struct qcom_hdmi_preqmp_phy *hdmi_phy)
> +{
> +	int ret;
> +	unsigned long status;
> +
> +	/* Global enable */
> +	writel(0x81, hdmi_phy->phy_reg + REG_HDMI_8x74_GLB_CFG);
> +
> +	/* Power up power gen */
> +	writel(0x00, hdmi_phy->phy_reg + REG_HDMI_8x74_PD_CTRL0);
> +	udelay(350);
[ ... ]
> +	/* Poll for PHY ready status */
> +	ret = readl_poll_timeout(hdmi_phy->phy_reg + REG_HDMI_8x74_STATUS,
> +				 status, status & BIT(0),
> +				 100, 2000);
> +	if (ret) {
> +		dev_warn(hdmi_phy->dev, "HDMI PHY not ready\n");
> +		goto err;
> +	}
> +
> +	return 0;
> +
> +err:
> +	writel(0, hdmi_phy->pll_reg + UNIPHY_PLL_GLB_CFG);
> +	udelay(5);
> +	writel(0, hdmi_phy->phy_reg + REG_HDMI_8x74_GLB_CFG);
> +
> +	return ret;
> +}

Does this error path leave the PHY partially powered up?

During the power-on sequence, the PHY is powered up by writing to
REG_HDMI_8x74_PD_CTRL0. If the subsequent polling fails, the code jumps to
the err label.

The error path clears the global configuration registers but fails to write
0x7f back to REG_HDMI_8x74_PD_CTRL0 to power down the generator. Because the
function returns an error, the PHY framework will not invoke the power_off
callback, leaking power indefinitely.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260513-fd-hdmi-phy-v9-0-ca98c72f1f9f@oss.qualcomm.com?part=5

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