Re: [PATCH v2 5/5] phy: cpcap-usb: add extcon support

[sashiko-bot@kernel.org]

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

New issues:
- [High] Missing Kconfig dependency on CONFIG_EXTCON causes linker errors or breaks USB probe entirely if Extcon is disabled or built as a module.

Pre-existing issues:
- [High] Race condition between module removal and the threaded IRQ handler can lead to a use-after-free of `ddata`.
- [High] Booting with a USB cable or charger already attached prevents the driver from detecting the connection.
--

commit 85bc183819335614278666a8396810aa855aa0d7
Author: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>

phy: cpcap-usb: add extcon support
    
Register an Extcon device and report the detected cable state.
The driver already determines the type of cable attached during USB cable
detection. Export the detected state through the Extcon framework so
other drivers can consume it using a standard kernel interface.

> diff --git a/drivers/phy/motorola/phy-cpcap-usb.c b/drivers/phy/motorola/phy-cpcap-usb.c
> index e776e54bbfd56..f1f2fe8183241 100644
> --- a/drivers/phy/motorola/phy-cpcap-usb.c
> +++ b/drivers/phy/motorola/phy-cpcap-usb.c
[ ... ]
> @@ -345,6 +400,8 @@ static void cpcap_usb_detect(struct work_struct *work)
>  		if (ddata->mode == CPCAP_USB)
>  			cpcap_usb_try_musb_mailbox(ddata, MUSB_VBUS_VALID);
>  
> +		cpcap_usb_set_extcon(ddata);
> +
>  		return;
>  	}

[Severity: High]
This is a pre-existing issue, but does the early return logic just above this
section prevent the driver from detecting a cable connected at boot time?

Looking earlier in cpcap_usb_detect():

	if (vbus && ddata->mode != CPCAP_USB_HOST) {
		enum cpcap_mode new_mode;

		if (!s.chrg_det || !s.sessvld || ddata->mode != CPCAP_IDLE)
			return;

At probe, ddata->mode is initialized to CPCAP_UNKNOWN. If a cable is already
attached when the device boots, vbus is valid. The condition checks if
ddata->mode != CPCAP_IDLE, which evaluates to true because the mode is set to
CPCAP_UNKNOWN. 

This causes an early return without updating the mode or rescheduling the
work, which seems to leave the USB connection undetected.

[ ... ]
> @@ -785,7 +843,18 @@ static int cpcap_usb_phy_probe(struct platform_device *pdev)
>  	if (!otg)
>  		return -ENOMEM;
>  
> +	ddata->edev = devm_extcon_dev_allocate(&pdev->dev, cpcap_extcon_cables);
> +	if (IS_ERR(ddata->edev)) {
> +		return dev_err_probe(&pdev->dev, PTR_ERR(ddata->edev),
> +				     "failed to allocate extcon device\n");
> +	}
> +
> +	error = devm_extcon_dev_register(&pdev->dev, ddata->edev);
> +	if (error < 0)
> +		return error;
> +

[Severity: High]
Does PHY_CPCAP_USB need a Kconfig dependency on CONFIG_EXTCON?

If the driver is built-in but the extcon subsystem is configured as a
module, won't this cause a build failure due to undefined references to
devm_extcon_dev_allocate() and devm_extcon_dev_register()?

Additionally, if the extcon subsystem is completely disabled, the stub
function for devm_extcon_dev_allocate() returns ERR_PTR(-ENOSYS). This
would cause cpcap_usb_phy_probe() to unconditionally fail.

[ ... ]

[Severity: High]
This is a pre-existing issue, but is there a race condition during module
removal that could lead to a use-after-free of the ddata structure?

During driver removal, cpcap_usb_phy_remove() sets ddata->active to 0 and
cancels the delayed work:

	atomic_set(&ddata->active, 0);
	cancel_delayed_work_sync(&ddata->detect_work);

However, the IRQ is managed by devm_request_threaded_irq() and remains
active until after the remove function completes. If the threaded IRQ handler
cpcap_phy_irq_thread() reads ddata->active == 1 but is preempted before
calling schedule_delayed_work():

	if (!atomic_read(&ddata->active))
		return IRQ_NONE;
	
	schedule_delayed_work(&ddata->detect_work, msecs_to_jiffies(1));

The remove function could finish, causing devm to free the ddata structure.
When the IRQ thread resumes, it would schedule work using the freed ddata
pointer. Could this be resolved by explicitly disabling the IRQ before
canceling the work?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260705075809.1793784-1-ivo.g.dimitrov.75@gmail.com?part=5