Re: [PATCH 2/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 6 potential issue(s) to consider:

New issues:
- [High] Renaming the expected IRQ name from 'se1' to 'chrg_det' breaks compatibility with existing Device Tree binaries.
- [Medium] Early update of `ddata->mode` causes the state machine to become permanently stuck if the subsequent hardware configuration fails.
- [Medium] The system's SysRq functionality is permanently left disabled globally if a hardware configuration step fails in `cpcap_usb_set_idle_mode()`.
- [Medium] The module parameter `enable_uart` is ignored on boot, leaving the PHY in a partially uninitialized state.
- [Medium] A Time-of-Check to Time-of-Use (TOCTOU) race condition exists when reading the global `cpcap_enable_uart` module parameter.

Pre-existing issues:
- [Critical] A race condition during driver removal allows the devm-managed threaded IRQ handler to schedule `detect_work` after it has been canceled, leading to a Use-After-Free.
--

commit e8047562da705a40f07fe646c6694386f4dad156
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. When disabled, the PHY remains in its USB/charger detection
configuration while idle. This reduces power usage in idle by 25mW.
    
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 c6cc8fce6940f..3be2b3afaaf4d 100644
> --- a/drivers/phy/motorola/phy-cpcap-usb.c
> +++ b/drivers/phy/motorola/phy-cpcap-usb.c
> @@ -296,36 +309,53 @@ static void cpcap_usb_detect(struct work_struct *work)
>  		return;
>  	}
>  
>  	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) {
> +		if (!s.chrg_det || !s.sessvld || ddata->mode != CPCAP_IDLE)
> +			return;
> +
> +		if (s.se1)
> +			ddata->mode = CPCAP_CHARGER;
> +		else if (!s.dm && !s.dp)
> +			ddata->mode = CPCAP_USB;
> +		else
> +			return;

[Severity: Medium]
Can the state machine become permanently stuck if the subsequent hardware
configuration fails?

The state variable ddata->mode is eagerly updated here to CPCAP_CHARGER or
CPCAP_USB before applying the hardware state via cpcap_usb_set_usb_mode().
If the configuration fails and returns early, the state is never reverted.

On the next evaluation, the condition checking ddata->mode != CPCAP_IDLE
will evaluate to true and return immediately, preventing recovery until the
cable is physically unplugged.

> +
> +		dev_dbg(ddata->dev, "connected to %s\n",
> +			s.se1 ? "DCP" : "USB host");
> +
> +		error = regmap_update_bits(ddata->reg, CPCAP_REG_USBC3,
> +					   CPCAP_BIT_VBUSSTBY_EN |
> +					   CPCAP_BIT_VBUSEN_SPI, 0);
>  		if (error)
>  			goto out_err;
> -		cpcap_usb_try_musb_mailbox(ddata, MUSB_VBUS_VALID);
> +
> +		if (ddata->mode == CPCAP_CHARGER)
> +			error = cpcap_usb_set_dcp_mode(ddata);
> +		else
> +			error = cpcap_usb_set_usb_mode(ddata);
> +
> +		if (error)
> +			goto out_err;
> +
> +		if (ddata->mode == CPCAP_USB)
> +			cpcap_usb_try_musb_mailbox(ddata, MUSB_VBUS_VALID);
>  
>  		return;
>  	}
>  
> -	ddata->vbus_provider = false;
> -	ddata->docked = false;
> +	if (ddata->mode == CPCAP_IDLE)
> +		return;

[Severity: Medium]
Does this early return prevent the enable_uart module parameter from being
applied on boot?

During initial probe, ddata->mode is zero-initialized to CPCAP_IDLE. When
cpcap_usb_detect() runs for the first time, it exits early here and completely
skips cpcap_usb_set_idle_mode(). This leaves the PHY in a partially
uninitialized state until a cable is attached and removed.

> +
> +	ddata->mode = CPCAP_IDLE;
>  	cpcap_usb_try_musb_mailbox(ddata, MUSB_VBUS_OFF);
>  
> -	/* Default to debug UART mode */
> -	error = cpcap_usb_set_uart_mode(ddata);
> +	error = cpcap_usb_set_idle_mode(ddata);
>  	if (error)
>  		goto out_err;

[ ... ]

> @@ -374,7 +404,7 @@ static const char * const cpcap_phy_irqs[] = {
>  	"id_ground", "id_float",
>  
>  	/* REG_INT1 */
> -	"se0conn", "vbusvld", "sessvld", "sessend", "se1",
> +	"se0conn", "vbusvld", "sessvld", "sessend", "chrg_det",

[Severity: High]
Does this break compatibility with existing Device Tree binaries?

The driver now strictly requests the "chrg_det" interrupt by name, but older
DTBs only provide "se1". Without a fallback to check for the old name,
cpcap_usb_init_irq() will return -ENODEV and abort the probe.

[ ... ]

> @@ -426,30 +457,52 @@ static int cpcap_usb_set_uart_mode(struct cpcap_phy_ddata *ddata)
>  	if (error)
>  		goto out_err;
>  
> -	if (ddata->pins_uart) {
> -		error = pinctrl_select_state(ddata->pins, ddata->pins_uart);
> +	if (cpcap_enable_uart && ddata->pins_uart) {

[Severity: Medium]
Can a race condition occur when reading the global cpcap_enable_uart module
parameter?

Since it is modifiable via sysfs and evaluated multiple times independently
within cpcap_usb_set_idle_mode(), a concurrent write could change its value
mid-execution. This might result in a mismatched configuration, such as
setting UART pinctrl but then improperly applying SUSPEND_SPI later in the
function.

> +		error = pinctrl_select_state(ddata->pins,  ddata->pins_uart);
> +		if (error)
> +			goto out_err;
> +	} else if (!cpcap_enable_uart && ddata->pins_ulpi) {
> +		error = pinctrl_select_state(ddata->pins, ddata->pins_ulpi);
>  		if (error)
>  			goto out_err;
>  	}

[ ... ]

> @@ -457,6 +510,8 @@ static int cpcap_usb_set_uart_mode(struct cpcap_phy_ddata *ddata)
>  	if (sysrq & 1)
>  		sysrq_toggle_support(sysrq);
>  
> +	dev_dbg(ddata->dev, "set %s mode\n", cpcap_enable_uart ? "UART" : "IDLE");
> +
>  	return 0;
>  
>  out_err:
> +	dev_err(ddata->dev, "%s failed with %i\n", __func__, error);
> +
> +	return error;
> +}

[Severity: Medium]
Is it possible for the system's SysRq functionality to be permanently left
disabled on an error path?

In cpcap_usb_set_idle_mode(), sysrq is disabled to prevent line glitches.
If any subsequent hardware configuration step fails, the function jumps to the
out_err label here and returns without restoring the original sysrq state via
sysrq_toggle_support().

[ ... ]

> @@ -697,13 +848,9 @@ 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");
> -
> +	cpcap_usb_set_idle_mode(ddata);
>  	cpcap_usb_try_musb_mailbox(ddata, MUSB_VBUS_OFF);
>  
>  	usb_remove_phy(&ddata->phy);

[Severity: Critical]
This is a pre-existing issue, but does a race condition exist during driver
removal that allows the devm-managed threaded IRQ handler to schedule
detect_work after it has been canceled?

Because the IRQ is devm-managed, it is still active when
cancel_delayed_work_sync() is called at the end of cpcap_usb_phy_remove().

If the IRQ thread in cpcap_phy_irq_thread() evaluates ddata->active != 0
before it is cleared, but is preempted before calling schedule_delayed_work(),
the remove function can finish and cancel the work.

Devres will then wait for the preempted IRQ thread to finish. The thread will
resume and schedule the delayed work, and devres will then free ddata. When
the timer expires, the workqueue will execute cpcap_usb_detect() using the
freed ddata.

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