Re: [PATCH v1 6/6] phy: tegra: Add support for Nvidia Tegra XMM6260 PHY

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Incorrect error checking of `of_parse_phandle()` leads to arbitrary device matching and memory corruption.
- [Critical] Type confusion and NULL pointer dereference due to illegal access of an external device's driver data.
- [High] Permanent memory and reference count leak of the USB platform device.
- [Medium] Improper usage of `dev_err_probe()` inside runtime execution paths.
- [Medium] Missing Kconfig dependency on `CONFIG_USB_PHY`.
--

commit c599fd7b5a962e5696a6eb64dff41cd0427d8dde
Author: Svyatoslav Ryhel <clamor95@gmail.com>

phy: tegra: Add support for Nvidia Tegra XMM6260 PHY

Nvidia Tegra XMM6260 PHY is a hardware configuration used in Tegra SoCs
to provide proper interaction between the application processor and the
modem, as well as control over one of the SoC's USB lines for the modem.

> diff --git a/drivers/phy/tegra/Kconfig b/drivers/phy/tegra/Kconfig
> index 342fb736da4b1..41b5ce460f37c 100644
> --- a/drivers/phy/tegra/Kconfig
> +++ b/drivers/phy/tegra/Kconfig
> @@ -18,3 +18,15 @@ config PHY_TEGRA194_P2U
>  	help
>  	  Enable this to support the P2U (PIPE to UPHY) that is part of Tegra 19x
>  	  and 234 SOCs.
> +
> +config PHY_TEGRA_XMM6260
> +	tristate "NVIDIA Tegra XMM6260 PHY driver"
> +	depends on ARCH_TEGRA && USB_NET_XMM6260 && USB_SUPPORT
> +	select GENERIC_PHY

This driver relies on USB PHY APIs like devm_usb_get_phy_by_phandle(), but
doesn't seem to depend on USB_PHY. Could this cause silent probe failures if
CONFIG_USB_PHY is disabled and the stub functions are used?

[ ... ]

> diff --git a/drivers/phy/tegra/phy-tegra-xmm6260.c b/drivers/phy/tegra/phy-tegra-xmm6260.c
> new file mode 100644
> index 0000000000000..7511de1333aaf
> --- /dev/null
> +++ b/drivers/phy/tegra/phy-tegra-xmm6260.c
[ ... ]
> +struct tegra_usb_device {
> +	struct ci_hdrc_platform_data data;
> +	struct platform_device *dev;
> +};
[ ... ]
> +static int tegra_xmm6260_phy_power_on(struct phy *phy)
> +{
> +	struct tegra_xmm6260_phy *mphy = phy_get_drvdata(phy);
> +	struct tegra_usb_device *usb = platform_get_drvdata(mphy->usb_dev);

Does this rely on the exact memory layout of an external driver's private
structure? The tegra_usb_device struct is defined locally here.

Also, if the external driver hasn't probed yet, platform_get_drvdata()
returns NULL, which could lead to a NULL pointer dereference later when
accessing usb->dev.

> +	int ret;
> +
> +	gpiod_set_value_cansleep(mphy->enable_gpio, 1);
> +
> +	ret = usb_phy_init(mphy->usb_phy);
> +	if (ret) {
> +		gpiod_set_value_cansleep(mphy->enable_gpio, 0);
> +		return dev_err_probe(mphy->dev, ret,
> +				     "failed to init USB PHY\n");
> +	}

Is it safe to call dev_err_probe() outside of a probe function? This is a
runtime PHY operation callback, and using dev_err_probe() here might pollute
the deferred probe lists if the error code happens to match -EPROBE_DEFER.

[ ... ]
> +static int tegra_xmm6260_phy_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	usb_node = of_parse_phandle(dev->of_node, "nvidia,usb-bus", 0);
> +	if (IS_ERR(usb_node))
> +		return dev_err_probe(dev, PTR_ERR(usb_node),
> +				     "failed to parse modem USB bus\n");

Should this check be if (!usb_node) instead? of_parse_phandle() returns NULL
when a property is missing, not an ERR_PTR.

If usb_node is NULL, this check is bypassed and of_find_device_by_node(NULL)
will be called below, which matches the very first platform device in the
system lacking a device tree node.

> +
> +	mphy->usb_dev = of_find_device_by_node(usb_node);
> +	of_node_put(usb_node);
> +	if (!mphy->usb_dev)
> +		return dev_err_probe(dev, -ENODEV,
> +				     "failed to get modem USB bus\n");

Does this leak the reference to the platform_device returned by
of_find_device_by_node()? It increments the reference count via
get_device(), but there doesn't seem to be a corresponding put_device() in
the error paths below or in a remove callback.

> +
> +	mphy->usb_phy = devm_usb_get_phy_by_phandle(dev, "nvidia,usb-bus", 1);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511135703.62470-1-clamor95@gmail.com?part=6

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