Skip to content

Commit

Permalink
usb: dwc2: host: use hrtimer for NAK retries
Browse files Browse the repository at this point in the history
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: 38d2b5f ("usb: dwc2: host: Don't retry NAKed transactions right away")
Cc: <stable@vger.kernel.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Acked-by: Minas Harutyunyan <hminas@synopsys.com>
Signed-off-by: Terin Stock <terin@terinstock.com>
Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
  • Loading branch information
terinjokes authored and Felipe Balbi committed Dec 5, 2018
1 parent 47b6f8b commit 6ed30a7
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 8 deletions.
2 changes: 1 addition & 1 deletion drivers/usb/dwc2/hcd.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
19 changes: 12 additions & 7 deletions drivers/usb/dwc2/hcd_queue.c
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;

Expand All @@ -1491,6 +1493,7 @@ static void dwc2_wait_timer_fn(struct timer_list *t)
}

spin_unlock_irqrestore(&hsotg->lock, flags);
return HRTIMER_NORESTART;
}

/**
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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);

Expand All @@ -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__);
Expand All @@ -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);
Expand Down

0 comments on commit 6ed30a7

Please sign in to comment.