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

From: Minas Harutyunyan <hidden>
Date: 2018-12-05 12:52:32
Also in: lkml

Hi,

On 12/4/2018 5:29 PM, Dan Carpenter wrote:

On Tue, Dec 04, 2018 at 12:34:08PM +0000, Minas Harutyunyan wrote:

quoted

@@ -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);


Should this part be in a separate patch?

I'm not trying to be rhetorical at all.  I literally don't know the
code very well.  Hopefully the full commit message will explain it.

Actually, this fragment of patch revert changes from V2 and keep 
untouched dwc2_hsotg_disconnect() function.

quoted

          }

          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);
          }

          /*

The idea here is that this is the only caller which is holding the
lock and we drop it here and take it again inside dwc2_hsotg_ep_disable().
I don't know the code very well and can't totally swear that this
doesn't introduce a small race condition...

Above fragment of patch allow to keep untouched dwc2_hsotg_ep_disble() 
function also, without changing spin_lock/_unlock stuff inside function.

My approach here minimally update code to add any races. Just in 
dwc2_hsotg_core_init_disconnected() function on USB reset interrupt 
perform disabling all EP's. Because on USB reset interrupt, called from interrupt 
handler with acquired lock and dwc2_hsotg_ep_disble() function (without 
changes) acquire lock, just need to unlock lock to avoid any troubles.

quoted hunk ↗ jump to hunk

Another option would be to introduce a new function which takes the lock
and change all the other callers instead.  To me that would be easier to
review...  See below for how it might look:

regards,
dan carpenter

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 94f3ba995580..b17a5dbefd5f 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c

@@ -3166,6 +3166,7 @@ static void kill_all_requests(struct dwc2_hsotg *hsotg,
  }
  
  static int dwc2_hsotg_ep_disable(struct usb_ep *ep);
+static int dwc2_hsotg_ep_disable_lock(struct usb_ep *ep);
  
  /**
   * dwc2_hsotg_disconnect - disconnect service

@@ -3188,9 +3189,9 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg *hsotg)
  	/* 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);
+			dwc2_hsotg_ep_disable_lock(&hsotg->eps_in[ep]->ep);
  		if (hsotg->eps_out[ep])
-			dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
+			dwc2_hsotg_ep_disable_lock(&hsotg->eps_out[ep]->ep);
  	}
  
  	call_gadget(hsotg, disconnect);

@@ -4069,10 +4070,8 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
  	struct dwc2_hsotg *hsotg = hs_ep->parent;
  	int dir_in = hs_ep->dir_in;
  	int index = hs_ep->index;
-	unsigned long flags;
  	u32 epctrl_reg;
  	u32 ctrl;
-	int locked;
  
  	dev_dbg(hsotg->dev, "%s(ep %p)\n", __func__, ep);

@@ -4088,10 +4087,6 @@ static int dwc2_hsotg_ep_disable(struct usb_ep *ep)
  
  	epctrl_reg = dir_in ? DIEPCTL(index) : DOEPCTL(index);
  
-	locked = spin_is_locked(&hsotg->lock);
-	if (!locked)
-		spin_lock_irqsave(&hsotg->lock, flags);
-
  	ctrl = dwc2_readl(hsotg, epctrl_reg);
  
  	if (ctrl & DXEPCTL_EPENA)

@@ -4114,12 +4109,23 @@ 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);
-
  	return 0;
  }
  
+static int dwc2_hsotg_ep_disable_lock(struct usb_ep *ep)
+{
+	struct dwc2_hsotg_ep *hs_ep = our_ep(ep);
+	struct dwc2_hsotg *hsotg = hs_ep->parent;
+	unsigned long flags;
+	int ret;
+
+	spin_lock_irqsave(&hsotg->lock, flags);
+	ret = dwc2_hsotg_ep_disable(ep);
+	spin_unlock_irqrestore(&hsotg->lock, flags);
+
+	return ret;
+}
+
  /**
   * on_list - check request is on the given endpoint
   * @ep: The endpoint to check.

@@ -4267,7 +4273,7 @@ static int dwc2_hsotg_ep_sethalt_lock(struct usb_ep *ep, int value)
  
  static const struct usb_ep_ops dwc2_hsotg_ep_ops = {
  	.enable		= dwc2_hsotg_ep_enable,
-	.disable	= dwc2_hsotg_ep_disable,
+	.disable	= dwc2_hsotg_ep_disable_lock,
  	.alloc_request	= dwc2_hsotg_ep_alloc_request,
  	.free_request	= dwc2_hsotg_ep_free_request,
  	.queue		= dwc2_hsotg_ep_queue_lock,

@@ -4407,9 +4413,9 @@ static int dwc2_hsotg_udc_stop(struct usb_gadget *gadget)
  	/* all endpoints should be shutdown */
  	for (ep = 1; ep < hsotg->num_of_eps; ep++) {
  		if (hsotg->eps_in[ep])
-			dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
+			dwc2_hsotg_ep_disable_lock(&hsotg->eps_in[ep]->ep);
  		if (hsotg->eps_out[ep])
-			dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
+			dwc2_hsotg_ep_disable_lock(&hsotg->eps_out[ep]->ep);
  	}
  
  	spin_lock_irqsave(&hsotg->lock, flags);

@@ -4857,9 +4863,9 @@ int dwc2_hsotg_suspend(struct dwc2_hsotg *hsotg)
  
  		for (ep = 0; ep < hsotg->num_of_eps; ep++) {
  			if (hsotg->eps_in[ep])
-				dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
+				dwc2_hsotg_ep_disable_lock(&hsotg->eps_in[ep]->ep);
  			if (hsotg->eps_out[ep])
-				dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
+				dwc2_hsotg_ep_disable_lock(&hsotg->eps_out[ep]->ep);
  		}
  	}

Your code doesn't take care about fifo_map warnings from 
dwc2_hsotg_init_fifo() function. Before calling dwc2_hsotg_init_fifo() 
from dwc2_hsotg_core_init_disconnected() function all Ep's should 
disabled and fifo bitmap should be cleared.


Thanks,
Minas

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help