* [PATCH] USB: Fix incorrect DMA allocations for local memory pool drivers
@ 2019-11-30 16:50 Fredrik Noring
2019-12-03 7:39 ` Christoph Hellwig
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Fredrik Noring @ 2019-11-30 16:50 UTC (permalink / raw)
To: Christoph Hellwig, linux-usb; +Cc: Greg Kroah-Hartman
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
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] USB: Fix incorrect DMA allocations for local memory pool drivers
2019-11-30 16:50 [PATCH] USB: Fix incorrect DMA allocations for local memory pool drivers Fredrik Noring
@ 2019-12-03 7:39 ` Christoph Hellwig
2019-12-03 16:54 ` Fredrik Noring
2019-12-03 8:12 ` Johan Hovold
2019-12-10 11:22 ` Greg Kroah-Hartman
2 siblings, 1 reply; 7+ messages in thread
From: Christoph Hellwig @ 2019-12-03 7:39 UTC (permalink / raw)
To: Fredrik Noring; +Cc: Christoph Hellwig, linux-usb, 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.
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>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] USB: Fix incorrect DMA allocations for local memory pool drivers
2019-12-03 7:39 ` Christoph Hellwig
@ 2019-12-03 16:54 ` Fredrik Noring
0 siblings, 0 replies; 7+ messages in thread
From: Fredrik Noring @ 2019-12-03 16:54 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-usb, Greg Kroah-Hartman
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);
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] USB: Fix incorrect DMA allocations for local memory pool drivers
2019-11-30 16:50 [PATCH] USB: Fix incorrect DMA allocations for local memory pool drivers Fredrik Noring
2019-12-03 7:39 ` Christoph Hellwig
@ 2019-12-03 8:12 ` Johan Hovold
2019-12-03 16:55 ` Fredrik Noring
2019-12-10 11:22 ` Greg Kroah-Hartman
2 siblings, 1 reply; 7+ messages in thread
From: Johan Hovold @ 2019-12-03 8:12 UTC (permalink / raw)
To: Fredrik Noring; +Cc: Christoph Hellwig, linux-usb, 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
> 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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] USB: Fix incorrect DMA allocations for local memory pool drivers
2019-12-03 8:12 ` Johan Hovold
@ 2019-12-03 16:55 ` Fredrik Noring
0 siblings, 0 replies; 7+ messages in thread
From: Fredrik Noring @ 2019-12-03 16:55 UTC (permalink / raw)
To: Johan Hovold; +Cc: Christoph Hellwig, linux-usb, Greg Kroah-Hartman
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] USB: Fix incorrect DMA allocations for local memory pool drivers
2019-11-30 16:50 [PATCH] USB: Fix incorrect DMA allocations for local memory pool drivers Fredrik Noring
2019-12-03 7:39 ` Christoph Hellwig
2019-12-03 8:12 ` Johan Hovold
@ 2019-12-10 11:22 ` Greg Kroah-Hartman
2019-12-10 17:29 ` [PATCH v2] " Fredrik Noring
2 siblings, 1 reply; 7+ messages in thread
From: Greg Kroah-Hartman @ 2019-12-10 11:22 UTC (permalink / raw)
To: Fredrik Noring; +Cc: Christoph Hellwig, linux-usb
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
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH v2] USB: Fix incorrect DMA allocations for local memory pool drivers
2019-12-10 11:22 ` Greg Kroah-Hartman
@ 2019-12-10 17:29 ` Fredrik Noring
0 siblings, 0 replies; 7+ messages in thread
From: Fredrik Noring @ 2019-12-10 17:29 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: Christoph Hellwig, linux-usb
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-12-10 17:29 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-30 16:50 [PATCH] USB: Fix incorrect DMA allocations for local memory pool drivers Fredrik Noring
2019-12-03 7:39 ` Christoph Hellwig
2019-12-03 16:54 ` Fredrik Noring
2019-12-03 8:12 ` Johan Hovold
2019-12-03 16:55 ` Fredrik Noring
2019-12-10 11:22 ` Greg Kroah-Hartman
2019-12-10 17:29 ` [PATCH v2] " Fredrik Noring
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).