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@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?
That is correct.
>
>
> > 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?
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!
--
Dmitry
--
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
