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

login
register
mail settings
Submitter Minas Harutyunyan
Date Dec. 4, 2018, 12:34 p.m.
Message ID <410670D7E743164D87FA6160E7907A56013A7B19C2@am04wembxa.internal.synopsys.com>
Download mbox | patch
Permalink /patch/671861/
State New
Headers show

Comments

Minas Harutyunyan - Dec. 4, 2018, 12:34 p.m.
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 :-)

   * @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
Marek Szyprowski - Dec. 6, 2018, 3:03 p.m.
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@samsung.com>

> 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
Maynard CABIENTE - Dec. 6, 2018, 11:09 p.m.
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.
Minas Harutyunyan - Dec. 7, 2018, 9:06 a.m.
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.
>
Minas Harutyunyan - Dec. 7, 2018, 9:06 a.m.
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@samsung.com>
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
Felipe Balbi - Dec. 7, 2018, 9:50 a.m.
Hi,


Minas Harutyunyan <minas.harutyunyan@synopsys.com> writes:

> 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@samsung.com>
> Thanks for testing.

should $Subject be applied? Can you resend rebased on testing/next with
Tested-by tags collected if you're happy with it?

thanks
Minas Harutyunyan - Dec. 7, 2018, 9:58 a.m.
Hi Filipe,

My patch dccf1bad4be7eaa096c1f3697bd37883f9a08ecb "usb: dwc2: Disable 
all EP's on disconnect" applied to 4.20-rc1.

I need to update this patch. What I should do. There are 2 options:

1. Ack Marek Szyprowski <m.szyprowski@samsung.com> [PATCH] usb: dwc2: 
Revert "usb: dwc2: Disable all EP's on disconnect" then submit final 
fixed version of patch?

2. Or submit new patch to fix existing "usb: dwc2: Disable all EP's on 
disconnect".

Please advise.

Thanks,
Minas
Felipe Balbi - Dec. 7, 2018, 10:11 a.m.
Hi,

Minas Harutyunyan <minas.harutyunyan@synopsys.com> writes:

> Hi Filipe,
>
> My patch dccf1bad4be7eaa096c1f3697bd37883f9a08ecb "usb: dwc2: Disable 
> all EP's on disconnect" applied to 4.20-rc1.
>
> I need to update this patch. What I should do. There are 2 options:
>
> 1. Ack Marek Szyprowski <m.szyprowski@samsung.com> [PATCH] usb: dwc2: 
> Revert "usb: dwc2: Disable all EP's on disconnect" then submit final 
> fixed version of patch?
>
> 2. Or submit new patch to fix existing "usb: dwc2: Disable all EP's on 
> disconnect".

I would say it's best to go with option 2. Just send a fix on top :-)

Patch

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