Re: [PATCH v3 3/4] phy: cpcap-usb: add DCP detection and make UART idle mode optional

[sashiko-bot@kernel.org]

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

New issues:
- [High] CPCAP_BIT_USBSUSPEND is left enabled when transitioning from idle mode to USB mode, permanently suspending the USB transceiver.

Pre-existing issues:
- [High] A Time-of-Check to Time-of-Use (TOCTOU) race condition between the threaded IRQ handler and `remove()` leads to a use-after-free of `ddata`.
--

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

phy: cpcap-usb: add DCP detection and make UART idle mode optional

Handle DCP separately from USB host connections based on CPCAP charger
detection status. Make the existing idle UART mode optional via the
"enable_uart" module parameter. Initialize the PHY into the baseline
configuration required for reliable charger detection during probe.

> diff --git a/drivers/phy/motorola/phy-cpcap-usb.c b/drivers/phy/motorola/phy-cpcap-usb.c
> index 7cb020dd3423c..78bb1773e5379 100644
> --- a/drivers/phy/motorola/phy-cpcap-usb.c
> +++ b/drivers/phy/motorola/phy-cpcap-usb.c

[ ... ]

> -	error = regmap_update_bits(ddata->reg, CPCAP_REG_USBC2,
> -				   0xffff, CPCAP_BIT_UARTMUX0 |
> -				   CPCAP_BIT_EMUMODE0);
> +	val = CPCAP_BIT_USBSUSPEND;
> +
> +	if (enable_uart)
> +		val |= (CPCAP_BIT_UARTMUX0 | CPCAP_BIT_EMUMODE0);
> +
> +	error = regmap_update_bits(ddata->reg, CPCAP_REG_USBC2, 0xffff, val);

[Severity: High]
Could this permanently suspend the USB transceiver? When unplugging a USB
cable, cpcap_usb_set_idle_mode() runs and sets CPCAP_BIT_USBSUSPEND.

However, when re-plugging the cable, cpcap_usb_set_usb_mode() is called,
which only masks CPCAP_BIT_USBXCVREN when updating CPCAP_REG_USBC2:

	error = regmap_update_bits(ddata->reg, CPCAP_REG_USBC2,
				   CPCAP_BIT_USBXCVREN,
				   CPCAP_BIT_USBXCVREN);

Since it doesn't clear the CPCAP_BIT_USBSUSPEND bit, will the transceiver
remain suspended and break subsequent data communication?

[ ... ]

> @@ -688,17 +845,13 @@ static int cpcap_usb_phy_probe(struct platform_device *pdev)
>  static void cpcap_usb_phy_remove(struct platform_device *pdev)
>  {
>  	struct cpcap_phy_ddata *ddata = platform_get_drvdata(pdev);
> -	int error;
>  
>  	atomic_set(&ddata->active, 0);
> -	error = cpcap_usb_set_uart_mode(ddata);
> -	if (error)
> -		dev_err(ddata->dev, "could not set UART mode\n");
> -
> +	cancel_delayed_work_sync(&ddata->detect_work);

[Severity: High]
This isn't a bug introduced by this patch, but does this sequence leave a
window for a use-after-free on ddata?

Because the IRQ is devm-managed, it remains active until the devres phase
after remove() returns. If cpcap_phy_irq_thread() is preempted right after
checking ddata->active but before scheduling the work:

	if (!atomic_read(&ddata->active))
		return IRQ_NONE;

	/* preempted here */
	schedule_delayed_work(&ddata->detect_work, msecs_to_jiffies(1));

And cpcap_usb_phy_remove() runs to completion, ddata will be freed. When
the IRQ thread resumes, it will queue the work again, causing the delayed
work to execute and access the freed ddata.

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