Re: [PATCH 2/3] usb: chipidea: udc: support dynamic gadget add/remove

Peter Chen (CIX)
2026-04-22T02:58:27Z

On 26-04-21 16:24:35, Xu Yang wrote:
> When the device is connected and enumerated by the host, switching the
> role from device to host leaves an asynchronous vbus_event_work() to be
> run. This can affect EHCI host controller initialization process.
>
> If vbus_event_work() runs after ehci_run() sets USBCMD.RUNSTOP bit, then
> RUNSTOP bit will be cleared. As a result, the host controller fails to
> operate.
>
> The log below shows what happens:
>
> [ 87.819925] ci_hdrc ci_hdrc.0: EHCI Host Controller
> [ 87.819963] ci_hdrc ci_hdrc.0: new USB bus registered, assigned bus number 1
> [ 87.955634] ci_hdrc ci_hdrc.0: USB 2.0, controller refused to start: -110
> [ 87.955658] ci_hdrc ci_hdrc.0: startup error -110
> [ 87.955682] ci_hdrc ci_hdrc.0: USB bus 1 deregistered
>
> The problem is that the chipidea UDC driver call usb_udc_vbus_handler() to
> pull down data line but it doesn't guarantee that this operation has
> completed before the host controller starts running.
>
> Now UDC core can properly delete usb gadget device and make sure that vbus
> work is cancelled or completed after usb_del_gadget_udc() is returned. But
> the udc.c only call usb_del_gadget_udc() in ci_hdrc_gadget_destroy(). To
> avoid above issue, let the gadget device add/remove dynamically during USB
> role switching.
>
> To support dynamic gadget add/remove, this firstly clear ci->gadget and
> ci->ci_hw_ep memory so the driver won't access stale data when initialize
> and reuse these data structures again. Secondly, this assign udc_start()
> and udc_stop() to rdrv->start and rdrv->stop, meanwhile it removes
> udc_id_switch_for_device() and udc_id_switch_for_host(). The things in
> them also properly be merged to udc_start()/udc_stop().
>
> Since ci_hdrc_gadget_init() doesn't add gadget anymore, this also adjust
> the order of ci_handle_vbus_change() and ci_role_start(), otherwise, NULL
> pointer will be met.
>
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>

Acked-by: Peter Chen <peter.chen@kernel.org>

Peter
> ---
> drivers/usb/chipidea/core.c | 11 +++----
> drivers/usb/chipidea/udc.c | 65 +++++++++++++++++++------------------
> 2 files changed, 38 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index 7cfabb04a4fb..95d9db159ce8 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -1191,19 +1191,16 @@ static int ci_hdrc_probe(struct platform_device *pdev)
>
> ci->role = ci_get_role(ci);
> if (!ci_otg_is_fsm_mode(ci)) {
> - /* only update vbus status for peripheral */
> - if (ci->role == CI_ROLE_GADGET) {
> - /* Pull down DP for possible charger detection */
> - hw_write(ci, OP_USBCMD, USBCMD_RS, 0);
> - ci_handle_vbus_change(ci);
> - }
> -
> ret = ci_role_start(ci, ci->role);
> if (ret) {
> dev_err(dev, "can't start %s role\n",
> ci_role(ci)->name);
> goto stop;
> }
> +
> + /* only update vbus status for peripheral */
> + if (ci->role == CI_ROLE_GADGET)
> + ci_handle_vbus_change(ci);
> }
>
> ret = devm_request_irq(dev, ci->irq, ci_irq_handler, IRQF_SHARED,
> diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
> index d4277d6611ee..d52f89489893 100644
> --- a/drivers/usb/chipidea/udc.c
> +++ b/drivers/usb/chipidea/udc.c
> @@ -2044,6 +2044,8 @@ static int init_eps(struct ci_hdrc *ci)
> {
> int retval = 0, i, j;
>
> + memset(ci->ci_hw_ep, 0, sizeof(ci->ci_hw_ep));
> +
> for (i = 0; i < ci->hw_ep_max/2; i++)
> for (j = RX; j <= TX; j++) {
> int k = i + j * ci->hw_ep_max/2;
> @@ -2289,6 +2291,8 @@ static int udc_start(struct ci_hdrc *ci)
> struct usb_otg_caps *otg_caps = &ci->platdata->ci_otg_caps;
> int retval = 0;
>
> + memset(&ci->gadget, 0, sizeof(ci->gadget));
> +
> ci->gadget.ops = &usb_gadget_ops;
> ci->gadget.speed = USB_SPEED_UNKNOWN;
> ci->gadget.max_speed = USB_SPEED_HIGH;
> @@ -2327,10 +2331,15 @@ static int udc_start(struct ci_hdrc *ci)
>
> ci->gadget.ep0 = &ci->ep0in->ep;
>
> + if (ci->platdata->pins_device)
> + pinctrl_select_state(ci->platdata->pctl,
> + ci->platdata->pins_device);
> +
> retval = usb_add_gadget_udc(dev, &ci->gadget);
> if (retval)
> goto destroy_eps;
>
> + ci_udc_enable_vbus_irq(ci, true);
> return retval;
>
> destroy_eps:
> @@ -2342,38 +2351,20 @@ static int udc_start(struct ci_hdrc *ci)
> return retval;
> }
>
> -/*
> - * ci_hdrc_gadget_destroy: parent remove must call this to remove UDC
> - *
> - * No interrupts active, the IRQ has been released
> +/**
> + * udc_stop: deinitialize gadget role
> + * @ci: chipidea controller
> */
> -void ci_hdrc_gadget_destroy(struct ci_hdrc *ci)
> +static void udc_stop(struct ci_hdrc *ci)
> {
> - if (!ci->roles[CI_ROLE_GADGET])
> - return;
> -
> + ci_udc_enable_vbus_irq(ci, false);
> usb_del_gadget_udc(&ci->gadget);
> + ci->vbus_active = 0;
>
> destroy_eps(ci);
>
> dma_pool_destroy(ci->td_pool);
> dma_pool_destroy(ci->qh_pool);
> -}
> -
> -static int udc_id_switch_for_device(struct ci_hdrc *ci)
> -{
> - if (ci->platdata->pins_device)
> - pinctrl_select_state(ci->platdata->pctl,
> - ci->platdata->pins_device);
> -
> - ci_udc_enable_vbus_irq(ci, true);
> - return 0;
> -}
> -
> -static void udc_id_switch_for_host(struct ci_hdrc *ci)
> -{
> - ci_udc_enable_vbus_irq(ci, false);
> - ci->vbus_active = 0;
>
> if (ci->platdata->pins_device && ci->platdata->pins_default)
> pinctrl_select_state(ci->platdata->pctl,
> @@ -2422,7 +2413,6 @@ static void udc_resume(struct ci_hdrc *ci, bool power_lost)
> int ci_hdrc_gadget_init(struct ci_hdrc *ci)
> {
> struct ci_role_driver *rdrv;
> - int ret;
>
> if (!hw_read(ci, CAP_DCCPARAMS, DCCPARAMS_DC))
> return -ENXIO;
> @@ -2431,8 +2421,8 @@ int ci_hdrc_gadget_init(struct ci_hdrc *ci)
> if (!rdrv)
> return -ENOMEM;
>
> - rdrv->start = udc_id_switch_for_device;
> - rdrv->stop = udc_id_switch_for_host;
> + rdrv->start = udc_start;
> + rdrv->stop = udc_stop;
> #ifdef CONFIG_PM_SLEEP
> rdrv->suspend = udc_suspend;
> rdrv->resume = udc_resume;
> @@ -2440,9 +2430,22 @@ int ci_hdrc_gadget_init(struct ci_hdrc *ci)
> rdrv->irq = udc_irq;
> rdrv->name = "gadget";
>
> - ret = udc_start(ci);
> - if (!ret)
> - ci->roles[CI_ROLE_GADGET] = rdrv;
> + ci->roles[CI_ROLE_GADGET] = rdrv;
>
> - return ret;
> + /* Pull down DP for possible charger detection */
> + hw_write(ci, OP_USBCMD, USBCMD_RS, 0);
> + return 0;
> +}
> +
> +/*
> + * ci_hdrc_gadget_destroy: parent remove must call this to remove UDC
> + *
> + * No interrupts active, the IRQ has been released
> + */
> +void ci_hdrc_gadget_destroy(struct ci_hdrc *ci)
> +{
> + struct device *dev = &ci->gadget.dev;
> +
> + if (ci->roles[CI_ROLE_GADGET] && device_is_registered(dev))
> + udc_stop(ci);
> }
> --
> 2.34.1
>

--

Best regards,
Peter