Re: [PATCH 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 3 potential issue(s) to consider:
- [High] Double unregister of the PHY platform device on error paths.
- [High] HDMI audio codec platform device is leaked due to a copy-paste error.
- [High] Ignoring the return value of phy_power_on() causes an unbalanced phy_power_off() call if power-on fails.
--

commit 6e6e078245707d85aa500b0a872e31948a456e4f
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.

> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
> index 97bce8966a1f3..3b5348ab487de 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
> @@ -1425,21 +1427,37 @@ 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);
> -	mtk_dp_write(mtk_dp, DP_PHY_AUX_RX_CTL,
> -		     RG_DPAUX_RX_VALID_DEGLITCH_EN | RG_XTP_GLB_CKDET_EN |
> -		     RG_DPAUX_RX_EN);
> -	mtk_dp_update_bits(mtk_dp, MTK_DP_0034, 0, DA_CKM_CKTX0_EN_FORCE_EN);
> +
> +	if (mtk_dp->phy_dev) {
> +		mtk_dp_write(mtk_dp, DP_PHY_AUX_RX_CTL,
> +			     RG_DPAUX_RX_VALID_DEGLITCH_EN | RG_XTP_GLB_CKDET_EN |
> +			     RG_DPAUX_RX_EN);
> +		mtk_dp_update_bits(mtk_dp, MTK_DP_0034, 0, 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");

[Severity: High]
Does ignoring the return value of phy_power_on() cause an unbalanced
phy_power_off() call if power-on fails?

If phy_power_on() fails, the error is swallowed and only a warning is printed.
The function returns normally, leaving the PHY core's internal power_count at 0.

> +	}
>  }
>  
>  static void mtk_dp_power_disable(struct mtk_dp *mtk_dp)
>  {
> +	int ret;
> +
>  	mtk_dp_write(mtk_dp, MTK_DP_TOP_PWR_STATE, 0);
>  
> -	mtk_dp_update_bits(mtk_dp, MTK_DP_0034,
> +	if (unlikely(mtk_dp->phy_dev)) {
> +		mtk_dp_update_bits(mtk_dp, MTK_DP_0034,
>  			   DA_CKM_CKTX0_EN_FORCE_EN, DA_CKM_CKTX0_EN_FORCE_EN);
>  
> -	/* Disable RX */
> -	mtk_dp_write(mtk_dp, DP_PHY_AUX_RX_CTL, 0);
> +		/* Disable RX */
> +		mtk_dp_write(mtk_dp, DP_PHY_AUX_RX_CTL, 0);
> +	} else {
> +		ret = phy_power_off(mtk_dp->phy);

[Severity: High]
Can this unbalanced teardown lead to refcount underflows?

This function unconditionally calls phy_power_off() even if the initial
power-on failed. This decrements power_count to -1 and attempts to disable
regulators and release PM runtime references that were never actually acquired.

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

[ ... ]

The following issues were introduced earlier in this patch series in commit
"drm/mediatek: mtk_dp: Fix hdmi codec and phy driver unregistration":

> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
> @@ -2680,18 +2687,29 @@ static int mtk_dp_register_audio_driver(struct device *dev)
>  		.data = mtk_dp,
>  		.no_capture_mute = 1,
>  	};
> +	int ret;
>  
>  	mtk_dp->audio_pdev = platform_device_register_data(dev,
>  							   HDMI_CODEC_DRV_NAME,
>  							   PLATFORM_DEVID_AUTO,
>  							   &codec_data,
>  							   sizeof(codec_data));
> -	return PTR_ERR_OR_ZERO(mtk_dp->audio_pdev);
> +	if (IS_ERR(mtk_dp->audio_pdev))
> +		return PTR_ERR(mtk_dp->audio_pdev);
> +
> +	ret = devm_add_action_or_reset(dev, mtk_dp_unregister_pdevs, mtk_dp->phy_dev);

[Severity: High]
Does this devm action incorrectly register mtk_dp->phy_dev for cleanup instead
of the newly created mtk_dp->audio_pdev?

Since this function is called before mtk_dp_register_phy(), mtk_dp->phy_dev is
NULL. When the driver is unbound, devres executes mtk_dp_unregister_pdevs(NULL),
and the actual audio_pdev is permanently leaked.

> +	if (ret) {
> +		platform_device_unregister(mtk_dp->audio_pdev);
> +		return dev_err_probe(dev, ret,
> +				     "Failed to add codec unregister devm action");
> +	}
> +	return 0;
>  }
>  
>  static int mtk_dp_register_phy(struct mtk_dp *mtk_dp)
>  {
>  	struct device *dev = mtk_dp->dev;
> +	int ret;
>  
>  	mtk_dp->phy_dev = platform_device_register_data(dev, "mediatek-dp-phy",
>  							PLATFORM_DEVID_AUTO,
> @@ -2701,6 +2719,13 @@ static int mtk_dp_register_phy(struct mtk_dp *mtk_dp)
>  		return dev_err_probe(dev, PTR_ERR(mtk_dp->phy_dev),
>  				     "Failed to create device mediatek-dp-phy\n");
>  
> +	ret = devm_add_action_or_reset(dev, mtk_dp_unregister_pdevs, mtk_dp->phy_dev);
> +	if (ret) {
> +		platform_device_unregister(mtk_dp->phy_dev);

[Severity: High]
Does this error path cause a double unregister of the PHY platform device?

If devm_add_action_or_reset() fails, it automatically executes the cleanup
action (mtk_dp_unregister_pdevs), which calls
platform_device_unregister(mtk_dp->phy_dev). The error handler then manually
calls platform_device_unregister(mtk_dp->phy_dev) a second time.

Alternatively, if devm_add_action_or_reset() succeeds but devm_phy_get() fails
later in the function, the error handler manually unregisters the device. The
probe then fails, and devres cleans up by executing the registered action,
unregistering the device a second time.

> +		return dev_err_probe(dev, ret,
> +				     "Failed to add phy unregister devm action");
> +	}
> +
>  	mtk_dp_get_calibration_data(mtk_dp);
>  
>  	mtk_dp->phy = devm_phy_get(&mtk_dp->phy_dev->dev, "dp");

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