[3/3] usb: dwc2: host: do not schedule delayed QH unnecessarily

[3/3] usb: dwc2: host: do not schedule delayed QH unnecessarily

[Torokhov]

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(-)

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);
 		}

[3/3] usb: dwc2: host: do not schedule delayed QH unnecessarily

[Doug Anderson]

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

[3/3] usb: dwc2: host: do not schedule delayed QH unnecessarily

[Torokhov]

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!