Re: [PATCH v3 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 4 potential issue(s) to consider:

New issues:
- [High] Register accesses are performed in `mtk_dp_phy_configure()` while the device's PM runtime state is suspended, risking system hangs.
- [Medium] Missing validation of `device_get_match_data()` return value can lead to a NULL pointer dereference.
- [Low] Invalid compound literal initialization for the 'off_dig_lane' array member causes a compilation error.

Pre-existing issues:
- [High] The legacy probe path unconditionally dereferences `dev->platform_data` before validating that it is not NULL.
--

--- Patch [4]: [PATCH v3 04/12] phy: phy-mtk-dp: Migrate register offsets to SoC specific pdata ---
commit c49d221e57bf6a9816b6ce4e691a20564bc66896
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

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

In preparation for adding support for newer SoCs and for adding
more capabilities to this driver in an efficient manner, migrate
all of the hardcoded register offsets to SoC specific pdata and
assign that for both DT and platform probing.

> diff --git a/drivers/phy/mediatek/phy-mtk-dp.c b/drivers/phy/mediatek/phy-mtk-dp.c
> index d241004e28286..20974650d9409 100644
> --- a/drivers/phy/mediatek/phy-mtk-dp.c
> +++ b/drivers/phy/mediatek/phy-mtk-dp.c
> @@ -111,9 +173,12 @@ static int mtk_dp_phy_init(struct phy *phy)
>  static int mtk_dp_phy_configure(struct phy *phy, union phy_configure_opts *opts)
>  {
>  	struct mtk_dp_phy *dp_phy = phy_get_drvdata(phy);
> +	const struct mtk_dp_phy_pdata *pdata = dp_phy->pdata;
>  	u32 val;
>  
>  	if (opts->dp.set_rate) {
> +		const u32 reg_bit_rate = pdata->regs_dig_glb[DP_PHYD_BIT_RATE];
> +

[ ... ]

> -		regmap_write(dp_phy->regmap, MTK_DP_PHY_DIG_BIT_RATE, val);
> +		regmap_write(dp_phy->regmap, pdata->off_dig_glb + reg_bit_rate, val);
>  	}
>  
> -	regmap_update_bits(dp_phy->regmap, MTK_DP_PHY_DIG_PLL_CTL_1,
> +	regmap_update_bits(dp_phy->regmap,
> +			   pdata->off_dig_glb + pdata->regs_dig_glb[DP_PHYD_PLL_CTL_1],
>  			   TPLL_SSC_EN, opts->dp.ssc ? TPLL_SSC_EN : 0);

[Severity: High]
Since phy_configure() is not wrapped by phy_pm_runtime_get_sync() in the
PHY core, and the consumer driver may not call phy_power_on() before this
point, the PHY PM runtime usage counter might remain 0 during
mtk_dp_phy_configure().

If the PHY power domain is turned off, will these unclocked regmap_write()
and regmap_update_bits() calls cause the system to hang?

Should there be PM runtime protection around these register accesses?

>  
>  	return 0;
>  }

[ ... ]

> @@ -170,12 +241,19 @@ static void mtk_dp_phy_legacy_remove_lookup(void *data)
>  	phy_remove_lookup(phy, "dp", dev_name(dp_phy->dev));
>  }
>  
> +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;
>  	int ret;
>  
> +	/*
> +	 * 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 dev->platform_data is unconditionally
dereferenced here before it is checked for NULL.

If this legacy probe path is triggered without platform_data provided (for
example, via the sysfs new_device interface), won't this cause an immediate
NULL pointer dereference?

>  	if (!dp_phy->regmap)
>  		return dev_err_probe(dev, -EINVAL, "No platform data available\n");

[ ... ]

> @@ -235,6 +313,8 @@ static int mtk_dp_phy_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	dp_phy->pdata = device_get_match_data(dev);
> +

[Severity: Medium]
If a user binds the mediatek-dp-phy driver via sysfs to a platform device
that doesn't match the driver's compatible strings, device_get_match_data()
will return NULL.

Does this need a NULL check before storing it in dp_phy->pdata?

Without validation, subsequent PHY operations like mtk_dp_phy_init() will
dereference the NULL pointer and crash.

>  	phy = devm_phy_create(dev, NULL, &mtk_dp_phy_dev_ops);
>  	if (IS_ERR(phy))
>  		return dev_err_probe(dev, PTR_ERR(phy),

[ ... ]

> @@ -249,8 +329,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]
Is this compound literal initialization valid for an array struct member?

Compiling this with GCC or Clang may throw an invalid initializer error
because C99 requires array members to be initialized directly with
brace-enclosed lists rather than compound literal expressions.

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

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

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy