* [PATCH v4 0/2] usb: dwc2: fix isoc split in transfer issue @ 2018-05-09 10:10 William Wu 2018-05-09 10:11 ` [v4,1/2] " William Wu 2018-05-09 10:11 ` [v4,2/2] " William Wu 0 siblings, 2 replies; 11+ messages in thread From: William Wu @ 2018-05-09 10:10 UTC (permalink / raw) To: hminas, felipe.balbi, gregkh Cc: sergei.shtylyov, heiko, linux-kernel, linux-usb, linux-rockchip, frank.wang, huangtao, dianders, daniel.meng, John.Youn, william.wu, wzz, zsq, Allen.Hsu, StanTsui, Spruce.Wu, Martin.Tsai, Kevin.Shai, Mon-Jer.Wu, Claud.Chang, San.Lin, Ren.Kuo, davidhtwang, fonglin, stevencheng, tomchen, donchang, milesschofield 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/core.h | 3 ++ drivers/usb/dwc2/hcd.c | 87 ++++++++++++++++++++++++++++++++++++++++++-- drivers/usb/dwc2/hcd.h | 8 ++++ drivers/usb/dwc2/hcd_intr.c | 11 +++++- drivers/usb/dwc2/hcd_queue.c | 3 ++ 5 files changed, 106 insertions(+), 6 deletions(-) -- 2.0.0 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v4 1/2] usb: dwc2: alloc dma aligned buffer for isoc split in @ 2018-05-09 10:11 ` William Wu 0 siblings, 0 replies; 11+ messages in thread From: William Wu @ 2018-05-09 10:11 UTC (permalink / raw) To: hminas, felipe.balbi, gregkh Cc: sergei.shtylyov, heiko, linux-kernel, linux-usb, linux-rockchip, frank.wang, huangtao, dianders, daniel.meng, John.Youn, william.wu, wzz, zsq, Allen.Hsu, StanTsui, Spruce.Wu, Martin.Tsai, Kevin.Shai, Mon-Jer.Wu, Claud.Chang, San.Lin, Ren.Kuo, davidhtwang, fonglin, stevencheng, tomchen, donchang, milesschofield 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 cause DATA0 packet transmission error. This patch use kmem_cache to allocate aligned DMA buf for isoc split in transaction. Note that according to usb 2.0 spec, the maximum data payload size is 1023 bytes for each fs isoc ep, and the maximum allowable interrupt data payload size is 64 bytes or less for fs interrupt ep. So we set the size of object to be 1024 bytes in the kmem cache. Signed-off-by: William Wu <william.wu@rock-chips.com> --- Changes in v4: - get rid of "qh->dw_align_buf_size" - allocate unaligned_cache for Address DMA mode and Desc DMA mode - freeing order opposite of allocation - do dma_map_single() with the size of DWC2_KMEM_UNALIGNED_BUF_SIZE Changes in v3: - Modify the commit message - Use Kmem_cache to allocate aligned DMA buf - Fix coding style Changes in v2: - None drivers/usb/dwc2/core.h | 3 ++ drivers/usb/dwc2/hcd.c | 87 ++++++++++++++++++++++++++++++++++++++++++-- drivers/usb/dwc2/hcd.h | 8 ++++ drivers/usb/dwc2/hcd_intr.c | 8 ++++ drivers/usb/dwc2/hcd_queue.c | 3 ++ 5 files changed, 105 insertions(+), 4 deletions(-) diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h index d83be56..c1983f8 100644 --- a/drivers/usb/dwc2/core.h +++ b/drivers/usb/dwc2/core.h @@ -915,6 +915,7 @@ struct dwc2_hregs_backup { * @frame_list_sz: Frame list size * @desc_gen_cache: Kmem cache for generic descriptors * @desc_hsisoc_cache: Kmem cache for hs isochronous descriptors + * @unaligned_cache: Kmem cache for DMA mode to handle non-aligned buf * * These are for peripheral mode: * @@ -1059,6 +1060,8 @@ struct dwc2_hsotg { u32 frame_list_sz; struct kmem_cache *desc_gen_cache; struct kmem_cache *desc_hsisoc_cache; + struct kmem_cache *unaligned_cache; +#define DWC2_KMEM_UNALIGNED_BUF_SIZE 1024 #endif /* CONFIG_USB_DWC2_HOST || CONFIG_USB_DWC2_DUAL_ROLE */ diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c index 190f959..64666cf 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,35 @@ 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 (!hsotg->unaligned_cache || + chan->max_packet > DWC2_KMEM_UNALIGNED_BUF_SIZE) + return -ENOMEM; + + if (!qh->dw_align_buf) { + qh->dw_align_buf = kmem_cache_alloc(hsotg->unaligned_cache, + GFP_ATOMIC | GFP_DMA); + if (!qh->dw_align_buf) + return -ENOMEM; + } + + qh->dw_align_buf_dma = dma_map_single(hsotg->dev, qh->dw_align_buf, + DWC2_KMEM_UNALIGNED_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 +2835,32 @@ 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 && 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, + "Failed to allocate memory to handle non-aligned buffer\n"); + /* 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 { + /* + * We assume that DMA is always aligned in non-split + * case or split out case. Warn if not. + */ + WARN_ON_ONCE(hsotg->params.host_dma && + (chan->xfer_dma & 0x3)); + chan->align_buf = 0; + } + if (chan->ep_type == USB_ENDPOINT_XFER_INT || chan->ep_type == USB_ENDPOINT_XFER_ISOC) /* @@ -5243,6 +5307,19 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg) } } + if (hsotg->params.host_dma) { + /* + * Create kmem caches to handle non-aligned buffer + * in Buffer DMA mode. + */ + hsotg->unaligned_cache = kmem_cache_create("dwc2-unaligned-dma", + DWC2_KMEM_UNALIGNED_BUF_SIZE, 4, + SLAB_CACHE_DMA, NULL); + if (!hsotg->unaligned_cache) + dev_err(hsotg->dev, + "unable to create dwc2 unaligned cache\n"); + } + hsotg->otg_port = 1; hsotg->frame_list = NULL; hsotg->frame_list_dma = 0; @@ -5277,8 +5354,9 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg) return 0; error4: - kmem_cache_destroy(hsotg->desc_gen_cache); + kmem_cache_destroy(hsotg->unaligned_cache); kmem_cache_destroy(hsotg->desc_hsisoc_cache); + kmem_cache_destroy(hsotg->desc_gen_cache); error3: dwc2_hcd_release(hsotg); error2: @@ -5321,6 +5399,7 @@ void dwc2_hcd_remove(struct dwc2_hsotg *hsotg) kmem_cache_destroy(hsotg->desc_gen_cache); kmem_cache_destroy(hsotg->desc_hsisoc_cache); + kmem_cache_destroy(hsotg->unaligned_cache); dwc2_hcd_release(hsotg); usb_put_hcd(hcd); diff --git a/drivers/usb/dwc2/hcd.h b/drivers/usb/dwc2/hcd.h index 96a9da5..5b5b9e6 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,9 @@ 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_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 +356,8 @@ 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; + 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..ba6229e 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, "non-aligned buffer\n"); + dma_unmap_single(hsotg->dev, chan->qh->dw_align_buf_dma, + DWC2_KMEM_UNALIGNED_BUF_SIZE, DMA_FROM_DEVICE); + memcpy(qtd->urb->buf + (chan->xfer_dma - qtd->urb->dma), + 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..56464cbd 100644 --- a/drivers/usb/dwc2/hcd_queue.c +++ b/drivers/usb/dwc2/hcd_queue.c @@ -1695,6 +1695,9 @@ void dwc2_hcd_qh_free(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh) if (qh->desc_list) dwc2_hcd_qh_free_ddma(hsotg, qh); + else if (hsotg->unaligned_cache && qh->dw_align_buf) + kmem_cache_free(hsotg->unaligned_cache, qh->dw_align_buf); + kfree(qh); } -- 2.0.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [v4,1/2] usb: dwc2: alloc dma aligned buffer for isoc split in @ 2018-05-09 10:11 ` William Wu 0 siblings, 0 replies; 11+ messages in thread From: William Wu @ 2018-05-09 10:11 UTC (permalink / raw) To: hminas, felipe.balbi, gregkh Cc: sergei.shtylyov, heiko, linux-kernel, linux-usb, linux-rockchip, frank.wang, huangtao, dianders, daniel.meng, John.Youn, william.wu, wzz, zsq, Allen.Hsu, StanTsui, Spruce.Wu, Martin.Tsai, Kevin.Shai, Mon-Jer.Wu, Claud.Chang, San.Lin, Ren.Kuo, davidhtwang, fonglin, stevencheng, tomchen, donchang, milesschofield 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 cause DATA0 packet transmission error. This patch use kmem_cache to allocate aligned DMA buf for isoc split in transaction. Note that according to usb 2.0 spec, the maximum data payload size is 1023 bytes for each fs isoc ep, and the maximum allowable interrupt data payload size is 64 bytes or less for fs interrupt ep. So we set the size of object to be 1024 bytes in the kmem cache. Signed-off-by: William Wu <william.wu@rock-chips.com> --- Changes in v4: - get rid of "qh->dw_align_buf_size" - allocate unaligned_cache for Address DMA mode and Desc DMA mode - freeing order opposite of allocation - do dma_map_single() with the size of DWC2_KMEM_UNALIGNED_BUF_SIZE Changes in v3: - Modify the commit message - Use Kmem_cache to allocate aligned DMA buf - Fix coding style Changes in v2: - None drivers/usb/dwc2/core.h | 3 ++ drivers/usb/dwc2/hcd.c | 87 ++++++++++++++++++++++++++++++++++++++++++-- drivers/usb/dwc2/hcd.h | 8 ++++ drivers/usb/dwc2/hcd_intr.c | 8 ++++ drivers/usb/dwc2/hcd_queue.c | 3 ++ 5 files changed, 105 insertions(+), 4 deletions(-) diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h index d83be56..c1983f8 100644 --- a/drivers/usb/dwc2/core.h +++ b/drivers/usb/dwc2/core.h @@ -915,6 +915,7 @@ struct dwc2_hregs_backup { * @frame_list_sz: Frame list size * @desc_gen_cache: Kmem cache for generic descriptors * @desc_hsisoc_cache: Kmem cache for hs isochronous descriptors + * @unaligned_cache: Kmem cache for DMA mode to handle non-aligned buf * * These are for peripheral mode: * @@ -1059,6 +1060,8 @@ struct dwc2_hsotg { u32 frame_list_sz; struct kmem_cache *desc_gen_cache; struct kmem_cache *desc_hsisoc_cache; + struct kmem_cache *unaligned_cache; +#define DWC2_KMEM_UNALIGNED_BUF_SIZE 1024 #endif /* CONFIG_USB_DWC2_HOST || CONFIG_USB_DWC2_DUAL_ROLE */ diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c index 190f959..64666cf 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,35 @@ 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 (!hsotg->unaligned_cache || + chan->max_packet > DWC2_KMEM_UNALIGNED_BUF_SIZE) + return -ENOMEM; + + if (!qh->dw_align_buf) { + qh->dw_align_buf = kmem_cache_alloc(hsotg->unaligned_cache, + GFP_ATOMIC | GFP_DMA); + if (!qh->dw_align_buf) + return -ENOMEM; + } + + qh->dw_align_buf_dma = dma_map_single(hsotg->dev, qh->dw_align_buf, + DWC2_KMEM_UNALIGNED_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 +2835,32 @@ 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 && 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, + "Failed to allocate memory to handle non-aligned buffer\n"); + /* 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 { + /* + * We assume that DMA is always aligned in non-split + * case or split out case. Warn if not. + */ + WARN_ON_ONCE(hsotg->params.host_dma && + (chan->xfer_dma & 0x3)); + chan->align_buf = 0; + } + if (chan->ep_type == USB_ENDPOINT_XFER_INT || chan->ep_type == USB_ENDPOINT_XFER_ISOC) /* @@ -5243,6 +5307,19 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg) } } + if (hsotg->params.host_dma) { + /* + * Create kmem caches to handle non-aligned buffer + * in Buffer DMA mode. + */ + hsotg->unaligned_cache = kmem_cache_create("dwc2-unaligned-dma", + DWC2_KMEM_UNALIGNED_BUF_SIZE, 4, + SLAB_CACHE_DMA, NULL); + if (!hsotg->unaligned_cache) + dev_err(hsotg->dev, + "unable to create dwc2 unaligned cache\n"); + } + hsotg->otg_port = 1; hsotg->frame_list = NULL; hsotg->frame_list_dma = 0; @@ -5277,8 +5354,9 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg) return 0; error4: - kmem_cache_destroy(hsotg->desc_gen_cache); + kmem_cache_destroy(hsotg->unaligned_cache); kmem_cache_destroy(hsotg->desc_hsisoc_cache); + kmem_cache_destroy(hsotg->desc_gen_cache); error3: dwc2_hcd_release(hsotg); error2: @@ -5321,6 +5399,7 @@ void dwc2_hcd_remove(struct dwc2_hsotg *hsotg) kmem_cache_destroy(hsotg->desc_gen_cache); kmem_cache_destroy(hsotg->desc_hsisoc_cache); + kmem_cache_destroy(hsotg->unaligned_cache); dwc2_hcd_release(hsotg); usb_put_hcd(hcd); diff --git a/drivers/usb/dwc2/hcd.h b/drivers/usb/dwc2/hcd.h index 96a9da5..5b5b9e6 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,9 @@ 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_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 +356,8 @@ 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; + 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..ba6229e 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, "non-aligned buffer\n"); + dma_unmap_single(hsotg->dev, chan->qh->dw_align_buf_dma, + DWC2_KMEM_UNALIGNED_BUF_SIZE, DMA_FROM_DEVICE); + memcpy(qtd->urb->buf + (chan->xfer_dma - qtd->urb->dma), + 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..56464cbd 100644 --- a/drivers/usb/dwc2/hcd_queue.c +++ b/drivers/usb/dwc2/hcd_queue.c @@ -1695,6 +1695,9 @@ void dwc2_hcd_qh_free(struct dwc2_hsotg *hsotg, struct dwc2_qh *qh) if (qh->desc_list) dwc2_hcd_qh_free_ddma(hsotg, qh); + else if (hsotg->unaligned_cache && qh->dw_align_buf) + kmem_cache_free(hsotg->unaligned_cache, qh->dw_align_buf); + kfree(qh); } ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v4 1/2] usb: dwc2: alloc dma aligned buffer for isoc split in @ 2018-05-10 21:01 ` Doug Anderson 0 siblings, 0 replies; 11+ messages in thread From: Doug Anderson @ 2018-05-10 21:01 UTC (permalink / raw) To: William Wu Cc: hminas, felipe.balbi, Greg Kroah-Hartman, Sergei Shtylyov, Heiko Stübner, LKML, linux-usb, open list:ARM/Rockchip SoC..., Frank Wang, 黄涛, daniel.meng, John Youn, 王征增, zsq, 許嘉銘, Stan Tsui, Spruce Wu (吳建勳), Martin Tsai, Kevin Hsueh, Mon-Jer Wu (吳孟哲), Claud Chang (張恭築), San Lin (林建菱), Ren Kuo, David H.T. Wang, Fong Lin, Steven Cheng, Tom Chen, Don Chang, milesschofield Hi, On Wed, May 9, 2018 at 3:11 AM, 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 cause DATA0 packet transmission error. > > This patch use kmem_cache to allocate aligned DMA buf for isoc > split in transaction. Note that according to usb 2.0 spec, the > maximum data payload size is 1023 bytes for each fs isoc ep, > and the maximum allowable interrupt data payload size is 64 bytes > or less for fs interrupt ep. So we set the size of object to be > 1024 bytes in the kmem cache. > > Signed-off-by: William Wu <william.wu@rock-chips.com> > --- > Changes in v4: > - get rid of "qh->dw_align_buf_size" > - allocate unaligned_cache for Address DMA mode and Desc DMA mode > - freeing order opposite of allocation You only did half of this. You changed the order under "error4" but forgot to make dwc2_hcd_remove() match. > - do dma_map_single() with the size of DWC2_KMEM_UNALIGNED_BUF_SIZE > > Changes in v3: > - Modify the commit message > - Use Kmem_cache to allocate aligned DMA buf > - Fix coding style > > Changes in v2: > - None > > drivers/usb/dwc2/core.h | 3 ++ > drivers/usb/dwc2/hcd.c | 87 ++++++++++++++++++++++++++++++++++++++++++-- > drivers/usb/dwc2/hcd.h | 8 ++++ > drivers/usb/dwc2/hcd_intr.c | 8 ++++ > drivers/usb/dwc2/hcd_queue.c | 3 ++ > 5 files changed, 105 insertions(+), 4 deletions(-) My only remaining comment is a really nitty detail. Unless someone else feels strongly about it, I'm not sure it's worth spinning the patch just for that nit unless someone else has review comments. I believe I've looked at this enough to say: Reviewed-by: Douglas Anderson <dianders@chromium.org> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4 1/2] usb: dwc2: alloc dma aligned buffer for isoc split in @ 2018-05-10 21:01 ` Doug Anderson 0 siblings, 0 replies; 11+ messages in thread From: Doug Anderson @ 2018-05-10 21:01 UTC (permalink / raw) To: William Wu Cc: hminas, felipe.balbi, Greg Kroah-Hartman, Sergei Shtylyov, Heiko Stübner, LKML, linux-usb, open list:ARM/Rockchip SoC..., Frank Wang, 黄涛, daniel.meng, John Youn, 王征增, zsq, 許嘉銘, Stan Tsui, Spruce Wu (吳建勳), Martin Tsai Hi, On Wed, May 9, 2018 at 3:11 AM, 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 cause DATA0 packet transmission error. > > This patch use kmem_cache to allocate aligned DMA buf for isoc > split in transaction. Note that according to usb 2.0 spec, the > maximum data payload size is 1023 bytes for each fs isoc ep, > and the maximum allowable interrupt data payload size is 64 bytes > or less for fs interrupt ep. So we set the size of object to be > 1024 bytes in the kmem cache. > > Signed-off-by: William Wu <william.wu@rock-chips.com> > --- > Changes in v4: > - get rid of "qh->dw_align_buf_size" > - allocate unaligned_cache for Address DMA mode and Desc DMA mode > - freeing order opposite of allocation You only did half of this. You changed the order under "error4" but forgot to make dwc2_hcd_remove() match. > - do dma_map_single() with the size of DWC2_KMEM_UNALIGNED_BUF_SIZE > > Changes in v3: > - Modify the commit message > - Use Kmem_cache to allocate aligned DMA buf > - Fix coding style > > Changes in v2: > - None > > drivers/usb/dwc2/core.h | 3 ++ > drivers/usb/dwc2/hcd.c | 87 ++++++++++++++++++++++++++++++++++++++++++-- > drivers/usb/dwc2/hcd.h | 8 ++++ > drivers/usb/dwc2/hcd_intr.c | 8 ++++ > drivers/usb/dwc2/hcd_queue.c | 3 ++ > 5 files changed, 105 insertions(+), 4 deletions(-) My only remaining comment is a really nitty detail. Unless someone else feels strongly about it, I'm not sure it's worth spinning the patch just for that nit unless someone else has review comments. I believe I've looked at this enough to say: Reviewed-by: Douglas Anderson <dianders@chromium.org> ^ permalink raw reply [flat|nested] 11+ messages in thread
* [v4,1/2] usb: dwc2: alloc dma aligned buffer for isoc split in @ 2018-05-10 21:01 ` Doug Anderson 0 siblings, 0 replies; 11+ messages in thread From: Doug Anderson @ 2018-05-10 21:01 UTC (permalink / raw) To: William Wu Cc: hminas, felipe.balbi, Greg Kroah-Hartman, Sergei Shtylyov, Heiko Stübner, LKML, linux-usb, open list:ARM/Rockchip SoC..., Frank Wang, 黄涛, daniel.meng, John Youn, 王征增, zsq, 許嘉銘, Stan Tsui, Spruce Wu (吳建勳), Martin Tsai, Kevin Hsueh, Mon-Jer Wu (吳孟哲), Claud Chang (張恭築), San Lin (林建菱), Ren Kuo, David H.T. Wang, Fong Lin, Steven Cheng, Tom Chen, Don Chang, milesschofield Hi, On Wed, May 9, 2018 at 3:11 AM, 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 cause DATA0 packet transmission error. > > This patch use kmem_cache to allocate aligned DMA buf for isoc > split in transaction. Note that according to usb 2.0 spec, the > maximum data payload size is 1023 bytes for each fs isoc ep, > and the maximum allowable interrupt data payload size is 64 bytes > or less for fs interrupt ep. So we set the size of object to be > 1024 bytes in the kmem cache. > > Signed-off-by: William Wu <william.wu@rock-chips.com> > --- > Changes in v4: > - get rid of "qh->dw_align_buf_size" > - allocate unaligned_cache for Address DMA mode and Desc DMA mode > - freeing order opposite of allocation You only did half of this. You changed the order under "error4" but forgot to make dwc2_hcd_remove() match. > - do dma_map_single() with the size of DWC2_KMEM_UNALIGNED_BUF_SIZE > > Changes in v3: > - Modify the commit message > - Use Kmem_cache to allocate aligned DMA buf > - Fix coding style > > Changes in v2: > - None > > drivers/usb/dwc2/core.h | 3 ++ > drivers/usb/dwc2/hcd.c | 87 ++++++++++++++++++++++++++++++++++++++++++-- > drivers/usb/dwc2/hcd.h | 8 ++++ > drivers/usb/dwc2/hcd_intr.c | 8 ++++ > drivers/usb/dwc2/hcd_queue.c | 3 ++ > 5 files changed, 105 insertions(+), 4 deletions(-) My only remaining comment is a really nitty detail. Unless someone else feels strongly about it, I'm not sure it's worth spinning the patch just for that nit unless someone else has review comments. I believe I've looked at this enough to say: 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4 1/2] usb: dwc2: alloc dma aligned buffer for isoc split in @ 2018-05-11 9:33 ` William Wu 0 siblings, 0 replies; 11+ messages in thread From: wlf @ 2018-05-11 9:33 UTC (permalink / raw) To: Doug Anderson, William Wu Cc: hminas, felipe.balbi, Greg Kroah-Hartman, Sergei Shtylyov, Heiko Stübner, LKML, linux-usb, open list:ARM/Rockchip SoC..., Frank Wang, 黄涛, daniel.meng, John Youn, 王征增, zsq, 許嘉銘, Stan Tsui, Spruce Wu (吳建勳), Martin Tsai, Kevin Hsueh, Mon-Jer Wu (吳孟哲), Claud Chang (張恭築), San Lin (林建菱), Ren Kuo, David H.T. Wang, Fong Lin, Steven Cheng, Tom Chen, Don Chang, milesschofield Dear Doug, 在 2018年05月11日 05:01, Doug Anderson 写道: > Hi, > > On Wed, May 9, 2018 at 3:11 AM, 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 cause DATA0 packet transmission error. >> >> This patch use kmem_cache to allocate aligned DMA buf for isoc >> split in transaction. Note that according to usb 2.0 spec, the >> maximum data payload size is 1023 bytes for each fs isoc ep, >> and the maximum allowable interrupt data payload size is 64 bytes >> or less for fs interrupt ep. So we set the size of object to be >> 1024 bytes in the kmem cache. >> >> Signed-off-by: William Wu <william.wu@rock-chips.com> >> --- >> Changes in v4: >> - get rid of "qh->dw_align_buf_size" >> - allocate unaligned_cache for Address DMA mode and Desc DMA mode >> - freeing order opposite of allocation > You only did half of this. You changed the order under "error4" but > forgot to make dwc2_hcd_remove() match. Ah, sorry, I'm careless. > > >> - do dma_map_single() with the size of DWC2_KMEM_UNALIGNED_BUF_SIZE >> >> Changes in v3: >> - Modify the commit message >> - Use Kmem_cache to allocate aligned DMA buf >> - Fix coding style >> >> Changes in v2: >> - None >> >> drivers/usb/dwc2/core.h | 3 ++ >> drivers/usb/dwc2/hcd.c | 87 ++++++++++++++++++++++++++++++++++++++++++-- >> drivers/usb/dwc2/hcd.h | 8 ++++ >> drivers/usb/dwc2/hcd_intr.c | 8 ++++ >> drivers/usb/dwc2/hcd_queue.c | 3 ++ >> 5 files changed, 105 insertions(+), 4 deletions(-) > My only remaining comment is a really nitty detail. Unless someone > else feels strongly about it, I'm not sure it's worth spinning the > patch just for that nit unless someone else has review comments. > > I believe I've looked at this enough to say: > > Reviewed-by: Douglas Anderson <dianders@chromium.org> Thank you very much for your review. I will submit V5 patches to fix the order of memory free in dwc2_hcd_remove()， and also add Review-by. > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4 1/2] usb: dwc2: alloc dma aligned buffer for isoc split in @ 2018-05-11 9:33 ` William Wu 0 siblings, 0 replies; 11+ messages in thread From: wlf @ 2018-05-11 9:33 UTC (permalink / raw) To: Doug Anderson, William Wu Cc: hminas, felipe.balbi, Greg Kroah-Hartman, Sergei Shtylyov, Heiko Stübner, LKML, linux-usb, open list:ARM/Rockchip SoC..., Frank Wang, 黄涛, daniel.meng, John Youn, 王征增, zsq, 許嘉銘, Stan Tsui, Spruce Wu (吳建勳), Martin Tsai Dear Doug, 在 2018年05月11日 05:01, Doug Anderson 写道: > Hi, > > On Wed, May 9, 2018 at 3:11 AM, 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 cause DATA0 packet transmission error. >> >> This patch use kmem_cache to allocate aligned DMA buf for isoc >> split in transaction. Note that according to usb 2.0 spec, the >> maximum data payload size is 1023 bytes for each fs isoc ep, >> and the maximum allowable interrupt data payload size is 64 bytes >> or less for fs interrupt ep. So we set the size of object to be >> 1024 bytes in the kmem cache. >> >> Signed-off-by: William Wu <william.wu@rock-chips.com> >> --- >> Changes in v4: >> - get rid of "qh->dw_align_buf_size" >> - allocate unaligned_cache for Address DMA mode and Desc DMA mode >> - freeing order opposite of allocation > You only did half of this. You changed the order under "error4" but > forgot to make dwc2_hcd_remove() match. Ah, sorry, I'm careless. > > >> - do dma_map_single() with the size of DWC2_KMEM_UNALIGNED_BUF_SIZE >> >> Changes in v3: >> - Modify the commit message >> - Use Kmem_cache to allocate aligned DMA buf >> - Fix coding style >> >> Changes in v2: >> - None >> >> drivers/usb/dwc2/core.h | 3 ++ >> drivers/usb/dwc2/hcd.c | 87 ++++++++++++++++++++++++++++++++++++++++++-- >> drivers/usb/dwc2/hcd.h | 8 ++++ >> drivers/usb/dwc2/hcd_intr.c | 8 ++++ >> drivers/usb/dwc2/hcd_queue.c | 3 ++ >> 5 files changed, 105 insertions(+), 4 deletions(-) > My only remaining comment is a really nitty detail. Unless someone > else feels strongly about it, I'm not sure it's worth spinning the > patch just for that nit unless someone else has review comments. > > I believe I've looked at this enough to say: > > Reviewed-by: Douglas Anderson <dianders@chromium.org> Thank you very much for your review. I will submit V5 patches to fix the order of memory free in dwc2_hcd_remove()， and also add Review-by. > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [v4,1/2] usb: dwc2: alloc dma aligned buffer for isoc split in @ 2018-05-11 9:33 ` William Wu 0 siblings, 0 replies; 11+ messages in thread From: William Wu @ 2018-05-11 9:33 UTC (permalink / raw) To: Doug Anderson, William Wu Cc: hminas, felipe.balbi, Greg Kroah-Hartman, Sergei Shtylyov, Heiko Stübner, LKML, linux-usb, open list:ARM/Rockchip SoC..., Frank Wang, 黄涛, daniel.meng, John Youn, 王征增, zsq, 許嘉銘, Stan Tsui, Spruce Wu (吳建勳), Martin Tsai, Kevin Hsueh, Mon-Jer Wu (吳孟哲), Claud Chang (張恭築), San Lin (林建菱), Ren Kuo, David H.T. Wang, Fong Lin, Steven Cheng, Tom Chen, Don Chang, milesschofield Dear Doug, 在 2018年05月11日 05:01, Doug Anderson 写道: > Hi, > > On Wed, May 9, 2018 at 3:11 AM, 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 cause DATA0 packet transmission error. >> >> This patch use kmem_cache to allocate aligned DMA buf for isoc >> split in transaction. Note that according to usb 2.0 spec, the >> maximum data payload size is 1023 bytes for each fs isoc ep, >> and the maximum allowable interrupt data payload size is 64 bytes >> or less for fs interrupt ep. So we set the size of object to be >> 1024 bytes in the kmem cache. >> >> Signed-off-by: William Wu <william.wu@rock-chips.com> >> --- >> Changes in v4: >> - get rid of "qh->dw_align_buf_size" >> - allocate unaligned_cache for Address DMA mode and Desc DMA mode >> - freeing order opposite of allocation > You only did half of this. You changed the order under "error4" but > forgot to make dwc2_hcd_remove() match. Ah, sorry, I'm careless. > > >> - do dma_map_single() with the size of DWC2_KMEM_UNALIGNED_BUF_SIZE >> >> Changes in v3: >> - Modify the commit message >> - Use Kmem_cache to allocate aligned DMA buf >> - Fix coding style >> >> Changes in v2: >> - None >> >> drivers/usb/dwc2/core.h | 3 ++ >> drivers/usb/dwc2/hcd.c | 87 ++++++++++++++++++++++++++++++++++++++++++-- >> drivers/usb/dwc2/hcd.h | 8 ++++ >> drivers/usb/dwc2/hcd_intr.c | 8 ++++ >> drivers/usb/dwc2/hcd_queue.c | 3 ++ >> 5 files changed, 105 insertions(+), 4 deletions(-) > My only remaining comment is a really nitty detail. Unless someone > else feels strongly about it, I'm not sure it's worth spinning the > patch just for that nit unless someone else has review comments. > > I believe I've looked at this enough to say: > > Reviewed-by: Douglas Anderson <dianders@chromium.org> Thank you very much for your review. I will submit V5 patches to fix the order of memory free in dwc2_hcd_remove()， and also add Review-by. > > > --- 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v4 2/2] usb: dwc2: fix isoc split in transfer with no data @ 2018-05-09 10:11 ` William Wu 0 siblings, 0 replies; 11+ messages in thread From: William Wu @ 2018-05-09 10:11 UTC (permalink / raw) To: hminas, felipe.balbi, gregkh Cc: sergei.shtylyov, heiko, linux-kernel, linux-usb, linux-rockchip, frank.wang, huangtao, dianders, daniel.meng, John.Youn, william.wu, wzz, zsq, Allen.Hsu, StanTsui, Spruce.Wu, Martin.Tsai, Kevin.Shai, Mon-Jer.Wu, Claud.Chang, San.Lin, Ren.Kuo, davidhtwang, fonglin, stevencheng, tomchen, donchang, milesschofield 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 v4: - None Changes in v3: - Remove "qtd->isoc_split_offset = 0" in the if test Changes in v2: - Modify the commit message drivers/usb/dwc2/hcd_intr.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c index ba6229e..9751785 100644 --- a/drivers/usb/dwc2/hcd_intr.c +++ b/drivers/usb/dwc2/hcd_intr.c @@ -930,9 +930,8 @@ 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [v4,2/2] usb: dwc2: fix isoc split in transfer with no data @ 2018-05-09 10:11 ` William Wu 0 siblings, 0 replies; 11+ messages in thread From: William Wu @ 2018-05-09 10:11 UTC (permalink / raw) To: hminas, felipe.balbi, gregkh Cc: sergei.shtylyov, heiko, linux-kernel, linux-usb, linux-rockchip, frank.wang, huangtao, dianders, daniel.meng, John.Youn, william.wu, wzz, zsq, Allen.Hsu, StanTsui, Spruce.Wu, Martin.Tsai, Kevin.Shai, Mon-Jer.Wu, Claud.Chang, San.Lin, Ren.Kuo, davidhtwang, fonglin, stevencheng, tomchen, donchang, milesschofield 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 v4: - None Changes in v3: - Remove "qtd->isoc_split_offset = 0" in the if test Changes in v2: - Modify the commit message drivers/usb/dwc2/hcd_intr.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c index ba6229e..9751785 100644 --- a/drivers/usb/dwc2/hcd_intr.c +++ b/drivers/usb/dwc2/hcd_intr.c @@ -930,9 +930,8 @@ 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; } ^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-05-11 9:33 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-05-09 10:10 [PATCH v4 0/2] usb: dwc2: fix isoc split in transfer issue William Wu 2018-05-09 10:11 ` [PATCH v4 1/2] usb: dwc2: alloc dma aligned buffer for isoc split in William Wu 2018-05-09 10:11 ` [v4,1/2] " William Wu 2018-05-10 21:01 ` [PATCH v4 1/2] " Doug Anderson 2018-05-10 21:01 ` Doug Anderson 2018-05-10 21:01 ` [v4,1/2] " Doug Anderson 2018-05-11 9:33 ` [PATCH v4 1/2] " wlf 2018-05-11 9:33 ` wlf 2018-05-11 9:33 ` [v4,1/2] " William Wu 2018-05-09 10:11 ` [PATCH v4 2/2] usb: dwc2: fix isoc split in transfer with no data William Wu 2018-05-09 10:11 ` [v4,2/2] " William Wu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.