Re: [PATCH] usb: dwc2: extend treatment for incomplete transfer

["Antti Seppälä" <a.seppala@gmail.com>]

[-- 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