staging: dwc2: refactor dwc2_host_complete()

The parameters to dwc2_host_complete() didn't make much sense.
The 'context' parameter always came from the ->priv member of the
'dwc2_urb' parameter, and both of those always came from a struct
dwc2_qtd. So just pass in the struct dwc2_qtd instead.

This also allows us to null out the dwc2_qtd->urb member after it
is freed, which the calling code forgot to do in several places,
causing random driver crashes from dereferencing the freed pointer.

This also requires the calls to dwc2_hc_handle_tt_clear() to be
moved before the calls to dwc2_host_complete(), otherwise that
routine would do nothing because dwc2_qtd->urb has already been
freed.

Signed-off-by: Paul Zimmerman <paulz@synopsys.com>
Tested-by: Stephen Warren <swarren@wwwdotorg.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

drivers/staging/dwc2/hcd.c

@@ -134,11 +134,8 @@ static void dwc2_kill_urbs_in_qh_list(struct dwc2_hsotg *hsotg,
 	list_for_each_entry_safe(qh, qh_tmp, qh_list, qh_list_entry) {
 		list_for_each_entry_safe(qtd, qtd_tmp, &qh->qtd_list,
 					 qtd_list_entry) {
-			if (qtd->urb != NULL) {
-				dwc2_host_complete(hsotg, qtd->urb->priv,
-						   qtd->urb, -ETIMEDOUT);
-				dwc2_hcd_qtd_unlink_and_free(hsotg, qtd, qh);
-			}
+			dwc2_host_complete(hsotg, qtd, -ETIMEDOUT);
+			dwc2_hcd_qtd_unlink_and_free(hsotg, qtd, qh);
 		}
 	}
 }
@@ -421,6 +418,8 @@ static int dwc2_hcd_urb_dequeue(struct dwc2_hsotg *hsotg,
 		return -EINVAL;
 	}
 
+	urb->priv = NULL;
+
 	if (urb_qtd->in_process && qh->channel) {
 		dwc2_dump_channel_info(hsotg, qh->channel);
 
@@ -2088,23 +2087,29 @@ static void dwc2_free_bus_bandwidth(struct usb_hcd *hcd, u16 bw,
  *
  * Must be called with interrupt disabled and spinlock held
  */
-void dwc2_host_complete(struct dwc2_hsotg *hsotg, void *context,
-			struct dwc2_hcd_urb *dwc2_urb, int status)
+void dwc2_host_complete(struct dwc2_hsotg *hsotg, struct dwc2_qtd *qtd,
+			int status)
 {
-	struct urb *urb = context;
+	struct urb *urb;
 	int i;
 
-	if (!urb) {
-		dev_dbg(hsotg->dev, "## %s: context is NULL ##\n", __func__);
+	if (!qtd) {
+		dev_dbg(hsotg->dev, "## %s: qtd is NULL ##\n", __func__);
 		return;
 	}
 
-	if (!dwc2_urb) {
-		dev_dbg(hsotg->dev, "## %s: dwc2_urb is NULL ##\n", __func__);
+	if (!qtd->urb) {
+		dev_dbg(hsotg->dev, "## %s: qtd->urb is NULL ##\n", __func__);
 		return;
 	}
 
-	urb->actual_length = dwc2_hcd_urb_get_actual_length(dwc2_urb);
+	urb = qtd->urb->priv;
+	if (!urb) {
+		dev_dbg(hsotg->dev, "## %s: urb->priv is NULL ##\n", __func__);
+		return;
+	}
+
+	urb->actual_length = dwc2_hcd_urb_get_actual_length(qtd->urb);
 
 	if (dbg_urb(urb))
 		dev_vdbg(hsotg->dev,
@@ -2121,18 +2126,17 @@ void dwc2_host_complete(struct dwc2_hsotg *hsotg, void *context,
 	}
 
 	if (usb_pipetype(urb->pipe) == PIPE_ISOCHRONOUS) {
-		urb->error_count = dwc2_hcd_urb_get_error_count(dwc2_urb);
+		urb->error_count = dwc2_hcd_urb_get_error_count(qtd->urb);
 		for (i = 0; i < urb->number_of_packets; ++i) {
 			urb->iso_frame_desc[i].actual_length =
 				dwc2_hcd_urb_get_iso_desc_actual_length(
-						dwc2_urb, i);
+						qtd->urb, i);
 			urb->iso_frame_desc[i].status =
-				dwc2_hcd_urb_get_iso_desc_status(dwc2_urb, i);
+				dwc2_hcd_urb_get_iso_desc_status(qtd->urb, i);
 		}
 	}
 
 	urb->status = status;
-	urb->hcpriv = NULL;
 	if (!status) {
 		if ((urb->transfer_flags & URB_SHORT_NOT_OK) &&
 		    urb->actual_length < urb->transfer_buffer_length)
@@ -2149,7 +2153,9 @@ void dwc2_host_complete(struct dwc2_hsotg *hsotg, void *context,
 					urb);
 	}
 
-	kfree(dwc2_urb);
+	urb->hcpriv = NULL;
+	kfree(qtd->urb);
+	qtd->urb = NULL;
 
 	spin_unlock(&hsotg->lock);
 	usb_hcd_giveback_urb(dwc2_hsotg_to_hcd(hsotg), urb, status);

drivers/staging/dwc2/hcd.h

@@ -716,8 +716,8 @@ extern void dwc2_host_disconnect(struct dwc2_hsotg *hsotg);
 extern void dwc2_host_hub_info(struct dwc2_hsotg *hsotg, void *context,
 			       int *hub_addr, int *hub_port);
 extern int dwc2_host_get_speed(struct dwc2_hsotg *hsotg, void *context);
-extern void dwc2_host_complete(struct dwc2_hsotg *hsotg, void *context,
-			       struct dwc2_hcd_urb *dwc2_urb, int status);
+extern void dwc2_host_complete(struct dwc2_hsotg *hsotg, struct dwc2_qtd *qtd,
+			       int status);
 
 #ifdef DEBUG
 /*

drivers/staging/dwc2/hcd_ddma.c

@@ -800,6 +800,9 @@ static int dwc2_cmpl_host_isoc_dma_desc(struct dwc2_hsotg *hsotg,
 	u16 remain = 0;
 	int rc = 0;
 
+	if (!qtd->urb)
+		return -EINVAL;
+
 	frame_desc = &qtd->urb->iso_descs[qtd->isoc_frame_index_last];
 	dma_desc->buf = (u32)(qtd->urb->dma + frame_desc->offset);
 	if (chan->ep_is_in)
@@ -826,7 +829,7 @@ static int dwc2_cmpl_host_isoc_dma_desc(struct dwc2_hsotg *hsotg,
 		 * urb->status is not used for isoc transfers here. The
 		 * individual frame_desc status are used instead.
 		 */
-		dwc2_host_complete(hsotg, qtd->urb->priv, qtd->urb, 0);
+		dwc2_host_complete(hsotg, qtd, 0);
 		dwc2_hcd_qtd_unlink_and_free(hsotg, qtd, qh);
 
 		/*
@@ -884,13 +887,16 @@ static void dwc2_complete_isoc_xfer_ddma(struct dwc2_hsotg *hsotg,
 
 		list_for_each_entry_safe(qtd, qtd_tmp, &qh->qtd_list,
 					 qtd_list_entry) {
-			for (idx = 0; idx < qtd->urb->packet_count; idx++) {
-				frame_desc = &qtd->urb->iso_descs[idx];
-				frame_desc->status = err;
+			if (qtd->urb) {
+				for (idx = 0; idx < qtd->urb->packet_count;
+				     idx++) {
+					frame_desc = &qtd->urb->iso_descs[idx];
+					frame_desc->status = err;
+				}
+
+				dwc2_host_complete(hsotg, qtd, err);
 			}
 
-			dwc2_host_complete(hsotg, qtd->urb->priv, qtd->urb,
-					   err);
 			dwc2_hcd_qtd_unlink_and_free(hsotg, qtd, qh);
 		}
 
@@ -1015,6 +1021,9 @@ static int dwc2_process_non_isoc_desc(struct dwc2_hsotg *hsotg,
 
 	dev_vdbg(hsotg->dev, "%s()\n", __func__);
 
+	if (!urb)
+		return -EINVAL;
+
 	dma_desc = &qh->desc_list[desc_num];
 	n_bytes = qh->n_bytes[desc_num];
 	dev_vdbg(hsotg->dev,
@@ -1024,7 +1033,7 @@ static int dwc2_process_non_isoc_desc(struct dwc2_hsotg *hsotg,
 						     halt_status, n_bytes,
 						     xfer_done);
 	if (failed || (*xfer_done && urb->status != -EINPROGRESS)) {
-		dwc2_host_complete(hsotg, urb->priv, urb, urb->status);
+		dwc2_host_complete(hsotg, qtd, urb->status);
 		dwc2_hcd_qtd_unlink_and_free(hsotg, qtd, qh);
 		dev_vdbg(hsotg->dev, "failed=%1x xfer_done=%1x status=%08x\n",
 			 failed, *xfer_done, urb->status);

drivers/staging/dwc2/hcd_intr.c

@@ -623,7 +623,7 @@ static enum dwc2_halt_status dwc2_update_isoc_urb_state(
 		 * urb->status is not used for isoc transfers. The individual
 		 * frame_desc statuses are used instead.
 		 */
-		dwc2_host_complete(hsotg, urb->priv, urb, 0);
+		dwc2_host_complete(hsotg, qtd, 0);
 		halt_status = DWC2_HC_XFER_URB_COMPLETE;
 	} else {
 		halt_status = DWC2_HC_XFER_COMPLETE;
@@ -714,11 +714,7 @@ static void dwc2_release_channel(struct dwc2_hsotg *hsotg,
 			dev_vdbg(hsotg->dev,
 				 "  Complete URB with transaction error\n");
 			free_qtd = 1;
-			if (qtd->urb) {
-				qtd->urb->status = -EPROTO;
-				dwc2_host_complete(hsotg, qtd->urb->priv,
-						   qtd->urb, -EPROTO);
-			}
+			dwc2_host_complete(hsotg, qtd, -EPROTO);
 		}
 		break;
 	case DWC2_HC_XFER_URB_DEQUEUE:
@@ -731,11 +727,7 @@ static void dwc2_release_channel(struct dwc2_hsotg *hsotg,
 	case DWC2_HC_XFER_PERIODIC_INCOMPLETE:
 		dev_vdbg(hsotg->dev, "  Complete URB with I/O error\n");
 		free_qtd = 1;
-		if (qtd && qtd->urb) {
-			qtd->urb->status = -EIO;
-			dwc2_host_complete(hsotg, qtd->urb->priv, qtd->urb,
-					   -EIO);
-		}
+		dwc2_host_complete(hsotg, qtd, -EIO);
 		break;
 	case DWC2_HC_XFER_NO_HALT_STATUS:
 	default:
@@ -957,7 +949,7 @@ static int dwc2_xfercomp_isoc_split_in(struct dwc2_hsotg *hsotg,
 	}
 
 	if (qtd->isoc_frame_index == qtd->urb->packet_count) {
-		dwc2_host_complete(hsotg, qtd->urb->priv, qtd->urb, 0);
+		dwc2_host_complete(hsotg, qtd, 0);
 		dwc2_release_channel(hsotg, chan, qtd,
 				     DWC2_HC_XFER_URB_COMPLETE);
 	} else {
@@ -1040,7 +1032,7 @@ static void dwc2_hc_xfercomp_intr(struct dwc2_hsotg *hsotg,
 			dev_vdbg(hsotg->dev, "  Control transfer complete\n");
 			if (urb->status == -EINPROGRESS)
 				urb->status = 0;
-			dwc2_host_complete(hsotg, urb->priv, urb, urb->status);
+			dwc2_host_complete(hsotg, qtd, urb->status);
 			halt_status = DWC2_HC_XFER_URB_COMPLETE;
 			break;
 		}
@@ -1053,7 +1045,7 @@ static void dwc2_hc_xfercomp_intr(struct dwc2_hsotg *hsotg,
 		urb_xfer_done = dwc2_update_urb_state(hsotg, chan, chnum, urb,
 						      qtd);
 		if (urb_xfer_done) {
-			dwc2_host_complete(hsotg, urb->priv, urb, urb->status);
+			dwc2_host_complete(hsotg, qtd, urb->status);
 			halt_status = DWC2_HC_XFER_URB_COMPLETE;
 		} else {
 			halt_status = DWC2_HC_XFER_COMPLETE;
@@ -1073,11 +1065,10 @@ static void dwc2_hc_xfercomp_intr(struct dwc2_hsotg *hsotg,
 		 * interrupt
 		 */
 		if (urb_xfer_done) {
-				dwc2_host_complete(hsotg, urb->priv, urb,
-						   urb->status);
-				halt_status = DWC2_HC_XFER_URB_COMPLETE;
+			dwc2_host_complete(hsotg, qtd, urb->status);
+			halt_status = DWC2_HC_XFER_URB_COMPLETE;
 		} else {
-				halt_status = DWC2_HC_XFER_COMPLETE;
+			halt_status = DWC2_HC_XFER_COMPLETE;
 		}
 
 		dwc2_hcd_save_data_toggle(hsotg, chan, chnum, qtd);
@@ -1123,11 +1114,11 @@ static void dwc2_hc_stall_intr(struct dwc2_hsotg *hsotg,
 		goto handle_stall_halt;
 
 	if (pipe_type == USB_ENDPOINT_XFER_CONTROL)
-		dwc2_host_complete(hsotg, urb->priv, urb, -EPIPE);
+		dwc2_host_complete(hsotg, qtd, -EPIPE);
 
 	if (pipe_type == USB_ENDPOINT_XFER_BULK ||
 	    pipe_type == USB_ENDPOINT_XFER_INT) {
-		dwc2_host_complete(hsotg, urb->priv, urb, -EPIPE);
+		dwc2_host_complete(hsotg, qtd, -EPIPE);
 		/*
 		 * USB protocol requires resetting the data toggle for bulk
 		 * and interrupt endpoints when a CLEAR_FEATURE(ENDPOINT_HALT)
@@ -1372,10 +1363,10 @@ static void dwc2_hc_nyet_intr(struct dwc2_hsotg *hsotg,
 		    hsotg->core_params->dma_enable > 0) {
 			qtd->complete_split = 0;
 			qtd->isoc_split_offset = 0;
+			qtd->isoc_frame_index++;
 			if (qtd->urb &&
-			    ++qtd->isoc_frame_index == qtd->urb->packet_count) {
-				dwc2_host_complete(hsotg, qtd->urb->priv,
-						   qtd->urb, 0);
+			    qtd->isoc_frame_index == qtd->urb->packet_count) {
+				dwc2_host_complete(hsotg, qtd, 0);
 				dwc2_release_channel(hsotg, chan, qtd,
 						     DWC2_HC_XFER_URB_COMPLETE);
 			} else {
@@ -1445,16 +1436,16 @@ static void dwc2_hc_babble_intr(struct dwc2_hsotg *hsotg,
 	dev_dbg(hsotg->dev, "--Host Channel %d Interrupt: Babble Error--\n",
 		chnum);
 
+	dwc2_hc_handle_tt_clear(hsotg, chan, qtd);
+
 	if (hsotg->core_params->dma_desc_enable > 0) {
 		dwc2_hcd_complete_xfer_ddma(hsotg, chan, chnum,
 					    DWC2_HC_XFER_BABBLE_ERR);
-		goto handle_babble_done;
+		goto disable_int;
 	}
 
 	if (chan->ep_type != USB_ENDPOINT_XFER_ISOC) {
-		if (qtd->urb)
-			dwc2_host_complete(hsotg, qtd->urb->priv, qtd->urb,
-					   -EOVERFLOW);
+		dwc2_host_complete(hsotg, qtd, -EOVERFLOW);
 		dwc2_halt_channel(hsotg, chan, qtd, DWC2_HC_XFER_BABBLE_ERR);
 	} else {
 		enum dwc2_halt_status halt_status;
@@ -1464,8 +1455,7 @@ static void dwc2_hc_babble_intr(struct dwc2_hsotg *hsotg,
 		dwc2_halt_channel(hsotg, chan, qtd, halt_status);
 	}
 
-handle_babble_done:
-	dwc2_hc_handle_tt_clear(hsotg, chan, qtd);
+disable_int:
 	disable_hc_int(hsotg, chnum, HCINTMSK_BBLERR);
 }
 
@@ -1490,6 +1480,8 @@ static void dwc2_hc_ahberr_intr(struct dwc2_hsotg *hsotg,
 	if (!urb)
 		goto handle_ahberr_halt;
 
+	dwc2_hc_handle_tt_clear(hsotg, chan, qtd);
+
 	hcchar = readl(hsotg->regs + HCCHAR(chnum));
 	hcsplt = readl(hsotg->regs + HCSPLT(chnum));
 	hctsiz = readl(hsotg->regs + HCTSIZ(chnum));
@@ -1557,7 +1549,7 @@ static void dwc2_hc_ahberr_intr(struct dwc2_hsotg *hsotg,
 		goto handle_ahberr_done;
 	}
 
-	dwc2_host_complete(hsotg, urb->priv, urb, -EIO);
+	dwc2_host_complete(hsotg, qtd, -EIO);
 
 handle_ahberr_halt:
 	/*
@@ -1567,7 +1559,6 @@ handle_ahberr_halt:
 	dwc2_hc_halt(hsotg, chan, DWC2_HC_XFER_AHB_ERR);
 
 handle_ahberr_done:
-	dwc2_hc_handle_tt_clear(hsotg, chan, qtd);
 	disable_hc_int(hsotg, chnum, HCINTMSK_AHBERR);
 }
 
@@ -1582,6 +1573,8 @@ static void dwc2_hc_xacterr_intr(struct dwc2_hsotg *hsotg,
 	dev_dbg(hsotg->dev,
 		"--Host Channel %d Interrupt: Transaction Error--\n", chnum);
 
+	dwc2_hc_handle_tt_clear(hsotg, chan, qtd);
+
 	if (hsotg->core_params->dma_desc_enable > 0) {
 		dwc2_hcd_complete_xfer_ddma(hsotg, chan, chnum,
 					    DWC2_HC_XFER_XACT_ERR);
@@ -1625,7 +1618,6 @@ static void dwc2_hc_xacterr_intr(struct dwc2_hsotg *hsotg,
 	}
 
 handle_xacterr_done:
-	dwc2_hc_handle_tt_clear(hsotg, chan, qtd);
 	disable_hc_int(hsotg, chnum, HCINTMSK_XACTERR);
 }
 
@@ -1643,6 +1635,8 @@ static void dwc2_hc_frmovrun_intr(struct dwc2_hsotg *hsotg,
 		dev_dbg(hsotg->dev, "--Host Channel %d Interrupt: Frame Overrun--\n",
 			chnum);
 
+	dwc2_hc_handle_tt_clear(hsotg, chan, qtd);
+
 	switch (dwc2_hcd_get_pipe_type(&qtd->urb->pipe_info)) {
 	case USB_ENDPOINT_XFER_CONTROL:
 	case USB_ENDPOINT_XFER_BULK:
@@ -1657,7 +1651,6 @@ static void dwc2_hc_frmovrun_intr(struct dwc2_hsotg *hsotg,
 		break;
 	}
 
-	dwc2_hc_handle_tt_clear(hsotg, chan, qtd);
 	disable_hc_int(hsotg, chnum, HCINTMSK_FRMOVRUN);
 }