[PATCH 0/3] usb: dwc2: Fixes and improvements

[PATCH 0/3] usb: dwc2: Fixes and improvements

[Nicolas Saenz Julienne]

I'm picking up this series by Guenter Roeck as he stated he has no time
for it ATM. It was found to solve some unaligned DMA access issues on
Raspberry Pi 3. You can find the original discussion here:
https://lore.kernel.org/linux-usb/20200226210414.28133-1-linux@roeck-us.net/

I removed the fist patch from the original series as it turned out to be
contententious and needs more in-depth testing. Following is the edited
origin series description. Note that extra testing was performed on
RPi3:

"This series addresses the following problems:

- Fix receive transfers with 0 byte transfer length
- Abort transactions after unknown receive errors
  if the receive buffer is full
- Reduce "trimming xfer length" logging noise

The problems fixed with this series were observed when connecting
a DM9600 Ethernet adapter to Veyron Chromebooks such as the ASUS
Chromebook C201PA. The series was tested extensively with this and
other adapters.

The observed problems are also reported when tethering various
phones, so test coverage with such phones would be very appreciated."

---

Guenter Roeck (3):
  usb: dwc2: Do not update data length if it is 0 on inbound transfers
  usb: dwc2: Abort transaction after errors with unknown reason
  usb: dwc2: Make "trimming xfer length" a debug message

 drivers/usb/dwc2/hcd.c      | 15 ++++++++-------
 drivers/usb/dwc2/hcd_intr.c | 14 +++++++++++++-
 2 files changed, 21 insertions(+), 8 deletions(-)

-- 
2.29.2

[PATCH 1/3] usb: dwc2: Do not update data length if it is 0 on inbound transfers

[Nicolas Saenz Julienne]

From: Guenter Roeck <linux@roeck-us.net>

The DWC2 documentation states that transfers with zero data length should
set the number of packets to 1 and the transfer length to 0. This is not
currently the case for inbound transfers: the transfer length is set to
the maximum packet length. This can have adverse effects if the chip
actually does transfer data as it is programmed to do. Follow chip
documentation and keep the transfer length set to 0 in that situation.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Fixes: 56f5b1cff22a1 ("staging: Core files for the DWC2 driver")
Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
Tested-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
---
 drivers/usb/dwc2/hcd.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index e9ac215b9663..fc3269f5faf1 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -1313,19 +1313,20 @@ static void dwc2_hc_start_transfer(struct dwc2_hsotg *hsotg,
 			if (num_packets > max_hc_pkt_count) {
 				num_packets = max_hc_pkt_count;
 				chan->xfer_len = num_packets * chan->max_packet;
+			} else if (chan->ep_is_in) {
+				/*
+				 * Always program an integral # of max packets
+				 * for IN transfers.
+				 * Note: This assumes that the input buffer is
+				 * aligned and sized accordingly.
+				 */
+				chan->xfer_len = num_packets * chan->max_packet;
 			}
 		} else {
 			/* Need 1 packet for transfer length of 0 */
 			num_packets = 1;
 		}
 
-		if (chan->ep_is_in)
-			/*
-			 * Always program an integral # of max packets for IN
-			 * transfers
-			 */
-			chan->xfer_len = num_packets * chan->max_packet;
-
 		if (chan->ep_type == USB_ENDPOINT_XFER_INT ||
 		    chan->ep_type == USB_ENDPOINT_XFER_ISOC)
 			/*
-- 
2.29.2

[PATCH 2/3] usb: dwc2: Abort transaction after errors with unknown reason

[Nicolas Saenz Julienne]

From: Guenter Roeck <linux@roeck-us.net>

In some situations, the following error messages are reported.

dwc2 ff540000.usb: dwc2_hc_chhltd_intr_dma: Channel 1 - ChHltd set, but reason is unknown
dwc2 ff540000.usb: hcint 0x00000002, intsts 0x04000021

This is sometimes followed by:

dwc2 ff540000.usb: dwc2_update_urb_state_abn(): trimming xfer length

and then:

WARNING: CPU: 0 PID: 0 at kernel/v4.19/drivers/usb/dwc2/hcd.c:2913
			dwc2_assign_and_init_hc+0x98c/0x990

The warning suggests that an odd buffer address is to be used for DMA.

After an error is observed, the receive buffer may be full
(urb->actual_length >= urb->length). However, the urb is still left in
the queue unless three errors were observed in a row. When it is queued
again, the dwc2 hcd code translates this into a 1-block transfer.
If urb->actual_length (ie the total expected receive length) is not
DMA-aligned, the buffer pointer programmed into the chip will be
unaligned. This results in the observed warning.

To solve the problem, abort input transactions after an error with
unknown cause if the entire packet was already received. This may be
a bit drastic, but we don't really know why the transfer was aborted
even though the entire packet was received. Aborting the transfer in
this situation is less risky than accepting a potentially corrupted
packet.

With this patch in place, the 'ChHltd set' and 'trimming xfer length'
messages are still observed, but there are no more transfer attempts
with odd buffer addresses.

Cc: Boris ARZUR <boris@konbu.org>
Cc: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Fixes: 151d0cbdbe860 ("usb: dwc2: make the scheduler handle excessive NAKs better")
Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
Tested-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
---
 drivers/usb/dwc2/hcd_intr.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
index a052d39b4375..12819e019e13 100644
--- a/drivers/usb/dwc2/hcd_intr.c
+++ b/drivers/usb/dwc2/hcd_intr.c
@@ -1977,6 +1977,18 @@ static void dwc2_hc_chhltd_intr_dma(struct dwc2_hsotg *hsotg,
 		qtd->error_count++;
 		dwc2_update_urb_state_abn(hsotg, chan, chnum, qtd->urb,
 					  qtd, DWC2_HC_XFER_XACT_ERR);
+		/*
+		 * We can get here after a completed transaction
+		 * (urb->actual_length >= urb->length) which was not reported
+		 * as completed. If that is the case, and we do not abort
+		 * the transfer, a transfer of size 0 will be enqueued
+		 * subsequently. If urb->actual_length is not DMA-aligned,
+		 * the buffer will then point to an unaligned address, and
+		 * the resulting behavior is undefined. Bail out in that
+		 * situation.
+		 */
+		if (qtd->urb->actual_length >= qtd->urb->length)
+			qtd->error_count = 3;
 		dwc2_hcd_save_data_toggle(hsotg, chan, chnum, qtd);
 		dwc2_halt_channel(hsotg, chan, qtd, DWC2_HC_XFER_XACT_ERR);
 	}
-- 
2.29.2

[PATCH 3/3] usb: dwc2: Make "trimming xfer length" a debug message

[Nicolas Saenz Julienne]

From: Guenter Roeck <linux@roeck-us.net>

With some USB network adapters, such as DM96xx, the following message
is seen for each maximum size receive packet.

dwc2 ff540000.usb: dwc2_update_urb_state(): trimming xfer length

This happens because the packet size requested by the driver is 1522
bytes, wMaxPacketSize is 64, the dwc2 driver configures the chip to
receive 24*64 = 1536 bytes, and the chip does indeed send more than
1522 bytes of data. Since the event does not indicate an error condition,
the message is just noise. Demote it to debug level.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Fixes: 7359d482eb4d3 ("staging: HCD files for the DWC2 driver")
Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
Tested-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
---
 drivers/usb/dwc2/hcd_intr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
index 12819e019e13..d5f4ec1b73b1 100644
--- a/drivers/usb/dwc2/hcd_intr.c
+++ b/drivers/usb/dwc2/hcd_intr.c
@@ -500,7 +500,7 @@ static int dwc2_update_urb_state(struct dwc2_hsotg *hsotg,
 						      &short_read);
 
 	if (urb->actual_length + xfer_length > urb->length) {
-		dev_warn(hsotg->dev, "%s(): trimming xfer length\n", __func__);
+		dev_dbg(hsotg->dev, "%s(): trimming xfer length\n", __func__);
 		xfer_length = urb->length - urb->actual_length;
 	}
 
-- 
2.29.2

Re: [PATCH 0/3] usb: dwc2: Fixes and improvements

[Doug Anderson]

Hi,

On Wed, Jan 13, 2021 at 3:21 AM Nicolas Saenz Julienne
<nsaenzjulienne@suse.de> wrote:
>
> I'm picking up this series by Guenter Roeck as he stated he has no time
> for it ATM. It was found to solve some unaligned DMA access issues on
> Raspberry Pi 3. You can find the original discussion here:
> https://lore.kernel.org/linux-usb/20200226210414.28133-1-linux@roeck-us.net/
>
> I removed the fist patch from the original series as it turned out to be
> contententious and needs more in-depth testing. Following is the edited
> origin series description. Note that extra testing was performed on
> RPi3:
>
> "This series addresses the following problems:
>
> - Fix receive transfers with 0 byte transfer length
> - Abort transactions after unknown receive errors
>   if the receive buffer is full
> - Reduce "trimming xfer length" logging noise
>
> The problems fixed with this series were observed when connecting
> a DM9600 Ethernet adapter to Veyron Chromebooks such as the ASUS
> Chromebook C201PA. The series was tested extensively with this and
> other adapters.
>
> The observed problems are also reported when tethering various
> phones, so test coverage with such phones would be very appreciated."
>
> ---
>
> Guenter Roeck (3):
>   usb: dwc2: Do not update data length if it is 0 on inbound transfers
>   usb: dwc2: Abort transaction after errors with unknown reason
>   usb: dwc2: Make "trimming xfer length" a debug message
>
>  drivers/usb/dwc2/hcd.c      | 15 ++++++++-------
>  drivers/usb/dwc2/hcd_intr.c | 14 +++++++++++++-
>  2 files changed, 21 insertions(+), 8 deletions(-)

It's been long enough ago that I've forgotten where this was left off,
but IIRC the 3 patches that you have here are all fine to land (and
have my Reviewed-by tag).  However, I think Guenter was still tracking
down additional problems.  Guenter: does that match your recollection?

It looks like there are still bugs open for this on our public bug tracker:

https://issuetracker.google.com/issues/172208170
https://issuetracker.google.com/issues/172216241

...but, as Guenter said, I don't think there's anyone actively working on them.

I'm not really doing too much with dwc2 these days either and don't
currently have good HW setup for testing, so for the most part I'll
leave it to you.  I wanted to at least summarize what I remembered,
though!  :-)

-Doug

Re: [PATCH 0/3] usb: dwc2: Fixes and improvements

[Guenter Roeck]

On Wed, Jan 13, 2021 at 03:20:55PM -0800, Doug Anderson wrote:
> Hi,
> 
[ ... ]
> 
> It's been long enough ago that I've forgotten where this was left off,
> but IIRC the 3 patches that you have here are all fine to land (and
> have my Reviewed-by tag).  However, I think Guenter was still tracking
> down additional problems.  Guenter: does that match your recollection?
> 
> It looks like there are still bugs open for this on our public bug tracker:
> 
> https://issuetracker.google.com/issues/172208170
> https://issuetracker.google.com/issues/172216241
> 
> ...but, as Guenter said, I don't think there's anyone actively working on them.
> 
> I'm not really doing too much with dwc2 these days either and don't
> currently have good HW setup for testing, so for the most part I'll
> leave it to you.  I wanted to at least summarize what I remembered,
> though!  :-)
> 

The patches in this series still match what I had in my latest test code,
so it makes sense to move forward with them. I don't think I ever found
an acceptable version of the DMA alignment code.

Guenter

Re: [PATCH 0/3] usb: dwc2: Fixes and improvements

[Nicolas Saenz Julienne]

[-- Attachment #1: Type: text/plain, Size: 1376 bytes --]

Hi Guenter, Doug, thanks for having a look at this.

On Wed, 2021-01-13 at 19:07 -0800, Guenter Roeck wrote:
> On Wed, Jan 13, 2021 at 03:20:55PM -0800, Doug Anderson wrote:
> > Hi,
> > 
> [ ... ]
> > 
> > It's been long enough ago that I've forgotten where this was left off,
> > but IIRC the 3 patches that you have here are all fine to land (and
> > have my Reviewed-by tag).  However, I think Guenter was still tracking
> > down additional problems.  Guenter: does that match your recollection?
> > 
> > It looks like there are still bugs open for this on our public bug tracker:
> > 
> > https://issuetracker.google.com/issues/172208170
> > https://issuetracker.google.com/issues/172216241
> > 
> > ...but, as Guenter said, I don't think there's anyone actively working on them.
> > 
> > I'm not really doing too much with dwc2 these days either and don't
> > currently have good HW setup for testing, so for the most part I'll
> > leave it to you.  I wanted to at least summarize what I remembered,
> > though!  :-)
> > 
> 
> The patches in this series still match what I had in my latest test code,
> so it makes sense to move forward with them. I don't think I ever found
> an acceptable version of the DMA alignment code.

As for the alignment code rework, can you recall the underlying issue that
warranted it?

Regards,
Nicolas


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/3] usb: dwc2: Fixes and improvements

[Guenter Roeck]

On Thu, Jan 14, 2021 at 10:26:25AM +0100, Nicolas Saenz Julienne wrote:
> Hi Guenter, Doug, thanks for having a look at this.
> 
> On Wed, 2021-01-13 at 19:07 -0800, Guenter Roeck wrote:
> > On Wed, Jan 13, 2021 at 03:20:55PM -0800, Doug Anderson wrote:
> > > Hi,
> > > 
> > [ ... ]
> > > 
> > > It's been long enough ago that I've forgotten where this was left off,
> > > but IIRC the 3 patches that you have here are all fine to land (and
> > > have my Reviewed-by tag).  However, I think Guenter was still tracking
> > > down additional problems.  Guenter: does that match your recollection?
> > > 
> > > It looks like there are still bugs open for this on our public bug tracker:
> > > 
> > > https://issuetracker.google.com/issues/172208170
> > > https://issuetracker.google.com/issues/172216241
> > > 
> > > ...but, as Guenter said, I don't think there's anyone actively working on them.
> > > 
> > > I'm not really doing too much with dwc2 these days either and don't
> > > currently have good HW setup for testing, so for the most part I'll
> > > leave it to you.  I wanted to at least summarize what I remembered,
> > > though!  :-)
> > > 
> > 
> > The patches in this series still match what I had in my latest test code,
> > so it makes sense to move forward with them. I don't think I ever found
> > an acceptable version of the DMA alignment code.
> 
> As for the alignment code rework, can you recall the underlying issue that
> warranted it?
> 

See

https://patchwork.kernel.org/project/linux-usb/patch/20200226210414.28133-2-linux@roeck-us.net/

for details. It isn't up to date - it says that buffer alignment to
DWC2_USB_DMA_ALIGN would be acceptable. However, it turned out in testing
that buffers do have to be aligned to dma_get_cache_alignment(), at least
on some mips systems.

My latest work-in-progress patch describes the changes made as:

    To simplify the code, move the old data pointer back to the beginning of
    the new buffer, restoring most of the original commit. Increase buffer
    alignment to dma_get_cache_alignment(). Ensure that the data pointer is
    DMA aligned by using ____cacheline_aligned instead of realigning it after
    allocation. Ensure that the allocated buffer is a multiple of
    wMaxPacketSize to guarantee that the chip does not write beyond the end
    of the buffer.

I can provide that version of the patch in case someone wants to pick it up,
but it would need thorough testing on a variety of systems before it is
applied.

Guenter