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

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

[Pengpeng Hou]

twl4030_usb_probe() publishes the USB PHY and may create the vbus sysfs
file before enabling runtime PM and requesting the IRQ.

If the IRQ request or PHY lookup fails, probe returns directly. That
leaves the USB PHY registered, can leave the vbus sysfs file published,
and leaves the runtime-PM get from probe unbalanced. The return value
from usb_add_phy_dev() is also ignored.

Check usb_add_phy_dev(), remember whether the vbus file was created, and
unwind the runtime-PM, sysfs and USB-PHY state on the late failure paths.

Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 drivers/phy/ti/phy-twl4030-usb.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/drivers/phy/ti/phy-twl4030-usb.c b/drivers/phy/ti/phy-twl4030-usb.c
index a26aec3ab29e..6a5968e6cfd8 100644
--- a/drivers/phy/ti/phy-twl4030-usb.c
+++ b/drivers/phy/ti/phy-twl4030-usb.c
@@ -162,6 +162,7 @@ struct twl4030_usb {
 	enum musb_vbus_id_status linkstat;
 	atomic_t		connected;
 	bool			vbus_supplied;
+	bool			vbus_file;
 	bool			musb_mailbox_pending;
 	unsigned long		runtime_suspended:1;
 	unsigned long		needs_resume:1;
@@ -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;
 
 	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);
 
@@ -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);
+	pm_runtime_put_noidle(twl->dev);
+	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)
@@ -795,7 +811,8 @@ static void twl4030_usb_remove(struct platform_device *pdev)
 	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);
 
 	/* set transceiver mode to power on defaults */
 	twl4030_usb_set_mode(twl, -1);
-- 
2.50.1 (Apple Git-155)


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

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

[sashiko-bot]

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