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

[Laurentiu Tudor]

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
From 3cc21aa6c15894f3eb662cc39788e55b72e24634 Mon Sep 17 00:00:00 2001
From: Laurentiu Tudor <laurentiu.tudor@xxxxxxx>
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@xxxxxxx>
---
 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