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

[sashiko-bot@kernel.org]

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