* Re: [PATCH] usb: dwc2: extend treatment for incomplete transfer @ 2020-02-10 21:39 Guenter Roeck 2020-02-11 5:49 ` Boris ARZUR 0 siblings, 1 reply; 20+ messages in thread From: Guenter Roeck @ 2020-02-10 21:39 UTC (permalink / raw) To: Boris ARZUR Cc: linux-usb, FelipeBalbi, Greg Kroah-Hartman, Minas Harutyunyan, William Wu, Dmitry Torokhov, Douglas Anderson Hi Boris, On Mon, Feb 10, 2020 at 09:29:10PM +0000, Boris ARZUR wrote: > <felipe.balbi@linux.intel.com>, Greg Kroah-Hartman > <gregkh@linuxfoundation.org>, Minas Harutyunyan <hminas@synopsys.com>, > William Wu <william.wu@rock-chips.com>, Dmitry Torokhov > <dmitry.torokhov@gmail.com>, Douglas Anderson <dianders@chromium.org> > > > Hello Guenter, > > > >good find, and good analysis. We stated to see this problem as well in the > >latest ChromeOS kernel. > I'm glad you find my report helpful. > > > >be able to reproduce the problem. Maybe you can help me. How do you tether > >your phone through USB ? > You mention thethering, so I think you have read my follow-up: > https://www.spinics.net/lists/linux-usb/msg187497.html > Unfortunately, I have been unable to reproduce the problem. It is seen only with certain phones and with certain Ethernet adapters, and I was unable to get any of those. I'll keep trying. In the meantime, can you by any chance test the attached patch ? It _might_ fix the problem, but it is a bit of a wild shot. Thanks, Guenter --- From 29e0949531a27f14a5b46d70e34aa43546e6a3d1 Mon Sep 17 00:00:00 2001 From: Guenter Roeck <linux@roeck-us.net> Date: Mon, 10 Feb 2020 13:11:00 -0800 Subject: [PATCH] usb: dwc2: constrain endpoint transfer size on split IN The following messages are seen on Veyron Chromebooks running v4.19 or later kernels. dwc2 ff580000.usb: dwc2_update_urb_state(): trimming xfer length dwc2 ff580000.usb: dwc2_hc_chhltd_intr_dma: Channel 7 - ChHltd set, but reason is unknown dwc2 ff580000.usb: hcint 0x00000002, intsts 0x04600021 This is typically followed by a crash. Unable to handle kernel paging request at virtual address 29f9d9fc pgd = 4797dac9 [29f9d9fc] *pgd=80000000004003, *pmd=00000000 Internal error: Oops: a06 [#1] PREEMPT SMP ARM Modules linked in: ip6t_REJECT rfcomm i2c_dev uinput hci_uart btbcm ... CPU: 0 PID: 0 Comm: swapper/0 Tainted: G W 4.19.87-07825-g4ab3515f6e4d #1 Hardware name: Rockchip (Device Tree) PC is at memcpy+0x50/0x330 LR is at 0xdd9ac94 ... [<c0a89f50>] (memcpy) from [<c0783b94>] (dwc2_free_dma_aligned_buffer+0x5c/0x7c) [<c0783b94>] (dwc2_free_dma_aligned_buffer) from [<c0765dcc>] (__usb_hcd_giveback_urb+0x78/0x130) [<c0765dcc>] (__usb_hcd_giveback_urb) from [<c07678fc>] (usb_giveback_urb_bh+0xa0/0xe4) [<c07678fc>] (usb_giveback_urb_bh) from [<c023a164>] (tasklet_action_common+0xc0/0xdc) [<c023a164>] (tasklet_action_common) from [<c02021f0>] (__do_softirq+0x1b8/0x434) [<c02021f0>] (__do_softirq) from [<c0239a14>] (irq_exit+0xdc/0xe0) [<c0239a14>] (irq_exit) from [<c029f260>] (__handle_domain_irq+0x94/0xd0) [<c029f260>] (__handle_domain_irq) from [<c05da780>] (gic_handle_irq+0x74/0xb0) [<c05da780>] (gic_handle_irq) from [<c02019f8>] (__irq_svc+0x58/0x8c) The crash suggests that the memory after the end of a temporary DMA-aligned buffer is overwritten. The Raspberry Pi Linux kernel includes a patch suggesting that a similar problem was observed with the dwg2 otc driver used there. The patch description is as follows. The hcd would unconditionally set the transfer length to the endpoint packet size for non-isoc IN transfers. If the remaining buffer length was less than the length of returned data, random memory would get scribbled over, with bad effects if it crossed a page boundary. Force a babble error if this happens by limiting the max transfer size to the available buffer space. DMA will stop writing to memory on a babble condition. Apply the same fix to this driver. Reported-by: Boris ARZUR <boris@konbu.org> Cc: Boris ARZUR <boris@konbu.org> Cc: Jonathan Bell <jonathan@raspberrypi.org> Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- drivers/usb/dwc2/hcd.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c index b90f858af960..2c81b346b464 100644 --- a/drivers/usb/dwc2/hcd.c +++ b/drivers/usb/dwc2/hcd.c @@ -1264,7 +1264,8 @@ static void dwc2_hc_start_transfer(struct dwc2_hsotg *hsotg, */ chan->xfer_len = 0; else if (chan->ep_is_in || chan->xfer_len > chan->max_packet) - chan->xfer_len = chan->max_packet; + chan->xfer_len = min_t(uint32_t, chan->xfer_len, + chan->max_packet); else if (!chan->ep_is_in && chan->xfer_len > 188) chan->xfer_len = 188; -- 2.17.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH] usb: dwc2: extend treatment for incomplete transfer 2020-02-10 21:39 [PATCH] usb: dwc2: extend treatment for incomplete transfer Guenter Roeck @ 2020-02-11 5:49 ` Boris ARZUR 2020-02-11 13:26 ` Guenter Roeck 2020-02-11 16:15 ` Guenter Roeck 0 siblings, 2 replies; 20+ messages in thread From: Boris ARZUR @ 2020-02-11 5:49 UTC (permalink / raw) To: Guenter Roeck Cc: linux-usb, FelipeBalbi, Greg Kroah-Hartman, Minas Harutyunyan, William Wu, Dmitry Torokhov, Douglas Anderson Hello Guenter, >In the meantime, can you by any chance test the attached patch ? It _might_ >fix the problem, but it is a bit of a wild shot. I tried your patch, but the machine does not finish booting. I would like to give you a dump, but the screen scrolls fast, and what's left when paused is not interesting. How do I get it to dump on disk? My journalctl doesn't show anything. I have no kmesg.log anywhere. The first time around I was 0/ changing fonts 1/ trimming the dump message in the kernel 2/ filming my screen. That's not practical at all... I have been looking a bit at things. I believe that part of the issue is the need to re-align the buffer we get in the URB. I'm wondering if asking for a specific alignment when creating the URB could be doable. As a stop-gap, maybe doing things like in tegra ehci could fix our bug: https://github.com/torvalds/linux/blob/master/drivers/usb/host/ehci-tegra.c#L288 i.e. having the old pointer before the new buffer instead of at the end of it. Now if something is overwriting the buffer end, that would also be hiding the issue... but if the bug is related to lengths that don't match between allocation and free, that could work. In this case, that would also be hiding the issue :) >Unfortunately, I have been unable to reproduce the problem. It is seen only >with certain phones and with certain Ethernet adapters, and I was unable >to get any of those. I'll keep trying. If you want, I can run a kernel with some printk instrumentation or run experiments. I'll research a bit on how to get that kernel oops data, that does not involve serial or net console. Thanks, have a good day, Boris. Guenter Roeck wrote: >Hi Boris, > >On Mon, Feb 10, 2020 at 09:29:10PM +0000, Boris ARZUR wrote: >> <felipe.balbi@linux.intel.com>, Greg Kroah-Hartman >> <gregkh@linuxfoundation.org>, Minas Harutyunyan <hminas@synopsys.com>, >> William Wu <william.wu@rock-chips.com>, Dmitry Torokhov >> <dmitry.torokhov@gmail.com>, Douglas Anderson <dianders@chromium.org> >> >> >> Hello Guenter, >> >> >> >good find, and good analysis. We stated to see this problem as well in the >> >latest ChromeOS kernel. >> I'm glad you find my report helpful. >> >> >> >be able to reproduce the problem. Maybe you can help me. How do you tether >> >your phone through USB ? >> You mention thethering, so I think you have read my follow-up: >> https://www.spinics.net/lists/linux-usb/msg187497.html >> > >Unfortunately, I have been unable to reproduce the problem. It is seen only >with certain phones and with certain Ethernet adapters, and I was unable >to get any of those. I'll keep trying. > >In the meantime, can you by any chance test the attached patch ? It _might_ >fix the problem, but it is a bit of a wild shot. > >Thanks, >Guenter > >--- >From 29e0949531a27f14a5b46d70e34aa43546e6a3d1 Mon Sep 17 00:00:00 2001 >From: Guenter Roeck <linux@roeck-us.net> >Date: Mon, 10 Feb 2020 13:11:00 -0800 >Subject: [PATCH] usb: dwc2: constrain endpoint transfer size on split IN > >The following messages are seen on Veyron Chromebooks running v4.19 or >later kernels. > >dwc2 ff580000.usb: dwc2_update_urb_state(): trimming xfer length >dwc2 ff580000.usb: dwc2_hc_chhltd_intr_dma: Channel 7 - ChHltd set, but reason is unknown >dwc2 ff580000.usb: hcint 0x00000002, intsts 0x04600021 > >This is typically followed by a crash. > >Unable to handle kernel paging request at virtual address 29f9d9fc >pgd = 4797dac9 >[29f9d9fc] *pgd=80000000004003, *pmd=00000000 >Internal error: Oops: a06 [#1] PREEMPT SMP ARM >Modules linked in: ip6t_REJECT rfcomm i2c_dev uinput hci_uart btbcm ... >CPU: 0 PID: 0 Comm: swapper/0 Tainted: G W 4.19.87-07825-g4ab3515f6e4d #1 >Hardware name: Rockchip (Device Tree) >PC is at memcpy+0x50/0x330 >LR is at 0xdd9ac94 >... >[<c0a89f50>] (memcpy) from [<c0783b94>] (dwc2_free_dma_aligned_buffer+0x5c/0x7c) >[<c0783b94>] (dwc2_free_dma_aligned_buffer) from [<c0765dcc>] (__usb_hcd_giveback_urb+0x78/0x130) >[<c0765dcc>] (__usb_hcd_giveback_urb) from [<c07678fc>] (usb_giveback_urb_bh+0xa0/0xe4) >[<c07678fc>] (usb_giveback_urb_bh) from [<c023a164>] (tasklet_action_common+0xc0/0xdc) >[<c023a164>] (tasklet_action_common) from [<c02021f0>] (__do_softirq+0x1b8/0x434) >[<c02021f0>] (__do_softirq) from [<c0239a14>] (irq_exit+0xdc/0xe0) >[<c0239a14>] (irq_exit) from [<c029f260>] (__handle_domain_irq+0x94/0xd0) >[<c029f260>] (__handle_domain_irq) from [<c05da780>] (gic_handle_irq+0x74/0xb0) >[<c05da780>] (gic_handle_irq) from [<c02019f8>] (__irq_svc+0x58/0x8c) > >The crash suggests that the memory after the end of a temporary DMA-aligned >buffer is overwritten. > >The Raspberry Pi Linux kernel includes a patch suggesting that a similar >problem was observed with the dwg2 otc driver used there. The patch >description is as follows. > > The hcd would unconditionally set the transfer length to the endpoint > packet size for non-isoc IN transfers. If the remaining buffer length > was less than the length of returned data, random memory would get > scribbled over, with bad effects if it crossed a page boundary. > > Force a babble error if this happens by limiting the max transfer size > to the available buffer space. DMA will stop writing to memory on a > babble condition. > >Apply the same fix to this driver. > >Reported-by: Boris ARZUR <boris@konbu.org> >Cc: Boris ARZUR <boris@konbu.org> >Cc: Jonathan Bell <jonathan@raspberrypi.org> >Signed-off-by: Guenter Roeck <linux@roeck-us.net> >--- > drivers/usb/dwc2/hcd.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > >diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c >index b90f858af960..2c81b346b464 100644 >--- a/drivers/usb/dwc2/hcd.c >+++ b/drivers/usb/dwc2/hcd.c >@@ -1264,7 +1264,8 @@ static void dwc2_hc_start_transfer(struct dwc2_hsotg *hsotg, > */ > chan->xfer_len = 0; > else if (chan->ep_is_in || chan->xfer_len > chan->max_packet) >- chan->xfer_len = chan->max_packet; >+ chan->xfer_len = min_t(uint32_t, chan->xfer_len, >+ chan->max_packet); > else if (!chan->ep_is_in && chan->xfer_len > 188) > chan->xfer_len = 188; > >-- >2.17.1 > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] usb: dwc2: extend treatment for incomplete transfer 2020-02-11 5:49 ` Boris ARZUR @ 2020-02-11 13:26 ` Guenter Roeck 2020-02-11 16:15 ` Guenter Roeck 1 sibling, 0 replies; 20+ messages in thread From: Guenter Roeck @ 2020-02-11 13:26 UTC (permalink / raw) To: Boris ARZUR Cc: linux-usb, FelipeBalbi, Greg Kroah-Hartman, Minas Harutyunyan, William Wu, Dmitry Torokhov, Douglas Anderson On 2/10/20 9:49 PM, Boris ARZUR wrote: > Hello Guenter, > >> In the meantime, can you by any chance test the attached patch ? It _might_ >> fix the problem, but it is a bit of a wild shot. > I tried your patch, but the machine does not finish booting. > Weird. Can you enable pstore ? That should work on this system. Then you would have the log from the previous boot in /sys/fs/pstore/. > > I would like to give you a dump, but the screen scrolls fast, and what's > left when paused is not interesting. How do I get it to dump on disk? > > My journalctl doesn't show anything. I have no kmesg.log anywhere. > The first time around I was 0/ changing fonts 1/ trimming the dump message > in the kernel 2/ filming my screen. That's not practical at all... > > > I have been looking a bit at things. I believe that part of the issue > is the need to re-align the buffer we get in the URB. I'm wondering if asking > for a specific alignment when creating the URB could be doable. > > > As a stop-gap, maybe doing things like in tegra ehci could fix our bug: > https://github.com/torvalds/linux/blob/master/drivers/usb/host/ehci-tegra.c#L288 > i.e. having the old pointer before the new buffer instead of at the end of > it. > Yes, that was the original solution. Unfortunately it didn't really DMA-align buffers, so the temporary pointer was moved to the end. It still doesn't guarantee DMA alignment, though, so I am working on fixing that and moving the old pointer back to the beginning. > Now if something is overwriting the buffer end, that would also be hiding the > issue... but if the bug is related to lengths that don't match between > allocation and free, that could work. In this case, that would also be > hiding the issue :) > Yes, that is why moving the old pointer to the beginning won't be sufficient. Either case, if the current patch causes a boot loop there is obviously something wrong with it, or the fix is incomplete. I'll keep trying to get equipment that lets me reproduce the problem. Thanks for trying! Guenter > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] usb: dwc2: extend treatment for incomplete transfer 2020-02-11 5:49 ` Boris ARZUR 2020-02-11 13:26 ` Guenter Roeck @ 2020-02-11 16:15 ` Guenter Roeck 2020-02-15 5:36 ` Boris ARZUR 1 sibling, 1 reply; 20+ messages in thread From: Guenter Roeck @ 2020-02-11 16:15 UTC (permalink / raw) To: Boris ARZUR Cc: linux-usb, FelipeBalbi, Greg Kroah-Hartman, Minas Harutyunyan, William Wu, Dmitry Torokhov, Douglas Anderson Hi Boris, On Tue, Feb 11, 2020 at 02:49:53PM +0900, Boris ARZUR wrote: > Hello Guenter, > > >In the meantime, can you by any chance test the attached patch ? It _might_ > >fix the problem, but it is a bit of a wild shot. > I tried your patch, but the machine does not finish booting. > > > I would like to give you a dump, but the screen scrolls fast, and what's > left when paused is not interesting. How do I get it to dump on disk? > > My journalctl doesn't show anything. I have no kmesg.log anywhere. > The first time around I was 0/ changing fonts 1/ trimming the dump message > in the kernel 2/ filming my screen. That's not practical at all... > > > I have been looking a bit at things. I believe that part of the issue > is the need to re-align the buffer we get in the URB. I'm wondering if asking > for a specific alignment when creating the URB could be doable. > > > As a stop-gap, maybe doing things like in tegra ehci could fix our bug: > https://github.com/torvalds/linux/blob/master/drivers/usb/host/ehci-tegra.c#L288 > i.e. having the old pointer before the new buffer instead of at the end of > it. > > Now if something is overwriting the buffer end, that would also be hiding the > issue... but if the bug is related to lengths that don't match between > allocation and free, that could work. In this case, that would also be > hiding the issue :) > See below for a patch (untested) doing just that. It may solve your immediate problem, though it would still suffer from the buffer end overwrite. > > >Unfortunately, I have been unable to reproduce the problem. It is seen only > >with certain phones and with certain Ethernet adapters, and I was unable > >to get any of those. I'll keep trying. > If you want, I can run a kernel with some printk instrumentation or run > experiments. I'll research a bit on how to get that kernel oops data, that Unfortunately I have no real idea what to look out for. The problem may be related to the phone sending more than one Ethernet packet in a single USB transfer. See rndis_rx_fixup() for how that is handled in the driver. I don't know how that would result in the observed problem, though. Thanks, Guenter --- From 8efa9c598f2390dca2e97cbbe41e981caba41ca1 Mon Sep 17 00:00:00 2001 From: Guenter Roeck <linux@roeck-us.net> Date: Mon, 10 Feb 2020 14:04:06 -0800 Subject: [PATCH] usb: dwc2: Simplify DMA alignment code The code to align buffers for DMA was first introduced with commit 3bc04e28a030 ("usb: dwc2: host: Get aligned DMA in a more supported way"). It was updated with commit 56406e017a88 ("usb: dwc2: Fix DMA alignment to start at allocated boundary") because it did not really align buffers to DMA boundaries but to DWC2_USB_DMA_ALIGN. This was then optimized in commit 1e111e885238 ("usb: dwc2: Fix inefficient copy of unaligned buffers") to only copy actual data rather than the whole buffer. Commit 4a4863bf2e79 ("usb: dwc2: Fix DMA cache alignment issues") changed this further to add a padding at the end of the buffer to ensure that the old data pointer is not in the same cache line as the buffer. This last commit states "Otherwise, the stored_xfer_buffer gets corrupted for IN URBs on non-cache-coherent systems". However, such corruptions are still observed. Either case, DMA is not expected to overwrite more memory than it is supposed to do, suggesting that the commit may have been hiding a problem rather than fixing it. On top of that, DMA alignment is still not guaranteed since it only happens if the original buffer is not aligned to DWC2_USB_DMA_ALIGN, which is still a constant of 4 and not associated with DMA alignment. Move the old data pointer back to the beginning of the new buffer, restoring most of the original commit and to simplify the code. Define DWC2_USB_DMA_ALIGN to dma_get_cache_alignment() to fix the DMA alignment for real this time. Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- drivers/usb/dwc2/hcd.c | 50 +++++++++++++++++++----------------------- 1 file changed, 22 insertions(+), 28 deletions(-) diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c index 2c81b346b464..9e04b3a314eb 100644 --- a/drivers/usb/dwc2/hcd.c +++ b/drivers/usb/dwc2/hcd.c @@ -2470,21 +2470,24 @@ static int dwc2_alloc_split_dma_aligned_buf(struct dwc2_hsotg *hsotg, return 0; } -#define DWC2_USB_DMA_ALIGN 4 +#define DWC2_USB_DMA_ALIGN dma_get_cache_alignment() + +struct dma_aligned_buffer { + void *kmalloc_ptr; + void *old_xfer_buffer; + u8 data[0]; +}; static void dwc2_free_dma_aligned_buffer(struct urb *urb) { - void *stored_xfer_buffer; + struct dma_aligned_buffer *temp; size_t length; if (!(urb->transfer_flags & URB_ALIGNED_TEMP_BUFFER)) return; - /* Restore urb->transfer_buffer from the end of the allocated area */ - memcpy(&stored_xfer_buffer, - PTR_ALIGN(urb->transfer_buffer + urb->transfer_buffer_length, - dma_get_cache_alignment()), - sizeof(urb->transfer_buffer)); + temp = container_of(urb->transfer_buffer, + struct dma_aligned_buffer, data); if (usb_urb_dir_in(urb)) { if (usb_pipeisoc(urb->pipe)) @@ -2492,17 +2495,17 @@ static void dwc2_free_dma_aligned_buffer(struct urb *urb) else length = urb->actual_length; - memcpy(stored_xfer_buffer, urb->transfer_buffer, length); + memcpy(temp->old_xfer_buffer, temp->data, length); } - kfree(urb->transfer_buffer); - urb->transfer_buffer = stored_xfer_buffer; + urb->transfer_buffer = temp->old_xfer_buffer; + kfree(temp->kmalloc_ptr); urb->transfer_flags &= ~URB_ALIGNED_TEMP_BUFFER; } static int dwc2_alloc_dma_aligned_buffer(struct urb *urb, gfp_t mem_flags) { - void *kmalloc_ptr; + struct dma_aligned_buffer *temp, *kmalloc_ptr; size_t kmalloc_size; if (urb->num_sgs || urb->sg || @@ -2510,31 +2513,22 @@ static int dwc2_alloc_dma_aligned_buffer(struct urb *urb, gfp_t mem_flags) !((uintptr_t)urb->transfer_buffer & (DWC2_USB_DMA_ALIGN - 1))) return 0; - /* - * Allocate a buffer with enough padding for original transfer_buffer - * pointer. This allocation is guaranteed to be aligned properly for - * DMA - */ + /* Allocate a buffer with enough padding for alignment */ kmalloc_size = urb->transfer_buffer_length + - (dma_get_cache_alignment() - 1) + - sizeof(urb->transfer_buffer); + sizeof(struct dma_aligned_buffer) + DWC2_USB_DMA_ALIGN - 1; kmalloc_ptr = kmalloc(kmalloc_size, mem_flags); if (!kmalloc_ptr) return -ENOMEM; - /* - * Position value of original urb->transfer_buffer pointer to the end - * of allocation for later referencing - */ - memcpy(PTR_ALIGN(kmalloc_ptr + urb->transfer_buffer_length, - dma_get_cache_alignment()), - &urb->transfer_buffer, sizeof(urb->transfer_buffer)); - + /* Position our struct dma_aligned_buffer such that data is aligned */ + temp = PTR_ALIGN(kmalloc_ptr + 1, DWC2_USB_DMA_ALIGN) - 1; + temp->kmalloc_ptr = kmalloc_ptr; + temp->old_xfer_buffer = urb->transfer_buffer; if (usb_urb_dir_out(urb)) - memcpy(kmalloc_ptr, urb->transfer_buffer, + memcpy(temp->data, urb->transfer_buffer, urb->transfer_buffer_length); - urb->transfer_buffer = kmalloc_ptr; + urb->transfer_buffer = temp->data; urb->transfer_flags |= URB_ALIGNED_TEMP_BUFFER; -- 2.17.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH] usb: dwc2: extend treatment for incomplete transfer 2020-02-11 16:15 ` Guenter Roeck @ 2020-02-15 5:36 ` Boris ARZUR 2020-02-19 21:10 ` Guenter Roeck 2020-02-20 21:22 ` Guenter Roeck 0 siblings, 2 replies; 20+ messages in thread From: Boris ARZUR @ 2020-02-15 5:36 UTC (permalink / raw) To: Guenter Roeck Cc: linux-usb, Greg Kroah-Hartman, Minas Harutyunyan, William Wu, Dmitry Torokhov, Douglas Anderson Hi Guenter, >> The first time around I was 0/ changing fonts 1/ trimming the dump message >> in the kernel 2/ filming my screen. That's not practical at all... Mmm, pstore does seem to work on my machine. CHROME_PSTORE is not available to me because I'm not on x86, I just enabled the rest and nothing pops up in /sys/fs/pstore... So I took pictures (OCR did not help): - https://funyu.konbu.org/pQUF2P08etcpVxuKZ0A720%2b0/IMG_20200215_134343.jpg.webp is a stack trace for your earlier patch "min_t", in https://www.spinics.net/lists/linux-usb/msg191019.html ; - https://funyu.konbu.org/pQUF2P08etcpVxuKZ0A720%2b0/IMG_20200215_135816.jpg.webp is a stack trace for your later patch "container_of", in https://www.spinics.net/lists/linux-usb/msg191074.html . Both patches crash (without even loading the machine), but "container_of" is a bit more resilient. Thanks, Boris. Guenter Roeck wrote: >Hi Boris, > >On Tue, Feb 11, 2020 at 02:49:53PM +0900, Boris ARZUR wrote: >> Hello Guenter, >> >> >In the meantime, can you by any chance test the attached patch ? It _might_ >> >fix the problem, but it is a bit of a wild shot. >> I tried your patch, but the machine does not finish booting. >> >> >> I would like to give you a dump, but the screen scrolls fast, and what's >> left when paused is not interesting. How do I get it to dump on disk? >> >> My journalctl doesn't show anything. I have no kmesg.log anywhere. >> The first time around I was 0/ changing fonts 1/ trimming the dump message >> in the kernel 2/ filming my screen. That's not practical at all... >> >> >> I have been looking a bit at things. I believe that part of the issue >> is the need to re-align the buffer we get in the URB. I'm wondering if asking >> for a specific alignment when creating the URB could be doable. >> >> >> As a stop-gap, maybe doing things like in tegra ehci could fix our bug: >> https://github.com/torvalds/linux/blob/master/drivers/usb/host/ehci-tegra.c#L288 >> i.e. having the old pointer before the new buffer instead of at the end of >> it. >> >> Now if something is overwriting the buffer end, that would also be hiding the >> issue... but if the bug is related to lengths that don't match between >> allocation and free, that could work. In this case, that would also be >> hiding the issue :) >> >See below for a patch (untested) doing just that. It may solve your immediate >problem, though it would still suffer from the buffer end overwrite. > >> >> >Unfortunately, I have been unable to reproduce the problem. It is seen only >> >with certain phones and with certain Ethernet adapters, and I was unable >> >to get any of those. I'll keep trying. >> If you want, I can run a kernel with some printk instrumentation or run >> experiments. I'll research a bit on how to get that kernel oops data, that > >Unfortunately I have no real idea what to look out for. The problem may be >related to the phone sending more than one Ethernet packet in a single USB >transfer. See rndis_rx_fixup() for how that is handled in the driver. >I don't know how that would result in the observed problem, though. > >Thanks, >Guenter > >--- >From 8efa9c598f2390dca2e97cbbe41e981caba41ca1 Mon Sep 17 00:00:00 2001 >From: Guenter Roeck <linux@roeck-us.net> >Date: Mon, 10 Feb 2020 14:04:06 -0800 >Subject: [PATCH] usb: dwc2: Simplify DMA alignment code > >The code to align buffers for DMA was first introduced with commit >3bc04e28a030 ("usb: dwc2: host: Get aligned DMA in a more supported way"). >It was updated with commit 56406e017a88 ("usb: dwc2: Fix DMA alignment >to start at allocated boundary") because it did not really align buffers to >DMA boundaries but to DWC2_USB_DMA_ALIGN. This was then optimized in commit >1e111e885238 ("usb: dwc2: Fix inefficient copy of unaligned buffers") >to only copy actual data rather than the whole buffer. Commit 4a4863bf2e79 >("usb: dwc2: Fix DMA cache alignment issues") changed this further to add >a padding at the end of the buffer to ensure that the old data pointer is >not in the same cache line as the buffer. > >This last commit states "Otherwise, the stored_xfer_buffer gets corrupted >for IN URBs on non-cache-coherent systems". However, such corruptions are >still observed. Either case, DMA is not expected to overwrite more memory >than it is supposed to do, suggesting that the commit may have been hiding >a problem rather than fixing it. > >On top of that, DMA alignment is still not guaranteed since it only happens >if the original buffer is not aligned to DWC2_USB_DMA_ALIGN, which is still >a constant of 4 and not associated with DMA alignment. > >Move the old data pointer back to the beginning of the new buffer, >restoring most of the original commit and to simplify the code. Define >DWC2_USB_DMA_ALIGN to dma_get_cache_alignment() to fix the DMA alignment >for real this time. > >Signed-off-by: Guenter Roeck <linux@roeck-us.net> >--- > drivers/usb/dwc2/hcd.c | 50 +++++++++++++++++++----------------------- > 1 file changed, 22 insertions(+), 28 deletions(-) > >diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c >index 2c81b346b464..9e04b3a314eb 100644 >--- a/drivers/usb/dwc2/hcd.c >+++ b/drivers/usb/dwc2/hcd.c >@@ -2470,21 +2470,24 @@ static int dwc2_alloc_split_dma_aligned_buf(struct dwc2_hsotg *hsotg, > return 0; > } > >-#define DWC2_USB_DMA_ALIGN 4 >+#define DWC2_USB_DMA_ALIGN dma_get_cache_alignment() >+ >+struct dma_aligned_buffer { >+ void *kmalloc_ptr; >+ void *old_xfer_buffer; >+ u8 data[0]; >+}; > > static void dwc2_free_dma_aligned_buffer(struct urb *urb) > { >- void *stored_xfer_buffer; >+ struct dma_aligned_buffer *temp; > size_t length; > > if (!(urb->transfer_flags & URB_ALIGNED_TEMP_BUFFER)) > return; > >- /* Restore urb->transfer_buffer from the end of the allocated area */ >- memcpy(&stored_xfer_buffer, >- PTR_ALIGN(urb->transfer_buffer + urb->transfer_buffer_length, >- dma_get_cache_alignment()), >- sizeof(urb->transfer_buffer)); >+ temp = container_of(urb->transfer_buffer, >+ struct dma_aligned_buffer, data); > > if (usb_urb_dir_in(urb)) { > if (usb_pipeisoc(urb->pipe)) >@@ -2492,17 +2495,17 @@ static void dwc2_free_dma_aligned_buffer(struct urb *urb) > else > length = urb->actual_length; > >- memcpy(stored_xfer_buffer, urb->transfer_buffer, length); >+ memcpy(temp->old_xfer_buffer, temp->data, length); > } >- kfree(urb->transfer_buffer); >- urb->transfer_buffer = stored_xfer_buffer; >+ urb->transfer_buffer = temp->old_xfer_buffer; >+ kfree(temp->kmalloc_ptr); > > urb->transfer_flags &= ~URB_ALIGNED_TEMP_BUFFER; > } > > static int dwc2_alloc_dma_aligned_buffer(struct urb *urb, gfp_t mem_flags) > { >- void *kmalloc_ptr; >+ struct dma_aligned_buffer *temp, *kmalloc_ptr; > size_t kmalloc_size; > > if (urb->num_sgs || urb->sg || >@@ -2510,31 +2513,22 @@ static int dwc2_alloc_dma_aligned_buffer(struct urb *urb, gfp_t mem_flags) > !((uintptr_t)urb->transfer_buffer & (DWC2_USB_DMA_ALIGN - 1))) > return 0; > >- /* >- * Allocate a buffer with enough padding for original transfer_buffer >- * pointer. This allocation is guaranteed to be aligned properly for >- * DMA >- */ >+ /* Allocate a buffer with enough padding for alignment */ > kmalloc_size = urb->transfer_buffer_length + >- (dma_get_cache_alignment() - 1) + >- sizeof(urb->transfer_buffer); >+ sizeof(struct dma_aligned_buffer) + DWC2_USB_DMA_ALIGN - 1; > > kmalloc_ptr = kmalloc(kmalloc_size, mem_flags); > if (!kmalloc_ptr) > return -ENOMEM; > >- /* >- * Position value of original urb->transfer_buffer pointer to the end >- * of allocation for later referencing >- */ >- memcpy(PTR_ALIGN(kmalloc_ptr + urb->transfer_buffer_length, >- dma_get_cache_alignment()), >- &urb->transfer_buffer, sizeof(urb->transfer_buffer)); >- >+ /* Position our struct dma_aligned_buffer such that data is aligned */ >+ temp = PTR_ALIGN(kmalloc_ptr + 1, DWC2_USB_DMA_ALIGN) - 1; >+ temp->kmalloc_ptr = kmalloc_ptr; >+ temp->old_xfer_buffer = urb->transfer_buffer; > if (usb_urb_dir_out(urb)) >- memcpy(kmalloc_ptr, urb->transfer_buffer, >+ memcpy(temp->data, urb->transfer_buffer, > urb->transfer_buffer_length); >- urb->transfer_buffer = kmalloc_ptr; >+ urb->transfer_buffer = temp->data; > > urb->transfer_flags |= URB_ALIGNED_TEMP_BUFFER; > >-- >2.17.1 > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] usb: dwc2: extend treatment for incomplete transfer 2020-02-15 5:36 ` Boris ARZUR @ 2020-02-19 21:10 ` Guenter Roeck 2020-02-23 11:00 ` Antti Seppälä 2020-02-23 12:02 ` Boris ARZUR 2020-02-20 21:22 ` Guenter Roeck 1 sibling, 2 replies; 20+ messages in thread From: Guenter Roeck @ 2020-02-19 21:10 UTC (permalink / raw) To: Boris ARZUR Cc: linux-usb, Greg Kroah-Hartman, Minas Harutyunyan, William Wu, Dmitry Torokhov, Douglas Anderson [-- Attachment #1: Type: text/plain, Size: 1082 bytes --] On Sat, Feb 15, 2020 at 02:36:47PM +0900, Boris ARZUR wrote: > Hi Guenter, > > >> The first time around I was 0/ changing fonts 1/ trimming the dump message > >> in the kernel 2/ filming my screen. That's not practical at all... > Mmm, pstore does seem to work on my machine. CHROME_PSTORE is not available > to me because I'm not on x86, I just enabled the rest and nothing pops up > in /sys/fs/pstore... > > So I took pictures (OCR did not help): > - https://funyu.konbu.org/pQUF2P08etcpVxuKZ0A720%2b0/IMG_20200215_134343.jpg.webp > is a stack trace for your earlier patch "min_t", in > https://www.spinics.net/lists/linux-usb/msg191019.html ; > - https://funyu.konbu.org/pQUF2P08etcpVxuKZ0A720%2b0/IMG_20200215_135816.jpg.webp > is a stack trace for your later patch "container_of", in > https://www.spinics.net/lists/linux-usb/msg191074.html . > > Both patches crash (without even loading the machine), but "container_of" is > a bit more resilient. > Yes, those patches didn't address the core problem. Can you test with the attached two patches ? Thanks, Guenter [-- Attachment #2: 0001-usb-dwc2-Simplify-DMA-alignment-code.patch --] [-- Type: text/x-diff, Size: 5066 bytes --] From a1c0551b62b038b495177737828f986961184110 Mon Sep 17 00:00:00 2001 From: Guenter Roeck <linux@roeck-us.net> Date: Mon, 10 Feb 2020 14:04:06 -0800 Subject: [PATCH 1/2] usb: dwc2: Simplify DMA alignment code The code to align buffers for DMA was first introduced with commit 3bc04e28a030 ("usb: dwc2: host: Get aligned DMA in a more supported way"). It was updated with commit 56406e017a88 ("usb: dwc2: Fix DMA alignment to start at allocated boundary") because it did not really align buffers to DMA boundaries but to DWC2_USB_DMA_ALIGN. This was then optimized in commit 1e111e885238 ("usb: dwc2: Fix inefficient copy of unaligned buffers") to only copy actual data rather than the whole buffer. Commit 4a4863bf2e79 ("usb: dwc2: Fix DMA cache alignment issues") changed this further to add a padding at the end of the buffer to ensure that the old data pointer is not in the same cache line as the buffer. This last commit states "Otherwise, the stored_xfer_buffer gets corrupted for IN URBs on non-cache-coherent systems". However, such corruptions are still observed. Either case, DMA is not expected to overwrite more memory than it is supposed to do, suggesting that the commit may have been hiding a problem rather than fixing it. On top of that, DMA alignment is still not guaranteed since it only happens if the original buffer is not aligned to DWC2_USB_DMA_ALIGN, which is still a constant of 4 and not really associated with DMA alignment. Move the old data pointer back to the beginning of the new buffer, restoring most of the original commit and to simplify the code. Define DWC2_USB_DMA_ALIGN to dma_get_cache_alignment() to fix the DMA alignment for real this time. Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- drivers/usb/dwc2/hcd.c | 50 +++++++++++++++++++----------------------- 1 file changed, 22 insertions(+), 28 deletions(-) diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c index b90f858af960..b5841197165a 100644 --- a/drivers/usb/dwc2/hcd.c +++ b/drivers/usb/dwc2/hcd.c @@ -2469,21 +2469,24 @@ static int dwc2_alloc_split_dma_aligned_buf(struct dwc2_hsotg *hsotg, return 0; } -#define DWC2_USB_DMA_ALIGN 4 +#define DWC2_USB_DMA_ALIGN dma_get_cache_alignment() + +struct dma_aligned_buffer { + void *kmalloc_ptr; + void *old_xfer_buffer; + u8 data[0]; +}; static void dwc2_free_dma_aligned_buffer(struct urb *urb) { - void *stored_xfer_buffer; + struct dma_aligned_buffer *temp; size_t length; if (!(urb->transfer_flags & URB_ALIGNED_TEMP_BUFFER)) return; - /* Restore urb->transfer_buffer from the end of the allocated area */ - memcpy(&stored_xfer_buffer, - PTR_ALIGN(urb->transfer_buffer + urb->transfer_buffer_length, - dma_get_cache_alignment()), - sizeof(urb->transfer_buffer)); + temp = container_of(urb->transfer_buffer, + struct dma_aligned_buffer, data); if (usb_urb_dir_in(urb)) { if (usb_pipeisoc(urb->pipe)) @@ -2491,17 +2494,17 @@ static void dwc2_free_dma_aligned_buffer(struct urb *urb) else length = urb->actual_length; - memcpy(stored_xfer_buffer, urb->transfer_buffer, length); + memcpy(temp->old_xfer_buffer, temp->data, length); } - kfree(urb->transfer_buffer); - urb->transfer_buffer = stored_xfer_buffer; + urb->transfer_buffer = temp->old_xfer_buffer; + kfree(temp->kmalloc_ptr); urb->transfer_flags &= ~URB_ALIGNED_TEMP_BUFFER; } static int dwc2_alloc_dma_aligned_buffer(struct urb *urb, gfp_t mem_flags) { - void *kmalloc_ptr; + struct dma_aligned_buffer *temp, *kmalloc_ptr; size_t kmalloc_size; if (urb->num_sgs || urb->sg || @@ -2509,31 +2512,22 @@ static int dwc2_alloc_dma_aligned_buffer(struct urb *urb, gfp_t mem_flags) !((uintptr_t)urb->transfer_buffer & (DWC2_USB_DMA_ALIGN - 1))) return 0; - /* - * Allocate a buffer with enough padding for original transfer_buffer - * pointer. This allocation is guaranteed to be aligned properly for - * DMA - */ + /* Allocate a buffer with enough padding for alignment */ kmalloc_size = urb->transfer_buffer_length + - (dma_get_cache_alignment() - 1) + - sizeof(urb->transfer_buffer); + sizeof(struct dma_aligned_buffer) + DWC2_USB_DMA_ALIGN - 1; kmalloc_ptr = kmalloc(kmalloc_size, mem_flags); if (!kmalloc_ptr) return -ENOMEM; - /* - * Position value of original urb->transfer_buffer pointer to the end - * of allocation for later referencing - */ - memcpy(PTR_ALIGN(kmalloc_ptr + urb->transfer_buffer_length, - dma_get_cache_alignment()), - &urb->transfer_buffer, sizeof(urb->transfer_buffer)); - + /* Position our struct dma_aligned_buffer such that data is aligned */ + temp = PTR_ALIGN(kmalloc_ptr + 1, DWC2_USB_DMA_ALIGN) - 1; + temp->kmalloc_ptr = kmalloc_ptr; + temp->old_xfer_buffer = urb->transfer_buffer; if (usb_urb_dir_out(urb)) - memcpy(kmalloc_ptr, urb->transfer_buffer, + memcpy(temp->data, urb->transfer_buffer, urb->transfer_buffer_length); - urb->transfer_buffer = kmalloc_ptr; + urb->transfer_buffer = temp->data; urb->transfer_flags |= URB_ALIGNED_TEMP_BUFFER; -- 2.17.1 [-- Attachment #3: 0002-usb-dwc2-Allocate-input-buffers-as-multiples-of-wMax.patch --] [-- Type: text/x-diff, Size: 5633 bytes --] From 9df13854b3717f8c16a2012dec614f737bb8c15d Mon Sep 17 00:00:00 2001 From: Guenter Roeck <linux@roeck-us.net> Date: Mon, 10 Feb 2020 13:11:00 -0800 Subject: [PATCH 2/2] usb: dwc2: Allocate input buffers as multiples of wMaxPacketSize MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The following messages are seen on Veyron Chromebooks running v4.19 or later kernels. dwc2 ff580000.usb: dwc2_update_urb_state(): trimming xfer length dwc2 ff580000.usb: dwc2_hc_chhltd_intr_dma: Channel 7 - ChHltd set, but reason is unknown dwc2 ff580000.usb: hcint 0x00000002, intsts 0x04600021 This is typically followed by a crash. Unable to handle kernel paging request at virtual address 29f9d9fc pgd = 4797dac9 [29f9d9fc] *pgd=80000000004003, *pmd=00000000 Internal error: Oops: a06 [#1] PREEMPT SMP ARM Modules linked in: ip6t_REJECT rfcomm i2c_dev uinput hci_uart btbcm ... CPU: 0 PID: 0 Comm: swapper/0 Tainted: G W 4.19.87-07825-g4ab3515f6e4d #1 Hardware name: Rockchip (Device Tree) PC is at memcpy+0x50/0x330 LR is at 0xdd9ac94 ... [<c0a89f50>] (memcpy) from [<c0783b94>] (dwc2_free_dma_aligned_buffer+0x5c/0x7c) [<c0783b94>] (dwc2_free_dma_aligned_buffer) from [<c0765dcc>] (__usb_hcd_giveback_urb+0x78/0x130) [<c0765dcc>] (__usb_hcd_giveback_urb) from [<c07678fc>] (usb_giveback_urb_bh+0xa0/0xe4) [<c07678fc>] (usb_giveback_urb_bh) from [<c023a164>] (tasklet_action_common+0xc0/0xdc) [<c023a164>] (tasklet_action_common) from [<c02021f0>] (__do_softirq+0x1b8/0x434) [<c02021f0>] (__do_softirq) from [<c0239a14>] (irq_exit+0xdc/0xe0) [<c0239a14>] (irq_exit) from [<c029f260>] (__handle_domain_irq+0x94/0xd0) [<c029f260>] (__handle_domain_irq) from [<c05da780>] (gic_handle_irq+0x74/0xb0) [<c05da780>] (gic_handle_irq) from [<c02019f8>] (__irq_svc+0x58/0x8c) The crash suggests that the memory after the end of a temporary DMA-aligned buffer is overwritten. The problem is typically only seen in kernels which include commit 56406e017a88 ("usb: dwc2: Fix DMA alignment to start at allocated boundary"), presumably because that commit moves the pointer to the old buffer to the end of the newly allocated buffer, where it is more likely to be overwritten. Code analysis shows that the transfer size programmed into the chip for input transfers is a multiple of an endpoint's maximum packet size (wMaxPacketSize). In the observed situation, the transfer size and thus the size of the input buffer is 1522 bytes. With a maximum packet size of 64 bytes, the chip is programmed to receive up to 1536 bytes of data. This overwrites the end of the buffer. To work around the problem, always allocate a multiple of wMaxPacketSize bytes for receive buffers. Do this even for DMA-aligned buffers if the receive buffer size is not a multiple of wMaxPacketSize. At the same time, do not update chan->xfer_len if the transfer size is 0. Reported-by: Boris ARZUR <boris@konbu.org> Cc: Boris ARZUR <boris@konbu.org> Cc: Jonathan Bell <jonathan@raspberrypi.org> Cc: Antti Seppälä <a.seppala@gmail.com> Fixes: 56406e017a88 ("usb: dwc2: Fix DMA alignment to start at allocated boundary") Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- drivers/usb/dwc2/hcd.c | 37 +++++++++++++++++++++++++++---------- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c index b5841197165a..f27dc11e409c 100644 --- a/drivers/usb/dwc2/hcd.c +++ b/drivers/usb/dwc2/hcd.c @@ -1313,18 +1313,20 @@ static void dwc2_hc_start_transfer(struct dwc2_hsotg *hsotg, if (num_packets > max_hc_pkt_count) { num_packets = max_hc_pkt_count; chan->xfer_len = num_packets * chan->max_packet; + } else if (chan->ep_is_in) { + /* + * Always program an integral # of max packets for IN + * transfers. + * Note: This assumes that the input buffer is + * aligned accordingly. + */ + chan->xfer_len = num_packets * chan->max_packet; } } else { /* Need 1 packet for transfer length of 0 */ num_packets = 1; } - if (chan->ep_is_in) - /* - * Always program an integral # of max packets for IN - * transfers - */ - chan->xfer_len = num_packets * chan->max_packet; if (chan->ep_type == USB_ENDPOINT_XFER_INT || chan->ep_type == USB_ENDPOINT_XFER_ISOC) @@ -2505,16 +2507,31 @@ static void dwc2_free_dma_aligned_buffer(struct urb *urb) static int dwc2_alloc_dma_aligned_buffer(struct urb *urb, gfp_t mem_flags) { struct dma_aligned_buffer *temp, *kmalloc_ptr; + struct usb_host_endpoint *ep = urb->ep; + int maxp = usb_endpoint_maxp(&ep->desc); size_t kmalloc_size; - if (urb->num_sgs || urb->sg || - urb->transfer_buffer_length == 0 || + if (urb->num_sgs || urb->sg || urb->transfer_buffer_length == 0) + return 0; + + /* + * Input transfer buffer size must be a multiple of the endpoint's + * maximum packet size to match the transfer limit programmed into + * the chip. + * See calculation of chan->xfer_len in dwc2_hc_start_transfer(). + */ + if (usb_urb_dir_out(urb)) + kmalloc_size = urb->transfer_buffer_length; + else + kmalloc_size = roundup(urb->transfer_buffer_length, maxp); + + if (kmalloc_size == urb->transfer_buffer_length && !((uintptr_t)urb->transfer_buffer & (DWC2_USB_DMA_ALIGN - 1))) return 0; /* Allocate a buffer with enough padding for alignment */ - kmalloc_size = urb->transfer_buffer_length + - sizeof(struct dma_aligned_buffer) + DWC2_USB_DMA_ALIGN - 1; + kmalloc_size += sizeof(struct dma_aligned_buffer) + + DWC2_USB_DMA_ALIGN - 1; kmalloc_ptr = kmalloc(kmalloc_size, mem_flags); if (!kmalloc_ptr) -- 2.17.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH] usb: dwc2: extend treatment for incomplete transfer 2020-02-19 21:10 ` Guenter Roeck @ 2020-02-23 11:00 ` Antti Seppälä 2020-02-23 12:10 ` Boris ARZUR 2020-02-23 13:45 ` Guenter Roeck 2020-02-23 12:02 ` Boris ARZUR 1 sibling, 2 replies; 20+ messages in thread From: Antti Seppälä @ 2020-02-23 11:00 UTC (permalink / raw) To: Guenter Roeck Cc: Boris ARZUR, linux-usb, Greg Kroah-Hartman, Minas Harutyunyan, William Wu, Dmitry Torokhov, Douglas Anderson [-- Attachment #1: Type: text/plain, Size: 1397 bytes --] Hi Guenter, On Wed, 19 Feb 2020 at 23:11, Guenter Roeck <linux@roeck-us.net> wrote: > > Yes, those patches didn't address the core problem. Can you test with the > attached two patches ? > > Thanks, > Guenter I took a look at your patch (usb: dwc2: Simplify DMA alignment code) and I don't believe it is correct. The patch re-introduces the dma_aligned_buffer struct and takes some care to align the beginning of the struct to dma cache lines. However we should be aligning the data[0] pointer inside the struct instead. With the code in the patch data[0] gets pushed to be at an offset from the alignment by kmalloc_ptr and old_xfer_buffer pointers. In other words data[0] is now not aligned to dma cache boundaries. Reviewing the code got me thinking that what if we stopped playing with the alignment hacks altogether and hit the issue with a heavier hammer instead? Attached you can find a new patch that introduces a list to keep track of the allocations. The code then looks up the entry from the list when it is time to restore the original pointer. This way the allocations for the aligned dma area and the original pointer are separate and no corruptions should occur. Thoughts, comments? I should note that the patch has received only light testing and not very thorough thinking. I can prepare a proper patch to be sent inline if the idea seems worth exploring further. -- Antti [-- Attachment #2: 0001-usb-dwc2-Use-list-to-keep-track-of-DMA-aligned-buffe.patch --] [-- Type: application/octet-stream, Size: 8201 bytes --] From b7847e790e87ee10b0002d4c0cabcf830bf54812 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Antti=20Sepp=C3=A4l=C3=A4?= <a.seppala@gmail.com> Date: Sun, 23 Feb 2020 10:33:59 +0200 Subject: [PATCH] usb: dwc2: Use list to keep track of DMA aligned buffer allocations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Using same memory area allocation for original pointer values and direct memory accesses is prone to memory corruption issues due to cache coherency reasons. Separate the allocations and use a list to keep track of the DMA aligned buffers. Signed-off-by: Antti Seppälä <a.seppala@gmail.com> --- Notes: I'm currently not sure if struct dwc2_hsotg is the right place for storing the DMA allocation list. Perhaps there are better structs in dwc2? Is list even the right data structure for the job? drivers/usb/dwc2/core.h | 3 ++ drivers/usb/dwc2/hcd.c | 116 +++++++++++++++++++++++++++++------------------- 2 files changed, 74 insertions(+), 45 deletions(-) diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h index 968e03b89d04..a7a7820a89ad 100644 --- a/drivers/usb/dwc2/core.h +++ b/drivers/usb/dwc2/core.h @@ -948,6 +948,8 @@ struct dwc2_hregs_backup { * the next frame. Otherwise, the item moves to * periodic_sched_inactive. * @split_order: List keeping track of channels doing splits, in order. + * @dma_aligned_buffers: List of urb->transfer_buffers that were specifically + * DMA-aligned and need to be later referenced and freed. * @periodic_usecs: Total bandwidth claimed so far for periodic transfers. * This value is in microseconds per (micro)frame. The * assumption is that all periodic transfers may occur in @@ -1127,6 +1129,7 @@ struct dwc2_hsotg { struct list_head periodic_sched_assigned; struct list_head periodic_sched_queued; struct list_head split_order; + struct list_head dma_aligned_buffers; u16 periodic_usecs; unsigned long hs_periodic_bitmap[ DIV_ROUND_UP(DWC2_HS_SCHEDULE_US, BITS_PER_LONG)]; diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c index b90f858af960..c375611de8c5 100644 --- a/drivers/usb/dwc2/hcd.c +++ b/drivers/usb/dwc2/hcd.c @@ -2469,21 +2469,36 @@ static int dwc2_alloc_split_dma_aligned_buf(struct dwc2_hsotg *hsotg, return 0; } -#define DWC2_USB_DMA_ALIGN 4 +#define DWC2_USB_DMA_ALIGN dma_get_cache_alignment() -static void dwc2_free_dma_aligned_buffer(struct urb *urb) +struct dma_aligned_buffer { + void *kmalloc_ptr; + void *old_xfer_buffer; + struct list_head dma_list; +}; + +static void dwc2_free_dma_aligned_buffer(struct dwc2_hsotg *hsotg, + struct urb *urb) { - void *stored_xfer_buffer; + struct dma_aligned_buffer *temp = NULL; size_t length; + unsigned long flags; if (!(urb->transfer_flags & URB_ALIGNED_TEMP_BUFFER)) return; - /* Restore urb->transfer_buffer from the end of the allocated area */ - memcpy(&stored_xfer_buffer, - PTR_ALIGN(urb->transfer_buffer + urb->transfer_buffer_length, - dma_get_cache_alignment()), - sizeof(urb->transfer_buffer)); + spin_lock_irqsave(&hsotg->lock, flags); + list_for_each_entry(temp, &hsotg->dma_aligned_buffers, dma_list) { + if (temp->kmalloc_ptr == urb->transfer_buffer) + break; + } + if (!temp) { + spin_unlock_irqrestore(&hsotg->lock, flags); + WARN_ONCE(!temp, "dma aligned temporary buffer lost"); + return; + } + list_del(&temp->dma_list); + spin_unlock_irqrestore(&hsotg->lock, flags); if (usb_urb_dir_in(urb)) { if (usb_pipeisoc(urb->pipe)) @@ -2491,18 +2506,21 @@ static void dwc2_free_dma_aligned_buffer(struct urb *urb) else length = urb->actual_length; - memcpy(stored_xfer_buffer, urb->transfer_buffer, length); + memcpy(temp->old_xfer_buffer, urb->transfer_buffer, length); } - kfree(urb->transfer_buffer); - urb->transfer_buffer = stored_xfer_buffer; + urb->transfer_buffer = temp->old_xfer_buffer; + + kfree(temp->kmalloc_ptr); + kfree(temp); urb->transfer_flags &= ~URB_ALIGNED_TEMP_BUFFER; } -static int dwc2_alloc_dma_aligned_buffer(struct urb *urb, gfp_t mem_flags) +static int dwc2_alloc_dma_aligned_buffer(struct dwc2_hsotg *hsotg, + struct urb *urb, gfp_t mem_flags) { - void *kmalloc_ptr; - size_t kmalloc_size; + struct dma_aligned_buffer *temp; + unsigned long flags; if (urb->num_sgs || urb->sg || urb->transfer_buffer_length == 0 || @@ -2510,60 +2528,80 @@ static int dwc2_alloc_dma_aligned_buffer(struct urb *urb, gfp_t mem_flags) return 0; /* - * Allocate a buffer with enough padding for original transfer_buffer + * Allocate a list entry and a new buffer for original transfer_buffer * pointer. This allocation is guaranteed to be aligned properly for * DMA + * Drop potential DMA flag from list entry allocation because the list + * itself is not accessed via DMA */ - kmalloc_size = urb->transfer_buffer_length + - (dma_get_cache_alignment() - 1) + - sizeof(urb->transfer_buffer); - - kmalloc_ptr = kmalloc(kmalloc_size, mem_flags); - if (!kmalloc_ptr) + temp = kmalloc(sizeof(struct dma_aligned_buffer), mem_flags & ~GFP_DMA); + if (!temp) + return -ENOMEM; + temp->old_xfer_buffer = urb->transfer_buffer; + temp->kmalloc_ptr = kmalloc(urb->transfer_buffer_length, mem_flags); + if (!temp->kmalloc_ptr) { + kfree(temp); return -ENOMEM; + } /* - * Position value of original urb->transfer_buffer pointer to the end - * of allocation for later referencing + * Use our newly allocated and aligned buffer and store the old + * transfer_buffer into the list */ - memcpy(PTR_ALIGN(kmalloc_ptr + urb->transfer_buffer_length, - dma_get_cache_alignment()), - &urb->transfer_buffer, sizeof(urb->transfer_buffer)); - if (usb_urb_dir_out(urb)) - memcpy(kmalloc_ptr, urb->transfer_buffer, + memcpy(temp->kmalloc_ptr, urb->transfer_buffer, urb->transfer_buffer_length); - urb->transfer_buffer = kmalloc_ptr; + urb->transfer_buffer = temp->kmalloc_ptr; + + spin_lock_irqsave(&hsotg->lock, flags); + list_add_tail(&temp->dma_list, &hsotg->dma_aligned_buffers); + spin_unlock_irqrestore(&hsotg->lock, flags); urb->transfer_flags |= URB_ALIGNED_TEMP_BUFFER; return 0; } +struct wrapper_priv_data { + struct dwc2_hsotg *hsotg; +}; + +/* Gets the dwc2_hsotg from a usb_hcd */ +static struct dwc2_hsotg *dwc2_hcd_to_hsotg(struct usb_hcd *hcd) +{ + struct wrapper_priv_data *p; + + p = (struct wrapper_priv_data *)&hcd->hcd_priv; + return p->hsotg; +} + static int dwc2_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flags) { int ret; + struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd); /* We assume setup_dma is always aligned; warn if not */ WARN_ON_ONCE(urb->setup_dma && (urb->setup_dma & (DWC2_USB_DMA_ALIGN - 1))); - ret = dwc2_alloc_dma_aligned_buffer(urb, mem_flags); + ret = dwc2_alloc_dma_aligned_buffer(hsotg, urb, mem_flags); if (ret) return ret; ret = usb_hcd_map_urb_for_dma(hcd, urb, mem_flags); if (ret) - dwc2_free_dma_aligned_buffer(urb); + dwc2_free_dma_aligned_buffer(hsotg, urb); return ret; } static void dwc2_unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb) { + struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd); + usb_hcd_unmap_urb_for_dma(hcd, urb); - dwc2_free_dma_aligned_buffer(urb); + dwc2_free_dma_aligned_buffer(hsotg, urb); } /** @@ -3956,19 +3994,6 @@ void dwc2_hcd_dump_state(struct dwc2_hsotg *hsotg) #endif } -struct wrapper_priv_data { - struct dwc2_hsotg *hsotg; -}; - -/* Gets the dwc2_hsotg from a usb_hcd */ -static struct dwc2_hsotg *dwc2_hcd_to_hsotg(struct usb_hcd *hcd) -{ - struct wrapper_priv_data *p; - - p = (struct wrapper_priv_data *)&hcd->hcd_priv; - return p->hsotg; -} - /** * dwc2_host_get_tt_info() - Get the dwc2_tt associated with context * @@ -5112,6 +5137,7 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg) INIT_LIST_HEAD(&hsotg->periodic_sched_queued); INIT_LIST_HEAD(&hsotg->split_order); + INIT_LIST_HEAD(&hsotg->dma_aligned_buffers); /* * Create a host channel descriptor for each host channel implemented -- 2.13.6 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH] usb: dwc2: extend treatment for incomplete transfer 2020-02-23 11:00 ` Antti Seppälä @ 2020-02-23 12:10 ` Boris ARZUR 2020-02-23 13:45 ` Guenter Roeck 1 sibling, 0 replies; 20+ messages in thread From: Boris ARZUR @ 2020-02-23 12:10 UTC (permalink / raw) To: Antti Seppälä Cc: Guenter Roeck, linux-usb, Greg Kroah-Hartman, Minas Harutyunyan, William Wu, Dmitry Torokhov, Douglas Anderson Hi Antti, >we should be aligning the data[0] pointer inside the struct instead. I believe you are correct. Now, I checked to see at runtime if temp->data was aligned and it was. I cannot tell you why :) That code is copy-paste from the tegra-ehci driver. >with the alignment hacks altogether and hit the issue with a heavier I feel bad about the alignment hacks as well, and would like the original allocation from the URB thing to be aligned... no additional kmalloc, no memcpy. Is there a reason why we shouldn't try to fix that? >pointer are separate and no corruptions should occur. The corruptions themselves are bad, and should be cured. Thanks, Boris. Antti Seppälä wrote: >Hi Guenter, > >On Wed, 19 Feb 2020 at 23:11, Guenter Roeck <linux@roeck-us.net> wrote: >> >> Yes, those patches didn't address the core problem. Can you test with the >> attached two patches ? >> >> Thanks, >> Guenter > >I took a look at your patch (usb: dwc2: Simplify DMA alignment code) >and I don't believe it is correct. > >The patch re-introduces the dma_aligned_buffer struct and takes some >care to align the beginning of the struct to dma cache lines. However >we should be aligning the data[0] pointer inside the struct instead. >With the code in the patch data[0] gets pushed to be at an offset from >the alignment by kmalloc_ptr and old_xfer_buffer pointers. In other >words data[0] is now not aligned to dma cache boundaries. > >Reviewing the code got me thinking that what if we stopped playing >with the alignment hacks altogether and hit the issue with a heavier >hammer instead? Attached you can find a new patch that introduces a >list to keep track of the allocations. The code then looks up the >entry from the list when it is time to restore the original pointer. >This way the allocations for the aligned dma area and the original >pointer are separate and no corruptions should occur. > >Thoughts, comments? I should note that the patch has received only >light testing and not very thorough thinking. I can prepare a proper >patch to be sent inline if the idea seems worth exploring further. > >-- >Antti ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] usb: dwc2: extend treatment for incomplete transfer 2020-02-23 11:00 ` Antti Seppälä 2020-02-23 12:10 ` Boris ARZUR @ 2020-02-23 13:45 ` Guenter Roeck 2020-02-23 18:20 ` Antti Seppälä 1 sibling, 1 reply; 20+ messages in thread From: Guenter Roeck @ 2020-02-23 13:45 UTC (permalink / raw) To: Antti Seppälä Cc: Boris ARZUR, linux-usb, Greg Kroah-Hartman, Minas Harutyunyan, William Wu, Dmitry Torokhov, Douglas Anderson On 2/23/20 3:00 AM, Antti Seppälä wrote: > Hi Guenter, > > On Wed, 19 Feb 2020 at 23:11, Guenter Roeck <linux@roeck-us.net> wrote: >> >> Yes, those patches didn't address the core problem. Can you test with the >> attached two patches ? >> >> Thanks, >> Guenter > > I took a look at your patch (usb: dwc2: Simplify DMA alignment code) > and I don't believe it is correct. > > The patch re-introduces the dma_aligned_buffer struct and takes some > care to align the beginning of the struct to dma cache lines. However > we should be aligning the data[0] pointer inside the struct instead. > With the code in the patch data[0] gets pushed to be at an offset from > the alignment by kmalloc_ptr and old_xfer_buffer pointers. In other > words data[0] is now not aligned to dma cache boundaries. > I thought so too initially. However, temp = PTR_ALIGN(kmalloc_ptr + 1, TEGRA_USB_DMA_ALIGN) - 1; aligns the structure pointer such that its _end_ is DMA-aligned, which is ->data[0]. Just to be sure, I'll add some debug code warning me if data[0] is not DMA aligned. > Reviewing the code got me thinking that what if we stopped playing > with the alignment hacks altogether and hit the issue with a heavier > hammer instead? Attached you can find a new patch that introduces a > list to keep track of the allocations. The code then looks up the > entry from the list when it is time to restore the original pointer. > This way the allocations for the aligned dma area and the original > pointer are separate and no corruptions should occur. > > Thoughts, comments? I should note that the patch has received only > light testing and not very thorough thinking. I can prepare a proper > patch to be sent inline if the idea seems worth exploring further. > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] usb: dwc2: extend treatment for incomplete transfer 2020-02-23 13:45 ` Guenter Roeck @ 2020-02-23 18:20 ` Antti Seppälä 2020-02-23 18:47 ` Guenter Roeck 0 siblings, 1 reply; 20+ messages in thread From: Antti Seppälä @ 2020-02-23 18:20 UTC (permalink / raw) To: Guenter Roeck Cc: Boris ARZUR, linux-usb, Greg Kroah-Hartman, Minas Harutyunyan, William Wu, Dmitry Torokhov, Douglas Anderson On Sun, 23 Feb 2020 at 15:45, Guenter Roeck <linux@roeck-us.net> wrote: > > On 2/23/20 3:00 AM, Antti Seppälä wrote: > > Hi Guenter, > > > > On Wed, 19 Feb 2020 at 23:11, Guenter Roeck <linux@roeck-us.net> wrote: > >> > >> Yes, those patches didn't address the core problem. Can you test with the > >> attached two patches ? > >> > >> Thanks, > >> Guenter > > > > I took a look at your patch (usb: dwc2: Simplify DMA alignment code) > > and I don't believe it is correct. > > > > The patch re-introduces the dma_aligned_buffer struct and takes some > > care to align the beginning of the struct to dma cache lines. However > > we should be aligning the data[0] pointer inside the struct instead. > > With the code in the patch data[0] gets pushed to be at an offset from > > the alignment by kmalloc_ptr and old_xfer_buffer pointers. In other > > words data[0] is now not aligned to dma cache boundaries. > > > > I thought so too initially. However, > > temp = PTR_ALIGN(kmalloc_ptr + 1, TEGRA_USB_DMA_ALIGN) - 1; > > aligns the structure pointer such that its _end_ is DMA-aligned, > which is ->data[0]. > Hmm, looks like you're right. I somehow missed the - 1 at the end. Sorry for the noise I guess. Would be nice to know what makes the previous code prone to pointer corruption issues though. With the added padding that pointer should also be on another dma cache line. -- Antti ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] usb: dwc2: extend treatment for incomplete transfer 2020-02-23 18:20 ` Antti Seppälä @ 2020-02-23 18:47 ` Guenter Roeck 0 siblings, 0 replies; 20+ messages in thread From: Guenter Roeck @ 2020-02-23 18:47 UTC (permalink / raw) To: Antti Seppälä Cc: Boris ARZUR, linux-usb, Greg Kroah-Hartman, Minas Harutyunyan, William Wu, Dmitry Torokhov, Douglas Anderson On 2/23/20 10:20 AM, Antti Seppälä wrote: > On Sun, 23 Feb 2020 at 15:45, Guenter Roeck <linux@roeck-us.net> wrote: >> >> On 2/23/20 3:00 AM, Antti Seppälä wrote: >>> Hi Guenter, >>> >>> On Wed, 19 Feb 2020 at 23:11, Guenter Roeck <linux@roeck-us.net> wrote: >>>> >>>> Yes, those patches didn't address the core problem. Can you test with the >>>> attached two patches ? >>>> >>>> Thanks, >>>> Guenter >>> >>> I took a look at your patch (usb: dwc2: Simplify DMA alignment code) >>> and I don't believe it is correct. >>> >>> The patch re-introduces the dma_aligned_buffer struct and takes some >>> care to align the beginning of the struct to dma cache lines. However >>> we should be aligning the data[0] pointer inside the struct instead. >>> With the code in the patch data[0] gets pushed to be at an offset from >>> the alignment by kmalloc_ptr and old_xfer_buffer pointers. In other >>> words data[0] is now not aligned to dma cache boundaries. >>> >> >> I thought so too initially. However, >> >> temp = PTR_ALIGN(kmalloc_ptr + 1, TEGRA_USB_DMA_ALIGN) - 1; >> >> aligns the structure pointer such that its _end_ is DMA-aligned, >> which is ->data[0]. >> > > Hmm, looks like you're right. I somehow missed the - 1 at the end. > Sorry for the noise I guess. > No worries. It took me a while to understand hat code, and I initially also thought it was wrong, so you are in good company. > Would be nice to know what makes the previous code prone to pointer > corruption issues though. With the added padding that pointer should > also be on another dma cache line. > Padding to DMA cache line size doesn't fix the real problem. The dwc2 IP expects input buffer size to be a multiple of wMaxPacketSize. dwc2_hc_start_transfer() has the following code(where wMaxPacketSize == chan->max_packet). if (chan->xfer_len > 0) { num_packets = (chan->xfer_len + chan->max_packet - 1) / chan->max_packet; ... } ... if (chan->ep_is_in) chan->xfer_len = num_packets * chan->max_packet; If, for example, wMaxPacketSize is 64, and the original xfer_len is, say, 1522 (as observed in our case), xfer_len is updated to 1536, and the chip is programmed to receive up to 1536 bytes. In most cases, padding the buffer to the DMA cache line size (64 in our case) catches this, but not if xfer_len is much lower than wMaxPacketSize. My code tries to address that situation, but as Boris noticed there is still something wrong with my fix. Guenter ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] usb: dwc2: extend treatment for incomplete transfer 2020-02-19 21:10 ` Guenter Roeck 2020-02-23 11:00 ` Antti Seppälä @ 2020-02-23 12:02 ` Boris ARZUR 2020-02-23 13:53 ` Guenter Roeck 2020-02-25 0:18 ` Guenter Roeck 1 sibling, 2 replies; 20+ messages in thread From: Boris ARZUR @ 2020-02-23 12:02 UTC (permalink / raw) To: Guenter Roeck Cc: linux-usb, Greg Kroah-Hartman, Minas Harutyunyan, William Wu, Dmitry Torokhov, Douglas Anderson, a.seppala [-- Attachment #1: Type: text/plain, Size: 12945 bytes --] Hi Guenter, I tried your series of patch. rndis_host tethering & loading the machine seems to work fine. No more crashing. That being said, I now have an issue with regular USB keys (I tried a few): usb 3-1: reset high-speed USB device number 2 using dwc2 I was able to reproduce this issue with the unpatched kernel, by disabling the early return in dwc2_alloc_dma_aligned_buffer(), see attached. There are times were re-allocation fails, either with your patch or with the (almost-)original code. In particular it seems that there is a packet of lenght 13, usb_urb_dir_in() == true, usb_pipetype(urb->pipe) == PIPE_BULK, that comes in every 2s or so, that does not reallocate properly. In the original code, it's not a problem thanks to the early return, but your code wants 512B (maxp) and forces reallocation... Thanks, Boris. Guenter Roeck wrote: >On Sat, Feb 15, 2020 at 02:36:47PM +0900, Boris ARZUR wrote: >> Hi Guenter, >> >> >> The first time around I was 0/ changing fonts 1/ trimming the dump message >> >> in the kernel 2/ filming my screen. That's not practical at all... >> Mmm, pstore does seem to work on my machine. CHROME_PSTORE is not available >> to me because I'm not on x86, I just enabled the rest and nothing pops up >> in /sys/fs/pstore... >> >> So I took pictures (OCR did not help): >> - https://funyu.konbu.org/pQUF2P08etcpVxuKZ0A720%2b0/IMG_20200215_134343.jpg.webp >> is a stack trace for your earlier patch "min_t", in >> https://www.spinics.net/lists/linux-usb/msg191019.html ; >> - https://funyu.konbu.org/pQUF2P08etcpVxuKZ0A720%2b0/IMG_20200215_135816.jpg.webp >> is a stack trace for your later patch "container_of", in >> https://www.spinics.net/lists/linux-usb/msg191074.html . >> >> Both patches crash (without even loading the machine), but "container_of" is >> a bit more resilient. >> > >Yes, those patches didn't address the core problem. Can you test with the >attached two patches ? > >Thanks, >Guenter >From a1c0551b62b038b495177737828f986961184110 Mon Sep 17 00:00:00 2001 >From: Guenter Roeck <linux@roeck-us.net> >Date: Mon, 10 Feb 2020 14:04:06 -0800 >Subject: [PATCH 1/2] usb: dwc2: Simplify DMA alignment code > >The code to align buffers for DMA was first introduced with commit >3bc04e28a030 ("usb: dwc2: host: Get aligned DMA in a more supported way"). >It was updated with commit 56406e017a88 ("usb: dwc2: Fix DMA alignment >to start at allocated boundary") because it did not really align buffers to >DMA boundaries but to DWC2_USB_DMA_ALIGN. This was then optimized in commit >1e111e885238 ("usb: dwc2: Fix inefficient copy of unaligned buffers") >to only copy actual data rather than the whole buffer. Commit 4a4863bf2e79 >("usb: dwc2: Fix DMA cache alignment issues") changed this further to add >a padding at the end of the buffer to ensure that the old data pointer is >not in the same cache line as the buffer. > >This last commit states "Otherwise, the stored_xfer_buffer gets corrupted >for IN URBs on non-cache-coherent systems". However, such corruptions are >still observed. Either case, DMA is not expected to overwrite more memory >than it is supposed to do, suggesting that the commit may have been hiding >a problem rather than fixing it. > >On top of that, DMA alignment is still not guaranteed since it only happens >if the original buffer is not aligned to DWC2_USB_DMA_ALIGN, which is still >a constant of 4 and not really associated with DMA alignment. > >Move the old data pointer back to the beginning of the new buffer, >restoring most of the original commit and to simplify the code. Define >DWC2_USB_DMA_ALIGN to dma_get_cache_alignment() to fix the DMA alignment >for real this time. > >Signed-off-by: Guenter Roeck <linux@roeck-us.net> >--- > drivers/usb/dwc2/hcd.c | 50 +++++++++++++++++++----------------------- > 1 file changed, 22 insertions(+), 28 deletions(-) > >diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c >index b90f858af960..b5841197165a 100644 >--- a/drivers/usb/dwc2/hcd.c >+++ b/drivers/usb/dwc2/hcd.c >@@ -2469,21 +2469,24 @@ static int dwc2_alloc_split_dma_aligned_buf(struct dwc2_hsotg *hsotg, > return 0; > } > >-#define DWC2_USB_DMA_ALIGN 4 >+#define DWC2_USB_DMA_ALIGN dma_get_cache_alignment() >+ >+struct dma_aligned_buffer { >+ void *kmalloc_ptr; >+ void *old_xfer_buffer; >+ u8 data[0]; >+}; > > static void dwc2_free_dma_aligned_buffer(struct urb *urb) > { >- void *stored_xfer_buffer; >+ struct dma_aligned_buffer *temp; > size_t length; > > if (!(urb->transfer_flags & URB_ALIGNED_TEMP_BUFFER)) > return; > >- /* Restore urb->transfer_buffer from the end of the allocated area */ >- memcpy(&stored_xfer_buffer, >- PTR_ALIGN(urb->transfer_buffer + urb->transfer_buffer_length, >- dma_get_cache_alignment()), >- sizeof(urb->transfer_buffer)); >+ temp = container_of(urb->transfer_buffer, >+ struct dma_aligned_buffer, data); > > if (usb_urb_dir_in(urb)) { > if (usb_pipeisoc(urb->pipe)) >@@ -2491,17 +2494,17 @@ static void dwc2_free_dma_aligned_buffer(struct urb *urb) > else > length = urb->actual_length; > >- memcpy(stored_xfer_buffer, urb->transfer_buffer, length); >+ memcpy(temp->old_xfer_buffer, temp->data, length); > } >- kfree(urb->transfer_buffer); >- urb->transfer_buffer = stored_xfer_buffer; >+ urb->transfer_buffer = temp->old_xfer_buffer; >+ kfree(temp->kmalloc_ptr); > > urb->transfer_flags &= ~URB_ALIGNED_TEMP_BUFFER; > } > > static int dwc2_alloc_dma_aligned_buffer(struct urb *urb, gfp_t mem_flags) > { >- void *kmalloc_ptr; >+ struct dma_aligned_buffer *temp, *kmalloc_ptr; > size_t kmalloc_size; > > if (urb->num_sgs || urb->sg || >@@ -2509,31 +2512,22 @@ static int dwc2_alloc_dma_aligned_buffer(struct urb *urb, gfp_t mem_flags) > !((uintptr_t)urb->transfer_buffer & (DWC2_USB_DMA_ALIGN - 1))) > return 0; > >- /* >- * Allocate a buffer with enough padding for original transfer_buffer >- * pointer. This allocation is guaranteed to be aligned properly for >- * DMA >- */ >+ /* Allocate a buffer with enough padding for alignment */ > kmalloc_size = urb->transfer_buffer_length + >- (dma_get_cache_alignment() - 1) + >- sizeof(urb->transfer_buffer); >+ sizeof(struct dma_aligned_buffer) + DWC2_USB_DMA_ALIGN - 1; > > kmalloc_ptr = kmalloc(kmalloc_size, mem_flags); > if (!kmalloc_ptr) > return -ENOMEM; > >- /* >- * Position value of original urb->transfer_buffer pointer to the end >- * of allocation for later referencing >- */ >- memcpy(PTR_ALIGN(kmalloc_ptr + urb->transfer_buffer_length, >- dma_get_cache_alignment()), >- &urb->transfer_buffer, sizeof(urb->transfer_buffer)); >- >+ /* Position our struct dma_aligned_buffer such that data is aligned */ >+ temp = PTR_ALIGN(kmalloc_ptr + 1, DWC2_USB_DMA_ALIGN) - 1; >+ temp->kmalloc_ptr = kmalloc_ptr; >+ temp->old_xfer_buffer = urb->transfer_buffer; > if (usb_urb_dir_out(urb)) >- memcpy(kmalloc_ptr, urb->transfer_buffer, >+ memcpy(temp->data, urb->transfer_buffer, > urb->transfer_buffer_length); >- urb->transfer_buffer = kmalloc_ptr; >+ urb->transfer_buffer = temp->data; > > urb->transfer_flags |= URB_ALIGNED_TEMP_BUFFER; > >-- >2.17.1 > >From 9df13854b3717f8c16a2012dec614f737bb8c15d Mon Sep 17 00:00:00 2001 >From: Guenter Roeck <linux@roeck-us.net> >Date: Mon, 10 Feb 2020 13:11:00 -0800 >Subject: [PATCH 2/2] usb: dwc2: Allocate input buffers as multiples of > wMaxPacketSize >MIME-Version: 1.0 >Content-Type: text/plain; charset=UTF-8 >Content-Transfer-Encoding: 8bit > >The following messages are seen on Veyron Chromebooks running v4.19 or >later kernels. > >dwc2 ff580000.usb: dwc2_update_urb_state(): trimming xfer length >dwc2 ff580000.usb: dwc2_hc_chhltd_intr_dma: Channel 7 - ChHltd set, but reason is unknown >dwc2 ff580000.usb: hcint 0x00000002, intsts 0x04600021 > >This is typically followed by a crash. > >Unable to handle kernel paging request at virtual address 29f9d9fc >pgd = 4797dac9 >[29f9d9fc] *pgd=80000000004003, *pmd=00000000 >Internal error: Oops: a06 [#1] PREEMPT SMP ARM >Modules linked in: ip6t_REJECT rfcomm i2c_dev uinput hci_uart btbcm ... >CPU: 0 PID: 0 Comm: swapper/0 Tainted: G W 4.19.87-07825-g4ab3515f6e4d #1 >Hardware name: Rockchip (Device Tree) >PC is at memcpy+0x50/0x330 >LR is at 0xdd9ac94 >... >[<c0a89f50>] (memcpy) from [<c0783b94>] (dwc2_free_dma_aligned_buffer+0x5c/0x7c) >[<c0783b94>] (dwc2_free_dma_aligned_buffer) from [<c0765dcc>] (__usb_hcd_giveback_urb+0x78/0x130) >[<c0765dcc>] (__usb_hcd_giveback_urb) from [<c07678fc>] (usb_giveback_urb_bh+0xa0/0xe4) >[<c07678fc>] (usb_giveback_urb_bh) from [<c023a164>] (tasklet_action_common+0xc0/0xdc) >[<c023a164>] (tasklet_action_common) from [<c02021f0>] (__do_softirq+0x1b8/0x434) >[<c02021f0>] (__do_softirq) from [<c0239a14>] (irq_exit+0xdc/0xe0) >[<c0239a14>] (irq_exit) from [<c029f260>] (__handle_domain_irq+0x94/0xd0) >[<c029f260>] (__handle_domain_irq) from [<c05da780>] (gic_handle_irq+0x74/0xb0) >[<c05da780>] (gic_handle_irq) from [<c02019f8>] (__irq_svc+0x58/0x8c) > >The crash suggests that the memory after the end of a temporary DMA-aligned >buffer is overwritten. > >The problem is typically only seen in kernels which include commit >56406e017a88 ("usb: dwc2: Fix DMA alignment to start at allocated >boundary"), presumably because that commit moves the pointer to the old >buffer to the end of the newly allocated buffer, where it is more likely >to be overwritten. > >Code analysis shows that the transfer size programmed into the chip for >input transfers is a multiple of an endpoint's maximum packet size >(wMaxPacketSize). In the observed situation, the transfer size and thus >the size of the input buffer is 1522 bytes. With a maximum packet size >of 64 bytes, the chip is programmed to receive up to 1536 bytes of data. >This overwrites the end of the buffer. > >To work around the problem, always allocate a multiple of wMaxPacketSize >bytes for receive buffers. Do this even for DMA-aligned buffers if the >receive buffer size is not a multiple of wMaxPacketSize. At the same time, >do not update chan->xfer_len if the transfer size is 0. > >Reported-by: Boris ARZUR <boris@konbu.org> >Cc: Boris ARZUR <boris@konbu.org> >Cc: Jonathan Bell <jonathan@raspberrypi.org> >Cc: Antti Seppälä <a.seppala@gmail.com> >Fixes: 56406e017a88 ("usb: dwc2: Fix DMA alignment to start at allocated boundary") >Signed-off-by: Guenter Roeck <linux@roeck-us.net> >--- > drivers/usb/dwc2/hcd.c | 37 +++++++++++++++++++++++++++---------- > 1 file changed, 27 insertions(+), 10 deletions(-) > >diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c >index b5841197165a..f27dc11e409c 100644 >--- a/drivers/usb/dwc2/hcd.c >+++ b/drivers/usb/dwc2/hcd.c >@@ -1313,18 +1313,20 @@ static void dwc2_hc_start_transfer(struct dwc2_hsotg *hsotg, > if (num_packets > max_hc_pkt_count) { > num_packets = max_hc_pkt_count; > chan->xfer_len = num_packets * chan->max_packet; >+ } else if (chan->ep_is_in) { >+ /* >+ * Always program an integral # of max packets for IN >+ * transfers. >+ * Note: This assumes that the input buffer is >+ * aligned accordingly. >+ */ >+ chan->xfer_len = num_packets * chan->max_packet; > } > } else { > /* Need 1 packet for transfer length of 0 */ > num_packets = 1; > } > >- if (chan->ep_is_in) >- /* >- * Always program an integral # of max packets for IN >- * transfers >- */ >- chan->xfer_len = num_packets * chan->max_packet; > > if (chan->ep_type == USB_ENDPOINT_XFER_INT || > chan->ep_type == USB_ENDPOINT_XFER_ISOC) >@@ -2505,16 +2507,31 @@ static void dwc2_free_dma_aligned_buffer(struct urb *urb) > static int dwc2_alloc_dma_aligned_buffer(struct urb *urb, gfp_t mem_flags) > { > struct dma_aligned_buffer *temp, *kmalloc_ptr; >+ struct usb_host_endpoint *ep = urb->ep; >+ int maxp = usb_endpoint_maxp(&ep->desc); > size_t kmalloc_size; > >- if (urb->num_sgs || urb->sg || >- urb->transfer_buffer_length == 0 || >+ if (urb->num_sgs || urb->sg || urb->transfer_buffer_length == 0) >+ return 0; >+ >+ /* >+ * Input transfer buffer size must be a multiple of the endpoint's >+ * maximum packet size to match the transfer limit programmed into >+ * the chip. >+ * See calculation of chan->xfer_len in dwc2_hc_start_transfer(). >+ */ >+ if (usb_urb_dir_out(urb)) >+ kmalloc_size = urb->transfer_buffer_length; >+ else >+ kmalloc_size = roundup(urb->transfer_buffer_length, maxp); >+ >+ if (kmalloc_size == urb->transfer_buffer_length && > !((uintptr_t)urb->transfer_buffer & (DWC2_USB_DMA_ALIGN - 1))) > return 0; > > /* Allocate a buffer with enough padding for alignment */ >- kmalloc_size = urb->transfer_buffer_length + >- sizeof(struct dma_aligned_buffer) + DWC2_USB_DMA_ALIGN - 1; >+ kmalloc_size += sizeof(struct dma_aligned_buffer) + >+ DWC2_USB_DMA_ALIGN - 1; > > kmalloc_ptr = kmalloc(kmalloc_size, mem_flags); > if (!kmalloc_ptr) >-- >2.17.1 > [-- Attachment #2: break_original.patch --] [-- Type: text/plain, Size: 1197 bytes --] diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c index 2192a28..4c45642 100644 --- a/drivers/usb/dwc2/hcd.c +++ b/drivers/usb/dwc2/hcd.c @@ -2503,12 +2503,27 @@ static int dwc2_alloc_dma_aligned_buffer(struct urb *urb, gfp_t mem_flags) { void *kmalloc_ptr; size_t kmalloc_size; + struct usb_host_endpoint *ep = urb->ep; + int maxp = usb_endpoint_maxp(&ep->desc); if (urb->num_sgs || urb->sg || - urb->transfer_buffer_length == 0 || - !((uintptr_t)urb->transfer_buffer & (DWC2_USB_DMA_ALIGN - 1))) + urb->transfer_buffer_length == 0) return 0; + if (!((uintptr_t)urb->transfer_buffer & (DWC2_USB_DMA_ALIGN - 1))) { + //[ 8906.761517] EARLY RET len:13 out:0 in:1 type:3 mx:512 + //[ 8908.776755] EARLY RET len:13 out:0 in:1 type:3 mx:512 + if (urb->transfer_buffer_length == 13) { + printk("EARLY RET len:%u out:%d in:%d type:%d mx:%d ", + urb->transfer_buffer_length, + usb_urb_dir_out(urb) ? 1 : 0, + usb_urb_dir_in(urb) ? 1 : 0, + usb_pipetype(urb->pipe), + maxp); + return 0; + } + } + /* * Allocate a buffer with enough padding for original transfer_buffer * pointer. This allocation is guaranteed to be aligned properly for ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH] usb: dwc2: extend treatment for incomplete transfer 2020-02-23 12:02 ` Boris ARZUR @ 2020-02-23 13:53 ` Guenter Roeck 2020-02-25 0:18 ` Guenter Roeck 1 sibling, 0 replies; 20+ messages in thread From: Guenter Roeck @ 2020-02-23 13:53 UTC (permalink / raw) To: Boris ARZUR Cc: linux-usb, Greg Kroah-Hartman, Minas Harutyunyan, William Wu, Dmitry Torokhov, Douglas Anderson, a.seppala Hi Boris, On 2/23/20 4:02 AM, Boris ARZUR wrote: > Hi Guenter, > > I tried your series of patch. rndis_host tethering & loading the machine > seems to work fine. No more crashing. > > That being said, I now have an issue with regular USB keys (I tried a few): > usb 3-1: reset high-speed USB device number 2 using dwc2 > > I was able to reproduce this issue with the unpatched kernel, by disabling > the early return in dwc2_alloc_dma_aligned_buffer(), see attached. > There are times were re-allocation fails, either with your patch or with > the (almost-)original code. > > In particular it seems that there is a packet of lenght 13, usb_urb_dir_in() == true, > usb_pipetype(urb->pipe) == PIPE_BULK, that comes in every 2s or so, that > does not reallocate properly. > > In the original code, it's not a problem thanks to the early return, but your code > wants 512B (maxp) and forces reallocation... > Thanks for finding this. Back to the drawing board. > Thanks, Boris. > > Guenter Roeck wrote: >> On Sat, Feb 15, 2020 at 02:36:47PM +0900, Boris ARZUR wrote: >>> Hi Guenter, >>> >>>>> The first time around I was 0/ changing fonts 1/ trimming the dump message >>>>> in the kernel 2/ filming my screen. That's not practical at all... >>> Mmm, pstore does seem to work on my machine. CHROME_PSTORE is not available >>> to me because I'm not on x86, I just enabled the rest and nothing pops up Your system is Veyron, isn't it ? It does support pstore (I am using one for my testing as well), but I guess that depends on the BIOS/firmware installed. Guenter >>> in /sys/fs/pstore... >>> >>> So I took pictures (OCR did not help): >>> - https://funyu.konbu.org/pQUF2P08etcpVxuKZ0A720%2b0/IMG_20200215_134343.jpg.webp >>> is a stack trace for your earlier patch "min_t", in >>> https://www.spinics.net/lists/linux-usb/msg191019.html ; >>> - https://funyu.konbu.org/pQUF2P08etcpVxuKZ0A720%2b0/IMG_20200215_135816.jpg.webp >>> is a stack trace for your later patch "container_of", in >>> https://www.spinics.net/lists/linux-usb/msg191074.html . >>> >>> Both patches crash (without even loading the machine), but "container_of" is >>> a bit more resilient. >>> >> >> Yes, those patches didn't address the core problem. Can you test with the >> attached two patches ? >> >> Thanks, >> Guenter > >>From a1c0551b62b038b495177737828f986961184110 Mon Sep 17 00:00:00 2001 >> From: Guenter Roeck <linux@roeck-us.net> >> Date: Mon, 10 Feb 2020 14:04:06 -0800 >> Subject: [PATCH 1/2] usb: dwc2: Simplify DMA alignment code >> >> The code to align buffers for DMA was first introduced with commit >> 3bc04e28a030 ("usb: dwc2: host: Get aligned DMA in a more supported way"). >> It was updated with commit 56406e017a88 ("usb: dwc2: Fix DMA alignment >> to start at allocated boundary") because it did not really align buffers to >> DMA boundaries but to DWC2_USB_DMA_ALIGN. This was then optimized in commit >> 1e111e885238 ("usb: dwc2: Fix inefficient copy of unaligned buffers") >> to only copy actual data rather than the whole buffer. Commit 4a4863bf2e79 >> ("usb: dwc2: Fix DMA cache alignment issues") changed this further to add >> a padding at the end of the buffer to ensure that the old data pointer is >> not in the same cache line as the buffer. >> >> This last commit states "Otherwise, the stored_xfer_buffer gets corrupted >> for IN URBs on non-cache-coherent systems". However, such corruptions are >> still observed. Either case, DMA is not expected to overwrite more memory >> than it is supposed to do, suggesting that the commit may have been hiding >> a problem rather than fixing it. >> >> On top of that, DMA alignment is still not guaranteed since it only happens >> if the original buffer is not aligned to DWC2_USB_DMA_ALIGN, which is still >> a constant of 4 and not really associated with DMA alignment. >> >> Move the old data pointer back to the beginning of the new buffer, >> restoring most of the original commit and to simplify the code. Define >> DWC2_USB_DMA_ALIGN to dma_get_cache_alignment() to fix the DMA alignment >> for real this time. >> >> Signed-off-by: Guenter Roeck <linux@roeck-us.net> >> --- >> drivers/usb/dwc2/hcd.c | 50 +++++++++++++++++++----------------------- >> 1 file changed, 22 insertions(+), 28 deletions(-) >> >> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c >> index b90f858af960..b5841197165a 100644 >> --- a/drivers/usb/dwc2/hcd.c >> +++ b/drivers/usb/dwc2/hcd.c >> @@ -2469,21 +2469,24 @@ static int dwc2_alloc_split_dma_aligned_buf(struct dwc2_hsotg *hsotg, >> return 0; >> } >> >> -#define DWC2_USB_DMA_ALIGN 4 >> +#define DWC2_USB_DMA_ALIGN dma_get_cache_alignment() >> + >> +struct dma_aligned_buffer { >> + void *kmalloc_ptr; >> + void *old_xfer_buffer; >> + u8 data[0]; >> +}; >> >> static void dwc2_free_dma_aligned_buffer(struct urb *urb) >> { >> - void *stored_xfer_buffer; >> + struct dma_aligned_buffer *temp; >> size_t length; >> >> if (!(urb->transfer_flags & URB_ALIGNED_TEMP_BUFFER)) >> return; >> >> - /* Restore urb->transfer_buffer from the end of the allocated area */ >> - memcpy(&stored_xfer_buffer, >> - PTR_ALIGN(urb->transfer_buffer + urb->transfer_buffer_length, >> - dma_get_cache_alignment()), >> - sizeof(urb->transfer_buffer)); >> + temp = container_of(urb->transfer_buffer, >> + struct dma_aligned_buffer, data); >> >> if (usb_urb_dir_in(urb)) { >> if (usb_pipeisoc(urb->pipe)) >> @@ -2491,17 +2494,17 @@ static void dwc2_free_dma_aligned_buffer(struct urb *urb) >> else >> length = urb->actual_length; >> >> - memcpy(stored_xfer_buffer, urb->transfer_buffer, length); >> + memcpy(temp->old_xfer_buffer, temp->data, length); >> } >> - kfree(urb->transfer_buffer); >> - urb->transfer_buffer = stored_xfer_buffer; >> + urb->transfer_buffer = temp->old_xfer_buffer; >> + kfree(temp->kmalloc_ptr); >> >> urb->transfer_flags &= ~URB_ALIGNED_TEMP_BUFFER; >> } >> >> static int dwc2_alloc_dma_aligned_buffer(struct urb *urb, gfp_t mem_flags) >> { >> - void *kmalloc_ptr; >> + struct dma_aligned_buffer *temp, *kmalloc_ptr; >> size_t kmalloc_size; >> >> if (urb->num_sgs || urb->sg || >> @@ -2509,31 +2512,22 @@ static int dwc2_alloc_dma_aligned_buffer(struct urb *urb, gfp_t mem_flags) >> !((uintptr_t)urb->transfer_buffer & (DWC2_USB_DMA_ALIGN - 1))) >> return 0; >> >> - /* >> - * Allocate a buffer with enough padding for original transfer_buffer >> - * pointer. This allocation is guaranteed to be aligned properly for >> - * DMA >> - */ >> + /* Allocate a buffer with enough padding for alignment */ >> kmalloc_size = urb->transfer_buffer_length + >> - (dma_get_cache_alignment() - 1) + >> - sizeof(urb->transfer_buffer); >> + sizeof(struct dma_aligned_buffer) + DWC2_USB_DMA_ALIGN - 1; >> >> kmalloc_ptr = kmalloc(kmalloc_size, mem_flags); >> if (!kmalloc_ptr) >> return -ENOMEM; >> >> - /* >> - * Position value of original urb->transfer_buffer pointer to the end >> - * of allocation for later referencing >> - */ >> - memcpy(PTR_ALIGN(kmalloc_ptr + urb->transfer_buffer_length, >> - dma_get_cache_alignment()), >> - &urb->transfer_buffer, sizeof(urb->transfer_buffer)); >> - >> + /* Position our struct dma_aligned_buffer such that data is aligned */ >> + temp = PTR_ALIGN(kmalloc_ptr + 1, DWC2_USB_DMA_ALIGN) - 1; >> + temp->kmalloc_ptr = kmalloc_ptr; >> + temp->old_xfer_buffer = urb->transfer_buffer; >> if (usb_urb_dir_out(urb)) >> - memcpy(kmalloc_ptr, urb->transfer_buffer, >> + memcpy(temp->data, urb->transfer_buffer, >> urb->transfer_buffer_length); >> - urb->transfer_buffer = kmalloc_ptr; >> + urb->transfer_buffer = temp->data; >> >> urb->transfer_flags |= URB_ALIGNED_TEMP_BUFFER; >> >> -- >> 2.17.1 >> > >>From 9df13854b3717f8c16a2012dec614f737bb8c15d Mon Sep 17 00:00:00 2001 >> From: Guenter Roeck <linux@roeck-us.net> >> Date: Mon, 10 Feb 2020 13:11:00 -0800 >> Subject: [PATCH 2/2] usb: dwc2: Allocate input buffers as multiples of >> wMaxPacketSize >> MIME-Version: 1.0 >> Content-Type: text/plain; charset=UTF-8 >> Content-Transfer-Encoding: 8bit >> >> The following messages are seen on Veyron Chromebooks running v4.19 or >> later kernels. >> >> dwc2 ff580000.usb: dwc2_update_urb_state(): trimming xfer length >> dwc2 ff580000.usb: dwc2_hc_chhltd_intr_dma: Channel 7 - ChHltd set, but reason is unknown >> dwc2 ff580000.usb: hcint 0x00000002, intsts 0x04600021 >> >> This is typically followed by a crash. >> >> Unable to handle kernel paging request at virtual address 29f9d9fc >> pgd = 4797dac9 >> [29f9d9fc] *pgd=80000000004003, *pmd=00000000 >> Internal error: Oops: a06 [#1] PREEMPT SMP ARM >> Modules linked in: ip6t_REJECT rfcomm i2c_dev uinput hci_uart btbcm ... >> CPU: 0 PID: 0 Comm: swapper/0 Tainted: G W 4.19.87-07825-g4ab3515f6e4d #1 >> Hardware name: Rockchip (Device Tree) >> PC is at memcpy+0x50/0x330 >> LR is at 0xdd9ac94 >> ... >> [<c0a89f50>] (memcpy) from [<c0783b94>] (dwc2_free_dma_aligned_buffer+0x5c/0x7c) >> [<c0783b94>] (dwc2_free_dma_aligned_buffer) from [<c0765dcc>] (__usb_hcd_giveback_urb+0x78/0x130) >> [<c0765dcc>] (__usb_hcd_giveback_urb) from [<c07678fc>] (usb_giveback_urb_bh+0xa0/0xe4) >> [<c07678fc>] (usb_giveback_urb_bh) from [<c023a164>] (tasklet_action_common+0xc0/0xdc) >> [<c023a164>] (tasklet_action_common) from [<c02021f0>] (__do_softirq+0x1b8/0x434) >> [<c02021f0>] (__do_softirq) from [<c0239a14>] (irq_exit+0xdc/0xe0) >> [<c0239a14>] (irq_exit) from [<c029f260>] (__handle_domain_irq+0x94/0xd0) >> [<c029f260>] (__handle_domain_irq) from [<c05da780>] (gic_handle_irq+0x74/0xb0) >> [<c05da780>] (gic_handle_irq) from [<c02019f8>] (__irq_svc+0x58/0x8c) >> >> The crash suggests that the memory after the end of a temporary DMA-aligned >> buffer is overwritten. >> >> The problem is typically only seen in kernels which include commit >> 56406e017a88 ("usb: dwc2: Fix DMA alignment to start at allocated >> boundary"), presumably because that commit moves the pointer to the old >> buffer to the end of the newly allocated buffer, where it is more likely >> to be overwritten. >> >> Code analysis shows that the transfer size programmed into the chip for >> input transfers is a multiple of an endpoint's maximum packet size >> (wMaxPacketSize). In the observed situation, the transfer size and thus >> the size of the input buffer is 1522 bytes. With a maximum packet size >> of 64 bytes, the chip is programmed to receive up to 1536 bytes of data. >> This overwrites the end of the buffer. >> >> To work around the problem, always allocate a multiple of wMaxPacketSize >> bytes for receive buffers. Do this even for DMA-aligned buffers if the >> receive buffer size is not a multiple of wMaxPacketSize. At the same time, >> do not update chan->xfer_len if the transfer size is 0. >> >> Reported-by: Boris ARZUR <boris@konbu.org> >> Cc: Boris ARZUR <boris@konbu.org> >> Cc: Jonathan Bell <jonathan@raspberrypi.org> >> Cc: Antti Seppälä <a.seppala@gmail.com> >> Fixes: 56406e017a88 ("usb: dwc2: Fix DMA alignment to start at allocated boundary") >> Signed-off-by: Guenter Roeck <linux@roeck-us.net> >> --- >> drivers/usb/dwc2/hcd.c | 37 +++++++++++++++++++++++++++---------- >> 1 file changed, 27 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c >> index b5841197165a..f27dc11e409c 100644 >> --- a/drivers/usb/dwc2/hcd.c >> +++ b/drivers/usb/dwc2/hcd.c >> @@ -1313,18 +1313,20 @@ static void dwc2_hc_start_transfer(struct dwc2_hsotg *hsotg, >> if (num_packets > max_hc_pkt_count) { >> num_packets = max_hc_pkt_count; >> chan->xfer_len = num_packets * chan->max_packet; >> + } else if (chan->ep_is_in) { >> + /* >> + * Always program an integral # of max packets for IN >> + * transfers. >> + * Note: This assumes that the input buffer is >> + * aligned accordingly. >> + */ >> + chan->xfer_len = num_packets * chan->max_packet; >> } >> } else { >> /* Need 1 packet for transfer length of 0 */ >> num_packets = 1; >> } >> >> - if (chan->ep_is_in) >> - /* >> - * Always program an integral # of max packets for IN >> - * transfers >> - */ >> - chan->xfer_len = num_packets * chan->max_packet; >> >> if (chan->ep_type == USB_ENDPOINT_XFER_INT || >> chan->ep_type == USB_ENDPOINT_XFER_ISOC) >> @@ -2505,16 +2507,31 @@ static void dwc2_free_dma_aligned_buffer(struct urb *urb) >> static int dwc2_alloc_dma_aligned_buffer(struct urb *urb, gfp_t mem_flags) >> { >> struct dma_aligned_buffer *temp, *kmalloc_ptr; >> + struct usb_host_endpoint *ep = urb->ep; >> + int maxp = usb_endpoint_maxp(&ep->desc); >> size_t kmalloc_size; >> >> - if (urb->num_sgs || urb->sg || >> - urb->transfer_buffer_length == 0 || >> + if (urb->num_sgs || urb->sg || urb->transfer_buffer_length == 0) >> + return 0; >> + >> + /* >> + * Input transfer buffer size must be a multiple of the endpoint's >> + * maximum packet size to match the transfer limit programmed into >> + * the chip. >> + * See calculation of chan->xfer_len in dwc2_hc_start_transfer(). >> + */ >> + if (usb_urb_dir_out(urb)) >> + kmalloc_size = urb->transfer_buffer_length; >> + else >> + kmalloc_size = roundup(urb->transfer_buffer_length, maxp); >> + >> + if (kmalloc_size == urb->transfer_buffer_length && >> !((uintptr_t)urb->transfer_buffer & (DWC2_USB_DMA_ALIGN - 1))) >> return 0; >> >> /* Allocate a buffer with enough padding for alignment */ >> - kmalloc_size = urb->transfer_buffer_length + >> - sizeof(struct dma_aligned_buffer) + DWC2_USB_DMA_ALIGN - 1; >> + kmalloc_size += sizeof(struct dma_aligned_buffer) + >> + DWC2_USB_DMA_ALIGN - 1; >> >> kmalloc_ptr = kmalloc(kmalloc_size, mem_flags); >> if (!kmalloc_ptr) >> -- >> 2.17.1 >> > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] usb: dwc2: extend treatment for incomplete transfer 2020-02-23 12:02 ` Boris ARZUR 2020-02-23 13:53 ` Guenter Roeck @ 2020-02-25 0:18 ` Guenter Roeck 1 sibling, 0 replies; 20+ messages in thread From: Guenter Roeck @ 2020-02-25 0:18 UTC (permalink / raw) To: Boris ARZUR Cc: linux-usb, Greg Kroah-Hartman, Minas Harutyunyan, William Wu, Dmitry Torokhov, Douglas Anderson, a.seppala On Sun, Feb 23, 2020 at 09:02:47PM +0900, Boris ARZUR wrote: > Hi Guenter, > > I tried your series of patch. rndis_host tethering & loading the machine > seems to work fine. No more crashing. > > That being said, I now have an issue with regular USB keys (I tried a few): > usb 3-1: reset high-speed USB device number 2 using dwc2 > > I was able to reproduce this issue with the unpatched kernel, by disabling > the early return in dwc2_alloc_dma_aligned_buffer(), see attached. > There are times were re-allocation fails, either with your patch or with > the (almost-)original code. > > In particular it seems that there is a packet of lenght 13, usb_urb_dir_in() == true, > usb_pipetype(urb->pipe) == PIPE_BULK, that comes in every 2s or so, that > does not reallocate properly. > Those packets have URB_NO_TRANSFER_DMA_MAP set. If that is the case, the packet is not received into the transfer buffer but into an already assigned DMA buffer/address. Providing a temporary buffer does not have an effect; the packet is still received into the orginal buffer (and then overwritten with the data in the temporary buffer). That means we have to leave such packets alone. I'll send out an updated series later tonight or tomorrow. I'll probably send it as RFT series this time. Guenter ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] usb: dwc2: extend treatment for incomplete transfer 2020-02-15 5:36 ` Boris ARZUR 2020-02-19 21:10 ` Guenter Roeck @ 2020-02-20 21:22 ` Guenter Roeck 1 sibling, 0 replies; 20+ messages in thread From: Guenter Roeck @ 2020-02-20 21:22 UTC (permalink / raw) To: Boris ARZUR Cc: linux-usb, Greg Kroah-Hartman, Minas Harutyunyan, William Wu, Dmitry Torokhov, Douglas Anderson Hi Boris, On Sat, Feb 15, 2020 at 02:36:47PM +0900, Boris ARZUR wrote: > Hi Guenter, > > >> The first time around I was 0/ changing fonts 1/ trimming the dump message > >> in the kernel 2/ filming my screen. That's not practical at all... > Mmm, pstore does seem to work on my machine. CHROME_PSTORE is not available > to me because I'm not on x86, I just enabled the rest and nothing pops up > in /sys/fs/pstore... > > So I took pictures (OCR did not help): > - https://funyu.konbu.org/pQUF2P08etcpVxuKZ0A720%2b0/IMG_20200215_134343.jpg.webp > is a stack trace for your earlier patch "min_t", in > https://www.spinics.net/lists/linux-usb/msg191019.html ; > - https://funyu.konbu.org/pQUF2P08etcpVxuKZ0A720%2b0/IMG_20200215_135816.jpg.webp > is a stack trace for your later patch "container_of", in > https://www.spinics.net/lists/linux-usb/msg191074.html . > > Both patches crash (without even loading the machine), but "container_of" is > a bit more resilient. > Please find yet another patch attached. This one applies on top of the two patches I sent you yesterday. This patch still needs more testing, but it or something similar will be essential to fix the problem. Thanks, Guenter --- From 2ac7dd226942c48532587f69e48491de940176e2 Mon Sep 17 00:00:00 2001 From: Guenter Roeck <linux@roeck-us.net> Date: Thu, 20 Feb 2020 12:47:57 -0800 Subject: [PATCH] usb: dwc2: Abort transaction after errors if the receive buffer is full. In some situations, the following error messages are reported. dwc2 ff540000.usb: dwc2_hc_chhltd_intr_dma: Channel 1 - ChHltd set, but reason is unknown dwc2 ff540000.usb: hcint 0x00000002, intsts 0x04000021 This is sometimes followed by: dwc2 ff540000.usb: dwc2_update_urb_state_abn(): trimming xfer length and then: WARNING: CPU: 0 PID: 0 at kernel/v4.19/drivers/usb/dwc2/hcd.c:2913 dwc2_assign_and_init_hc+0x98c/0x990 The warning suggests an odd buffer address to be used for DMA. After the first messages, the receive buffer is full (urb->actual_length >= urb->length). However, the urb is still left in the queue. When it is selected again, the dwc2 hcd code translates this into a 1-block transfer; the length of that transfer depends on the endpoint's maximum block size. If and when that packet is received, it is placed at the end of the receive buffer (which was already full). This can have all kinds of adverse affects. To solve the problem, abort input transactions if there was an error and the receive buffer is full. We could possibly handle this situation as "transfer complete", but that seems risky since we don't really know why the transaction was aborted. Cc: Boris ARZUR <boris@konbu.org> Cc: Douglas Anderson <dianders@chromium.org> Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- drivers/usb/dwc2/hcd_intr.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c index a052d39b4375..1d99af809033 100644 --- a/drivers/usb/dwc2/hcd_intr.c +++ b/drivers/usb/dwc2/hcd_intr.c @@ -1167,8 +1167,11 @@ static void dwc2_hc_stall_intr(struct dwc2_hsotg *hsotg, * abnormal condition before the transfer completes. Modifies the * actual_length field of the URB to reflect the number of bytes that have * actually been transferred via the host channel. + * + * Return true if the receive buffer is full on an input transaction, + * false otherwise. */ -static void dwc2_update_urb_state_abn(struct dwc2_hsotg *hsotg, +static bool dwc2_update_urb_state_abn(struct dwc2_hsotg *hsotg, struct dwc2_host_chan *chan, int chnum, struct dwc2_hcd_urb *urb, struct dwc2_qtd *qtd, @@ -1199,6 +1202,8 @@ static void dwc2_update_urb_state_abn(struct dwc2_hsotg *hsotg, urb->actual_length); dev_vdbg(hsotg->dev, " urb->transfer_buffer_length %d\n", urb->length); + + return chan->ep_is_in && urb->actual_length >= urb->length; } /* @@ -1973,10 +1978,15 @@ static void dwc2_hc_chhltd_intr_dma(struct dwc2_hsotg *hsotg, "NYET/NAK/ACK/other in non-error case, 0x%08x\n", chan->hcint); error: - /* Failthrough: use 3-strikes rule */ - qtd->error_count++; - dwc2_update_urb_state_abn(hsotg, chan, chnum, qtd->urb, - qtd, DWC2_HC_XFER_XACT_ERR); + /* + * Failthrough: use 3-strikes rule. Abort input transactions + * after errors if the receive buffer is full. + */ + if (dwc2_update_urb_state_abn(hsotg, chan, chnum, qtd->urb, + qtd, DWC2_HC_XFER_XACT_ERR)) + qtd->error_count = 3; + else + qtd->error_count++; dwc2_hcd_save_data_toggle(hsotg, chan, chnum, qtd); dwc2_halt_channel(hsotg, chan, qtd, DWC2_HC_XFER_XACT_ERR); } -- 2.17.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH] usb: dwc2: extend treatment for incomplete transfer @ 2019-11-05 3:29 Boris ARZUR 2019-11-05 3:39 ` Boris ARZUR 2020-01-31 22:09 ` Guenter Roeck 0 siblings, 2 replies; 20+ messages in thread From: Boris ARZUR @ 2019-11-05 3:29 UTC (permalink / raw) To: linux-usb Cc: Felipe Balbi, Greg Kroah-Hartman, Minas Harutyunyan, Grigor Tovmasyan, Gevorg Sahakyan, John Youn, Sevak Arakelyan, William Wu, Dmitry Torokhov, Douglas Anderson Channel halt can happen with BULK endpoints when the cpu is under high load. Treating it as an error leads to a null-pointer dereference in dwc2_free_dma_aligned_buffer(). Signed-off-by: Boris Arzur <boris@konbu.org> --- drivers/usb/dwc2/hcd_intr.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c index a052d39b4375..697fed530aeb 100644 --- a/drivers/usb/dwc2/hcd_intr.c +++ b/drivers/usb/dwc2/hcd_intr.c @@ -1944,7 +1944,8 @@ static void dwc2_hc_chhltd_intr_dma(struct dwc2_hsotg *hsotg, */ dwc2_hc_ack_intr(hsotg, chan, chnum, qtd); } else { - if (chan->ep_type == USB_ENDPOINT_XFER_INT || + if (chan->ep_type == USB_ENDPOINT_XFER_BULK || + chan->ep_type == USB_ENDPOINT_XFER_INT || chan->ep_type == USB_ENDPOINT_XFER_ISOC) { /* * A periodic transfer halted with no other -- 2.23.0 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH] usb: dwc2: extend treatment for incomplete transfer 2019-11-05 3:29 Boris ARZUR @ 2019-11-05 3:39 ` Boris ARZUR 2020-01-31 22:09 ` Guenter Roeck 1 sibling, 0 replies; 20+ messages in thread From: Boris ARZUR @ 2019-11-05 3:39 UTC (permalink / raw) To: linux-usb Cc: Felipe Balbi, Greg Kroah-Hartman, Minas Harutyunyan, Gevorg Sahakyan, John Youn, William Wu, Dmitry Torokhov, Douglas Anderson Hi, First post in this list, please be lenient. Replying to self to give some context: I'm on a Asus c201 (rk3288) and I see some crashes with cdc_ether. Here is how to repro: - create heavy usb network load: I tether my phone and netcat some file from it; - create heavy CPU load (pushd linux; make -j 6) - observe kernel messages: dwc2 ff580000.usb: dwc2_hc_chhltd_intr_dma: Channel 7 - ChHltd set, but reason is unknown dwc2 ff580000.usb: hcint 0x00000002, intsts 0x04200021 dwc2 ff580000.usb: ep_type 0x00000002 bulk /* ba: ADDED LOG */ The kernel will write to 0 at line 2494 below in file drivers/usb/dwc2/hcd.c 2474 static void dwc2_free_dma_aligned_buffer(struct urb *urb) 2475 { /* ... */ 2482 /* Restore urb->transfer_buffer from the end of the allocated area */ 2483 memcpy(&stored_xfer_buffer, 2484 PTR_ALIGN(urb->transfer_buffer + urb->transfer_buffer_length, 2485 dma_get_cache_alignment()), 2486 sizeof(urb->transfer_buffer)); /* ... */ 2494 memcpy(stored_xfer_buffer, urb->transfer_buffer, length); /* ... */ 2500 } The fix I propose has been working fine on my machine, but I confess I am less than familiar with this area... My guess is that the kernel misses some deadlines due to contention and we see channel halts. I tried treating these as we do the other (with other end point types) and it solved the crashes. I verified on next-20191030 that the data is correctly transfered over the network (no corruption). Thank you & regards, Boris. >Channel halt can happen with BULK endpoints when the >cpu is under high load. Treating it as an error leads >to a null-pointer dereference in dwc2_free_dma_aligned_buffer(). > >Signed-off-by: Boris Arzur <boris@konbu.org> >--- > drivers/usb/dwc2/hcd_intr.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > >diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c >index a052d39b4375..697fed530aeb 100644 >--- a/drivers/usb/dwc2/hcd_intr.c >+++ b/drivers/usb/dwc2/hcd_intr.c >@@ -1944,7 +1944,8 @@ static void dwc2_hc_chhltd_intr_dma(struct dwc2_hsotg >*hsotg, > */ > dwc2_hc_ack_intr(hsotg, chan, chnum, qtd); > } else { >- if (chan->ep_type == USB_ENDPOINT_XFER_INT || >+ if (chan->ep_type == USB_ENDPOINT_XFER_BULK || >+ chan->ep_type == USB_ENDPOINT_XFER_INT || > chan->ep_type == USB_ENDPOINT_XFER_ISOC) { > /* > * A periodic transfer halted with no other >-- >2.23.0 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] usb: dwc2: extend treatment for incomplete transfer 2019-11-05 3:29 Boris ARZUR 2019-11-05 3:39 ` Boris ARZUR @ 2020-01-31 22:09 ` Guenter Roeck 2020-02-02 5:15 ` Boris ARZUR 1 sibling, 1 reply; 20+ messages in thread From: Guenter Roeck @ 2020-01-31 22:09 UTC (permalink / raw) To: Boris ARZUR Cc: linux-usb, Felipe Balbi, Greg Kroah-Hartman, Minas Harutyunyan, Grigor Tovmasyan, Gevorg Sahakyan, John Youn, Sevak Arakelyan, William Wu, Dmitry Torokhov, Douglas Anderson Hi Boris, On Tue, Nov 05, 2019 at 12:29:22PM +0900, Boris ARZUR wrote: > Channel halt can happen with BULK endpoints when the > cpu is under high load. Treating it as an error leads > to a null-pointer dereference in dwc2_free_dma_aligned_buffer(). > good find, and good analysis. We stated to see this problem as well in the latest ChromeOS kernel. I am still trying understand what exactly happens. To do that, I'll need to be able to reproduce the problem. Maybe you can help me. How do you tether your phone through USB ? Thanks, Guenter > Signed-off-by: Boris Arzur <boris@konbu.org> > --- > drivers/usb/dwc2/hcd_intr.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > * A periodic transfer halted with no other > -- > 2.23.0 > > diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c > index a052d39b4375..697fed530aeb 100644 > --- a/drivers/usb/dwc2/hcd_intr.c > +++ b/drivers/usb/dwc2/hcd_intr.c > @@ -1944,7 +1944,8 @@ static void dwc2_hc_chhltd_intr_dma(struct dwc2_hsotg > *hsotg, > */ > dwc2_hc_ack_intr(hsotg, chan, chnum, qtd); > } else { > - if (chan->ep_type == USB_ENDPOINT_XFER_INT || > + if (chan->ep_type == USB_ENDPOINT_XFER_BULK || > + chan->ep_type == USB_ENDPOINT_XFER_INT || > chan->ep_type == USB_ENDPOINT_XFER_ISOC) { > /* ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] usb: dwc2: extend treatment for incomplete transfer 2020-01-31 22:09 ` Guenter Roeck @ 2020-02-02 5:15 ` Boris ARZUR 2020-02-02 18:52 ` Guenter Roeck 0 siblings, 1 reply; 20+ messages in thread From: Boris ARZUR @ 2020-02-02 5:15 UTC (permalink / raw) To: Guenter Roeck Cc: linux-usb, Felipe Balbi, Greg Kroah-Hartman, Minas Harutyunyan, William Wu, Dmitry Torokhov, Douglas Anderson Hello Guenter, >good find, and good analysis. We stated to see this problem as well in the >latest ChromeOS kernel. I'm glad you find my report helpful. >be able to reproduce the problem. Maybe you can help me. How do you tether >your phone through USB ? You mention thethering, so I think you have read my follow-up: https://www.spinics.net/lists/linux-usb/msg187497.html My setup is as follows: - 'kenzo' phone (https://wiki.lineageos.org/devices/kenzo) on AICP 12.1 (android 7.1.2 linux 3.10.105); - 'veyron speedy' chromebook (https://wiki.gentoo.org/wiki/Asus_Chromebook_C201) on Arch Linux ARM, vanilla linux 5.2.14; Here are my repro steps, sorry if tedious, I'm not sure of the level of details you want, so I will go verbose squared :) : 0. plug in phone to chromebook, with a USB2 micro b cable; 1. activate usb tethering in phone settings: settings> more> tethering & portable hotspot> USB tethering click and confirm "tethered"; 2. chromebook sees phone as: [ 2128.080551] rndis_host 2-1:1.0 usb0: register 'rndis_host' at usb-ff580000.usb-1, RNDIS device, 4a:5e:0c:89:ec:09 3. chromebook$ sudo dhcpcd --noarp usb0 usb0: adding default route via 192.168.42.129 4. on phone, start termux (https://f-droid.org/en/packages/com.termux/) 5. phone$ dd if=/dev/urandom of=blob count=50 bs=1M 6. phone$ sha512sum blob b9e...14d blob 7. phone$ pkg install netcat 8. phone$ while true; do <blob netcat -l -p 9999; done 9. chromebook$ sudo pacman -Syu extra/gnu-netcat community/pigz 10. chromebook$ dd if=/dev/urandom of=job count=10 bs=1M 11. chromebook terminal 0$ while true; do <job pigz -11 -i -p 1024 >/dev/null; done 12. chromebook terminal 1$ cat /proc/loadavg 28.18 8.76 3.74 54/521 8826 13. chromebook terminal 1$ while true; do netcat 192.168.42.129 9999 | sha512sum; done b9e...14d - 13. chromebook will panic soon (I see repros in tens of seconds); I managed to track the issue to: > The kernel will write to 0 at line 2494 below in file drivers/usb/dwc2/hcd.c >2474 static void dwc2_free_dma_aligned_buffer(struct urb *urb) >2494 memcpy(stored_xfer_buffer, urb->transfer_buffer, length); I discussed the below patch with hminas@synopsys.com, who expressed doubts about its correctness. I tested it a while back and it seemed solid (no crash & correct hashes), but while writing this mail I see that sometimes the output of sha512sum on the chromebook is wrong... also, I'm thinking that the fix below may be a memory leak. In conclusion, do not commit, the fix needs more work :) I hope to restart experimenting in a short while, when I get a bit more free time. I am waiting for any question you may have, thank you for your time. Boris. Guenter Roeck wrote: >Hi Boris, > >On Tue, Nov 05, 2019 at 12:29:22PM +0900, Boris ARZUR wrote: >> Channel halt can happen with BULK endpoints when the >> cpu is under high load. Treating it as an error leads >> to a null-pointer dereference in dwc2_free_dma_aligned_buffer(). >> > >good find, and good analysis. We stated to see this problem as well in the >latest ChromeOS kernel. > >I am still trying understand what exactly happens. To do that, I'll need to >be able to reproduce the problem. Maybe you can help me. How do you tether >your phone through USB ? > >Thanks, >Guenter > >> Signed-off-by: Boris Arzur <boris@konbu.org> >> --- >> drivers/usb/dwc2/hcd_intr.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> * A periodic transfer halted with no other >> -- >> 2.23.0 >> >> diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c >> index a052d39b4375..697fed530aeb 100644 >> --- a/drivers/usb/dwc2/hcd_intr.c >> +++ b/drivers/usb/dwc2/hcd_intr.c >> @@ -1944,7 +1944,8 @@ static void dwc2_hc_chhltd_intr_dma(struct dwc2_hsotg >> *hsotg, >> */ >> dwc2_hc_ack_intr(hsotg, chan, chnum, qtd); >> } else { >> - if (chan->ep_type == USB_ENDPOINT_XFER_INT || >> + if (chan->ep_type == USB_ENDPOINT_XFER_BULK || >> + chan->ep_type == USB_ENDPOINT_XFER_INT || >> chan->ep_type == USB_ENDPOINT_XFER_ISOC) { >> /* ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] usb: dwc2: extend treatment for incomplete transfer 2020-02-02 5:15 ` Boris ARZUR @ 2020-02-02 18:52 ` Guenter Roeck 0 siblings, 0 replies; 20+ messages in thread From: Guenter Roeck @ 2020-02-02 18:52 UTC (permalink / raw) To: Boris ARZUR Cc: linux-usb, Felipe Balbi, Greg Kroah-Hartman, Minas Harutyunyan, William Wu, Dmitry Torokhov, Douglas Anderson Hi Boris, On 2/1/20 9:15 PM, Boris ARZUR wrote: > Hello Guenter, > > >> good find, and good analysis. We stated to see this problem as well in the >> latest ChromeOS kernel. > I'm glad you find my report helpful. > > >> be able to reproduce the problem. Maybe you can help me. How do you tether >> your phone through USB ? > You mention thethering, so I think you have read my follow-up: > https://www.spinics.net/lists/linux-usb/msg187497.html > > > My setup is as follows: > - 'kenzo' phone (https://wiki.lineageos.org/devices/kenzo) on AICP 12.1 > (android 7.1.2 linux 3.10.105); > - 'veyron speedy' chromebook (https://wiki.gentoo.org/wiki/Asus_Chromebook_C201) > on Arch Linux ARM, vanilla linux 5.2.14; > > > Here are my repro steps, sorry if tedious, I'm not sure of the level of > details you want, so I will go verbose squared :) : > > 0. plug in phone to chromebook, with a USB2 micro b cable; > > 1. activate usb tethering in phone settings: > settings> more> tethering & portable hotspot> USB tethering > click and confirm "tethered"; > > 2. chromebook sees phone as: > [ 2128.080551] rndis_host 2-1:1.0 usb0: register 'rndis_host' at usb-ff580000.usb-1, RNDIS device, 4a:5e:0c:89:ec:09 > > 3. chromebook$ sudo dhcpcd --noarp usb0 > usb0: adding default route via 192.168.42.129 > > 4. on phone, start termux (https://f-droid.org/en/packages/com.termux/) > > 5. phone$ dd if=/dev/urandom of=blob count=50 bs=1M > > 6. phone$ sha512sum blob > b9e...14d blob > > 7. phone$ pkg install netcat > > 8. phone$ while true; do <blob netcat -l -p 9999; done > > 9. chromebook$ sudo pacman -Syu extra/gnu-netcat community/pigz > > 10. chromebook$ dd if=/dev/urandom of=job count=10 bs=1M > > 11. chromebook terminal 0$ while true; do <job pigz -11 -i -p 1024 >/dev/null; done > > 12. chromebook terminal 1$ cat /proc/loadavg > 28.18 8.76 3.74 54/521 8826 > > 13. chromebook terminal 1$ while true; do netcat 192.168.42.129 9999 | sha512sum; done > b9e...14d - > > 13. chromebook will panic soon (I see repros in tens of seconds); > Thanks a lot for the information. I'll see if I can reproduce the problem using this (or a similar) approach. Tethering an Android phone isn't really difficult, but the traffic pattern seems to play a role as well. > I managed to track the issue to: >> The kernel will write to 0 at line 2494 below in file drivers/usb/dwc2/hcd.c >> 2474 static void dwc2_free_dma_aligned_buffer(struct urb *urb) >> 2494 memcpy(stored_xfer_buffer, urb->transfer_buffer, length); > > > I discussed the below patch with hminas@synopsys.com, who expressed doubts about its > correctness. > > I tested it a while back and it seemed solid (no crash & correct hashes), but while > writing this mail I see that sometimes the output of sha512sum on the > chromebook is wrong... also, I'm thinking that the fix below may be a memory > leak. > > > In conclusion, do not commit, the fix needs more work :) > Yes, I suspect that your patch is not a real fix but rather a bandage; that is why I want to reproduce the problem and spend some time trying to figure out what is going on. In a nutshell, even if the current code doesn't handle the situation well, it should not result in the observed problem (which looks like a memory overwrite). > I hope to restart experimenting in a short while, when I get a bit more free > time. > > > I am waiting for any question you may have, thank you for your time. > Boris. > Thanks! Guenter > Guenter Roeck wrote: >> Hi Boris, >> >> On Tue, Nov 05, 2019 at 12:29:22PM +0900, Boris ARZUR wrote: >>> Channel halt can happen with BULK endpoints when the >>> cpu is under high load. Treating it as an error leads >>> to a null-pointer dereference in dwc2_free_dma_aligned_buffer(). >>> >> >> good find, and good analysis. We stated to see this problem as well in the >> latest ChromeOS kernel. >> >> I am still trying understand what exactly happens. To do that, I'll need to >> be able to reproduce the problem. Maybe you can help me. How do you tether >> your phone through USB ? >> >> Thanks, >> Guenter >> >>> Signed-off-by: Boris Arzur <boris@konbu.org> >>> --- >>> drivers/usb/dwc2/hcd_intr.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> * A periodic transfer halted with no other >>> -- >>> 2.23.0 >>> >>> diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c >>> index a052d39b4375..697fed530aeb 100644 >>> --- a/drivers/usb/dwc2/hcd_intr.c >>> +++ b/drivers/usb/dwc2/hcd_intr.c >>> @@ -1944,7 +1944,8 @@ static void dwc2_hc_chhltd_intr_dma(struct dwc2_hsotg >>> *hsotg, >>> */ >>> dwc2_hc_ack_intr(hsotg, chan, chnum, qtd); >>> } else { >>> - if (chan->ep_type == USB_ENDPOINT_XFER_INT || >>> + if (chan->ep_type == USB_ENDPOINT_XFER_BULK || >>> + chan->ep_type == USB_ENDPOINT_XFER_INT || >>> chan->ep_type == USB_ENDPOINT_XFER_ISOC) { >>> /* ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2020-02-25 0:18 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-02-10 21:39 [PATCH] usb: dwc2: extend treatment for incomplete transfer Guenter Roeck 2020-02-11 5:49 ` Boris ARZUR 2020-02-11 13:26 ` Guenter Roeck 2020-02-11 16:15 ` Guenter Roeck 2020-02-15 5:36 ` Boris ARZUR 2020-02-19 21:10 ` Guenter Roeck 2020-02-23 11:00 ` Antti Seppälä 2020-02-23 12:10 ` Boris ARZUR 2020-02-23 13:45 ` Guenter Roeck 2020-02-23 18:20 ` Antti Seppälä 2020-02-23 18:47 ` Guenter Roeck 2020-02-23 12:02 ` Boris ARZUR 2020-02-23 13:53 ` Guenter Roeck 2020-02-25 0:18 ` Guenter Roeck 2020-02-20 21:22 ` Guenter Roeck -- strict thread matches above, loose matches on Subject: below -- 2019-11-05 3:29 Boris ARZUR 2019-11-05 3:39 ` Boris ARZUR 2020-01-31 22:09 ` Guenter Roeck 2020-02-02 5:15 ` Boris ARZUR 2020-02-02 18:52 ` Guenter Roeck
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).