From 29c1ed6b6e31176eee794552a8a99432e31dc62c Mon Sep 17 00:00:00 2001 From: P33M Date: Fri, 12 May 2017 12:24:00 +0100 Subject: [PATCH 1/3] dwc_otg: fix several potential crash sources On root port disconnect events, the host driver state is cleared and in-progress host channels are forcibly stopped. This doesn't play well with the FIQ running in the background, so: - Guard the disconnect callback with both the host spinlock and FIQ spinlock - Move qtd dereference in dwc_otg_handle_hc_fsm() after the early-out so we don't dereference a qtd that has gone away - Turn catch-all BUG()s in dwc_otg_handle_hc_fsm() into warnings. --- drivers/usb/host/dwc_otg/dwc_otg_cil_intr.c | 2 ++ drivers/usb/host/dwc_otg/dwc_otg_hcd.c | 27 ++++++++++++++--- drivers/usb/host/dwc_otg/dwc_otg_hcd_intr.c | 33 ++++++++++++++------- 3 files changed, 47 insertions(+), 15 deletions(-) diff --git a/drivers/usb/host/dwc_otg/dwc_otg_cil_intr.c b/drivers/usb/host/dwc_otg/dwc_otg_cil_intr.c index 96c76e38cd372b..9fb7229f43ae45 100644 --- a/drivers/usb/host/dwc_otg/dwc_otg_cil_intr.c +++ b/drivers/usb/host/dwc_otg/dwc_otg_cil_intr.c @@ -973,7 +973,9 @@ int32_t dwc_otg_handle_disconnect_intr(dwc_otg_core_if_t * core_if) } else { if (core_if->op_state == A_HOST) { /* A-Cable still connected but device disconnected. */ + DWC_SPINUNLOCK(core_if->lock); cil_hcd_disconnect(core_if); + DWC_SPINLOCK(core_if->lock); if (core_if->adp_enable) { gpwrdn_data_t gpwrdn = { .d32 = 0 }; cil_hcd_stop(core_if); diff --git a/drivers/usb/host/dwc_otg/dwc_otg_hcd.c b/drivers/usb/host/dwc_otg/dwc_otg_hcd.c index 37c9d7d38287b6..9a647b75027378 100644 --- a/drivers/usb/host/dwc_otg/dwc_otg_hcd.c +++ b/drivers/usb/host/dwc_otg/dwc_otg_hcd.c @@ -290,13 +290,16 @@ static int32_t dwc_otg_hcd_disconnect_cb(void *p) gintsts_data_t intr; dwc_otg_hcd_t *dwc_otg_hcd = p; + DWC_SPINLOCK(dwc_otg_hcd->lock); /* * Set status flags for the hub driver. */ dwc_otg_hcd->flags.b.port_connect_status_change = 1; dwc_otg_hcd->flags.b.port_connect_status = 0; - if(fiq_enable) + if(fiq_enable) { local_fiq_disable(); + fiq_fsm_spin_lock(&dwc_otg_hcd->fiq_state->lock); + } /* * Shutdown any transfers in process by clearing the Tx FIFO Empty * interrupt mask and status bits and disabling subsequent host @@ -389,7 +392,8 @@ static int32_t dwc_otg_hcd_disconnect_cb(void *p) * in release_channel_ddma(). Which called from ep_disable * when device disconnect. */ - channel->qh = NULL; + if (dwc_otg_hcd->core_if->dma_desc_enable) + channel->qh = NULL; } } if(fiq_fsm_enable) { @@ -400,13 +404,16 @@ static int32_t dwc_otg_hcd_disconnect_cb(void *p) } - if(fiq_enable) + if(fiq_enable) { + fiq_fsm_spin_unlock(&dwc_otg_hcd->fiq_state->lock); local_fiq_enable(); + } if (dwc_otg_hcd->fops->disconnect) { dwc_otg_hcd->fops->disconnect(dwc_otg_hcd); } + DWC_SPINUNLOCK(dwc_otg_hcd->lock); return 1; } @@ -1750,8 +1757,20 @@ int fiq_fsm_queue_split_transaction(dwc_otg_hcd_t *hcd, dwc_otg_qh_t *qh) int hub_addr, port_addr, frame, uframe; struct fiq_channel_state *st = &hcd->fiq_state->channel[hc->hc_num]; - if (st->fsm != FIQ_PASSTHROUGH) + /* + * Non-periodic channel assignments stay in the non_periodic_active queue. + * Therefore we get repeatedly called until the FIQ's done processing this channel. + */ + if (qh->channel->xfer_started == 1) return 0; + + if (st->fsm != FIQ_PASSTHROUGH) { + pr_warn_ratelimited("%s:%d: Queue called for an active channel\n", __func__, __LINE__); + return 0; + } + + qh->channel->xfer_started = 1; + st->nr_errors = 0; st->hcchar_copy.d32 = 0; diff --git a/drivers/usb/host/dwc_otg/dwc_otg_hcd_intr.c b/drivers/usb/host/dwc_otg/dwc_otg_hcd_intr.c index 718a1accc0c219..cf23baaa388562 100644 --- a/drivers/usb/host/dwc_otg/dwc_otg_hcd_intr.c +++ b/drivers/usb/host/dwc_otg/dwc_otg_hcd_intr.c @@ -2374,8 +2374,7 @@ void dwc_otg_hcd_handle_hc_fsm(dwc_otg_hcd_t *hcd, uint32_t num) { struct fiq_channel_state *st = &hcd->fiq_state->channel[num]; dwc_hc_t *hc = hcd->hc_ptr_array[num]; - dwc_otg_qtd_t *qtd = DWC_CIRCLEQ_FIRST(&hc->qh->qtd_list); - dwc_otg_qh_t *qh = hc->qh; + dwc_otg_qtd_t *qtd; dwc_otg_hc_regs_t *hc_regs = hcd->core_if->host_if->hc_regs[num]; hcint_data_t hcint = hcd->fiq_state->channel[num].hcint_copy; hctsiz_data_t hctsiz = hcd->fiq_state->channel[num].hctsiz_copy; @@ -2385,16 +2384,19 @@ void dwc_otg_hcd_handle_hc_fsm(dwc_otg_hcd_t *hcd, uint32_t num) hostchannels = hcd->available_host_channels; if (hc->halt_pending) { /* Dequeue: The FIQ was allowed to complete the transfer but state has been cleared. */ - if (st->fsm == FIQ_NP_SPLIT_DONE && hcint.b.xfercomp && qh->ep_type == UE_BULK) { + if (hc->qh && st->fsm == FIQ_NP_SPLIT_DONE && + hcint.b.xfercomp && hc->qh->ep_type == UE_BULK) { if (hctsiz.b.pid == DWC_HCTSIZ_DATA0) { - qh->data_toggle = DWC_OTG_HC_PID_DATA1; + hc->qh->data_toggle = DWC_OTG_HC_PID_DATA1; } else { - qh->data_toggle = DWC_OTG_HC_PID_DATA0; + hc->qh->data_toggle = DWC_OTG_HC_PID_DATA0; } } release_channel(hcd, hc, NULL, hc->halt_status); return; } + + qtd = DWC_CIRCLEQ_FIRST(&hc->qh->qtd_list); switch (st->fsm) { case FIQ_TEST: break; @@ -2413,6 +2415,11 @@ void dwc_otg_hcd_handle_hc_fsm(dwc_otg_hcd_t *hcd, uint32_t num) handle_hc_xfercomp_intr(hcd, hc, hc_regs, qtd); } else if (hcint.b.nak) { handle_hc_nak_intr(hcd, hc, hc_regs, qtd); + } else { + DWC_WARN("Unexpected IRQ state on FSM transaction:" + "dev_addr=%d ep=%d fsm=%d, hcint=0x%08x\n", + hc->dev_addr, hc->ep_num, st->fsm, hcint.d32); + release_channel(hcd, hc, qtd, DWC_OTG_HC_XFER_NO_HALT_STATUS); } break; @@ -2428,8 +2435,10 @@ void dwc_otg_hcd_handle_hc_fsm(dwc_otg_hcd_t *hcd, uint32_t num) } else if (hcint.b.ahberr) { handle_hc_ahberr_intr(hcd, hc, hc_regs, qtd); } else { - local_fiq_disable(); - BUG(); + DWC_WARN("Unexpected IRQ state on FSM transaction:" + "dev_addr=%d ep=%d fsm=%d, hcint=0x%08x\n", + hc->dev_addr, hc->ep_num, st->fsm, hcint.d32); + release_channel(hcd, hc, qtd, DWC_OTG_HC_XFER_NO_HALT_STATUS); } break; @@ -2445,8 +2454,10 @@ void dwc_otg_hcd_handle_hc_fsm(dwc_otg_hcd_t *hcd, uint32_t num) } else if (hcint.b.ahberr) { handle_hc_ahberr_intr(hcd, hc, hc_regs, qtd); } else { - local_fiq_disable(); - BUG(); + DWC_WARN("Unexpected IRQ state on FSM transaction:" + "dev_addr=%d ep=%d fsm=%d, hcint=0x%08x\n", + hc->dev_addr, hc->ep_num, st->fsm, hcint.d32); + release_channel(hcd, hc, qtd, DWC_OTG_HC_XFER_NO_HALT_STATUS); } break; @@ -2504,7 +2515,7 @@ void dwc_otg_hcd_handle_hc_fsm(dwc_otg_hcd_t *hcd, uint32_t num) } else { frame_desc->status = 0; /* Unswizzle dma */ - len = dwc_otg_fiq_unsetup_per_dma(hcd, qh, qtd, num); + len = dwc_otg_fiq_unsetup_per_dma(hcd, hc->qh, qtd, num); frame_desc->actual_length = len; } qtd->isoc_frame_index++; @@ -2566,7 +2577,7 @@ void dwc_otg_hcd_handle_hc_fsm(dwc_otg_hcd_t *hcd, uint32_t num) * The status is recorded as the interrupt state should the transaction * fail. */ - dwc_otg_fiq_unmangle_isoc(hcd, qh, qtd, num); + dwc_otg_fiq_unmangle_isoc(hcd, hc->qh, qtd, num); hcd->fops->complete(hcd, qtd->urb->priv, qtd->urb, 0); release_channel(hcd, hc, qtd, DWC_OTG_HC_XFER_URB_COMPLETE); break; From 46fe230ddd4d95a02d2583809c24cab755e16475 Mon Sep 17 00:00:00 2001 From: P33M Date: Mon, 15 May 2017 14:27:48 +0100 Subject: [PATCH 2/3] dwc_otg: delete hcd->channel_lock The lock serves no purpose as it is only held while the HCD spinlock is already being held. --- drivers/usb/host/dwc_otg/dwc_otg_hcd.c | 15 --------------- drivers/usb/host/dwc_otg/dwc_otg_hcd.h | 1 - drivers/usb/host/dwc_otg/dwc_otg_hcd_ddma.c | 5 ----- drivers/usb/host/dwc_otg/dwc_otg_hcd_intr.c | 4 ---- 4 files changed, 25 deletions(-) diff --git a/drivers/usb/host/dwc_otg/dwc_otg_hcd.c b/drivers/usb/host/dwc_otg/dwc_otg_hcd.c index 9a647b75027378..5ec991624c7865 100644 --- a/drivers/usb/host/dwc_otg/dwc_otg_hcd.c +++ b/drivers/usb/host/dwc_otg/dwc_otg_hcd.c @@ -938,7 +938,6 @@ static void dwc_otg_hcd_free(dwc_otg_hcd_t * dwc_otg_hcd) } else if (dwc_otg_hcd->status_buf != NULL) { DWC_FREE(dwc_otg_hcd->status_buf); } - DWC_SPINLOCK_FREE(dwc_otg_hcd->channel_lock); DWC_SPINLOCK_FREE(dwc_otg_hcd->lock); /* Set core_if's lock pointer to NULL */ dwc_otg_hcd->core_if->lock = NULL; @@ -969,10 +968,8 @@ int dwc_otg_hcd_init(dwc_otg_hcd_t * hcd, dwc_otg_core_if_t * core_if) #if (defined(DWC_LINUX) && defined(CONFIG_DEBUG_SPINLOCK)) DWC_SPINLOCK_ALLOC_LINUX_DEBUG(hcd->lock); - DWC_SPINLOCK_ALLOC_LINUX_DEBUG(hcd->channel_lock); #else hcd->lock = DWC_SPINLOCK_ALLOC(); - hcd->channel_lock = DWC_SPINLOCK_ALLOC(); #endif DWC_DEBUGPL(DBG_HCDV, "init of HCD %p given core_if %p\n", hcd, core_if); @@ -1997,7 +1994,6 @@ dwc_otg_transaction_type_e dwc_otg_hcd_select_transactions(dwc_otg_hcd_t * hcd) dwc_otg_qh_t *qh; int num_channels; dwc_irqflags_t flags; - dwc_spinlock_t *channel_lock = hcd->channel_lock; dwc_otg_transaction_type_e ret_val = DWC_OTG_TRANSACTION_NONE; #ifdef DEBUG_HOST_CHANNELS @@ -2016,13 +2012,10 @@ dwc_otg_transaction_type_e dwc_otg_hcd_select_transactions(dwc_otg_hcd_t * hcd) if (microframe_schedule) { // Make sure we leave one channel for non periodic transactions. - DWC_SPINLOCK_IRQSAVE(channel_lock, &flags); if (hcd->available_host_channels <= 1) { - DWC_SPINUNLOCK_IRQRESTORE(channel_lock, flags); break; } hcd->available_host_channels--; - DWC_SPINUNLOCK_IRQRESTORE(channel_lock, flags); #ifdef DEBUG_HOST_CHANNELS last_sel_trans_num_per_scheduled++; #endif /* DEBUG_HOST_CHANNELS */ @@ -2035,10 +2028,8 @@ dwc_otg_transaction_type_e dwc_otg_hcd_select_transactions(dwc_otg_hcd_t * hcd) * periodic assigned schedule. */ qh_ptr = DWC_LIST_NEXT(qh_ptr); - DWC_SPINLOCK_IRQSAVE(channel_lock, &flags); DWC_LIST_MOVE_HEAD(&hcd->periodic_sched_assigned, &qh->qh_list_entry); - DWC_SPINUNLOCK_IRQRESTORE(channel_lock, flags); } /* @@ -2076,13 +2067,10 @@ dwc_otg_transaction_type_e dwc_otg_hcd_select_transactions(dwc_otg_hcd_t * hcd) } if (microframe_schedule) { - DWC_SPINLOCK_IRQSAVE(channel_lock, &flags); if (hcd->available_host_channels < 1) { - DWC_SPINUNLOCK_IRQRESTORE(channel_lock, flags); break; } hcd->available_host_channels--; - DWC_SPINUNLOCK_IRQRESTORE(channel_lock, flags); #ifdef DEBUG_HOST_CHANNELS last_sel_trans_num_nonper_scheduled++; #endif /* DEBUG_HOST_CHANNELS */ @@ -2095,11 +2083,8 @@ dwc_otg_transaction_type_e dwc_otg_hcd_select_transactions(dwc_otg_hcd_t * hcd) * non-periodic active schedule. */ qh_ptr = DWC_LIST_NEXT(qh_ptr); - DWC_SPINLOCK_IRQSAVE(channel_lock, &flags); DWC_LIST_MOVE_HEAD(&hcd->non_periodic_sched_active, &qh->qh_list_entry); - DWC_SPINUNLOCK_IRQRESTORE(channel_lock, flags); - if (!microframe_schedule) hcd->non_periodic_channels++; diff --git a/drivers/usb/host/dwc_otg/dwc_otg_hcd.h b/drivers/usb/host/dwc_otg/dwc_otg_hcd.h index 4539cd7b802d3e..7f7e9eaffd6a3c 100644 --- a/drivers/usb/host/dwc_otg/dwc_otg_hcd.h +++ b/drivers/usb/host/dwc_otg/dwc_otg_hcd.h @@ -568,7 +568,6 @@ struct dwc_otg_hcd { /* */ dwc_spinlock_t *lock; - dwc_spinlock_t *channel_lock; /** * Private data that could be used by OS wrapper. */ diff --git a/drivers/usb/host/dwc_otg/dwc_otg_hcd_ddma.c b/drivers/usb/host/dwc_otg/dwc_otg_hcd_ddma.c index 126e99ab2fc43f..bd8a2040371344 100644 --- a/drivers/usb/host/dwc_otg/dwc_otg_hcd_ddma.c +++ b/drivers/usb/host/dwc_otg/dwc_otg_hcd_ddma.c @@ -279,17 +279,12 @@ void dump_frame_list(dwc_otg_hcd_t * hcd) static void release_channel_ddma(dwc_otg_hcd_t * hcd, dwc_otg_qh_t * qh) { - dwc_irqflags_t flags; - dwc_spinlock_t *channel_lock = hcd->channel_lock; - dwc_hc_t *hc = qh->channel; if (dwc_qh_is_non_per(qh)) { - DWC_SPINLOCK_IRQSAVE(channel_lock, &flags); if (!microframe_schedule) hcd->non_periodic_channels--; else hcd->available_host_channels++; - DWC_SPINUNLOCK_IRQRESTORE(channel_lock, flags); } else update_frame_list(hcd, qh, 0); diff --git a/drivers/usb/host/dwc_otg/dwc_otg_hcd_intr.c b/drivers/usb/host/dwc_otg/dwc_otg_hcd_intr.c index cf23baaa388562..a4355afc77b687 100644 --- a/drivers/usb/host/dwc_otg/dwc_otg_hcd_intr.c +++ b/drivers/usb/host/dwc_otg/dwc_otg_hcd_intr.c @@ -945,7 +945,6 @@ static void release_channel(dwc_otg_hcd_t * hcd, dwc_otg_transaction_type_e tr_type; int free_qtd; dwc_irqflags_t flags; - dwc_spinlock_t *channel_lock = hcd->channel_lock; int hog_port = 0; @@ -1034,11 +1033,8 @@ static void release_channel(dwc_otg_hcd_t * hcd, break; } } else { - - DWC_SPINLOCK_IRQSAVE(channel_lock, &flags); hcd->available_host_channels++; fiq_print(FIQDBG_INT, hcd->fiq_state, "AHC = %d ", hcd->available_host_channels); - DWC_SPINUNLOCK_IRQRESTORE(channel_lock, flags); } /* Try to queue more transfers now that there's a free channel. */ From 9df0bc48f7dc6b256990d261338a440f5cf72903 Mon Sep 17 00:00:00 2001 From: P33M Date: Mon, 15 May 2017 14:51:42 +0100 Subject: [PATCH 3/3] dwc_otg: remove unnecessary dma-mode channel halts on disconnect interrupt Host channels are already halted in kill_urbs_in_qh_list() with the subsequent interrupt processing behaving as if the URB was dequeued via HCD callback. There's no need to clobber the host channel registers a second time as this exposes races between the driver and host channel resulting in hcd->free_hc_list becoming corrupted. --- drivers/usb/host/dwc_otg/dwc_otg_hcd.c | 30 -------------------------- 1 file changed, 30 deletions(-) diff --git a/drivers/usb/host/dwc_otg/dwc_otg_hcd.c b/drivers/usb/host/dwc_otg/dwc_otg_hcd.c index 5ec991624c7865..a2dc6337836b27 100644 --- a/drivers/usb/host/dwc_otg/dwc_otg_hcd.c +++ b/drivers/usb/host/dwc_otg/dwc_otg_hcd.c @@ -200,8 +200,6 @@ static void kill_urbs_in_qh_list(dwc_otg_hcd_t * hcd, dwc_list_link_t * qh_list) dwc_otg_hc_halt(hcd->core_if, qh->channel, DWC_OTG_HC_XFER_URB_DEQUEUE); } - if(microframe_schedule) - hcd->available_host_channels++; qh->channel = NULL; } dwc_otg_hcd_qh_remove(hcd, qh); @@ -369,39 +367,11 @@ static int32_t dwc_otg_hcd_disconnect_cb(void *p) } } - for (i = 0; i < num_channels; i++) { - channel = dwc_otg_hcd->hc_ptr_array[i]; - if (DWC_CIRCLEQ_EMPTY_ENTRY(channel, hc_list_entry)) { - hc_regs = - dwc_otg_hcd->core_if->host_if->hc_regs[i]; - hcchar.d32 = DWC_READ_REG32(&hc_regs->hcchar); - if (hcchar.b.chen) { - /* Halt the channel. */ - hcchar.b.chdis = 1; - DWC_WRITE_REG32(&hc_regs->hcchar, - hcchar.d32); - } - - dwc_otg_hc_cleanup(dwc_otg_hcd->core_if, - channel); - DWC_CIRCLEQ_INSERT_TAIL - (&dwc_otg_hcd->free_hc_list, channel, - hc_list_entry); - /* - * Added for Descriptor DMA to prevent channel double cleanup - * in release_channel_ddma(). Which called from ep_disable - * when device disconnect. - */ - if (dwc_otg_hcd->core_if->dma_desc_enable) - channel->qh = NULL; - } - } if(fiq_fsm_enable) { for(i=0; i < 128; i++) { dwc_otg_hcd->hub_port[i] = 0; } } - } if(fiq_enable) {