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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAD=FV=V_5W7U-vo-HosN6bfBjqMkAs2hNG=o26soXn3H54YDzA@mail.gmail.com>
Date:   Fri, 15 Jun 2018 17:00:03 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     Dmitry Torokhov <dmitry.torokhov@...il.com>
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

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?


> 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?


> +       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
>