From e66eaacae2bfb8117e941880297f5f8657425ae2 Mon Sep 17 00:00:00 2001 From: P33M Date: Thu, 18 Jul 2013 16:32:41 +0100 Subject: [PATCH 1/2] dwc_otg: whitespace cleanup in dwc_otg_urb_enqueue --- drivers/usb/host/dwc_otg/dwc_otg_hcd_linux.c | 59 ++++++++++---------- 1 file changed, 29 insertions(+), 30 deletions(-) diff --git a/drivers/usb/host/dwc_otg/dwc_otg_hcd_linux.c b/drivers/usb/host/dwc_otg/dwc_otg_hcd_linux.c index bb3ec6869b4bcc..80d3d791984df9 100644 --- a/drivers/usb/host/dwc_otg/dwc_otg_hcd_linux.c +++ b/drivers/usb/host/dwc_otg/dwc_otg_hcd_linux.c @@ -733,10 +733,10 @@ static int dwc_otg_urb_enqueue(struct usb_hcd *hcd, if(dwc_otg_urb == NULL) return -ENOMEM; - urb->hcpriv = dwc_otg_urb; - if (!dwc_otg_urb && urb->number_of_packets) - return -ENOMEM; - + urb->hcpriv = dwc_otg_urb; + if (!dwc_otg_urb && urb->number_of_packets) + return -ENOMEM; + dwc_otg_hcd_urb_set_pipeinfo(dwc_otg_urb, usb_pipedevice(urb->pipe), usb_pipeendpoint(urb->pipe), ep_type, usb_pipein(urb->pipe), @@ -776,36 +776,35 @@ static int dwc_otg_urb_enqueue(struct usb_hcd *hcd, } #if USB_URB_EP_LINKING - DWC_SPINLOCK_IRQSAVE(dwc_otg_hcd->lock, &irqflags); + DWC_SPINLOCK_IRQSAVE(dwc_otg_hcd->lock, &irqflags); retval = usb_hcd_link_urb_to_ep(hcd, urb); - DWC_SPINUNLOCK_IRQRESTORE(dwc_otg_hcd->lock, irqflags); - if (0 == retval) + DWC_SPINUNLOCK_IRQRESTORE(dwc_otg_hcd->lock, irqflags); + if (0 == retval) #endif - { - retval = dwc_otg_hcd_urb_enqueue(dwc_otg_hcd, dwc_otg_urb, - /*(dwc_otg_qh_t **)*/ - ref_ep_hcpriv, - mem_flags == GFP_ATOMIC ? 1 : 0); - if (0 == retval) { - if (alloc_bandwidth) { - allocate_bus_bandwidth(hcd, - dwc_otg_hcd_get_ep_bandwidth( - dwc_otg_hcd, *ref_ep_hcpriv), - urb); - } - } else { + { + retval = dwc_otg_hcd_urb_enqueue(dwc_otg_hcd, dwc_otg_urb, + /*(dwc_otg_qh_t **)*/ + ref_ep_hcpriv, + mem_flags == GFP_ATOMIC ? 1 : 0); + if (0 == retval) { + if (alloc_bandwidth) { + allocate_bus_bandwidth(hcd, + dwc_otg_hcd_get_ep_bandwidth( + dwc_otg_hcd, *ref_ep_hcpriv), + urb); + } + } else { #if USB_URB_EP_LINKING - dwc_irqflags_t irqflags; - DWC_DEBUGPL(DBG_HCD, "DWC OTG dwc_otg_hcd_urb_enqueue failed rc %d\n", retval); - DWC_SPINLOCK_IRQSAVE(dwc_otg_hcd->lock, &irqflags); - usb_hcd_unlink_urb_from_ep(hcd, urb); - DWC_SPINUNLOCK_IRQRESTORE(dwc_otg_hcd->lock, irqflags); + dwc_irqflags_t irqflags; + DWC_DEBUGPL(DBG_HCD, "DWC OTG dwc_otg_hcd_urb_enqueue failed rc %d\n", retval); + DWC_SPINLOCK_IRQSAVE(dwc_otg_hcd->lock, &irqflags); + usb_hcd_unlink_urb_from_ep(hcd, urb); + DWC_SPINUNLOCK_IRQRESTORE(dwc_otg_hcd->lock, irqflags); #endif - if (retval == -DWC_E_NO_DEVICE) { - retval = -ENODEV; - } - } - } + if (retval == -DWC_E_NO_DEVICE) + retval = -ENODEV; + } + } return retval; } From eb1b482a93dffb390117f58c47b9fc0e7a3cedd7 Mon Sep 17 00:00:00 2001 From: P33M Date: Thu, 18 Jul 2013 17:07:26 +0100 Subject: [PATCH 2/2] dwc_otg: prevent OOPSes during device disconnects The dwc_otg_urb_enqueue function is thread-unsafe. In particular the access of urb->hcpriv, usb_hcd_link_urb_to_ep, dwc_otg_urb->qtd and friends does not occur within a critical section and so if a device was unplugged during activity there was a high chance that the usbcore hub_thread would try to disable the endpoint with partially- formed entries in the URB queue. This would result in BUG() or null pointer dereferences. Fix so that access of urb->hcpriv, enqueuing to the hardware and adding to usbcore endpoint URB lists is contained within a single critical section. --- drivers/usb/host/dwc_otg/dwc_otg_hcd.c | 3 --- drivers/usb/host/dwc_otg/dwc_otg_hcd_linux.c | 14 +++++--------- drivers/usb/host/dwc_otg/dwc_otg_hcd_queue.c | 6 +----- 3 files changed, 6 insertions(+), 17 deletions(-) diff --git a/drivers/usb/host/dwc_otg/dwc_otg_hcd.c b/drivers/usb/host/dwc_otg/dwc_otg_hcd.c index af9108c013928f..a1970dcd08e728 100644 --- a/drivers/usb/host/dwc_otg/dwc_otg_hcd.c +++ b/drivers/usb/host/dwc_otg/dwc_otg_hcd.c @@ -464,7 +464,6 @@ int dwc_otg_hcd_urb_enqueue(dwc_otg_hcd_t * hcd, dwc_otg_hcd_urb_t * dwc_otg_urb, void **ep_handle, int atomic_alloc) { - dwc_irqflags_t flags; int retval = 0; uint8_t needs_scheduling = 0; dwc_otg_transaction_type_e tr_type; @@ -515,12 +514,10 @@ int dwc_otg_hcd_urb_enqueue(dwc_otg_hcd_t * hcd, } if(needs_scheduling) { - DWC_SPINLOCK_IRQSAVE(hcd->lock, &flags); tr_type = dwc_otg_hcd_select_transactions(hcd); if (tr_type != DWC_OTG_TRANSACTION_NONE) { dwc_otg_hcd_queue_transactions(hcd, tr_type); } - DWC_SPINUNLOCK_IRQRESTORE(hcd->lock, flags); } return retval; } diff --git a/drivers/usb/host/dwc_otg/dwc_otg_hcd_linux.c b/drivers/usb/host/dwc_otg/dwc_otg_hcd_linux.c index 80d3d791984df9..4c51c984bc1d41 100644 --- a/drivers/usb/host/dwc_otg/dwc_otg_hcd_linux.c +++ b/drivers/usb/host/dwc_otg/dwc_otg_hcd_linux.c @@ -679,9 +679,7 @@ static int dwc_otg_urb_enqueue(struct usb_hcd *hcd, #if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,28) struct usb_host_endpoint *ep = urb->ep; #endif -#if USB_URB_EP_LINKING dwc_irqflags_t irqflags; -#endif void **ref_ep_hcpriv = &ep->hcpriv; dwc_otg_hcd_t *dwc_otg_hcd = hcd_to_dwc_otg_hcd(hcd); dwc_otg_hcd_urb_t *dwc_otg_urb; @@ -733,7 +731,6 @@ static int dwc_otg_urb_enqueue(struct usb_hcd *hcd, if(dwc_otg_urb == NULL) return -ENOMEM; - urb->hcpriv = dwc_otg_urb; if (!dwc_otg_urb && urb->number_of_packets) return -ENOMEM; @@ -775,10 +772,10 @@ static int dwc_otg_urb_enqueue(struct usb_hcd *hcd, iso_frame_desc[i].length); } -#if USB_URB_EP_LINKING DWC_SPINLOCK_IRQSAVE(dwc_otg_hcd->lock, &irqflags); + urb->hcpriv = dwc_otg_urb; +#if USB_URB_EP_LINKING retval = usb_hcd_link_urb_to_ep(hcd, urb); - DWC_SPINUNLOCK_IRQRESTORE(dwc_otg_hcd->lock, irqflags); if (0 == retval) #endif { @@ -794,17 +791,16 @@ static int dwc_otg_urb_enqueue(struct usb_hcd *hcd, urb); } } else { -#if USB_URB_EP_LINKING - dwc_irqflags_t irqflags; DWC_DEBUGPL(DBG_HCD, "DWC OTG dwc_otg_hcd_urb_enqueue failed rc %d\n", retval); - DWC_SPINLOCK_IRQSAVE(dwc_otg_hcd->lock, &irqflags); +#if USB_URB_EP_LINKING usb_hcd_unlink_urb_from_ep(hcd, urb); - DWC_SPINUNLOCK_IRQRESTORE(dwc_otg_hcd->lock, irqflags); #endif + urb->hcpriv = NULL; if (retval == -DWC_E_NO_DEVICE) retval = -ENODEV; } } + DWC_SPINUNLOCK_IRQRESTORE(dwc_otg_hcd->lock, irqflags); return retval; } diff --git a/drivers/usb/host/dwc_otg/dwc_otg_hcd_queue.c b/drivers/usb/host/dwc_otg/dwc_otg_hcd_queue.c index 81253076350d3d..5aed4162bcfb4c 100644 --- a/drivers/usb/host/dwc_otg/dwc_otg_hcd_queue.c +++ b/drivers/usb/host/dwc_otg/dwc_otg_hcd_queue.c @@ -919,6 +919,7 @@ void dwc_otg_hcd_qtd_init(dwc_otg_qtd_t * qtd, dwc_otg_hcd_urb_t * urb) * QH to place the QTD into. If it does not find a QH, then it will create a * new QH. If the QH to which the QTD is added is not currently scheduled, it * is placed into the proper schedule based on its EP type. + * HCD lock must be held and interrupts must be disabled on entry * * @param[in] qtd The QTD to add * @param[in] hcd The DWC HCD structure @@ -931,8 +932,6 @@ int dwc_otg_hcd_qtd_add(dwc_otg_qtd_t * qtd, dwc_otg_hcd_t * hcd, dwc_otg_qh_t ** qh, int atomic_alloc) { int retval = 0; - dwc_irqflags_t flags; - dwc_otg_hcd_urb_t *urb = qtd->urb; /* @@ -946,15 +945,12 @@ int dwc_otg_hcd_qtd_add(dwc_otg_qtd_t * qtd, goto done; } } - DWC_SPINLOCK_IRQSAVE(hcd->lock, &flags); retval = dwc_otg_hcd_qh_add(hcd, *qh); if (retval == 0) { DWC_CIRCLEQ_INSERT_TAIL(&((*qh)->qtd_list), qtd, qtd_list_entry); qtd->qh = *qh; } - DWC_SPINUNLOCK_IRQRESTORE(hcd->lock, flags); - done: return retval;