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

["Peter Chen (CIX)" <peter.chen@kernel.org>]

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