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

[RFC PATCH v2 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 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     | 12 ++++++-
 drivers/usb/host/ohci-hcd.c   | 23 +++++++++++---
 drivers/usb/host/ohci-sm501.c | 60 +++++++++++++++++++----------------
 drivers/usb/host/ohci-tmio.c  | 23 +++++++++-----
 include/linux/usb/hcd.h       |  3 ++
 5 files changed, 80 insertions(+), 41 deletions(-)

-- 
2.17.1

[RFC PATCH v2 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   | 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

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

[Christoph Hellwig]

> @@ -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!

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

[Laurentiu Tudor]

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

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

[Christoph Hellwig]

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.

[RFC PATCH v2 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 | 60 +++++++++++++++++++----------------
 1 file changed, 33 insertions(+), 27 deletions(-)

diff --git a/drivers/usb/host/ohci-sm501.c b/drivers/usb/host/ohci-sm501.c
index c26228c25f99..8b829eca04ab 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>
 
@@ -110,40 +111,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 +143,36 @@ 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;
+	}
+	retval = gen_pool_add_virt(hcd->localmem_pool,
+				   (unsigned long)mem->start -
+					mem->parent->start,
+				   mem->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 +190,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 +204,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

[RFC PATCH v2 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 | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/host/ohci-tmio.c b/drivers/usb/host/ohci-tmio.c
index f88a0370659f..34869382618f 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>
 
 /*-------------------------------------------------------------------------*/
 
@@ -224,11 +225,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 +235,20 @@ 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;
+	}
+	ret = gen_pool_add_virt(hcd->localmem_pool, sram->start, 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 +264,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 +284,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: [RFC PATCH v2 0/3] prerequisites for device reserved local mem rework

[Robin Murphy]

+Fredrik, Juergen

On 14/05/2019 15:38, 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.
> 
> Only compiled tested, so any volunteers willing to test are most welcome.

I recall an out-of-tree PlayStation 2 OHCI driver being another 
HCD_LOCAL_MEM user - if Fredrik and Juergen are still active on that, 
hopefully they might be able to comment on whether this approach can 
work for them too. Patchwork link just in case: 
https://patchwork.kernel.org/project/linux-usb/list/?series=117433

Robin.

> 
> Thank you!
> 
> For context, see thread here: https://lkml.org/lkml/2019/4/22/357
> 
> 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     | 12 ++++++-
>   drivers/usb/host/ohci-hcd.c   | 23 +++++++++++---
>   drivers/usb/host/ohci-sm501.c | 60 +++++++++++++++++++----------------
>   drivers/usb/host/ohci-tmio.c  | 23 +++++++++-----
>   include/linux/usb/hcd.h       |  3 ++
>   5 files changed, 80 insertions(+), 41 deletions(-)
> 

Re: [RFC PATCH v2 0/3] prerequisites for device reserved local mem rework

[Fredrik Noring]

Thanks Robin!

> > 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.
> 
> I recall an out-of-tree PlayStation 2 OHCI driver being another 
> HCD_LOCAL_MEM user - if Fredrik and Juergen are still active on that, 
> hopefully they might be able to comment on whether this approach can 
> work for them too. Patchwork link just in case: 
> https://patchwork.kernel.org/project/linux-usb/list/?series=117433

True. In fact I'm preparing a patch submission for this PS2 OHCI driver,
along with about a hundred other patches (unrelated to the USB subsystem).
Hopefully in a few weeks. My patches are currently on top of v5.0. What
branch/version is recommended to try this DMA update?

Here is the v5.0.11 PS2 OHCI driver, for reference:

https://github.com/frno7/linux/blob/ps2-v5.0.11/drivers/usb/host/ohci-ps2.c

Fredrik

Re: [RFC PATCH v2 0/3] prerequisites for device reserved local mem rework

[Laurentiu Tudor]

Hello,

On 14.05.2019 21:29, Fredrik Noring wrote:
> Thanks Robin!
> 
>>> 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.
>>
>> I recall an out-of-tree PlayStation 2 OHCI driver being another
>> HCD_LOCAL_MEM user - if Fredrik and Juergen are still active on that,
>> hopefully they might be able to comment on whether this approach can
>> work for them too. Patchwork link just in case:
>> https://patchwork.kernel.org/project/linux-usb/list/?series=117433
> 
> True. In fact I'm preparing a patch submission for this PS2 OHCI driver,
> along with about a hundred other patches (unrelated to the USB subsystem).
> Hopefully in a few weeks. My patches are currently on top of v5.0. What
> branch/version is recommended to try this DMA update?

I think that any recent kernel will do, so I'd say your current branch 
should be fine.

> Here is the v5.0.11 PS2 OHCI driver, for reference:
> 
> https://github.com/frno7/linux/blob/ps2-v5.0.11/drivers/usb/host/ohci-ps2.c
Please note that the driver will need to be updated, see here for an 
example:

https://patchwork.kernel.org/patch/10943105/

---
Best Regards, Lauretniu

Re: [RFC PATCH v2 0/3] prerequisites for device reserved local mem rework

[Fredrik Noring]

Hi Lauretniu,

> I think that any recent kernel will do, so I'd say your current branch 
> should be fine.

The kernel oopses with "unable to handle kernel paging request at virtual
address 000aba0b" in hcd_alloc_coherent via usb_hcd_map_urb_for_dma. This
relates to patch 2/3 that I didn't quite understand, where

-	retval = dma_declare_coherent_memory(dev, mem->start,
-					 mem->start - mem->parent->start,
-					 resource_size(mem));

becomes

+	retval = gen_pool_add_virt(hcd->localmem_pool,
+				   (unsigned long)mem->start -
+					mem->parent->start,
+				   mem->start, resource_size(mem),

so that arguments two and three switch places. Given

int dma_declare_coherent_memory(struct device *dev, phys_addr_t phys_addr,
				dma_addr_t device_addr, size_t size);

and

int gen_pool_add_virt(struct gen_pool *pool, unsigned long virt, phys_addr_t phys
		 size_t size, int nid)

it seems that the device address (dma_addr_t device_addr) now becomes a
virtual address (unsigned long virt). Is that intended?

My corresponding patch is below for reference. I applied your 1/3 patch
to test it.

Fredrik

diff --git a/drivers/usb/host/ohci-ps2.c b/drivers/usb/host/ohci-ps2.c
--- a/drivers/usb/host/ohci-ps2.c
+++ b/drivers/usb/host/ohci-ps2.c
@@ -7,6 +7,7 @@
  */
 
 #include <linux/dma-mapping.h>
+#include <linux/genalloc.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/usb.h>
@@ -92,12 +93,12 @@ static irqreturn_t ohci_ps2_irq(struct usb_hcd *hcd)
 	return ohci_irq(hcd); /* Call normal IRQ handler. */
 }
 
-static int iopheap_alloc_coherent(struct platform_device *pdev,
-	size_t size, int flags)
+static int iopheap_alloc_coherent(struct platform_device *pdev, size_t size)
 {
 	struct device *dev = &pdev->dev;
 	struct usb_hcd *hcd = platform_get_drvdata(pdev);
 	struct ps2_hcd *ps2priv = hcd_to_priv(hcd);
+	int err;
 
 	if (ps2priv->iop_dma_addr != 0)
 		return 0;
@@ -112,28 +113,37 @@ static int iopheap_alloc_coherent(struct platform_device *pdev,
 		return -ENOMEM;
 	}
 
-	if (dma_declare_coherent_memory(dev,
-			iop_bus_to_phys(ps2priv->iop_dma_addr),
-			ps2priv->iop_dma_addr, size, flags)) {
-		dev_err(dev, "dma_declare_coherent_memory failed\n");
-		iop_free(ps2priv->iop_dma_addr);
-		ps2priv->iop_dma_addr = 0;
-		return -ENOMEM;
+	hcd->localmem_pool = devm_gen_pool_create(dev,
+		PAGE_SHIFT, dev_to_node(dev), DRV_NAME);
+	if (IS_ERR(hcd->localmem_pool)) {
+		err = PTR_ERR(hcd->localmem_pool);
+		goto out_err;
+	}
+
+	err = gen_pool_add_virt(hcd->localmem_pool, ps2priv->iop_dma_addr,
+		iop_bus_to_phys(ps2priv->iop_dma_addr), size, dev_to_node(dev));
+	if (err < 0) {
+		dev_err(dev, "gen_pool_add_virt failed with %d\n", err);
+		goto out_err;
 	}
 
 	return 0;
+
+out_err:
+	iop_free(ps2priv->iop_dma_addr);
+	ps2priv->iop_dma_addr = 0;
+
+	return err;
 }
 
 static void iopheap_free_coherent(struct platform_device *pdev)
 {
-	struct device *dev = &pdev->dev;
 	struct usb_hcd *hcd = platform_get_drvdata(pdev);
 	struct ps2_hcd *ps2priv = hcd_to_priv(hcd);
 
 	if (ps2priv->iop_dma_addr == 0)
 		return;
 
-	dma_release_declared_memory(dev);
 	iop_free(ps2priv->iop_dma_addr);
 	ps2priv->iop_dma_addr = 0;
 }
@@ -177,8 +187,7 @@ static int ohci_hcd_ps2_probe(struct platform_device *pdev)
 		goto err;
 	}
 
-	ret = iopheap_alloc_coherent(pdev,
-		DMA_BUFFER_SIZE, DMA_MEMORY_EXCLUSIVE);
+	ret = iopheap_alloc_coherent(pdev, DMA_BUFFER_SIZE);
 	if (ret != 0)
 		goto err_alloc_coherent;
 

Re: [RFC PATCH v2 0/3] prerequisites for device reserved local mem rework

[Laurentiu Tudor]

Hi Fredrik,

Thanks very much for taking the time to look into this. Please see 
comments inline.

On 15.05.2019 19:28, Fredrik Noring wrote:
> Hi Lauretniu,
> 
>> I think that any recent kernel will do, so I'd say your current branch
>> should be fine.
> 
> The kernel oopses with "unable to handle kernel paging request at virtual
> address 000aba0b" in hcd_alloc_coherent via usb_hcd_map_urb_for_dma. 

By any chance, does this address looks like the dma_addr that the device 
should target?

> This relates to patch 2/3 that I didn't quite understand, where
> 
> -	retval = dma_declare_coherent_memory(dev, mem->start,
> -					 mem->start - mem->parent->start,
> -					 resource_size(mem));
> 
> becomes
> 
> +	retval = gen_pool_add_virt(hcd->localmem_pool,
> +				   (unsigned long)mem->start -
> +					mem->parent->start,
> +				   mem->start, resource_size(mem),
> 
> so that arguments two and three switch places. Given
> 
> int dma_declare_coherent_memory(struct device *dev, phys_addr_t phys_addr,
> 				dma_addr_t device_addr, size_t size);
> 
> and
> 
> int gen_pool_add_virt(struct gen_pool *pool, unsigned long virt, phys_addr_t phys
> 		 size_t size, int nid)
> 
> it seems that the device address (dma_addr_t device_addr) now becomes a
> virtual address (unsigned long virt). Is that intended?

Actually, I think I'm misusing genalloc and also it appears that i need 
to add a mapping on the phys address. So my plan is to change the 
"unsigned long virt" to be the void * returned by the mapping operation 
and the phys_addr_t be the dma_addr_t. I'll return with a patch.

Regarding the usage of unsigned long in genalloc, yeah seems a bit 
strange but by looking at other users in the kernel it appears that 
that's the design.

---
Best Regards, Laurentiu

> My corresponding patch is below for reference. I applied your 1/3 patch
> to test it.
> 
> Fredrik
> 
> diff --git a/drivers/usb/host/ohci-ps2.c b/drivers/usb/host/ohci-ps2.c
> --- a/drivers/usb/host/ohci-ps2.c
> +++ b/drivers/usb/host/ohci-ps2.c
> @@ -7,6 +7,7 @@
>    */
>   
>   #include <linux/dma-mapping.h>
> +#include <linux/genalloc.h>
>   #include <linux/module.h>
>   #include <linux/platform_device.h>
>   #include <linux/usb.h>
> @@ -92,12 +93,12 @@ static irqreturn_t ohci_ps2_irq(struct usb_hcd *hcd)
>   	return ohci_irq(hcd); /* Call normal IRQ handler. */
>   }
>   
> -static int iopheap_alloc_coherent(struct platform_device *pdev,
> -	size_t size, int flags)
> +static int iopheap_alloc_coherent(struct platform_device *pdev, size_t size)
>   {
>   	struct device *dev = &pdev->dev;
>   	struct usb_hcd *hcd = platform_get_drvdata(pdev);
>   	struct ps2_hcd *ps2priv = hcd_to_priv(hcd);
> +	int err;
>   
>   	if (ps2priv->iop_dma_addr != 0)
>   		return 0;
> @@ -112,28 +113,37 @@ static int iopheap_alloc_coherent(struct platform_device *pdev,
>   		return -ENOMEM;
>   	}
>   
> -	if (dma_declare_coherent_memory(dev,
> -			iop_bus_to_phys(ps2priv->iop_dma_addr),
> -			ps2priv->iop_dma_addr, size, flags)) {
> -		dev_err(dev, "dma_declare_coherent_memory failed\n");
> -		iop_free(ps2priv->iop_dma_addr);
> -		ps2priv->iop_dma_addr = 0;
> -		return -ENOMEM;
> +	hcd->localmem_pool = devm_gen_pool_create(dev,
> +		PAGE_SHIFT, dev_to_node(dev), DRV_NAME);
> +	if (IS_ERR(hcd->localmem_pool)) {
> +		err = PTR_ERR(hcd->localmem_pool);
> +		goto out_err;
> +	}
> +
> +	err = gen_pool_add_virt(hcd->localmem_pool, ps2priv->iop_dma_addr,
> +		iop_bus_to_phys(ps2priv->iop_dma_addr), size, dev_to_node(dev));
> +	if (err < 0) {
> +		dev_err(dev, "gen_pool_add_virt failed with %d\n", err);
> +		goto out_err;
>   	}
>   
>   	return 0;
> +
> +out_err:
> +	iop_free(ps2priv->iop_dma_addr);
> +	ps2priv->iop_dma_addr = 0;
> +
> +	return err;
>   }
>   
>   static void iopheap_free_coherent(struct platform_device *pdev)
>   {
> -	struct device *dev = &pdev->dev;
>   	struct usb_hcd *hcd = platform_get_drvdata(pdev);
>   	struct ps2_hcd *ps2priv = hcd_to_priv(hcd);
>   
>   	if (ps2priv->iop_dma_addr == 0)
>   		return;
>   
> -	dma_release_declared_memory(dev);
>   	iop_free(ps2priv->iop_dma_addr);
>   	ps2priv->iop_dma_addr = 0;
>   }
> @@ -177,8 +187,7 @@ static int ohci_hcd_ps2_probe(struct platform_device *pdev)
>   		goto err;
>   	}
>   
> -	ret = iopheap_alloc_coherent(pdev,
> -		DMA_BUFFER_SIZE, DMA_MEMORY_EXCLUSIVE);
> +	ret = iopheap_alloc_coherent(pdev, DMA_BUFFER_SIZE);
>   	if (ret != 0)
>   		goto err_alloc_coherent;
>   
> 

Re: [RFC PATCH v2 0/3] prerequisites for device reserved local mem rework

[Fredrik Noring]

Hi Laurentiu,

> > The kernel oopses with "unable to handle kernel paging request at virtual
> > address 000aba0b" in hcd_alloc_coherent via usb_hcd_map_urb_for_dma. 
> 
> By any chance, does this address looks like the dma_addr that the device 
> should target?

Yes, that looks like a typical device address suitable for its DMA.

> Actually, I think I'm misusing genalloc and also it appears that i need 
> to add a mapping on the phys address. So my plan is to change the 
> "unsigned long virt" to be the void * returned by the mapping operation 
> and the phys_addr_t be the dma_addr_t. I'll return with a patch.

Great, I'm happy to test your next patch revision.

Fredrik

Re: [RFC PATCH v2 0/3] prerequisites for device reserved local mem rework

[Laurentiu Tudor]

[-- Attachment #1: Type: text/plain, Size: 1421 bytes --]

Hi Fredrick,

On 15.05.2019 19:28, Fredrik Noring wrote:
> Hi Lauretniu,
> 
>> I think that any recent kernel will do, so I'd say your current branch
>> should be fine.
> 
> The kernel oopses with "unable to handle kernel paging request at virtual
> address 000aba0b" in hcd_alloc_coherent via usb_hcd_map_urb_for_dma. This
> relates to patch 2/3 that I didn't quite understand, where
> 
> -	retval = dma_declare_coherent_memory(dev, mem->start,
> -					 mem->start - mem->parent->start,
> -					 resource_size(mem));
> 
> becomes
> 
> +	retval = gen_pool_add_virt(hcd->localmem_pool,
> +				   (unsigned long)mem->start -
> +					mem->parent->start,
> +				   mem->start, resource_size(mem),
> 
> so that arguments two and three switch places. Given
> 
> int dma_declare_coherent_memory(struct device *dev, phys_addr_t phys_addr,
> 				dma_addr_t device_addr, size_t size);
> 
> and
> 
> int gen_pool_add_virt(struct gen_pool *pool, unsigned long virt, phys_addr_t phys
> 		 size_t size, int nid)
> 
> it seems that the device address (dma_addr_t device_addr) now becomes a
> virtual address (unsigned long virt). Is that intended?
> 
> My corresponding patch is below for reference. I applied your 1/3 patch
> to test it.

I took your code, added the missing mapping and placed it in a patch. 
Please see attached (compile tested only).

---
Thanks & Best Regards, Laurentiu

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-usb-host-ohci-ps2-init-genalloc-for-local-memory.patch --]
[-- Type: text/x-patch; name="0001-usb-host-ohci-ps2-init-genalloc-for-local-memory.patch", Size: 3428 bytes --]

From 510c34990bbe899a2e4f43f3b904183f126f81de Mon Sep 17 00:00:00 2001
From: Laurentiu Tudor <laurentiu.tudor@nxp.com>
Date: Thu, 16 May 2019 14:35:06 +0300
Subject: [PATCH] usb: host: ohci-ps2: init genalloc for local memory

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-ps2.c | 44 ++++++++++++++++++++++++++-----------
 1 file changed, 31 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/host/ohci-ps2.c b/drivers/usb/host/ohci-ps2.c
index 7669b7358bc3..454e5e3ac96e 100755
--- a/drivers/usb/host/ohci-ps2.c
+++ b/drivers/usb/host/ohci-ps2.c
@@ -7,6 +7,7 @@
  */
 
 #include <linux/dma-mapping.h>
+#include <linux/genalloc.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/usb.h>
@@ -92,12 +93,13 @@ static irqreturn_t ohci_ps2_irq(struct usb_hcd *hcd)
 	return ohci_irq(hcd); /* Call normal IRQ handler. */
 }
 
-static int iopheap_alloc_coherent(struct platform_device *pdev,
-	size_t size, int flags)
+static int iopheap_alloc_coherent(struct platform_device *pdev, size_t size)
 {
 	struct device *dev = &pdev->dev;
 	struct usb_hcd *hcd = platform_get_drvdata(pdev);
 	struct ps2_hcd *ps2priv = hcd_to_priv(hcd);
+	void __iomem *local_mem;
+	int err;
 
 	if (ps2priv->iop_dma_addr != 0)
 		return 0;
@@ -112,28 +114,45 @@ static int iopheap_alloc_coherent(struct platform_device *pdev,
 		return -ENOMEM;
 	}
 
-	if (dma_declare_coherent_memory(dev,
-			iop_bus_to_phys(ps2priv->iop_dma_addr),
-			ps2priv->iop_dma_addr, size, flags)) {
-		dev_err(dev, "dma_declare_coherent_memory failed\n");
-		iop_free(ps2priv->iop_dma_addr);
-		ps2priv->iop_dma_addr = 0;
-		return -ENOMEM;
+	hcd->localmem_pool = devm_gen_pool_create(dev, PAGE_SHIFT,
+						  dev_to_node(dev), DRV_NAME);
+	if (IS_ERR(hcd->localmem_pool)) {
+		err = PTR_ERR(hcd->localmem_pool);
+		goto out_err;
+	}
+
+	local_mem = devm_ioremap(dev,
+				 iop_bus_to_phys(ps2priv->iop_dma_addr),
+				 size);
+	if (!local_mem) {
+		err = -ENOMEM;
+		goto out_err;
+	}
+
+	err = gen_pool_add_virt(hcd->localmem_pool, (unsigned long)local_mem,
+				ps2priv->iop_dma_addr, size, dev_to_node(dev));
+	if (err < 0) {
+		dev_err(dev, "gen_pool_add_virt failed with %d\n", err);
+		goto out_err;
 	}
 
 	return 0;
+
+out_err:
+	iop_free(ps2priv->iop_dma_addr);
+	ps2priv->iop_dma_addr = 0;
+
+	return err;
 }
 
 static void iopheap_free_coherent(struct platform_device *pdev)
 {
-	struct device *dev = &pdev->dev;
 	struct usb_hcd *hcd = platform_get_drvdata(pdev);
 	struct ps2_hcd *ps2priv = hcd_to_priv(hcd);
 
 	if (ps2priv->iop_dma_addr == 0)
 		return;
 
-	dma_release_declared_memory(dev);
 	iop_free(ps2priv->iop_dma_addr);
 	ps2priv->iop_dma_addr = 0;
 }
@@ -177,8 +196,7 @@ static int ohci_hcd_ps2_probe(struct platform_device *pdev)
 		goto err;
 	}
 
-	ret = iopheap_alloc_coherent(pdev,
-		DMA_BUFFER_SIZE, DMA_MEMORY_EXCLUSIVE);
+	ret = iopheap_alloc_coherent(pdev, DMA_BUFFER_SIZE);
 	if (ret != 0)
 		goto err_alloc_coherent;
 
-- 
2.17.1

Re: [RFC PATCH v2 0/3] prerequisites for device reserved local mem rework

[Fredrik Noring]

Hi Laurentiu,

> I took your code, added the missing mapping and placed it in a patch. 
> Please see attached (compile tested only).

Thanks! Unfortunately, the OHCI fails with errors such as

	usb 1-1: device descriptor read/64, error -12

that I tracked down to the calls

	   hub_port_init
	-> usb_control_msg
	-> usb_internal_control_msg
	-> usb_start_wait_urb
	-> usb_submit_urb
	-> usb_hcd_submit_urb
	-> hcd->driver->urb_enqueue
	-> ohci_urb_enqueue
	-> ed_get
	-> ed_alloc
	-> dma_pool_zalloc
	-> dma_pool_alloc
	-> pool_alloc_page,

which returns NULL. Then I noticed

	/* pool_alloc_page() might sleep, so temporarily drop &pool->lock */

that might be a problem considering that the HCD handles pool memory in
IRQ handlers, for instance:

	   do_IRQ
	-> generic_handle_irq
	-> handle_level_irq
	-> handle_irq_event
	-> handle_irq_event_percpu
	-> __handle_irq_event_percpu
	-> usb_hcd_irq
	-> ohci_irq
	-> ohci_work
	-> finish_urb
	-> __usb_hcd_giveback_urb
	-> usb_hcd_unmap_urb_for_dma
	-> hcd_buffer_free

Also, DMA_BUFFER_SIZE in ohci-ps2.c is only 256 KiB in total. Is the new
pool implementation at least as efficient as the previous one?

Fredrik

Re: [RFC PATCH v2 0/3] prerequisites for device reserved local mem rework

[Laurentiu Tudor]

[-- Attachment #1: Type: text/plain, Size: 2220 bytes --]

Hi Fredrik,

On 16.05.2019 18:15, Fredrik Noring wrote:
> Hi Laurentiu,
> 
>> I took your code, added the missing mapping and placed it in a patch.
>> Please see attached (compile tested only).
> 
> Thanks! Unfortunately, the OHCI fails with errors such as
> 
> 	usb 1-1: device descriptor read/64, error -12

Alright, at least the crash went away. :-)

> that I tracked down to the calls
> 
> 	   hub_port_init
> 	-> usb_control_msg
> 	-> usb_internal_control_msg
> 	-> usb_start_wait_urb
> 	-> usb_submit_urb
> 	-> usb_hcd_submit_urb
> 	-> hcd->driver->urb_enqueue
> 	-> ohci_urb_enqueue
> 	-> ed_get
> 	-> ed_alloc
> 	-> dma_pool_zalloc
> 	-> dma_pool_alloc
> 	-> pool_alloc_page,
> 
> which returns NULL. Then I noticed
> 
> 	/* pool_alloc_page() might sleep, so temporarily drop &pool->lock */
> that might be a problem considering that the HCD handles pool memory in
> IRQ handlers, for instance:
> 
> 	   do_IRQ
> 	-> generic_handle_irq
> 	-> handle_level_irq
> 	-> handle_irq_event
> 	-> handle_irq_event_percpu
> 	-> __handle_irq_event_percpu
> 	-> usb_hcd_irq
> 	-> ohci_irq
> 	-> ohci_work
> 	-> finish_urb
> 	-> __usb_hcd_giveback_urb
> 	-> usb_hcd_unmap_urb_for_dma
> 	-> hcd_buffer_free


That looks indeed worrisome but I'd expect that it's not related to 
these genalloc changes that I'm working on. I'll try to look into it but 
as I'm not familiar with the area maybe others will comment.

> Also, DMA_BUFFER_SIZE in ohci-ps2.c is only 256 KiB in total. Is the new
> pool implementation at least as efficient as the previous one?

I don't see any obvious reasons for genalloc to be less efficient.

One thing I noticed between genalloc and the original implementation is 
that previously the device memory was mapped with memremap() while with 
genalloc I'm doing a devm_ioremap(). I can't think of a relevant 
difference except that memremap() maps with WC buth maybe there are 
other arch specific subtleties. I've attached a new version of the 
ohci-ps2 patch replacing the devm_ioremap() with devm_memremap() to 
better match the original implementation. Please test if you find some time.

---
Thanks & Best Regards, Laurentiu

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: v2-0001-usb-host-ohci-ps2-init-genalloc-for-local-memory.patch --]
[-- Type: text/x-patch; name="v2-0001-usb-host-ohci-ps2-init-genalloc-for-local-memory.patch", Size: 3447 bytes --]

From 3cc21aa6c15894f3eb662cc39788e55b72e24634 Mon Sep 17 00:00:00 2001
From: Laurentiu Tudor <laurentiu.tudor@nxp.com>
Date: Thu, 16 May 2019 14:35:06 +0300
Subject: [PATCH v2] usb: host: ohci-ps2: init genalloc for local memory

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-ps2.c | 44 ++++++++++++++++++++++++++-----------
 1 file changed, 31 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/host/ohci-ps2.c b/drivers/usb/host/ohci-ps2.c
index 7669b7358bc3..9f4023f0097a 100755
--- a/drivers/usb/host/ohci-ps2.c
+++ b/drivers/usb/host/ohci-ps2.c
@@ -7,6 +7,7 @@
  */
 
 #include <linux/dma-mapping.h>
+#include <linux/genalloc.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/usb.h>
@@ -92,12 +93,13 @@ static irqreturn_t ohci_ps2_irq(struct usb_hcd *hcd)
 	return ohci_irq(hcd); /* Call normal IRQ handler. */
 }
 
-static int iopheap_alloc_coherent(struct platform_device *pdev,
-	size_t size, int flags)
+static int iopheap_alloc_coherent(struct platform_device *pdev, size_t size)
 {
 	struct device *dev = &pdev->dev;
 	struct usb_hcd *hcd = platform_get_drvdata(pdev);
 	struct ps2_hcd *ps2priv = hcd_to_priv(hcd);
+	void __iomem *local_mem;
+	int err;
 
 	if (ps2priv->iop_dma_addr != 0)
 		return 0;
@@ -112,28 +114,45 @@ static int iopheap_alloc_coherent(struct platform_device *pdev,
 		return -ENOMEM;
 	}
 
-	if (dma_declare_coherent_memory(dev,
-			iop_bus_to_phys(ps2priv->iop_dma_addr),
-			ps2priv->iop_dma_addr, size, flags)) {
-		dev_err(dev, "dma_declare_coherent_memory failed\n");
-		iop_free(ps2priv->iop_dma_addr);
-		ps2priv->iop_dma_addr = 0;
-		return -ENOMEM;
+	hcd->localmem_pool = devm_gen_pool_create(dev, PAGE_SHIFT,
+						  dev_to_node(dev), DRV_NAME);
+	if (IS_ERR(hcd->localmem_pool)) {
+		err = PTR_ERR(hcd->localmem_pool);
+		goto out_err;
+	}
+
+	local_mem = devm_memremap(dev,
+				  iop_bus_to_phys(ps2priv->iop_dma_addr),
+				  size, MEMREMAP_WC);
+	if (!local_mem) {
+		err = -ENOMEM;
+		goto out_err;
+	}
+
+	err = gen_pool_add_virt(hcd->localmem_pool, (unsigned long)local_mem,
+				ps2priv->iop_dma_addr, size, dev_to_node(dev));
+	if (err < 0) {
+		dev_err(dev, "gen_pool_add_virt failed with %d\n", err);
+		goto out_err;
 	}
 
 	return 0;
+
+out_err:
+	iop_free(ps2priv->iop_dma_addr);
+	ps2priv->iop_dma_addr = 0;
+
+	return err;
 }
 
 static void iopheap_free_coherent(struct platform_device *pdev)
 {
-	struct device *dev = &pdev->dev;
 	struct usb_hcd *hcd = platform_get_drvdata(pdev);
 	struct ps2_hcd *ps2priv = hcd_to_priv(hcd);
 
 	if (ps2priv->iop_dma_addr == 0)
 		return;
 
-	dma_release_declared_memory(dev);
 	iop_free(ps2priv->iop_dma_addr);
 	ps2priv->iop_dma_addr = 0;
 }
@@ -177,8 +196,7 @@ static int ohci_hcd_ps2_probe(struct platform_device *pdev)
 		goto err;
 	}
 
-	ret = iopheap_alloc_coherent(pdev,
-		DMA_BUFFER_SIZE, DMA_MEMORY_EXCLUSIVE);
+	ret = iopheap_alloc_coherent(pdev, DMA_BUFFER_SIZE);
 	if (ret != 0)
 		goto err_alloc_coherent;
 
-- 
2.17.1

Re: [RFC PATCH v2 0/3] prerequisites for device reserved local mem rework

[Fredrik Noring]

Hi Laurentiu,

> > that I tracked down to the calls
> > 
> > 	   hub_port_init
> > 	-> usb_control_msg
> > 	-> usb_internal_control_msg
> > 	-> usb_start_wait_urb
> > 	-> usb_submit_urb
> > 	-> usb_hcd_submit_urb
> > 	-> hcd->driver->urb_enqueue
> > 	-> ohci_urb_enqueue
> > 	-> ed_get
> > 	-> ed_alloc

I found that the generic OHCI driver takes a wrong turn here, in ed_alloc,
and eventually also in td_alloc. Fortunately, your patch can be easily
extended to fix them as well. Please see attached patch below.

With that, the OHCI seems to work as expected with HCD_LOCAL_MEM. :)

Would you like me to submit gen_pool_dma_zalloc as a separate patch?

> That looks indeed worrisome but I'd expect that it's not related to 
> these genalloc changes that I'm working on. I'll try to look into it but 
> as I'm not familiar with the area maybe others will comment.

It seems appropriate to verify that the IRQ will not end up sleeping
somewhere in the pool.

> > Also, DMA_BUFFER_SIZE in ohci-ps2.c is only 256 KiB in total. Is the new
> > pool implementation at least as efficient as the previous one?
> 
> I don't see any obvious reasons for genalloc to be less efficient.

OK, good.

> One thing I noticed between genalloc and the original implementation is 
> that previously the device memory was mapped with memremap() while with 
> genalloc I'm doing a devm_ioremap(). I can't think of a relevant 
> difference except that memremap() maps with WC buth maybe there are 
> other arch specific subtleties. I've attached a new version of the 
> ohci-ps2 patch replacing the devm_ioremap() with devm_memremap() to 
> better match the original implementation. Please test if you find some time.

You're right, it makes no difference.

Fredrik

diff --git a/drivers/usb/host/ohci-mem.c b/drivers/usb/host/ohci-mem.c
--- a/drivers/usb/host/ohci-mem.c
+++ b/drivers/usb/host/ohci-mem.c
@@ -82,10 +82,14 @@ dma_to_td (struct ohci_hcd *hc, dma_addr_t td_dma)
 static struct td *
 td_alloc (struct ohci_hcd *hc, gfp_t mem_flags)
 {
+	struct usb_hcd	*hcd = ohci_to_hcd(hc);
 	dma_addr_t	dma;
 	struct td	*td;
 
-	td = dma_pool_zalloc (hc->td_cache, mem_flags, &dma);
+	if (hcd->driver->flags & HCD_LOCAL_MEM)
+		td = gen_pool_dma_zalloc (hcd->localmem_pool, sizeof(*td), &dma);
+	else
+		td = dma_pool_zalloc (hc->td_cache, mem_flags, &dma);
 	if (td) {
 		/* in case hc fetches it, make it look dead */
 		td->hwNextTD = cpu_to_hc32 (hc, dma);
@@ -98,6 +102,7 @@ td_alloc (struct ohci_hcd *hc, gfp_t mem_flags)
 static void
 td_free (struct ohci_hcd *hc, struct td *td)
 {
+	struct usb_hcd	*hcd = ohci_to_hcd(hc);
 	struct td	**prev = &hc->td_hash [TD_HASH_FUNC (td->td_dma)];
 
 	while (*prev && *prev != td)
@@ -106,7 +111,11 @@ td_free (struct ohci_hcd *hc, struct td *td)
 		*prev = td->td_hash;
 	else if ((td->hwINFO & cpu_to_hc32(hc, TD_DONE)) != 0)
 		ohci_dbg (hc, "no hash for td %p\n", td);
-	dma_pool_free (hc->td_cache, td, td->td_dma);
+
+	if (hcd->driver->flags & HCD_LOCAL_MEM)
+		gen_pool_free (hcd->localmem_pool, (unsigned long)td, sizeof(*td));
+	else
+		dma_pool_free (hc->td_cache, td, td->td_dma);
 }
 
 /*-------------------------------------------------------------------------*/
@@ -115,10 +124,14 @@ td_free (struct ohci_hcd *hc, struct td *td)
 static struct ed *
 ed_alloc (struct ohci_hcd *hc, gfp_t mem_flags)
 {
+	struct usb_hcd	*hcd = ohci_to_hcd(hc);
 	dma_addr_t	dma;
 	struct ed	*ed;
 
-	ed = dma_pool_zalloc (hc->ed_cache, mem_flags, &dma);
+	if (hcd->driver->flags & HCD_LOCAL_MEM)
+		ed = gen_pool_dma_zalloc (hcd->localmem_pool, sizeof(*ed), &dma);
+	else
+		ed = dma_pool_zalloc (hc->ed_cache, mem_flags, &dma);
 	if (ed) {
 		INIT_LIST_HEAD (&ed->td_list);
 		ed->dma = dma;
@@ -129,6 +142,11 @@ ed_alloc (struct ohci_hcd *hc, gfp_t mem_flags)
 static void
 ed_free (struct ohci_hcd *hc, struct ed *ed)
 {
-	dma_pool_free (hc->ed_cache, ed, ed->dma);
+	struct usb_hcd	*hcd = ohci_to_hcd(hc);
+
+	if (hcd->driver->flags & HCD_LOCAL_MEM)
+		gen_pool_free (hcd->localmem_pool, (unsigned long)ed, sizeof(*ed));
+	else
+		dma_pool_free (hc->ed_cache, ed, ed->dma);
 }
 
diff --git a/include/linux/genalloc.h b/include/linux/genalloc.h
--- a/include/linux/genalloc.h
+++ b/include/linux/genalloc.h
@@ -121,6 +121,8 @@ extern unsigned long gen_pool_alloc_algo(struct gen_pool *, size_t,
 		genpool_algo_t algo, void *data);
 extern void *gen_pool_dma_alloc(struct gen_pool *pool, size_t size,
 		dma_addr_t *dma);
+extern void *gen_pool_dma_zalloc(struct gen_pool *pool, size_t size,
+		dma_addr_t *dma);
 extern void gen_pool_free(struct gen_pool *, unsigned long, size_t);
 extern void gen_pool_for_each_chunk(struct gen_pool *,
 	void (*)(struct gen_pool *, struct gen_pool_chunk *, void *), void *);
diff --git a/lib/genalloc.c b/lib/genalloc.c
--- a/lib/genalloc.c
+++ b/lib/genalloc.c
@@ -362,6 +362,17 @@ void *gen_pool_dma_alloc(struct gen_pool *pool, size_t size, dma_addr_t *dma)
 }
 EXPORT_SYMBOL(gen_pool_dma_alloc);
 
+void *gen_pool_dma_zalloc(struct gen_pool *pool, size_t size, dma_addr_t *dma)
+{
+	void *vaddr = gen_pool_dma_alloc(pool, size, dma);
+
+	if (vaddr)
+		memset(vaddr, 0, size);
+
+	return vaddr;
+}
+EXPORT_SYMBOL(gen_pool_dma_zalloc);
+
 /**
  * gen_pool_free - free allocated special memory back to the pool
  * @pool: pool to free to

Re: [RFC PATCH v2 0/3] prerequisites for device reserved local mem rework

[Laurentiu Tudor]

Hello Fredrik,

On 17.05.2019 20:41, Fredrik Noring wrote:
> Hi Laurentiu,
> 
>>> that I tracked down to the calls
>>>
>>> 	   hub_port_init
>>> 	-> usb_control_msg
>>> 	-> usb_internal_control_msg
>>> 	-> usb_start_wait_urb
>>> 	-> usb_submit_urb
>>> 	-> usb_hcd_submit_urb
>>> 	-> hcd->driver->urb_enqueue
>>> 	-> ohci_urb_enqueue
>>> 	-> ed_get
>>> 	-> ed_alloc
> 
> I found that the generic OHCI driver takes a wrong turn here, in ed_alloc,
> and eventually also in td_alloc. Fortunately, your patch can be easily
> extended to fix them as well. Please see attached patch below.
> 
> With that, the OHCI seems to work as expected with HCD_LOCAL_MEM. :)

Wow, that's excellent news! Thanks a lot for looking into this.
Are you ok if I add your Signed-Off-by and maybe also Tested-by in the 
first patch of the series?
As a side note, I plan to add a new HCD function and name it something 
like hcd_setup_local_mem() that would take care of setting up the 
genalloc for drivers.

> Would you like me to submit gen_pool_dma_zalloc as a separate patch?

Yes, I think it would make sense to put the new API in a distinct patch. 
I think we can either include it in the next version of the patch series 
or you can submit separately and I'll mention it as dependency for this 
patch series, however you prefer.


---
Thanks & Best Regards, Laurentiu

Re: [RFC PATCH v2 0/3] prerequisites for device reserved local mem rework

[Fredrik Noring]

Hi Laurentiu,

> Wow, that's excellent news! Thanks a lot for looking into this.

You are welcome!

> Are you ok if I add your Signed-Off-by and maybe also Tested-by in the 
> first patch of the series?

Yes, but I have two comments:

1. ohci_mem_init() allocates two DMA pools that are no longer relevant, so
   it seems appropriate to assign NULL to ohci->td_cache and ohci->ed_cache,
   and document this exception in struct ohci_hcd. Unless something more
   elegant can be done, of course.

2. A device address is supplied as phys_addr_t phys to gen_pool_add_virt().
   This seems to work in this particular DMA application, but there will be
   problems if someone does phys_to_virt() or suchlike. Can this be improved
   or clearly explained? gen_pool_virt_to_phys() searches in address ranges,
   for example, so it appears the implementation uses phys quite carefully.

> As a side note, I plan to add a new HCD function and name it something 
> like hcd_setup_local_mem() that would take care of setting up the 
> genalloc for drivers.

Good! Then I suppose the HCD_LOCAL_MEM assignment can be removed from the
drivers too? Like this one:

	ohci_ps2_hc_driver.flags |= HCD_LOCAL_MEM;

Regarding the previous HCD IRQ question, lib/genalloc.c says that

	It is safe to use the allocator in NMI handlers and other special
	unblockable contexts that could otherwise deadlock on locks.  This
	is implemented by using atomic operations and retries on any
	conflicts.  The disadvantage is that there may be livelocks in
	extreme cases.  For better scalability, one allocator can be used
	for each CPU.

so it appears to be good enough for USB purposes.

> Yes, I think it would make sense to put the new API in a distinct patch. 
> I think we can either include it in the next version of the patch series 
> or you can submit separately and I'll mention it as dependency for this 
> patch series, however you prefer.

Please find the patch below and if possible include it in your patch series.

Fredrik

lib/genalloc.c: Add gen_pool_dma_zalloc() for zeroed DMA allocations

gen_pool_dma_zalloc() is a zeroed memory variant of gen_pool_dma_alloc().
Document return values of both, and indicate NULL as a "%NULL" constant.

Signed-off-by: Fredrik Noring <noring@nocrew.org>

--- a/include/linux/genalloc.h
+++ b/include/linux/genalloc.h
@@ -121,6 +121,8 @@ extern unsigned long gen_pool_alloc_algo(struct gen_pool *, size_t,
 		genpool_algo_t algo, void *data);
 extern void *gen_pool_dma_alloc(struct gen_pool *pool, size_t size,
 		dma_addr_t *dma);
+extern void *gen_pool_dma_zalloc(struct gen_pool *pool, size_t size,
+		dma_addr_t *dma);
 extern void gen_pool_free(struct gen_pool *, unsigned long, size_t);
 extern void gen_pool_for_each_chunk(struct gen_pool *,
 	void (*)(struct gen_pool *, struct gen_pool_chunk *, void *), void *);
--- a/lib/genalloc.c
+++ b/lib/genalloc.c
@@ -337,12 +337,14 @@ EXPORT_SYMBOL(gen_pool_alloc_algo);
  * gen_pool_dma_alloc - allocate special memory from the pool for DMA usage
  * @pool: pool to allocate from
  * @size: number of bytes to allocate from the pool
- * @dma: dma-view physical address return value.  Use NULL if unneeded.
+ * @dma: dma-view physical address return value.  Use %NULL if unneeded.
  *
  * Allocate the requested number of bytes from the specified pool.
  * Uses the pool allocation function (with first-fit algorithm by default).
  * Can not be used in NMI handler on architectures without
  * NMI-safe cmpxchg implementation.
+ *
+ * Return: virtual address of the allocated memory, or %NULL on failure
  */
 void *gen_pool_dma_alloc(struct gen_pool *pool, size_t size, dma_addr_t *dma)
 {
@@ -362,6 +364,30 @@ void *gen_pool_dma_alloc(struct gen_pool *pool, size_t size, dma_addr_t *dma)
 }
 EXPORT_SYMBOL(gen_pool_dma_alloc);
 
+/**
+ * gen_pool_dma_zalloc - allocate special zeroed memory from the pool for DMA usage
+ * @pool: pool to allocate from
+ * @size: number of bytes to allocate from the pool
+ * @dma: dma-view physical address return value.  Use %NULL if unneeded.
+ *
+ * Allocate the requested number of zeroed bytes from the specified pool.
+ * Uses the pool allocation function (with first-fit algorithm by default).
+ * Can not be used in NMI handler on architectures without
+ * NMI-safe cmpxchg implementation.
+ *
+ * Return: virtual address of the allocated zeroed memory, or %NULL on failure
+ */
+void *gen_pool_dma_zalloc(struct gen_pool *pool, size_t size, dma_addr_t *dma)
+{
+	void *vaddr = gen_pool_dma_alloc(pool, size, dma);
+
+	if (vaddr)
+		memset(vaddr, 0, size);
+
+	return vaddr;
+}
+EXPORT_SYMBOL(gen_pool_dma_zalloc);
+
 /**
  * gen_pool_free - free allocated special memory back to the pool
  * @pool: pool to free to

Re: [RFC PATCH v2 0/3] prerequisites for device reserved local mem rework

[hch]

On Mon, May 20, 2019 at 05:41:56PM +0200, Fredrik Noring wrote:
> 2. A device address is supplied as phys_addr_t phys to gen_pool_add_virt().
>    This seems to work in this particular DMA application, but there will be
>    problems if someone does phys_to_virt() or suchlike. Can this be improved
>    or clearly explained? gen_pool_virt_to_phys() searches in address ranges,
>    for example, so it appears the implementation uses phys quite carefully.

Well, it is a physical address, but not one that has a kernel mapping.
So phys_addr_t seems right here, but an additional comment explaining it
is not in the kernel mapping never hurts.