Re: [PATCH v2] usb: chipidea: otg: fix fsm deadlock and B-device gadget registration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] usb: chipidea: ci_role_start() in ci_hdrc_otg_fsm_init() zeroes ci->gadget
- [High] usb: chipidea: mutex_trylock() in ci_udc_stop_for_otg_fsm() drops FSM reset
- [High] usb: chipidea: ci_hdrc_otg_fsm_init() starts gadget before IRQ and PM setup
- [Medium] usb: chipidea: ci_role_start() failure in ci_hdrc_otg_fsm_init() leaks wq
- [Medium] usb: chipidea: ci_get_role() in ci_hdrc_probe() overwrites FSM ci->role
--

commit 654769b6450cc8234eb867172a8eeda3be413ebe
Author: Xu Yang <xu.yang_2@nxp.com>

usb: chipidea: otg: fix fsm deadlock and B-device gadget registration

This commit addresses two OTG FSM regressions from dynamic gadget support by
using mutex_trylock() in ci_udc_stop_for_otg_fsm() to avoid a self-deadlock on
ci->fsm.lock and by calling ci_role_start(ci, CI_ROLE_GADGET) in
ci_hdrc_otg_fsm_init() when starting as a B-device.

> diff --git a/drivers/usb/chipidea/otg_fsm.c b/drivers/usb/chipidea/otg_fsm.c
> index 642729893cc20..6d6baea69b687 100644
> --- a/drivers/usb/chipidea/otg_fsm.c
> +++ b/drivers/usb/chipidea/otg_fsm.c
> @@ -840,6 +840,12 @@ int ci_hdrc_otg_fsm_init(struct ci_hdrc *ci)
>  			hw_read_otgsc(ci, OTGSC_BSV) ? 1 : 0;
>  		/* Enable BSV irq */
>  		hw_write_otgsc(ci, OTGSC_BSVIE, OTGSC_BSVIE);
> +
> +		retval = ci_role_start(ci, CI_ROLE_GADGET);

[Severity: High]
Does calling ci_role_start(ci, CI_ROLE_GADGET) here wipe out
ci->gadget.hnp_polling_support?

Earlier in ci_hdrc_otg_fsm_init(), hnp_polling_support is initialized on
ci->gadget:

drivers/usb/chipidea/otg_fsm.c:ci_hdrc_otg_fsm_init() {
	...
	ci->fsm.ops = &ci_otg_ops;
	ci->gadget.hnp_polling_support = 1;
	ci->fsm.host_req_flag = devm_kzalloc(ci->dev, 1, GFP_KERNEL);
	...
}

When ci_role_start(ci, CI_ROLE_GADGET) runs, it calls udc_start(), which
zeroes the entire ci->gadget structure and does not restore
hnp_polling_support:

drivers/usb/chipidea/udc.c:udc_start() {
	...
	memset(&ci->gadget, 0, sizeof(ci->gadget));

	ci->gadget.ops          = &usb_gadget_ops;
	...
}

The same memset() in udc_start() also runs on host-to-gadget role switches in
ci_otg_start_host() -> ci_role_start(ci, CI_ROLE_GADGET), clearing both
ci->gadget.hnp_polling_support and ci->gadget.is_a_peripheral, which causes
composite_setup() to reject OTG_STS_SELECTOR status polling requests with
-EOPNOTSUPP.

[Severity: High]
Can starting CI_ROLE_GADGET inside ci_hdrc_otg_fsm_init() also trigger OTG FSM
work before ci->irq and runtime PM are initialized in ci_hdrc_probe()?

In ci_hdrc_probe(), ci_hdrc_otg_init() -> ci_hdrc_otg_fsm_init() runs before
devm_request_irq() and pm_runtime_enable(). Calling
ci_role_start(ci, CI_ROLE_GADGET) invokes udc_start() -> usb_add_gadget_udc().
If a gadget driver is already registered, it binds synchronously:

usb_add_gadget_udc()
  -> gadget_bind_driver()
    -> ci_udc_start()
      -> ci_hdrc_otg_fsm_start()
        -> ci_otg_queue_work()

Because ci->fsm.id is 1, ci_udc_start() immediately calls
ci_hdrc_otg_fsm_start():

drivers/usb/chipidea/udc.c:ci_udc_start() {
	...
	/* Start otg fsm for B-device */
	if (ci_otg_is_fsm_mode(ci) && ci->fsm.id) {
		ci_hdrc_otg_fsm_start(ci);
		return retval;
	}
	...
}

ci_otg_queue_work() calls disable_irq_nosync(ci->irq) before ci->irq has been
requested and queues ci_otg_work before runtime PM and extcon are set up. When
devm_request_irq() subsequently runs irq_startup(), the IRQ disable depth is
reset to 0, causing the later enable_irq(ci->irq) in ci_otg_work() to warn on
an unbalanced IRQ enable.

In addition, if usb_role_switch_register(), devm_request_irq(), or
ci_extcon_register() fails in ci_hdrc_probe(), the error path runs deinit_otg
(destroying ci->wq) before deinit_gadget, where ci_hdrc_gadget_destroy() ->
udc_stop() -> ci_udc_stop_for_otg_fsm() can still touch the OTG FSM.

[Severity: Medium]
As reported by Sashiko on v1 [1], if ci_role_start() fails in
ci_hdrc_otg_fsm_init(), does this error path leak ci->wq and leave OTGSC_AVVIE
and OTGSC_BSVIE enabled in hardware?

ci_hdrc_otg_init() allocates ci->wq before calling ci_hdrc_otg_fsm_init():

drivers/usb/chipidea/otg.c:ci_hdrc_otg_init() {
	INIT_WORK(&ci->work, ci_otg_work);
	ci->wq = create_freezable_workqueue("ci_otg");
	if (!ci->wq) {
		dev_err(ci->dev, "can't create workqueue\n");
		return -ENODEV;
	}

	if (ci_otg_is_fsm_mode(ci))
		return ci_hdrc_otg_fsm_init(ci);

	return 0;
}

And in ci_hdrc_probe(), a failure from ci_hdrc_otg_init() jumps directly to
deinit_gadget, bypassing deinit_otg where ci_hdrc_otg_destroy() would destroy
ci->wq and disable the OTGSC interrupts:

drivers/usb/chipidea/core.c:ci_hdrc_probe() {
	...
	if (ci->is_otg && ci->roles[CI_ROLE_GADGET]) {
		ret = ci_hdrc_otg_init(ci);
		if (ret) {
			dev_err(dev, "init otg fails, ret = %d\n", ret);
			goto deinit_gadget;
		}
	}
	...
}

> +		if (retval < 0) {
> +			sysfs_remove_group(&ci->dev->kobj, &inputs_attr_group);
> +			return retval;
> +		}
>  	}

[Severity: Medium]
As reported by Sashiko on v1 [2], can ci->role become out of sync with the
running role when ci_hdrc_probe() later calls ci_get_role(ci)?

ci_role_start(ci, CI_ROLE_GADGET) sets ci->role = CI_ROLE_GADGET when
ci->fsm.id is 1, but ci_hdrc_probe() subsequently overwrites ci->role by
re-reading OTGSC_ID without starting or stopping roles in FSM mode:

drivers/usb/chipidea/core.c:ci_hdrc_probe() {
	...
	ci->role = ci_get_role(ci);
	if (!ci_otg_is_fsm_mode(ci)) {
		ret = ci_role_start(ci, ci->role);
	...
}

If the ID pin changes from 1 to 0 between ci_hdrc_otg_fsm_init() and
ci_get_role(), ci->role is overwritten to CI_ROLE_HOST while CI_ROLE_GADGET
remains active, so a subsequent ci_role_stop() will call host_stop() instead
of udc_stop().

> diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
> index c29b2d0dcc7c8..1e0d295f5431a 100644
> --- a/drivers/usb/chipidea/udc.c
> +++ b/drivers/usb/chipidea/udc.c
> @@ -2167,7 +2167,8 @@ static void ci_udc_stop_for_otg_fsm(struct ci_hdrc *ci)
>  	if (!ci_otg_is_fsm_mode(ci))
>  		return;
>  
> -	mutex_lock(&ci->fsm.lock);
> +	if (!mutex_trylock(&ci->fsm.lock))
> +		return;

[Severity: High]
As reported by Sashiko on v1 [3], what happens if a gadget driver is unbound
from userspace while another thread holds ci->fsm.lock?

For example, if userspace unbinds the gadget driver (gadget_unbind_driver() ->
usb_gadget_udc_stop_locked() -> ci_udc_stop() -> ci_udc_stop_for_otg_fsm())
at the same time another CPU holds ci->fsm.lock in an OTG sysfs store handler
such as b_bus_req_store(), a_bus_req_store(), a_bus_drop_store(), or
a_clr_err_store(), or in otg_statemachine() for an unrelated event,
mutex_trylock() will fail and ci_udc_stop_for_otg_fsm() will return early:

drivers/usb/chipidea/udc.c:ci_udc_stop_for_otg_fsm() {
	if (!ci_otg_is_fsm_mode(ci))
		return;

	if (!mutex_trylock(&ci->fsm.lock))
		return;
	if (ci->fsm.otg->state == OTG_STATE_A_PERIPHERAL) {
		ci->fsm.a_bidl_adis_tmout = 1;
		ci_hdrc_otg_fsm_start(ci);
	} else if (ci->fsm.otg->state == OTG_STATE_B_PERIPHERAL) {
		ci->fsm.protocol = PROTO_UNDEF;
		ci->fsm.otg->state = OTG_STATE_UNDEFINED;
	}
	mutex_unlock(&ci->fsm.lock);
}

In OTG_STATE_B_PERIPHERAL, skipping the reset leaves ci->fsm.protocol set to
PROTO_GADGET and ci->fsm.otg->state set to OTG_STATE_B_PERIPHERAL after
ci->driver is cleared, preventing a later gadget rebind from re-entering
OTG_STATE_B_PERIPHERAL and enabling the pull-up. In OTG_STATE_A_PERIPHERAL,
skipping a_bidl_adis_tmout = 1 and ci_hdrc_otg_fsm_start() leaves the A-device
stuck in OTG_STATE_A_PERIPHERAL without a bound gadget driver.

[1] https://lore.kernel.org/r/20261015084140.61544-1-xu.yang_2@oss.nxp.com
[2] https://lore.kernel.org/r/20261015084140.61544-1-xu.yang_2@oss.nxp.com
[3] https://lore.kernel.org/r/20261015084140.61544-1-xu.yang_2@oss.nxp.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030071921.276377-1-xu.yang_2@oss.nxp.com?part=1