Re: [PATCH] usb: hcd: initialize hcd->flags to 0 when rm hcd
Hi Roger， 在 2017年01月12日 23:38, Roger Quadros 写道: On 12/01/17 17:33, Alan Stern wrote: On Thu, 12 Jan 2017, Roger Quadros wrote: William, On 12/01/17 14:03, William Wu wrote: From: William wu On some platforms(e.g. rk3399 board), we can call hcd_add/remove consecutively without calling usb_put_hcd/usb_create_hcd in between, so hcd->flags can be stale. If the HC dies due to whatever reason then without this patch we get the below error on next hcd_add. [173.296154] xhci-hcd xhci-hcd.2.auto: HC died; cleaning up [173.296209] xhci-hcd xhci-hcd.2.auto: xHCI Host Controller [173.296762] xhci-hcd xhci-hcd.2.auto: new USB bus registered, assigned bus number 6 [173.296931] usb usb6: We don't know the algorithms for LPM for this host, disabling LPM. [173.297179] usb usb6: New USB device found, idVendor=1d6b, idProduct=0003 [173.297203] usb usb6: New USB device strings: Mfr=3, Product=2, SerialNumber=1 [173.297222] usb usb6: Product: xHCI Host Controller [173.297240] usb usb6: Manufacturer: Linux 4.4.21 xhci-hcd [173.297257] usb usb6: SerialNumber: xhci-hcd.2.auto [173.298680] hub 6-0:1.0: USB hub found [173.298749] hub 6-0:1.0: 1 port detected [173.299382] rockchip-dwc3 usb@fe80: USB HOST connected [173.395418] hub 5-0:1.0: activate --> -19 [173.603447] irq 228: nobody cared (try booting with the "irqpoll" option) [173.603493] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.4.21 #9 [173.603513] Hardware name: Google Kevin (DT) [173.603531] Call trace: [173.603568] [] dump_backtrace+0x0/0x160 [173.603596] [] show_stack+0x20/0x28 [173.603623] [] dump_stack+0x90/0xb0 [173.603650] [] __report_bad_irq+0x48/0xe8 [173.603674] [] note_interrupt+0x1e8/0x28c [173.603698] [] handle_irq_event_percpu+0x1d4/0x25c [173.603722] [] handle_irq_event+0x4c/0x7c [173.603748] [] handle_fasteoi_irq+0xb4/0x124 [173.603777] [] generic_handle_irq+0x30/0x44 [173.603804] [] __handle_domain_irq+0x90/0xbc [173.603827] [] gic_handle_irq+0xcc/0x188 ... [173.604500] [] el1_irq+0x80/0xf8 [173.604530] [] cpu_startup_entry+0x38/0x3cc [173.604558] [] rest_init+0x8c/0x94 [173.604585] [] start_kernel+0x3d0/0x3fc [173.604607] [<00b16000>] 0xb16000 [173.604622] handlers: [173.604648] [] usb_hcd_irq [173.604673] Disabling IRQ #228 Signed-off-by: William wu Signed-off-by: William wu --- drivers/usb/core/hcd.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c index 479e223..612fab6 100644 --- a/drivers/usb/core/hcd.c +++ b/drivers/usb/core/hcd.c @@ -3017,6 +3017,7 @@ void usb_remove_hcd(struct usb_hcd *hcd) } usb_put_invalidate_rhdev(hcd); + hcd->flags = 0; } EXPORT_SYMBOL_GPL(usb_remove_hcd); I suggest to initialize hcd->flags to 0 in usb_add_hcd() instead. cheers, -roger Roger, didn't you originally propose this very same patch in http://marc.info/?l=linux-usb&m=146556415503583&w=2 (and of course, the earlier versions before v10)? What happened to it? Alan, You are right. That patch got lost in the ML. Looks like I didn't push it hard enough and then forgot about it. Sorry. William, You don't need to do any changes. My earlier version was clearing the flag in usb_add_hcd() and I guess Alan suggested to move it to usb_remove_hcd(). So your patch is good. You can add my. Acked-by: Roger Quadros cheers, -roger Thanks very much for your suggestion，I will add acked-by immediately. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: host: xhci: plat: check hcc_params after add hcd
Hi Roger,
在 2017年01月13日 19:02, Roger Quadros 写道:
Hi,
On 13/01/17 05:18, William Wu wrote:
From: William wu
The commit 4ac53087d6d4 ("usb: xhci: plat: Create both
HCDs before adding them") move add hcd to the end of
probe, this cause hcc_params uninitiated, because xHCI
driver sets hcc_params in xhci_gen_setup() called from
usb_add_hcd().
This patch checks the Maximum Primary Stream Array Size
in the hcc_params register after add hcd.
Signed-off-by: William wu
---
drivers/usb/host/xhci-plat.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index ddfab30..52ce697 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -232,9 +232,6 @@ static int xhci_plat_probe(struct platform_device *pdev)
if (device_property_read_bool(&pdev->dev, "usb3-lpm-capable"))
xhci->quirks |= XHCI_LPM_SUPPORT;
- if (HCC_MAX_PSA(xhci->hcc_params) >= 4)
- xhci->shared_hcd->can_do_streams = 1;
-
hcd->usb_phy = devm_usb_get_phy_by_phandle(&pdev->dev, "usb-phy", 0);
if (IS_ERR(hcd->usb_phy)) {
ret = PTR_ERR(hcd->usb_phy);
@@ -255,6 +252,9 @@ static int xhci_plat_probe(struct platform_device *pdev)
if (ret)
goto dealloc_usb2_hcd;
+ if (HCC_MAX_PSA(xhci->hcc_params) >= 4)
+ xhci->shared_hcd->can_do_streams = 1;
+
xhci->hcc_params is initialized after the first usb_add_hcd().
Should this bit come before the usb_add_hcd(xhci->shared_hcd,..)?
Yes, good idea！I will move it behind the first usb_add_hcd().
You will also need to copy to v4.2+ . Thanks.
OK, thank you for reminding me!
return 0;
cheers,
-roger
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] usb: dwc2: fix isoc split in transfer with no data
Dear Sergei, 在 2018年04月24日 16:27, Sergei Shtylyov 写道: Hello! On 4/24/2018 5:43 AM, William Wu wrote: If isoc split in transfer with no data (the length of DATA0 packet is 0), 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 on data ^^ no? Thank you for correcting me. I will fix it in next patch version. is in the first transaction, we can return immediately. But if the the DATA0 packet with on data is in the second transaction One "the" too many. And that "on data" again... :-) Ah, sorry to make such a simple mistake. I will fix these in next patch version. 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 [...] MBR, Sergei Best regards, wulf -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] usb: dwc2: alloc dma aligned buffer for isoc split in
Dear Doug,
在 2018年05月02日 12:33, Doug Anderson 写道:
Hi,
On Tue, May 1, 2018 at 8:04 PM, William Wu 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
---
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
Re: [PATCH v2 2/2] usb: dwc2: fix isoc split in transfer with no data
Dear Doug,
在 2018年05月02日 13:02, Doug Anderson 写道:
Hi,
On Tue, May 1, 2018 at 8:04 PM, William Wu 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
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] usb: dwc2: alloc dma aligned buffer for isoc split in
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:diand...@google.com]
Sent: Friday, May 4, 2018 11:58 PM
To: wlf
Cc: William Wu ; hmi...@synopsys.com; felipe.ba...@linux.intel.com; Greg Kroah-Hartman ;
Sergei Shtylyov ; Heiko Stübner ; LKML ;
linux-usb@vger.kernel.org; open list:ARM/Rockchip SoC... ; Frank Wang ; 黄涛
; daniel.meng ; John Youn ; 王征增 ;
z...@rock-chips.com; Allen Hsu (許嘉銘) ; Stan Tsui
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 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 enabl
Re: [PATCH v3 2/2] usb: dwc2: fix isoc split in transfer with no data
Dear Doug,
在 2018年05月08日 13:13, Doug Anderson 写道:
Hi,
On Mon, May 7, 2018 at 8:07 PM, William Wu 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
---
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 ba6fd852..3003594 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;
}
This looks fine to me now, but as per my comments on the previous
version I don't think I've dug through this problem enough to add my
Reviewed-by tag. I'll assume that John or someone with more knowledge
of the USB protocol than I have will Review / Ack.
Thanks very much for your review. Let's wait for other experts' suggestion.
-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/2] usb: dwc2: alloc dma aligned buffer for isoc split in
Dear Doug,
在 2018年05月08日 13:11, Doug Anderson 写道:
Hi,
On Mon, May 7, 2018 at 8:07 PM, William Wu wrote:
+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)
+ 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_size = min_t(u32, chan->max_packet,
+ DWC2_KMEM_UNALIGNED_BUF_SIZE);
Rather than using min_t, wouldn't it be better to return -ENOMEM if
"max_packet" > DWC2_KMEM_UNALIGNED_BUF_SIZE? As it is, you might
allocate less space than you need, right? That seems like it would be
bad (even though this is probably impossible).
Yes, good idea! So is it good to fix it like this?
if (!qh->dw_align_buf || chan->max_packet >
DWC2_KMEM_UNALIGNED_BUF_SIZE)
return -ENOMEM;
qh->dw_align_buf_size = chan->max_packet;
@@ -2797,6 +2837,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)
/*
@@ -5241,6 +5307,17 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg)
hsotg->params.dma_desc_enable = false;
hsotg->params.dma_desc_fs_enable = false;
}
+ } else if (hsotg->params.host_dma) {
Are you sure this is "else if"? Can't you have descriptor DMA enabled
in the controller and still need to do a normal DMA transfer if you
plug in a hub? Seems like this should be just "if".
Sorry, I don't understand the case "have descriptor DMA enabled in the
controller and still need to do a normal DMA transfer". But maybe it
still has another problem if just use "if" here, because it will create
kmem caches for Slave mode which actually doesn't need aligned DMA buf.
+ /*
+* 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,
Worth using "DWC2_USB_DMA_ALIGN" rather than 4?
+ SLAB_CACHE_DMA, NULL);
+ if (!hsotg->unaligned_cache)
+ dev_err(hsotg->dev,
+ "unable to create dwc2 unaligned cache\n");
}
hsotg->otg_port = 1;
@@ -5279,6 +5356,7 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg)
error4:
kmem_cache_destroy(hsotg->desc_gen_cache);
kmem_cache_destroy(hsotg->desc_hsisoc_cache);
+ kmem_cache_destroy(hsotg->unaligned_cache);
nitty nit: freeing order should be opposite of allocation, so the new
line should be above the other two.
Ah, I got it. But note that it's impossible to allocate the
"unaligned_cache" and "desc *cache" at the same time. Should we still
fix the free order? If yes, maybe the correct free order is:
kmem_cache_destroy(hsotg->unaligned_cache);
kmem_cache_destroy(hsotg->desc_hsisoc_cache);
kmem_cache_destroy(hsotg->desc_gen_cache);
Right?
And should we also need to fix the same free order in the "dwc2_hcd_remove"?
Best regards,
wulf
--
To unsubscribe from this list: send the line "unsubscri
Re: [PATCH v3 1/2] usb: dwc2: alloc dma aligned buffer for isoc split in
Dear Doug,
在 2018年05月08日 23:29, Doug Anderson 写道:
Hi,
On Tue, May 8, 2018 at 12:43 AM, wlf wrote:
Dear Doug,
在 2018年05月08日 13:11, Doug Anderson 写道:
Hi,
On Mon, May 7, 2018 at 8:07 PM, William Wu
wrote:
+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)
+ 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_size = min_t(u32, chan->max_packet,
+
DWC2_KMEM_UNALIGNED_BUF_SIZE);
Rather than using min_t, wouldn't it be better to return -ENOMEM if
"max_packet" > DWC2_KMEM_UNALIGNED_BUF_SIZE? As it is, you might
allocate less space than you need, right? That seems like it would be
bad (even though this is probably impossible).
Yes, good idea! So is it good to fix it like this?
if (!qh->dw_align_buf || chan->max_packet >
DWC2_KMEM_UNALIGNED_BUF_SIZE)
return -ENOMEM;
qh->dw_align_buf_size = chan->max_packet;
Won't that orphan memory in the case that the allocation is OK but the
size is wrong?
I would have put it all the way at the top:
if (!hsotg->unaligned_cache ||
chan->max_packet > DWC2_KMEM_UNALIGNED_BUF_SIZE)
return -ENOMEM;
Ah, yes, you're right! Thank you for correcting me.
It also feels like you should get rid of "qh->dw_align_buf_size". The
buffer is always DWC2_KMEM_UNALIGNED_BUF_SIZE.
...if you need to keep track of how many bytes you mapped with
dma_map_single() and somehow can't get back to "chan", rename this to
qh->dw_align_buf_mapped_bytes or something. I haven't followed
through all the code, but I _think_ you'd want to make sure to set
qh->dw_align_buf_mapped_bytes every time through
dwc2_alloc_split_dma_aligned_buf(), even if dw_align_buf was already
allocated. Specifically, my worry is in the case where you enter
dwc2_alloc_split_dma_aligned_buf() and qh->dw_align_buf is non-NULL.
Could "max_packet" be different in this case compared to what it was
when dw_align_buf was first allocated?
Sorry to make you feel confused. It's really not suitable to use
"qh->dw_align_buf_size" for the dma mapped size. And I think the
"max_packet" should be always be the same. However, just in case, maybe
I can get rid of "qh->dw_align_buf_size" and just use
DWC2_KMEM_UNALIGNED_BUF_SIZE to do dma_map_single().
@@ -2797,6 +2837,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)
/*
@@ -5241,6 +5307,17 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg)
hsotg->params.dma_desc_enable = false;
hsotg->params.dma_desc_fs_enable = false;
}
+ } else if (hsotg->params.host_dma) {
Are you sure this is "else if"? Can't you have descriptor DMA enabled
in the controller and still need to do a normal DMA transfer if you
plug in a hub? Seems like this should be just "if".
Sorry, I don't understand
Re: [PATCH v3 1/2] usb: dwc2: alloc dma aligned buffer for isoc split in
Dear Doug,
在 2018年05月11日 04:59, Doug Anderson 写道:
Hi,
On Wed, May 9, 2018 at 1:55 AM, wlf wrote:
+ } else if (hsotg->params.host_dma) {
Are you sure this is "else if"? Can't you have descriptor DMA enabled
in the controller and still need to do a normal DMA transfer if you
plug in a hub? Seems like this should be just "if".
Sorry, I don't understand the case "have descriptor DMA enabled in the
controller and still need to do a normal DMA transfer". But maybe it
still
has another problem if just use "if" here, because it will create kmem
caches for Slave mode which actually doesn't need aligned DMA buf.
So right now your code looks like:
if (hsotg->params.dma_desc_enable ||
hsotg->params.dma_desc_fs_enable) {
...
...
} else if (hsotg->params.host_dma) {
...
}
I've never played much with "descriptor DMA" on dwc2 because every
time I enabled it (last I tried was years ago) a whole bunch of
peripherals stopped working and I didn't dig (I just left it off).
Thus, I'm no expert on descriptor DMA. ...that being said, my
understanding is that if you enable descriptor DMA in the controller
then it will enable a smarter DMA mode for some of the transfers.
IIRC Descriptor DMA mode specifically can't handle splits. Is my
memory correct there? Presumably that means that when you enable
descriptor DMA in the controller the driver will automatically fall
back to normal Address DMA mode if things get too complicated. When
it falls back to Address DMA mode, presumably dwc2_hcd_init() won't
get called again. That means that any data structures that are needed
for Address DMA need to be allocated in dwc2_hcd_init() even if
descriptor DMA is enabled because we might need to fall back to
Address DMA.
Thus, I'm suggesting changing the code to:
if (hsotg->params.dma_desc_enable ||
hsotg->params.dma_desc_fs_enable) {
...
...
}
if (hsotg->params.host_dma) {
...
}
...as I said, I'm no expert and I could just be confused. If
something I say seems wrong please correct me.
Ah, I got it. Thanks for your detailed explanation. Although I don't know if
it's possible that dwc2 driver automatically fall back to normal Address DMA
mode when desc DMA work abnormally with split, I agree with your suggestion.
I'll fix it in V4 patch.
Hmm, I guess you're right. I did a quick search and I can't find any
evidence of a fallback like this. Maybe I dreamed that. I found some
old comment in the commit history that said:
I think you're right. I find that it's possible to fall back to Address
DMA mode if desc DMA initialization failure. The error case is：in
dwc2_hcd_init(), if it fails to create dwc2 generic desc cache or dwc2
hs isoc desc cache, then it will disable desc DMA and fall back to
Address DMA mode.
/*
* Disable descriptor dma mode by default as the HW can support
* it, but does not support it for SPLIT transactions.
* Disable it for FS devices as well.
*/
...and it looks there's currently nobody using descriptor DMA anyway
(assuming I read the code correctly). It does seem like people were
ever going to turn it on then it would have to be dynamic as I was
dreaming it was...
Yes, the dwc2 desc DMA mode can't support split transaction. So few
people use desc DMA mode for OTG Host mode. BTW, I find that the latest
code enable desc DMA mode by default for the OTG peripheral mode if the
dwc2 hardware support desc DMA.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/2] usb: dwc2: alloc dma aligned buffer for isoc split in
Dear Doug,
在 2018年05月11日 05:01, Doug Anderson 写道:
Hi,
On Wed, May 9, 2018 at 3:11 AM, William Wu 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
---
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
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc2: host: fix isoc urb actual length
Hi Minas, 在 2017年11月06日 17:28, Minas Harutyunyan 写道: Hi, On 11/6/2017 12:46 PM, William Wu wrote: The actual_length in dwc2_hcd_urb structure is used to indicate the total data length transferred so far, but in dwc2_update_isoc_urb_state(), it just updates the actual_length of isoc frame, and don't update the urb actual_length at the same time, this will cause device drivers working error which depend on the urb actual_length. we can easily find this issue if use an USB camera， the userspace use libusb to get USB data from kernel via devio driver.In usb devio driver, processcompl() function will process urb complete and copy data to userspace depending on urb actual_length. Let's update the urb actual_length if the isoc frame is valid. Signed-off-by: William Wu --- drivers/usb/dwc2/hcd_intr.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c index 28a8210..01b1e13 100644 --- a/drivers/usb/dwc2/hcd_intr.c +++ b/drivers/usb/dwc2/hcd_intr.c @@ -580,6 +580,7 @@ static enum dwc2_halt_status dwc2_update_isoc_urb_state( frame_desc->status = 0; frame_desc->actual_length = dwc2_get_actual_xfer_length(hsotg, chan, chnum, qtd, halt_status, NULL); + urb->actual_length += frame_desc->actual_length; break; case DWC2_HC_XFER_FRAME_OVERRUN: urb->error_count++; @@ -599,6 +600,7 @@ static enum dwc2_halt_status dwc2_update_isoc_urb_state( frame_desc->status = -EPROTO; frame_desc->actual_length = dwc2_get_actual_xfer_length(hsotg, chan, chnum, qtd, halt_status, NULL); + urb->actual_length += frame_desc->actual_length; /* Skip whole frame */ if (chan->qh->do_split && According urb description in usb.h urb->actual_length used for non-iso transfers: "@actual_length: This is read in non-iso completion functions, and ... * ISO transfer status is reported in the status and actual_length fields * of the iso_frame_desc array, " Yes, urb->actual_length is used for non-iso transfers. And for ISO transfer, the most usb class drivers can only use iso frame status and actual_length to handle usb data, like: kernel v4.4 drivers/media/usb/uvc/uvc_video.c But the usb devio driver really need the urb actual_length in the processcompl() if handle ISO transfer, just as I mentioned in the commit message. And I also found the same issue on raspberrypi board: https://github.com/raspberrypi/linux/issues/903 So do you think we need to fix the usb devio driver, rather than fix dwc2? I think maybe we can use urb actual length for ISO transfer, it seems that don't cause any side-effect. Thanks, Minas -- 吴良峰 William.Wu 福建省福州市铜盘路软件大道89号软件园A区21号楼 No.21 Building, A District, No.89,software Boulevard Fuzhou,Fujian, PRC 手机: 13685012275 座机: 0591-83991906-8520 邮件:w...@rock-chips.com -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc2: host: fix isoc urb actual length
Hi Alan,
在 2017年11月07日 03:17, Alan Stern 写道:
On Mon, 6 Nov 2017, wlf wrote:
Hi Minas,
在 2017年11月06日 17:28, Minas Harutyunyan 写道:
Hi,
On 11/6/2017 12:46 PM, William Wu wrote:
The actual_length in dwc2_hcd_urb structure is used
to indicate the total data length transferred so far,
but in dwc2_update_isoc_urb_state(), it just updates
the actual_length of isoc frame, and don't update the
urb actual_length at the same time, this will cause
device drivers working error which depend on the urb
actual_length.
we can easily find this issue if use an USB camera，
the userspace use libusb to get USB data from kernel
via devio driver.In usb devio driver, processcompl()
function will process urb complete and copy data to
userspace depending on urb actual_length.
Let's update the urb actual_length if the isoc frame
is valid.
Signed-off-by: William Wu
---
drivers/usb/dwc2/hcd_intr.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
index 28a8210..01b1e13 100644
--- a/drivers/usb/dwc2/hcd_intr.c
+++ b/drivers/usb/dwc2/hcd_intr.c
@@ -580,6 +580,7 @@ static enum dwc2_halt_status dwc2_update_isoc_urb_state(
frame_desc->status = 0;
frame_desc->actual_length = dwc2_get_actual_xfer_length(hsotg,
chan, chnum, qtd, halt_status, NULL);
+ urb->actual_length += frame_desc->actual_length;
break;
case DWC2_HC_XFER_FRAME_OVERRUN:
urb->error_count++;
@@ -599,6 +600,7 @@ static enum dwc2_halt_status dwc2_update_isoc_urb_state(
frame_desc->status = -EPROTO;
frame_desc->actual_length = dwc2_get_actual_xfer_length(hsotg,
chan, chnum, qtd, halt_status, NULL);
+ urb->actual_length += frame_desc->actual_length;
/* Skip whole frame */
if (chan->qh->do_split &&
According urb description in usb.h urb->actual_length used for non-iso
transfers:
"@actual_length: This is read in non-iso completion functions, and ...
* ISO transfer status is reported in the status and actual_length fields
* of the iso_frame_desc array, "
Yes, urb->actual_length is used for non-iso transfers. And for ISO
transfer, the most
usb class drivers can only use iso frame status and actual_length to
handle usb data,
like: kernel v4.4 drivers/media/usb/uvc/uvc_video.c
But the usb devio driver really need the urb actual_length in the
processcompl() if
handle ISO transfer, just as I mentioned in the commit message.
And I also found the same issue on raspberrypi board:
https://github.com/raspberrypi/linux/issues/903
So do you think we need to fix the usb devio driver, rather than fix dwc2?
I think maybe we can use urb actual length for ISO transfer, it seems that
don't cause any side-effect.
That sounds like a good idea. Minas, does the following patch fix your
problem?
In theory we could do this calculation for every isochronous URB, not
just those coming from usbfs. But I don't think there's any point,
since the USB class drivers don't use it.
Alan Stern
Index: usb-4.x/drivers/usb/core/devio.c
===
--- usb-4.x.orig/drivers/usb/core/devio.c
+++ usb-4.x/drivers/usb/core/devio.c
@@ -1828,6 +1828,18 @@ static int proc_unlinkurb(struct usb_dev
return 0;
}
+static void compute_isochronous_actual_length(struct urb *urb)
+{
+ unsigned i;
+
+ if (urb->number_of_packets > 0) {
+ urb->actual_length = 0;
+ for (i = 0; i < urb->number_of_packets; i++)
+ urb->actual_length +=
+ urb->iso_frame_desc[i].actual_length;
+ }
+}
+
static int processcompl(struct async *as, void __user * __user *arg)
{
struct urb *urb = as->urb;
@@ -1835,6 +1847,8 @@ static int processcompl(struct async *as
void __user *addr = as->userurb;
unsigned int i;
+ compute_isochronous_actual_length(urb);
+
if (as->userbuffer && urb->actual_length) {
if (copy_urb_data_to_user(as->userbuffer, urb))
goto err_out;
@@ -2003,6 +2017,8 @@ static int processcompl_compat(struct as
void __user *addr = as->userurb;
unsigned int i;
+ compute_isochronous_actual_length(urb);
+
if (as->userbuffer && urb->actual_length) {
if (copy_urb_data_to_user(as->userbuffer, urb))
return -EFAULT;
Yeah, it's a good idea to calculate the urb actual length in the devio
driver.
Your patch seems good, and I think we can do a small optimization base
your patch,
like the following patch:
diff --git a/drivers/usb/core/dev
Re: [PATCH] usb: dwc2: host: fix isoc urb actual length
Hi Alan,
在 2017年11月07日 23:18, Alan Stern 写道:
On Tue, 7 Nov 2017, wlf wrote:
That sounds like a good idea. Minas, does the following patch fix your
problem?
In theory we could do this calculation for every isochronous URB, not
just those coming from usbfs. But I don't think there's any point,
since the USB class drivers don't use it.
Alan Stern
Index: usb-4.x/drivers/usb/core/devio.c
===
--- usb-4.x.orig/drivers/usb/core/devio.c
+++ usb-4.x/drivers/usb/core/devio.c
@@ -1828,6 +1828,18 @@ static int proc_unlinkurb(struct usb_dev
return 0;
}
+static void compute_isochronous_actual_length(struct urb *urb)
+{
+ unsigned i;
+
+ if (urb->number_of_packets > 0) {
+ urb->actual_length = 0;
+ for (i = 0; i < urb->number_of_packets; i++)
+ urb->actual_length +=
+ urb->iso_frame_desc[i].actual_length;
+ }
+}
+
static int processcompl(struct async *as, void __user * __user *arg)
{
struct urb *urb = as->urb;
@@ -1835,6 +1847,8 @@ static int processcompl(struct async *as
void __user *addr = as->userurb;
unsigned int i;
+ compute_isochronous_actual_length(urb);
+
if (as->userbuffer && urb->actual_length) {
if (copy_urb_data_to_user(as->userbuffer, urb))
goto err_out;
@@ -2003,6 +2017,8 @@ static int processcompl_compat(struct as
void __user *addr = as->userurb;
unsigned int i;
+ compute_isochronous_actual_length(urb);
+
if (as->userbuffer && urb->actual_length) {
if (copy_urb_data_to_user(as->userbuffer, urb))
return -EFAULT;
Yeah, it's a good idea to calculate the urb actual length in the devio
driver.
Your patch seems good, and I think we can do a small optimization base
your patch,
like the following patch:
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index bd94192..a2e7b97 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -1664,6 +1664,9 @@ static int processcompl(struct async *as, void
__user * __user *arg)
void __user *addr = as->userurb;
unsigned int i;
+ if (usb_endpoint_xfer_isoc(&urb->ep->desc))
+ compute_isochronous_actual_length(urb);
+
if (as->userbuffer && urb->actual_length) {
if (copy_urb_data_to_user(as->userbuffer, urb))
goto err_out;
@@ -1833,6 +1836,9 @@ static int processcompl_compat(struct async *as,
void __user * __user *arg)
void __user *addr = as->userurb;
unsigned int i;
+ if (usb_endpoint_xfer_isoc(&urb->ep->desc))
+ compute_isochronous_actual_length(urb);
+
Well, this depends on whether you want to optimize for space or for
speed. I've been going for space. And since usbfs is inherently
rather slow, I don't think this will make any significant speed
difference. So I don't think adding the extra tests is worthwhile.
(Besides, if you really wanted to do it this way, you should have moved
the test for "urb->number_of_packets > 0" into the callers instead of
adding an additional test of the endpoint type.)
Yes, agree with you.
However, nobody has answered my original question: Does the patch
actually fix the problem?
I test your patch on Rockchip RK3188 platform, use UsbWebCamera APP by
Serenegiant
(libusb + devio) with usb camera, it work well.
Alan Stern
--
吴良峰 William.Wu
福建省福州市铜盘路软件大道89号软件园A区21号楼
No.21 Building, A District, No.89,software Boulevard Fuzhou,Fujian, PRC
手机: 13685012275
座机: 0591-83991906-8520
邮件:w...@rock-chips.com
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: gadget: f_fs: get the correct address of comp_desc
Hi Jack,
在 2018年02月06日 02:17, Jack Pham 写道:
Hi William,
On Mon, Feb 05, 2018 at 07:33:38PM +0800, William Wu wrote:
Refer to the USB 3.0 spec '9.6.7 SuperSpeed Endpoint Companion',
the companion descriptor follows the standard endpoint descriptor.
This descriptor is only defined for SuperSpeed endpoints. The
f_fs driver gets the address of the companion descriptor via
'ds + USB_DT_ENDPOINT_SIZE', and actually, the ds variable is
a pointer to the struct usb_endpoint_descriptor, so the offset
of the companion descriptor which we get is USB_DT_ENDPOINT_SIZE *
sizeof(struct usb_endpoint_descriptor), the wrong offset is 63
bytes. This cause out-of-bound with the following error log if
CONFIG_KASAN and CONFIG_SLUB_DEBUG is enabled on Rockchip RK3399
Evaluation Board.
android_work: sent uevent USB_STATE=CONNECTED
configfs-gadget gadget: super-speed config #1: b
==
BUG: KASAN: slab-out-of-bounds in ffs_func_set_alt+0x230/0x398
Read of size 1 at addr ffc0ce2d0b10 by task irq/224-dwc3/364
Memory state around the buggy address:
ffc0ce2d0a00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
ffc0ce2d0a80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>ffc0ce2d0b00: 00 04 fc fc fc fc fc fc fc fc fc fc fc fc fc fc
^
ffc0ce2d0b80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
ffc0ce2d0c00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
==
Disabling lock debugging due to kernel taint
android_work: sent uevent USB_STATE=CONFIGURED
This patch adds struct usb_endpoint_descriptor * -> u8 * type conversion
for ds variable, then we can get the correct address of comp_desc
with offset USB_DT_ENDPOINT_SIZE bytes.
Signed-off-by: William Wu
---
drivers/usb/gadget/function/f_fs.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/gadget/function/f_fs.c
b/drivers/usb/gadget/function/f_fs.c
index 6756472..f13ead0 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -1882,8 +1882,8 @@ static int ffs_func_eps_enable(struct ffs_function *func)
ep->ep->desc = ds;
if (needs_comp_desc) {
- comp_desc = (struct usb_ss_ep_comp_descriptor *)(ds +
- USB_DT_ENDPOINT_SIZE);
+ comp_desc = (struct usb_ss_ep_comp_descriptor *)
+((u8 *)ds + USB_DT_ENDPOINT_SIZE);
ep->ep->maxburst = comp_desc->bMaxBurst + 1;
ep->ep->comp_desc = comp_desc;
}
Please see my alternative fix for this. I proposed changing this
function to use config_ep_by_speed() instead.
https://www.spinics.net/lists/linux-usb/msg165149.html
Thanks for your great job!
Your patch seems good, I will test your patch on my RK3399-EVB board.
William
Jack
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] usb: dwc3: add disable u2mac linestate check quirk
Dear Guenter,
在 2017年04月18日 21:18, Guenter Roeck 写道:
On Mon, Apr 17, 2017 at 10:17 PM, William Wu wrote:
This patch adds a quirk to disable USB 2.0 MAC linestate check
during HS transmit. Refer the dwc3 databook, we can use it for
some special platforms if the linestate not reflect the expected
line state(J) during transmission.
When use this quirk, the controller implements a fixed 40-bit
TxEndDelay after the packet is given on UTMI and ignores the
linestate during the transmit of a token (during token-to-token
and token-to-data IPGAP).
On some rockchip platforms (e.g. rk3399), it requires to disable
the u2mac linestate check to decrease the SSPLIT token to SETUP
token inter-packet delay from 566ns to 466ns, and fix the issue
that FS/LS devices not recognized if inserted through USB 3.0 HUB.
Signed-off-by: William Wu
---
Changes in v2:
- fix coding style
Documentation/devicetree/bindings/usb/dwc3.txt | 2 ++
drivers/usb/dwc3/core.c| 14 ++
drivers/usb/dwc3/core.h| 4
3 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt
b/Documentation/devicetree/bindings/usb/dwc3.txt
index f658f39..6a89f0c 100644
--- a/Documentation/devicetree/bindings/usb/dwc3.txt
+++ b/Documentation/devicetree/bindings/usb/dwc3.txt
@@ -45,6 +45,8 @@ Optional properties:
a free-running PHY clock.
- snps,dis-del-phy-power-chg-quirk: when set core will change PHY power
from P0 to P1/P2/P3 without delay.
+ - snps,tx-ipgap-linecheck-dis-quirk: when set, disable u2mac linestate check
+ during HS transmit.
All other disable-something quirks are named
"snps,dis-something-quirk". Maybe use the same naming convention ?
Yes, good idea！ I will fix it with "snps,dis-tx-ipgap-linecheck-quirk"
in next patch verison.
Thanks:-)
- snps,is-utmi-l1-suspend: true when DWC3 asserts output signal
utmi_l1_suspend_n, false when asserts utmi_sleep_n
- snps,hird-threshold: HIRD threshold
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 455d89a..03429c5 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -796,15 +796,19 @@ static int dwc3_core_init(struct dwc3 *dwc)
dwc3_writel(dwc->regs, DWC3_GUCTL2, reg);
}
+ reg = dwc3_readl(dwc->regs, DWC3_GUCTL1);
+
My understanding is that the register was only introduced with dwc3
revision 2.50a. Is it ok to read and write it unconditionally ?
Yes, refer to dwc3 databook, the DWC3_GUCTL1 was introduced since 2.50a.
Maybe it's better
to read and write it only when we know our controller version.
Is it good to fix it like the following patch?
But this patch has a problem that we need to read and write the register
twice if our controller verison > = 2.90a, and need this quirk.
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -806,6 +806,12 @@ static int dwc3_core_init(struct dwc3 *dwc)
dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
}
+ if (dwc->dis_tx_ipgap_linecheck_quirk) {
+ reg = dwc3_readl(dwc->regs, DWC3_GUCTL1);
+ reg |= DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS;
+ dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
+ }
+
Hi John & Felipe,
Could you provide me some suggestion？
Thank you！
/*
* Enable hardware control of sending remote wakeup in HS when
* the device is in the L1 state.
*/
- if (dwc->revision >= DWC3_REVISION_290A) {
- reg = dwc3_readl(dwc->regs, DWC3_GUCTL1);
+ if (dwc->revision >= DWC3_REVISION_290A)
reg |= DWC3_GUCTL1_DEV_L1_EXIT_BY_HW;
- dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
- }
+
+ if (dwc->tx_ipgap_linecheck_dis_quirk)
+ reg |= DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS;
+
+ dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
return 0;
@@ -1023,6 +1027,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
"snps,dis-u2-freeclk-exists-quirk");
dwc->dis_del_phy_power_chg_quirk = device_property_read_bool(dev,
"snps,dis-del-phy-power-chg-quirk");
+ dwc->tx_ipgap_linecheck_dis_quirk = device_property_read_bool(dev,
+ "snps,tx-ipgap-linecheck-dis-quirk");
dwc->tx_de_emphasis_quirk = device_property_read_bool(dev,
"snps,tx_de_emphasis_quirk");
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 981c77f..3c2537b 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -204,6 +204,7 @@
#define DWC3_GCTL_DSBLCLKGTNG BIT(0)
/* Global User Control 1 Register */
+#define DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS BIT(28)
#define DWC3_GUCTL1_DEV_L1_EXIT_BY_HW BIT(24)
/* Global USB2 P
Re: [PATCH v2] usb: dwc3: add disable u2mac linestate check quirk
Dear Guenter，
在 2017年04月19日 13:15, Guenter Roeck 写道:
On Tue, Apr 18, 2017 at 8:59 PM, wlf wrote:
Dear Guenter,
在 2017年04月18日 21:18, Guenter Roeck 写道:
On Mon, Apr 17, 2017 at 10:17 PM, William Wu
wrote:
This patch adds a quirk to disable USB 2.0 MAC linestate check
during HS transmit. Refer the dwc3 databook, we can use it for
some special platforms if the linestate not reflect the expected
line state(J) during transmission.
When use this quirk, the controller implements a fixed 40-bit
TxEndDelay after the packet is given on UTMI and ignores the
linestate during the transmit of a token (during token-to-token
and token-to-data IPGAP).
On some rockchip platforms (e.g. rk3399), it requires to disable
the u2mac linestate check to decrease the SSPLIT token to SETUP
token inter-packet delay from 566ns to 466ns, and fix the issue
that FS/LS devices not recognized if inserted through USB 3.0 HUB.
Signed-off-by: William Wu
---
Changes in v2:
- fix coding style
Documentation/devicetree/bindings/usb/dwc3.txt | 2 ++
drivers/usb/dwc3/core.c| 14 ++
drivers/usb/dwc3/core.h| 4
3 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt
b/Documentation/devicetree/bindings/usb/dwc3.txt
index f658f39..6a89f0c 100644
--- a/Documentation/devicetree/bindings/usb/dwc3.txt
+++ b/Documentation/devicetree/bindings/usb/dwc3.txt
@@ -45,6 +45,8 @@ Optional properties:
a free-running PHY clock.
- snps,dis-del-phy-power-chg-quirk: when set core will change PHY
power
from P0 to P1/P2/P3 without delay.
+ - snps,tx-ipgap-linecheck-dis-quirk: when set, disable u2mac linestate
check
+ during HS transmit.
All other disable-something quirks are named
"snps,dis-something-quirk". Maybe use the same naming convention ?
Yes, good idea！ I will fix it with "snps,dis-tx-ipgap-linecheck-quirk" in
next patch verison.
Thanks:-)
- snps,is-utmi-l1-suspend: true when DWC3 asserts output signal
utmi_l1_suspend_n, false when asserts
utmi_sleep_n
- snps,hird-threshold: HIRD threshold
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 455d89a..03429c5 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -796,15 +796,19 @@ static int dwc3_core_init(struct dwc3 *dwc)
dwc3_writel(dwc->regs, DWC3_GUCTL2, reg);
}
+ reg = dwc3_readl(dwc->regs, DWC3_GUCTL1);
+
My understanding is that the register was only introduced with dwc3
revision 2.50a. Is it ok to read and write it unconditionally ?
Yes, refer to dwc3 databook, the DWC3_GUCTL1 was introduced since 2.50a.
Maybe it's better
to read and write it only when we know our controller version.
Is it good to fix it like the following patch?
But this patch has a problem that we need to read and write the register
twice if our controller verison > = 2.90a, and need this quirk.
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -806,6 +806,12 @@ static int dwc3_core_init(struct dwc3 *dwc)
dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
}
+ if (dwc->dis_tx_ipgap_linecheck_quirk) {
+ reg = dwc3_readl(dwc->regs, DWC3_GUCTL1);
+ reg |= DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS;
+ dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
+ }
+
How about this ?
if (dwc->revision >= DWC3_REVISION_250A) {
reg = dwc3_readl(dwc->regs, DWC3_GUCTL1);
if (dwc->revision >= DWC3_REVISION_290A)
reg |= DWC3_GUCTL1_DEV_L1_EXIT_BY_HW;
if (dwc->dis_tx_ipgap_linecheck_quirk)
reg |= DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS;
dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
}
Thanks,
Guenter
Thanks， looks good to me. I will fix it in patch v2.
Hi John & Felipe,
Could you provide me some suggestion？
Thank you！
/*
* Enable hardware control of sending remote wakeup in HS when
* the device is in the L1 state.
*/
- if (dwc->revision >= DWC3_REVISION_290A) {
- reg = dwc3_readl(dwc->regs, DWC3_GUCTL1);
+ if (dwc->revision >= DWC3_REVISION_290A)
reg |= DWC3_GUCTL1_DEV_L1_EXIT_BY_HW;
- dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
- }
+
+ if (dwc->tx_ipgap_linecheck_dis_quirk)
+ reg |= DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS;
+
+ dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
return 0;
@@ -1023,6 +1027,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
"snps,dis-u2-freeclk-exists-quirk");
dwc->dis_del_phy_power_chg_quirk =
device_property_read_bool(dev,
&q
