Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1319777

Re: [PATCH v5 16/21] usb: dwc2: host: Manage frame nums better in scheduler

Show all headers | View raw

Hi,

On Fri, Jan 22, 2016 at 10:18 AM, Douglas Anderson
<dianders@chromium.org> wrote:
> The dwc2 scheduler (contained in hcd_queue.c) was a bit confusing in the
> way it initted / kept track of which frames a QH was going to be active
> in.  Let's clean things up a little bit in preparation for a rewrite of
> the microframe scheduler.
>
> Specifically:
> * Old code would pick a frame number in dwc2_qh_init() and would try to
>   pick it "in a slightly future (micro)frame".  As far as I can tell the
>   reason for this was that there was a delay between dwc2_qh_init() and
>   when we actually wanted to dwc2_hcd_qh_add().  ...but apparently this
>   attempt to be slightly in the future wasn't enough because
>   dwc2_hcd_qh_add() then had code to reset things if the frame _wasn't_
>   in the future.  There's no reason not to just pick the frame later.
>   For non-periodic QH we now pick the frame in dwc2_hcd_qh_add().  For
>   periodic QH we pick the frame at dwc2_schedule_periodic() time.
> * The old "dwc2_qh_init() actually assigned to "hsotg->frame_number".
>   This doesn't seem like a great idea since that variable is supposed to
>   be used to keep track of which SOF the interrupt handler has seen.
>   Let's be clean: anyone who wants the current frame number (instead of
>   the one as of the last interrupt) should ask for it.
> * The old code wasn't terribly consistent about trying to use the frame
>   that the microframe scheduler assigned to it.  In
>   dwc2_sched_periodic_split() when it was scheduling the first frame it
>   always "ORed" in 0x7 (!).  Since the frame goes on the wire 1 uFrame
>   after next_active_frame it meant that the SSPLIT would always try for
>   uFrame 0 and the transaction would happen on the low speed bus during
>   uFrame 1.  This is irregardless of what the microframe scheduler
>   said.
> * The old code assumed it would get called to schedule the next in a
>   periodic split very quickly.  That is if next_active_frame was
>   0 (transfer on wire in uFrame 1) it assumed it was getting called to
>   schedule the next uFrame during uFrame 1 too (so it could queue
>   something up for uFrame 2).  It should be possible to actually queue
>   something up for uFrame 2 while in uFrame 2 (AKA queue up ASAP).  To
>   do this, code needs to look at the previously scheduled frame when
>   deciding when to next be active, not look at the current frame number.
> * If there was no microframe scheduler, the old code would check for
>   whether we should be active using "qh->next_active_frame ==
>   frame_number".  This seemed like a race waiting to happen.  ...plus
>   there's no way that you wouldn't want to schedule if next_active_frame
>   was actually less than frame number.
>
> Note that this change doesn't make 100% sense on its own since it's
> expecting some sanity in the frame numbers assigned by the microframe
> scheduler and (as per the future patch which rewries it) I think that
> the current microframe scheduler is quite insane.  However, it seems
> like splitting this up from the microframe scheduler patch makes things
> into smaller chunks and hopefully adds to clarity rather than reduces
> it.  The two patches could certainly be squashed.  Not that in the very
> least, I don't see any obvious bad behavior introduced with just this
> patch.
>
> I've attempted to keep the config parameter to disable the microframe
> scheduler in tact in this change, though I'm not sure it's worth it.
> Obviously the code is touched a lot so it's possible I regressed
> something when the microframe scheduler is disabled, though I did some
> basic testing and it seemed to work OK.  I'm still not 100% sure why you
> wouldn't want the microframe scheduler (presuming it works), so maybe a
> future patch (or a future version of this patch?) could remove that
> parameter.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> Changes in v5: None
> Changes in v4:
> - Manage frame nums better in scheduler new for v4.
>
> Changes in v3: None
> Changes in v2: None
>
>  drivers/usb/dwc2/hcd.h       |  10 +-
>  drivers/usb/dwc2/hcd_queue.c | 355 ++++++++++++++++++++++++++++++++-----------
>  2 files changed, 273 insertions(+), 92 deletions(-)

As an FYI to anyone considering reviewing or applying this particular patch.

It's proposed to squash
<https://chromium-review.googlesource.com/#/c/324185/> as a fixup to
this patch for the next version.  This is intended to fix problems
that Alan Stern reported at
<http://www.spinics.net/lists/linux-usb/msg135678.html>

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH v5 0/21] usb: dwc2: host: Fix and speed up all the stuff, especially with splits Douglas Anderson <dianders@chromium.org> - 2016-01-22 19:30 +0100
  [PATCH v5 18/21] usb: dwc2: host: Add dwc2_hcd_get_future_frame_number() call Douglas Anderson <dianders@chromium.org> - 2016-01-22 19:30 +0100
  [PATCH v5 03/21] usb: dwc2: host: Set host_rx_fifo_size to 528 for rk3066 Douglas Anderson <dianders@chromium.org> - 2016-01-22 19:30 +0100
    Re: [PATCH v5 03/21] usb: dwc2: host: Set host_rx_fifo_size to 528  for rk3066 Kever Yang <kever.yang@rock-chips.com> - 2016-01-27 11:20 +0100
      Re: [PATCH v5 03/21] usb: dwc2: host: Set host_rx_fifo_size to 528  for rk3066 Doug Anderson <dianders@chromium.org> - 2016-01-27 20:50 +0100
        Re: [PATCH v5 03/21] usb: dwc2: host: Set host_rx_fifo_size to 528  for rk3066 Kever Yang <kever.yang@rock-chips.com> - 2016-01-28 09:30 +0100
  [PATCH v5 14/21] usb: dwc2: host: Split code out to make dwc2_do_reserve() Douglas Anderson <dianders@chromium.org> - 2016-01-22 19:30 +0100
  [PATCH v5 16/21] usb: dwc2: host: Manage frame nums better in scheduler Douglas Anderson <dianders@chromium.org> - 2016-01-22 19:30 +0100
    Re: [PATCH v5 16/21] usb: dwc2: host: Manage frame nums better in scheduler Doug Anderson <dianders@chromium.org> - 2016-01-27 21:50 +0100
  [PATCH v5 11/21] usb: dwc2: host: Use periodic interrupt even with DMA Douglas Anderson <dianders@chromium.org> - 2016-01-22 19:30 +0100
  [PATCH v5 05/21] usb: dwc2: host: Avoid use of chan->qh after qh freed Douglas Anderson <dianders@chromium.org> - 2016-01-22 19:30 +0100
    Re: [PATCH v5 05/21] usb: dwc2: host: Avoid use of chan->qh after  qh freed Kever Yang <kever.yang@rock-chips.com> - 2016-01-28 04:30 +0100
      Re: [PATCH v5 05/21] usb: dwc2: host: Avoid use of chan->qh after qh freed Doug Anderson <dianders@chromium.org> - 2016-01-29 00:30 +0100
  [PATCH v5 15/21] usb: dwc2: host: Add scheduler logging for missed SOFs Douglas Anderson <dianders@chromium.org> - 2016-01-22 19:30 +0100
  [PATCH v5 21/21] usb: dwc2: host: If using uframe scheduler, end splits better Douglas Anderson <dianders@chromium.org> - 2016-01-22 19:30 +0100
  Re: [PATCH v5 0/21] usb: dwc2: host: Fix and speed up all the stuff, especially with splits Heiko Stuebner <heiko@sntech.de> - 2016-01-23 19:00 +0100
    Re: [PATCH v5 0/21] usb: dwc2: host: Fix and speed up all the stuff,  especially with splits Doug Anderson <dianders@chromium.org> - 2016-01-24 00:20 +0100
      Re: [PATCH v5 0/21] usb: dwc2: host: Fix and speed up all the stuff,  especially with splits Doug Anderson <dianders@chromium.org> - 2016-01-24 06:40 +0100
  Re: [PATCH v5 20/21] usb: dwc2: host: Totally redo the microframe scheduler Doug Anderson <dianders@chromium.org> - 2016-01-24 06:50 +0100

csiph-web