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

[PATCH] 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.

Closes: https://lore.kernel.org/linux-usb/8e659c4f-723b-4241-9c2b-8ea42c6c3600@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>
---
 drivers/usb/chipidea/otg_fsm.c | 2 ++
 drivers/usb/chipidea/udc.c     | 3 ++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/chipidea/otg_fsm.c b/drivers/usb/chipidea/otg_fsm.c
index 642729893cc2..03bebee17eb1 100644
--- a/drivers/usb/chipidea/otg_fsm.c
+++ b/drivers/usb/chipidea/otg_fsm.c
@@ -840,6 +840,8 @@ 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);
+
+		return ci_role_start(ci, CI_ROLE_GADGET);
 	}
 
 	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] usb: chipidea: otg: fix fsm deadlock and B-device gadget registration

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] usb: mutex_trylock causes FSM state desync on concurrent gadget unbind
- [High] usb: chipidea: ci->role tracking overwritten causing role leak
- [Medium] usb: sysfs group and interrupt leak on ci_role_start failure
--

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

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

This commit fixes a self-deadlock on gadget-to-host role switches during
OTG FSM state transitions by changing to mutex_trylock. It also ensures the
B-device gadget UDC is properly registered in FSM mode by invoking
ci_role_start.

Closes: https://lore.kernel.org/linux-usb/8e659c4f-723b-4241-9c2b-8ea42c6c3600@gmail.com

> diff --git a/drivers/usb/chipidea/otg_fsm.c b/drivers/usb/chipidea/otg_fsm.c
> --- a/drivers/usb/chipidea/otg_fsm.c
> +++ b/drivers/usb/chipidea/otg_fsm.c
> @@ -840,6 +840,8 @@ 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);
> +
> +		return ci_role_start(ci, CI_ROLE_GADGET);

[Severity: Medium]
Does this return path potentially leak the inputs_attr_group and
ci_otg_wq resources if ci_role_start fails?

Earlier in ci_hdrc_otg_fsm_init, inputs_attr_group is registered in sysfs
and the OTGSC_BSVIE interrupt is enabled just above this new call. If
ci_role_start returns an error, it appears this immediately propagates up to
ci_hdrc_probe, bypassing local cleanup like removing the sysfs group and
disabling the interrupt.

Furthermore, returning an error here propagates to ci_hdrc_probe, which
jumps to the deinit_gadget label, bypassing the deinit_otg label where the
ci_otg_wq workqueue would normally be destroyed.

[Severity: High]
Could this also cause the ci->role tracking state to become corrupted
during probe?

This new call to ci_role_start internally sets ci->role = CI_ROLE_GADGET.
However, when returning back to ci_hdrc_probe, the code unconditionally
executes:

ci->role = ci_get_role(ci);

If the ID pin changes from 1 to 0 in the brief window between
ci_hdrc_otg_fsm_init and ci_get_role during probe, ci->role is overwritten
to CI_ROLE_HOST while the CI_ROLE_GADGET role remains actively running. When
the FSM later transitions and attempts to stop the current role via
ci_role_stop, it uses the corrupted ci->role state to stop the non-existent
Host role instead of the running Gadget role, which can lead to both drivers
being active simultaneously.

>  	}
>  
>  	return 0;

[ ... ]

> diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
> --- 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]
Can this mutex_trylock abort state cleanup on contention from unrelated
threads?

The commit message notes this is to fix a self-deadlock when the FSM thread
already holds the lock. However, mutex_trylock returns false if any thread
holds the lock, not just the current thread.

If an independent thread unbinds the gadget driver (e.g. via sysfs or module
unload) while the FSM worker thread happens to hold the lock for an
unrelated state transition, mutex_trylock will fail here.

The unbind thread would then silently return, skipping the necessary
OTG_STATE_UNDEFINED and PROTO_UNDEF state resets. This would leave the FSM
in an inconsistent state while the driver is unbound, causing it to ignore
future events.

>  	if (ci->fsm.otg->state == OTG_STATE_A_PERIPHERAL) {
>  		ci->fsm.a_bidl_adis_tmout = 1;
>  		ci_hdrc_otg_fsm_start(ci);

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