Hi,
On 12/4/2018 5:29 PM, Dan Carpenter wrote:
> On Tue, Dec 04, 2018 at 12:34:08PM +0000, Minas Harutyunyan wrote:
>> @@ -3185,12 +3183,13 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg *hsotg)
>> hsotg->connected = 0;
>> hsotg->test_mode = 0;
>>
>> - /* all endpoints should be shutdown */
>> for (ep = 0; ep < hsotg->num_of_eps; ep++) {
>> if (hsotg->eps_in[ep])
>> - dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
>> + kill_all_requests(hsotg, hsotg->eps_in[ep],
>> + -ESHUTDOWN);
>> if (hsotg->eps_out[ep])
>> - dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
>> + kill_all_requests(hsotg, hsotg->eps_out[ep],
>> + -ESHUTDOWN);
>
>
> Should this part be in a separate patch?
>
> I'm not trying to be rhetorical at all. I literally don't know the
> code very well. Hopefully the full commit message will explain it.
>
Actually, this fragment of patch revert changes from V2 and keep
untouched dwc2_hsotg_disconnect() function.
>> }
>>
>> call_gadget(hsotg, disconnect);
>> @@ -3234,6 +3233,8 @@ static void dwc2_hsotg_irq_fifoempty(struct
>> dwc2_hsotg *hsotg, bool periodic)
>> GINTSTS_PTXFEMP | \
>> GINTSTS_RXFLVL)
>>
>> +static int dwc2_hsotg_ep_disable(struct usb_ep *ep);
>> +
>> /**
>> * dwc2_hsotg_core_init - issue softreset to the core
>> * @hsotg: The device state
>> @@ -3258,12 +3259,14 @@ void dwc2_hsotg_core_init_disconnected(struct
>> dwc2_hsotg *hsotg,
>> return;
>> } else {
>> /* all endpoints should be shutdown */
>> + spin_unlock(&hsotg->lock);
>> for (ep = 1; ep < hsotg->num_of_eps; ep++) {
>> if (hsotg->eps_in[ep])
>>
>> dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
>> if (hsotg->eps_out[ep])
>>
>> dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
>> }
>> + spin_lock(&hsotg->lock);
>> }
>>
>> /*
>
> The idea here is that this is the only caller which is holding the
> lock and we drop it here and take it again inside dwc2_hsotg_ep_disable().
> I don't know the code very well and can't totally swear that this
> doesn't introduce a small race condition...
>
Above fragment of patch allow to keep untouched dwc2_hsotg_ep_disble()
function also, without changing spin_lock/_unlock stuff inside function.
My approach here minimally update code to add any races. Just in
dwc2_hsotg_core_init_disconnected() function on USB reset interrupt
perform disabling all EP's. Because on USB reset interrupt, called from interrupt
handler with acquired lock and dwc2_hsotg_ep_disble() function (without
changes) acquire lock, just need to unlock lock to avoid any troubles.
> Another option would be to introduce a new function which takes the lock
> and change all the other callers instead. To me that would be easier to
> review... See below for how it might look:
>
> regards,
> dan carpenter
>
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index 94f3ba995580..b17a5dbefd5f 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -3166,6 +3166,7 @@ static void kill_all_requests(struct dwc2_hsotg *hsotg,
> }
>
> static int dwc2_hsotg_ep_disable(struct usb_ep *ep);
> +static int dwc2_hsotg_ep_disable_lock(struct usb_ep *ep);
>
> /**
> * dwc2_hsotg_disconnect - disconnect service
> @@ -3188,9 +3189,9 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg *hsotg)
> /* all endpoints should be shutdown */
> for (ep = 0; ep < hsotg->num_of_eps; ep++) {
> if (hsotg->eps_in[ep])
> - dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
> + dwc2_hsotg_ep_disable_lock(&hsotg->eps_in[ep]->ep);
> if (hsotg->eps_out[ep])
> - dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
> + dwc2_hsotg_ep_disable_lock(&hsotg->eps_out[ep]->ep);
> }
>
> call_gadget(hsotg, disconnect);
> @@ -4069,10 +4070,8 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
> struct dwc2_hsotg *hsotg = hs_ep->parent;
> int dir_in = hs_ep->dir_in;
> int index = hs_ep->index;
> - unsigned long flags;
> u32 epctrl_reg;
> u32 ctrl;
> - int locked;
>
> dev_dbg(hsotg->dev, "%s(ep %p)\n", __func__, ep);
>
> @@ -4088,10 +4087,6 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
>
> epctrl_reg = dir_in ? DIEPCTL(index) : DOEPCTL(index);
>
> - locked = spin_is_locked(&hsotg->lock);
> - if (!locked)
> - spin_lock_irqsave(&hsotg->lock, flags);
> -
> ctrl = dwc2_readl(hsotg, epctrl_reg);
>
> if (ctrl & DXEPCTL_EPENA)
> @@ -4114,12 +4109,23 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
> hs_ep->fifo_index = 0;
> hs_ep->fifo_size = 0;
>
> - if (!locked)
> - spin_unlock_irqrestore(&hsotg->lock, flags);
> -
> return 0;
> }
>
> +static int dwc2_hsotg_ep_disable_lock(struct usb_ep *ep)
> +{
> + struct dwc2_hsotg_ep *hs_ep = our_ep(ep);
> + struct dwc2_hsotg *hsotg = hs_ep->parent;
> + unsigned long flags;
> + int ret;
> +
> + spin_lock_irqsave(&hsotg->lock, flags);
> + ret = dwc2_hsotg_ep_disable(ep);
> + spin_unlock_irqrestore(&hsotg->lock, flags);
> +
> + return ret;
> +}
> +
> /**
> * on_list - check request is on the given endpoint
> * @ep: The endpoint to check.
> @@ -4267,7 +4273,7 @@ static int dwc2_hsotg_ep_sethalt_lock(struct usb_ep *ep, int value)
>
> static const struct usb_ep_ops dwc2_hsotg_ep_ops = {
> .enable = dwc2_hsotg_ep_enable,
> - .disable = dwc2_hsotg_ep_disable,
> + .disable = dwc2_hsotg_ep_disable_lock,
> .alloc_request = dwc2_hsotg_ep_alloc_request,
> .free_request = dwc2_hsotg_ep_free_request,
> .queue = dwc2_hsotg_ep_queue_lock,
> @@ -4407,9 +4413,9 @@ static int dwc2_hsotg_udc_stop(struct usb_gadget *gadget)
> /* all endpoints should be shutdown */
> for (ep = 1; ep < hsotg->num_of_eps; ep++) {
> if (hsotg->eps_in[ep])
> - dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
> + dwc2_hsotg_ep_disable_lock(&hsotg->eps_in[ep]->ep);
> if (hsotg->eps_out[ep])
> - dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
> + dwc2_hsotg_ep_disable_lock(&hsotg->eps_out[ep]->ep);
> }
>
> spin_lock_irqsave(&hsotg->lock, flags);
> @@ -4857,9 +4863,9 @@ int dwc2_hsotg_suspend(struct dwc2_hsotg *hsotg)
>
> for (ep = 0; ep < hsotg->num_of_eps; ep++) {
> if (hsotg->eps_in[ep])
> - dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
> + dwc2_hsotg_ep_disable_lock(&hsotg->eps_in[ep]->ep);
> if (hsotg->eps_out[ep])
> - dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
> + dwc2_hsotg_ep_disable_lock(&hsotg->eps_out[ep]->ep);
> }
> }
>
>
Your code doesn't take care about fifo_map warnings from
dwc2_hsotg_init_fifo() function. Before calling dwc2_hsotg_init_fifo()
from dwc2_hsotg_core_init_disconnected() function all Ep's should
disabled and fifo bitmap should be cleared.
Thanks,
Minas
