RE: [PATCH] usb: dwc2: Revert "usb: dwc2: Disable all EP's on disconnect"
From: Maynard CABIENTE
Date: Thu Dec 06 2018 - 18:09:45 EST
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
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.