Hi,
On Fri, Jun 15, 2018 at 3:01 PM, Dmitry Torokhov
<dmitry.torokhov@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
