From: Laurentiu Tudor <[email protected]>
For HCs that have local memory, replace the current DMA API usage
with a genalloc generic allocator to manage the mappings for these
devices.
This is in preparation for dropping the existing "coherent" dma
mem declaration APIs. Current implementation was relying on a short
circuit in the DMA API that in the end, was acting as an allocator
for these type of devices.
For context, see thread here: https://lkml.org/lkml/2019/4/22/357
Signed-off-by: Laurentiu Tudor <[email protected]>
---
drivers/usb/core/buffer.c | 12 +++++++++++-
drivers/usb/host/ohci-hcd.c | 23 ++++++++++++++++++-----
include/linux/usb/hcd.h | 3 +++
3 files changed, 32 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/core/buffer.c b/drivers/usb/core/buffer.c
index f641342cdec0..5729801e82e0 100644
--- a/drivers/usb/core/buffer.c
+++ b/drivers/usb/core/buffer.c
@@ -16,6 +16,7 @@
#include <linux/io.h>
#include <linux/dma-mapping.h>
#include <linux/dmapool.h>
+#include <linux/genalloc.h>
#include <linux/usb.h>
#include <linux/usb/hcd.h>
@@ -136,6 +137,10 @@ void *hcd_buffer_alloc(
if (size <= pool_max[i])
return dma_pool_alloc(hcd->pool[i], mem_flags, dma);
}
+
+ if (hcd->driver->flags & HCD_LOCAL_MEM)
+ return gen_pool_dma_alloc(hcd->localmem_pool, size, dma);
+
return dma_alloc_coherent(hcd->self.sysdev, size, dma, mem_flags);
}
@@ -165,5 +170,10 @@ void hcd_buffer_free(
return;
}
}
- dma_free_coherent(hcd->self.sysdev, size, addr, dma);
+
+ if (hcd->driver->flags & HCD_LOCAL_MEM)
+ gen_pool_free(hcd->localmem_pool, (unsigned long)addr,
+ size);
+ else
+ dma_free_coherent(hcd->self.sysdev, size, addr, dma);
}
diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c
index 210181fd98d2..f9462c372943 100644
--- a/drivers/usb/host/ohci-hcd.c
+++ b/drivers/usb/host/ohci-hcd.c
@@ -40,6 +40,7 @@
#include <linux/dmapool.h>
#include <linux/workqueue.h>
#include <linux/debugfs.h>
+#include <linux/genalloc.h>
#include <asm/io.h>
#include <asm/irq.h>
@@ -505,8 +506,15 @@ static int ohci_init (struct ohci_hcd *ohci)
timer_setup(&ohci->io_watchdog, io_watchdog_func, 0);
ohci->prev_frame_no = IO_WATCHDOG_OFF;
- ohci->hcca = dma_alloc_coherent (hcd->self.controller,
- sizeof(*ohci->hcca), &ohci->hcca_dma, GFP_KERNEL);
+ if (hcd->driver->flags & HCD_LOCAL_MEM)
+ ohci->hcca = gen_pool_dma_alloc(hcd->localmem_pool,
+ sizeof(*ohci->hcca),
+ &ohci->hcca_dma);
+ else
+ ohci->hcca = dma_alloc_coherent(hcd->self.controller,
+ sizeof(*ohci->hcca),
+ &ohci->hcca_dma,
+ GFP_KERNEL);
if (!ohci->hcca)
return -ENOMEM;
@@ -990,9 +998,14 @@ static void ohci_stop (struct usb_hcd *hcd)
remove_debug_files (ohci);
ohci_mem_cleanup (ohci);
if (ohci->hcca) {
- dma_free_coherent (hcd->self.controller,
- sizeof *ohci->hcca,
- ohci->hcca, ohci->hcca_dma);
+ if (hcd->driver->flags & HCD_LOCAL_MEM)
+ gen_pool_free(hcd->localmem_pool,
+ (unsigned long)ohci->hcca,
+ sizeof(*ohci->hcca));
+ else
+ dma_free_coherent(hcd->self.controller,
+ sizeof(*ohci->hcca),
+ ohci->hcca, ohci->hcca_dma);
ohci->hcca = NULL;
ohci->hcca_dma = 0;
}
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 695931b03684..0fee81ef5d52 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -215,6 +215,9 @@ struct usb_hcd {
#define HC_IS_RUNNING(state) ((state) & __ACTIVE)
#define HC_IS_SUSPENDED(state) ((state) & __SUSPEND)
+ /* allocator for HCs having local memory */
+ struct gen_pool *localmem_pool;
+
/* more shared queuing code would be good; it should support
* smarter scheduling, handle transaction translators, etc;
* input size of periodic table to an interrupt scheduler.
--
2.17.1
> @@ -136,6 +137,10 @@ void *hcd_buffer_alloc(
> if (size <= pool_max[i])
> return dma_pool_alloc(hcd->pool[i], mem_flags, dma);
> }
> +
> + if (hcd->driver->flags & HCD_LOCAL_MEM)
> + return gen_pool_dma_alloc(hcd->localmem_pool, size, dma);
I think this check needs to be before the above code to use the dma
pools, as we should always use the HCD local memory. Probably all the
way up just below the size == 0 check, that way we can also remove the
other HCD_LOCAL_MEM check.
> @@ -165,5 +170,10 @@ void hcd_buffer_free(
> return;
> }
> }
> - dma_free_coherent(hcd->self.sysdev, size, addr, dma);
> +
> + if (hcd->driver->flags & HCD_LOCAL_MEM)
> + gen_pool_free(hcd->localmem_pool, (unsigned long)addr,
> + size);
> + else
> + dma_free_coherent(hcd->self.sysdev, size, addr, dma);
Same here.
> @@ -505,8 +506,15 @@ static int ohci_init (struct ohci_hcd *ohci)
> timer_setup(&ohci->io_watchdog, io_watchdog_func, 0);
> ohci->prev_frame_no = IO_WATCHDOG_OFF;
>
> - ohci->hcca = dma_alloc_coherent (hcd->self.controller,
> - sizeof(*ohci->hcca), &ohci->hcca_dma, GFP_KERNEL);
> + if (hcd->driver->flags & HCD_LOCAL_MEM)
> + ohci->hcca = gen_pool_dma_alloc(hcd->localmem_pool,
> + sizeof(*ohci->hcca),
> + &ohci->hcca_dma);
> + else
> + ohci->hcca = dma_alloc_coherent(hcd->self.controller,
> + sizeof(*ohci->hcca),
> + &ohci->hcca_dma,
> + GFP_KERNEL);
I wonder if we could just use hcd_buffer_alloc/free here, althought
that would require them to be exported. I'll leave that decision to
the relevant maintainers, though.
Except for this the series looks exactly what I had envisioned to
get rid of the device local dma_declare_coherent use case, thanks!
On 14.05.2019 17:42, Christoph Hellwig wrote:
>> @@ -136,6 +137,10 @@ void *hcd_buffer_alloc(
>> if (size <= pool_max[i])
>> return dma_pool_alloc(hcd->pool[i], mem_flags, dma);
>> }
>> +
>> + if (hcd->driver->flags & HCD_LOCAL_MEM)
>> + return gen_pool_dma_alloc(hcd->localmem_pool, size, dma);
>
> I think this check needs to be before the above code to use the dma
> pools, as we should always use the HCD local memory. Probably all the
> way up just below the size == 0 check, that way we can also remove the
> other HCD_LOCAL_MEM check.
Alright.
>> @@ -165,5 +170,10 @@ void hcd_buffer_free(
>> return;
>> }
>> }
>> - dma_free_coherent(hcd->self.sysdev, size, addr, dma);
>> +
>> + if (hcd->driver->flags & HCD_LOCAL_MEM)
>> + gen_pool_free(hcd->localmem_pool, (unsigned long)addr,
>> + size);
>> + else
>> + dma_free_coherent(hcd->self.sysdev, size, addr, dma);
>
> Same here.
Ok.
>> @@ -505,8 +506,15 @@ static int ohci_init (struct ohci_hcd *ohci)
>> timer_setup(&ohci->io_watchdog, io_watchdog_func, 0);
>> ohci->prev_frame_no = IO_WATCHDOG_OFF;
>>
>> - ohci->hcca = dma_alloc_coherent (hcd->self.controller,
>> - sizeof(*ohci->hcca), &ohci->hcca_dma, GFP_KERNEL);
>> + if (hcd->driver->flags & HCD_LOCAL_MEM)
>> + ohci->hcca = gen_pool_dma_alloc(hcd->localmem_pool,
>> + sizeof(*ohci->hcca),
>> + &ohci->hcca_dma);
>> + else
>> + ohci->hcca = dma_alloc_coherent(hcd->self.controller,
>> + sizeof(*ohci->hcca),
>> + &ohci->hcca_dma,
>> + GFP_KERNEL);
>
> I wonder if we could just use hcd_buffer_alloc/free here, althought
> that would require them to be exported. I'll leave that decision to
> the relevant maintainers, though.
>
> Except for this the series looks exactly what I had envisioned to
> get rid of the device local dma_declare_coherent use case, thanks!
Glad I could help. On the remoteproc_virtio.c case, I had a cursory look
and found out that the dma_declare_coherent_memory() usage was
introduced quite recently, by this patch:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=086d08725d34c6b3333db710344ae9c4fdafb2d5
---
Best Regards, Laurentiu
On Wed, May 15, 2019 at 09:57:12AM +0000, Laurentiu Tudor wrote:
> Glad I could help. On the remoteproc_virtio.c case, I had a cursory look
> and found out that the dma_declare_coherent_memory() usage was
> introduced quite recently, by this patch:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=086d08725d34c6b3333db710344ae9c4fdafb2d5
Yes. I took a look at it to, and while it isn't exactly a clean
usage of the API it at least declares system memory and not a resource.
So it doesn't really affect our plan.