[PATCH v2 0/2] usb: dwc2: fix isoc split in transfer issue

[PATCH v2 0/2] usb: dwc2: fix isoc split in transfer issue

[William Wu]

This patch fix dma unaligned problem and data lost problem for
isoc split in transfer.

Test on rk3288 platform, use an usb hs Hub (GL852G-12) and an usb
fs audio device (Plantronics headset) to capture and playback.

William Wu (2):
  usb: dwc2: alloc dma aligned buffer for isoc split in
  usb: dwc2: fix isoc split in transfer with no data

 drivers/usb/dwc2/hcd.c       | 63 +++++++++++++++++++++++++++++++++++++++++---
 drivers/usb/dwc2/hcd.h       | 10 +++++++
 drivers/usb/dwc2/hcd_intr.c  | 10 ++++++-
 drivers/usb/dwc2/hcd_queue.c |  8 +++++-
 4 files changed, 86 insertions(+), 5 deletions(-)

-- 
2.0.0

[PATCH v2 1/2] usb: dwc2: alloc dma aligned buffer for isoc split in

[William Wu]

The commit 3bc04e28a030 ("usb: dwc2: host: Get aligned DMA in
a more supported way") rips out a lot of code to simply the
allocation of aligned DMA. However, it also introduces a new
issue when use isoc split in transfer.

In my test case, I connect the dwc2 controller with an usb hs
Hub (GL852G-12), and plug an usb fs audio device (Plantronics
headset) into the downstream port of Hub. Then use the usb mic
to record, we can find noise when playback.

It's because that the usb Hub uses an MDATA for the first
transaction and a DATA0 for the second transaction for the isoc
split in transaction. An typical isoc split in transaction sequence
like this:

- SSPLIT IN transaction
- CSPLIT IN transaction
  - MDATA packet
- CSPLIT IN transaction
  - DATA0 packet

The DMA address of MDATA (urb->dma) is always DWORD-aligned, but
the DMA address of DATA0 (urb->dma + qtd->isoc_split_offset) may
not be DWORD-aligned, it depends on the qtd->isoc_split_offset (the
length of MDATA). In my test case, the length of MDATA is usually
unaligned, this casue DATA0 packet transmission error.

This patch base on the old way of aligned DMA allocation in the
dwc2 driver to get aligned DMA for isoc split in.

Signed-off-by: William Wu <william.wu@rock-chips.com>
---
Changes in v2:
- None

 drivers/usb/dwc2/hcd.c       | 63 +++++++++++++++++++++++++++++++++++++++++---
 drivers/usb/dwc2/hcd.h       | 10 +++++++
 drivers/usb/dwc2/hcd_intr.c  |  8 ++++++
 drivers/usb/dwc2/hcd_queue.c |  8 +++++-
 4 files changed, 85 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index 190f959..8c2b35f 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -1562,11 +1562,20 @@ static void dwc2_hc_start_transfer(struct dwc2_hsotg *hsotg,
 	}
 
 	if (hsotg->params.host_dma) {
-		dwc2_writel((u32)chan->xfer_dma,
-			    hsotg->regs + HCDMA(chan->hc_num));
+		dma_addr_t dma_addr;
+
+		if (chan->align_buf) {
+			if (dbg_hc(chan))
+				dev_vdbg(hsotg->dev, "align_buf\n");
+			dma_addr = chan->align_buf;
+		} else {
+			dma_addr = chan->xfer_dma;
+		}
+		dwc2_writel((u32)dma_addr, hsotg->regs + HCDMA(chan->hc_num));
+
 		if (dbg_hc(chan))
 			dev_vdbg(hsotg->dev, "Wrote %08lx to HCDMA(%d)\n",
-				 (unsigned long)chan->xfer_dma, chan->hc_num);
+				 (unsigned long)dma_addr, chan->hc_num);
 	}
 
 	/* Start the split */
@@ -2620,6 +2629,33 @@ static void dwc2_hc_init_xfer(struct dwc2_hsotg *hsotg,
 	}
 }
 
+static int dwc2_alloc_split_dma_aligned_buf(struct dwc2_hsotg *hsotg,
+					    struct dwc2_qh *qh,
+					    struct dwc2_host_chan *chan)
+{
+	if (!qh->dw_align_buf) {
+		qh->dw_align_buf = kmalloc(chan->max_packet,
+					   GFP_ATOMIC | GFP_DMA);
+		if (!qh->dw_align_buf)
+			return -ENOMEM;
+
+		qh->dw_align_buf_size = chan->max_packet;
+	}
+
+	qh->dw_align_buf_dma = dma_map_single(hsotg->dev, qh->dw_align_buf,
+					      qh->dw_align_buf_size,
+					      DMA_FROM_DEVICE);
+
+	if (dma_mapping_error(hsotg->dev, qh->dw_align_buf_dma)) {
+		dev_err(hsotg->dev, "can't map align_buf\n");
+		chan->align_buf = 0;
+		return -EINVAL;
+	}
+
+	chan->align_buf = qh->dw_align_buf_dma;
+	return 0;
+}
+
 #define DWC2_USB_DMA_ALIGN 4
 
 struct dma_aligned_buffer {
@@ -2797,6 +2833,27 @@ static int dwc2_assign_and_init_hc(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh)
 	/* Set the transfer attributes */
 	dwc2_hc_init_xfer(hsotg, chan, qtd);
 
+	/* For non-dword aligned buffers */
+	if (hsotg->params.host_dma > 0 && qh->do_split &&
+	    chan->ep_is_in && (chan->xfer_dma & 0x3)) {
+		dev_vdbg(hsotg->dev, "Non-aligned buffer\n");
+		if (dwc2_alloc_split_dma_aligned_buf(hsotg, qh, chan)) {
+			dev_err(hsotg->dev,
+				"%s: Failed to allocate memory to handle non-dword aligned buffer\n",
+				__func__);
+			/* Add channel back to free list */
+			chan->align_buf = 0;
+			chan->multi_count = 0;
+			list_add_tail(&chan->hc_list_entry,
+				      &hsotg->free_hc_list);
+			qtd->in_process = 0;
+			qh->channel = NULL;
+			return -ENOMEM;
+		}
+	} else {
+		chan->align_buf = 0;
+	}
+
 	if (chan->ep_type == USB_ENDPOINT_XFER_INT ||
 	    chan->ep_type == USB_ENDPOINT_XFER_ISOC)
 		/*
diff --git a/drivers/usb/dwc2/hcd.h b/drivers/usb/dwc2/hcd.h
index 96a9da5..adf1fd0 100644
--- a/drivers/usb/dwc2/hcd.h
+++ b/drivers/usb/dwc2/hcd.h
@@ -76,6 +76,8 @@ struct dwc2_qh;
  *                      (micro)frame
  * @xfer_buf:           Pointer to current transfer buffer position
  * @xfer_dma:           DMA address of xfer_buf
+ * @align_buf:		In Buffer DMA mode this will be used if xfer_buf is not
+ *			DWORD aligned
  * @xfer_len:           Total number of bytes to transfer
  * @xfer_count:         Number of bytes transferred so far
  * @start_pkt_count:    Packet count at start of transfer
@@ -133,6 +135,7 @@ struct dwc2_host_chan {
 
 	u8 *xfer_buf;
 	dma_addr_t xfer_dma;
+	dma_addr_t align_buf;
 	u32 xfer_len;
 	u32 xfer_count;
 	u16 start_pkt_count;
@@ -303,6 +306,10 @@ struct dwc2_hs_transfer_time {
  *                           is tightly packed.
  * @ls_duration_us:     Duration on the low speed bus schedule.
  * @ntd:                Actual number of transfer descriptors in a list
+ * @dw_align_buf:	Used instead of original buffer if its physical address
+ *			is not dword-aligned
+ * @dw_align_buf_size:	Size of dw_align_buf
+ * @dw_align_buf_dma:	DMA address for dw_align_buf
  * @qtd_list:           List of QTDs for this QH
  * @channel:            Host channel currently processing transfers for this QH
  * @qh_list_entry:      Entry for QH in either the periodic or non-periodic
@@ -350,6 +357,9 @@ struct dwc2_qh {
 	struct dwc2_hs_transfer_time hs_transfers[DWC2_HS_SCHEDULE_UFRAMES];
 	u32 ls_start_schedule_slice;
 	u16 ntd;
+	u8 *dw_align_buf;
+	int dw_align_buf_size;
+	dma_addr_t dw_align_buf_dma;
 	struct list_head qtd_list;
 	struct dwc2_host_chan *channel;
 	struct list_head qh_list_entry;
diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
index a5dfd9d..5e2378f 100644
--- a/drivers/usb/dwc2/hcd_intr.c
+++ b/drivers/usb/dwc2/hcd_intr.c
@@ -938,6 +938,14 @@ static int dwc2_xfercomp_isoc_split_in(struct dwc2_hsotg *hsotg,
 
 	frame_desc->actual_length += len;
 
+	if (chan->align_buf) {
+		dev_vdbg(hsotg->dev, "%s(): non-aligned buffer\n", __func__);
+		dma_unmap_single(hsotg->dev, chan->qh->dw_align_buf_dma,
+				 chan->qh->dw_align_buf_size, DMA_FROM_DEVICE);
+		memcpy(qtd->urb->buf + frame_desc->offset +
+		       qtd->isoc_split_offset, chan->qh->dw_align_buf, len);
+	}
+
 	qtd->isoc_split_offset += len;
 
 	hctsiz = dwc2_readl(hsotg->regs + HCTSIZ(chnum));
diff --git a/drivers/usb/dwc2/hcd_queue.c b/drivers/usb/dwc2/hcd_queue.c
index e34ad5e..a120bb0 100644
--- a/drivers/usb/dwc2/hcd_queue.c
+++ b/drivers/usb/dwc2/hcd_queue.c
@@ -1693,8 +1693,14 @@ void dwc2_hcd_qh_free(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh)
 
 	dwc2_host_put_tt_info(hsotg, qh->dwc_tt);
 
-	if (qh->desc_list)
+	if (qh->desc_list) {
 		dwc2_hcd_qh_free_ddma(hsotg, qh);
+	} else {
+		/* kfree(NULL) is safe */
+		kfree(qh->dw_align_buf);
+		qh->dw_align_buf_dma = (dma_addr_t)0;
+	}
+
 	kfree(qh);
 }
 
-- 
2.0.0

Re: [PATCH v2 1/2] usb: dwc2: alloc dma aligned buffer for isoc split in

[Doug Anderson]

Hi,

On Tue, May 1, 2018 at 8:04 PM, William Wu <william.wu@rock-chips.com> wrote:
> The commit 3bc04e28a030 ("usb: dwc2: host: Get aligned DMA in
> a more supported way") rips out a lot of code to simply the
> allocation of aligned DMA. However, it also introduces a new
> issue when use isoc split in transfer.
>
> In my test case, I connect the dwc2 controller with an usb hs
> Hub (GL852G-12), and plug an usb fs audio device (Plantronics
> headset) into the downstream port of Hub. Then use the usb mic
> to record, we can find noise when playback.
>
> It's because that the usb Hub uses an MDATA for the first
> transaction and a DATA0 for the second transaction for the isoc
> split in transaction. An typical isoc split in transaction sequence
> like this:
>
> - SSPLIT IN transaction
> - CSPLIT IN transaction
>   - MDATA packet
> - CSPLIT IN transaction
>   - DATA0 packet
>
> The DMA address of MDATA (urb->dma) is always DWORD-aligned, but
> the DMA address of DATA0 (urb->dma + qtd->isoc_split_offset) may
> not be DWORD-aligned, it depends on the qtd->isoc_split_offset (the
> length of MDATA). In my test case, the length of MDATA is usually
> unaligned, this casue DATA0 packet transmission error.
>
> This patch base on the old way of aligned DMA allocation in the
> dwc2 driver to get aligned DMA for isoc split in.
>
> Signed-off-by: William Wu <william.wu@rock-chips.com>
> ---
> Changes in v2:
> - None
>
>  drivers/usb/dwc2/hcd.c       | 63 +++++++++++++++++++++++++++++++++++++++++---
>  drivers/usb/dwc2/hcd.h       | 10 +++++++
>  drivers/usb/dwc2/hcd_intr.c  |  8 ++++++
>  drivers/usb/dwc2/hcd_queue.c |  8 +++++-
>  4 files changed, 85 insertions(+), 4 deletions(-)

A word of warning that I'm pretty rusty on dwc2 and even when I was
making lots of patches I still considered myself a bit clueless.
...so if something seems wrong, please call me on it...


> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index 190f959..8c2b35f 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -1562,11 +1562,20 @@ static void dwc2_hc_start_transfer(struct dwc2_hsotg *hsotg,
>         }
>
>         if (hsotg->params.host_dma) {
> -               dwc2_writel((u32)chan->xfer_dma,
> -                           hsotg->regs + HCDMA(chan->hc_num));
> +               dma_addr_t dma_addr;
> +
> +               if (chan->align_buf) {
> +                       if (dbg_hc(chan))
> +                               dev_vdbg(hsotg->dev, "align_buf\n");
> +                       dma_addr = chan->align_buf;
> +               } else {
> +                       dma_addr = chan->xfer_dma;
> +               }
> +               dwc2_writel((u32)dma_addr, hsotg->regs + HCDMA(chan->hc_num));
> +
>                 if (dbg_hc(chan))
>                         dev_vdbg(hsotg->dev, "Wrote %08lx to HCDMA(%d)\n",
> -                                (unsigned long)chan->xfer_dma, chan->hc_num);
> +                                (unsigned long)dma_addr, chan->hc_num);
>         }
>
>         /* Start the split */
> @@ -2620,6 +2629,33 @@ static void dwc2_hc_init_xfer(struct dwc2_hsotg *hsotg,
>         }
>  }
>
> +static int dwc2_alloc_split_dma_aligned_buf(struct dwc2_hsotg *hsotg,
> +                                           struct dwc2_qh *qh,
> +                                           struct dwc2_host_chan *chan)
> +{
> +       if (!qh->dw_align_buf) {
> +               qh->dw_align_buf = kmalloc(chan->max_packet,

So you're potentially doing a bounce buffer atop a bounce buffer now,
right?  That seems pretty non-optimal.  You're also back to doing a
kmalloc at interrupt time which I found was pretty bad for
performance.

Is there really no way you can do your memory allocation in
dwc2_alloc_dma_aligned_buffer() instead of here?  For input packets,
if you could just allocate an extra 3 bytes in the original bounce
buffer you could just re-use the original bounce buffer together with
a memmove?  AKA:

transfersize = 13 + 64;
buf = alloc(16 + 64);

// Do the first transfer, no problems.
dma_into(buf, 13);

// 2nd transfer isn't aligned, so align.
// we allocated a little extra to account for this
dma_into(buf + 16, 64);

// move back where it belongs.
memmove(buf + 13, buf + 16, 64);


To make the above work you'd need to still allocate a bounce buffer
even if the original "urb->transfer_buffer" was already aligned.
Ideally you'd be able to know when dwc2_alloc_dma_aligned_buffer()
that this is one of the special cases where you need a slightly
oversized bounce buffer.

---

If you somehow need to do something for output, you'd do the opposite.
You'd copy backwards top already transferred data to align.


> +                                          GFP_ATOMIC | GFP_DMA);
> +               if (!qh->dw_align_buf)
> +                       return -ENOMEM;
> +
> +               qh->dw_align_buf_size = chan->max_packet;
> +       }
> +
> +       qh->dw_align_buf_dma = dma_map_single(hsotg->dev, qh->dw_align_buf,
> +                                             qh->dw_align_buf_size,
> +                                             DMA_FROM_DEVICE);
> +
> +       if (dma_mapping_error(hsotg->dev, qh->dw_align_buf_dma)) {
> +               dev_err(hsotg->dev, "can't map align_buf\n");
> +               chan->align_buf = 0;
> +               return -EINVAL;
> +       }
> +
> +       chan->align_buf = qh->dw_align_buf_dma;
> +       return 0;
> +}
> +
>  #define DWC2_USB_DMA_ALIGN 4
>
>  struct dma_aligned_buffer {
> @@ -2797,6 +2833,27 @@ static int dwc2_assign_and_init_hc(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh)
>         /* Set the transfer attributes */
>         dwc2_hc_init_xfer(hsotg, chan, qtd);
>
> +       /* For non-dword aligned buffers */
> +       if (hsotg->params.host_dma > 0 && qh->do_split &&
> +           chan->ep_is_in && (chan->xfer_dma & 0x3)) {

So what happens if were unaligned (AKA (chan->xfer_dma & 0x3)) but
we're not doing split or it's not an IN EP?  Do we just fail then?

I guess the rest of this patch only handles the "in" case and maybe
you expect that the problems will only come about for do_split, but it
still might be wise to at least print a warning in the other cases?
From reading dwc2_hc_init_xfer() it seems like you could run into this
same problem in the "out" case?


> +               dev_vdbg(hsotg->dev, "Non-aligned buffer\n");
> +               if (dwc2_alloc_split_dma_aligned_buf(hsotg, qh, chan)) {
> +                       dev_err(hsotg->dev,
> +                               "%s: Failed to allocate memory to handle non-dword aligned buffer\n",
> +                               __func__);

Here and elsewhere in this patch: In general I think policy is that
you shouldn't include __func__ for dev_err().  The string together
with the name of the device is supposed to uniquely determine where
you are.


> +                       /* Add channel back to free list */
> +                       chan->align_buf = 0;
> +                       chan->multi_count = 0;
> +                       list_add_tail(&chan->hc_list_entry,
> +                                     &hsotg->free_hc_list);
> +                       qtd->in_process = 0;
> +                       qh->channel = NULL;
> +                       return -ENOMEM;
> +               }
> +       } else {
> +               chan->align_buf = 0;
> +       }
> +
>         if (chan->ep_type == USB_ENDPOINT_XFER_INT ||
>             chan->ep_type == USB_ENDPOINT_XFER_ISOC)
>                 /*
> diff --git a/drivers/usb/dwc2/hcd.h b/drivers/usb/dwc2/hcd.h
> index 96a9da5..adf1fd0 100644
> --- a/drivers/usb/dwc2/hcd.h
> +++ b/drivers/usb/dwc2/hcd.h
> @@ -76,6 +76,8 @@ struct dwc2_qh;
>   *                      (micro)frame
>   * @xfer_buf:           Pointer to current transfer buffer position
>   * @xfer_dma:           DMA address of xfer_buf
> + * @align_buf:         In Buffer DMA mode this will be used if xfer_buf is not
> + *                     DWORD aligned

Your patch uses tabs but everything else in this comment uses spaces.
Please be consistent with surrounding code.  It looks like I may have
screwed this up in the past on the comment of "struct dwc2_qh", but
that's no reason to mess it up again...


>   * @xfer_len:           Total number of bytes to transfer
>   * @xfer_count:         Number of bytes transferred so far
>   * @start_pkt_count:    Packet count at start of transfer
> @@ -133,6 +135,7 @@ struct dwc2_host_chan {
>
>         u8 *xfer_buf;
>         dma_addr_t xfer_dma;
> +       dma_addr_t align_buf;
>         u32 xfer_len;
>         u32 xfer_count;
>         u16 start_pkt_count;
> @@ -303,6 +306,10 @@ struct dwc2_hs_transfer_time {
>   *                           is tightly packed.
>   * @ls_duration_us:     Duration on the low speed bus schedule.
>   * @ntd:                Actual number of transfer descriptors in a list
> + * @dw_align_buf:      Used instead of original buffer if its physical address
> + *                     is not dword-aligned
> + * @dw_align_buf_size: Size of dw_align_buf
> + * @dw_align_buf_dma:  DMA address for dw_align_buf
>   * @qtd_list:           List of QTDs for this QH
>   * @channel:            Host channel currently processing transfers for this QH
>   * @qh_list_entry:      Entry for QH in either the periodic or non-periodic
> @@ -350,6 +357,9 @@ struct dwc2_qh {
>         struct dwc2_hs_transfer_time hs_transfers[DWC2_HS_SCHEDULE_UFRAMES];
>         u32 ls_start_schedule_slice;
>         u16 ntd;
> +       u8 *dw_align_buf;
> +       int dw_align_buf_size;
> +       dma_addr_t dw_align_buf_dma;
>         struct list_head qtd_list;
>         struct dwc2_host_chan *channel;
>         struct list_head qh_list_entry;
> diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
> index a5dfd9d..5e2378f 100644
> --- a/drivers/usb/dwc2/hcd_intr.c
> +++ b/drivers/usb/dwc2/hcd_intr.c
> @@ -938,6 +938,14 @@ static int dwc2_xfercomp_isoc_split_in(struct dwc2_hsotg *hsotg,
>
>         frame_desc->actual_length += len;
>
> +       if (chan->align_buf) {
> +               dev_vdbg(hsotg->dev, "%s(): non-aligned buffer\n", __func__);
> +               dma_unmap_single(hsotg->dev, chan->qh->dw_align_buf_dma,
> +                                chan->qh->dw_align_buf_size, DMA_FROM_DEVICE);
> +               memcpy(qtd->urb->buf + frame_desc->offset +
> +                      qtd->isoc_split_offset, chan->qh->dw_align_buf, len);

Assuming I'm understanding this patch correctly, I think it would be
better to write:

  memcpy(qtd->xfer_dma, chan->qh->dw_align_buf, len);

Then if you ever end up having to align a transfer other than a split
you won't be doing the wrong math.  As it is it's very non-obvious
that you're hardcoding the same formula that's in dwc2_hc_init_xfer()



> +       }
> +
>         qtd->isoc_split_offset += len;
>
>         hctsiz = dwc2_readl(hsotg->regs + HCTSIZ(chnum));
> diff --git a/drivers/usb/dwc2/hcd_queue.c b/drivers/usb/dwc2/hcd_queue.c
> index e34ad5e..a120bb0 100644
> --- a/drivers/usb/dwc2/hcd_queue.c
> +++ b/drivers/usb/dwc2/hcd_queue.c
> @@ -1693,8 +1693,14 @@ void dwc2_hcd_qh_free(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh)
>
>         dwc2_host_put_tt_info(hsotg, qh->dwc_tt);
>
> -       if (qh->desc_list)
> +       if (qh->desc_list) {
>                 dwc2_hcd_qh_free_ddma(hsotg, qh);
> +       } else {
> +               /* kfree(NULL) is safe */
> +               kfree(qh->dw_align_buf);
> +               qh->dw_align_buf_dma = (dma_addr_t)0;

Why assign qh->dw_align_buf_dma to 0?  The next thing you're doing is
kfree(qh).  If you want extra debugging, turn on slub_debug.


> +       }
> +
>         kfree(qh);
>  }

Re: [PATCH v2 1/2] usb: dwc2: alloc dma aligned buffer for isoc split in

[wlf]

Dear Doug,


在 2018年05月02日 12:33, Doug Anderson 写道:
> Hi,
>
> On Tue, May 1, 2018 at 8:04 PM, William Wu <william.wu@rock-chips.com> wrote:
>> The commit 3bc04e28a030 ("usb: dwc2: host: Get aligned DMA in
>> a more supported way") rips out a lot of code to simply the
>> allocation of aligned DMA. However, it also introduces a new
>> issue when use isoc split in transfer.
>>
>> In my test case, I connect the dwc2 controller with an usb hs
>> Hub (GL852G-12), and plug an usb fs audio device (Plantronics
>> headset) into the downstream port of Hub. Then use the usb mic
>> to record, we can find noise when playback.
>>
>> It's because that the usb Hub uses an MDATA for the first
>> transaction and a DATA0 for the second transaction for the isoc
>> split in transaction. An typical isoc split in transaction sequence
>> like this:
>>
>> - SSPLIT IN transaction
>> - CSPLIT IN transaction
>>    - MDATA packet
>> - CSPLIT IN transaction
>>    - DATA0 packet
>>
>> The DMA address of MDATA (urb->dma) is always DWORD-aligned, but
>> the DMA address of DATA0 (urb->dma + qtd->isoc_split_offset) may
>> not be DWORD-aligned, it depends on the qtd->isoc_split_offset (the
>> length of MDATA). In my test case, the length of MDATA is usually
>> unaligned, this casue DATA0 packet transmission error.
>>
>> This patch base on the old way of aligned DMA allocation in the
>> dwc2 driver to get aligned DMA for isoc split in.
>>
>> Signed-off-by: William Wu <william.wu@rock-chips.com>
>> ---
>> Changes in v2:
>> - None
>>
>>   drivers/usb/dwc2/hcd.c       | 63 +++++++++++++++++++++++++++++++++++++++++---
>>   drivers/usb/dwc2/hcd.h       | 10 +++++++
>>   drivers/usb/dwc2/hcd_intr.c  |  8 ++++++
>>   drivers/usb/dwc2/hcd_queue.c |  8 +++++-
>>   4 files changed, 85 insertions(+), 4 deletions(-)
> A word of warning that I'm pretty rusty on dwc2 and even when I was
> making lots of patches I still considered myself a bit clueless.
> ...so if something seems wrong, please call me on it...
Most of your suggestions are great, thanks very much！
>
>> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
>> index 190f959..8c2b35f 100644
>> --- a/drivers/usb/dwc2/hcd.c
>> +++ b/drivers/usb/dwc2/hcd.c
>> @@ -1562,11 +1562,20 @@ static void dwc2_hc_start_transfer(struct dwc2_hsotg *hsotg,
>>          }
>>
>>          if (hsotg->params.host_dma) {
>> -               dwc2_writel((u32)chan->xfer_dma,
>> -                           hsotg->regs + HCDMA(chan->hc_num));
>> +               dma_addr_t dma_addr;
>> +
>> +               if (chan->align_buf) {
>> +                       if (dbg_hc(chan))
>> +                               dev_vdbg(hsotg->dev, "align_buf\n");
>> +                       dma_addr = chan->align_buf;
>> +               } else {
>> +                       dma_addr = chan->xfer_dma;
>> +               }
>> +               dwc2_writel((u32)dma_addr, hsotg->regs + HCDMA(chan->hc_num));
>> +
>>                  if (dbg_hc(chan))
>>                          dev_vdbg(hsotg->dev, "Wrote %08lx to HCDMA(%d)\n",
>> -                                (unsigned long)chan->xfer_dma, chan->hc_num);
>> +                                (unsigned long)dma_addr, chan->hc_num);
>>          }
>>
>>          /* Start the split */
>> @@ -2620,6 +2629,33 @@ static void dwc2_hc_init_xfer(struct dwc2_hsotg *hsotg,
>>          }
>>   }
>>
>> +static int dwc2_alloc_split_dma_aligned_buf(struct dwc2_hsotg *hsotg,
>> +                                           struct dwc2_qh *qh,
>> +                                           struct dwc2_host_chan *chan)
>> +{
>> +       if (!qh->dw_align_buf) {
>> +               qh->dw_align_buf = kmalloc(chan->max_packet,
> So you're potentially doing a bounce buffer atop a bounce buffer now,
> right?  That seems pretty non-optimal.  You're also back to doing a
> kmalloc at interrupt time which I found was pretty bad for
> performance.
Yes, I just allocate an additional bounce buffer here. I haven't thought 
consummately
about this patch, it's really not a good way to use a kmalloc at 
interrut time.
> Is there really no way you can do your memory allocation in
> dwc2_alloc_dma_aligned_buffer() instead of here?  For input packets,
> if you could just allocate an extra 3 bytes in the original bounce
> buffer you could just re-use the original bounce buffer together with
> a memmove?  AKA:
>
> transfersize = 13 + 64;
> buf = alloc(16 + 64);
>
> // Do the first transfer, no problems.
> dma_into(buf, 13);
>
> // 2nd transfer isn't aligned, so align.
> // we allocated a little extra to account for this
> dma_into(buf + 16, 64);
>
> // move back where it belongs.
> memmove(buf + 13, buf + 16, 64);
>
>
> To make the above work you'd need to still allocate a bounce buffer
> even if the original "urb->transfer_buffer" was already aligned.
> Ideally you'd be able to know when dwc2_alloc_dma_aligned_buffer()
> that this is one of the special cases where you need a slightly
> oversized bounce buffer.
It's a good way to allocate an extra 3 bytes in the original bounce 
buffer for this unaligned
issue， it's similar to the tailroom of sk_buff. However, just as you 
said, we'd better find
the special cases where we need an oversized bounce buffer, otherwise，we 
need to allocate
a bounce buffer for all of urbs.

It's hard for me to know the special cases in the 
dwc2_alloc_dma_aligned_buffer(), because
it's called from usb_submit_urb() in the device class driver, and I 
hardly know the split state
in this process, much less if the split transaction need aligned buffer. 
Do you have any idea?

I suppose that we can't find the special cases where we need an 
oversized bounce buffer
in the dwc2_alloc_dma_aligned_buffer(), and if we still want to re-use 
the original bounce
buffer with extra 3 bytes, then we need to allocate a  bounce buffer for 
all of urbs, and do
unnecessary  data copy for these urbs  whose transfer_buffer were 
already aligned.  This
may reduce the transmission rate of USB.

Can we just pre-allocate an additional aligned buffer (the size is 200 
bytes) for split transaction
in dwc2_map_urb_for_dma for all of urbs. And if we find the split 
transaction is unaligned，
we can easily use the pre-allocated aligned buffer.
>
> ---
>
> If you somehow need to do something for output, you'd do the opposite.
> You'd copy backwards top already transferred data to align.
>
>
>> +                                          GFP_ATOMIC | GFP_DMA);
>> +               if (!qh->dw_align_buf)
>> +                       return -ENOMEM;
>> +
>> +               qh->dw_align_buf_size = chan->max_packet;
>> +       }
>> +
>> +       qh->dw_align_buf_dma = dma_map_single(hsotg->dev, qh->dw_align_buf,
>> +                                             qh->dw_align_buf_size,
>> +                                             DMA_FROM_DEVICE);
>> +
>> +       if (dma_mapping_error(hsotg->dev, qh->dw_align_buf_dma)) {
>> +               dev_err(hsotg->dev, "can't map align_buf\n");
>> +               chan->align_buf = 0;
>> +               return -EINVAL;
>> +       }
>> +
>> +       chan->align_buf = qh->dw_align_buf_dma;
>> +       return 0;
>> +}
>> +
>>   #define DWC2_USB_DMA_ALIGN 4
>>
>>   struct dma_aligned_buffer {
>> @@ -2797,6 +2833,27 @@ static int dwc2_assign_and_init_hc(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh)
>>          /* Set the transfer attributes */
>>          dwc2_hc_init_xfer(hsotg, chan, qtd);
>>
>> +       /* For non-dword aligned buffers */
>> +       if (hsotg->params.host_dma > 0 && qh->do_split &&
>> +           chan->ep_is_in && (chan->xfer_dma & 0x3)) {
> So what happens if were unaligned (AKA (chan->xfer_dma & 0x3)) but
> we're not doing split or it's not an IN EP?  Do we just fail then?
>
> I guess the rest of this patch only handles the "in" case and maybe
> you expect that the problems will only come about for do_split, but it
> still might be wise to at least print a warning in the other cases?
> >From reading dwc2_hc_init_xfer() it seems like you could run into this
> same problem in the "out" case?
Actually, I only find non-dword aligned issue in the case of split in 
transaction.
And I think that if we're not doing split or it's an OUT EP, we can 
always get aligned buffer
in the current code. For non-split case, the 
dwc2_alloc_dma_aligned_buffer()
is enough. And for split out case, if the transaction is subdivided into 
multiple start-splits,
each with a data payload of 188 bytes or less, so the DMA address is 
always aligned.

>
>
>> +               dev_vdbg(hsotg->dev, "Non-aligned buffer\n");
>> +               if (dwc2_alloc_split_dma_aligned_buf(hsotg, qh, chan)) {
>> +                       dev_err(hsotg->dev,
>> +                               "%s: Failed to allocate memory to handle non-dword aligned buffer\n",
>> +                               __func__);
> Here and elsewhere in this patch: In general I think policy is that
> you shouldn't include __func__ for dev_err().  The string together
> with the name of the device is supposed to uniquely determine where
> you are.
Ah, I got it. I will fix it in next patch.
>> +                       /* Add channel back to free list */
>> +                       chan->align_buf = 0;
>> +                       chan->multi_count = 0;
>> +                       list_add_tail(&chan->hc_list_entry,
>> +                                     &hsotg->free_hc_list);
>> +                       qtd->in_process = 0;
>> +                       qh->channel = NULL;
>> +                       return -ENOMEM;
>> +               }
>> +       } else {
>> +               chan->align_buf = 0;
>> +       }
>> +
>>          if (chan->ep_type == USB_ENDPOINT_XFER_INT ||
>>              chan->ep_type == USB_ENDPOINT_XFER_ISOC)
>>                  /*
>> diff --git a/drivers/usb/dwc2/hcd.h b/drivers/usb/dwc2/hcd.h
>> index 96a9da5..adf1fd0 100644
>> --- a/drivers/usb/dwc2/hcd.h
>> +++ b/drivers/usb/dwc2/hcd.h
>> @@ -76,6 +76,8 @@ struct dwc2_qh;
>>    *                      (micro)frame
>>    * @xfer_buf:           Pointer to current transfer buffer position
>>    * @xfer_dma:           DMA address of xfer_buf
>> + * @align_buf:         In Buffer DMA mode this will be used if xfer_buf is not
>> + *                     DWORD aligned
> Your patch uses tabs but everything else in this comment uses spaces.
> Please be consistent with surrounding code.  It looks like I may have
> screwed this up in the past on the comment of "struct dwc2_qh", but
> that's no reason to mess it up again...
Ah, I got it. I will fix it in next patch.
>
>>    * @xfer_len:           Total number of bytes to transfer
>>    * @xfer_count:         Number of bytes transferred so far
>>    * @start_pkt_count:    Packet count at start of transfer
>> @@ -133,6 +135,7 @@ struct dwc2_host_chan {
>>
>>          u8 *xfer_buf;
>>          dma_addr_t xfer_dma;
>> +       dma_addr_t align_buf;
>>          u32 xfer_len;
>>          u32 xfer_count;
>>          u16 start_pkt_count;
>> @@ -303,6 +306,10 @@ struct dwc2_hs_transfer_time {
>>    *                           is tightly packed.
>>    * @ls_duration_us:     Duration on the low speed bus schedule.
>>    * @ntd:                Actual number of transfer descriptors in a list
>> + * @dw_align_buf:      Used instead of original buffer if its physical address
>> + *                     is not dword-aligned
>> + * @dw_align_buf_size: Size of dw_align_buf
>> + * @dw_align_buf_dma:  DMA address for dw_align_buf
>>    * @qtd_list:           List of QTDs for this QH
>>    * @channel:            Host channel currently processing transfers for this QH
>>    * @qh_list_entry:      Entry for QH in either the periodic or non-periodic
>> @@ -350,6 +357,9 @@ struct dwc2_qh {
>>          struct dwc2_hs_transfer_time hs_transfers[DWC2_HS_SCHEDULE_UFRAMES];
>>          u32 ls_start_schedule_slice;
>>          u16 ntd;
>> +       u8 *dw_align_buf;
>> +       int dw_align_buf_size;
>> +       dma_addr_t dw_align_buf_dma;
>>          struct list_head qtd_list;
>>          struct dwc2_host_chan *channel;
>>          struct list_head qh_list_entry;
>> diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
>> index a5dfd9d..5e2378f 100644
>> --- a/drivers/usb/dwc2/hcd_intr.c
>> +++ b/drivers/usb/dwc2/hcd_intr.c
>> @@ -938,6 +938,14 @@ static int dwc2_xfercomp_isoc_split_in(struct dwc2_hsotg *hsotg,
>>
>>          frame_desc->actual_length += len;
>>
>> +       if (chan->align_buf) {
>> +               dev_vdbg(hsotg->dev, "%s(): non-aligned buffer\n", __func__);
>> +               dma_unmap_single(hsotg->dev, chan->qh->dw_align_buf_dma,
>> +                                chan->qh->dw_align_buf_size, DMA_FROM_DEVICE);
>> +               memcpy(qtd->urb->buf + frame_desc->offset +
>> +                      qtd->isoc_split_offset, chan->qh->dw_align_buf, len);
> Assuming I'm understanding this patch correctly, I think it would be
> better to write:
>
>    memcpy(qtd->xfer_dma, chan->qh->dw_align_buf, len);
Sorry, there's no "xfer_buf" in qtd, do you means the "chan->xfer_dma"? 
If it's, I think we can't
do memcpy from a transfer buffer to a DMA address. Maybe chan->xfer_buf 
is more suitable,
but it seems that the dwc2 driver doesn't update the chan->xfer_buf for 
isoc transfer with dma
enabled in dwc2_hc_init_xfer().
>
> Then if you ever end up having to align a transfer other than a split
> you won't be doing the wrong math.  As it is it's very non-obvious
> that you're hardcoding the same formula that's in dwc2_hc_init_xfer()
Actually, I'm hardcoding the same formula from the old code which has 
been ripped out
in the commit 3bc04e28a030 ("usb: dwc2: host: Get aligned DMA in a more 
supported way").
>
>
>> +       }
>> +
>>          qtd->isoc_split_offset += len;
>>
>>          hctsiz = dwc2_readl(hsotg->regs + HCTSIZ(chnum));
>> diff --git a/drivers/usb/dwc2/hcd_queue.c b/drivers/usb/dwc2/hcd_queue.c
>> index e34ad5e..a120bb0 100644
>> --- a/drivers/usb/dwc2/hcd_queue.c
>> +++ b/drivers/usb/dwc2/hcd_queue.c
>> @@ -1693,8 +1693,14 @@ void dwc2_hcd_qh_free(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh)
>>
>>          dwc2_host_put_tt_info(hsotg, qh->dwc_tt);
>>
>> -       if (qh->desc_list)
>> +       if (qh->desc_list) {
>>                  dwc2_hcd_qh_free_ddma(hsotg, qh);
>> +       } else {
>> +               /* kfree(NULL) is safe */
>> +               kfree(qh->dw_align_buf);
>> +               qh->dw_align_buf_dma = (dma_addr_t)0;
> Why assign qh->dw_align_buf_dma to 0?  The next thing you're doing is
> kfree(qh).  If you want extra debugging, turn on slub_debug.
I just copy the same code from the old code which has been ripped out in the
commit 3bc04e28a030 ("usb: dwc2: host: Get aligned DMA in a more 
supported way").
I really don't know why assign qh->dw_align_buf_dma to 0.
>
>
>> +       }
>> +
>>          kfree(qh);
>>   }
>
>

Re: [PATCH v2 1/2] usb: dwc2: alloc dma aligned buffer for isoc split in

[Doug Anderson]

Hi,

On Wed, May 2, 2018 at 10:14 AM, wlf <wulf@rock-chips.com> wrote:
> It's a good way to allocate an extra 3 bytes in the original bounce buffer
> for this unaligned
> issue， it's similar to the tailroom of sk_buff. However, just as you said,
> we'd better find
> the special cases where we need an oversized bounce buffer, otherwise，we
> need to allocate
> a bounce buffer for all of urbs.
>
> It's hard for me to know the special cases in the
> dwc2_alloc_dma_aligned_buffer(), because
> it's called from usb_submit_urb() in the device class driver, and I hardly
> know the split state
> in this process, much less if the split transaction need aligned buffer. Do
> you have any idea?
>
> I suppose that we can't find the special cases where we need an oversized
> bounce buffer
> in the dwc2_alloc_dma_aligned_buffer(), and if we still want to re-use the
> original bounce
> buffer with extra 3 bytes, then we need to allocate a  bounce buffer for all
> of urbs, and do
> unnecessary  data copy for these urbs  whose transfer_buffer were already
> aligned.  This
> may reduce the transmission rate of USB.
>
> Can we just pre-allocate an additional aligned buffer (the size is 200
> bytes) for split transaction
> in dwc2_map_urb_for_dma for all of urbs. And if we find the split
> transaction is unaligned，
> we can easily use the pre-allocated aligned buffer.

OK, so thinking about this more...

Previously things got really slow at interrupt time because we were
trying to allocate as much as 64K at interrupt time.  That wasn't so
great.  In your case, you're only allocating 200 bytes.  As I
understand things, allocating 200 bytes at interrupt time is probably
not a huge deal.

...so I guess it come down to a tradeoff here: is it worth eating 200
bytes for each URB to save an 200 byte allocation at interrupt time in
this one rare case.

I'd certainly welcome anyone's opinion here, but I'm going to go with
saying it's fine to allocate the 200 bytes at interrupt time (like
your patch does).  ...but, I _think_ you want to use
kmem_cache_create() to create a cache and then kmem_cache_zalloc().
Since all allocations are the same size and you want this to be fast,
I think using kmem_cache is good.



>>> +       /* For non-dword aligned buffers */
>>> +       if (hsotg->params.host_dma > 0 && qh->do_split &&
>>> +           chan->ep_is_in && (chan->xfer_dma & 0x3)) {
>>
>> So what happens if were unaligned (AKA (chan->xfer_dma & 0x3)) but
>> we're not doing split or it's not an IN EP?  Do we just fail then?
>>
>> I guess the rest of this patch only handles the "in" case and maybe
>> you expect that the problems will only come about for do_split, but it
>> still might be wise to at least print a warning in the other cases?
>> >From reading dwc2_hc_init_xfer() it seems like you could run into this
>> same problem in the "out" case?
>
> Actually, I only find non-dword aligned issue in the case of split in
> transaction.
> And I think that if we're not doing split or it's an OUT EP, we can always
> get aligned buffer
> in the current code. For non-split case, the dwc2_alloc_dma_aligned_buffer()
> is enough. And for split out case, if the transaction is subdivided into
> multiple start-splits,
> each with a data payload of 188 bytes or less, so the DMA address is always
> aligned.

Can you at least print an error message if you end up with non-aligned
DMA in one of the other cases?


>>> DMA_FROM_DEVICE);
>>> +               memcpy(qtd->urb->buf + frame_desc->offset +
>>> +                      qtd->isoc_split_offset, chan->qh->dw_align_buf,
>>> len);
>>
>> Assuming I'm understanding this patch correctly, I think it would be
>> better to write:
>>
>>    memcpy(qtd->xfer_dma, chan->qh->dw_align_buf, len);
>
> Sorry, there's no "xfer_buf" in qtd, do you means the "chan->xfer_dma"? If
> it's, I think we can't
> do memcpy from a transfer buffer to a DMA address. Maybe chan->xfer_buf is
> more suitable,
> but it seems that the dwc2 driver doesn't update the chan->xfer_buf for isoc
> transfer with dma
> enabled in dwc2_hc_init_xfer().

Yes, I meant chan->xfer_dma.  Ah, right.  xfer_dma is a DMA address.

I guess you could in theory you could do:

memcpy(qtd->urb->buf + (chan->xfer_dma - urb->dma),
    chan->qh->dw_align_buf);

That at least avoids duplicating the math.  Maybe either do that, or
if you don't like it at least add a comment saying that the math needs
to match the math in dwc2_hc_init_xfer().


>> Then if you ever end up having to align a transfer other than a split
>> you won't be doing the wrong math.  As it is it's very non-obvious
>> that you're hardcoding the same formula that's in dwc2_hc_init_xfer()
>
> Actually, I'm hardcoding the same formula from the old code which has been
> ripped out
> in the commit 3bc04e28a030 ("usb: dwc2: host: Get aligned DMA in a more
> supported way").

Ah, got it.  Well, I think the old code was just hardcoding the same
formula in two places then.  ;-)


>>> -       if (qh->desc_list)
>>> +       if (qh->desc_list) {
>>>                  dwc2_hcd_qh_free_ddma(hsotg, qh);
>>> +       } else {
>>> +               /* kfree(NULL) is safe */
>>> +               kfree(qh->dw_align_buf);
>>> +               qh->dw_align_buf_dma = (dma_addr_t)0;
>>
>> Why assign qh->dw_align_buf_dma to 0?  The next thing you're doing is
>> kfree(qh).  If you want extra debugging, turn on slub_debug.
>
> I just copy the same code from the old code which has been ripped out in the
> commit 3bc04e28a030 ("usb: dwc2: host: Get aligned DMA in a more supported
> way").
> I really don't know why assign qh->dw_align_buf_dma to 0.

There's no reason.  Just remove it.


-Doug

RE: [PATCH v2 1/2] usb: dwc2: alloc dma aligned buffer for isoc split in

[Allen Hsu (許嘉銘)]

Add more:


Best regards,
BU4 EE
Allen Hsu
QCI 886-3-327-2345 ext 15410

-----Original Message-----
From: Doug Anderson [mailto:dianders@google.com] 
Sent: Friday, May 4, 2018 11:58 PM
To: wlf <wulf@rock-chips.com>
Cc: William Wu <william.wu@rock-chips.com>; hminas@synopsys.com; felipe.balbi@linux.intel.com; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>; Heiko Stübner <heiko@sntech.de>; LKML <linux-kernel@vger.kernel.org>; linux-usb@vger.kernel.org; open list:ARM/Rockchip SoC... <linux-rockchip@lists.infradead.org>; Frank Wang <frank.wang@rock-chips.com>; 黄涛 <huangtao@rock-chips.com>; daniel.meng <daniel.meng@rock-chips.com>; John Youn <John.Youn@synopsys.com>; 王征增 <wzz@rock-chips.com>; zsq@rock-chips.com; Allen Hsu (許嘉銘) <Allen.Hsu@quantatw.com>; Stan Tsui <StanTsui@aopen.com>
Subject: Re: [PATCH v2 1/2] usb: dwc2: alloc dma aligned buffer for isoc split in

Hi,

On Wed, May 2, 2018 at 10:14 AM, wlf <wulf@rock-chips.com> wrote:
> It's a good way to allocate an extra 3 bytes in the original bounce 
> buffer for this unaligned
> issue， it's similar to the tailroom of sk_buff. However, just as you 
> said, we'd better find the special cases where we need an oversized 
> bounce buffer, otherwise，we
> need to allocate
> a bounce buffer for all of urbs.
>
> It's hard for me to know the special cases in the 
> dwc2_alloc_dma_aligned_buffer(), because it's called from 
> usb_submit_urb() in the device class driver, and I hardly know the 
> split state in this process, much less if the split transaction need 
> aligned buffer. Do you have any idea?
>
> I suppose that we can't find the special cases where we need an 
> oversized bounce buffer in the dwc2_alloc_dma_aligned_buffer(), and if 
> we still want to re-use the original bounce buffer with extra 3 bytes, 
> then we need to allocate a  bounce buffer for all of urbs, and do 
> unnecessary  data copy for these urbs  whose transfer_buffer were 
> already aligned.  This may reduce the transmission rate of USB.
>
> Can we just pre-allocate an additional aligned buffer (the size is 200
> bytes) for split transaction
> in dwc2_map_urb_for_dma for all of urbs. And if we find the split 
> transaction is unaligned，
> we can easily use the pre-allocated aligned buffer.

OK, so thinking about this more...

Previously things got really slow at interrupt time because we were trying to allocate as much as 64K at interrupt time.  That wasn't so great.  In your case, you're only allocating 200 bytes.  As I understand things, allocating 200 bytes at interrupt time is probably not a huge deal.

...so I guess it come down to a tradeoff here: is it worth eating 200 bytes for each URB to save an 200 byte allocation at interrupt time in this one rare case.

I'd certainly welcome anyone's opinion here, but I'm going to go with saying it's fine to allocate the 200 bytes at interrupt time (like your patch does).  ...but, I _think_ you want to use
kmem_cache_create() to create a cache and then kmem_cache_zalloc().
Since all allocations are the same size and you want this to be fast, I think using kmem_cache is good.



>>> +       /* For non-dword aligned buffers */
>>> +       if (hsotg->params.host_dma > 0 && qh->do_split &&
>>> +           chan->ep_is_in && (chan->xfer_dma & 0x3)) {
>>
>> So what happens if were unaligned (AKA (chan->xfer_dma & 0x3)) but 
>> we're not doing split or it's not an IN EP?  Do we just fail then?
>>
>> I guess the rest of this patch only handles the "in" case and maybe 
>> you expect that the problems will only come about for do_split, but 
>> it still might be wise to at least print a warning in the other cases?
>> >From reading dwc2_hc_init_xfer() it seems like you could run into 
>> >this
>> same problem in the "out" case?
>
> Actually, I only find non-dword aligned issue in the case of split in 
> transaction.
> And I think that if we're not doing split or it's an OUT EP, we can 
> always get aligned buffer in the current code. For non-split case, the 
> dwc2_alloc_dma_aligned_buffer() is enough. And for split out case, if 
> the transaction is subdivided into multiple start-splits, each with a 
> data payload of 188 bytes or less, so the DMA address is always 
> aligned.

Can you at least print an error message if you end up with non-aligned DMA in one of the other cases?


>>> DMA_FROM_DEVICE);
>>> +               memcpy(qtd->urb->buf + frame_desc->offset +
>>> +                      qtd->isoc_split_offset, 
>>> + chan->qh->dw_align_buf,
>>> len);
>>
>> Assuming I'm understanding this patch correctly, I think it would be 
>> better to write:
>>
>>    memcpy(qtd->xfer_dma, chan->qh->dw_align_buf, len);
>
> Sorry, there's no "xfer_buf" in qtd, do you means the 
> "chan->xfer_dma"? If it's, I think we can't do memcpy from a transfer 
> buffer to a DMA address. Maybe chan->xfer_buf is more suitable, but it 
> seems that the dwc2 driver doesn't update the chan->xfer_buf for isoc 
> transfer with dma enabled in dwc2_hc_init_xfer().

Yes, I meant chan->xfer_dma.  Ah, right.  xfer_dma is a DMA address.

I guess you could in theory you could do:

memcpy(qtd->urb->buf + (chan->xfer_dma - urb->dma),
    chan->qh->dw_align_buf);

That at least avoids duplicating the math.  Maybe either do that, or if you don't like it at least add a comment saying that the math needs to match the math in dwc2_hc_init_xfer().


>> Then if you ever end up having to align a transfer other than a split 
>> you won't be doing the wrong math.  As it is it's very non-obvious 
>> that you're hardcoding the same formula that's in dwc2_hc_init_xfer()
>
> Actually, I'm hardcoding the same formula from the old code which has 
> been ripped out in the commit 3bc04e28a030 ("usb: dwc2: host: Get 
> aligned DMA in a more supported way").

Ah, got it.  Well, I think the old code was just hardcoding the same formula in two places then.  ;-)


>>> -       if (qh->desc_list)
>>> +       if (qh->desc_list) {
>>>                  dwc2_hcd_qh_free_ddma(hsotg, qh);
>>> +       } else {
>>> +               /* kfree(NULL) is safe */
>>> +               kfree(qh->dw_align_buf);
>>> +               qh->dw_align_buf_dma = (dma_addr_t)0;
>>
>> Why assign qh->dw_align_buf_dma to 0?  The next thing you're doing is 
>> kfree(qh).  If you want extra debugging, turn on slub_debug.
>
> I just copy the same code from the old code which has been ripped out 
> in the commit 3bc04e28a030 ("usb: dwc2: host: Get aligned DMA in a 
> more supported way").
> I really don't know why assign qh->dw_align_buf_dma to 0.

There's no reason.  Just remove it.


-Doug
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v2 1/2] usb: dwc2: alloc dma aligned buffer for isoc split in

[wlf]

Dear Doug,

在 2018年05月07日 15:19, Allen Hsu (許嘉銘) 写道:
> Add more:
>
>
> Best regards,
> BU4 EE
> Allen Hsu
> QCI 886-3-327-2345 ext 15410
>
> -----Original Message-----
> From: Doug Anderson [mailto:dianders@google.com]
> Sent: Friday, May 4, 2018 11:58 PM
> To: wlf <wulf@rock-chips.com>
> Cc: William Wu <william.wu@rock-chips.com>; hminas@synopsys.com; felipe.balbi@linux.intel.com; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>; Heiko Stübner <heiko@sntech.de>; LKML <linux-kernel@vger.kernel.org>; linux-usb@vger.kernel.org; open list:ARM/Rockchip SoC... <linux-rockchip@lists.infradead.org>; Frank Wang <frank.wang@rock-chips.com>; 黄涛 <huangtao@rock-chips.com>; daniel.meng <daniel.meng@rock-chips.com>; John Youn <John.Youn@synopsys.com>; 王征增 <wzz@rock-chips.com>; zsq@rock-chips.com; Allen Hsu (許嘉銘) <Allen.Hsu@quantatw.com>; Stan Tsui <StanTsui@aopen.com>
> Subject: Re: [PATCH v2 1/2] usb: dwc2: alloc dma aligned buffer for isoc split in
>
> Hi,
>
> On Wed, May 2, 2018 at 10:14 AM, wlf <wulf@rock-chips.com> wrote:
>> It's a good way to allocate an extra 3 bytes in the original bounce
>> buffer for this unaligned
>> issue， it's similar to the tailroom of sk_buff. However, just as you
>> said, we'd better find the special cases where we need an oversized
>> bounce buffer, otherwise，we
>> need to allocate
>> a bounce buffer for all of urbs.
>>
>> It's hard for me to know the special cases in the
>> dwc2_alloc_dma_aligned_buffer(), because it's called from
>> usb_submit_urb() in the device class driver, and I hardly know the
>> split state in this process, much less if the split transaction need
>> aligned buffer. Do you have any idea?
>>
>> I suppose that we can't find the special cases where we need an
>> oversized bounce buffer in the dwc2_alloc_dma_aligned_buffer(), and if
>> we still want to re-use the original bounce buffer with extra 3 bytes,
>> then we need to allocate a  bounce buffer for all of urbs, and do
>> unnecessary  data copy for these urbs  whose transfer_buffer were
>> already aligned.  This may reduce the transmission rate of USB.
>>
>> Can we just pre-allocate an additional aligned buffer (the size is 200
>> bytes) for split transaction
>> in dwc2_map_urb_for_dma for all of urbs. And if we find the split
>> transaction is unaligned，
>> we can easily use the pre-allocated aligned buffer.
> OK, so thinking about this more...
>
> Previously things got really slow at interrupt time because we were trying to allocate as much as 64K at interrupt time.  That wasn't so great.  In your case, you're only allocating 200 bytes.  As I understand things, allocating 200 bytes at interrupt time is probably not a huge deal.
>
> ...so I guess it come down to a tradeoff here: is it worth eating 200 bytes for each URB to save an 200 byte allocation at interrupt time in this one rare case.
>
> I'd certainly welcome anyone's opinion here, but I'm going to go with saying it's fine to allocate the 200 bytes at interrupt time (like your patch does).  ...but, I _think_ you want to use
> kmem_cache_create() to create a cache and then kmem_cache_zalloc().
> Since all allocations are the same size and you want this to be fast, I think using kmem_cache is good.
Yes, it seems good to use kmem_cache. I'm trying to test a new patch 
with kmem_cache, and I will upstream v3 patch as soon as possible.
>>>> +       /* For non-dword aligned buffers */
>>>> +       if (hsotg->params.host_dma > 0 && qh->do_split &&
>>>> +           chan->ep_is_in && (chan->xfer_dma & 0x3)) {
>>> So what happens if were unaligned (AKA (chan->xfer_dma & 0x3)) but
>>> we're not doing split or it's not an IN EP?  Do we just fail then?
>>>
>>> I guess the rest of this patch only handles the "in" case and maybe
>>> you expect that the problems will only come about for do_split, but
>>> it still might be wise to at least print a warning in the other cases?
>>> >From reading dwc2_hc_init_xfer() it seems like you could run into 
>>>> this
>>> same problem in the "out" case?
>> Actually, I only find non-dword aligned issue in the case of split in
>> transaction.
>> And I think that if we're not doing split or it's an OUT EP, we can
>> always get aligned buffer in the current code. For non-split case, the
>> dwc2_alloc_dma_aligned_buffer() is enough. And for split out case, if
>> the transaction is subdivided into multiple start-splits, each with a
>> data payload of 188 bytes or less, so the DMA address is always
>> aligned.
> Can you at least print an error message if you end up with non-aligned DMA in one of the other cases?
That's an excellent suggestion. I will add warning message if end up 
with unexpected non-aligned DMA.
>>>> DMA_FROM_DEVICE);
>>>> +               memcpy(qtd->urb->buf + frame_desc->offset +
>>>> +                      qtd->isoc_split_offset,
>>>> + chan->qh->dw_align_buf,
>>>> len);
>>> Assuming I'm understanding this patch correctly, I think it would be
>>> better to write:
>>>
>>>     memcpy(qtd->xfer_dma, chan->qh->dw_align_buf, len);
>> Sorry, there's no "xfer_buf" in qtd, do you means the
>> "chan->xfer_dma"? If it's, I think we can't do memcpy from a transfer
>> buffer to a DMA address. Maybe chan->xfer_buf is more suitable, but it
>> seems that the dwc2 driver doesn't update the chan->xfer_buf for isoc
>> transfer with dma enabled in dwc2_hc_init_xfer().
> Yes, I meant chan->xfer_dma.  Ah, right.  xfer_dma is a DMA address.
>
> I guess you could in theory you could do:
>
> memcpy(qtd->urb->buf + (chan->xfer_dma - urb->dma),
>      chan->qh->dw_align_buf);
>
> That at least avoids duplicating the math.  Maybe either do that, or if you don't like it at least add a comment saying that the math needs to match the math in dwc2_hc_init_xfer().
Thanks, I will fix it in V3 patch according to your suggestion.
>>> Then if you ever end up having to align a transfer other than a split
>>> you won't be doing the wrong math.  As it is it's very non-obvious
>>> that you're hardcoding the same formula that's in dwc2_hc_init_xfer()
>> Actually, I'm hardcoding the same formula from the old code which has
>> been ripped out in the commit 3bc04e28a030 ("usb: dwc2: host: Get
>> aligned DMA in a more supported way").
> Ah, got it.  Well, I think the old code was just hardcoding the same formula in two places then.  ;-)
>
>
>>>> -       if (qh->desc_list)
>>>> +       if (qh->desc_list) {
>>>>                   dwc2_hcd_qh_free_ddma(hsotg, qh);
>>>> +       } else {
>>>> +               /* kfree(NULL) is safe */
>>>> +               kfree(qh->dw_align_buf);
>>>> +               qh->dw_align_buf_dma = (dma_addr_t)0;
>>> Why assign qh->dw_align_buf_dma to 0?  The next thing you're doing is
>>> kfree(qh).  If you want extra debugging, turn on slub_debug.
>> I just copy the same code from the old code which has been ripped out
>> in the commit 3bc04e28a030 ("usb: dwc2: host: Get aligned DMA in a
>> more supported way").
>> I really don't know why assign qh->dw_align_buf_dma to 0.
> There's no reason.  Just remove it.
OK, thank you for reminding me!
> -Doug
Best regards,
         wulf


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v2 2/2] usb: dwc2: fix isoc split in transfer with no data

[William Wu]

If isoc split in transfer with no data (the length of DATA0
packet is zero), we can't simply return immediately. Because
the DATA0 can be the first transaction or the second transaction
for the isoc split in transaction. If the DATA0 packet with no
data is in the first transaction, we can return immediately.
But if the DATA0 packet with no data is in the second transaction
of isoc split in transaction sequence, we need to increase the
qtd->isoc_frame_index and giveback urb to device driver if needed,
otherwise, the MDATA packet will be lost.

A typical test case is that connect the dwc2 controller with an
usb hs Hub (GL852G-12), and plug an usb fs audio device (Plantronics
headset) into the downstream port of Hub. Then use the usb mic
to record, we can find noise when playback.

In the case, the isoc split in transaction sequence like this:

- SSPLIT IN transaction
- CSPLIT IN transaction
  - MDATA packet (176 bytes)
- CSPLIT IN transaction
  - DATA0 packet (0 byte)

This patch use both the length of DATA0 and qtd->isoc_split_offset
to check if the DATA0 is in the second transaction.

Signed-off-by: William Wu <william.wu-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---
Changes in v2:
- Modify the commit message

 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 5e2378f..479f628 100644
--- a/drivers/usb/dwc2/hcd_intr.c
+++ b/drivers/usb/dwc2/hcd_intr.c
@@ -930,7 +930,7 @@ static int dwc2_xfercomp_isoc_split_in(struct dwc2_hsotg *hsotg,
 	frame_desc = &qtd->urb->iso_descs[qtd->isoc_frame_index];
 	len = dwc2_get_actual_xfer_length(hsotg, chan, chnum, qtd,
 					  DWC2_HC_XFER_COMPLETE, NULL);
-	if (!len) {
+	if (!len && !qtd->isoc_split_offset) {
 		qtd->complete_split = 0;
 		qtd->isoc_split_offset = 0;
 		return 0;
-- 
2.0.0

Re: [PATCH v2 2/2] usb: dwc2: fix isoc split in transfer with no data

[Doug Anderson]

Hi,

On Tue, May 1, 2018 at 8:04 PM, William Wu <william.wu@rock-chips.com> wrote:
> If isoc split in transfer with no data (the length of DATA0
> packet is zero), we can't simply return immediately. Because
> the DATA0 can be the first transaction or the second transaction
> for the isoc split in transaction. If the DATA0 packet with no
> data is in the first transaction, we can return immediately.
> But if the DATA0 packet with no data is in the second transaction
> of isoc split in transaction sequence, we need to increase the
> qtd->isoc_frame_index and giveback urb to device driver if needed,
> otherwise, the MDATA packet will be lost.
>
> A typical test case is that connect the dwc2 controller with an
> usb hs Hub (GL852G-12), and plug an usb fs audio device (Plantronics
> headset) into the downstream port of Hub. Then use the usb mic
> to record, we can find noise when playback.
>
> In the case, the isoc split in transaction sequence like this:
>
> - SSPLIT IN transaction
> - CSPLIT IN transaction
>   - MDATA packet (176 bytes)
> - CSPLIT IN transaction
>   - DATA0 packet (0 byte)
>
> This patch use both the length of DATA0 and qtd->isoc_split_offset
> to check if the DATA0 is in the second transaction.
>
> Signed-off-by: William Wu <william.wu@rock-chips.com>
> ---
> Changes in v2:
> - Modify the commit message
>
>  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 5e2378f..479f628 100644
> --- a/drivers/usb/dwc2/hcd_intr.c
> +++ b/drivers/usb/dwc2/hcd_intr.c
> @@ -930,7 +930,7 @@ static int dwc2_xfercomp_isoc_split_in(struct dwc2_hsotg *hsotg,
>         frame_desc = &qtd->urb->iso_descs[qtd->isoc_frame_index];
>         len = dwc2_get_actual_xfer_length(hsotg, chan, chnum, qtd,
>                                           DWC2_HC_XFER_COMPLETE, NULL);
> -       if (!len) {
> +       if (!len && !qtd->isoc_split_offset) {
>                 qtd->complete_split = 0;
>                 qtd->isoc_split_offset = 0;
>                 return 0;

I don't think my USB-fu is strong enough to do a real review of this
patch, but one small nitpick is that you can remove
"qtd->isoc_split_offset = 0" in the if test now.  AKA:

if (!len && !qtd->isoc_split_offset) {
  qtd->complete_split = 0;
  return 0;
}

...since you only enter the "if" test now when isoc_split_offset is
already 0...  Hopefully John Youn will have some time to comment on
this patch with more real USB knowledge...


-Doug

Re: [PATCH v2 2/2] usb: dwc2: fix isoc split in transfer with no data

[wlf]

Dear Doug,


在 2018年05月02日 13:02, Doug Anderson 写道:
> Hi,
>
> On Tue, May 1, 2018 at 8:04 PM, William Wu <william.wu@rock-chips.com> wrote:
>> If isoc split in transfer with no data (the length of DATA0
>> packet is zero), we can't simply return immediately. Because
>> the DATA0 can be the first transaction or the second transaction
>> for the isoc split in transaction. If the DATA0 packet with no
>> data is in the first transaction, we can return immediately.
>> But if the DATA0 packet with no data is in the second transaction
>> of isoc split in transaction sequence, we need to increase the
>> qtd->isoc_frame_index and giveback urb to device driver if needed,
>> otherwise, the MDATA packet will be lost.
>>
>> A typical test case is that connect the dwc2 controller with an
>> usb hs Hub (GL852G-12), and plug an usb fs audio device (Plantronics
>> headset) into the downstream port of Hub. Then use the usb mic
>> to record, we can find noise when playback.
>>
>> In the case, the isoc split in transaction sequence like this:
>>
>> - SSPLIT IN transaction
>> - CSPLIT IN transaction
>>    - MDATA packet (176 bytes)
>> - CSPLIT IN transaction
>>    - DATA0 packet (0 byte)
>>
>> This patch use both the length of DATA0 and qtd->isoc_split_offset
>> to check if the DATA0 is in the second transaction.
>>
>> Signed-off-by: William Wu <william.wu@rock-chips.com>
>> ---
>> Changes in v2:
>> - Modify the commit message
>>
>>   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 5e2378f..479f628 100644
>> --- a/drivers/usb/dwc2/hcd_intr.c
>> +++ b/drivers/usb/dwc2/hcd_intr.c
>> @@ -930,7 +930,7 @@ static int dwc2_xfercomp_isoc_split_in(struct dwc2_hsotg *hsotg,
>>          frame_desc = &qtd->urb->iso_descs[qtd->isoc_frame_index];
>>          len = dwc2_get_actual_xfer_length(hsotg, chan, chnum, qtd,
>>                                            DWC2_HC_XFER_COMPLETE, NULL);
>> -       if (!len) {
>> +       if (!len && !qtd->isoc_split_offset) {
>>                  qtd->complete_split = 0;
>>                  qtd->isoc_split_offset = 0;
>>                  return 0;
> I don't think my USB-fu is strong enough to do a real review of this
> patch, but one small nitpick is that you can remove
> "qtd->isoc_split_offset = 0" in the if test now.  AKA:
>
> if (!len && !qtd->isoc_split_offset) {
>    qtd->complete_split = 0;
>    return 0;
> }
Yes, good idea! I will fix it in next patch. Thank you!
>
> ...since you only enter the "if" test now when isoc_split_offset is
> already 0...  Hopefully John Youn will have some time to comment on
> this patch with more real USB knowledge...
>
>
> -Doug
>
Best regards,
      wulf
>