Re: [PATCH v2 05/12] phy: phy-mtk-dp: Implement power_on and power_off PHY callbacks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [Medium] mtk_dp_phy_power_off() aborts early on regmap_set_bits failure, leaving the RX channel perpetually enabled.
- [Medium] Missing check for device_get_match_data() returning NULL in mtk_dp_phy_probe, leading to a crash in PHY operations.
- [Low] Undeclared variable 'ret' in mtk_dp_phy_probe() causes a strict build failure.

Pre-existing issues:
- [High] Dereferencing dev->platform_data without checking for NULL in mtk_dp_phy_legacy_probe leads to a kernel panic.
--

--- Patch [5]: [PATCH v2 05/12] phy: phy-mtk-dp: Implement power_on and power_off PHY callbacks ---
commit db273838783f1ea341d7728660183966ad3c4d16
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

phy: phy-mtk-dp: Implement power_on and power_off PHY callbacks

This commit adds .power_on() and .power_off() callbacks to the DisplayPort
PHY operations. These additions allow the DisplayPort driver to use the
phy_power_on() and phy_power_off() APIs instead of hardcoded register writes.

> diff --git a/drivers/phy/mediatek/phy-mtk-dp.c b/drivers/phy/mediatek/phy-mtk-dp.c
> index 915f148325c49..0c3f06791613a 100644
> --- a/drivers/phy/mediatek/phy-mtk-dp.c
> +++ b/drivers/phy/mediatek/phy-mtk-dp.c

[ ... ]

> @@ -200,6 +225,59 @@ static int mtk_dp_phy_configure(struct phy *phy, union phy_configure_opts *opts)
>  	return 0;
>  }
>  
> +static int mtk_dp_phy_power_on(struct phy *phy)
> +{

[ ... ]

> +static int mtk_dp_phy_power_off(struct phy *phy)
> +{
> +	struct mtk_dp_phy *dp_phy = phy_get_drvdata(phy);
> +	const struct mtk_dp_phy_pdata *pdata = dp_phy->pdata;
> +	const u8 *regs_dig = pdata->regs_dig_glb;
> +	const u8 *regs_ana = pdata->regs_ana_glb;
> +	int ret;
> +
> +	ret = regmap_set_bits(dp_phy->regmap,
> +			      pdata->off_ana_glb + regs_ana[DP_PHYA_GLB_FORCE_CTRL_1],
> +			      CKM_CKTX0_EN_FORCE_MODE);
> +	if (ret)
> +		return ret;

[Severity: Medium]
If regmap_set_bits() fails and returns early, could this leave the hardware
RX channel powered on indefinitely? 

Teardown steps usually proceed on a best-effort basis to ensure subsequent
hardware disable operations are executed.

> +
> +	/* Disable RX */
> +	ret = regmap_write(dp_phy->regmap,
> +			   pdata->off_dig_glb + regs_dig[DP_PHYD_AUX_RX_CTL], 0);

[ ... ]

> @@ -319,8 +399,10 @@ static int mtk_dp_phy_probe(struct platform_device *pdev)
>  }

[Severity: Low]
Does this code cause a build failure?

drivers/phy/mediatek/phy-mtk-dp.c:mtk_dp_phy_probe() {
    ...
    ret = devm_pm_runtime_enable(dev);
    if (ret)
        return ret;
    ...
}

It appears the variable 'ret' is used but not declared.

[Severity: Medium]
Does this code need a NULL check for the return value of
device_get_match_data()?

drivers/phy/mediatek/phy-mtk-dp.c:mtk_dp_phy_probe() {
    ...
    dp_phy->pdata = device_get_match_data(dev);

    phy = devm_phy_create(dev, NULL, &mtk_dp_phy_dev_ops);
    ...
}

If the device is bound manually via sysfs and the compatible string doesn't
match, device_get_match_data() could return NULL, which might crash the
kernel in subsequent operations like mtk_dp_phy_power_on().

[Severity: High]
This is a pre-existing issue, but does dereferencing dev->platform_data
without checking for NULL in mtk_dp_phy_legacy_probe() lead to a crash?

drivers/phy/mediatek/phy-mtk-dp.c:mtk_dp_phy_legacy_probe() {
    ...
    dp_phy->regmap = *(struct regmap **)dev->platform_data;
    if (!dp_phy->regmap)
        return dev_err_probe(dev, -EINVAL, "No platform data available\n");
    ...
}

If pdev->dev.platform_data is NULL, the blind pointer dereference happens
before the sanity check is reached.

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