Re: [PATCH v4 2/6] dt-bindings: phy: motorola,cpcap-usb-phy: add optional safe pinctrl state

Re: [PATCH v4 2/6] dt-bindings: phy: motorola,cpcap-usb-phy: add optional safe pinctrl state

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The commit message claims to document the 'safe' pinctrl state used by the CPCAP USB PHY driver, but the driver itself never references or uses this state.
--

--- Patch [2]: [PATCH v4 2/6] dt-bindings: phy: motorola,cpcap-usb-phy: add optional safe pinctrl state ---
commit a9d2c62cbbf0fb6ec2341d2c0459bff90670c671
Author: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>

dt-bindings: phy: motorola,cpcap-usb-phy: add optional safe pinctrl state

This commit adds a new safe pinctrl state to the CPCAP USB PHY device tree
bindings.

> Document the optional "safe" pinctrl state used by the CPCAP USB PHY
> driver.

[Severity: Medium]
Is this commit message accurate regarding the driver actually using this state?

Looking at the driver implementation in
drivers/phy/motorola/phy-cpcap-usb.c:cpcap_usb_init_optional_pins(), it
appears to only request the ulpi, utmi, and uart states:

	ddata->pins_ulpi = pinctrl_lookup_state(ddata->pins, "ulpi");
	if (IS_ERR(ddata->pins_ulpi))
	...
	ddata->pins_utmi = pinctrl_lookup_state(ddata->pins, "utmi");
	...
	ddata->pins_uart = pinctrl_lookup_state(ddata->pins, "uart");

The safe state does not appear to be requested or used anywhere in the driver
code.

Does the driver still need to be updated to support this new state, or could
the commit message be clarified to reflect that this is laying the groundwork
for future driver support?

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

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

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

[sashiko-bot]

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

New issues:
- [High] `cpcap_usb_set_idle_mode()` unconditionally sets `CPCAP_BIT_DMPD_SPI` and `CPCAP_BIT_DPPD_SPI`, overriding hardware pulldowns and breaking subsequent charger detection.
- [Medium] The `cpcap_usb_set_dcp_mode` function fails to fully configure the PHY registers, leaving them in a non-deterministic state.

Pre-existing issues:
- [High] A TOCTOU race condition exists between the IRQ handler thread and `cpcap_usb_phy_remove`, leading to a use-after-free of the `ddata` structure.
--

--- Patch [3]: [PATCH v4 3/6] phy: cpcap-usb: add DCP detection and make UART idle mode optional ---
commit ccdf9470fbb5fbd414c5546a329d78f69ecf50b7
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 using CPCAP charger
detection status. Make the existing idle UART mode optional via the
enable_uart module parameter. Also 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..d4eb8086b6000 100644
> --- a/drivers/phy/motorola/phy-cpcap-usb.c
> +++ b/drivers/phy/motorola/phy-cpcap-usb.c
[ ... ]
> @@ -411,43 +446,84 @@ static int cpcap_usb_gpio_set_mode(struct cpcap_phy_ddata *ddata,
[ ... ]
> +static int cpcap_usb_set_idle_mode(struct cpcap_phy_ddata *ddata)
> +{
> +	int error;
> +	unsigned int val;
> +	bool enable_uart = cpcap_enable_uart;
> +
> +	error = cpcap_usb_set_safe_mode(ddata);
> +	if (error)
> +		return error;
> +
>  	error = regmap_update_bits(ddata->reg, CPCAP_REG_USBC1,
> +				   CPCAP_BIT_DP150KPU |
> +				   CPCAP_BIT_DP1K5PU |
> +				   CPCAP_BIT_DM1K5PU |
> +				   CPCAP_BIT_DPPD |
> +				   CPCAP_BIT_DMPD |
>  				   CPCAP_BIT_VBUSPD,
> +				   CPCAP_BIT_DP150KPU |
>  				   CPCAP_BIT_VBUSPD);
>  	if (error)
>  		goto out_err;
>  
> -	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);
>  	if (error)
>  		goto out_err;
>  
> -	error = regmap_update_bits(ddata->reg, CPCAP_REG_USBC3, 0x7fff,
> -				   CPCAP_BIT_IDPU_SPI);
> +	val = CPCAP_BIT_VBUSSTBY_EN |
> +	      CPCAP_BIT_VBUSPD_SPI |
> +	      CPCAP_BIT_DMPD_SPI |
> +	      CPCAP_BIT_DPPD_SPI |
> +	      CPCAP_BIT_PU_SPI |
> +	      CPCAP_BIT_IDPU_SPI |
> +	      CPCAP_BIT_ULPI_SPI_SEL;
> +
> +	if (!enable_uart)
> +		val |= CPCAP_BIT_SUSPEND_SPI;
> +
> +	error = regmap_update_bits(ddata->reg, CPCAP_REG_USBC3, 0x7fff, val);

[Severity: High]
Does setting CPCAP_BIT_DMPD_SPI and CPCAP_BIT_DPPD_SPI here break charger
detection?

Writing this value to USBC3 unconditionally sets both DMPD_SPI and DPPD_SPI to
1. This overrides the hardware pulldowns and forces them off. The original
code correctly cleared these bits, and as noted in cpcap_usb_init_usb(),
disabling these pulldowns can break subsequent charger detection when the
cable is disconnected.

>  	if (error)
>  		goto out_err;
>  
[ ... ]
> @@ -508,6 +583,31 @@ static int cpcap_usb_set_usb_mode(struct cpcap_phy_ddata *ddata)
>  	return error;
>  }
>  
> +static int cpcap_usb_set_dcp_mode(struct cpcap_phy_ddata *ddata)
> +{
> +	int error;
> +
> +	error = cpcap_usb_set_safe_mode(ddata);
> +	if (error)
> +		return error;
> +
> +	error = regmap_update_bits(ddata->reg, CPCAP_REG_USBC3,
> +				   CPCAP_BIT_SUSPEND_SPI, 0);
> +	if (error)
> +		goto out_err;

[Severity: Medium]
Does cpcap_usb_set_dcp_mode() need to configure USBC1 and USBC2 as well?

Because this only updates USBC3, it seems to inherit whatever state was left
by the previous mode. For example, if transitioning from USB mode, USBXCVREN
in USBC2 might remain set. If transitioning from IDLE mode with
enable_uart=1, UARTMUX0 might remain set, leaving the UART connected to the
data lines during charging.

> +
> +	error = cpcap_usb_gpio_set_mode(ddata, CPCAP_DM_DP);
> +	if (error)
> +		goto out_err;
> +
> +	return 0;
> +
> +out_err:
> +	dev_err(ddata->dev, "%s failed with %i\n", __func__, error);
> +
> +	return error;
> +}
> +
[ ... ]
> @@ -688,17 +849,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);
> +	cpcap_usb_set_idle_mode(ddata);
>  	cpcap_usb_try_musb_mailbox(ddata, MUSB_VBUS_OFF);
>  
>  	usb_remove_phy(&ddata->phy);
> -	cancel_delayed_work_sync(&ddata->detect_work);
>  	regulator_disable(ddata->vusb);
>  }

[Severity: High]
This is a pre-existing issue, but is there a TOCTOU race condition between
cpcap_phy_irq_thread() and cpcap_usb_phy_remove() that can result in a
use-after-free?

If the IRQ thread reads ddata->active as 1 in cpcap_phy_irq_thread() and gets
preempted:

cpcap_phy_irq_thread() {
    if (!atomic_read(&ddata->active))
        return IRQ_NONE;
    /* preempted here */

And cpcap_usb_phy_remove() completes, setting ddata->active = 0, canceling the
work, and allowing devres to free ddata:

cpcap_usb_phy_remove() {
    atomic_set(&ddata->active, 0);
    cancel_delayed_work_sync(&ddata->detect_work);
    ...
}
/* devres frees ddata */

Could the IRQ thread then resume and call schedule_delayed_work() on the freed
ddata?

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

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

Re: [PATCH v4 4/6] phy: cpcap-usb: add extcon support

[sashiko-bot]

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

New issues:
- [Medium] The ordering of extcon state updates exposes invalid intermediate cable states to userspace.

Pre-existing issues:
- [High] Pre-existing UAF: Delayed work can be re-armed after cancel_delayed_work_sync() during driver removal due to an IRQ thread race.
--

--- Patch [4]: [PATCH v4 4/6] phy: cpcap-usb: add extcon support ---
commit 5fa6aab542d92a2c79fbe5195a861859976507b6
Author: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>

phy: cpcap-usb: add extcon support

This commit registers an Extcon device and reports the detected USB cable
state. It exports this state through the Extcon framework so other drivers
can consume it using standard kernel interfaces.

> diff --git a/drivers/phy/motorola/phy-cpcap-usb.c b/drivers/phy/motorola/phy-cpcap-usb.c
> index d4eb8086b6000..8f9bea1d8a212 100644
> --- a/drivers/phy/motorola/phy-cpcap-usb.c
> +++ b/drivers/phy/motorola/phy-cpcap-usb.c
[ ... ]
> @@ -226,6 +239,46 @@ static void cpcap_usb_try_musb_mailbox(struct cpcap_phy_ddata *ddata,
>  		__func__, error);
>  }
>  
> +static void cpcap_usb_set_extcon(struct cpcap_phy_ddata *ddata)
> +{
> +	int cable;
> +
> +	switch (ddata->mode) {
[ ... ]
> +	if (cable == ddata->cable)
> +		return;
> +
> +	if (ddata->cable == EXTCON_CHG_USB_SDP)
> +		extcon_set_state_sync(ddata->edev, EXTCON_USB, false);
> +	else if (cable == EXTCON_CHG_USB_SDP)
> +		extcon_set_state_sync(ddata->edev, EXTCON_USB, true);
> +
> +	if (ddata->cable != EXTCON_NONE)
> +		extcon_set_state_sync(ddata->edev, ddata->cable, false);

[Severity: Medium]
Could this ordering expose invalid intermediate cable states to userspace?

When transitioning to a new cable state (specifically EXTCON_CHG_USB_SDP),
the new state's associated EXTCON_USB state is enabled before the old
ddata->cable state is cleared.

Because extcon_set_state_sync() immediately notifies consumers and emits a
uevent, consumers might briefly observe overlapping, mutually exclusive
states (such as both DCP and SDP active simultaneously).

Would it be safer to completely clear the old states before asserting any
new states?

> +
> +	if (cable != EXTCON_NONE)
> +		extcon_set_state_sync(ddata->edev, cable, true);
> +
> +	ddata->cable = cable;
> +}
[ ... ]


[Severity: High]
This is a pre-existing issue, but I noticed a potential use-after-free race
involving the delayed work during driver unbind. Does this execution sequence
leave the work queued after removal?

If the IRQ thread checks ddata->active and gets preempted before calling
schedule_delayed_work():

cpcap_phy_irq_thread() {
    ...
    if (atomic_read(&ddata->active))
        ... <preempted here> ...

cpcap_usb_phy_remove() can concurrently set active to 0 and call
cancel_delayed_work_sync(). Since the work isn't queued yet, the
cancellation completes immediately:

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

During devres cleanup, free_irq() waits for the IRQ thread to finish.
The thread then resumes, unconditionally calls schedule_delayed_work(),
and exits:

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

After free_irq() completes, devres frees ddata. The delayed work will
subsequently execute and access the freed memory.

Could this be prevented by disabling IRQs prior to cancelling the work?

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

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