Hi Marek,
On 12/6/2018 7:04 PM, Marek Szyprowski wrote:
> Dear Minas,
>
> On 2018-12-04 13:34, Minas Harutyunyan wrote:
>> On 11/23/2018 6:43 PM, Dan Carpenter wrote:
>>> Ugh... We also had a long thread about the v2 patch but it turns out
>>> the list was not CC'd. I should have checked for that.
>>>
>>> You have to pass a flag to say if the caller holds the lock or not...
>>>
>>> regards,
>>> dan carpenter
>>>
>> Hi Dan, Marek, Maynard,
>>
>> Could you please apply bellow patch over follow patches:
>>
>> dccf1bad4be7 usb: dwc2: Disable all EP's on disconnect
>> 6f774b501928 usb: dwc2: Fix ep disable spinlock flow.
>>
>> Please review and test. Feedback is appreciated :-)
>
> Okay, I finally managed to find some time to check this. Your diff is
> mangled, so I had to manually apply it. Frankly, it is very similar to
> the revert I proposed. I've checked it on my test machines and it fixes
> the issues. I'm not very happy about the unlock/lock design, but it
> should be safe in this case and doesn't make the code even more complex.
> Feel free to add a following tag to the final patch:
>
> Tested-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
Thanks for testing.
>
>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
>> index 8eb25e3597b0..db438d13c36a 100644
>> --- a/drivers/usb/dwc2/gadget.c
>> +++ b/drivers/usb/dwc2/gadget.c
>> @@ -3165,8 +3165,6 @@ static void kill_all_requests(struct dwc2_hsotg
>> *hsotg,
>> dwc2_hsotg_txfifo_flush(hsotg, ep->fifo_index);
>> }
>>
>> -static int dwc2_hsotg_ep_disable(struct usb_ep *ep);
>> -
>> /**
>> * dwc2_hsotg_disconnect - disconnect service
>> * @hsotg: The device state.
>> @@ -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);
>> }
>>
>> 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);
>> }
>>
>> /*
>> @@ -4072,7 +4075,6 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
>> unsigned long flags;
>> u32 epctrl_reg;
>> u32 ctrl;
>> - int locked;
>>
>> dev_dbg(hsotg->dev, "%s(ep %p)\n", __func__, ep);
>>
>> @@ -4088,7 +4090,7 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
>>
>> epctrl_reg = dir_in ? DIEPCTL(index) : DOEPCTL(index);
>>
>> - locked = spin_trylock_irqsave(&hsotg->lock, flags);
>> + spin_lock_irqsave(&hsotg->lock, flags);
>>
>> ctrl = dwc2_readl(hsotg, epctrl_reg);
>>
>> @@ -4112,8 +4114,7 @@ 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);
>> + spin_unlock_irqrestore(&hsotg->lock, flags);
>>
>> return 0;
>> }
>>
>> Thanks,
>> Minas
>>
>>
> Best regards
>
Thanks,
Minas
