Re: [PATCH v2 4/5] 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] The USB PHY state machine gets permanently stuck in CPCAP_UNKNOWN if a USB host or charger is connected during boot.

Pre-existing issues:
- [High] A Time-Of-Check to Time-Of-Use (TOCTOU) race condition allows a delayed work timer to be queued just before the driver's data structure is freed.
--

commit c15ba246412946a134691c79cf0b6023882d635b
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 and initialize
the PHY into the baseline configuration. Fix a race in removal.

> diff --git a/drivers/phy/motorola/phy-cpcap-usb.c b/drivers/phy/motorola/phy-cpcap-usb.c
> index d1c70c7153466..e776e54bbfd56 100644
> --- a/drivers/phy/motorola/phy-cpcap-usb.c
> +++ b/drivers/phy/motorola/phy-cpcap-usb.c
[ ... ]
> @@ -293,38 +302,61 @@ static void cpcap_usb_detect(struct work_struct *work)
[ ... ]
>  	vbus = cpcap_usb_vbus_valid(ddata);
>  
> -	/* Otherwise assume we're connected to a USB host */
> -	if (vbus) {
> -		dev_dbg(ddata->dev, "connected to USB host\n");
> -		error = cpcap_usb_set_usb_mode(ddata);
> +	/* Check if we're connected to a USB host */
> +	if (vbus && ddata->mode != CPCAP_USB_HOST) {
> +		enum cpcap_mode new_mode;
> +
> +		if (!s.chrg_det || !s.sessvld || ddata->mode != CPCAP_IDLE)
> +			return;

[Severity: High]
Does this early return prevent the PHY from ever initializing if a cable
is connected during boot?

During cpcap_usb_phy_probe(), ddata->mode is initialized to CPCAP_UNKNOWN.
If a charger or host is already connected, vbus is true, and this block is
entered in cpcap_usb_detect(). Because ddata->mode is CPCAP_UNKNOWN, the
condition ddata->mode != CPCAP_IDLE is unconditionally true, which causes
an immediate return without transitioning the state.

[ ... ]
> @@ -694,17 +853,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 is a pre-existing issue, but does moving cancel_delayed_work_sync()
here still leave a window for a use-after-free if the IRQ thread is
preempted?

Looking at cpcap_phy_irq_thread():

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

    schedule_delayed_work(&ddata->detect_work, msecs_to_jiffies(1));

If the IRQ handler checks ddata->active and sees 1, but is then preempted
before scheduling the work, cpcap_usb_phy_remove() can run, set active to 0,
and cancel the currently empty workqueue.

When the IRQ handler resumes, it queues the work, which then fires after
devres has freed ddata.

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