Re: WARN_ON(irqs_disabled()) in dma_free_attrs?

2018-03-15 Thread Robin Murphy

On 15/03/18 07:58, Christoph Hellwig wrote:

On Wed, Mar 14, 2018 at 05:43:46PM +, 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: WARN_ON(irqs_disabled()) in dma_free_attrs?

2018-03-15 Thread Christoph Hellwig
On Wed, Mar 14, 2018 at 05:43:46PM +, 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: WARN_ON(irqs_disabled()) in dma_free_attrs?

2018-03-14 Thread Robin Murphy

On 13/03/18 13:17, Christoph Hellwig wrote:

On Tue, Mar 13, 2018 at 12:11:49PM +, 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: WARN_ON(irqs_disabled()) in dma_free_attrs?

2018-03-13 Thread Christoph Hellwig
On Tue, Mar 13, 2018 at 12:11:49PM +, 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: WARN_ON(irqs_disabled()) in dma_free_attrs?

2018-03-13 Thread 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: WARN_ON(irqs_disabled()) in dma_free_attrs?

2018-03-11 Thread 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

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;
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: WARN_ON(irqs_disabled()) in dma_free_attrs?

2018-03-05 Thread Christoph Hellwig
On Sat, Mar 03, 2018 at 07:19:06PM +0100, Fredrik Noring wrote:
> Christoph, Alan,
> 
> > If it is allocating / freeing this memory all the time in the hot path
> > it should really use a dma pool (see include/ilinux/dmapool.h).
> > The dma coherent APIs aren't really built for being called in the
> > hot path.
> 
> hcd_buffer_free uses a combination of dma pools and dma coherent APIs:
> 
>   ...
>   for (i = 0; i < HCD_BUFFER_POOLS; i++) {
>   if (size <= pool_max[i]) {
>   dma_pool_free(hcd->pool[i], addr, dma);
>   return;
>   }
>   }
>   dma_free_coherent(hcd->self.sysdev, size, addr, dma);
> 
> Alan, can dma_free_coherent be delayed to a point when IRQs are enabled?

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: WARN_ON(irqs_disabled()) in dma_free_attrs?

2018-03-04 Thread 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: WARN_ON(irqs_disabled()) in dma_free_attrs?

2018-03-04 Thread 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: GW4.15.0+ #831
Workqueue: events request_firmware_work_func
Stack : 0009 01d7 0001 81714028 81714000 8006161c 80519af4 000f
805e3b08 01d7 80519a68 81cd3ad4 81714000 10058c00 81cd3aa8 80587340
  03f8 0007  03f8  
0040 0008  81ccd288   803036a8 8053e854
0009 01d7 0001 81714028 0008 ffc7  805e
...
Call Trace:
[<2318fb9b>] show_stack+0x74/0x104
[] __warn+0x118/0x120
[<01359d10>] warn_slowpath_fmt+0x30/0x3c
[] 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]
[] 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]
[] ath9k_htc_probe_device+0x38c/0xa2c [ath9k_htc]
[] ath9k_htc_hw_init+0x20/0x4c [ath9k_htc]
[<6500cd86>] ath9k_hif_usb_firmware_cb+0x780/0x854 [ath9k_htc]
[] 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

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

...
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  1700 0004 4 <
81f9ab00 299095041 S Bi:1:003:1 -150 8192 <
81f9a800 299095229 C Ci:1:002:0 0 4 = 2d00
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 

Re: WARN_ON(irqs_disabled()) in dma_free_attrs?

2018-03-04 Thread Alan Stern
On Sat, 3 Mar 2018, Fredrik Noring wrote:

> Christoph, Alan,
> 
> > If it is allocating / freeing this memory all the time in the hot path
> > it should really use a dma pool (see include/ilinux/dmapool.h).
> > The dma coherent APIs aren't really built for being called in the
> > hot path.
> 
> hcd_buffer_free uses a combination of dma pools and dma coherent APIs:
> 
>   ...
>   for (i = 0; i < HCD_BUFFER_POOLS; i++) {
>   if (size <= pool_max[i]) {
>   dma_pool_free(hcd->pool[i], addr, dma);
>   return;
>   }
>   }
>   dma_free_coherent(hcd->self.sysdev, size, addr, dma);
> 
> 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.

Alan Stern

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


Re: WARN_ON(irqs_disabled()) in dma_free_attrs?

2018-03-03 Thread Fredrik Noring
Christoph, Alan,

> If it is allocating / freeing this memory all the time in the hot path
> it should really use a dma pool (see include/ilinux/dmapool.h).
> The dma coherent APIs aren't really built for being called in the
> hot path.

hcd_buffer_free uses a combination of dma pools and dma coherent APIs:

...
for (i = 0; i < HCD_BUFFER_POOLS; i++) {
if (size <= pool_max[i]) {
dma_pool_free(hcd->pool[i], addr, dma);
return;
}
}
dma_free_coherent(hcd->self.sysdev, size, addr, dma);

Alan, can dma_free_coherent be delayed to a point when IRQs are enabled?

[ Links to previous messages on this topic are listed below. ]

Fredrik

https://www.spinics.net/lists/linux-usb/msg162817.html
https://lists.linuxfoundation.org/pipermail/iommu/2018-March/026334.html
https://lists.linuxfoundation.org/pipermail/iommu/2018-March/026335.html
https://lists.linuxfoundation.org/pipermail/iommu/2018-March/026336.html
https://lists.linuxfoundation.org/pipermail/iommu/2018-March/026337.html
https://lists.linuxfoundation.org/pipermail/iommu/2018-March/026338.html
https://lists.linuxfoundation.org/pipermail/iommu/2018-March/026339.html
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html