Re: [PATCH phy-next 09/13] phy: lynx-28g: common probe() and remove()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing NULL check for the result of `of_device_get_match_data(dev)`.
--

commit 4307af42a82313eaf19153e90d7c48e0bd5f9626
Author: Vladimir Oltean <vladimir.oltean@nxp.com>

phy: lynx-28g: common probe() and remove()

Factor the device-agnostic logic from lynx_28g_probe() and
lynx_28g_remove() into lynx_probe() and lynx_remove() inside
phy-fsl-lynx-core.c. These will be shared with the 10G Lynx driver.

> diff --git a/drivers/phy/freescale/phy-fsl-lynx-28g.c b/drivers/phy/freescale/phy-fsl-lynx-28g.c
> --- a/drivers/phy/freescale/phy-fsl-lynx-28g.c
> +++ b/drivers/phy/freescale/phy-fsl-lynx-28g.c
[ ... ]
>  static int lynx_28g_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> +	const struct lynx_info *info;
>  
>  	/*
>  	 * If we get here it means we probed on a device tree where
>  	 * "fsl,lynx-28g" wasn't the fallback, but the sole compatible string.
>  	 */
> +	info = of_device_get_match_data(dev);

[Severity: High]
This is a pre-existing issue, but should we check if info is NULL before
proceeding?

If a user binds the driver to a device using the sysfs bind interface, and
the device's OF node does not match the driver's of_match_table,
of_device_get_match_data() will return NULL.

> +	if (info == &lynx_info_compat)
>  		dev_warn(dev, "Please update device tree to use per-device compatible strings\n");
>  
> +	return lynx_probe(pdev, info, &lynx_28g_ops);
>  }

[ ... ]
> diff --git a/drivers/phy/freescale/phy-fsl-lynx-core.c b/drivers/phy/freescale/phy-fsl-lynx-core.c
> --- a/drivers/phy/freescale/phy-fsl-lynx-core.c
> +++ b/drivers/phy/freescale/phy-fsl-lynx-core.c
[ ... ]
> +int lynx_probe(struct platform_device *pdev, const struct lynx_info *info,
> +	       const struct phy_ops *phy_ops)
> +{
[ ... ]
> +	priv->dev = dev;
> +	priv->info = info;
> +	dev_set_drvdata(dev, priv);
> +	spin_lock_init(&priv->pcc_lock);
> +	INIT_DELAYED_WORK(&priv->cdr_check, lynx_cdr_lock_check);
> +
> +	priv->lane = devm_kcalloc(dev, priv->info->num_lanes,

[Severity: High]
This isn't a bug introduced by this patch, but if info was NULL as mentioned
above, would this unconditionally dereference it and cause a kernel panic?

> +				  sizeof(*priv->lane), GFP_KERNEL);
> +	if (!priv->lane)
> +		return -ENOMEM;
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260528172404.733196-1-vladimir.oltean@nxp.com?part=9

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