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

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

[Xu Yang]

From: Xu Yang <xu.yang_2@nxp.com>

Commit dd0c03d68522 ("usb: chipidea: udc: support dynamic gadget
add/remove") introduced two regressions on the OTG FSM path:

1. Self-deadlock on gadget->host role switch.

   otg_statemachine() holds fsm.lock while calling otg_set_state()
   -> otg_set_protocol() -> ci_otg_start_host(fsm, 1), which calls
   ci_role_stop() to tear down the gadget role. Since rdrv->stop is
   now udc_stop() (previously the lightweight udc_id_switch_for_host()),
   this reaches:

     ci_role_stop(ci)
       -> udc_stop(ci)
         -> usb_del_gadget_udc()
           -> gadget_unbind_driver()
             -> usb_gadget_udc_stop_locked()
               -> ci_udc_stop()
                 -> ci_udc_stop_for_otg_fsm()
                   -> mutex_lock(&ci->fsm.lock)  <- deadlock

   The same thread that already holds fsm.lock tries to acquire it
   again. Use mutex_trylock() in ci_udc_stop_for_otg_fsm() instead:
   when the lock is held by this thread, otg_statemachine() is
   already driving the exact same transition and will leave the FSM
   state consistent on its own (otg_set_protocol()/otg_set_state()
   overwrite whatever ci_udc_stop_for_otg_fsm() would set), so it is
   safe to skip the redundant correction rather than deadlock on it.

2. B-device gadget UDC never registered in FSM mode.

   Before dd0c03d68522, ci_hdrc_gadget_init() unconditionally called
   udc_start(), which registers the gadget via usb_add_gadget_udc().
   That commit moved udc_start()/udc_stop() to rdrv->start/stop and
   removed the call from ci_hdrc_gadget_init(), relying on
   ci_role_start() to invoke rdrv->start when a role becomes active.

   However, ci_hdrc_otg_fsm_init() does not go through
   ci_role_start() for the B-device (id=1) case, so the gadget is
   never registered in FSM mode, and no gadget driver can ever bind
   to start the state machine. Call ci_role_start(ci, CI_ROLE_GADGET)
   there to register the gadget, same as the non-FSM mode path does.

Reported-by: Rudraksha Gupta <guptarud@gmail.com>
Closes: https://lore.kernel.org/linux-usb/8e659c4f-723b-4241-9c2b-8ea42c6c3600@gmail.com
Tested-by: Rudraksha Gupta <guptarud@gmail.com>
Fixes: dd0c03d68522 ("usb: chipidea: udc: support dynamic gadget add/remove")
Assisted-by: Claude:claude-sonnet-5
Signed-off-by: Xu Yang <xu.yang_2@nxp.com>

---
Changes in v2:
 - do sysfs_remove_group() if starting role fail reported by sashiko
 - add Reported-by and Tested-by tag
---
 drivers/usb/chipidea/otg_fsm.c | 6 ++++++
 drivers/usb/chipidea/udc.c     | 3 ++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/chipidea/otg_fsm.c b/drivers/usb/chipidea/otg_fsm.c
index 642729893cc2..6d6baea69b68 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);
+		if (retval < 0) {
+			sysfs_remove_group(&ci->dev->kobj, &inputs_attr_group);
+			return retval;
+		}
 	}
 
 	return 0;
diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
index c29b2d0dcc7c..1e0d295f5431 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;
 	if (ci->fsm.otg->state == OTG_STATE_A_PERIPHERAL) {
 		ci->fsm.a_bidl_adis_tmout = 1;
 		ci_hdrc_otg_fsm_start(ci);
-- 
2.34.1

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

[sashiko-bot]

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