| Message ID | 153655347149.19910.4781444209586693527.stgit@Rincon.localdomain (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 6ed30a7d8ec29d3aba46e47aa8b4a44f077dda4e |
| Headers | show |
| Series | [v2] usb: dwc2: host: use hrtimer for NAK retries | expand |
Hi, On Sun, Sep 9, 2018 at 9:24 PM Terin Stock <terin@terinstock.com> wrote: > > Modify the wait delay utilize the high resolution timer API to allow for > more precisely scheduled callbacks. > > A previous commit added a 1ms retry delay after multiple consecutive > NAKed transactions using jiffies. On systems with a low timer interrupt > frequency, this delay may be significantly longer than specified, > resulting in misbehavior with some USB devices. > > This scenario was reached on a Raspberry Pi 3B with a Macally FDD-USB > floppy drive (identified as 0424:0fdc Standard Microsystems Corp. > Floppy, based on the USB97CFDC USB FDC). With the relay delay, the drive > would be unable to mount a disk, replying with NAKs until the device was > reset. > > Using ktime, the delta between starting the timer (in dwc2_hcd_qh_add) > and the callback function can be determined. With the original delay > implementation, this value was consistently approximately 12ms. (output > in us). > > <idle>-0 [000] ..s. 1600.559974: dwc2_wait_timer_fn: wait_timer delta: 11976 > <idle>-0 [000] ..s. 1600.571974: dwc2_wait_timer_fn: wait_timer delta: 11977 > <idle>-0 [000] ..s. 1600.583974: dwc2_wait_timer_fn: wait_timer delta: 11976 > <idle>-0 [000] ..s. 1600.595974: dwc2_wait_timer_fn: wait_timer delta: 11977 > > After converting the relay delay to using a higher resolution timer, the > delay was much closer to 1ms. > > <idle>-0 [000] d.h. 1956.553017: dwc2_wait_timer_fn: wait_timer delta: 1002 > <idle>-0 [000] d.h. 1956.554114: dwc2_wait_timer_fn: wait_timer delta: 1002 > <idle>-0 [000] d.h. 1957.542660: dwc2_wait_timer_fn: wait_timer delta: 1004 > <idle>-0 [000] d.h. 1957.543701: dwc2_wait_timer_fn: wait_timer delta: 1002 > > The floppy drive operates properly with delays up to approximately 5ms, > and sends NAKs for any delays that are longer. > > Fixes: 38d2b5fb75c1 ("usb: dwc2: host: Don't retry NAKed transactions right away") > Signed-off-by: Terin Stock <terin@terinstock.com> > --- > drivers/usb/dwc2/hcd.h | 2 +- > drivers/usb/dwc2/hcd_queue.c | 19 ++++++++++++------- > 2 files changed, 13 insertions(+), 8 deletions(-) Reviewed-by: Douglas Anderson <dianders@chromium.org> Cc: stable@vger.kernel.org
On 11/14/2018 3:30 AM, Doug Anderson wrote: > Hi, > > On Sun, Sep 9, 2018 at 9:24 PM Terin Stock <terin@terinstock.com> wrote: >> >> Modify the wait delay utilize the high resolution timer API to allow for >> more precisely scheduled callbacks. >> >> A previous commit added a 1ms retry delay after multiple consecutive >> NAKed transactions using jiffies. On systems with a low timer interrupt >> frequency, this delay may be significantly longer than specified, >> resulting in misbehavior with some USB devices. >> >> This scenario was reached on a Raspberry Pi 3B with a Macally FDD-USB >> floppy drive (identified as 0424:0fdc Standard Microsystems Corp. >> Floppy, based on the USB97CFDC USB FDC). With the relay delay, the drive >> would be unable to mount a disk, replying with NAKs until the device was >> reset. >> >> Using ktime, the delta between starting the timer (in dwc2_hcd_qh_add) >> and the callback function can be determined. With the original delay >> implementation, this value was consistently approximately 12ms. (output >> in us). >> >> <idle>-0 [000] ..s. 1600.559974: dwc2_wait_timer_fn: wait_timer delta: 11976 >> <idle>-0 [000] ..s. 1600.571974: dwc2_wait_timer_fn: wait_timer delta: 11977 >> <idle>-0 [000] ..s. 1600.583974: dwc2_wait_timer_fn: wait_timer delta: 11976 >> <idle>-0 [000] ..s. 1600.595974: dwc2_wait_timer_fn: wait_timer delta: 11977 >> >> After converting the relay delay to using a higher resolution timer, the >> delay was much closer to 1ms. >> >> <idle>-0 [000] d.h. 1956.553017: dwc2_wait_timer_fn: wait_timer delta: 1002 >> <idle>-0 [000] d.h. 1956.554114: dwc2_wait_timer_fn: wait_timer delta: 1002 >> <idle>-0 [000] d.h. 1957.542660: dwc2_wait_timer_fn: wait_timer delta: 1004 >> <idle>-0 [000] d.h. 1957.543701: dwc2_wait_timer_fn: wait_timer delta: 1002 >> >> The floppy drive operates properly with delays up to approximately 5ms, >> and sends NAKs for any delays that are longer. >> >> Fixes: 38d2b5fb75c1 ("usb: dwc2: host: Don't retry NAKed transactions right away") >> Signed-off-by: Terin Stock <terin@terinstock.com> >> --- >> drivers/usb/dwc2/hcd.h | 2 +- >> drivers/usb/dwc2/hcd_queue.c | 19 ++++++++++++------- >> 2 files changed, 13 insertions(+), 8 deletions(-) > > Reviewed-by: Douglas Anderson <dianders@chromium.org> > Cc: stable@vger.kernel.org > Acked-by: Minas Harutyunyan <hminas@synopsys.com>
Felipe, On Tue, Nov 20, 2018 at 11:16 PM Minas Harutyunyan <minas.harutyunyan@synopsys.com> wrote: > > On 11/14/2018 3:30 AM, Doug Anderson wrote: > > Hi, > > > > On Sun, Sep 9, 2018 at 9:24 PM Terin Stock <terin@terinstock.com> wrote: > >> > >> Modify the wait delay utilize the high resolution timer API to allow for > >> more precisely scheduled callbacks. > >> > >> A previous commit added a 1ms retry delay after multiple consecutive > >> NAKed transactions using jiffies. On systems with a low timer interrupt > >> frequency, this delay may be significantly longer than specified, > >> resulting in misbehavior with some USB devices. > >> > >> This scenario was reached on a Raspberry Pi 3B with a Macally FDD-USB > >> floppy drive (identified as 0424:0fdc Standard Microsystems Corp. > >> Floppy, based on the USB97CFDC USB FDC). With the relay delay, the drive > >> would be unable to mount a disk, replying with NAKs until the device was > >> reset. > >> > >> Using ktime, the delta between starting the timer (in dwc2_hcd_qh_add) > >> and the callback function can be determined. With the original delay > >> implementation, this value was consistently approximately 12ms. (output > >> in us). > >> > >> <idle>-0 [000] ..s. 1600.559974: dwc2_wait_timer_fn: wait_timer delta: 11976 > >> <idle>-0 [000] ..s. 1600.571974: dwc2_wait_timer_fn: wait_timer delta: 11977 > >> <idle>-0 [000] ..s. 1600.583974: dwc2_wait_timer_fn: wait_timer delta: 11976 > >> <idle>-0 [000] ..s. 1600.595974: dwc2_wait_timer_fn: wait_timer delta: 11977 > >> > >> After converting the relay delay to using a higher resolution timer, the > >> delay was much closer to 1ms. > >> > >> <idle>-0 [000] d.h. 1956.553017: dwc2_wait_timer_fn: wait_timer delta: 1002 > >> <idle>-0 [000] d.h. 1956.554114: dwc2_wait_timer_fn: wait_timer delta: 1002 > >> <idle>-0 [000] d.h. 1957.542660: dwc2_wait_timer_fn: wait_timer delta: 1004 > >> <idle>-0 [000] d.h. 1957.543701: dwc2_wait_timer_fn: wait_timer delta: 1002 > >> > >> The floppy drive operates properly with delays up to approximately 5ms, > >> and sends NAKs for any delays that are longer. > >> > >> Fixes: 38d2b5fb75c1 ("usb: dwc2: host: Don't retry NAKed transactions right away") > >> Signed-off-by: Terin Stock <terin@terinstock.com> > >> --- > >> drivers/usb/dwc2/hcd.h | 2 +- > >> drivers/usb/dwc2/hcd_queue.c | 19 ++++++++++++------- > >> 2 files changed, 13 insertions(+), 8 deletions(-) > > > > Reviewed-by: Douglas Anderson <dianders@chromium.org> > > Cc: stable@vger.kernel.org > > > Acked-by: Minas Harutyunyan <hminas@synopsys.com> It looks like Terin missed CCing you on the original patch. I assume you are still picking up dwc2 changes like this? If so, should he resend the patch targeting you (and including Minas and my tags) or do you want to pick it up from one of the mailing lists he CCed? Thanks! -Doug
Hi, Doug Anderson <dianders@chromium.org> writes: > >> Signed-off-by: Terin Stock <terin@terinstock.com> >> >> --- >> >> drivers/usb/dwc2/hcd.h | 2 +- >> >> drivers/usb/dwc2/hcd_queue.c | 19 ++++++++++++------- >> >> 2 files changed, 13 insertions(+), 8 deletions(-) >> > >> > Reviewed-by: Douglas Anderson <dianders@chromium.org> >> > Cc: stable@vger.kernel.org >> > >> Acked-by: Minas Harutyunyan <hminas@synopsys.com> > > It looks like Terin missed CCing you on the original patch. I assume > you are still picking up dwc2 changes like this? If so, should he > resend the patch targeting you (and including Minas and my tags) or do > you want to pick it up from one of the mailing lists he CCed? I'm picking it up, thanks
diff --git a/drivers/usb/dwc2/hcd.h b/drivers/usb/dwc2/hcd.h index 5502a501f516..93483dc37801 100644 --- a/drivers/usb/dwc2/hcd.h +++ b/drivers/usb/dwc2/hcd.h @@ -366,7 +366,7 @@ struct dwc2_qh { u32 desc_list_sz; u32 *n_bytes; struct timer_list unreserve_timer; - struct timer_list wait_timer; + struct hrtimer wait_timer; struct dwc2_tt *dwc_tt; int ttport; unsigned tt_buffer_dirty:1; diff --git a/drivers/usb/dwc2/hcd_queue.c b/drivers/usb/dwc2/hcd_queue.c index 301ced1618f8..60caf8e0b762 100644 --- a/drivers/usb/dwc2/hcd_queue.c +++ b/drivers/usb/dwc2/hcd_queue.c @@ -59,7 +59,7 @@ #define DWC2_UNRESERVE_DELAY (msecs_to_jiffies(5)) /* If we get a NAK, wait this long before retrying */ -#define DWC2_RETRY_WAIT_DELAY (msecs_to_jiffies(1)) +#define DWC2_RETRY_WAIT_DELAY 1*1E6L /** * dwc2_periodic_channel_available() - Checks that a channel is available for a @@ -1464,10 +1464,12 @@ static void dwc2_deschedule_periodic(struct dwc2_hsotg *hsotg, * qh back to the "inactive" list, then queues transactions. * * @t: Pointer to wait_timer in a qh. + * + * Return: HRTIMER_NORESTART to not automatically restart this timer. */ -static void dwc2_wait_timer_fn(struct timer_list *t) +static enum hrtimer_restart dwc2_wait_timer_fn(struct hrtimer *t) { - struct dwc2_qh *qh = from_timer(qh, t, wait_timer); + struct dwc2_qh *qh = container_of(t, struct dwc2_qh, wait_timer); struct dwc2_hsotg *hsotg = qh->hsotg; unsigned long flags; @@ -1491,6 +1493,7 @@ static void dwc2_wait_timer_fn(struct timer_list *t) } spin_unlock_irqrestore(&hsotg->lock, flags); + return HRTIMER_NORESTART; } /** @@ -1521,7 +1524,8 @@ static void dwc2_qh_init(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh, /* Initialize QH */ qh->hsotg = hsotg; timer_setup(&qh->unreserve_timer, dwc2_unreserve_timer_fn, 0); - timer_setup(&qh->wait_timer, dwc2_wait_timer_fn, 0); + hrtimer_init(&qh->wait_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL); + qh->wait_timer.function = &dwc2_wait_timer_fn; qh->ep_type = ep_type; qh->ep_is_in = ep_is_in; @@ -1690,7 +1694,7 @@ void dwc2_hcd_qh_free(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh) * won't do anything anyway, but we want it to finish before we free * memory. */ - del_timer_sync(&qh->wait_timer); + hrtimer_cancel(&qh->wait_timer); dwc2_host_put_tt_info(hsotg, qh->dwc_tt); @@ -1716,6 +1720,7 @@ int dwc2_hcd_qh_add(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh) { int status; u32 intr_mask; + ktime_t delay; if (dbg_qh(qh)) dev_vdbg(hsotg->dev, "%s()\n", __func__); @@ -1734,8 +1739,8 @@ int dwc2_hcd_qh_add(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh) list_add_tail(&qh->qh_list_entry, &hsotg->non_periodic_sched_waiting); qh->wait_timer_cancel = false; - mod_timer(&qh->wait_timer, - jiffies + DWC2_RETRY_WAIT_DELAY + 1); + delay = ktime_set(0, DWC2_RETRY_WAIT_DELAY); + hrtimer_start(&qh->wait_timer, delay, HRTIMER_MODE_REL); } else { list_add_tail(&qh->qh_list_entry, &hsotg->non_periodic_sched_inactive);
Modify the wait delay utilize the high resolution timer API to allow for more precisely scheduled callbacks. A previous commit added a 1ms retry delay after multiple consecutive NAKed transactions using jiffies. On systems with a low timer interrupt frequency, this delay may be significantly longer than specified, resulting in misbehavior with some USB devices. This scenario was reached on a Raspberry Pi 3B with a Macally FDD-USB floppy drive (identified as 0424:0fdc Standard Microsystems Corp. Floppy, based on the USB97CFDC USB FDC). With the relay delay, the drive would be unable to mount a disk, replying with NAKs until the device was reset. Using ktime, the delta between starting the timer (in dwc2_hcd_qh_add) and the callback function can be determined. With the original delay implementation, this value was consistently approximately 12ms. (output in us). <idle>-0 [000] ..s. 1600.559974: dwc2_wait_timer_fn: wait_timer delta: 11976 <idle>-0 [000] ..s. 1600.571974: dwc2_wait_timer_fn: wait_timer delta: 11977 <idle>-0 [000] ..s. 1600.583974: dwc2_wait_timer_fn: wait_timer delta: 11976 <idle>-0 [000] ..s. 1600.595974: dwc2_wait_timer_fn: wait_timer delta: 11977 After converting the relay delay to using a higher resolution timer, the delay was much closer to 1ms. <idle>-0 [000] d.h. 1956.553017: dwc2_wait_timer_fn: wait_timer delta: 1002 <idle>-0 [000] d.h. 1956.554114: dwc2_wait_timer_fn: wait_timer delta: 1002 <idle>-0 [000] d.h. 1957.542660: dwc2_wait_timer_fn: wait_timer delta: 1004 <idle>-0 [000] d.h. 1957.543701: dwc2_wait_timer_fn: wait_timer delta: 1002 The floppy drive operates properly with delays up to approximately 5ms, and sends NAKs for any delays that are longer. Fixes: 38d2b5fb75c1 ("usb: dwc2: host: Don't retry NAKed transactions right away") Signed-off-by: Terin Stock <terin@terinstock.com> --- drivers/usb/dwc2/hcd.h | 2 +- drivers/usb/dwc2/hcd_queue.c | 19 ++++++++++++------- 2 files changed, 13 insertions(+), 8 deletions(-)