[PATCH v5 1/2] usb: dwc2: host: Add missing spinlock in dwc2_hcd_reset_func()

[PATCH v5 1/2] usb: dwc2: host: Add missing spinlock in dwc2_hcd_reset_func()

[Douglas Anderson]

The dwc2_hcd_reset_func() function is only ever called directly by a
delayed work function.  As such no locks are already held when the
function is called.

Doing a read-modify-write of CPU registers and setting fields in the
main hsotg data structure is a bad idea without locks.  Let's add
locks.

The bug was found by code inspection only.  It turns out that the
dwc2_hcd_reset_func() is only ever called today if the
"host_support_fs_ls_low_power" parameter is enabled and no code in
mainline enables that parameter.  Thus no known issues in mainline are
fixed by this patch, but it's still probably wise to fix the function.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
Changes in v5:
- New for v5 of the series

 drivers/usb/dwc2/hcd.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index 42b6c5127618..e8924cecc83a 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -2357,13 +2357,19 @@ static void dwc2_hcd_reset_func(struct work_struct *work)
 {
 	struct dwc2_hsotg *hsotg = container_of(work, struct dwc2_hsotg,
 						reset_work.work);
+	unsigned long flags;
 	u32 hprt0;
 
 	dev_dbg(hsotg->dev, "USB RESET function called\n");
+
+	spin_lock_irqsave(&hsotg->lock, flags);
+
 	hprt0 = dwc2_read_hprt0(hsotg);
 	hprt0 &= ~HPRT0_RST;
 	dwc2_writel(hprt0, hsotg->regs + HPRT0);
 	hsotg->flags.b.port_reset_change = 1;
+
+	spin_unlock_irqrestore(&hsotg->lock, flags);
 }
 
 /*
-- 
2.6.0.rc2.230.g3dd15c0

[PATCH v5 2/2] usb: dwc2: host: Clear interrupts before handling them

[Douglas Anderson]

In general it is wise to clear interrupts before processing them.  If
you don't do that, you can get:
 1. Interrupt happens
 2. You look at system state and process interrupt
 3. A new interrupt happens
 4. You clear interrupt without processing it.

This patch was actually a first attempt to fix missing device insertions
as described in (usb: dwc2: host: Fix missing device insertions) and it
did solve some of the signal bouncing problems but not all of
them (which is why I submitted the other patch).  Specifically, this
patch itself would sometimes change:
 1. hardware sees connect
 2. hardware sees disconnect
 3. hardware sees connect
 4. dwc2_port_intr() - clears connect interrupt
 5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect()

...to:
 1. hardware sees connect
 2. hardware sees disconnect
 3. dwc2_port_intr() - clears connect interrupt
 4. hardware sees connect
 5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect()

...but with different timing then sometimes we'd still miss cable
insertions.

In any case, though this patch doesn't fix any (known) problems, it
still seems wise as a general policy to clear interrupt before handling
them.

Note that for dwc2_handle_usb_port_intr(), instead of moving the clear
of PRTINT to the beginning of the function we remove it completely.  The
only way to clear PRTINT is to clear the sources that set it in the
first place.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
Changes in v5:
- Rebased upon testing/next from 15-11-20.
- Fixed missing reset as found by code inspection during rebase.
- Now atop fix for missing spinlock in reset.
- Dropped John Youn's ack / tested by since there's a bugfix now.

Changes in v4:
- Don't replace dwc2_writel with writel (Antti Seppälä).
- Update description to explain why we remove PRTINT clear.

Changes in v3:
- Don't (uselessly) clear the PRTINT anymore (Felipe Balbi).

 drivers/usb/dwc2/core_intr.c | 49 +++++++++++++++++++++-----------------------
 drivers/usb/dwc2/hcd_intr.c  | 18 ++++++++--------
 2 files changed, 32 insertions(+), 35 deletions(-)

diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
index 61601d16e233..d85c5c9f96c1 100644
--- a/drivers/usb/dwc2/core_intr.c
+++ b/drivers/usb/dwc2/core_intr.c
@@ -86,9 +86,6 @@ static void dwc2_handle_usb_port_intr(struct dwc2_hsotg *hsotg)
 		hprt0 &= ~HPRT0_ENA;
 		dwc2_writel(hprt0, hsotg->regs + HPRT0);
 	}
-
-	/* Clear interrupt */
-	dwc2_writel(GINTSTS_PRTINT, hsotg->regs + GINTSTS);
 }
 
 /**
@@ -98,11 +95,11 @@ static void dwc2_handle_usb_port_intr(struct dwc2_hsotg *hsotg)
  */
 static void dwc2_handle_mode_mismatch_intr(struct dwc2_hsotg *hsotg)
 {
-	dev_warn(hsotg->dev, "Mode Mismatch Interrupt: currently in %s mode\n",
-		 dwc2_is_host_mode(hsotg) ? "Host" : "Device");
-
 	/* Clear interrupt */
 	dwc2_writel(GINTSTS_MODEMIS, hsotg->regs + GINTSTS);
+
+	dev_warn(hsotg->dev, "Mode Mismatch Interrupt: currently in %s mode\n",
+		 dwc2_is_host_mode(hsotg) ? "Host" : "Device");
 }
 
 /**
@@ -276,9 +273,13 @@ static void dwc2_handle_otg_intr(struct dwc2_hsotg *hsotg)
  */
 static void dwc2_handle_conn_id_status_change_intr(struct dwc2_hsotg *hsotg)
 {
-	u32 gintmsk = dwc2_readl(hsotg->regs + GINTMSK);
+	u32 gintmsk;
+
+	/* Clear interrupt */
+	dwc2_writel(GINTSTS_CONIDSTSCHNG, hsotg->regs + GINTSTS);
 
 	/* Need to disable SOF interrupt immediately */
+	gintmsk = dwc2_readl(hsotg->regs + GINTMSK);
 	gintmsk &= ~GINTSTS_SOF;
 	dwc2_writel(gintmsk, hsotg->regs + GINTMSK);
 
@@ -295,9 +296,6 @@ static void dwc2_handle_conn_id_status_change_intr(struct dwc2_hsotg *hsotg)
 		queue_work(hsotg->wq_otg, &hsotg->wf_otg);
 		spin_lock(&hsotg->lock);
 	}
-
-	/* Clear interrupt */
-	dwc2_writel(GINTSTS_CONIDSTSCHNG, hsotg->regs + GINTSTS);
 }
 
 /**
@@ -315,12 +313,12 @@ static void dwc2_handle_session_req_intr(struct dwc2_hsotg *hsotg)
 {
 	int ret;
 
-	dev_dbg(hsotg->dev, "Session request interrupt - lx_state=%d\n",
-							hsotg->lx_state);
-
 	/* Clear interrupt */
 	dwc2_writel(GINTSTS_SESSREQINT, hsotg->regs + GINTSTS);
 
+	dev_dbg(hsotg->dev, "Session request interrupt - lx_state=%d\n",
+							hsotg->lx_state);
+
 	if (dwc2_is_device_mode(hsotg)) {
 		if (hsotg->lx_state == DWC2_L2) {
 			ret = dwc2_exit_hibernation(hsotg, true);
@@ -347,6 +345,10 @@ static void dwc2_handle_session_req_intr(struct dwc2_hsotg *hsotg)
 static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
 {
 	int ret;
+
+	/* Clear interrupt */
+	dwc2_writel(GINTSTS_WKUPINT, hsotg->regs + GINTSTS);
+
 	dev_dbg(hsotg->dev, "++Resume or Remote Wakeup Detected Interrupt++\n");
 	dev_dbg(hsotg->dev, "%s lxstate = %d\n", __func__, hsotg->lx_state);
 
@@ -368,10 +370,9 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
 		/* Change to L0 state */
 		hsotg->lx_state = DWC2_L0;
 	} else {
-		if (hsotg->core_params->hibernation) {
-			dwc2_writel(GINTSTS_WKUPINT, hsotg->regs + GINTSTS);
+		if (hsotg->core_params->hibernation)
 			return;
-		}
+
 		if (hsotg->lx_state != DWC2_L1) {
 			u32 pcgcctl = dwc2_readl(hsotg->regs + PCGCTL);
 
@@ -385,9 +386,6 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
 			hsotg->lx_state = DWC2_L0;
 		}
 	}
-
-	/* Clear interrupt */
-	dwc2_writel(GINTSTS_WKUPINT, hsotg->regs + GINTSTS);
 }
 
 /*
@@ -396,14 +394,14 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
  */
 static void dwc2_handle_disconnect_intr(struct dwc2_hsotg *hsotg)
 {
+	dwc2_writel(GINTSTS_DISCONNINT, hsotg->regs + GINTSTS);
+
 	dev_dbg(hsotg->dev, "++Disconnect Detected Interrupt++ (%s) %s\n",
 		dwc2_is_host_mode(hsotg) ? "Host" : "Device",
 		dwc2_op_state_str(hsotg));
 
 	if (hsotg->op_state == OTG_STATE_A_HOST)
 		dwc2_hcd_disconnect(hsotg, false);
-
-	dwc2_writel(GINTSTS_DISCONNINT, hsotg->regs + GINTSTS);
 }
 
 /*
@@ -419,6 +417,9 @@ static void dwc2_handle_usb_suspend_intr(struct dwc2_hsotg *hsotg)
 	u32 dsts;
 	int ret;
 
+	/* Clear interrupt */
+	dwc2_writel(GINTSTS_USBSUSP, hsotg->regs + GINTSTS);
+
 	dev_dbg(hsotg->dev, "USB SUSPEND\n");
 
 	if (dwc2_is_device_mode(hsotg)) {
@@ -437,7 +438,7 @@ static void dwc2_handle_usb_suspend_intr(struct dwc2_hsotg *hsotg)
 			if (!dwc2_is_device_connected(hsotg)) {
 				dev_dbg(hsotg->dev,
 						"ignore suspend request before enumeration\n");
-				goto clear_int;
+				return;
 			}
 
 			ret = dwc2_enter_hibernation(hsotg);
@@ -476,10 +477,6 @@ skip_power_saving:
 			hsotg->op_state = OTG_STATE_A_HOST;
 		}
 	}
-
-clear_int:
-	/* Clear interrupt */
-	dwc2_writel(GINTSTS_USBSUSP, hsotg->regs + GINTSTS);
 }
 
 #define GINTMSK_COMMON	(GINTSTS_WKUPINT | GINTSTS_SESSREQINT |		\
diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
index a1eb48e54858..f8253803a050 100644
--- a/drivers/usb/dwc2/hcd_intr.c
+++ b/drivers/usb/dwc2/hcd_intr.c
@@ -122,6 +122,9 @@ static void dwc2_sof_intr(struct dwc2_hsotg *hsotg)
 	struct dwc2_qh *qh;
 	enum dwc2_transaction_type tr_type;
 
+	/* Clear interrupt */
+	dwc2_writel(GINTSTS_SOF, hsotg->regs + GINTSTS);
+
 #ifdef DEBUG_SOF
 	dev_vdbg(hsotg->dev, "--Start of Frame Interrupt--\n");
 #endif
@@ -146,9 +149,6 @@ static void dwc2_sof_intr(struct dwc2_hsotg *hsotg)
 	tr_type = dwc2_hcd_select_transactions(hsotg);
 	if (tr_type != DWC2_TRANSACTION_NONE)
 		dwc2_hcd_queue_transactions(hsotg, tr_type);
-
-	/* Clear interrupt */
-	dwc2_writel(GINTSTS_SOF, hsotg->regs + GINTSTS);
 }
 
 /*
@@ -312,6 +312,7 @@ static void dwc2_hprt0_enable(struct dwc2_hsotg *hsotg, u32 hprt0,
 
 	if (do_reset) {
 		*hprt0_modify |= HPRT0_RST;
+		dwc2_writel(*hprt0_modify, hsotg->regs + HPRT0);
 		queue_delayed_work(hsotg->wq_otg, &hsotg->reset_work,
 				   msecs_to_jiffies(60));
 	} else {
@@ -347,11 +348,12 @@ static void dwc2_port_intr(struct dwc2_hsotg *hsotg)
 	 * Set flag and clear if detected
 	 */
 	if (hprt0 & HPRT0_CONNDET) {
+		dwc2_writel(hprt0_modify | HPRT0_CONNDET, hsotg->regs + HPRT0);
+
 		dev_vdbg(hsotg->dev,
 			 "--Port Interrupt HPRT0=0x%08x Port Connect Detected--\n",
 			 hprt0);
 		dwc2_hcd_connect(hsotg);
-		hprt0_modify |= HPRT0_CONNDET;
 
 		/*
 		 * The Hub driver asserts a reset when it sees port connect
@@ -364,10 +366,10 @@ static void dwc2_port_intr(struct dwc2_hsotg *hsotg)
 	 * Clear if detected - Set internal flag if disabled
 	 */
 	if (hprt0 & HPRT0_ENACHG) {
+		dwc2_writel(hprt0_modify | HPRT0_ENACHG, hsotg->regs + HPRT0);
 		dev_vdbg(hsotg->dev,
 			 "  --Port Interrupt HPRT0=0x%08x Port Enable Changed (now %d)--\n",
 			 hprt0, !!(hprt0 & HPRT0_ENA));
-		hprt0_modify |= HPRT0_ENACHG;
 		if (hprt0 & HPRT0_ENA) {
 			hsotg->new_connection = true;
 			dwc2_hprt0_enable(hsotg, hprt0, &hprt0_modify);
@@ -387,15 +389,13 @@ static void dwc2_port_intr(struct dwc2_hsotg *hsotg)
 
 	/* Overcurrent Change Interrupt */
 	if (hprt0 & HPRT0_OVRCURRCHG) {
+		dwc2_writel(hprt0_modify | HPRT0_OVRCURRCHG,
+			    hsotg->regs + HPRT0);
 		dev_vdbg(hsotg->dev,
 			 "  --Port Interrupt HPRT0=0x%08x Port Overcurrent Changed--\n",
 			 hprt0);
 		hsotg->flags.b.port_over_current_change = 1;
-		hprt0_modify |= HPRT0_OVRCURRCHG;
 	}
-
-	/* Clear Port Interrupts */
-	dwc2_writel(hprt0_modify, hsotg->regs + HPRT0);
 }
 
 /*
-- 
2.6.0.rc2.230.g3dd15c0

Re: [PATCH v5 2/2] usb: dwc2: host: Clear interrupts before handling them

[John Youn]

On 11/20/2015 9:06 AM, Douglas Anderson wrote:
> In general it is wise to clear interrupts before processing them.  If
> you don't do that, you can get:
>  1. Interrupt happens
>  2. You look at system state and process interrupt
>  3. A new interrupt happens
>  4. You clear interrupt without processing it.
> 
> This patch was actually a first attempt to fix missing device insertions
> as described in (usb: dwc2: host: Fix missing device insertions) and it
> did solve some of the signal bouncing problems but not all of
> them (which is why I submitted the other patch).  Specifically, this
> patch itself would sometimes change:
>  1. hardware sees connect
>  2. hardware sees disconnect
>  3. hardware sees connect
>  4. dwc2_port_intr() - clears connect interrupt
>  5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect()
> 
> ...to:
>  1. hardware sees connect
>  2. hardware sees disconnect
>  3. dwc2_port_intr() - clears connect interrupt
>  4. hardware sees connect
>  5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect()
> 
> ...but with different timing then sometimes we'd still miss cable
> insertions.
> 
> In any case, though this patch doesn't fix any (known) problems, it
> still seems wise as a general policy to clear interrupt before handling
> them.
> 
> Note that for dwc2_handle_usb_port_intr(), instead of moving the clear
> of PRTINT to the beginning of the function we remove it completely.  The
> only way to clear PRTINT is to clear the sources that set it in the
> first place.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> Changes in v5:
> - Rebased upon testing/next from 15-11-20.
> - Fixed missing reset as found by code inspection during rebase.
> - Now atop fix for missing spinlock in reset.
> - Dropped John Youn's ack / tested by since there's a bugfix now.
> 
> Changes in v4:
> - Don't replace dwc2_writel with writel (Antti Seppälä).
> - Update description to explain why we remove PRTINT clear.
> 
> Changes in v3:
> - Don't (uselessly) clear the PRTINT anymore (Felipe Balbi).
> 
>  drivers/usb/dwc2/core_intr.c | 49 +++++++++++++++++++++-----------------------
>  drivers/usb/dwc2/hcd_intr.c  | 18 ++++++++--------
>  2 files changed, 32 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
> index 61601d16e233..d85c5c9f96c1 100644
> --- a/drivers/usb/dwc2/core_intr.c
> +++ b/drivers/usb/dwc2/core_intr.c
> @@ -86,9 +86,6 @@ static void dwc2_handle_usb_port_intr(struct dwc2_hsotg *hsotg)
>  		hprt0 &= ~HPRT0_ENA;
>  		dwc2_writel(hprt0, hsotg->regs + HPRT0);
>  	}
> -
> -	/* Clear interrupt */
> -	dwc2_writel(GINTSTS_PRTINT, hsotg->regs + GINTSTS);
>  }
>  
>  /**
> @@ -98,11 +95,11 @@ static void dwc2_handle_usb_port_intr(struct dwc2_hsotg *hsotg)
>   */
>  static void dwc2_handle_mode_mismatch_intr(struct dwc2_hsotg *hsotg)
>  {
> -	dev_warn(hsotg->dev, "Mode Mismatch Interrupt: currently in %s mode\n",
> -		 dwc2_is_host_mode(hsotg) ? "Host" : "Device");
> -
>  	/* Clear interrupt */
>  	dwc2_writel(GINTSTS_MODEMIS, hsotg->regs + GINTSTS);
> +
> +	dev_warn(hsotg->dev, "Mode Mismatch Interrupt: currently in %s mode\n",
> +		 dwc2_is_host_mode(hsotg) ? "Host" : "Device");
>  }
>  
>  /**
> @@ -276,9 +273,13 @@ static void dwc2_handle_otg_intr(struct dwc2_hsotg *hsotg)
>   */
>  static void dwc2_handle_conn_id_status_change_intr(struct dwc2_hsotg *hsotg)
>  {
> -	u32 gintmsk = dwc2_readl(hsotg->regs + GINTMSK);
> +	u32 gintmsk;
> +
> +	/* Clear interrupt */
> +	dwc2_writel(GINTSTS_CONIDSTSCHNG, hsotg->regs + GINTSTS);
>  
>  	/* Need to disable SOF interrupt immediately */
> +	gintmsk = dwc2_readl(hsotg->regs + GINTMSK);
>  	gintmsk &= ~GINTSTS_SOF;
>  	dwc2_writel(gintmsk, hsotg->regs + GINTMSK);
>  
> @@ -295,9 +296,6 @@ static void dwc2_handle_conn_id_status_change_intr(struct dwc2_hsotg *hsotg)
>  		queue_work(hsotg->wq_otg, &hsotg->wf_otg);
>  		spin_lock(&hsotg->lock);
>  	}
> -
> -	/* Clear interrupt */
> -	dwc2_writel(GINTSTS_CONIDSTSCHNG, hsotg->regs + GINTSTS);
>  }
>  
>  /**
> @@ -315,12 +313,12 @@ static void dwc2_handle_session_req_intr(struct dwc2_hsotg *hsotg)
>  {
>  	int ret;
>  
> -	dev_dbg(hsotg->dev, "Session request interrupt - lx_state=%d\n",
> -							hsotg->lx_state);
> -
>  	/* Clear interrupt */
>  	dwc2_writel(GINTSTS_SESSREQINT, hsotg->regs + GINTSTS);
>  
> +	dev_dbg(hsotg->dev, "Session request interrupt - lx_state=%d\n",
> +							hsotg->lx_state);
> +
>  	if (dwc2_is_device_mode(hsotg)) {
>  		if (hsotg->lx_state == DWC2_L2) {
>  			ret = dwc2_exit_hibernation(hsotg, true);
> @@ -347,6 +345,10 @@ static void dwc2_handle_session_req_intr(struct dwc2_hsotg *hsotg)
>  static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
>  {
>  	int ret;
> +
> +	/* Clear interrupt */
> +	dwc2_writel(GINTSTS_WKUPINT, hsotg->regs + GINTSTS);
> +
>  	dev_dbg(hsotg->dev, "++Resume or Remote Wakeup Detected Interrupt++\n");
>  	dev_dbg(hsotg->dev, "%s lxstate = %d\n", __func__, hsotg->lx_state);
>  
> @@ -368,10 +370,9 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
>  		/* Change to L0 state */
>  		hsotg->lx_state = DWC2_L0;
>  	} else {
> -		if (hsotg->core_params->hibernation) {
> -			dwc2_writel(GINTSTS_WKUPINT, hsotg->regs + GINTSTS);
> +		if (hsotg->core_params->hibernation)
>  			return;
> -		}
> +
>  		if (hsotg->lx_state != DWC2_L1) {
>  			u32 pcgcctl = dwc2_readl(hsotg->regs + PCGCTL);
>  
> @@ -385,9 +386,6 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
>  			hsotg->lx_state = DWC2_L0;
>  		}
>  	}
> -
> -	/* Clear interrupt */
> -	dwc2_writel(GINTSTS_WKUPINT, hsotg->regs + GINTSTS);
>  }
>  
>  /*
> @@ -396,14 +394,14 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
>   */
>  static void dwc2_handle_disconnect_intr(struct dwc2_hsotg *hsotg)
>  {
> +	dwc2_writel(GINTSTS_DISCONNINT, hsotg->regs + GINTSTS);
> +
>  	dev_dbg(hsotg->dev, "++Disconnect Detected Interrupt++ (%s) %s\n",
>  		dwc2_is_host_mode(hsotg) ? "Host" : "Device",
>  		dwc2_op_state_str(hsotg));
>  
>  	if (hsotg->op_state == OTG_STATE_A_HOST)
>  		dwc2_hcd_disconnect(hsotg, false);
> -
> -	dwc2_writel(GINTSTS_DISCONNINT, hsotg->regs + GINTSTS);
>  }
>  
>  /*
> @@ -419,6 +417,9 @@ static void dwc2_handle_usb_suspend_intr(struct dwc2_hsotg *hsotg)
>  	u32 dsts;
>  	int ret;
>  
> +	/* Clear interrupt */
> +	dwc2_writel(GINTSTS_USBSUSP, hsotg->regs + GINTSTS);
> +
>  	dev_dbg(hsotg->dev, "USB SUSPEND\n");
>  
>  	if (dwc2_is_device_mode(hsotg)) {
> @@ -437,7 +438,7 @@ static void dwc2_handle_usb_suspend_intr(struct dwc2_hsotg *hsotg)
>  			if (!dwc2_is_device_connected(hsotg)) {
>  				dev_dbg(hsotg->dev,
>  						"ignore suspend request before enumeration\n");
> -				goto clear_int;
> +				return;
>  			}
>  
>  			ret = dwc2_enter_hibernation(hsotg);
> @@ -476,10 +477,6 @@ skip_power_saving:
>  			hsotg->op_state = OTG_STATE_A_HOST;
>  		}
>  	}
> -
> -clear_int:
> -	/* Clear interrupt */
> -	dwc2_writel(GINTSTS_USBSUSP, hsotg->regs + GINTSTS);
>  }
>  
>  #define GINTMSK_COMMON	(GINTSTS_WKUPINT | GINTSTS_SESSREQINT |		\
> diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
> index a1eb48e54858..f8253803a050 100644
> --- a/drivers/usb/dwc2/hcd_intr.c
> +++ b/drivers/usb/dwc2/hcd_intr.c
> @@ -122,6 +122,9 @@ static void dwc2_sof_intr(struct dwc2_hsotg *hsotg)
>  	struct dwc2_qh *qh;
>  	enum dwc2_transaction_type tr_type;
>  
> +	/* Clear interrupt */
> +	dwc2_writel(GINTSTS_SOF, hsotg->regs + GINTSTS);
> +
>  #ifdef DEBUG_SOF
>  	dev_vdbg(hsotg->dev, "--Start of Frame Interrupt--\n");
>  #endif
> @@ -146,9 +149,6 @@ static void dwc2_sof_intr(struct dwc2_hsotg *hsotg)
>  	tr_type = dwc2_hcd_select_transactions(hsotg);
>  	if (tr_type != DWC2_TRANSACTION_NONE)
>  		dwc2_hcd_queue_transactions(hsotg, tr_type);
> -
> -	/* Clear interrupt */
> -	dwc2_writel(GINTSTS_SOF, hsotg->regs + GINTSTS);
>  }
>  
>  /*
> @@ -312,6 +312,7 @@ static void dwc2_hprt0_enable(struct dwc2_hsotg *hsotg, u32 hprt0,
>  
>  	if (do_reset) {
>  		*hprt0_modify |= HPRT0_RST;
> +		dwc2_writel(*hprt0_modify, hsotg->regs + HPRT0);
>  		queue_delayed_work(hsotg->wq_otg, &hsotg->reset_work,
>  				   msecs_to_jiffies(60));
>  	} else {
> @@ -347,11 +348,12 @@ static void dwc2_port_intr(struct dwc2_hsotg *hsotg)
>  	 * Set flag and clear if detected
>  	 */
>  	if (hprt0 & HPRT0_CONNDET) {
> +		dwc2_writel(hprt0_modify | HPRT0_CONNDET, hsotg->regs + HPRT0);
> +
>  		dev_vdbg(hsotg->dev,
>  			 "--Port Interrupt HPRT0=0x%08x Port Connect Detected--\n",
>  			 hprt0);
>  		dwc2_hcd_connect(hsotg);
> -		hprt0_modify |= HPRT0_CONNDET;
>  
>  		/*
>  		 * The Hub driver asserts a reset when it sees port connect
> @@ -364,10 +366,10 @@ static void dwc2_port_intr(struct dwc2_hsotg *hsotg)
>  	 * Clear if detected - Set internal flag if disabled
>  	 */
>  	if (hprt0 & HPRT0_ENACHG) {
> +		dwc2_writel(hprt0_modify | HPRT0_ENACHG, hsotg->regs + HPRT0);
>  		dev_vdbg(hsotg->dev,
>  			 "  --Port Interrupt HPRT0=0x%08x Port Enable Changed (now %d)--\n",
>  			 hprt0, !!(hprt0 & HPRT0_ENA));
> -		hprt0_modify |= HPRT0_ENACHG;
>  		if (hprt0 & HPRT0_ENA) {
>  			hsotg->new_connection = true;
>  			dwc2_hprt0_enable(hsotg, hprt0, &hprt0_modify);
> @@ -387,15 +389,13 @@ static void dwc2_port_intr(struct dwc2_hsotg *hsotg)
>  
>  	/* Overcurrent Change Interrupt */
>  	if (hprt0 & HPRT0_OVRCURRCHG) {
> +		dwc2_writel(hprt0_modify | HPRT0_OVRCURRCHG,
> +			    hsotg->regs + HPRT0);
>  		dev_vdbg(hsotg->dev,
>  			 "  --Port Interrupt HPRT0=0x%08x Port Overcurrent Changed--\n",
>  			 hprt0);
>  		hsotg->flags.b.port_over_current_change = 1;
> -		hprt0_modify |= HPRT0_OVRCURRCHG;
>  	}
> -
> -	/* Clear Port Interrupts */
> -	dwc2_writel(hprt0_modify, hsotg->regs + HPRT0);
>  }
>  
>  /*
> 

Acked-by: John Youn <johnyoun@synopsys.com>
Tested-by: John Youn <johnyoun@synopsys.com>

Tested on IP v3.20 on HAPS PCI system.

Regards,
John

Re: [PATCH v5 1/2] usb: dwc2: host: Add missing spinlock in dwc2_hcd_reset_func()

[John Youn]

On 11/20/2015 9:06 AM, Douglas Anderson wrote:
> The dwc2_hcd_reset_func() function is only ever called directly by a
> delayed work function.  As such no locks are already held when the
> function is called.
> 
> Doing a read-modify-write of CPU registers and setting fields in the
> main hsotg data structure is a bad idea without locks.  Let's add
> locks.
> 
> The bug was found by code inspection only.  It turns out that the
> dwc2_hcd_reset_func() is only ever called today if the
> "host_support_fs_ls_low_power" parameter is enabled and no code in
> mainline enables that parameter.  Thus no known issues in mainline are
> fixed by this patch, but it's still probably wise to fix the function.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> Changes in v5:
> - New for v5 of the series
> 
>  drivers/usb/dwc2/hcd.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index 42b6c5127618..e8924cecc83a 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -2357,13 +2357,19 @@ static void dwc2_hcd_reset_func(struct work_struct *work)
>  {
>  	struct dwc2_hsotg *hsotg = container_of(work, struct dwc2_hsotg,
>  						reset_work.work);
> +	unsigned long flags;
>  	u32 hprt0;
>  
>  	dev_dbg(hsotg->dev, "USB RESET function called\n");
> +
> +	spin_lock_irqsave(&hsotg->lock, flags);
> +
>  	hprt0 = dwc2_read_hprt0(hsotg);
>  	hprt0 &= ~HPRT0_RST;
>  	dwc2_writel(hprt0, hsotg->regs + HPRT0);
>  	hsotg->flags.b.port_reset_change = 1;
> +
> +	spin_unlock_irqrestore(&hsotg->lock, flags);
>  }
>  
>  /*
> 

Acked-by: John Youn <johnyoun@synopsys.com>

John