Message-ID: <410670D7E743164D87FA6160E7907A56013A7B4794@am04wembxa.internal.synopsys.com>
Date:   Fri, 7 Dec 2018 09:06:26 +0000
From:   Minas Harutyunyan <minas.harutyunyan@...opsys.com>
To:     Maynard CABIENTE <maynard.cabiente@...itan.com>,
        Minas Harutyunyan <minas.harutyunyan@...opsys.com>,
        Dan Carpenter <dan.carpenter@...cle.com>,
        Marek Szyprowski <m.szyprowski@...sung.com>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Felipe Balbi <felipe.balbi@...ux.intel.com>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>
Subject: Re: [PATCH] usb: dwc2: Revert "usb: dwc2: Disable all EP's on
 disconnect"

Hi Maynard,

On 12/7/2018 3:09 AM, Maynard CABIENTE wrote:
> Hi Minas,
> 
> I tried your new patch on top of the other 2 patches for a couple of days now and I do not see the issue that Marek encountered. Of course, I did not see it also on the original two patches you created. I also do not see the original FIFO map warning issue that I have with all 3 patches intact.
> 
> However, I do have 2 issues that I'm not certain if this was created by these patches or already there in the beginning (prior to applying all your 3 patches). Again my system is an Altera Cyclone V SoC FPGA running on linux kernel 4.14.44. We are using both USB HID and mass storage gadget. The 2 issues may occur when USB mass storage is in use.
> 
> First issue I am encountering is (sometimes continuous) multiple USB resets occurring when using the USB mass storage gadget. When I used a USB sniffer (Ellisys) to see the cause of the USB reset, I am seeing an out-of-order USB mass storage transfer. That is, the transfer is supposed to follow (1) Command Block Wrapper (CBW), (2) Data, (3) Command Status Wrapper (CSW). When the USB reset occur, I see either one of the following prior to the reset:
> 
> - Data comes in first before the CBW
> - Request Sense CBW, Data then no CSW followed by a Test Unit Ready CBW and Data with no CSW again
> - CBW, CSW and then Data
> 
> Second issue that I see is a deadlock then my system will reboot. This happens when I re-enable the USB mass storage gadget after disabling it before.
> 
> The two issues may not be related to your changes. I will revert back all your patches to see if the issue is present already. However, I will hit my original problem when I don't apply at least 2 of your patches, which is the WARNING: CPU: 0 PID: 0 at ../drivers/usb/dwc2/gadget.c:300 dwc2_hsotg_init_fifo+0x34/0x1b4.
> 
> I will get back to you once I test it without your patches.
> 
> Regards,
> Maynard
> 
Thank you for testing.
Issue which described looks like not related to these patches. For that 
issue please open another mail thread with more details. We will work on it.

Thanks,
Minas


> On Tuesday, December 04, 2018 7:34 AM, 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 :-)
>>
>> 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
> 
> ________________________________
> 
> Ce message, ainsi que tous les fichiers joints à ce message, peuvent contenir des informations sensibles et/ ou confidentielles ne devant pas être divulguées. Si vous n'êtes pas le destinataire de ce message (ou que vous recevez ce message par erreur), nous vous remercions de le notifier immédiatement à son expéditeur, et de détruire ce message. Toute copie, divulgation, modification, utilisation ou diffusion, non autorisée, directe ou indirecte, de tout ou partie de ce message, est strictement interdite.
> 
> 
> This e-mail, and any document attached hereby, may contain confidential and/or privileged information. If you are not the intended recipient (or have received this e-mail in error) please notify the sender immediately and destroy this e-mail. Any unauthorized, direct or indirect, copying, disclosure, distribution or other use of the material or parts thereof is strictly forbidden.
> 

Powered by blists - more mailing lists