| Message ID | 20180615220131.65402-3-dmitry.torokhov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Fri, Jun 15, 2018 at 3:01 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > When we are ready to retry the delayed QH, we do not need to manually > scan queues and schedule them if controller is already running; we only > need to do that if SOF interrupt is masked, otherwise we'll pick them up > at the next frame. Just to confirm: this patch fixes no known issues, right? It's based on code inspection? > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- > drivers/usb/dwc2/hcd_queue.c | 31 ++++++++++++++++++------------- > 1 file changed, 18 insertions(+), 13 deletions(-) > > diff --git a/drivers/usb/dwc2/hcd_queue.c b/drivers/usb/dwc2/hcd_queue.c > index e34ad5e653501..db9e7c9d31554 100644 > --- a/drivers/usb/dwc2/hcd_queue.c > +++ b/drivers/usb/dwc2/hcd_queue.c > @@ -1468,6 +1468,8 @@ static void dwc2_wait_timer_fn(struct timer_list *t) > { > struct dwc2_qh *qh = from_timer(qh, t, wait_timer); > struct dwc2_hsotg *hsotg = qh->hsotg; > + enum dwc2_transaction_type tr_type; > + u32 intr_mask; > unsigned long flags; > > spin_lock_irqsave(&hsotg->lock, flags); > @@ -1476,19 +1478,22 @@ static void dwc2_wait_timer_fn(struct timer_list *t) > * We'll set wait_timer_cancel to true if we want to cancel this > * operation in dwc2_hcd_qh_unlink(). > */ > - if (!qh->wait_timer_cancel) { > - enum dwc2_transaction_type tr_type; > + if (qh->wait_timer_cancel) > + goto out_unlock; > > - qh->want_wait = false; The removal of this "want_wait = false" isn't mentioned in the commit message and seems unrelated. Did you decide that setting this to false is not important and thus you're removing it? Could you move this part to a separate patch? > + list_move(&qh->qh_list_entry, &hsotg->non_periodic_sched_inactive); > > - list_move(&qh->qh_list_entry, > - &hsotg->non_periodic_sched_inactive); > + /* See if we should kick the controller if it was idle */ > + intr_mask = dwc2_readl(hsotg->regs + GINTMSK); > + if (intr_mask & GINTSTS_SOF) > + goto out_unlock; > > - tr_type = dwc2_hcd_select_transactions(hsotg); > - if (tr_type != DWC2_TRANSACTION_NONE) > - dwc2_hcd_queue_transactions(hsotg, tr_type); > - } > + /* The controller was idle, let's try queue our postponed work */ > + tr_type = dwc2_hcd_select_transactions(hsotg); > + if (tr_type != DWC2_TRANSACTION_NONE) > + dwc2_hcd_queue_transactions(hsotg, tr_type); > > +out_unlock: > spin_unlock_irqrestore(&hsotg->lock, flags); > } > > @@ -1722,10 +1727,6 @@ int dwc2_hcd_qh_add(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh) > > /* Add the new QH to the appropriate schedule */ > if (dwc2_qh_is_non_per(qh)) { > - /* Schedule right away */ > - qh->start_active_frame = hsotg->frame_number; > - qh->next_active_frame = qh->start_active_frame; Where do we set start_active_frame and next_active_frame in the "want_wait" case now? Shouldn't you be doing that in "dwc2_wait_timer_fn()" now that you've removed it from here? ...or is it just not important for non-periodic transfers (in which case you probably don't need to add it to the "not want_wait" case below)? ...this change also seems unrelated and the motivation is not included in the commit description. Should it too be a separate change? > - > if (qh->want_wait) { > list_add_tail(&qh->qh_list_entry, > &hsotg->non_periodic_sched_waiting); > @@ -1733,6 +1734,10 @@ int dwc2_hcd_qh_add(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh) > mod_timer(&qh->wait_timer, > jiffies + DWC2_RETRY_WAIT_DELAY + 1); > } else { > + /* Schedule right away */ > + qh->start_active_frame = hsotg->frame_number; > + qh->next_active_frame = qh->start_active_frame; > + > list_add_tail(&qh->qh_list_entry, > &hsotg->non_periodic_sched_inactive); > } > -- > 2.18.0.rc1.244.gcf134e6275-goog > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jun 15, 2018 at 05:00:03PM -0700, Doug Anderson wrote: > Hi, > > On Fri, Jun 15, 2018 at 3:01 PM, Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > > When we are ready to retry the delayed QH, we do not need to manually > > scan queues and schedule them if controller is already running; we only > > need to do that if SOF interrupt is masked, otherwise we'll pick them up > > at the next frame. > > Just to confirm: this patch fixes no known issues, right? It's based > on code inspection? That is correct. > > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > --- > > drivers/usb/dwc2/hcd_queue.c | 31 ++++++++++++++++++------------- > > 1 file changed, 18 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/usb/dwc2/hcd_queue.c b/drivers/usb/dwc2/hcd_queue.c > > index e34ad5e653501..db9e7c9d31554 100644 > > --- a/drivers/usb/dwc2/hcd_queue.c > > +++ b/drivers/usb/dwc2/hcd_queue.c > > @@ -1468,6 +1468,8 @@ static void dwc2_wait_timer_fn(struct timer_list *t) > > { > > struct dwc2_qh *qh = from_timer(qh, t, wait_timer); > > struct dwc2_hsotg *hsotg = qh->hsotg; > > + enum dwc2_transaction_type tr_type; > > + u32 intr_mask; > > unsigned long flags; > > > > spin_lock_irqsave(&hsotg->lock, flags); > > @@ -1476,19 +1478,22 @@ static void dwc2_wait_timer_fn(struct timer_list *t) > > * We'll set wait_timer_cancel to true if we want to cancel this > > * operation in dwc2_hcd_qh_unlink(). > > */ > > - if (!qh->wait_timer_cancel) { > > - enum dwc2_transaction_type tr_type; > > + if (qh->wait_timer_cancel) > > + goto out_unlock; > > > > - qh->want_wait = false; > > The removal of this "want_wait = false" isn't mentioned in the commit > message and seems unrelated. Did you decide that setting this to > false is not important and thus you're removing it? Could you move > this part to a separate patch? Yes I will. My opinion is that we set/reset the flag in hcd_intr.c when we receive a NAK. Scheduling a transfer does not really affect the state of "NAKiness" of the QH, so it is not right to remove the flag. > > > > + list_move(&qh->qh_list_entry, &hsotg->non_periodic_sched_inactive); > > > > - list_move(&qh->qh_list_entry, > > - &hsotg->non_periodic_sched_inactive); > > + /* See if we should kick the controller if it was idle */ > > + intr_mask = dwc2_readl(hsotg->regs + GINTMSK); > > + if (intr_mask & GINTSTS_SOF) > > + goto out_unlock; > > > > - tr_type = dwc2_hcd_select_transactions(hsotg); > > - if (tr_type != DWC2_TRANSACTION_NONE) > > - dwc2_hcd_queue_transactions(hsotg, tr_type); > > - } > > + /* The controller was idle, let's try queue our postponed work */ > > + tr_type = dwc2_hcd_select_transactions(hsotg); > > + if (tr_type != DWC2_TRANSACTION_NONE) > > + dwc2_hcd_queue_transactions(hsotg, tr_type); > > > > +out_unlock: > > spin_unlock_irqrestore(&hsotg->lock, flags); > > } > > > > @@ -1722,10 +1727,6 @@ int dwc2_hcd_qh_add(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh) > > > > /* Add the new QH to the appropriate schedule */ > > if (dwc2_qh_is_non_per(qh)) { > > - /* Schedule right away */ > > - qh->start_active_frame = hsotg->frame_number; > > - qh->next_active_frame = qh->start_active_frame; > > Where do we set start_active_frame and next_active_frame in the > "want_wait" case now? Shouldn't you be doing that in > "dwc2_wait_timer_fn()" now that you've removed it from here? ...or is > it just not important for non-periodic transfers (in which case you > probably don't need to add it to the "not want_wait" case below)? > Hmm, I thought that we would adjust qh->start_active_frame and qh->next_active_frame as needed when we schedule QH again, similarly to the initial transfer request for a given URB. But I do not have strong opinion so I'll simply drop this change. Thanks!
diff --git a/drivers/usb/dwc2/hcd_queue.c b/drivers/usb/dwc2/hcd_queue.c index e34ad5e653501..db9e7c9d31554 100644 --- a/drivers/usb/dwc2/hcd_queue.c +++ b/drivers/usb/dwc2/hcd_queue.c @@ -1468,6 +1468,8 @@ static void dwc2_wait_timer_fn(struct timer_list *t) { struct dwc2_qh *qh = from_timer(qh, t, wait_timer); struct dwc2_hsotg *hsotg = qh->hsotg; + enum dwc2_transaction_type tr_type; + u32 intr_mask; unsigned long flags; spin_lock_irqsave(&hsotg->lock, flags); @@ -1476,19 +1478,22 @@ static void dwc2_wait_timer_fn(struct timer_list *t) * We'll set wait_timer_cancel to true if we want to cancel this * operation in dwc2_hcd_qh_unlink(). */ - if (!qh->wait_timer_cancel) { - enum dwc2_transaction_type tr_type; + if (qh->wait_timer_cancel) + goto out_unlock; - qh->want_wait = false; + list_move(&qh->qh_list_entry, &hsotg->non_periodic_sched_inactive); - list_move(&qh->qh_list_entry, - &hsotg->non_periodic_sched_inactive); + /* See if we should kick the controller if it was idle */ + intr_mask = dwc2_readl(hsotg->regs + GINTMSK); + if (intr_mask & GINTSTS_SOF) + goto out_unlock; - tr_type = dwc2_hcd_select_transactions(hsotg); - if (tr_type != DWC2_TRANSACTION_NONE) - dwc2_hcd_queue_transactions(hsotg, tr_type); - } + /* The controller was idle, let's try queue our postponed work */ + tr_type = dwc2_hcd_select_transactions(hsotg); + if (tr_type != DWC2_TRANSACTION_NONE) + dwc2_hcd_queue_transactions(hsotg, tr_type); +out_unlock: spin_unlock_irqrestore(&hsotg->lock, flags); } @@ -1722,10 +1727,6 @@ int dwc2_hcd_qh_add(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh) /* Add the new QH to the appropriate schedule */ if (dwc2_qh_is_non_per(qh)) { - /* Schedule right away */ - qh->start_active_frame = hsotg->frame_number; - qh->next_active_frame = qh->start_active_frame; - if (qh->want_wait) { list_add_tail(&qh->qh_list_entry, &hsotg->non_periodic_sched_waiting); @@ -1733,6 +1734,10 @@ int dwc2_hcd_qh_add(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh) mod_timer(&qh->wait_timer, jiffies + DWC2_RETRY_WAIT_DELAY + 1); } else { + /* Schedule right away */ + qh->start_active_frame = hsotg->frame_number; + qh->next_active_frame = qh->start_active_frame; + list_add_tail(&qh->qh_list_entry, &hsotg->non_periodic_sched_inactive); }
When we are ready to retry the delayed QH, we do not need to manually scan queues and schedule them if controller is already running; we only need to do that if SOF interrupt is masked, otherwise we'll pick them up at the next frame. Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- drivers/usb/dwc2/hcd_queue.c | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-)