On 3 July 2018 at 09:56, Doug Anderson <dianders@xxxxxxxxxxxx> wrote: > Hi, > > On Sun, Jul 1, 2018 at 11:30 PM, Antti Seppälä <a.seppala@xxxxxxxxx> wrote: >> On 30 June 2018 at 02:57, Doug Anderson <dianders@xxxxxxxxxxxx> wrote: >>> Hi, >>> >>> On Fri, Jun 29, 2018 at 11:29 AM, Antti Seppälä <a.seppala@xxxxxxxxx> wrote: >>>> Hi Doug, John and linux-usb. >>>> >>>> I'd like to report a regression in commit 3bc04e28a030 (usb: dwc2: >>>> host: Get aligned DMA in a more supported way) >>> >>> Seems unlikely, but any chance that >>> <https://patchwork.kernel.org/patch/10393775/> helps you? >>> >> >> Thank you for your suggestion but unfortunately the patch does not >> help and the crash remains. > > A few more shots in the dark in case they help: > > 1. For the kmalloc() in dwc2_alloc_dma_aligned_buffer(), change from: > > kmalloc_ptr = kmalloc(kmalloc_size, mem_flags); > > to: > > kmalloc_ptr = kmalloc(kmalloc_size, mem_flags | GFP_DMA); > > > The old code used to hardcode this, so maybe it somehow makes a difference? > I tried this but it did not have any effect on the crash. > --- > > 2. Change DWC2_USB_DMA_ALIGN to a larger size. Maybe 32 or 64? > I tried these values too but they did not help. It seems to me that these change the alignment of the start of the struct dma_aligned_buffer but since the issue is with the alignment of ->data inside that struct it is always moved to an unaligned location (offset by the two pointers at the start of the struct). There is even a mention of this requirement in Documentation/DMA-API.txt: Memory coherency operates at a granularity called the cache line width. In order for memory mapped by this API to operate correctly, the mapped region must begin exactly on a cache line boundary and end exactly on one (to prevent two separately mapped regions from sharing a single cache line). Since the cache line size may not be known at compile time, the API will not enforce this requirement. Therefore, it is recommended that driver writers who don't take special care to determine the cache line size at run time only map virtual regions that begin and end on page boundaries (which are guaranteed also to be cache line boundaries). > --- > > 3. Undo just the part of the patch that removed: > > /* > * Clip max_transfer_size to 65535. dwc2_hc_setup_align_buf() allocates > * coherent buffers with this size, and if it's too large we can > * exhaust the coherent DMA pool. > */ > if (hw->max_transfer_size > 65535) > hw->max_transfer_size = 65535; > > Maybe there's something on your platform where you have a problem with > big transfers? > I tried adding back this one as well but unfortunately no help from that either. Then I wrote a proof-of-concept patch that really solves the issue for me. The patch allocates additional space at the end of that temp bounce buffer and stores the original value of urb->transfer_buffer there for keeping until it is needed again. This way the bounce buffer for DMA is aligned on a page boundary that is compliant from DMA-API.txt perspective. Any thoughts or comments? If the patch doesn't look too crazy I can send it properly with git. -- a/drivers/usb/dwc2/hcd.c +++ b/drivers/usb/dwc2/hcd.c @@ -2606,34 +2606,28 @@ #define DWC2_USB_DMA_ALIGN 4 -struct dma_aligned_buffer { - void *kmalloc_ptr; - void *old_xfer_buffer; - u8 data[0]; -}; - static void dwc2_free_dma_aligned_buffer(struct urb *urb) { - struct dma_aligned_buffer *temp; + void *stored_xfer_buffer; if (!(urb->transfer_flags & URB_ALIGNED_TEMP_BUFFER)) return; - temp = container_of(urb->transfer_buffer, - struct dma_aligned_buffer, data); + memcpy(&stored_xfer_buffer, urb->transfer_buffer + + urb->transfer_buffer_length, sizeof(void *)); if (usb_urb_dir_in(urb)) - memcpy(temp->old_xfer_buffer, temp->data, + memcpy(stored_xfer_buffer, urb->transfer_buffer, urb->transfer_buffer_length); - urb->transfer_buffer = temp->old_xfer_buffer; - kfree(temp->kmalloc_ptr); + kfree(urb->transfer_buffer); + urb->transfer_buffer = stored_xfer_buffer; urb->transfer_flags &= ~URB_ALIGNED_TEMP_BUFFER; } static int dwc2_alloc_dma_aligned_buffer(struct urb *urb, gfp_t mem_flags) { - struct dma_aligned_buffer *temp, *kmalloc_ptr; + void *kmalloc_ptr; size_t kmalloc_size; if (urb->num_sgs || urb->sg || @@ -2641,22 +2635,21 @@ !((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; + /* Allocate a buffer with enough padding for original pointer */ + kmalloc_size = urb->transfer_buffer_length + sizeof(void *); kmalloc_ptr = kmalloc(kmalloc_size, mem_flags); if (!kmalloc_ptr) return -ENOMEM; - /* 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; + /* Position our original urb->transfer_buffer pointer to the end */ + memcpy(kmalloc_ptr + urb->transfer_buffer_length, + &urb->transfer_buffer, sizeof(void *)); + if (usb_urb_dir_out(urb)) - memcpy(temp->data, urb->transfer_buffer, + memcpy(kmalloc_ptr, urb->transfer_buffer, urb->transfer_buffer_length); - urb->transfer_buffer = temp->data; + urb->transfer_buffer = kmalloc_ptr; urb->transfer_flags |= URB_ALIGNED_TEMP_BUFFER; -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
