LinuxLists.cc - [PATCH v4 0/3] prerequisites for device reserved local mem rework

2019-05-16 11:50:44

Subject: [PATCH v4 0/3] prerequisites for device reserved local mem rework

2019-05-16 11:51:48

Subject: [PATCH v4 2/3] usb: host: ohci-sm501: init genalloc for local memory

From: Laurentiu Tudor <[email protected]>

In preparation for dropping the existing "coherent" dma mem declaration
APIs, replace the current dma_declare_coherent_memory() based mechanism
with the creation of a genalloc pool that will be used in the OHCI
subsystem as replacement for the DMA APIs.

For context, see thread here: https://lkml.org/lkml/2019/4/22/357

Signed-off-by: Laurentiu Tudor <[email protected]>
---
drivers/usb/host/ohci-sm501.c | 68 +++++++++++++++++++++--------------
1 file changed, 41 insertions(+), 27 deletions(-)

diff --git a/drivers/usb/host/ohci-sm501.c b/drivers/usb/host/ohci-sm501.c
index c26228c25f99..1a25f5396e3e 100644
--- a/drivers/usb/host/ohci-sm501.c
+++ b/drivers/usb/host/ohci-sm501.c
@@ -16,6 +16,7 @@
#include <linux/jiffies.h>
#include <linux/platform_device.h>
#include <linux/dma-mapping.h>
+#include <linux/genalloc.h>
#include <linux/sm501.h>
#include <linux/sm501-regs.h>

@@ -92,6 +93,7 @@ static int ohci_hcd_sm501_drv_probe(struct platform_device *pdev)
struct resource *res, *mem;
int retval, irq;
struct usb_hcd *hcd = NULL;
+ void __iomem *local_mem;

irq = retval = platform_get_irq(pdev, 0);
if (retval < 0)
@@ -110,40 +112,18 @@ static int ohci_hcd_sm501_drv_probe(struct platform_device *pdev)
goto err0;
}

- /* The sm501 chip is equipped with local memory that may be used
- * by on-chip devices such as the video controller and the usb host.
- * This driver uses dma_declare_coherent_memory() to make sure
- * usb allocations with dma_alloc_coherent() allocate from
- * this local memory. The dma_handle returned by dma_alloc_coherent()
- * will be an offset starting from 0 for the first local memory byte.
- *
- * So as long as data is allocated using dma_alloc_coherent() all is
- * fine. This is however not always the case - buffers may be allocated
- * using kmalloc() - so the usb core needs to be told that it must copy
- * data into our local memory if the buffers happen to be placed in
- * regular memory. The HCD_LOCAL_MEM flag does just that.
- */
-
- retval = dma_declare_coherent_memory(dev, mem->start,
- mem->start - mem->parent->start,
- resource_size(mem));
- if (retval) {
- dev_err(dev, "cannot declare coherent memory\n");
- goto err1;
- }
-
/* allocate, reserve and remap resources for registers */
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
if (res == NULL) {
dev_err(dev, "no resource definition for registers\n");
retval = -ENOENT;
- goto err2;
+ goto err1;
}

hcd = usb_create_hcd(driver, &pdev->dev, dev_name(&pdev->dev));
if (!hcd) {
retval = -ENOMEM;
- goto err2;
+ goto err1;
}

hcd->rsrc_start = res->start;
@@ -164,6 +144,43 @@ static int ohci_hcd_sm501_drv_probe(struct platform_device *pdev)

ohci_hcd_init(hcd_to_ohci(hcd));

+ /* The sm501 chip is equipped with local memory that may be used
+ * by on-chip devices such as the video controller and the usb host.
+ * This driver uses genalloc so that usb allocations with
+ * gen_pool_dma_alloc() allocate from this local memory. The dma_handle
+ * returned by gen_pool_dma_alloc() will be an offset starting from 0
+ * for the first local memory byte.
+ *
+ * So as long as data is allocated using gen_pool_dma_alloc() all is
+ * fine. This is however not always the case - buffers may be allocated
+ * using kmalloc() - so the usb core needs to be told that it must copy
+ * data into our local memory if the buffers happen to be placed in
+ * regular memory. The HCD_LOCAL_MEM flag does just that.
+ */
+
+ hcd->localmem_pool = devm_gen_pool_create(dev, PAGE_SHIFT,
+ dev_to_node(dev),
+ "ohci-sm501");
+ if (IS_ERR(hcd->localmem_pool)) {
+ retval = PTR_ERR(hcd->localmem_pool);
+ goto err5;
+ }
+
+ local_mem = devm_ioremap(dev, mem->start, resource_size(mem));
+ if (!local_mem) {
+ retval = -ENOMEM;
+ goto err5;
+ }
+
+ retval = gen_pool_add_virt(hcd->localmem_pool,
+ (unsigned long)local_mem,
+ mem->start - mem->parent->start,
+ resource_size(mem),
+ dev_to_node(dev));
+ if (retval < 0) {
+ dev_err(dev, "failed to add to pool: %d\n", retval);
+ goto err5;
+ }
retval = usb_add_hcd(hcd, irq, IRQF_SHARED);
if (retval)
goto err5;
@@ -181,8 +198,6 @@ static int ohci_hcd_sm501_drv_probe(struct platform_device *pdev)
release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
err3:
usb_put_hcd(hcd);
-err2:
- dma_release_declared_memory(dev);
err1:
release_mem_region(mem->start, resource_size(mem));
err0:
@@ -197,7 +212,6 @@ static int ohci_hcd_sm501_drv_remove(struct platform_device *pdev)
usb_remove_hcd(hcd);
release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
usb_put_hcd(hcd);
- dma_release_declared_memory(&pdev->dev);
mem = platform_get_resource(pdev, IORESOURCE_MEM, 1);
if (mem)
release_mem_region(mem->start, resource_size(mem));
--
2.17.1

2019-05-16 11:51:53

by Laurentiu Tudor

[permalink] [raw]

Subject: [PATCH v4 3/3] usb: host: ohci-tmio: init genalloc for local memory

2019-05-16 11:53:03

by Laurentiu Tudor

[permalink] [raw]

Subject: [PATCH v4 1/3] USB: use genalloc for USB HCs with local memory

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 | 15 +++++++++++----
drivers/usb/host/ohci-hcd.c | 23 ++++++++++++++++++-----
include/linux/usb/hcd.h | 3 +++
3 files changed, 32 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/core/buffer.c b/drivers/usb/core/buffer.c
index f641342cdec0..22a8f3f5679b 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>

@@ -124,10 +125,12 @@ void *hcd_buffer_alloc(
if (size == 0)
return NULL;

+ if (hcd->driver->flags & HCD_LOCAL_MEM)
+ return gen_pool_dma_alloc(hcd->localmem_pool, size, dma);
+
/* some USB hosts just use PIO */
if (!IS_ENABLED(CONFIG_HAS_DMA) ||
- (!is_device_dma_capable(bus->sysdev) &&
- !(hcd->driver->flags & HCD_LOCAL_MEM))) {
+ !is_device_dma_capable(bus->sysdev)) {
*dma = ~(dma_addr_t) 0;
return kmalloc(size, mem_flags);
}
@@ -152,9 +155,13 @@ void hcd_buffer_free(
if (!addr)
return;

+ if (hcd->driver->flags & HCD_LOCAL_MEM) {
+ gen_pool_free(hcd->localmem_pool, (unsigned long)addr, size);
+ return;
+ }
+
if (!IS_ENABLED(CONFIG_HAS_DMA) ||
- (!is_device_dma_capable(bus->sysdev) &&
- !(hcd->driver->flags & HCD_LOCAL_MEM))) {
+ !is_device_dma_capable(bus->sysdev)) {
kfree(addr);
return;
}
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 bb57b5af4700..1b0fc3cebc08 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -216,6 +216,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

2019-05-21 08:18:25

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [PATCH v4 1/3] USB: use genalloc for USB HCs with local memory

On Thu, May 16, 2019 at 02:47:19PM +0300, [email protected] wrote:
> 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 | 15 +++++++++++----
> drivers/usb/host/ohci-hcd.c | 23 ++++++++++++++++++-----
> include/linux/usb/hcd.h | 3 +++
> 3 files changed, 32 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/usb/core/buffer.c b/drivers/usb/core/buffer.c
> index f641342cdec0..22a8f3f5679b 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>
>
> @@ -124,10 +125,12 @@ void *hcd_buffer_alloc(
> if (size == 0)
> return NULL;
>
> + if (hcd->driver->flags & HCD_LOCAL_MEM)
> + return gen_pool_dma_alloc(hcd->localmem_pool, size, dma);

Does this patch now break things? hcd->localmem_pool at this point in
time is NULL, so this call will fail. There's no chance for any host
controller driver to actually set up this pool in this patch, so is
bisection broken?

I think you fix this up in later patches, right?

And if so, why do we even need HCD_LOCAL_MEM anymore? Can't we just
test for the presence of hcd->localmem_pool in order to determine which
allocation method to use?

thanks,

greg k-h

2019-05-21 10:31:36

by Christoph Hellwig

[permalink] [raw]

Subject: Re: [PATCH v4 1/3] USB: use genalloc for USB HCs with local memory

On Tue, May 21, 2019 at 10:16:57AM +0200, Greg KH wrote:
> > + if (hcd->driver->flags & HCD_LOCAL_MEM)
> > + return gen_pool_dma_alloc(hcd->localmem_pool, size, dma);
>
> Does this patch now break things? hcd->localmem_pool at this point in
> time is NULL, so this call will fail. There's no chance for any host
> controller driver to actually set up this pool in this patch, so is
> bisection broken?
>
> I think you fix this up in later patches, right?
>
> And if so, why do we even need HCD_LOCAL_MEM anymore? Can't we just
> test for the presence of hcd->localmem_pool in order to determine which
> allocation method to use?

True. And that also sound like a good bisectability strategy:

- first add hcd->localmem_pool and test for it
- convert drivers over to it
- remove HCD_LOCAL_MEM

2019-05-21 10:42:35

by Christoph Hellwig

[permalink] [raw]

Subject: Re: [PATCH v4 2/3] usb: host: ohci-sm501: init genalloc for local memory

On Thu, May 16, 2019 at 02:47:20PM +0300, [email protected] wrote:
> + hcd->localmem_pool = devm_gen_pool_create(dev, PAGE_SHIFT,
> + dev_to_node(dev),
> + "ohci-sm501");
> + if (IS_ERR(hcd->localmem_pool)) {
> + retval = PTR_ERR(hcd->localmem_pool);
> + goto err5;
> + }
> +
> + local_mem = devm_ioremap(dev, mem->start, resource_size(mem));
> + if (!local_mem) {
> + retval = -ENOMEM;
> + goto err5;
> + }
> +
> + retval = gen_pool_add_virt(hcd->localmem_pool,
> + (unsigned long)local_mem,
> + mem->start - mem->parent->start,
> + resource_size(mem),
> + dev_to_node(dev));

I wonder if having a helper for these operations would be useful,
explaining what we do here. That is create a pool for a resource,
where the virtual address is the ioremap of said resource. We
could also make that a managed API so that you can get rid of the
cleanup path.

2019-05-21 11:07:26

by Laurentiu Tudor

[permalink] [raw]

Subject: Re: [PATCH v4 1/3] USB: use genalloc for USB HCs with local memory

On 21.05.2019 11:16, Greg KH wrote:
> On Thu, May 16, 2019 at 02:47:19PM +0300, [email protected] wrote:
>> 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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%2Flkml%2F2019%2F4%2F22%2F357&data=02%7C01%7Claurentiu.tudor%40nxp.com%7Cf5242fb28d154ff9653208d6ddc4b41c%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636940234237524499&sdata=KEEUP1KH%2BaraWcVKogeYBzrauh%2FFTzGjSxjk%2BuNozjA%3D&reserved=0
>>
>> Signed-off-by: Laurentiu Tudor <[email protected]>
>> ---
>> drivers/usb/core/buffer.c | 15 +++++++++++----
>> drivers/usb/host/ohci-hcd.c | 23 ++++++++++++++++++-----
>> include/linux/usb/hcd.h | 3 +++
>> 3 files changed, 32 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/usb/core/buffer.c b/drivers/usb/core/buffer.c
>> index f641342cdec0..22a8f3f5679b 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>
>>
>> @@ -124,10 +125,12 @@ void *hcd_buffer_alloc(
>> if (size == 0)
>> return NULL;
>>
>> + if (hcd->driver->flags & HCD_LOCAL_MEM)
>> + return gen_pool_dma_alloc(hcd->localmem_pool, size, dma);
>
> Does this patch now break things? hcd->localmem_pool at this point in
> time is NULL, so this call will fail. There's no chance for any host
> controller driver to actually set up this pool in this patch, so is
> bisection broken?

Unfortunately, yes. I could lump the patches together but I think
Christoph suggestion is much better.

> I think you fix this up in later patches, right?

Correct. The last 2 patches update the driver.

> And if so, why do we even need HCD_LOCAL_MEM anymore? Can't we just
> test for the presence of hcd->localmem_pool in order to determine which
> allocation method to use?

Sure. There are a few more places that need updates but no big deal.

---
Best Regards, Laurentiu

2019-05-21 11:09:41

by Laurentiu Tudor

[permalink] [raw]

Subject: Re: [PATCH v4 2/3] usb: host: ohci-sm501: init genalloc for local memory

On 21.05.2019 13:39, Christoph Hellwig wrote:
> On Thu, May 16, 2019 at 02:47:20PM +0300, [email protected] wrote:
>> + hcd->localmem_pool = devm_gen_pool_create(dev, PAGE_SHIFT,
>> + dev_to_node(dev),
>> + "ohci-sm501");
>> + if (IS_ERR(hcd->localmem_pool)) {
>> + retval = PTR_ERR(hcd->localmem_pool);
>> + goto err5;
>> + }
>> +
>> + local_mem = devm_ioremap(dev, mem->start, resource_size(mem));
>> + if (!local_mem) {
>> + retval = -ENOMEM;
>> + goto err5;
>> + }
>> +
>> + retval = gen_pool_add_virt(hcd->localmem_pool,
>> + (unsigned long)local_mem,
>> + mem->start - mem->parent->start,
>> + resource_size(mem),
>> + dev_to_node(dev));
>
> I wonder if having a helper for these operations would be useful,
> explaining what we do here. That is create a pool for a resource,
> where the virtual address is the ioremap of said resource. We
> could also make that a managed API so that you can get rid of the
> cleanup path.

This is close to what I've already prepared in the next spin. It's a new
usb hcd api so it's not as abstract as your idea. I think we can discuss
on it after I'll send it.

---
Best Regards, Laurentiu

2019-05-21 11:18:35

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [PATCH v4 1/3] USB: use genalloc for USB HCs with local memory

On Tue, May 21, 2019 at 11:04:12AM +0000, Laurentiu Tudor wrote:
>
>
> On 21.05.2019 11:16, Greg KH wrote:
> > On Thu, May 16, 2019 at 02:47:19PM +0300, [email protected] wrote:
> >> 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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%2Flkml%2F2019%2F4%2F22%2F357&data=02%7C01%7Claurentiu.tudor%40nxp.com%7Cf5242fb28d154ff9653208d6ddc4b41c%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636940234237524499&sdata=KEEUP1KH%2BaraWcVKogeYBzrauh%2FFTzGjSxjk%2BuNozjA%3D&reserved=0
> >>
> >> Signed-off-by: Laurentiu Tudor <[email protected]>
> >> ---
> >> drivers/usb/core/buffer.c | 15 +++++++++++----
> >> drivers/usb/host/ohci-hcd.c | 23 ++++++++++++++++++-----
> >> include/linux/usb/hcd.h | 3 +++
> >> 3 files changed, 32 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/usb/core/buffer.c b/drivers/usb/core/buffer.c
> >> index f641342cdec0..22a8f3f5679b 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>
> >>
> >> @@ -124,10 +125,12 @@ void *hcd_buffer_alloc(
> >> if (size == 0)
> >> return NULL;
> >>
> >> + if (hcd->driver->flags & HCD_LOCAL_MEM)
> >> + return gen_pool_dma_alloc(hcd->localmem_pool, size, dma);
> >
> > Does this patch now break things? hcd->localmem_pool at this point in
> > time is NULL, so this call will fail. There's no chance for any host
> > controller driver to actually set up this pool in this patch, so is
> > bisection broken?
>
> Unfortunately, yes. I could lump the patches together but I think
> Christoph suggestion is much better.

I do too, can you redo these patches to work in that manner please?

thanks,

greg k-h