Patchwork usb: dwc2: Revert "usb: dwc2: Disable all EP's on disconnect"

login
register
mail settings
Submitter Dan Carpenter
Date Dec. 4, 2018, 1:29 p.m.
Message ID <20181204132913.GH3073@unbuntlaptop>
Download mbox | patch
Permalink /patch/671891/
State New
Headers show

Comments

Dan Carpenter - Dec. 4, 2018, 1:29 p.m.
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.

>          }
> 
>          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...

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
Minas Harutyunyan - Dec. 5, 2018, 12:52 p.m.
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
Dan Carpenter - Dec. 7, 2018, 10:15 a.m.
On Wed, Dec 05, 2018 at 12:52:22PM +0000, Minas Harutyunyan wrote:
> 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.
> 

To me it feels like there are two issues.  The first is this change, and
the second is fixing the lockdep warning.


> >>           }
> >>
> >>           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.
> 

Yes.  I understand that.  I just don't like it.

Although your patch is more "minimal" in that it touches fewer lines of
code it's actually more complicated because we have to verify that it's
safe to drop the lock.


> > 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.
> 

Correct.  I am only trying to fix the locking.  I hope you can fix the
rest in a separate patch.

regards,
dan carpenter
Minas Harutyunyan - Dec. 7, 2018, 11:20 a.m.
Hi Dan,

On 12/7/2018 2:16 PM, Dan Carpenter wrote:
> On Wed, Dec 05, 2018 at 12:52:22PM +0000, Minas Harutyunyan wrote:
>> 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.
>>
> 
> To me it feels like there are two issues.  The first is this change, and
> the second is fixing the lockdep warning.
> 
> 
>>>>            }
>>>>
>>>>            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.
>>
> 
> Yes.  I understand that.  I just don't like it.
> 
> Although your patch is more "minimal" in that it touches fewer lines of
> code it's actually more complicated because we have to verify that it's
> safe to drop the lock.
> 
> 
>>> 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.
>>
> 
> Correct.  I am only trying to fix the locking.  I hope you can fix the
> rest in a separate patch.
> 
Yeah. I'll try deeper investigate driver locking flow and fix it later. 
Actually, I like your idea with introducing dwc2_hsotg_ep_disable_lock() 
function. Maybe you yourself will submit new patch for safe locking 
fixes? But please just after my patch will applied :-)
Currently there are 2-3 high priority issues reported by community and I 
should find solutions/fixes.
Thank you very much for your time and useful feedback.

Thanks,
Minas


> regards,
> dan carpenter
> 
>
Minas Harutyunyan - Dec. 7, 2018, 2:13 p.m.
Hi Dan,

On 12/7/2018 3:20 PM, Minas Harutyunyan wrote:
> Hi Dan,
> 
> On 12/7/2018 2:16 PM, Dan Carpenter wrote:
>> On Wed, Dec 05, 2018 at 12:52:22PM +0000, Minas Harutyunyan wrote:
>>> 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.
>>>
>>
>> To me it feels like there are two issues.  The first is this change, and
>> the second is fixing the lockdep warning.
>>
>>
>>>>>             }
>>>>>
>>>>>             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.
>>>
>>
>> Yes.  I understand that.  I just don't like it.
>>
>> Although your patch is more "minimal" in that it touches fewer lines of
>> code it's actually more complicated because we have to verify that it's
>> safe to drop the lock.
>>
>>
>>>> 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.
>>>
>>
>> Correct.  I am only trying to fix the locking.  I hope you can fix the
>> rest in a separate patch.
>>
> Yeah. I'll try deeper investigate driver locking flow and fix it later.
> Actually, I like your idea with introducing dwc2_hsotg_ep_disable_lock()
> function. Maybe you yourself will submit new patch for safe locking
> fixes? But please just after my patch will applied :-)
> Currently there are 2-3 high priority issues reported by community and I
> should find solutions/fixes.
> Thank you very much for your time and useful feedback.
> 
> Thanks,
> Minas
> 
> 
>> regards,
>> dan carpenter
>>
>>
> 
> 

My patch doesn't pass sparse checking: "warning: context imbalance in 
'dwc2_hsotg_core_init_disconnected' - unexpected unlock". Sparse persist!
So, I need to re-work patch. Can I use your idea with 
dwc2_hsotg_ep_disable_lock() function to prepare new one?

Thanks,
Minas
Dan Carpenter - Dec. 7, 2018, 2:40 p.m.
On Fri, Dec 07, 2018 at 02:13:18PM +0000, Minas Harutyunyan wrote:
> 
> My patch doesn't pass sparse checking: "warning: context imbalance in 
> 'dwc2_hsotg_core_init_disconnected' - unexpected unlock". Sparse persist!
> So, I need to re-work patch. Can I use your idea with 
> dwc2_hsotg_ep_disable_lock() function to prepare new one?
> 

Sparse lock checking is pretty crap, and I wouldn't go out of my way
normally to make it happy...  But of course you can use my idea.

regards,
dan carpenter

Patch

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);
 		}
 	}