Re: [PATCH v2] usb: dwc2: host: Don't retry NAKed transactions right away

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Doug,

[add Felipe since this should go through his tree]

i'm back from vacation and happy to see this patch.

> Douglas Anderson <dianders@xxxxxxxxxxxx> hat am 26. Oktober 2017 um 22:22 geschrieben:
> 
> 
> On rk3288-veyron devices on Chrome OS it was found that plugging in an
> Arduino-based USB device could cause the system to lockup, especially
> if the CPU Frequency was at one of the slower operating points (like
> 100 MHz / 200 MHz).
> 
> Upon tracing, I found that the following was happening:
> * The USB device (full speed) was connected to a high speed hub and
>   then to the rk3288.  Thus, we were dealing with split transactions,
>   which is all handled in software on dwc2.
> * Userspace was initiating a BULK IN transfer
> * When we sent the SSPLIT (to start the split transaction), we got an
>   ACK.  Good.  Then we issued the CSPLIT.
> * When we sent the CSPLIT, we got back a NAK.  We immediately (from
>   the interrupt handler) started to retry and sent another SSPLIT.
> * The device kept NAKing our CSPLIT, so we kept ping-ponging between
>   sending a SSPLIT and a CSPLIT, each time sending from the interrupt
>   handler.
> * The handling of the interrupts was (because of the low CPU speed and
>   the inefficiency of the dwc2 interrupt handler) was actually taking
>   _longer_ than it took the other side to send the ACK/NAK.  Thus we
>   were _always_ in the USB interrupt routine.
> * The fact that USB interrupts were always going off was preventing
>   other things from happening in the system.  This included preventing
>   the system from being able to transition to a higher CPU frequency.
> 
> As I understand it, there is no requirement to retry super quickly
> after a NAK, we just have to retry sometime in the future.  Thus one
> solution to the above is to just add a delay between getting a NAK and
> retrying the transmission.  If this delay is sufficiently long to get
> out of the interrupt routine then the rest of the system will be able
> to make forward progress.  Even a 25 us delay would probably be
> enough, but we'll be extra conservative and try to delay 1 ms (the
> exact amount depends on HZ and the accuracy of the jiffy and how close
> the current jiffy is to ticking, but could be as much as 20 ms or as
> little as 1 ms).
> 
> Presumably adding a delay like this could impact the USB throughput,
> so we only add the delay with repeated NAKs.
> 
> NOTE: Upon further testing of a pl2303 serial adapter, I found that
> this fix may help with problems there.  Specifically I found that the
> pl2303 serial adapters tend to respond with a NAK when they have
> nothing to say and thus we end with this same sequence.

This patch fixes "dwc2: NMI watchdog: BUG: soft lockup - CPU#0 stuck for 146s" on bcm2835. After applying your patch on linux-next plus "mm: drop migrate type checks from has_unmovable_pages" i'm able to open 3 PL2303 serial adapters parallel on a Raspberry Pi 1 B without any issues.

Tested-by: Stefan Wahren <stefan.wahren@xxxxxxxx>

Please see my comment below

> 
> Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
> Reviewed-by: Julius Werner <jwerner@xxxxxxxxxxxx>
> ---
> 
> Changes in v2:
> - Address http://crosreview.com/737520 feedback
> 
>  drivers/usb/dwc2/core.h      |  1 +
>  drivers/usb/dwc2/hcd.c       |  7 ++++
>  drivers/usb/dwc2/hcd.h       |  9 +++++
>  drivers/usb/dwc2/hcd_intr.c  | 20 +++++++++++
>  drivers/usb/dwc2/hcd_queue.c | 81 +++++++++++++++++++++++++++++++++++++++++---
>  5 files changed, 114 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> index 8367d4f985c1..c0807e558726 100644
> --- a/drivers/usb/dwc2/core.h
> +++ b/drivers/usb/dwc2/core.h
> @@ -962,6 +962,7 @@ struct dwc2_hsotg {
>  	} flags;
>  
>  	struct list_head non_periodic_sched_inactive;
> +	struct list_head non_periodic_sched_waiting;
>  	struct list_head non_periodic_sched_active;
>  	struct list_head *non_periodic_qh_ptr;
>  	struct list_head periodic_sched_inactive;
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index c2631145f404..ed92ae78dcf9 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -653,6 +653,10 @@ static void dwc2_dump_channel_info(struct dwc2_hsotg *hsotg,
>  	list_for_each_entry(qh, &hsotg->non_periodic_sched_inactive,
>  			    qh_list_entry)
>  		dev_dbg(hsotg->dev, "    %p\n", qh);
> +	dev_dbg(hsotg->dev, "  NP waiting sched:\n");
> +	list_for_each_entry(qh, &hsotg->non_periodic_sched_waiting,
> +			    qh_list_entry)
> +		dev_dbg(hsotg->dev, "    %p\n", qh);
>  	dev_dbg(hsotg->dev, "  NP active sched:\n");
>  	list_for_each_entry(qh, &hsotg->non_periodic_sched_active,
>  			    qh_list_entry)
> @@ -1812,6 +1816,7 @@ static void dwc2_qh_list_free(struct dwc2_hsotg *hsotg,
>  static void dwc2_kill_all_urbs(struct dwc2_hsotg *hsotg)
>  {
>  	dwc2_kill_urbs_in_qh_list(hsotg, &hsotg->non_periodic_sched_inactive);
> +	dwc2_kill_urbs_in_qh_list(hsotg, &hsotg->non_periodic_sched_waiting);
>  	dwc2_kill_urbs_in_qh_list(hsotg, &hsotg->non_periodic_sched_active);
>  	dwc2_kill_urbs_in_qh_list(hsotg, &hsotg->periodic_sched_inactive);
>  	dwc2_kill_urbs_in_qh_list(hsotg, &hsotg->periodic_sched_ready);
> @@ -4989,6 +4994,7 @@ static void dwc2_hcd_free(struct dwc2_hsotg *hsotg)
>  
>  	/* Free memory for QH/QTD lists */
>  	dwc2_qh_list_free(hsotg, &hsotg->non_periodic_sched_inactive);
> +	dwc2_qh_list_free(hsotg, &hsotg->non_periodic_sched_waiting);
>  	dwc2_qh_list_free(hsotg, &hsotg->non_periodic_sched_active);
>  	dwc2_qh_list_free(hsotg, &hsotg->periodic_sched_inactive);
>  	dwc2_qh_list_free(hsotg, &hsotg->periodic_sched_ready);
> @@ -5151,6 +5157,7 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg)
>  
>  	/* Initialize the non-periodic schedule */
>  	INIT_LIST_HEAD(&hsotg->non_periodic_sched_inactive);
> +	INIT_LIST_HEAD(&hsotg->non_periodic_sched_waiting);
>  	INIT_LIST_HEAD(&hsotg->non_periodic_sched_active);
>  
>  	/* Initialize the periodic schedule */
> diff --git a/drivers/usb/dwc2/hcd.h b/drivers/usb/dwc2/hcd.h
> index 11c3c145b793..af4f4360e8b7 100644
> --- a/drivers/usb/dwc2/hcd.h
> +++ b/drivers/usb/dwc2/hcd.h
> @@ -313,12 +313,16 @@ struct dwc2_hs_transfer_time {
>   *                      descriptor and indicates original XferSize value for the
>   *                      descriptor
>   * @unreserve_timer:    Timer for releasing periodic reservation.
> + * @wait_timer:         Timer used to wait before re-queuing.
>   * @dwc2_tt:            Pointer to our tt info (or NULL if no tt).
>   * @ttport:             Port number within our tt.
>   * @tt_buffer_dirty     True if clear_tt_buffer_complete is pending
>   * @unreserve_pending:  True if we planned to unreserve but haven't yet.
>   * @schedule_low_speed: True if we have a low/full speed component (either the
>   *			host is in low/full speed mode or do_split).
> + * @want_wait:          We should wait before re-queuing; only matters for non-
> + *                      periodic transfers and is ignored for periodic ones.
> + * @wait_timer_cancel:  Set to true to cancel the wait_timer.
>   *
>   * A Queue Head (QH) holds the static characteristics of an endpoint and
>   * maintains a list of transfers (QTDs) for that endpoint. A QH structure may
> @@ -353,11 +357,14 @@ struct dwc2_qh {
>  	u32 desc_list_sz;
>  	u32 *n_bytes;
>  	struct timer_list unreserve_timer;
> +	struct timer_list wait_timer;
>  	struct dwc2_tt *dwc_tt;
>  	int ttport;
>  	unsigned tt_buffer_dirty:1;
>  	unsigned unreserve_pending:1;
>  	unsigned schedule_low_speed:1;
> +	unsigned want_wait:1;
> +	unsigned wait_timer_cancel:1;
>  };
>  
>  /**
> @@ -388,6 +395,7 @@ struct dwc2_qh {
>   * @n_desc:             Number of DMA descriptors for this QTD
>   * @isoc_frame_index_last: Last activated frame (packet) index, used in
>   *                      descriptor DMA mode only
> + * @num_naks:           Number of NAKs received on this QTD.
>   * @urb:                URB for this transfer
>   * @qh:                 Queue head for this QTD
>   * @qtd_list_entry:     For linking to the QH's list of QTDs
> @@ -418,6 +426,7 @@ struct dwc2_qtd {
>  	u8 error_count;
>  	u8 n_desc;
>  	u16 isoc_frame_index_last;
> +	u16 num_naks;
>  	struct dwc2_hcd_urb *urb;
>  	struct dwc2_qh *qh;
>  	struct list_head qtd_list_entry;
> diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
> index 28a8210710b1..aad3abd2ddf2 100644
> --- a/drivers/usb/dwc2/hcd_intr.c
> +++ b/drivers/usb/dwc2/hcd_intr.c
> @@ -52,6 +52,12 @@
>  #include "core.h"
>  #include "hcd.h"
>  
> +/*
> + * If we get this many NAKs on a split transaction we'll slow down
> + * retransmission.  A 1 here means delay after the first NAK.
> + */
> +#define DWC2_NAKS_BEFORE_DELAY		3
> +
>  /* This function is for debug only */
>  static void dwc2_track_missed_sofs(struct dwc2_hsotg *hsotg)
>  {
> @@ -1200,11 +1206,25 @@ static void dwc2_hc_nak_intr(struct dwc2_hsotg *hsotg,
>  	/*
>  	 * Handle NAK for IN/OUT SSPLIT/CSPLIT transfers, bulk, control, and
>  	 * interrupt. Re-start the SSPLIT transfer.
> +	 *
> +	 * Normally for non-periodic transfers we'll retry right away, but to
> +	 * avoid interrupt storms we'll wait before retrying if we've got
> +	 * several NAKs. If we didn't do this we'd retry directly from the
> +	 * interrupt handler and could end up quickly getting another
> +	 * interrupt (another NAK), which we'd retry.
> +	 *
> +	 * Note that in DMA mode software only gets involved to re-send NAKed
> +	 * transfers for split transactions, so we only need to apply this
> +	 * delaying logic when handling splits. In non-DMA mode presumably we
> +	 * might want a similar delay if someone can demonstrate this problem
> +	 * affects that code path too.
>  	 */
>  	if (chan->do_split) {
>  		if (chan->complete_split)
>  			qtd->error_count = 0;
>  		qtd->complete_split = 0;
> +		qtd->num_naks++;
> +		qtd->qh->want_wait = qtd->num_naks >= DWC2_NAKS_BEFORE_DELAY;
>  		dwc2_halt_channel(hsotg, chan, qtd, DWC2_HC_XFER_NAK);
>  		goto handle_nak_done;
>  	}
> diff --git a/drivers/usb/dwc2/hcd_queue.c b/drivers/usb/dwc2/hcd_queue.c
> index 3ae8b1bbaa55..bea0aadd756e 100644
> --- a/drivers/usb/dwc2/hcd_queue.c
> +++ b/drivers/usb/dwc2/hcd_queue.c
> @@ -57,6 +57,9 @@
>  /* Wait this long before releasing periodic reservation */
>  #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))
> +
>  /**
>   * dwc2_periodic_channel_available() - Checks that a channel is available for a
>   * periodic transfer
> @@ -1439,6 +1442,55 @@ static void dwc2_deschedule_periodic(struct dwc2_hsotg *hsotg,
>  	list_del_init(&qh->qh_list_entry);
>  }
>  
> +/**
> + * dwc2_wait_timer_fn() - Timer function to re-queue after waiting
> + *
> + * As per the spec, a NAK indicates that "a function is temporarily unable to
> + * transmit or receive data, but will eventually be able to do so without need
> + * of host intervention".
> + *
> + * That means that when we encounter a NAK we're supposed to retry.
> + *
> + * ...but if we retry right away (from the interrupt handler that saw the NAK)
> + * then we can end up with an interrupt storm (if the other side keeps NAKing
> + * us) because on slow enough CPUs it could take us longer to get out of the
> + * interrupt routine than it takes for the device to send another NAK.  That
> + * leads to a constant stream of NAK interrupts and the CPU locks.
> + *
> + * ...so instead of retrying right away in the case of a NAK we'll set a timer
> + * to retry some time later.  This function handles that timer and moves the
> + * qh back to the "inactive" list, then queues transactions.
> + *
> + * @data: Pointer to a qh to re-schedule.
> + */
> +static void dwc2_wait_timer_fn(unsigned long data)
> +{
> +	struct dwc2_qh *qh = (struct dwc2_qh *)data;
> +	struct dwc2_hsotg *hsotg = qh->hsotg;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&hsotg->lock, flags);
> +
> +	/*
> +	 * 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;
> +
> +		qh->want_wait = false;
> +
> +		list_move(&qh->qh_list_entry,
> +			  &hsotg->non_periodic_sched_inactive);
> +
> +		tr_type = dwc2_hcd_select_transactions(hsotg);
> +		if (tr_type != DWC2_TRANSACTION_NONE)
> +			dwc2_hcd_queue_transactions(hsotg, tr_type);
> +	}
> +
> +	spin_unlock_irqrestore(&hsotg->lock, flags);
> +}
> +
>  /**
>   * dwc2_qh_init() - Initializes a QH structure
>   *
> @@ -1468,6 +1520,7 @@ static void dwc2_qh_init(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh,
>  	qh->hsotg = hsotg;
>  	setup_timer(&qh->unreserve_timer, dwc2_unreserve_timer_fn,
>  		    (unsigned long)qh);
> +	setup_timer(&qh->wait_timer, dwc2_wait_timer_fn, (unsigned long)qh);

Kees Cook started to replace setup_timer() with the new timer_setup(). I'm not sure if it's okay to use the deprecated function and replace all of them later.

Best regards
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux