Re: [PATCH] usb: dwc2: Revert "usb: dwc2: Disable all EP's on disconnect"

[Marek Szyprowski]

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>

> 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
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland