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

[Manivannan Sadhasivam]

On Mon, Jun 15, 2026 at 07:13:29AM +0000, sashiko-bot at kernel.org wrote:
> 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 at 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?
> 

This is a valid point. pm_runtime_put_noidle() will not invoke runtime_suspend()
callback and hence the PHY will be in active state.

- Mani

-- 
மணிவண்ணன் சதாசிவம்