Re: [PATCH] usb: dwc2: Revert "usb: dwc2: Disable all EP's on disconnect"
From: Minas Harutyunyan
Date: Fri Dec 07 2018 - 06:20:26 EST
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
>
>