USB: Fix incorrect DMA allocations for local memory pool drivers

PATCH Superseded Fredrik Noring 4 participants 7 messages Nov 30, 2019 – Dec 10, 2019

Subsystems (2)

USB USB mass storage driver

lore

Share & tools

lore

Fredrik Noring

author

2019-11-30 16:59 UTC #0 lore raw

PATCH 1/1 USB: Fix incorrect DMA allocations for local memory pool drivers

Fix commit 7b81cb6bddd2 ("usb: add a HCD_DMA flag instead of guestimating DMA capabilities") where local memory USB drivers erroneously allocate DMA memory instead of pool memory, causing

OHCI Unrecoverable Error, disabled
HC died; cleaning up

The order between hcd_uses_dma() and hcd->localmem_pool is now arranged as in hcd_buffer_alloc() and hcd_buffer_free(), with the test for hcd->localmem_pool placed first.

As an alternative, one might consider adjusting hcd_uses_dma() with

static inline bool hcd_uses_dma(struct usb_hcd *hcd) {
- return IS_ENABLED(CONFIG_HAS_DMA) && (hcd->driver->flags & HCD_DMA);
+ return IS_ENABLED(CONFIG_HAS_DMA) &&
+ (hcd->driver->flags & HCD_DMA) &&
+ (hcd->localmem_pool == NULL);
}

One can also consider unsetting HCD_DMA for local memory pool drivers.

Fixes 7b81cb6bddd2 ("usb: add a HCD_DMA flag instead of guestimating DMA capabilities") where local memory USB drivers

Signed-off-by Fredrik Noring <[email protected]>

---
 drivers/usb/core/hcd.c         | 42 +++++++++++++++++-----------------
 drivers/usb/storage/scsiglue.c |  3 ++-
 2 files changed, 23 insertions(+), 22 deletions(-)

drivers/usb/core/hcd.c +21 ~~-21~~

index f225eaa98ff8..d0f45600b669 100644
@@ -1409,7 +1409,17 @@ int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
 	if (usb_endpoint_xfer_control(&urb->ep->desc)) {
 		if (hcd->self.uses_pio_for_control)
 			return ret;
-		if (hcd_uses_dma(hcd)) {
+		if (hcd->localmem_pool) {
+			ret = hcd_alloc_coherent(
+					urb->dev->bus, mem_flags,
+					&urb->setup_dma,
+					(void **)&urb->setup_packet,
+					sizeof(struct usb_ctrlrequest),
+					DMA_TO_DEVICE);
+			if (ret)
+				return ret;
+			urb->transfer_flags |= URB_SETUP_MAP_LOCAL;
+		} else if (hcd_uses_dma(hcd)) {
 			if (is_vmalloc_addr(urb->setup_packet)) {
 				WARN_ONCE(1, "setup packet is not dma capable\n");
 				return -EAGAIN;

@@ -1427,23 +1437,22 @@ int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
 						urb->setup_dma))
 				return -EAGAIN;
 			urb->transfer_flags |= URB_SETUP_MAP_SINGLE;
-		} else if (hcd->localmem_pool) {
-			ret = hcd_alloc_coherent(
-					urb->dev->bus, mem_flags,
-					&urb->setup_dma,
-					(void **)&urb->setup_packet,
-					sizeof(struct usb_ctrlrequest),
-					DMA_TO_DEVICE);
-			if (ret)
-				return ret;
-			urb->transfer_flags |= URB_SETUP_MAP_LOCAL;
 		}
 	}
 
 	dir = usb_urb_dir_in(urb) ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
 	if (urb->transfer_buffer_length != 0
 	    && !(urb->transfer_flags & URB_NO_TRANSFER_DMA_MAP)) {
-		if (hcd_uses_dma(hcd)) {
+		if (hcd->localmem_pool) {
+			ret = hcd_alloc_coherent(
+					urb->dev->bus, mem_flags,
+					&urb->transfer_dma,
+					&urb->transfer_buffer,
+					urb->transfer_buffer_length,
+					dir);
+			if (ret == 0)
+				urb->transfer_flags |= URB_MAP_LOCAL;
+		} else if (hcd_uses_dma(hcd)) {
 			if (urb->num_sgs) {
 				int n;

@@ -1497,15 +1506,6 @@ int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
 				else
 					urb->transfer_flags |= URB_DMA_MAP_SINGLE;
 			}
-		} else if (hcd->localmem_pool) {
-			ret = hcd_alloc_coherent(
-					urb->dev->bus, mem_flags,
-					&urb->transfer_dma,
-					&urb->transfer_buffer,
-					urb->transfer_buffer_length,
-					dir);
-			if (ret == 0)
-				urb->transfer_flags |= URB_MAP_LOCAL;
 		}
 		if (ret && (urb->transfer_flags & (URB_SETUP_MAP_SINGLE |
 				URB_SETUP_MAP_LOCAL)))

drivers/usb/storage/scsiglue.c +2 -1

index 54a3c8195c96..2adcabe060c5 100644
@@ -135,7 +135,8 @@ static int slave_configure(struct scsi_device *sdev)
 	 * For such controllers we need to make sure the block layer sets
 	 * up bounce buffers in addressable memory.
 	 */
-	if (!hcd_uses_dma(bus_to_hcd(us->pusb_dev->bus)))
+	if (!hcd_uses_dma(bus_to_hcd(us->pusb_dev->bus)) ||
+			(bus_to_hcd(us->pusb_dev->bus)->localmem_pool != NULL))
 		blk_queue_bounce_limit(sdev->request_queue, BLK_BOUNCE_HIGH);
 
 	/*

Signature · 1 line

2.23.0

Christoph Hellwig

Reviewed-by on v1 1/1

2019-12-03 07:39 UTC #1 lore raw

Quoted 21 lines · Fredrik Noring

Fix commit 7b81cb6bddd2 ("usb: add a HCD_DMA flag instead of guestimating DMA capabilities") where local memory USB drivers erroneously allocate DMA memory instead of pool memory, causing
OHCI Unrecoverable Error, disabled
HC died; cleaning up
The order between hcd_uses_dma() and hcd->localmem_pool is now arranged as in hcd_buffer_alloc() and hcd_buffer_free(), with the test for hcd->localmem_pool placed first.
As an alternative, one might consider adjusting hcd_uses_dma() with
static inline bool hcd_uses_dma(struct usb_hcd *hcd) {
(quoted diff) +3 -1
-	return IS_ENABLED(CONFIG_HAS_DMA) && (hcd->driver->flags & HCD_DMA);
+	return IS_ENABLED(CONFIG_HAS_DMA) &&
+		(hcd->driver->flags & HCD_DMA) &&
+		(hcd->localmem_pool == NULL);
 }

Quoted 1 line

One can also consider unsetting HCD_DMA for local memory pool drivers.

That seems like a good idea to me, as the "local DMA pool" really isn't DMA in the traditional sense. The host has to copy data into it by MMIO, and it then is accessed by the device.

But if the usb maintainers prefer your current variant that is ok with me as well:

Reviewed-by Christoph Hellwig <[email protected]>

Fredrik Noring

author

2019-12-03 17:01 UTC #2 lore raw

Hi Christoph,

Quoted 5 lines

Quoted 1 line
One can also consider unsetting HCD_DMA for local memory pool drivers.
That seems like a good idea to me, as the "local DMA pool" really isn't DMA in the traditional sense. The host has to copy data into it by MMIO, and it then is accessed by the device.

Perhaps we can combine several enhancements, given that the local memory pool drivers frequently break with peculiar errors? For example:

1. Arrange localmem_pool and hcd_uses_dma() statements consistently. Inconsistent ordering was a source of this bug.

2. Make localmem_pool and hcd_uses_dma() mutually exclusive. Allocating DMA memory was a source of this bug.

3. Introduce hcd_uses_localmem_pool(), as show below, to have it on the same abstraction level as hcd_uses_dma(). The current localmem_pool pointer tests throughout the code are not quite as readable.

A minor note: The commit sequence

7b81cb6bddd2 ("usb: add a HCD_DMA flag instead of guestimating DMA capabilities") 5d6ff300f011 ("usb/max3421: remove the dummy {un,}map_urb_for_dma methods") bd5defaee872 ("dma-mapping: remove is_device_dma_capable") cdfee5623290 ("driver core: initialize a default DMA mask for platform device")

broke bisection because the first commit 7b81cb6bddd2 crashes with "DMA map on device without dma_mask", that is fixed in the fourth commit cdfee5623290. Too late to do anything about that now, though.

Fredrik

include/linux/usb/hcd.h +5 -0

@@ -423,6 +423,11 @@ static inline bool hcd_periodic_completion_in_progress(struct usb_hcd *hcd,
 	return hcd->high_prio_bh.completing_ep == ep;
 }
 
+static inline bool hcd_uses_localmem_pool(struct usb_hcd *hcd)
+{
+	return hcd->localmem_pool != NULL;
+}
+
 static inline bool hcd_uses_dma(struct usb_hcd *hcd)
 {
 	return IS_ENABLED(CONFIG_HAS_DMA) && (hcd->driver->flags & HCD_DMA);

Johan Hovold

2019-12-03 08:12 UTC #3 lore raw

In reply to Fredrik Noring

Quoted 8 lines · Fredrik Noring

Fix commit 7b81cb6bddd2 ("usb: add a HCD_DMA flag instead of guestimating DMA capabilities") where local memory USB drivers erroneously allocate DMA memory instead of pool memory, causing
OHCI Unrecoverable Error, disabled
HC died; cleaning up
Fixes 7b81cb6bddd2 ("usb: add a HCD_DMA flag instead of guestimating DMA capabilities") where local memory USB drivers

Looks like you copied a bit too much text here for the fixes tag.

Johan

Fredrik Noring

author

2019-12-03 17:01 UTC #4 lore raw

Hi Johan,

Quoted 3 lines

Quoted 1 line
Fixes 7b81cb6bddd2 ("usb: add a HCD_DMA flag instead of guestimating DMA capabilities") where local memory USB drivers
Looks like you copied a bit too much text here for the fixes tag.

Indeed, thanks for noticing!

Fredrik

Greg Kroah-Hartman

maintainer

2019-12-10 11:22 UTC #5 lore raw

In reply to Fredrik Noring

Quoted 23 lines · Fredrik Noring

Fix commit 7b81cb6bddd2 ("usb: add a HCD_DMA flag instead of guestimating DMA capabilities") where local memory USB drivers erroneously allocate DMA memory instead of pool memory, causing
OHCI Unrecoverable Error, disabled
HC died; cleaning up
The order between hcd_uses_dma() and hcd->localmem_pool is now arranged as in hcd_buffer_alloc() and hcd_buffer_free(), with the test for hcd->localmem_pool placed first.
As an alternative, one might consider adjusting hcd_uses_dma() with
static inline bool hcd_uses_dma(struct usb_hcd *hcd) {
(quoted diff) +3 -1
-	return IS_ENABLED(CONFIG_HAS_DMA) && (hcd->driver->flags & HCD_DMA);
+	return IS_ENABLED(CONFIG_HAS_DMA) &&
+		(hcd->driver->flags & HCD_DMA) &&
+		(hcd->localmem_pool == NULL);
 }
One can also consider unsetting HCD_DMA for local memory pool drivers.

Quoted 8 lines

Fixes 7b81cb6bddd2 ("usb: add a HCD_DMA flag instead of guestimating DMA capabilities")
Cc stable <[email protected]>
Signed-off-by Fredrik Noring <[email protected]>
Reviewed-by Christoph Hellwig <[email protected]>
---
 drivers/usb/core/hcd.c         | 42 +++++++++++++++++-----------------
 drivers/usb/storage/scsiglue.c |  3 ++-
 2 files changed, 23 insertions(+), 22 deletions(-)

This patch doesn't apply against 5.5-rc1, can you refresh it and resend?

thanks,

greg k-h

Fredrik Noring

author

2019-12-10 17:29 UTC #6 lore raw

PATCH v2 1/1 USB: Fix incorrect DMA allocations for local memory pool drivers

Fix commit 7b81cb6bddd2 ("usb: add a HCD_DMA flag instead of guestimating DMA capabilities") where local memory USB drivers erroneously allocate DMA memory instead of pool memory, causing

OHCI Unrecoverable Error, disabled
HC died; cleaning up

The order between hcd_uses_dma() and hcd->localmem_pool is now arranged as in hcd_buffer_alloc() and hcd_buffer_free(), with the test for hcd->localmem_pool placed first.

As an alternative, one might consider adjusting hcd_uses_dma() with

One can also consider unsetting HCD_DMA for local memory pool drivers.

Fixes 7b81cb6bddd2 ("usb: add a HCD_DMA flag instead of guestimating DMA capabilities")

Cc stable <[email protected]>

Signed-off-by Fredrik Noring <[email protected]>

---
Hi Greg,

Quoted 1 line

This patch doesn't apply against 5.5-rc1, can you refresh it and resend?

Sure, please find updated patch v2 attached below.

Changes in v2:
- Update to v5.5-rc1 resolving conflict with commit b3d53f5fce5d ("usb: core: Remove redundant vmap checks")
- Fix "Fixes:" line
- Add "Cc: stable <[email protected]>"

---
 drivers/usb/core/hcd.c         | 42 +++++++++++++++++-----------------
 drivers/usb/storage/scsiglue.c |  3 ++-
 2 files changed, 23 insertions(+), 22 deletions(-)

drivers/usb/core/hcd.c +21 ~~-21~~

index 281568d464f9..aa45840d8273 100644
@@ -1409,7 +1409,17 @@ int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
 	if (usb_endpoint_xfer_control(&urb->ep->desc)) {
 		if (hcd->self.uses_pio_for_control)
 			return ret;
-		if (hcd_uses_dma(hcd)) {
+		if (hcd->localmem_pool) {
+			ret = hcd_alloc_coherent(
+					urb->dev->bus, mem_flags,
+					&urb->setup_dma,
+					(void **)&urb->setup_packet,
+					sizeof(struct usb_ctrlrequest),
+					DMA_TO_DEVICE);
+			if (ret)
+				return ret;
+			urb->transfer_flags |= URB_SETUP_MAP_LOCAL;
+		} else if (hcd_uses_dma(hcd)) {
 			if (object_is_on_stack(urb->setup_packet)) {
 				WARN_ONCE(1, "setup packet is on stack\n");
 				return -EAGAIN;

@@ -1424,23 +1434,22 @@ int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
 						urb->setup_dma))
 				return -EAGAIN;
 			urb->transfer_flags |= URB_SETUP_MAP_SINGLE;
-		} else if (hcd->localmem_pool) {
-			ret = hcd_alloc_coherent(
-					urb->dev->bus, mem_flags,
-					&urb->setup_dma,
-					(void **)&urb->setup_packet,
-					sizeof(struct usb_ctrlrequest),
-					DMA_TO_DEVICE);
-			if (ret)
-				return ret;
-			urb->transfer_flags |= URB_SETUP_MAP_LOCAL;
 		}
 	}
 
 	dir = usb_urb_dir_in(urb) ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
 	if (urb->transfer_buffer_length != 0
 	    && !(urb->transfer_flags & URB_NO_TRANSFER_DMA_MAP)) {
-		if (hcd_uses_dma(hcd)) {
+		if (hcd->localmem_pool) {
+			ret = hcd_alloc_coherent(
+					urb->dev->bus, mem_flags,
+					&urb->transfer_dma,
+					&urb->transfer_buffer,
+					urb->transfer_buffer_length,
+					dir);
+			if (ret == 0)
+				urb->transfer_flags |= URB_MAP_LOCAL;
+		} else if (hcd_uses_dma(hcd)) {
 			if (urb->num_sgs) {
 				int n;

@@ -1491,15 +1500,6 @@ int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
 				else
 					urb->transfer_flags |= URB_DMA_MAP_SINGLE;
 			}
-		} else if (hcd->localmem_pool) {
-			ret = hcd_alloc_coherent(
-					urb->dev->bus, mem_flags,
-					&urb->transfer_dma,
-					&urb->transfer_buffer,
-					urb->transfer_buffer_length,
-					dir);
-			if (ret == 0)
-				urb->transfer_flags |= URB_MAP_LOCAL;
 		}
 		if (ret && (urb->transfer_flags & (URB_SETUP_MAP_SINGLE |
 				URB_SETUP_MAP_LOCAL)))

drivers/usb/storage/scsiglue.c +2 -1

index 66a4dcbbb1fc..f4c2359abb1b 100644
@@ -135,7 +135,8 @@ static int slave_configure(struct scsi_device *sdev)
 	 * For such controllers we need to make sure the block layer sets
 	 * up bounce buffers in addressable memory.
 	 */
-	if (!hcd_uses_dma(bus_to_hcd(us->pusb_dev->bus)))
+	if (!hcd_uses_dma(bus_to_hcd(us->pusb_dev->bus)) ||
+			(bus_to_hcd(us->pusb_dev->bus)->localmem_pool != NULL))
 		blk_queue_bounce_limit(sdev->request_queue, BLK_BOUNCE_HIGH);
 
 	/*

Signature · 1 line

2.23.0