WARN_ON(irqs_disabled()) in dma_free_attrs?

WARN_ON(irqs_disabled()) in dma_free_attrs?

[Fredrik Noring]

Hi Alan Stern,

> > Alan, can dma_free_coherent be delayed to a point when IRQs are enabled?
> 
> Yes, subject to the usual concerns about not being delayed for too 
> long.  Also, some HCDs are highly memory-constrained.  I don't know if 
> they use this API, but if they do then delaying a free could result in 
> not enough memory being available when an allocation is needed.

The PS2 HCD in this case is limited to 256 KiB and memory allocation
failures can be observed frequently in USB traces (102 subsequent failures
in the trace below involving mass storage transactions, for example). Is
there any smartness to the allocations? My impression is that the USB core
loops until it gets what it wants, and then happily resumes. Does it busy
wait?

The RT3070 wireless USB device driver, for example, has hardcoded buffer
limits that exceed the total amount of available memory. It refuses to
accept devices unless adjusted in the source (as in the patch below), after
which it works nicely.

Other USB device drivers such as the one for the AR9271 wireless device
trigger endlessly repeating kernel warnings claiming

	BOGUS urb xfer, pipe 1 != type 3

as shown in this kernel backtrace:

------------[ cut here ]------------
WARNING: CPU: 0 PID: 15 at drivers/usb/core/urb.c:471 usb_submit_urb+0x280/0x528
usb 1-1: BOGUS urb xfer, pipe 1 != type 3
Modules linked in: ath9k_htc ath9k_common ath9k_hw ath mac80211 cfg80211 usbmon
CPU: 0 PID: 15 Comm: kworker/0:1 Tainted: G        W        4.15.0+ #831
Workqueue: events request_firmware_work_func
Stack : 00000009 000001d7 00000001 81714028 81714000 8006161c 80519af4 0000000f
        805e3b08 000001d7 80519a68 81cd3ad4 81714000 10058c00 81cd3aa8 80587340
        00000000 00000000 000003f8 00000007 00000000 000003f8 00000000 00000000
        00000040 00000008 00000000 81ccd288 00000000 00000000 803036a8 8053e854
        00000009 000001d7 00000001 81714028 00000008 ffffffc7 00000000 805e0000
        ...
Call Trace:
[<2318fb9b>] show_stack+0x74/0x104
[<cc3fea80>] __warn+0x118/0x120
[<01359d10>] warn_slowpath_fmt+0x30/0x3c
[<bfb4c964>] usb_submit_urb+0x280/0x528
[<89eb7b59>] hif_usb_send+0x3b0/0x3e0 [ath9k_htc]
[<51446a9c>] ath9k_wmi_cmd+0x194/0x228 [ath9k_htc]
[<2119badc>] ath9k_regread+0x5c/0x88 [ath9k_htc]
[<f56172da>] ath9k_hw_wait+0xa4/0xc0 [ath9k_hw]
[<368b3c01>] ath9k_hw_set_reset_reg+0x23c/0x288 [ath9k_hw]
[<28807ea6>] ath9k_hw_init+0x3e8/0xa24 [ath9k_hw]
[<a3367fed>] ath9k_htc_probe_device+0x38c/0xa2c [ath9k_htc]
[<da4d1bbe>] ath9k_htc_hw_init+0x20/0x4c [ath9k_htc]
[<6500cd86>] ath9k_hif_usb_firmware_cb+0x780/0x854 [ath9k_htc]
[<b4730ad0>] request_firmware_work_func+0x40/0x70
[<54f8834a>] process_one_work+0x1f4/0x358
[<23b84d12>] worker_thread+0x354/0x4b8
[<583bf61b>] kthread+0x134/0x13c
[<213b5c9e>] ret_from_kernel_thread+0x14/0x1c
---[ end trace 2a97c69dce30b650 ]---

I don't know if this is related to memory limitations or some other problem
though.

Fredrik


...
81f58280 299082198 C Bi:1:003:1 0 13 = 55534253 ...
81f58280 299082510 S Bo:1:003:2 -150 31 = 55534243 ...
81f58280 299083200 C Bo:1:003:2 0 31 >
81f9aa00 299083298 S Bi:1:003:1 -150 65536 <
81f9a800 299093849 S Ci:1:002:0 s c0 07 0000 1700 0004 4 <
81f9ab00 299095041 S Bi:1:003:1 -150 8192 <
81f9a800 299095229 C Ci:1:002:0 0 4 = 2d000000
81f9a680 299096808 S Bi:1:003:1 -150 49152 <
81f9a680 299096838 E Bi:1:003:1 -12 0
81f9a680 299097868 S Bi:1:003:1 -150 49152 <
81f9a680 299097896 E Bi:1:003:1 -12 0
81f9a680 299098894 S Bi:1:003:1 -150 49152 <
... 97 allocation failures repeated ...
81f9a680 299151156 S Bi:1:003:1 -150 49152 <
81f9a680 299151166 E Bi:1:003:1 -12 0
81f9a680 299151179 S Bi:1:003:1 -150 49152 <
81f9a680 299151190 E Bi:1:003:1 -12 0
81f9a680 299151203 S Bi:1:003:1 -150 49152 <
81f9a680 299151216 E Bi:1:003:1 -12 0
81f9a680 299151231 S Bi:1:003:1 -150 49152 <
81f9a680 299159909 S Bi:1:003:1 -150 49152 <
81f9a680 299160814 S Bi:1:003:1 -150 49152 <
81f9a680 299161732 S Bi:1:003:1 -150 49152 <
81f9a680 299162644 S Bi:1:003:1 -150 49152 <
81f9a680 299163574 S Bi:1:003:1 -150 49152 <
81f9a680 299164490 S Bi:1:003:1 -150 49152 <
81f9aa00 299172593 C Bi:1:003:1 0 65536 = f7b07b39 ...
81f9ab00 299175259 C Bi:1:003:1 0 8192 = 26e2c254 ...
81f9a680 299177824 S Bi:1:003:1 -150 49152 <
81f9a680 299239323 C Bi:1:003:1 0 49152 = a21c1219 ...
81f58280 299239459 S Bi:1:003:1 -150 13 <
81f58280 299241264 C Bi:1:003:1 0 13 = 55534253 ...
81f58280 299241580 S Bo:1:003:2 -150 31 = 55534243 ...
81f58280 299242263 C Bo:1:003:2 0 31 >
...
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--- a/drivers/net/wireless/ralink/rt2x00/rt2800usb.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2800usb.c
@@ -871,7 +871,7 @@ static void rt2800usb_queue_init(struct data_queue *queue)
 
 	switch (queue->qid) {
 	case QID_RX:
-		queue->limit = 128;
+		queue->limit = 8;
 		queue->data_size = AGGREGATION_SIZE;
 		queue->desc_size = RXINFO_DESC_SIZE;
 		queue->winfo_size = rxwi_size;
@@ -882,7 +882,7 @@ static void rt2800usb_queue_init(struct data_queue *queue)
 	case QID_AC_VI:
 	case QID_AC_BE:
 	case QID_AC_BK:
-		queue->limit = 16;
+		queue->limit = 8;
 		queue->data_size = AGGREGATION_SIZE;
 		queue->desc_size = TXINFO_DESC_SIZE;
 		queue->winfo_size = txwi_size;

WARN_ON(irqs_disabled()) in dma_free_attrs?

[Alan Stern]

On Sun, 4 Mar 2018, Fredrik Noring wrote:

> Hi Alan Stern,
> 
> > > Alan, can dma_free_coherent be delayed to a point when IRQs are enabled?
> > 
> > Yes, subject to the usual concerns about not being delayed for too 
> > long.  Also, some HCDs are highly memory-constrained.  I don't know if 
> > they use this API, but if they do then delaying a free could result in 
> > not enough memory being available when an allocation is needed.
> 
> The PS2 HCD in this case is limited to 256 KiB and memory allocation
> failures can be observed frequently in USB traces (102 subsequent failures
> in the trace below involving mass storage transactions, for example). Is
> there any smartness to the allocations?

Not as far as I know.

>  My impression is that the USB core
> loops until it gets what it wants, and then happily resumes. Does it busy
> wait?

No, it doesn't loop and it doesn't busy-wait.  It just fails if memory 
can't be allocated immediately.  Maybe the higher-level driver has a 
retry loop of some sort.

> The RT3070 wireless USB device driver, for example, has hardcoded buffer
> limits that exceed the total amount of available memory. It refuses to
> accept devices unless adjusted in the source (as in the patch below), after
> which it works nicely.
> 
> Other USB device drivers such as the one for the AR9271 wireless device
> trigger endlessly repeating kernel warnings claiming
> 
> 	BOGUS urb xfer, pipe 1 != type 3
> 
> as shown in this kernel backtrace:

...

> I don't know if this is related to memory limitations or some other problem
> though.

Another problem.  This message indicates there's a bug in the ath9k
driver; it says that the driver submitted an interrupt URB for a bulk
endpoint.

Alan Stern
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

WARN_ON(irqs_disabled()) in dma_free_attrs?

[Fredrik Noring]

Hi Christoph,

> The point is that you should always use a pool, period.
> dma_alloc*/dma_free* are fundamentally expensive operations on my
> architectures, so if you call them from a fast path you are doing
> something wrong.

The author's comment in commit b3476675320e "usb: dma bounce buffer support"
seems to suggest that the greatest concern is space efficiency as opposed to
speed. I tried to evaluate strict pool allocations, similar to the patch
below, but that didn't turn out as I expected.

I chose a 64 KiB pool maximum since it has been the largest requested size I
have observed in USB traces (which may not hold in general, of course). This
change caused the USB mass storage driver to get stuck in some kind of memory
deadlock, with endless busy-looping on 64 KiB allocation failures.

I also tried a progression of pool sizes including nonpowers of two, for
example 12288, to make better use of the 256 KiB memory capacity. However,
the following part of dma_pool_create in linux/mm/dmapool.c is somewhat
puzzling:

	if (!boundary)
		boundary = allocation;
	else if ((boundary < size) || (boundary & (boundary - 1)))
		return NULL;

Is the boundary variable required to be a power of two only when it is
explicitly given as nonzero?

Fredrik
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

diff --git a/drivers/usb/core/buffer.c b/drivers/usb/core/buffer.c
index 77eef8acff94..8cc8fbc91c76 100644
--- a/drivers/usb/core/buffer.c
+++ b/drivers/usb/core/buffer.c
@@ -26,7 +26,7 @@
 
 /* FIXME tune these based on pool statistics ... */
 static size_t pool_max[HCD_BUFFER_POOLS] = {
-	32, 128, 512, 2048,
+	32, 128, 256, 512, 1024, 2048, 4096, 8192, 16384, 32768, 65536
 };
 
 void __init usb_init_pool_max(void)
@@ -76,7 +76,7 @@ int hcd_buffer_create(struct usb_hcd *hcd)
 			continue;
 		snprintf(name, sizeof(name), "buffer-%d", size);
 		hcd->pool[i] = dma_pool_create(name, hcd->self.sysdev,
-				size, size, 0);
+				size, min_t(size_t, 4096, size), 0);
 		if (!hcd->pool[i]) {
 			hcd_buffer_destroy(hcd);
 			return -ENOMEM;
@@ -140,7 +140,7 @@ void *hcd_buffer_alloc(
 		if (size <= pool_max[i])
 			return dma_pool_alloc(hcd->pool[i], mem_flags, dma);
 	}
-	return dma_alloc_coherent(hcd->self.sysdev, size, dma, mem_flags);
+	return NULL;
 }
 
 void hcd_buffer_free(
@@ -169,5 +169,5 @@ void hcd_buffer_free(
 			return;
 		}
 	}
-	dma_free_coherent(hcd->self.sysdev, size, addr, dma);
+	BUG();		/* The buffer could not have been allocated. */
 }
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 176900528822..2075f1e22e32 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -189,7 +189,7 @@ struct usb_hcd {
 	struct usb_hcd		*primary_hcd;
 
 
-#define HCD_BUFFER_POOLS	4
+#define HCD_BUFFER_POOLS	11
 	struct dma_pool		*pool[HCD_BUFFER_POOLS];
 
 	int			state;

WARN_ON(irqs_disabled()) in dma_free_attrs?

[Robin Murphy]

On 11/03/18 18:01, Fredrik Noring wrote:
> Hi Christoph,
> 
>> The point is that you should always use a pool, period.
>> dma_alloc*/dma_free* are fundamentally expensive operations on my
>> architectures, so if you call them from a fast path you are doing
>> something wrong.
> 
> The author's comment in commit b3476675320e "usb: dma bounce buffer support"
> seems to suggest that the greatest concern is space efficiency as opposed to
> speed. I tried to evaluate strict pool allocations, similar to the patch
> below, but that didn't turn out as I expected.
> 
> I chose a 64 KiB pool maximum since it has been the largest requested size I
> have observed in USB traces (which may not hold in general, of course). This
> change caused the USB mass storage driver to get stuck in some kind of memory
> deadlock, with endless busy-looping on 64 KiB allocation failures.
> 
> I also tried a progression of pool sizes including nonpowers of two, for
> example 12288, to make better use of the 256 KiB memory capacity. However,
> the following part of dma_pool_create in linux/mm/dmapool.c is somewhat
> puzzling:
> 
>          if (!boundary)
>                  boundary = allocation;
>          else if ((boundary < size) || (boundary & (boundary - 1)))
>                  return NULL;
> 
> Is the boundary variable required to be a power of two only when it is
> explicitly given as nonzero?

I would think so; realistically, the notion of non-power-of-two 
boundaries doesn't make a great deal of sense. IIRC, XHCI has a 64KB 
boundary limitation, but [OE]HCI don't (as far as I could see from the 
specs).

> 
> Fredrik
> 
> diff --git a/drivers/usb/core/buffer.c b/drivers/usb/core/buffer.c
> index 77eef8acff94..8cc8fbc91c76 100644
> --- a/drivers/usb/core/buffer.c
> +++ b/drivers/usb/core/buffer.c
> @@ -26,7 +26,7 @@
> 
>   /* FIXME tune these based on pool statistics ... */
>   static size_t pool_max[HCD_BUFFER_POOLS] = {
> -       32, 128, 512, 2048,
> +       32, 128, 256, 512, 1024, 2048, 4096, 8192, 16384, 32768, 65536
>   };
> 
>   void __init usb_init_pool_max(void)
> @@ -76,7 +76,7 @@ int hcd_buffer_create(struct usb_hcd *hcd)
>                          continue;
>                  snprintf(name, sizeof(name), "buffer-%d", size);
>                  hcd->pool[i] = dma_pool_create(name, hcd->self.sysdev,
> -                               size, size, 0);
> +                               size, min_t(size_t, 4096, size), 0);
>                  if (!hcd->pool[i]) {
>                          hcd_buffer_destroy(hcd);
>                          return -ENOMEM;
> @@ -140,7 +140,7 @@ void *hcd_buffer_alloc(
>                  if (size <= pool_max[i])
>                          return dma_pool_alloc(hcd->pool[i], mem_flags, dma);
>          }
> -       return dma_alloc_coherent(hcd->self.sysdev, size, dma, mem_flags);

Taking a step back, though, provided the original rationale about 
dma_declare_coherent_memory() is still valid, I wonder if we should 
simply permit the USB code to call dma_{alloc,free}_from_dev_coherent() 
directly here instead of being "good" and indirecting through the 
top-level DMA API (which is the part which leads to problems). Given 
that it's a specific DMA bounce buffer implementation within a core API, 
not just any old driver code, I personally would consider that reasonable.

Robin.

> +       return NULL;
>   }
> 
>   void hcd_buffer_free(
> @@ -169,5 +169,5 @@ void hcd_buffer_free(
>                          return;
>                  }
>          }
> -       dma_free_coherent(hcd->self.sysdev, size, addr, dma);
> +       BUG();          /* The buffer could not have been allocated. */
>   }
> diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
> index 176900528822..2075f1e22e32 100644
> --- a/include/linux/usb/hcd.h
> +++ b/include/linux/usb/hcd.h
> @@ -189,7 +189,7 @@ struct usb_hcd {
>          struct usb_hcd          *primary_hcd;
> 
> 
> -#define HCD_BUFFER_POOLS       4
> +#define HCD_BUFFER_POOLS       11
>          struct dma_pool         *pool[HCD_BUFFER_POOLS];
> 
>          int                     state;
>
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

WARN_ON(irqs_disabled()) in dma_free_attrs?

[Christoph Hellwig]

On Tue, Mar 13, 2018 at 12:11:49PM +0000, Robin Murphy wrote:
> Taking a step back, though, provided the original rationale about 
> dma_declare_coherent_memory() is still valid, I wonder if we should simply 
> permit the USB code to call dma_{alloc,free}_from_dev_coherent() directly 
> here instead of being "good" and indirecting through the top-level DMA API 
> (which is the part which leads to problems). Given that it's a specific DMA 
> bounce buffer implementation within a core API, not just any old driver 
> code, I personally would consider that reasonable.

Looking back I don't really understand why we even indirect the "classic"
per-device dma_declare_coherent_memory use case through the DMA API.

It seems like a pretty different use case to me.  In the USB case we
also have the following additional twist in that it doesn't even need
the mapping to be coherent.

So maybe for now the quick fix is to move the sleep check as suggested
earlier in this thread, but in the long run we probably need to do some
major rework of how dma_declare_coherent_memory and friends work.
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

WARN_ON(irqs_disabled()) in dma_free_attrs?

[Robin Murphy]

On 13/03/18 13:17, Christoph Hellwig wrote:
> On Tue, Mar 13, 2018 at 12:11:49PM +0000, Robin Murphy wrote:
>> Taking a step back, though, provided the original rationale about
>> dma_declare_coherent_memory() is still valid, I wonder if we should simply
>> permit the USB code to call dma_{alloc,free}_from_dev_coherent() directly
>> here instead of being "good" and indirecting through the top-level DMA API
>> (which is the part which leads to problems). Given that it's a specific DMA
>> bounce buffer implementation within a core API, not just any old driver
>> code, I personally would consider that reasonable.
> 
> Looking back I don't really understand why we even indirect the "classic"
> per-device dma_declare_coherent_memory use case through the DMA API.

It certainly makes sense for devices which can exist in both 
shared-memory and device-local-memory configurations, so the driver 
doesn't have to care about the details (particularly on mobile SoCs 
where the 'local' memory might just be a chunk of system RAM reserved by 
the bootloader, and it's just a matter of different use-cases on 
identical hardware).

> It seems like a pretty different use case to me.  In the USB case we
> also have the following additional twist in that it doesn't even need
> the mapping to be coherent.

I'm pretty sure it does (in the sense that it needs to ensure the arch 
code makes the mapping non-cacheable), otherwise I can't see how the 
bouncing could work properly. I think the last bit of the comment above 
hcd_alloc_coherent() is a bit misleading.

> So maybe for now the quick fix is to move the sleep check as suggested
> earlier in this thread, but in the long run we probably need to do some
> major rework of how dma_declare_coherent_memory and friends work.

Maybe; I do think the specific hcd_alloc_coherent() case could still be 
fixed within the scope of the existing code, but it's not quite as clean 
and straightforward as I first thought, and the practical impact of 
tweaking the WARN should be effectively zero despite the theoretical 
edge cases it opens up. Do you want me to write it up as a proper patch?

Robin.
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

WARN_ON(irqs_disabled()) in dma_free_attrs?

[Christoph Hellwig]

On Wed, Mar 14, 2018 at 05:43:46PM +0000, Robin Murphy wrote:
>> Looking back I don't really understand why we even indirect the "classic"
>> per-device dma_declare_coherent_memory use case through the DMA API.
>
> It certainly makes sense for devices which can exist in both shared-memory 
> and device-local-memory configurations, so the driver doesn't have to care 
> about the details (particularly on mobile SoCs where the 'local' memory 
> might just be a chunk of system RAM reserved by the bootloader, and it's 
> just a matter of different use-cases on identical hardware).

Well, the "classic" case for me is memory buffers in the device.  Setting
some memory aside, either in a global pool as now done for arm-nommu
or even per-device as on some ARM SOCs is different indeed.

As far as I can tell the few devices that use 'local' memory always
use that.

>> It seems like a pretty different use case to me.  In the USB case we
>> also have the following additional twist in that it doesn't even need
>> the mapping to be coherent.
>
> I'm pretty sure it does (in the sense that it needs to ensure the arch code 
> makes the mapping non-cacheable), otherwise I can't see how the bouncing 
> could work properly. I think the last bit of the comment above 
> hcd_alloc_coherent() is a bit misleading.

Well, if it isn't marked non-cacheable we'd have to do dma_cache_sync
operations for it.  Which would probably still be faster than non-cacheable
mappings.

>> So maybe for now the quick fix is to move the sleep check as suggested
>> earlier in this thread, but in the long run we probably need to do some
>> major rework of how dma_declare_coherent_memory and friends work.
>
> Maybe; I do think the specific hcd_alloc_coherent() case could still be 
> fixed within the scope of the existing code, but it's not quite as clean 
> and straightforward as I first thought, and the practical impact of 
> tweaking the WARN should be effectively zero despite the theoretical edge 
> cases it opens up. Do you want me to write it up as a proper patch?

Yes.  Including a proper comment on why the might_sleep is placed there.

My mid-term plan was to actually remove the gfp flags argument from
the dma alloc routines as is creates more confusion than it helps.
I guess this means we'll at least need to introduce a DMA_ATTR_NON_BLOCK
or similar flag instead then unfortunately.
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

WARN_ON(irqs_disabled()) in dma_free_attrs?

[Robin Murphy]

On 15/03/18 07:58, Christoph Hellwig wrote:
> On Wed, Mar 14, 2018 at 05:43:46PM +0000, Robin Murphy wrote:
>>> Looking back I don't really understand why we even indirect the "classic"
>>> per-device dma_declare_coherent_memory use case through the DMA API.
>>
>> It certainly makes sense for devices which can exist in both shared-memory
>> and device-local-memory configurations, so the driver doesn't have to care
>> about the details (particularly on mobile SoCs where the 'local' memory
>> might just be a chunk of system RAM reserved by the bootloader, and it's
>> just a matter of different use-cases on identical hardware).
> 
> Well, the "classic" case for me is memory buffers in the device.  Setting
> some memory aside, either in a global pool as now done for arm-nommu
> or even per-device as on some ARM SOCs is different indeed.
> 
> As far as I can tell the few devices that use 'local' memory always
> use that.

I was thinking mostly of GPUs, where the same IP core might be available 
in a discrete PCIe chip with its own GDDR controller and on-board local 
RAM, and also in an APU/SoC-type configuration reliant on the CPU memory 
bus. But I guess in practice those drivers are already well beyond the 
generic DMA API and into complicated explicitly-managed stuff like TTM.

>>> It seems like a pretty different use case to me.  In the USB case we
>>> also have the following additional twist in that it doesn't even need
>>> the mapping to be coherent.
>>
>> I'm pretty sure it does (in the sense that it needs to ensure the arch code
>> makes the mapping non-cacheable), otherwise I can't see how the bouncing
>> could work properly. I think the last bit of the comment above
>> hcd_alloc_coherent() is a bit misleading.
> 
> Well, if it isn't marked non-cacheable we'd have to do dma_cache_sync
> operations for it.  Which would probably still be faster than non-cacheable
> mappings.

Ah, I see, we crossed wires there - the *current* implementation 
definitely requires a coherent mapping, but in general you're right that 
there are other ways to solve this particular problem which wouldn't. 
This case really wants to be able to overload the streaming map/unmap 
calls to automatically bounce through device-local memory, but I'm sure 
that's not worth the complexity or effort in general :)

>>> So maybe for now the quick fix is to move the sleep check as suggested
>>> earlier in this thread, but in the long run we probably need to do some
>>> major rework of how dma_declare_coherent_memory and friends work.
>>
>> Maybe; I do think the specific hcd_alloc_coherent() case could still be
>> fixed within the scope of the existing code, but it's not quite as clean
>> and straightforward as I first thought, and the practical impact of
>> tweaking the WARN should be effectively zero despite the theoretical edge
>> cases it opens up. Do you want me to write it up as a proper patch?
> 
> Yes.  Including a proper comment on why the might_sleep is placed there.

OK, will do.

> My mid-term plan was to actually remove the gfp flags argument from
> the dma alloc routines as is creates more confusion than it helps.
> I guess this means we'll at least need to introduce a DMA_ATTR_NON_BLOCK
> or similar flag instead then unfortunately.

At least we already have DMA_ATTR_NO_WARN, which could be implemented 
consistently to clean up the existing __GFP_NOWARN users.

Robin.
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

WARN_ON(irqs_disabled()) in dma_free_attrs?

[Fredrik Noring]

Hi Robin, Christoph,

On Thu, Mar 15, 2018 at 01:11:03PM +0000, Robin Murphy wrote:
> On 15/03/18 07:58, Christoph Hellwig wrote:
> > On Wed, Mar 14, 2018 at 05:43:46PM +0000, Robin Murphy wrote:
> > > > So maybe for now the quick fix is to move the sleep check as suggested
> > > > earlier in this thread, but in the long run we probably need to do some
> > > > major rework of how dma_declare_coherent_memory and friends work.
> > > 
> > > Maybe; I do think the specific hcd_alloc_coherent() case could still be
> > > fixed within the scope of the existing code, but it's not quite as clean
> > > and straightforward as I first thought, and the practical impact of
> > > tweaking the WARN should be effectively zero despite the theoretical edge
> > > cases it opens up. Do you want me to write it up as a proper patch?
> > 
> > Yes.  Including a proper comment on why the might_sleep is placed there.
> 
> OK, will do.

How is to going with moving the sleep check?

Another regression triggered by the OHCI turned up in v4.16 with commit
205e1b7f51e4af26 "dma-mapping: warn when there is no coherent_dma_mask"
by Christoph Hellwig:

------------[ cut here ]------------
WARNING: CPU: 0 PID: 62 at ./include/linux/dma-mapping.h:516 ohci_setup+0x41c/0x424 [ohci_hcd]
Modules linked in: ohci_ps2(+) ohci_hcd usbcore usb_common sd_mod iop iop_fio iop_module iop_memory sif
CPU: 0 PID: 62 Comm: modprobe Not tainted 4.16.0+ #1533
Stack : 00000000 00000000 80747392 00000037 81c6eb0c 804f32e7 80493b24 0000003e
        80743498 00000204 00000001 c01c0000 802a2fa0 10058c00 81ea5a68 804facc0
        00000000 00000000 80740000 00000007 00000000 00000060 00000000 00000000
        3a6d6d6f 00000000 0000005f 646f6d20 80000000 00000000 c01e66e8 c01e813c
        00000009 00000204 00000001 c01c0000 00000018 80278fe0 0007579f 00000001
        ...
Call Trace:
[<8001d6e4>] show_stack+0x74/0x104
[<800323a8>] __warn+0x118/0x120
[<8003246c>] warn_slowpath_null+0x44/0x58
[<c01e66e8>] ohci_setup+0x41c/0x424 [ohci_hcd]
[<c01f209c>] ohci_ps2_reset+0x30/0x70 [ohci_ps2]
[<c01a8aec>] usb_add_hcd+0x2d4/0x89c [usbcore]
[<c01f2360>] ohci_hcd_ps2_probe+0x284/0x2a4 [ohci_ps2]
[<802a8a74>] platform_drv_probe+0x2c/0x68
[<802a70b4>] driver_probe_device+0x22c/0x2e4
[<802a71f0>] __driver_attach+0x84/0xc8
[<802a53fc>] bus_for_each_dev+0x60/0x90
[<802a6580>] bus_add_driver+0x1b8/0x200
[<802a7980>] driver_register+0xc0/0x100
[<800106bc>] do_one_initcall+0x17c/0x190
[<800841f4>] do_init_module+0x74/0x1f0
[<80082f30>] load_module+0x1680/0x2044
[<80083adc>] SyS_finit_module+0xa0/0xb8
[<8002190c>] syscall_common+0x34/0x58
---[ end trace e71738b5fa6bf9aa ]---

Fredrik
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html