[PATCH] USB: Fix incorrect DMA allocations for local memory pool drivers

[PATCH] USB: Fix incorrect DMA allocations for local memory pool drivers

[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)
 {
-	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 <noring@nocrew.org>
---
 drivers/usb/core/hcd.c         | 42 +++++++++++++++++-----------------
 drivers/usb/storage/scsiglue.c |  3 ++-
 2 files changed, 23 insertions(+), 22 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index f225eaa98ff8..d0f45600b669 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -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)))
diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c
index 54a3c8195c96..2adcabe060c5 100644
--- a/drivers/usb/storage/scsiglue.c
+++ b/drivers/usb/storage/scsiglue.c
@@ -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);
 
 	/*
-- 
2.23.0

Re: [PATCH] USB: Fix incorrect DMA allocations for local memory pool drivers

[Christoph Hellwig]

On Sat, Nov 30, 2019 at 05:50:55PM +0100, Fredrik Noring wrote:
> 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.

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 <hch@lst.de>

Re: [PATCH] USB: Fix incorrect DMA allocations for local memory pool drivers

[Johan Hovold]

On Sat, Nov 30, 2019 at 05:50:55PM +0100, Fredrik Noring wrote:
> 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

Re: [PATCH] USB: Fix incorrect DMA allocations for local memory pool drivers

[Fredrik Noring]

Hi Christoph,

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

diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -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);

Re: [PATCH] USB: Fix incorrect DMA allocations for local memory pool drivers

[Fredrik Noring]

Hi Johan,

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

Re: [PATCH] USB: Fix incorrect DMA allocations for local memory pool drivers

[Greg Kroah-Hartman]

On Sat, Nov 30, 2019 at 05:50:55PM +0100, Fredrik Noring wrote:
> 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")
> Cc: stable <stable@vger.kernel.org>
> Signed-off-by: Fredrik Noring <noring@nocrew.org>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
>  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

[PATCH v2] USB: Fix incorrect DMA allocations for local memory pool drivers

[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)
 {
-	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")
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Fredrik Noring <noring@nocrew.org>
---
Hi Greg,

> 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 <stable@vger.kernel.org>"
---
 drivers/usb/core/hcd.c         | 42 +++++++++++++++++-----------------
 drivers/usb/storage/scsiglue.c |  3 ++-
 2 files changed, 23 insertions(+), 22 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 281568d464f9..aa45840d8273 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -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)))
diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c
index 66a4dcbbb1fc..f4c2359abb1b 100644
--- a/drivers/usb/storage/scsiglue.c
+++ b/drivers/usb/storage/scsiglue.c
@@ -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);
 
 	/*
-- 
2.23.0