Re: [PATCH v1 2/6] usb: chipidea: tegra: Avoid controller/PHY init if bus is externally controlled

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unbinding the `tegra_usb` driver when `nvidia,external-control` is present causes a NULL pointer dereference.
- [Medium] Unconditional call to `usb_phy_shutdown()` in `tegra_usb_remove()` on an uninitialized PHY triggers a kernel warning.
--

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

usb: chipidea: tegra: Avoid controller/PHY init if bus is externally controlled

If the USB controller and PHY are externally controlled, the registration
of the controller and the PHY initialization are skipped. This delegates
configuration to the device that controls the bus.

> diff --git a/drivers/usb/chipidea/ci_hdrc_tegra.c b/drivers/usb/chipidea/ci_hdrc_tegra.c
> index 372788f0f9701..593390a818d12 100644
> --- a/drivers/usb/chipidea/ci_hdrc_tegra.c
> +++ b/drivers/usb/chipidea/ci_hdrc_tegra.c

[ ... ]

> @@ -342,12 +348,14 @@ static int tegra_usb_probe(struct platform_device *pdev)
>  	if (of_usb_get_phy_mode(pdev->dev.of_node) == USBPHY_INTERFACE_MODE_ULPI)
>  		usb->data.flags &= ~CI_HDRC_SUPPORTS_RUNTIME_PM;
>  
> -	usb->dev = ci_hdrc_add_device(&pdev->dev, pdev->resource,
> -				      pdev->num_resources, &usb->data);
> -	if (IS_ERR(usb->dev)) {
> -		err = dev_err_probe(&pdev->dev, PTR_ERR(usb->dev),
> -				    "failed to add HDRC device");
> -		goto phy_shutdown;
> +	if (!usb->externally_controlled) {
> +		usb->dev = ci_hdrc_add_device(&pdev->dev, pdev->resource,
> +					      pdev->num_resources, &usb->data);
> +		if (IS_ERR(usb->dev)) {
> +			err = dev_err_probe(&pdev->dev, PTR_ERR(usb->dev),
> +					    "failed to add HDRC device");
> +			goto phy_shutdown;
> +		}
>  	}
>  
>  	return 0;

When nvidia,external-control is present, usb->dev is left uninitialized
and remains NULL. Does the unbind path still try to use it?

Looking at tegra_usb_remove(), it unconditionally calls
ci_hdrc_remove_device() with usb->dev:

    static void tegra_usb_remove(struct platform_device *pdev)
    {
        struct tegra_usb *usb = platform_get_drvdata(pdev);

        ci_hdrc_remove_device(usb->dev);
        ...

This looks like it could lead to a NULL pointer dereference, as
ci_hdrc_remove_device() dereferences the pointer immediately without checking:

    void ci_hdrc_remove_device(struct platform_device *pdev)
    {
        int id = pdev->id;
        ...

Could this regression cause a crash on device unbind or module removal
when the controller is externally controlled?

Additionally, when the PHY initialization is skipped during probe, does
tegra_usb_remove() still attempt to shut it down?

    static void tegra_usb_remove(struct platform_device *pdev)
    {
        ...
        usb_phy_shutdown(usb->phy);
        ...

Since the PHY wasn't initialized, its frequency will be 0. Calling
tegra_usb_phy_shutdown() on this uninitialized PHY appears to trigger
a warning regression:

    static void tegra_usb_phy_shutdown(struct usb_phy *u_phy)
    {
        struct tegra_usb_phy *phy = to_tegra_usb_phy(u_phy);

        if (WARN_ON(!phy->freq))
            return;
        ...

Should tegra_usb_remove() be updated to check if it is externally
controlled before calling the shutdown and remove functions?

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

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