Re: [PATCH v3 06/11] drm/mediatek: mtk_dp: Use PHY API for PHY power sequences

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Double unregistration of the `mtk_dp->phy_dev` platform device on the `devm_phy_get()` error path causes a use-after-free.
- [Medium] The newly added `phy_power_on()`/`phy_power_off()` calls are dead code, and if reachable, they would violate the PHY API contract and leak reference counts.
--

--- Patch [6]: [PATCH v3 06/11] drm/mediatek: mtk_dp: Use PHY API for PHY power sequences ---
commit e6bfbadbdfc23b97fa303c95119a421df2604e0a
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

drm/mediatek: mtk_dp: Use PHY API for PHY power sequences

The PHY driver has gained support for .power_on() and .power_off()
callbacks: use the API provided phy_power_on(), phy_power_off()
functions instead of writing into PHY registers from this driver.

I noticed an issue in mtk_dp_register_phy() from patch 4 of this series:

drivers/gpu/drm/mediatek/mtk_dp.c:mtk_dp_register_phy() {
    ...
	ret = devm_add_action_or_reset(dev, mtk_dp_unregister_pdevs, mtk_dp->phy_dev);
	if (ret)
		return dev_err_probe(dev, ret,
				     "Failed to add phy unregister devm action");
    ...
	mtk_dp->phy = devm_phy_get(&mtk_dp->phy_dev->dev, "dp");
	if (IS_ERR(mtk_dp->phy)) {
		platform_device_unregister(mtk_dp->phy_dev);
		return dev_err_probe(dev, PTR_ERR(mtk_dp->phy), "Failed to get phy\n");
	}
    ...
}

[Severity: High]
Does this explicit unregister conflict with the devm action registered earlier?

If devm_phy_get() fails, platform_device_unregister() is called on
mtk_dp->phy_dev, and then an error is returned. Returning an error triggers
the devres cleanup, which executes the registered devm action and calls
platform_device_unregister() on the exact same pointer a second time, causing
a double-free and use-after-free.

> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
> index 409d089550a90..b4cb1996a3314 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
> @@ -1414,20 +1416,35 @@ static void mtk_dp_power_enable(struct mtk_dp *mtk_dp)
>  			   SW_RST_B_PHYD, SW_RST_B_PHYD);
>  	mtk_dp_update_bits(mtk_dp, MTK_DP_TOP_PWR_STATE,
>  			   DP_PWR_STATE_BANDGAP_TPLL, DP_PWR_STATE_MASK);
> -	regmap_write(mtk_dp->regs, DP_PHY_AUX_RX_CTL,
> -		     RG_DPAUX_RX_VALID_DEGLITCH_EN | RG_XTP_GLB_CKDET_EN |
> -		     RG_DPAUX_RX_EN);
> -	regmap_clear_bits(mtk_dp->regs, MTK_DP_0034, DA_CKM_CKTX0_EN_FORCE_EN);
> +
> +	if (mtk_dp->phy_dev) {

[Severity: Medium]
The commit message claims this patch replaces manual PHY register writes with
phy_power_on() and phy_power_off(). However, these API calls are placed in
the else branch of if (mtk_dp->phy_dev).

Because mtk_dp_register_phy() unconditionally registers the dummy platform
device unless it fails (which would abort the probe), mtk_dp->phy_dev is
always a valid pointer.

This makes the if-condition always true, so the new phy_power_on() and
phy_power_off() API calls are never reached, and the legacy manual writes
continue to execute. Are these new calls unreachable dead code?

Additionally, if this code were reachable (for example, for a future
DT-provided PHY), does phy_power_on() get called during eDP probe before
phy_init() is called, violating the PHY API contract? 

And if phy_power_on() fails, the failure is warned but not propagated, leaving
the PHY power count un-incremented. This could cause a subsequent
phy_power_off() to underflow the reference count.

> +		regmap_write(mtk_dp->regs, DP_PHY_AUX_RX_CTL,
> +			     RG_DPAUX_RX_VALID_DEGLITCH_EN | RG_XTP_GLB_CKDET_EN |
> +			     RG_DPAUX_RX_EN);
> +		regmap_clear_bits(mtk_dp->regs, MTK_DP_0034, DA_CKM_CKTX0_EN_FORCE_EN);
> +	} else {
> +		ret = phy_power_on(mtk_dp->phy);
> +		if (ret)
> +			dev_warn(mtk_dp->dev, "Could not power on PHY!\n");
> +	}
>  }
>  
>  static void mtk_dp_power_disable(struct mtk_dp *mtk_dp)
>  {
> +	int ret;
> +
>  	mtk_dp_write(mtk_dp, MTK_DP_TOP_PWR_STATE, 0);
>  
> -	regmap_set_bits(mtk_dp->regs, MTK_DP_0034, DA_CKM_CKTX0_EN_FORCE_EN);
> +	if (unlikely(mtk_dp->phy_dev)) {
> +		regmap_set_bits(mtk_dp->regs, MTK_DP_0034, DA_CKM_CKTX0_EN_FORCE_EN);
>  
> -	/* Disable RX */
> -	regmap_write(mtk_dp->regs, DP_PHY_AUX_RX_CTL, 0);
> +		/* Disable RX */
> +		regmap_write(mtk_dp->regs, DP_PHY_AUX_RX_CTL, 0);
> +	} else {
> +		ret = phy_power_off(mtk_dp->phy);
> +		if (ret)
> +			dev_warn(mtk_dp->dev, "Could not power off PHY!\n");
> +	}
>  
>  	mtk_dp_write(mtk_dp, MTK_DP_TOP_MEM_PD,
>  		     0x550 | FUSE_SEL | MEM_ISO_EN);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709103431.46616-1-angelogioacchino.delregno@collabora.com?part=6