Re: [PATCH 07/11] drm/mediatek: mtk_dp: Add support for PHY from devicetree

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Double unregister of the mediatek-dp-phy platform device (`mtk_dp->phy_dev`) on error paths.
- [High] Legacy PHY registers are incorrectly offset by `mtk_dp->legacy_regoff`, corrupting DP TOP and ENC registers and breaking PHY initialization on legacy device trees.
- [Medium] The HDMI audio codec platform device (`mtk_dp->audio_pdev`) is leaked on driver removal or probe failure.
--

commit 60c3f9c6bb6ec600fc25d97fe75494a73e71c05b
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

    drm/mediatek: mtk_dp: Add support for PHY from devicetree
    
    Add support for specifying phys in devicetree to pass handle
    to the DisplayPort PHY.
    
    In order to retain compatibility with older devicetrees, check if
    phys was specified: if not, initialize the regmap_mmio with the
    legacy configuration and register the mediatek-dp-phy platform
    device from this driver, and get the PHY calibration data.

> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
> index 3b5348ab487de..bf3a4b15f3ec0 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
[ ... ]
> @@ -2769,6 +2790,7 @@ static int mtk_dp_register_phy(struct mtk_dp *mtk_dp)
>  				     "Failed to add phy unregister devm action");
>  	}
>  
> +	/* PHY calibration data is in mtk_dp only for legacy devicetree */
>  	mtk_dp_get_calibration_data(mtk_dp);
>  
>  	mtk_dp->phy = devm_phy_get(&mtk_dp->phy_dev->dev, "dp");

[Severity: High]
Does this code double-unregister the mediatek-dp-phy platform device?

Earlier in mtk_dp_register_phy(), devm_add_action_or_reset() is called to
register mtk_dp_unregister_pdevs. If devm_phy_get() fails, this error path
explicitly calls platform_device_unregister().

When returning an error, devres will unwind and execute the previously
registered devm action (mtk_dp_unregister_pdevs), unregistering the device
a second time.

Similarly, the devm_add_action_or_reset() error path explicitly unregisters
the device, but devm_add_action_or_reset() automatically executes the cleanup
function if it fails, also resulting in an immediate double-unregister.

[ ... ]
> @@ -2821,7 +2843,12 @@ static int mtk_dp_probe(struct platform_device *pdev)
>  
>  	mtk_dp->dev = dev;
>  	mtk_dp->data = (struct mtk_dp_data *)of_device_get_match_data(dev);
> -	mtk_dp->legacy_regoff = MTK_DP_TOP_OFFSET_LEGACY;
> +
> +	/* Prefer PHY from devicetree - if not found, this is legacy */
> +	if (of_property_present(dev->of_node, "phys"))
> +		mtk_dp->legacy_regoff = 0;
> +	else
> +		mtk_dp->legacy_regoff = MTK_DP_TOP_OFFSET_LEGACY;

[Severity: High]
Does adding mtk_dp->legacy_regoff to all register accesses break legacy PHY
initialization?

The code relies on mtk_dp_update_bits() and mtk_dp_write() which unconditionally
add mtk_dp->legacy_regoff (0x2000 for legacy device trees) to all register
accesses:

	int ret = regmap_update_bits(mtk_dp->regs,
				     offset + mtk_dp->legacy_regoff,
				     mask, val);

While this is correct for DP TOP and DP ENC registers that were rebased to 0x0
in the new header, the legacy PHY registers (like DP_PHY_GLB_BIAS_GEN_00 and
DP_PHY_AUX_RX_CTL) still use their original absolute offsets starting at 0x0.

Writing to them now ends up at 0x0 + 0x2000 = 0x2000, overwriting
MTK_DP_TOP_PWR_STATE instead of the intended PHY calibration registers.
Writing to DP_PHY_AUX_RX_CTL (0x1040) writes to 0x3040, which corrupts the
ENC0 register space.

>  
>  	ret = mtk_dp_dt_parse(mtk_dp, pdev);
>  	if (ret)
[ ... ]
> @@ -2872,9 +2899,18 @@ static int mtk_dp_probe(struct platform_device *pdev)
>  					     "Failed to register audio driver\n");
>  	}
>  

[Severity: Medium]
Is the HDMI audio codec platform device (mtk_dp->audio_pdev) leaked on driver
removal or probe failure?

In mtk_dp_register_audio_driver(), a devm action is registered to clean up
mtk_dp->audio_pdev. However, it incorrectly passes mtk_dp->phy_dev as the data
argument:

	ret = devm_add_action_or_reset(dev, mtk_dp_unregister_pdevs,
				       mtk_dp->phy_dev);

Because mtk_dp_register_audio_driver() runs before mtk_dp_register_phy(),
mtk_dp->phy_dev is NULL here. When the driver is unbound or probe fails, devres
executes platform_device_unregister(NULL) which does nothing, but the actual
audio platform device remains registered and is leaked.

> -	ret = mtk_dp_register_phy(mtk_dp);
> -	if (ret)
> -		return ret;
> +	if (!mtk_dp->legacy_regoff) {
> +		mtk_dp->phy = devm_phy_get(dev, NULL);
> +		if (IS_ERR(mtk_dp->phy))
> +			return dev_err_probe(dev, PTR_ERR(mtk_dp->phy),
> +					     "Failed to get phy\n");
> +
> +		mtk_dp->phy_dev = NULL;
> +	} else {
> +		ret = mtk_dp_register_phy(mtk_dp);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	mtk_dp->bridge.of_node = dev->of_node;
>  	mtk_dp->bridge.type = mtk_dp->data->bridge_type;

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