[PATCH 1/3] usb: dwc2: host: do not delay retries for CONTROL IN transfers

[PATCH 1/3] usb: dwc2: host: do not delay retries for CONTROL IN transfers

[Dmitry Torokhov]

When handling split transactions we will try to delay retry after
getting a NAK from the device. This works well for BULK transfers that
can be polled for essentially forever. Unfortunately, on slower systems
at boot time, when the kernel is busy enumerating all the devices (USB
or not), we issue a bunch of control requests (reading device
descriptors, etc). If we get a NAK for the IN part of the control
request and delay retry for too long (because the system is busy), we
may confuse the device when we finally get to reissue SSPLIT/CSPLIT IN
and the device will respond with STALL. As a result we end up with
failure to get device descriptor and will fail to enumerate the device:

[    3.428801] usb 2-1.2.1: new full-speed USB device number 9 using dwc2
[    3.508576] usb 2-1.2.1: device descriptor read/8, error -32
[    3.699150] usb 2-1.2.1: device descriptor read/8, error -32
[    3.891653] usb 2-1.2.1: new full-speed USB device number 10 using dwc2
[    3.968859] usb 2-1.2.1: device descriptor read/8, error -32
...

Let's not delay retries of split CONTROL IN transfers, as this allows us
to reliably enumerate devices at boot time.

Fixes 38d2b5fb75c1 ("usb: dwc2: host: Don't retry NAKed transactions right away")
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/usb/dwc2/hcd_intr.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
index a5dfd9d8bd9a2..d423b6a49f96c 100644
--- a/drivers/usb/dwc2/hcd_intr.c
+++ b/drivers/usb/dwc2/hcd_intr.c
@@ -1212,7 +1212,10 @@ static void dwc2_hc_nak_intr(struct dwc2_hsotg *hsotg,
 	 * 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.
+	 * interrupt (another NAK), which we'd retry. Note that we do not
+	 * delay retries for IN parts of control requests, as those are expected
+	 * to complete fairly quickly, and if we delay them we risk confusing
+	 * the device and cause it issue STALL.
 	 *
 	 * Note that in DMA mode software only gets involved to re-send NAKed
 	 * transfers for split transactions, so we only need to apply this
@@ -1225,7 +1228,9 @@ static void dwc2_hc_nak_intr(struct dwc2_hsotg *hsotg,
 			qtd->error_count = 0;
 		qtd->complete_split = 0;
 		qtd->num_naks++;
-		qtd->qh->want_wait = qtd->num_naks >= DWC2_NAKS_BEFORE_DELAY;
+		qtd->qh->want_wait = qtd->num_naks >= DWC2_NAKS_BEFORE_DELAY &&
+				!(chan->ep_type == USB_ENDPOINT_XFER_CONTROL &&
+				  chan->ep_is_in);
 		dwc2_halt_channel(hsotg, chan, qtd, DWC2_HC_XFER_NAK);
 		goto handle_nak_done;
 	}
-- 
2.18.0.rc1.244.gcf134e6275-goog

[1/3] usb: dwc2: host: do not delay retries for CONTROL IN transfers

[Torokhov]

When handling split transactions we will try to delay retry after
getting a NAK from the device. This works well for BULK transfers that
can be polled for essentially forever. Unfortunately, on slower systems
at boot time, when the kernel is busy enumerating all the devices (USB
or not), we issue a bunch of control requests (reading device
descriptors, etc). If we get a NAK for the IN part of the control
request and delay retry for too long (because the system is busy), we
may confuse the device when we finally get to reissue SSPLIT/CSPLIT IN
and the device will respond with STALL. As a result we end up with
failure to get device descriptor and will fail to enumerate the device:

[    3.428801] usb 2-1.2.1: new full-speed USB device number 9 using dwc2
[    3.508576] usb 2-1.2.1: device descriptor read/8, error -32
[    3.699150] usb 2-1.2.1: device descriptor read/8, error -32
[    3.891653] usb 2-1.2.1: new full-speed USB device number 10 using dwc2
[    3.968859] usb 2-1.2.1: device descriptor read/8, error -32
...

Let's not delay retries of split CONTROL IN transfers, as this allows us
to reliably enumerate devices at boot time.

Fixes 38d2b5fb75c1 ("usb: dwc2: host: Don't retry NAKed transactions right away")
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/usb/dwc2/hcd_intr.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
index a5dfd9d8bd9a2..d423b6a49f96c 100644
--- a/drivers/usb/dwc2/hcd_intr.c
+++ b/drivers/usb/dwc2/hcd_intr.c
@@ -1212,7 +1212,10 @@ static void dwc2_hc_nak_intr(struct dwc2_hsotg *hsotg,
 	 * 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.
+	 * interrupt (another NAK), which we'd retry. Note that we do not
+	 * delay retries for IN parts of control requests, as those are expected
+	 * to complete fairly quickly, and if we delay them we risk confusing
+	 * the device and cause it issue STALL.
 	 *
 	 * Note that in DMA mode software only gets involved to re-send NAKed
 	 * transfers for split transactions, so we only need to apply this
@@ -1225,7 +1228,9 @@ static void dwc2_hc_nak_intr(struct dwc2_hsotg *hsotg,
 			qtd->error_count = 0;
 		qtd->complete_split = 0;
 		qtd->num_naks++;
-		qtd->qh->want_wait = qtd->num_naks >= DWC2_NAKS_BEFORE_DELAY;
+		qtd->qh->want_wait = qtd->num_naks >= DWC2_NAKS_BEFORE_DELAY &&
+				!(chan->ep_type == USB_ENDPOINT_XFER_CONTROL &&
+				  chan->ep_is_in);
 		dwc2_halt_channel(hsotg, chan, qtd, DWC2_HC_XFER_NAK);
 		goto handle_nak_done;
 	}

[PATCH 2/3] usb: dwc2: host: do not delay retries after successful transfer

[Dmitry Torokhov]

When handling split transactions we should not be delaying retrying
SSPLIT/CSPLIT after we successfully communicate with the device, so
let's reset dtd->num_naks counter when handling XFERCOMPL.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/usb/dwc2/hcd_intr.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
index d423b6a49f96c..b9879de6230d0 100644
--- a/drivers/usb/dwc2/hcd_intr.c
+++ b/drivers/usb/dwc2/hcd_intr.c
@@ -1092,6 +1092,7 @@ static void dwc2_hc_xfercomp_intr(struct dwc2_hsotg *hsotg,
 	}
 
 handle_xfercomp_done:
+	qtd->num_naks = 0;
 	disable_hc_int(hsotg, chnum, HCINTMSK_XFERCOMPL);
 }
 
-- 
2.18.0.rc1.244.gcf134e6275-goog

[2/3] usb: dwc2: host: do not delay retries after successful transfer

[Torokhov]

When handling split transactions we should not be delaying retrying
SSPLIT/CSPLIT after we successfully communicate with the device, so
let's reset dtd->num_naks counter when handling XFERCOMPL.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/usb/dwc2/hcd_intr.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
index d423b6a49f96c..b9879de6230d0 100644
--- a/drivers/usb/dwc2/hcd_intr.c
+++ b/drivers/usb/dwc2/hcd_intr.c
@@ -1092,6 +1092,7 @@ static void dwc2_hc_xfercomp_intr(struct dwc2_hsotg *hsotg,
 	}
 
 handle_xfercomp_done:
+	qtd->num_naks = 0;
 	disable_hc_int(hsotg, chnum, HCINTMSK_XFERCOMPL);
 }
 

[PATCH 3/3] usb: dwc2: host: do not schedule delayed QH unnecessarily

[Dmitry Torokhov]

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.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.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;
+	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;
-
 		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

[3/3] usb: dwc2: host: do not schedule delayed QH unnecessarily

[Torokhov]

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.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.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;
+	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;
-
 		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);
 		}

Re: [PATCH 1/3] usb: dwc2: host: do not delay retries for CONTROL IN transfers

[Doug Anderson]

Hi,

On Fri, Jun 15, 2018 at 3:01 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> When handling split transactions we will try to delay retry after
> getting a NAK from the device. This works well for BULK transfers that
> can be polled for essentially forever. Unfortunately, on slower systems
> at boot time, when the kernel is busy enumerating all the devices (USB
> or not), we issue a bunch of control requests (reading device
> descriptors, etc). If we get a NAK for the IN part of the control
> request and delay retry for too long (because the system is busy), we
> may confuse the device when we finally get to reissue SSPLIT/CSPLIT IN
> and the device will respond with STALL. As a result we end up with
> failure to get device descriptor and will fail to enumerate the device:
>
> [    3.428801] usb 2-1.2.1: new full-speed USB device number 9 using dwc2
> [    3.508576] usb 2-1.2.1: device descriptor read/8, error -32
> [    3.699150] usb 2-1.2.1: device descriptor read/8, error -32
> [    3.891653] usb 2-1.2.1: new full-speed USB device number 10 using dwc2
> [    3.968859] usb 2-1.2.1: device descriptor read/8, error -32
> ...
>
> Let's not delay retries of split CONTROL IN transfers, as this allows us
> to reliably enumerate devices at boot time.
>
> Fixes 38d2b5fb75c1 ("usb: dwc2: host: Don't retry NAKed transactions right away")

nit: I think there's supposed to be a ":" after "Fixes"

> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/usb/dwc2/hcd_intr.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)

This seems like a sane fix to me.

Reviewed-by: Douglas Anderson <dianders@chromium.org>

[1/3] usb: dwc2: host: do not delay retries for CONTROL IN transfers

[Doug Anderson]

Hi,

On Fri, Jun 15, 2018 at 3:01 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> When handling split transactions we will try to delay retry after
> getting a NAK from the device. This works well for BULK transfers that
> can be polled for essentially forever. Unfortunately, on slower systems
> at boot time, when the kernel is busy enumerating all the devices (USB
> or not), we issue a bunch of control requests (reading device
> descriptors, etc). If we get a NAK for the IN part of the control
> request and delay retry for too long (because the system is busy), we
> may confuse the device when we finally get to reissue SSPLIT/CSPLIT IN
> and the device will respond with STALL. As a result we end up with
> failure to get device descriptor and will fail to enumerate the device:
>
> [    3.428801] usb 2-1.2.1: new full-speed USB device number 9 using dwc2
> [    3.508576] usb 2-1.2.1: device descriptor read/8, error -32
> [    3.699150] usb 2-1.2.1: device descriptor read/8, error -32
> [    3.891653] usb 2-1.2.1: new full-speed USB device number 10 using dwc2
> [    3.968859] usb 2-1.2.1: device descriptor read/8, error -32
> ...
>
> Let's not delay retries of split CONTROL IN transfers, as this allows us
> to reliably enumerate devices at boot time.
>
> Fixes 38d2b5fb75c1 ("usb: dwc2: host: Don't retry NAKed transactions right away")

nit: I think there's supposed to be a ":" after "Fixes"

> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/usb/dwc2/hcd_intr.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)

This seems like a sane fix to me.

Reviewed-by: Douglas Anderson <dianders@chromium.org>
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] usb: dwc2: host: do not delay retries after successful transfer

[Doug Anderson]

Hi,

On Fri, Jun 15, 2018 at 3:01 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> When handling split transactions we should not be delaying retrying
> SSPLIT/CSPLIT after we successfully communicate with the device, so
> let's reset dtd->num_naks counter when handling XFERCOMPL.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/usb/dwc2/hcd_intr.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Douglas Anderson <dianders@chromium.org>

[2/3] usb: dwc2: host: do not delay retries after successful transfer

[Doug Anderson]

Hi,

On Fri, Jun 15, 2018 at 3:01 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> When handling split transactions we should not be delaying retrying
> SSPLIT/CSPLIT after we successfully communicate with the device, so
> let's reset dtd->num_naks counter when handling XFERCOMPL.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/usb/dwc2/hcd_intr.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Douglas Anderson <dianders@chromium.org>
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] usb: dwc2: host: do not schedule delayed QH unnecessarily

[Doug Anderson]

Hi,

On Fri, Jun 15, 2018 at 3:01 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.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@gmail.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
>

[3/3] usb: dwc2: host: do not schedule delayed QH unnecessarily

[Doug Anderson]

Hi,

On Fri, Jun 15, 2018 at 3:01 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.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@gmail.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
>
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] usb: dwc2: host: do not schedule delayed QH unnecessarily

[Dmitry Torokhov]

On Fri, Jun 15, 2018 at 05:00:03PM -0700, Doug Anderson wrote:
> Hi,
> 
> On Fri, Jun 15, 2018 at 3:01 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.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?

That is correct.

> 
> 
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.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?

Yes I will. My opinion is that we set/reset the flag in hcd_intr.c when
we receive a NAK. Scheduling a transfer does not really affect the state
of "NAKiness" of the QH, so it is not right to remove the flag.

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

Hmm, I thought that we would adjust qh->start_active_frame and
qh->next_active_frame as needed when we schedule QH again, similarly to
the initial transfer request for a given URB. But I do not have strong
opinion so I'll simply drop this change.

Thanks!

-- 
Dmitry

[3/3] usb: dwc2: host: do not schedule delayed QH unnecessarily

[Torokhov]

On Fri, Jun 15, 2018 at 05:00:03PM -0700, Doug Anderson wrote:
> Hi,
> 
> On Fri, Jun 15, 2018 at 3:01 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.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?

That is correct.

> 
> 
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.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?

Yes I will. My opinion is that we set/reset the flag in hcd_intr.c when
we receive a NAK. Scheduling a transfer does not really affect the state
of "NAKiness" of the QH, so it is not right to remove the flag.

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

Hmm, I thought that we would adjust qh->start_active_frame and
qh->next_active_frame as needed when we schedule QH again, similarly to
the initial transfer request for a given URB. But I do not have strong
opinion so I'll simply drop this change.

Thanks!

Re: [PATCH 2/3] usb: dwc2: host: do not delay retries after successful transfer

[Doug Anderson]

Hi,

On Fri, Jun 15, 2018 at 4:49 PM, Doug Anderson <dianders@chromium.org> wrote:
> Hi,
>
> On Fri, Jun 15, 2018 at 3:01 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
>> When handling split transactions we should not be delaying retrying
>> SSPLIT/CSPLIT after we successfully communicate with the device, so
>> let's reset dtd->num_naks counter when handling XFERCOMPL.
>>
>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> ---
>>  drivers/usb/dwc2/hcd_intr.c | 1 +
>>  1 file changed, 1 insertion(+)
>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>

Though this looked good, I just tested this myself and found that it
corrupts memory.  Please do not merge it.

Specifically slub_debug complains of a use after free and sure enough
I can see that in one case we call dwc2_xfercomp_isoc_split_in() and
it can call dwc2_release_channel() and that can free the qtd.  Setting
the qtd->num_naks to 0 at the end of the function is a use after free.

I've just tested the same USB peripheral that Dmitry was testing with.
I found two things:

* This patch is important for making that peripheral work

* When I move the "qtd->num_naks" to the beginning it's all good.

Dmitry is OOTO at the moment, so I'll re-post this patch with that change.



-Doug

[2/3] usb: dwc2: host: do not delay retries after successful transfer

[Doug Anderson]

Hi,

On Fri, Jun 15, 2018 at 4:49 PM, Doug Anderson <dianders@chromium.org> wrote:
> Hi,
>
> On Fri, Jun 15, 2018 at 3:01 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
>> When handling split transactions we should not be delaying retrying
>> SSPLIT/CSPLIT after we successfully communicate with the device, so
>> let's reset dtd->num_naks counter when handling XFERCOMPL.
>>
>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> ---
>>  drivers/usb/dwc2/hcd_intr.c | 1 +
>>  1 file changed, 1 insertion(+)
>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>

Though this looked good, I just tested this myself and found that it
corrupts memory.  Please do not merge it.

Specifically slub_debug complains of a use after free and sure enough
I can see that in one case we call dwc2_xfercomp_isoc_split_in() and
it can call dwc2_release_channel() and that can free the qtd.  Setting
the qtd->num_naks to 0 at the end of the function is a use after free.

I've just tested the same USB peripheral that Dmitry was testing with.
I found two things:

* This patch is important for making that peripheral work

* When I move the "qtd->num_naks" to the beginning it's all good.

Dmitry is OOTO at the moment, so I'll re-post this patch with that change.



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