* Re: [PATCH v2] usb: chipidea: otg: fix fsm deadlock and B-device gadget registration
2026-10-30 7:19 [PATCH v2] usb: chipidea: otg: fix fsm deadlock and B-device gadget registration Xu Yang
@ 2026-10-30 7:32 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-10-30 7:32 UTC (permalink / raw)
To: Xu Yang; +Cc: linux-usb, imx, Frank.Li
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
^ permalink raw reply [flat|nested] 2+ messages in thread