linux-kernel - Re: [PATCH 3/3] usb: dwc2: host: do not schedule delayed QH unnecessarily

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 15 Jun 2018 17:19:23 -0700
From:   Dmitry Torokhov <dmitry.torokhov@...il.com>
To:     Doug Anderson <dianders@...omium.org>
Cc:     Felipe Balbi <felipe.balbi@...ux.intel.com>,
        Minas Harutyunyan <hminas@...opsys.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-usb@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/3] usb: dwc2: host: do not schedule delayed QH
 unnecessarily

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@...il.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@...il.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!

-- 
Dmitry