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

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

[laurentiu.tudor]

From: Laurentiu Tudor <laurentiu.tudor@nxp.com>

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.

Only compiled tested, so any volunteers willing to test are most welcome.

Thank you!

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

Changes in v4:
 - created mapping for local mem
 - fix genalloc misuse

Changes in v3:
 - rearranged calls into genalloc simplifying the code a bit (Christoph)
 - dropped RFC prefix

Changes in v2:
 - use genalloc also in core usb (based on comment from Robin)
 - moved genpool decl to usb_hcd to be accesible from core usb

Laurentiu Tudor (3):
  USB: use genalloc for USB HCs with local memory
  usb: host: ohci-sm501: init genalloc for local memory
  usb: host: ohci-tmio: init genalloc for local memory

 drivers/usb/core/buffer.c     | 15 +++++---
 drivers/usb/host/ohci-hcd.c   | 23 +++++++++---
 drivers/usb/host/ohci-sm501.c | 66 +++++++++++++++++++++--------------
 drivers/usb/host/ohci-tmio.c  | 32 ++++++++++++-----
 include/linux/usb/hcd.h       |  3 ++
 5 files changed, 95 insertions(+), 44 deletions(-)

-- 
2.17.1

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

[laurentiu.tudor]

From: Laurentiu Tudor <laurentiu.tudor@nxp.com>

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 <laurentiu.tudor@nxp.com>
---
 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

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

[laurentiu.tudor]

From: Laurentiu Tudor <laurentiu.tudor@nxp.com>

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 <laurentiu.tudor@nxp.com>
---
 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

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

[laurentiu.tudor]

From: Laurentiu Tudor <laurentiu.tudor@nxp.com>

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 <laurentiu.tudor@nxp.com>
---
 drivers/usb/host/ohci-tmio.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/host/ohci-tmio.c b/drivers/usb/host/ohci-tmio.c
index f88a0370659f..1c39099b9870 100644
--- a/drivers/usb/host/ohci-tmio.c
+++ b/drivers/usb/host/ohci-tmio.c
@@ -30,6 +30,7 @@
 #include <linux/mfd/core.h>
 #include <linux/mfd/tmio.h>
 #include <linux/dma-mapping.h>
+#include <linux/genalloc.h>
 
 /*-------------------------------------------------------------------------*/
 
@@ -188,6 +189,7 @@ static int ohci_hcd_tmio_drv_probe(struct platform_device *dev)
 	struct resource *config = platform_get_resource(dev, IORESOURCE_MEM, 1);
 	struct resource *sram = platform_get_resource(dev, IORESOURCE_MEM, 2);
 	int irq = platform_get_irq(dev, 0);
+	void __iomem *local_mem;
 	struct tmio_hcd *tmio;
 	struct ohci_hcd *ohci;
 	struct usb_hcd *hcd;
@@ -224,11 +226,6 @@ static int ohci_hcd_tmio_drv_probe(struct platform_device *dev)
 		goto err_ioremap_regs;
 	}
 
-	ret = dma_declare_coherent_memory(&dev->dev, sram->start, sram->start,
-				resource_size(sram));
-	if (ret)
-		goto err_dma_declare;
-
 	if (cell->enable) {
 		ret = cell->enable(dev);
 		if (ret)
@@ -239,6 +236,28 @@ static int ohci_hcd_tmio_drv_probe(struct platform_device *dev)
 	ohci = hcd_to_ohci(hcd);
 	ohci_hcd_init(ohci);
 
+	hcd->localmem_pool = devm_gen_pool_create(&dev->dev, PAGE_SHIFT,
+						  dev_to_node(&dev->dev),
+						  "ohci-sm501");
+	if (IS_ERR(hcd->localmem_pool)) {
+		ret = PTR_ERR(hcd->localmem_pool);
+		goto err_enable;
+	}
+
+	local_mem = devm_ioremap(&dev->dev, sram->start, resource_size(sram));
+	if (!local_mem) {
+		ret = -ENOMEM;
+		goto err_enable;
+	}
+
+	ret = gen_pool_add_virt(hcd->localmem_pool, (unsigned long)local_mem,
+				sram->start, resource_size(sram),
+				dev_to_node(&dev->dev));
+	if (ret < 0) {
+		dev_err(&dev->dev, "failed to add to pool: %d\n", ret);
+		goto err_enable;
+	}
+
 	ret = usb_add_hcd(hcd, irq, 0);
 	if (ret)
 		goto err_add_hcd;
@@ -254,8 +273,6 @@ static int ohci_hcd_tmio_drv_probe(struct platform_device *dev)
 	if (cell->disable)
 		cell->disable(dev);
 err_enable:
-	dma_release_declared_memory(&dev->dev);
-err_dma_declare:
 	iounmap(hcd->regs);
 err_ioremap_regs:
 	iounmap(tmio->ccr);
@@ -276,7 +293,6 @@ static int ohci_hcd_tmio_drv_remove(struct platform_device *dev)
 	tmio_stop_hc(dev);
 	if (cell->disable)
 		cell->disable(dev);
-	dma_release_declared_memory(&dev->dev);
 	iounmap(hcd->regs);
 	iounmap(tmio->ccr);
 	usb_put_hcd(hcd);
-- 
2.17.1

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

[Greg KH]

On Thu, May 16, 2019 at 02:47:19PM +0300, laurentiu.tudor@nxp.com wrote:
> From: Laurentiu Tudor <laurentiu.tudor@nxp.com>
> 
> 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 <laurentiu.tudor@nxp.com>
> ---
>  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

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

[Christoph Hellwig]

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

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

[Christoph Hellwig]

On Thu, May 16, 2019 at 02:47:20PM +0300, laurentiu.tudor@nxp.com 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.

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

[Laurentiu Tudor]

On 21.05.2019 11:16, Greg KH wrote:
> On Thu, May 16, 2019 at 02:47:19PM +0300, laurentiu.tudor@nxp.com wrote:
>> From: Laurentiu Tudor <laurentiu.tudor@nxp.com>
>>
>> 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&amp;data=02%7C01%7Claurentiu.tudor%40nxp.com%7Cf5242fb28d154ff9653208d6ddc4b41c%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636940234237524499&amp;sdata=KEEUP1KH%2BaraWcVKogeYBzrauh%2FFTzGjSxjk%2BuNozjA%3D&amp;reserved=0
>>
>> Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com>
>> ---
>>   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

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

[Laurentiu Tudor]

On 21.05.2019 13:39, Christoph Hellwig wrote:
> On Thu, May 16, 2019 at 02:47:20PM +0300, laurentiu.tudor@nxp.com 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

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

[Greg KH]

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, laurentiu.tudor@nxp.com wrote:
> >> From: Laurentiu Tudor <laurentiu.tudor@nxp.com>
> >>
> >> 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&amp;data=02%7C01%7Claurentiu.tudor%40nxp.com%7Cf5242fb28d154ff9653208d6ddc4b41c%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636940234237524499&amp;sdata=KEEUP1KH%2BaraWcVKogeYBzrauh%2FFTzGjSxjk%2BuNozjA%3D&amp;reserved=0
> >>
> >> Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com>
> >> ---
> >>   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