Re: [PATCH RESEND 05/17] phy: spacemit: add Innosilicon DP TX PHY driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] PHY provider is registered before probe finishes, exposing it to early concurrent access and potential Use-After-Free (UAF) on probe failure.
- [High] Lack of driver-level synchronization between CCF and PHY frameworks leads to concurrent hardware state corruption.
- [Medium] The MPLL (main link PLL) is powered on during PHY initialization but is never powered off in the power down sequence.
- [Medium] Missing hardware state cleanup on error paths in `k3_inno_dp_phy_power_on` leaves the PHY powered on.
- [Medium] Rate configuration functions unconditionally power on the PLLs, overriding the logical PHY power state.
- [Medium] PLL programming functions do not poll for hardware lock before proceeding.
--

--- Patch [5]: [PATCH RESEND 05/17] phy: spacemit: add Innosilicon DP TX PHY driver ---
commit 31db74eae7dc59ddc61373dbe3e2c6c1e6966ccd
Author: Cody Kang <codykang.hk@gmail.com>
Subject: phy: spacemit: add Innosilicon DP TX PHY driver

This commit introduces the SpacemiT K3 Innosilicon DisplayPort PHY driver.
It integrates the PHY as a child of the DP controller, registering a PHY
provider and exposing the pixel PLL as a clock provider.

> diff --git a/drivers/phy/spacemit/phy-k3-inno-dp.c b/drivers/phy/spacemit/phy-k3-inno-dp.c
> new file mode 100644
> index 0000000000000..9dd62edb7a5af
> --- /dev/null
> +++ b/drivers/phy/spacemit/phy-k3-inno-dp.c
> @@ -0,0 +1,948 @@

[ ... ]

> +static int k3_inno_dp_phy_power_on(struct phy *phy)
> +{
> +	struct k3_inno_dp_phy *p = phy_get_drvdata(phy);
> +	u32 lane_en;
> +	u32 val;
> +	int ret;

[ ... ]

> +	ret = regmap_read_poll_timeout(p->regmap, DPTX_ANA_MPLL, val,
> +				       val & DPTX_ANA_MPLL_LOCKED,
> +				       2000, 10 * 1000);
> +	if (ret) {
> +		dev_err(p->dev, "DP PHY core PLL lock timed out\n");
> +		return ret;
> +	}

[Severity: Medium]
If the MPLL or PREPLL fails to lock within 10ms, this immediately returns an
error without restoring the power down bits or XMIT_EN.

Since the generic PHY framework does not automatically call phy_power_off()
when phy_power_on() fails, could this leave the hardware running in an
inconsistent state and leak power?

> +
> +	ret = regmap_read_poll_timeout(p->regmap, DPTX_ANA_PREPLL, val,
> +				       val & DPTX_ANA_PREPLL_LOCKED,
> +				       2000, 10 * 1000);
> +	if (ret)
> +		dev_err(p->dev, "DP PHY pixel PLL lock timed out\n");
> +
> +	return ret;
> +}
> +
> +static int k3_inno_dp_phy_power_off(struct phy *phy)
> +{
> +	struct k3_inno_dp_phy *p = phy_get_drvdata(phy);
> +
> +	regmap_write_bits(p->regmap, DPTX_PHY_CTRL, DPTX_PHY_CTRL_XMIT_EN, 0);
> +	usleep_range(2000, 4000);
> +
> +	regmap_write_bits(p->regmap, DPTX_ANA_PREPLL, DPTX_ANA_PREPLL_PD,
> +			  DPTX_ANA_PREPLL_PD);
> +	usleep_range(2000, 4000);
> +
> +	return 0;
> +}

[Severity: Medium]
Does this power down sequence omit the MPLL?

The k3_inno_dp_phy_power_on() function powers up the MPLL by clearing
DPTX_ANA_MPLL_PD, but this power_off function does not appear to re-assert
it. Could this leave the main link PLL running indefinitely when the DP
controller is suspended or torn down?

[ ... ]

> +static void k3_inno_dp_phy_program_mpll(struct k3_inno_dp_phy *p,
> +					const struct k3_inno_dp_mpll_cfg *cfg)
> +{
> +	struct regmap *rm = p->regmap;

[ ... ]

> +	regmap_write_bits(rm, DPTX_ANA_PREPLL_CTRL, DPTX_ANA_PREPLL_LOCK_BYPEN,
> +		FIELD_PREP(DPTX_ANA_PREPLL_LOCK_BYPEN, 1));
> +
> +	regmap_write_bits(rm, DPTX_ANA_MPLL, DPTX_ANA_MPLL_PD,
> +		FIELD_PREP(DPTX_ANA_MPLL_PD, 0));

[Severity: Medium]
Does unconditionally writing 0 to DPTX_ANA_MPLL_PD override the logical
power state of the PHY?

If a DRM driver configures the link rate (calling phy_configure()) while the
PHY is logically powered off, this will force the PLL to run and
desynchronize the hardware from the PHY framework's power state.

> +	usleep_range(2000, 4000);
> +}

[Severity: Medium]
Is it safe to rely on a fixed 2-4ms sleep here instead of polling
DPTX_ANA_MPLL_LOCKED?

The power_on() function polls the locked bit for up to 10ms. A fixed delay
here might proceed with an unstable clock if the hardware takes longer to
lock, potentially causing display failures or link training issues.

[ ... ]

> +static void k3_inno_dp_program_prepll(struct k3_inno_dp_phy *p,
> +				      const struct k3_inno_dp_prepll_cfg *cfg)
> +{
> +	regmap_write_bits(p->regmap, DPTX_ANA_PREPLL, DPTX_ANA_PREPLL_PD,
> +		FIELD_PREP(DPTX_ANA_PREPLL_PD, 1));
> +	usleep_range(2000, 4000);

[Severity: High]
Is a driver-level lock needed to synchronize access to the PHY registers
between the CCF and PHY frameworks?

Since this driver exposes both a clock (via CCF) and a PHY that share the
same hardware registers, concurrent calls to clk_set_rate() and
phy_power_on() can interleave.

If clk_set_rate() begins this multi-step programming sequence, a concurrent
phy_power_on() could clear the PD bit while the clock is still being
configured, corrupting the hardware state.

[ ... ]

> +	regmap_write_bits(p->regmap, DPTX_ANA_PREPLL,
> +		DPTX_ANA_PREPLL_PCLK_NORMAL,
> +		FIELD_PREP(DPTX_ANA_PREPLL_PCLK_NORMAL, 1));
> +	usleep_range(2000, 4000);
> +
> +	regmap_write_bits(p->regmap, DPTX_ANA_PREPLL, DPTX_ANA_PREPLL_PD,
> +		FIELD_PREP(DPTX_ANA_PREPLL_PD, 0));

[Severity: Medium]
Similar to the MPLL configuration, does unconditionally clearing
DPTX_ANA_PREPLL_PD here override the PHY power state if the clock rate is
set while the PHY is logically off?

> +	usleep_range(2000, 4000);
> +}

[Severity: Medium]
Is it safe to rely on a fixed delay here instead of polling the
DPTX_ANA_PREPLL_LOCKED bit, which could lead to proceeding with an unstable
pixel clock?

[ ... ]

> +static int k3_inno_dp_phy_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct k3_inno_dp_phy *p;
> +	struct phy_provider *provider;
> +	struct clk_init_data init = {};
> +	int ret;

[ ... ]

> +	provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
> +	if (IS_ERR(provider))
> +		return dev_err_probe(dev, PTR_ERR(provider),
> +				     "failed to register PHY provider\n");
> +
> +	/* PHY takes a 24 MHz reference clock */
> +	p->ref_clk_khz = 24000;
> +
> +	/* The APMU pixel-clock mux references this clock by name. */
> +	of_property_read_string(dev->of_node, "clock-output-names", &init.name);
> +	if (!init.name)
> +		init.name = dev_name(dev);
> +	init.ops = &k3_inno_dp_pxclk_ops;
> +	init.parent_names = NULL;
> +	init.num_parents = 0;
> +	init.flags = 0;
> +	p->pxclk_hw.init = &init;
> +
> +	ret = devm_clk_hw_register(dev, &p->pxclk_hw);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "failed to register pixel clock\n");

[Severity: High]
Can the PHY provider be accessed by consumers before probe completes
successfully?

The PHY provider is registered via devm_of_phy_provider_register() before
the pixel clock is registered. If devm_clk_hw_register() fails, probe will
return an error and devres will free the driver context struct.

If a consumer driver acquires a reference to the PHY during this race
window, it could retain the reference after probe fails, leading to a
use-after-free when it attempts PHY operations on the freed pointer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260725-k3-display-v1-0-6de34d80e86c@gmail.com?part=5