Re: [PATCH] phy: ti: twl4030-usb: unwind late probe failures

[sashiko-bot@kernel.org]

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

New issues:
- [High] The newly added `err_runtime` cleanup path uses `pm_runtime_put_noidle()` before `pm_runtime_disable()`, violating PM guidelines and leaving hardware regulators permanently enabled.

Pre-existing issues:
- [High] The delayed work `id_workaround_work` is not cancelled in the probe error path, leading to a potential use-after-free if the work executes after the driver state is freed.
- [High] Re-initializing the PHY notifier head after the PHY has been published to the system drops concurrent registrations and corrupts the notifier list.
- [High] Registering the USB PHY early in probe exposes it to concurrent lookups, leading to a use-after-free if probe subsequently fails.
- [High] The PHY lookup created by `phy_create_lookup()` is never removed during driver unbind, leading to a memory leak and a dangling pointer in the global PHY list.
--

commit 6c9da7a5a1fe8af7dbe4915303ad564e9cf8b693
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

phy: ti: twl4030-usb: unwind late probe failures

This commit addresses missing unwinding in the late probe failure paths of
the twl4030-usb driver. It remembers if the vbus sysfs file was created and
adds an error path to properly undo PM, sysfs, and PHY registrations.

> --- a/drivers/phy/ti/phy-twl4030-usb.c
> +++ b/drivers/phy/ti/phy-twl4030-usb.c
[ ... ]
> @@ -746,11 +747,15 @@ static int twl4030_usb_probe(struct platform_device *pdev)
>  		dev_err(&pdev->dev, "ldo init failed\n");
>  		return err;
>  	}
> -	usb_add_phy_dev(&twl->phy);
> +	err = usb_add_phy_dev(&twl->phy);
> +	if (err)
> +		return err;

[Severity: High]
This is a pre-existing issue, but does registering the USB PHY this early in
probe expose it to concurrent lookups?

If a later probe step fails, we call usb_remove_phy() and free the twl
structure via devres. If a concurrent thread called usb_get_phy() during the
race window, could it lead to a use-after-free?

>  	platform_set_drvdata(pdev, twl);
>  	if (device_create_file(&pdev->dev, &dev_attr_vbus))
>  		dev_warn(&pdev->dev, "could not create sysfs file\n");
> +	else
> +		twl->vbus_file = true;
>  
>  	ATOMIC_INIT_NOTIFIER_HEAD(&twl->phy.notifier);

[Severity: High]
This is a pre-existing issue, but does re-initializing the notifier head
after usb_add_phy_dev() corrupt the notifier list?

Since usb_add_phy_dev() internally initializes this exact notifier head and
exposes the PHY globally, could this drop concurrent registrations made by
consumer drivers?

[ ... ]
> @@ -773,18 +778,29 @@ static int twl4030_usb_probe(struct platform_device *pdev)
>  	if (status < 0) {
>  		dev_dbg(&pdev->dev, "can't get IRQ %d, err %d\n",
>  			twl->irq, status);
> -		return status;
> +		err = status;
> +		goto err_runtime;
>  	}
>  
>  	if (pdata)
>  		err = phy_create_lookup(phy, "usb", "musb-hdrc.0");
>  	if (err)
> -		return err;
> +		goto err_runtime;
>  
>  	pm_runtime_mark_last_busy(&pdev->dev);
>  	pm_runtime_put_autosuspend(twl->dev);
>  
>  	return 0;
> +
> +err_runtime:
> +	pm_runtime_dont_use_autosuspend(&pdev->dev);

[Severity: High]
This is a pre-existing issue, but is it possible for the scheduled delayed
work twl->id_workaround_work to run after the driver state is freed?

Since devm_request_threaded_irq() can schedule the work before we jump to
err_runtime, shouldn't we call cancel_delayed_work_sync() here to prevent a
potential use-after-free?

> +	pm_runtime_put_noidle(twl->dev);

[Severity: High]
Does using pm_runtime_put_noidle() here bypass the suspend cycle?

Since pm_runtime_get_sync() resumed the device and enabled the regulators
earlier, and pm_runtime_disable() prevents future state transitions, it seems
the device will remain in RPM_ACTIVE. Should this use pm_runtime_put_sync()
to ensure the hardware is properly suspended and regulators disabled?

> +	pm_runtime_disable(&pdev->dev);
> +	if (twl->vbus_file)
> +		device_remove_file(twl->dev, &dev_attr_vbus);
> +	usb_remove_phy(&twl->phy);
> +
> +	return err;
>  }
>  
>  static void twl4030_usb_remove(struct platform_device *pdev)
>  {
>  	struct twl4030_usb *twl = platform_get_drvdata(pdev);
>  	int val;

[Severity: High]
This is a pre-existing issue, but does this function leak the PHY lookup
created during probe?

twl4030_usb_probe() calls phy_create_lookup(), which allocates a lookup
object. Since there is no corresponding phy_remove_lookup() here, could this
leave a dangling pointer in the global PHY list when devres frees the phy
object?

>  	usb_remove_phy(&twl->phy);
>  	pm_runtime_get_sync(twl->dev);
>  	cancel_delayed_work_sync(&twl->id_workaround_work);
> -	device_remove_file(twl->dev, &dev_attr_vbus);
> +	if (twl->vbus_file)
> +		device_remove_file(twl->dev, &dev_attr_vbus);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260615070211.59144-1-pengpeng@iscas.ac.cn?part=1

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