Re: [PATCH 04/12] phy: phy-mtk-dp: Migrate register offsets to SoC specific pdata

[sashiko-bot@kernel.org]

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

New issues:
- [High] Missing NULL pointer check for `device_get_match_data()` return value leads to kernel panic.
- [Low] Build failure due to invalid initialization of an array field with a compound literal.

Pre-existing issues:
- [High] Missing driver `.remove` callback leads to unbalanced runtime PM usage counter and resource leak.
- [High] Dangling `phy_lookup` entry in the global list causes a Use-After-Free upon driver unbind.
- [High] Unconditional dereference of `dev->platform_data` before validating it for NULL.
--

commit 2a851e9d237369ac4340bdce02183defb68c9865
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

phy: phy-mtk-dp: Migrate register offsets to SoC specific pdata

This patch migrates the hardcoded register offsets in the MediaTek DP PHY driver
to SoC-specific platform data. It assigns this platform data for both DeviceTree
and platform probing. It also loops through lanes to clean up the writing of
driving parameters.

> diff --git a/drivers/phy/mediatek/phy-mtk-dp.c b/drivers/phy/mediatek/phy-mtk-dp.c
> index 98e05fe05ce3b..ce33f6812bae6 100644
> --- a/drivers/phy/mediatek/phy-mtk-dp.c
> +++ b/drivers/phy/mediatek/phy-mtk-dp.c
[ ... ]
> @@ -161,11 +225,18 @@ static const struct phy_ops mtk_dp_phy_dev_ops = {
>  	.owner = THIS_MODULE,
>  };
>  
> +static const struct mtk_dp_phy_pdata mt8195_dp_phy_data;
> +
>  static int mtk_dp_phy_legacy_probe(struct platform_device *pdev, struct mtk_dp_phy *dp_phy)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct phy *phy;
>  
> +	/*
> +	 * If legacy platform driver probe, assume this is MT8195 or compatible
> +	 * with a devicetree that was not migrated to the new, proper bindings.
> +	 */
> +	dp_phy->pdata = &mt8195_dp_phy_data;
>  	dp_phy->regmap = *(struct regmap **)dev->platform_data;

[Severity: High]
This is a pre-existing issue, but does this code unconditionally dereference
dev->platform_data before checking if it is NULL?

If the platform device is probed through the legacy path without an of_node and
without platform_data provided (which can happen by manually binding an
arbitrary compatible device via sysfs), could this lead to a kernel panic?

>  	if (!dp_phy->regmap)
>  		return dev_err_probe(dev, -EINVAL, "No platform data available\n");
[ ... ]
> @@ -214,6 +285,8 @@ static int mtk_dp_phy_probe(struct platform_device *pdev)
>  	if (IS_ERR(dp_phy->regmap))
>  		return PTR_ERR(dp_phy->regmap);
>  
> +	dp_phy->pdata = device_get_match_data(dev);
> +

[Severity: High]
Can device_get_match_data() return NULL here?

If a user with privileges to write to sysfs binds the device manually using
/sys/bus/platform/drivers/.../bind, or the device is instantiated via platform
name matching while having an of_node, device_get_match_data() will return
NULL.

Could this lead to a NULL pointer dereference resulting in a kernel panic when
dp_phy->pdata is accessed later?

>  	phy = devm_phy_create(dev, NULL, &mtk_dp_phy_dev_ops);
>  	if (IS_ERR(phy))
>  		return dev_err_probe(dev, PTR_ERR(phy),
[ ... ]
> @@ -231,8 +304,15 @@ static int mtk_dp_phy_probe(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static const struct mtk_dp_phy_pdata mt8195_dp_phy_data = {
> +	.off_dig_glb = 0x1000,
> +	.off_dig_lane = (const u16[]) { 0x1100, 0x1200, 0x1300, 0x1400 },

[Severity: Low]
Will this fail to compile under standard C compilers?

The off_dig_lane field is declared as an array
u16 off_dig_lane[MTK_DP_PHY_MAX_LANES], but it is being initialized using a
pointer from a compound literal cast (const u16[]) { ... }.

Assigning an explicitly cast compound literal to an array field is
syntactically invalid and could prevent the driver from building.

> +	.regs_dig_glb = mt8195_phy_dig_glb_regs,
> +	.regs_dig_lane = mt8195_phy_dig_lane_regs,
> +};

[Severity: High]
This is a pre-existing issue, but does the mtk_dp_phy_driver missing a .remove
callback lead to unbalanced runtime PM usage?

drivers/phy/mediatek/phy-mtk-dp.c:mtk_dp_phy_probe() {
    ...
    pm_runtime_enable(dev);
    pm_runtime_get_sync(dev);

    return 0;
}

drivers/phy/mediatek/phy-mtk-dp.c:mtk_dp_phy_driver {
    .probe = mtk_dp_phy_probe,
    .driver = {
        .name = "mediatek-dp-phy",
        .of_match_table = mtk_dp_phy_of_match,
    },
};

Without a .remove implementation to handle unbinding the driver, could the
device remain permanently powered on and fail to cleanly suspend or unbind?

[Severity: High]
This is also a pre-existing issue, but is the phy_lookup entry left dangling in
the global list when the driver is unbound?

drivers/phy/mediatek/phy-mtk-dp.c:mtk_dp_phy_legacy_probe() {
    ...
    phy_create_lookup(phy, "dp", dev_name(dev));
    ...
}

Because there is no .remove callback, phy_remove_lookup() is never called.

Could a subsequent phy_get() call traversing the list access the freed
pointer, resulting in a Use-After-Free since devres automatically frees the
struct phy?

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