Re: [PATCH 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] The EXTCON_USB state is updated using `extcon_set_state` without a corresponding `extcon_set_state_sync` or `extcon_sync`, preventing registered listeners from receiving state change notifications.
- [Medium] Extcon state can become permanently desynchronized from the hardware/software state when regmap updates fail.

Pre-existing issues:
- [High] The driver's remove path races with the devres-managed threaded IRQ handler, leading to a Use-After-Free of the driver data structure.
--

commit 5e21d209058ee93304da0e1964b84339cf8709c2
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 3be2b3afaaf4d..8978a95c1c199 100644
> --- a/drivers/phy/motorola/phy-cpcap-usb.c
> +++ b/drivers/phy/motorola/phy-cpcap-usb.c
> @@ -123,6 +124,7 @@ struct cpcap_phy_ddata {
>  	struct regmap *reg;
>  	struct device *dev;
>  	struct usb_phy phy;
> +	struct extcon_dev *edev;
>  	struct delayed_work detect_work;

[Severity: High]
This is a pre-existing issue, but does the remove path race with the
devres-managed threaded IRQ handler, leading to a use-after-free of this
driver data structure?

If the devres-managed IRQ thread cpcap_phy_irq_thread() fires and reads
ddata->active as 1, but is preempted before calling schedule_delayed_work():

And then the driver remove path cpcap_usb_phy_remove() executes, sets
ddata->active to 0, cancels any pending work, and returns.

Devres cleanup then begins, calling devm_irq_release() -> free_irq() ->
synchronize_irq(), which blocks waiting for the preempted IRQ thread.

When the IRQ thread resumes, it successfully queues the delayed work and
returns.

Once devm_irq_release() finishes, devres proceeds to free ddata.
The delayed work timer expires and executes cpcap_usb_detect() on the freed
ddata memory.

> [ ... ]
> @@ -226,6 +239,43 @@ static void cpcap_usb_try_musb_mailbox(struct cpcap_phy_ddata *ddata,
> [ ... ]
> +static void cpcap_usb_set_extcon(struct cpcap_phy_ddata *ddata)
> +{
> [ ... ]
> +	if (ddata->cable == EXTCON_CHG_USB_SDP)
> +		extcon_set_state(ddata->edev, EXTCON_USB, false);
> +	else if (cable == EXTCON_CHG_USB_SDP)
> +		extcon_set_state(ddata->edev, EXTCON_USB, true);

[Severity: High]
Does this prevent registered listeners from receiving state change
notifications for EXTCON_USB?

The EXTCON_USB state is updated here using extcon_set_state() without a
corresponding extcon_set_state_sync() or extcon_sync(). When the driver
changes the state to or from CPCAP_USB (which maps to EXTCON_CHG_USB_SDP),
the subsequent extcon_set_state_sync() calls only target the charger cable
ID and its specific notifier chain.

The notifier chain for EXTCON_USB might never be fired.

> +
> +	if (ddata->cable != EXTCON_NONE)
> +		extcon_set_state_sync(ddata->edev, ddata->cable, false);
> +
> +	if (cable != EXTCON_NONE)
> +		extcon_set_state_sync(ddata->edev, cable, true);
> +
> +	ddata->cable = cable;
> +}
> [ ... ]
> @@ -356,6 +411,8 @@ static void cpcap_usb_detect(struct work_struct *work)
> [ ... ]
>  	if (ddata->mode == CPCAP_IDLE)
>  		return;
>  
>  	ddata->mode = CPCAP_IDLE;
>  	cpcap_usb_try_musb_mailbox(ddata, MUSB_VBUS_OFF);
>  
>  	error = cpcap_usb_set_idle_mode(ddata);
>  	if (error)
>  		goto out_err;
>  
> +	cpcap_usb_set_extcon(ddata);
> +
>  	return;
>  
>  out_err:

[Severity: Medium]
Can the extcon state become permanently desynchronized from the hardware and
software state if a regmap update fails?

Earlier in cpcap_usb_detect(), the internal software state ddata->mode is
updated prior to hardware configuration. If the hardware register update
fails, the code branches to out_err and skips calling
cpcap_usb_set_extcon().

Because ddata->mode is already modified, subsequent executions of
cpcap_usb_detect() will take the early return path and will never retry
updating the extcon state, leaving it stale indefinitely.

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