[PATCH v7 0/5] prerequisites for device reserved local mem rework

[PATCH v7 0/5] 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 v7:
 - drop useless __iomem annotation to fix sparse warning
 - select GENERIC_ALLOCATOR to fix compilation on sh arch

Changes in v6:
 - drop some unneeded initializations (Alan)
 - use device name for genpool instead of misleading "ohci-hcd" (Alan)
 - updated some comments (Alan, Fredrik)
 - added Tested-By tags

Changes in v5:
 - updated first patch to preserve bisectability (Christoph, Greg)
 - fixed a few more places where dma api was still being
   used (e.g. td_alloc, ed_alloc) (Fredrik)
 - included patch from Fredrik adding gen_pool_dma_zalloc() api
 - added patch that drops HCD_LOCAL_MEM altogether (Greg)
 - set td_cache / ed_cache to null for devices with local mem (Fredrik)
 - introduce usb_hcd_setup_local_mem() that sets up the genalloc
   pool for drivers and updated drivers to use it

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

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

Laurentiu Tudor (4):
  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
  USB: drop HCD_LOCAL_MEM flag

 drivers/usb/Kconfig            |  1 +
 drivers/usb/core/buffer.c      | 17 ++++++++----
 drivers/usb/core/hcd.c         | 51 ++++++++++++++++++++++++++++------
 drivers/usb/host/ehci-hcd.c    |  2 +-
 drivers/usb/host/fotg210-hcd.c |  2 +-
 drivers/usb/host/ohci-hcd.c    | 25 +++++++++++++----
 drivers/usb/host/ohci-mem.c    | 35 ++++++++++++++++++++---
 drivers/usb/host/ohci-sm501.c  | 50 +++++++++++++++------------------
 drivers/usb/host/ohci-tmio.c   | 15 ++++------
 drivers/usb/host/ohci.h        |  2 ++
 drivers/usb/host/uhci-hcd.c    |  2 +-
 include/linux/genalloc.h       |  1 +
 include/linux/usb/hcd.h        |  6 +++-
 lib/genalloc.c                 | 29 ++++++++++++++++++-
 14 files changed, 172 insertions(+), 66 deletions(-)

-- 
2.17.1

[PATCH v7 1/5] lib/genalloc.c: Add gen_pool_dma_zalloc() for zeroed DMA allocations

[laurentiu.tudor]

From: Fredrik Noring <noring@nocrew.org>

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>
Tested-by: Fredrik Noring <noring@nocrew.org>
---
 include/linux/genalloc.h |  1 +
 lib/genalloc.c           | 29 ++++++++++++++++++++++++++++-
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/include/linux/genalloc.h b/include/linux/genalloc.h
index dd0a452373e7..6c62eeca754f 100644
--- a/include/linux/genalloc.h
+++ b/include/linux/genalloc.h
@@ -121,6 +121,7 @@ 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);
+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
index 7e85d1e37a6e..5db43476a19b 100644
--- 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,31 @@ 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
-- 
2.17.1

[PATCH v7 2/5] 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. To help users, introduce a new HCD API,
usb_hcd_setup_local_mem() that will setup up the genalloc backing
up the device local memory. It will be used in subsequent patches.
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 sh arch to compile GENERIC_ALLOCATOR needs to be selected in
Kconfig.

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

Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com>
Signed-off-by: Fredrik Noring <noring@nocrew.org>
Tested-by: Fredrik Noring <noring@nocrew.org>
Reported-by: kbuild test robot <lkp@intel.com>
---
 drivers/usb/Kconfig         |  1 +
 drivers/usb/core/buffer.c   |  9 +++++++++
 drivers/usb/core/hcd.c      | 36 ++++++++++++++++++++++++++++++++++++
 drivers/usb/host/ohci-hcd.c | 23 ++++++++++++++++++-----
 drivers/usb/host/ohci-mem.c | 35 +++++++++++++++++++++++++++++++----
 drivers/usb/host/ohci.h     |  2 ++
 include/linux/usb/hcd.h     |  5 +++++
 7 files changed, 102 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
index e4b27413f528..389c57d8eba7 100644
--- a/drivers/usb/Kconfig
+++ b/drivers/usb/Kconfig
@@ -45,6 +45,7 @@ config USB_ARCH_HAS_HCD
 config USB
 	tristate "Support for Host-side USB"
 	depends on USB_ARCH_HAS_HCD
+	select GENERIC_ALLOCATOR
 	select USB_COMMON
 	select NLS  # for UTF-8 strings
 	---help---
diff --git a/drivers/usb/core/buffer.c b/drivers/usb/core/buffer.c
index f641342cdec0..d2064ad7ad14 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,6 +125,9 @@ void *hcd_buffer_alloc(
 	if (size == 0)
 		return NULL;
 
+	if (hcd->localmem_pool)
+		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) &&
@@ -152,6 +156,11 @@ void hcd_buffer_free(
 	if (!addr)
 		return;
 
+	if (hcd->localmem_pool) {
+		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))) {
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 94d22551fc1b..29b96e5e8621 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -29,6 +29,8 @@
 #include <linux/workqueue.h>
 #include <linux/pm_runtime.h>
 #include <linux/types.h>
+#include <linux/genalloc.h>
+#include <linux/io.h>
 
 #include <linux/phy/phy.h>
 #include <linux/usb.h>
@@ -3039,6 +3041,40 @@ usb_hcd_platform_shutdown(struct platform_device *dev)
 }
 EXPORT_SYMBOL_GPL(usb_hcd_platform_shutdown);
 
+int usb_hcd_setup_local_mem(struct usb_hcd *hcd, phys_addr_t phys_addr,
+			    dma_addr_t dma, size_t size)
+{
+	int err;
+	void *local_mem;
+
+	hcd->localmem_pool = devm_gen_pool_create(hcd->self.sysdev, PAGE_SHIFT,
+						  dev_to_node(hcd->self.sysdev),
+						  dev_name(hcd->self.sysdev));
+	if (IS_ERR(hcd->localmem_pool))
+		return PTR_ERR(hcd->localmem_pool);
+
+	local_mem = devm_memremap(hcd->self.sysdev, phys_addr,
+				  size, MEMREMAP_WC);
+	if (!local_mem)
+		return -ENOMEM;
+
+	/*
+	 * Here we pass a dma_addr_t but the arg type is a phys_addr_t.
+	 * It's not backed by system memory and thus there's no kernel mapping
+	 * for it.
+	 */
+	err = gen_pool_add_virt(hcd->localmem_pool, (unsigned long)local_mem,
+				dma, size, dev_to_node(hcd->self.sysdev));
+	if (err < 0) {
+		dev_err(hcd->self.sysdev, "gen_pool_add_virt failed with %d\n",
+			err);
+		return err;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(usb_hcd_setup_local_mem);
+
 /*-------------------------------------------------------------------------*/
 
 #if IS_ENABLED(CONFIG_USB_MON)
diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c
index 210181fd98d2..b200b19b44fa 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->localmem_pool)
+		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->localmem_pool)
+			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/drivers/usb/host/ohci-mem.c b/drivers/usb/host/ohci-mem.c
index 3965ac0341eb..4afe27cc7e46 100644
--- a/drivers/usb/host/ohci-mem.c
+++ b/drivers/usb/host/ohci-mem.c
@@ -36,6 +36,13 @@ static void ohci_hcd_init (struct ohci_hcd *ohci)
 
 static int ohci_mem_init (struct ohci_hcd *ohci)
 {
+	/*
+	 * HCs with local memory allocate from localmem_pool so there's
+	 * no need to create the below dma pools.
+	 */
+	if (ohci_to_hcd(ohci)->localmem_pool)
+		return 0;
+
 	ohci->td_cache = dma_pool_create ("ohci_td",
 		ohci_to_hcd(ohci)->self.controller,
 		sizeof (struct td),
@@ -84,8 +91,12 @@ td_alloc (struct ohci_hcd *hc, gfp_t mem_flags)
 {
 	dma_addr_t	dma;
 	struct td	*td;
+	struct usb_hcd	*hcd = ohci_to_hcd(hc);
 
-	td = dma_pool_zalloc (hc->td_cache, mem_flags, &dma);
+	if (hcd->localmem_pool)
+		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);
@@ -99,6 +110,7 @@ static void
 td_free (struct ohci_hcd *hc, struct td *td)
 {
 	struct td	**prev = &hc->td_hash [TD_HASH_FUNC (td->td_dma)];
+	struct usb_hcd	*hcd = ohci_to_hcd(hc);
 
 	while (*prev && *prev != td)
 		prev = &(*prev)->td_hash;
@@ -106,7 +118,12 @@ 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->localmem_pool)
+		gen_pool_free(hcd->localmem_pool, (unsigned long)td,
+			      sizeof(*td));
+	else
+		dma_pool_free(hc->td_cache, td, td->td_dma);
 }
 
 /*-------------------------------------------------------------------------*/
@@ -117,8 +134,12 @@ ed_alloc (struct ohci_hcd *hc, gfp_t mem_flags)
 {
 	dma_addr_t	dma;
 	struct ed	*ed;
+	struct usb_hcd	*hcd = ohci_to_hcd(hc);
 
-	ed = dma_pool_zalloc (hc->ed_cache, mem_flags, &dma);
+	if (hcd->localmem_pool)
+		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 +150,12 @@ 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->localmem_pool)
+		gen_pool_free(hcd->localmem_pool, (unsigned long)ed,
+			      sizeof(*ed));
+	else
+		dma_pool_free(hc->ed_cache, ed, ed->dma);
 }
 
diff --git a/drivers/usb/host/ohci.h b/drivers/usb/host/ohci.h
index ef4813bfc5bf..b015b00774b2 100644
--- a/drivers/usb/host/ohci.h
+++ b/drivers/usb/host/ohci.h
@@ -385,6 +385,8 @@ struct ohci_hcd {
 
 	/*
 	 * memory management for queue data structures
+	 *
+	 * @td_cache and @ed_cache are %NULL if &usb_hcd.localmem_pool is used.
 	 */
 	struct dma_pool		*td_cache;
 	struct dma_pool		*ed_cache;
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index bb57b5af4700..127560a4bfa0 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)
 
+	/* memory pool for HCs having local memory, or %NULL */
+	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.
@@ -461,6 +464,8 @@ extern int usb_add_hcd(struct usb_hcd *hcd,
 		unsigned int irqnum, unsigned long irqflags);
 extern void usb_remove_hcd(struct usb_hcd *hcd);
 extern int usb_hcd_find_raw_port_number(struct usb_hcd *hcd, int port1);
+int usb_hcd_setup_local_mem(struct usb_hcd *hcd, phys_addr_t phys_addr,
+			    dma_addr_t dma, size_t size);
 
 struct platform_device;
 extern void usb_hcd_platform_shutdown(struct platform_device *dev);
-- 
2.17.1

Re: [PATCH v7 2/5] USB: use genalloc for USB HCs with local memory

[Greg KH]

On Wed, May 29, 2019 at 01:28:40PM +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. To help users, introduce a new HCD API,
> usb_hcd_setup_local_mem() that will setup up the genalloc backing
> up the device local memory. It will be used in subsequent patches.
> 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 sh arch to compile GENERIC_ALLOCATOR needs to be selected in
> Kconfig.
> 
> For context, see thread here: https://lkml.org/lkml/2019/4/22/357
> 
> Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com>
> Signed-off-by: Fredrik Noring <noring@nocrew.org>
> Tested-by: Fredrik Noring <noring@nocrew.org>
> Reported-by: kbuild test robot <lkp@intel.com>
> ---
>  drivers/usb/Kconfig         |  1 +
>  drivers/usb/core/buffer.c   |  9 +++++++++
>  drivers/usb/core/hcd.c      | 36 ++++++++++++++++++++++++++++++++++++
>  drivers/usb/host/ohci-hcd.c | 23 ++++++++++++++++++-----
>  drivers/usb/host/ohci-mem.c | 35 +++++++++++++++++++++++++++++++----
>  drivers/usb/host/ohci.h     |  2 ++
>  include/linux/usb/hcd.h     |  5 +++++
>  7 files changed, 102 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
> index e4b27413f528..389c57d8eba7 100644
> --- a/drivers/usb/Kconfig
> +++ b/drivers/usb/Kconfig
> @@ -45,6 +45,7 @@ config USB_ARCH_HAS_HCD
>  config USB
>  	tristate "Support for Host-side USB"
>  	depends on USB_ARCH_HAS_HCD
> +	select GENERIC_ALLOCATOR

Are there any arches that does not have GENERIC_ALLOCATOR?  I don't want
to suddenly cut off a bunch of working systems from having USB support.

thanks,

greg k-h

Re: [PATCH v7 2/5] USB: use genalloc for USB HCs with local memory

[Laurentiu Tudor]

On 29.05.2019 13:38, Greg KH wrote:
> On Wed, May 29, 2019 at 01:28:40PM +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. To help users, introduce a new HCD API,
>> usb_hcd_setup_local_mem() that will setup up the genalloc backing
>> up the device local memory. It will be used in subsequent patches.
>> 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 sh arch to compile GENERIC_ALLOCATOR needs to be selected in
>> Kconfig.
>>
>> 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%7Cade28e1f322c4502cd4808d6e421d0ba%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636947231220264072&amp;sdata=xvmzDztMbeD9GwlcAfx7bBoxhARWgB3vmQkqiE81Lbg%3D&amp;reserved=0
>>
>> Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com>
>> Signed-off-by: Fredrik Noring <noring@nocrew.org>
>> Tested-by: Fredrik Noring <noring@nocrew.org>
>> Reported-by: kbuild test robot <lkp@intel.com>
>> ---
>>   drivers/usb/Kconfig         |  1 +
>>   drivers/usb/core/buffer.c   |  9 +++++++++
>>   drivers/usb/core/hcd.c      | 36 ++++++++++++++++++++++++++++++++++++
>>   drivers/usb/host/ohci-hcd.c | 23 ++++++++++++++++++-----
>>   drivers/usb/host/ohci-mem.c | 35 +++++++++++++++++++++++++++++++----
>>   drivers/usb/host/ohci.h     |  2 ++
>>   include/linux/usb/hcd.h     |  5 +++++
>>   7 files changed, 102 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
>> index e4b27413f528..389c57d8eba7 100644
>> --- a/drivers/usb/Kconfig
>> +++ b/drivers/usb/Kconfig
>> @@ -45,6 +45,7 @@ config USB_ARCH_HAS_HCD
>>   config USB
>>   	tristate "Support for Host-side USB"
>>   	depends on USB_ARCH_HAS_HCD
>> +	select GENERIC_ALLOCATOR
> 
> Are there any arches that does not have GENERIC_ALLOCATOR?  I don't want
> to suddenly cut off a bunch of working systems from having USB support.
> 

lkp report mentions only sh, but even if there are others, I think that 
now having the explicit select should cover them, right?

---
Best Regards, Laurentiu

Re: [PATCH v7 2/5] USB: use genalloc for USB HCs with local memory

[Greg KH]

On Wed, May 29, 2019 at 11:15:54AM +0000, Laurentiu Tudor wrote:
> 
> 
> On 29.05.2019 13:38, Greg KH wrote:
> > On Wed, May 29, 2019 at 01:28:40PM +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. To help users, introduce a new HCD API,
> >> usb_hcd_setup_local_mem() that will setup up the genalloc backing
> >> up the device local memory. It will be used in subsequent patches.
> >> 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 sh arch to compile GENERIC_ALLOCATOR needs to be selected in
> >> Kconfig.
> >>
> >> 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%7Cade28e1f322c4502cd4808d6e421d0ba%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636947231220264072&amp;sdata=xvmzDztMbeD9GwlcAfx7bBoxhARWgB3vmQkqiE81Lbg%3D&amp;reserved=0
> >>
> >> Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com>
> >> Signed-off-by: Fredrik Noring <noring@nocrew.org>
> >> Tested-by: Fredrik Noring <noring@nocrew.org>
> >> Reported-by: kbuild test robot <lkp@intel.com>
> >> ---
> >>   drivers/usb/Kconfig         |  1 +
> >>   drivers/usb/core/buffer.c   |  9 +++++++++
> >>   drivers/usb/core/hcd.c      | 36 ++++++++++++++++++++++++++++++++++++
> >>   drivers/usb/host/ohci-hcd.c | 23 ++++++++++++++++++-----
> >>   drivers/usb/host/ohci-mem.c | 35 +++++++++++++++++++++++++++++++----
> >>   drivers/usb/host/ohci.h     |  2 ++
> >>   include/linux/usb/hcd.h     |  5 +++++
> >>   7 files changed, 102 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
> >> index e4b27413f528..389c57d8eba7 100644
> >> --- a/drivers/usb/Kconfig
> >> +++ b/drivers/usb/Kconfig
> >> @@ -45,6 +45,7 @@ config USB_ARCH_HAS_HCD
> >>   config USB
> >>   	tristate "Support for Host-side USB"
> >>   	depends on USB_ARCH_HAS_HCD
> >> +	select GENERIC_ALLOCATOR
> > 
> > Are there any arches that does not have GENERIC_ALLOCATOR?  I don't want
> > to suddenly cut off a bunch of working systems from having USB support.
> > 
> 
> lkp report mentions only sh, but even if there are others, I think that 
> now having the explicit select should cover them, right?

As long as GENERIC_ALLOCATOR works on all arches, yes, that's fine, but
please verify that this is the case.

thanks,

greg k-h

Re: [PATCH v7 2/5] USB: use genalloc for USB HCs with local memory

[hch]

On Wed, May 29, 2019 at 04:23:34AM -0700, Greg KH wrote:
> As long as GENERIC_ALLOCATOR works on all arches, yes, that's fine, but
> please verify that this is the case.

It works everywhere.  The issue here is just that it get pulled in
by default on most architetures, but not on all.

Re: [PATCH v7 2/5] USB: use genalloc for USB HCs with local memory

[Greg KH]

On Wed, May 29, 2019 at 01:25:15PM +0200, hch@lst.de wrote:
> On Wed, May 29, 2019 at 04:23:34AM -0700, Greg KH wrote:
> > As long as GENERIC_ALLOCATOR works on all arches, yes, that's fine, but
> > please verify that this is the case.
> 
> It works everywhere.  The issue here is just that it get pulled in
> by default on most architetures, but not on all.

Ah, ok, that's fine then, no objection from me, let me go review the
patches again...

thanks,

greg k-h

[PATCH v7 3/5] 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 | 47 +++++++++++++++--------------------
 1 file changed, 20 insertions(+), 27 deletions(-)

diff --git a/drivers/usb/host/ohci-sm501.c b/drivers/usb/host/ohci-sm501.c
index c26228c25f99..b710e100aec9 100644
--- a/drivers/usb/host/ohci-sm501.c
+++ b/drivers/usb/host/ohci-sm501.c
@@ -110,40 +110,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 +142,24 @@ 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.
+	 */
+
+	if (usb_hcd_setup_local_mem(hcd, mem->start,
+				    mem->start - mem->parent->start,
+				    resource_size(mem)) < 0)
+		goto err5;
 	retval = usb_add_hcd(hcd, irq, IRQF_SHARED);
 	if (retval)
 		goto err5;
@@ -181,8 +177,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 +191,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

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

[Guenter Roeck]

On Wed, May 29, 2019 at 01:28:41PM +0300, laurentiu.tudor@nxp.com wrote:
> 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>

This patch results in usb access failures when trying to boot from the
sm501-usb controller on sh4 with qemu.

usb 1-2.1: reset full-speed USB device number 4 using sm501-usb
sd 1:0:0:0: [sda] tag#0 UNKNOWN(0x2003) Result: hostbyte=0x03 driverbyte=0x00
sd 1:0:0:0: [sda] tag#0 CDB: opcode=0x28 28 00 00 00 08 7c 00 00 f0 00
print_req_error: I/O error, dev sda, sector 2172 flags 80700
usb 1-2.1: reset full-speed USB device number 4 using sm501-usb
sd 1:0:0:0: [sda] tag#0 UNKNOWN(0x2003) Result: hostbyte=0x03 driverbyte=0x00
sd 1:0:0:0: [sda] tag#0 CDB: opcode=0x28 28 00 00 00 01 da 00 00 f0 00
print_req_error: I/O error, dev sda, sector 474 flags 84700
usb 1-2.1: reset full-speed USB device number 4 using sm501-usb
sd 1:0:0:0: [sda] tag#0 UNKNOWN(0x2003) Result: hostbyte=0x03 driverbyte=0x00
sd 1:0:0:0: [sda] tag#0 CDB: opcode=0x28 28 00 00 00 02 da 00 00 f0 00
print_req_error: I/O error, dev sda, sector 730 flags 84700
usb 1-2.1: reset full-speed USB device number 4 using sm501-usb
sd 1:0:0:0: [sda] tag#0 UNKNOWN(0x2003) Result: hostbyte=0x03 driverbyte=0x00
sd 1:0:0:0: [sda] tag#0 CDB: opcode=0x28 28 00 00 00 0b 50 00 00 f0 00
print_req_error: I/O error, dev sda, sector 2896 flags 84700

Qemu command line is:

The qemu command line is:

qemu-system-sh4 -M r2d \
        -kernel ./arch/sh/boot/zImage \
	-snapshot \
	-usb -device usb-storage,drive=d0 \
	-drive file=rootfs.ext2,if=none,id=d0,format=raw \
	-append 'panic=-1 slub_debug=FZPUA root=/dev/sda rootwait console=ttySC1,115200 earlycon=scif,mmio16,0xffe80000 noiotrap' \
	-serial null -serial stdio \
	-net nic,model=rtl8139 -net user -nographic -monitor null

Reverting this patch as well as "USB: drop HCD_LOCAL_MEM flag" fixes the
problem. Reverting "USB: drop HCD_LOCAL_MEM flag" alone does not help.

Guenter

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

[Guenter Roeck]

On Wed, Jun 05, 2019 at 02:46:22PM -0700, Guenter Roeck wrote:
> On Wed, May 29, 2019 at 01:28:41PM +0300, laurentiu.tudor@nxp.com wrote:
> > 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>
> 
> This patch results in usb access failures when trying to boot from the
> sm501-usb controller on sh4 with qemu.
> 
> usb 1-2.1: reset full-speed USB device number 4 using sm501-usb
> sd 1:0:0:0: [sda] tag#0 UNKNOWN(0x2003) Result: hostbyte=0x03 driverbyte=0x00
> sd 1:0:0:0: [sda] tag#0 CDB: opcode=0x28 28 00 00 00 08 7c 00 00 f0 00
> print_req_error: I/O error, dev sda, sector 2172 flags 80700
> usb 1-2.1: reset full-speed USB device number 4 using sm501-usb
> sd 1:0:0:0: [sda] tag#0 UNKNOWN(0x2003) Result: hostbyte=0x03 driverbyte=0x00
> sd 1:0:0:0: [sda] tag#0 CDB: opcode=0x28 28 00 00 00 01 da 00 00 f0 00
> print_req_error: I/O error, dev sda, sector 474 flags 84700
> usb 1-2.1: reset full-speed USB device number 4 using sm501-usb
> sd 1:0:0:0: [sda] tag#0 UNKNOWN(0x2003) Result: hostbyte=0x03 driverbyte=0x00
> sd 1:0:0:0: [sda] tag#0 CDB: opcode=0x28 28 00 00 00 02 da 00 00 f0 00
> print_req_error: I/O error, dev sda, sector 730 flags 84700
> usb 1-2.1: reset full-speed USB device number 4 using sm501-usb
> sd 1:0:0:0: [sda] tag#0 UNKNOWN(0x2003) Result: hostbyte=0x03 driverbyte=0x00
> sd 1:0:0:0: [sda] tag#0 CDB: opcode=0x28 28 00 00 00 0b 50 00 00 f0 00
> print_req_error: I/O error, dev sda, sector 2896 flags 84700
> 
> Qemu command line is:
> 
> The qemu command line is:
> 
> qemu-system-sh4 -M r2d \
>         -kernel ./arch/sh/boot/zImage \
> 	-snapshot \
> 	-usb -device usb-storage,drive=d0 \
> 	-drive file=rootfs.ext2,if=none,id=d0,format=raw \
> 	-append 'panic=-1 slub_debug=FZPUA root=/dev/sda rootwait console=ttySC1,115200 earlycon=scif,mmio16,0xffe80000 noiotrap' \
> 	-serial null -serial stdio \
> 	-net nic,model=rtl8139 -net user -nographic -monitor null
> 
> Reverting this patch as well as "USB: drop HCD_LOCAL_MEM flag" fixes the
> problem. Reverting "USB: drop HCD_LOCAL_MEM flag" alone does not help.
> 

This problem is still seen in next-20190611.
Has anyone actually tested this code ?

Guenter

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

[Fredrik Noring]

Hi Guenter,

> > This patch results in usb access failures when trying to boot from the
> > sm501-usb controller on sh4 with qemu.
> > 
> > usb 1-2.1: reset full-speed USB device number 4 using sm501-usb
> > sd 1:0:0:0: [sda] tag#0 UNKNOWN(0x2003) Result: hostbyte=0x03 driverbyte=0x00
> > sd 1:0:0:0: [sda] tag#0 CDB: opcode=0x28 28 00 00 00 08 7c 00 00 f0 00
> > print_req_error: I/O error, dev sda, sector 2172 flags 80700
> > usb 1-2.1: reset full-speed USB device number 4 using sm501-usb
> > sd 1:0:0:0: [sda] tag#0 UNKNOWN(0x2003) Result: hostbyte=0x03 driverbyte=0x00
> > sd 1:0:0:0: [sda] tag#0 CDB: opcode=0x28 28 00 00 00 01 da 00 00 f0 00
> > print_req_error: I/O error, dev sda, sector 474 flags 84700
> > usb 1-2.1: reset full-speed USB device number 4 using sm501-usb
> > sd 1:0:0:0: [sda] tag#0 UNKNOWN(0x2003) Result: hostbyte=0x03 driverbyte=0x00
> > sd 1:0:0:0: [sda] tag#0 CDB: opcode=0x28 28 00 00 00 02 da 00 00 f0 00
> > print_req_error: I/O error, dev sda, sector 730 flags 84700
> > usb 1-2.1: reset full-speed USB device number 4 using sm501-usb
> > sd 1:0:0:0: [sda] tag#0 UNKNOWN(0x2003) Result: hostbyte=0x03 driverbyte=0x00
> > sd 1:0:0:0: [sda] tag#0 CDB: opcode=0x28 28 00 00 00 0b 50 00 00 f0 00
> > print_req_error: I/O error, dev sda, sector 2896 flags 84700
> > 
> > Qemu command line is:
> > 
> > The qemu command line is:
> > 
> > qemu-system-sh4 -M r2d \
> >         -kernel ./arch/sh/boot/zImage \
> > 	-snapshot \
> > 	-usb -device usb-storage,drive=d0 \
> > 	-drive file=rootfs.ext2,if=none,id=d0,format=raw \
> > 	-append 'panic=-1 slub_debug=FZPUA root=/dev/sda rootwait console=ttySC1,115200 earlycon=scif,mmio16,0xffe80000 noiotrap' \
> > 	-serial null -serial stdio \
> > 	-net nic,model=rtl8139 -net user -nographic -monitor null
> > 
> > Reverting this patch as well as "USB: drop HCD_LOCAL_MEM flag" fixes the
> > problem. Reverting "USB: drop HCD_LOCAL_MEM flag" alone does not help.
> > 
> 
> This problem is still seen in next-20190611.
> Has anyone actually tested this code ?

I tested patches 1, 2 and 5 with v5.0.19. Perhaps yet another part of the
OHCI subsystem allocates memory from the wrong pool? With some luck it is
relatively easy to trace backwards from the error messages to the point
where the memory is being allocated. One way to establish this is to
sprinkle printk around if-statements. There may be 10-20 levels of calls
including one or two indirect calls via pointers. Would you be able to do
that?

Fredrik

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

[Guenter Roeck]

On Tue, Jun 11, 2019 at 07:26:54PM +0200, Fredrik Noring wrote:
> Hi Guenter,
> 
> > > This patch results in usb access failures when trying to boot from the
> > > sm501-usb controller on sh4 with qemu.
> > > 
> > > usb 1-2.1: reset full-speed USB device number 4 using sm501-usb
> > > sd 1:0:0:0: [sda] tag#0 UNKNOWN(0x2003) Result: hostbyte=0x03 driverbyte=0x00
> > > sd 1:0:0:0: [sda] tag#0 CDB: opcode=0x28 28 00 00 00 08 7c 00 00 f0 00
> > > print_req_error: I/O error, dev sda, sector 2172 flags 80700
> > > usb 1-2.1: reset full-speed USB device number 4 using sm501-usb
> > > sd 1:0:0:0: [sda] tag#0 UNKNOWN(0x2003) Result: hostbyte=0x03 driverbyte=0x00
> > > sd 1:0:0:0: [sda] tag#0 CDB: opcode=0x28 28 00 00 00 01 da 00 00 f0 00
> > > print_req_error: I/O error, dev sda, sector 474 flags 84700
> > > usb 1-2.1: reset full-speed USB device number 4 using sm501-usb
> > > sd 1:0:0:0: [sda] tag#0 UNKNOWN(0x2003) Result: hostbyte=0x03 driverbyte=0x00
> > > sd 1:0:0:0: [sda] tag#0 CDB: opcode=0x28 28 00 00 00 02 da 00 00 f0 00
> > > print_req_error: I/O error, dev sda, sector 730 flags 84700
> > > usb 1-2.1: reset full-speed USB device number 4 using sm501-usb
> > > sd 1:0:0:0: [sda] tag#0 UNKNOWN(0x2003) Result: hostbyte=0x03 driverbyte=0x00
> > > sd 1:0:0:0: [sda] tag#0 CDB: opcode=0x28 28 00 00 00 0b 50 00 00 f0 00
> > > print_req_error: I/O error, dev sda, sector 2896 flags 84700
> > > 
> > > Qemu command line is:
> > > 
> > > The qemu command line is:
> > > 
> > > qemu-system-sh4 -M r2d \
> > >         -kernel ./arch/sh/boot/zImage \
> > > 	-snapshot \
> > > 	-usb -device usb-storage,drive=d0 \
> > > 	-drive file=rootfs.ext2,if=none,id=d0,format=raw \
> > > 	-append 'panic=-1 slub_debug=FZPUA root=/dev/sda rootwait console=ttySC1,115200 earlycon=scif,mmio16,0xffe80000 noiotrap' \
> > > 	-serial null -serial stdio \
> > > 	-net nic,model=rtl8139 -net user -nographic -monitor null
> > > 
> > > Reverting this patch as well as "USB: drop HCD_LOCAL_MEM flag" fixes the
> > > problem. Reverting "USB: drop HCD_LOCAL_MEM flag" alone does not help.
> > > 
> > 
> > This problem is still seen in next-20190611.
> > Has anyone actually tested this code ?
> 
> I tested patches 1, 2 and 5 with v5.0.19. Perhaps yet another part of the
> OHCI subsystem allocates memory from the wrong pool? With some luck it is
> relatively easy to trace backwards from the error messages to the point
> where the memory is being allocated. One way to establish this is to
> sprinkle printk around if-statements. There may be 10-20 levels of calls
> including one or two indirect calls via pointers. Would you be able to do
> that?
> 

I don't think I'll have time to do that anytime soon. Not that I know what
exactly to look for in the first place.

Can you do that debugging yourself ? All you would need is a cross-compiler
(eg from kernel.org), qemu, and a working configuration (the root file
system doesn't really matter since the code doesn't get to the point of
loading it, but you can use [1]). For the configuration file, you can use
rts7751r2dplus_defconfig with CONFIG_CMDLINE and CONFIG_CMDLINE_OVERWRITE
removed.

Thanks,
Guenter

---
[1] https://github.com/groeck/linux-build-test/blob/master/rootfs/sh/rootfs.ext2.gz

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

[Fredrik Noring]

Hi Guenter,

> I don't think I'll have time to do that anytime soon. Not that I know what
> exactly to look for in the first place.

I can confirm that there is a problem with mass storage devices and these
local memory patches.

Fredrik

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

[Guenter Roeck]

Hi Fredrik,

On 6/13/19 6:40 AM, Fredrik Noring wrote:
> Hi Guenter,
> 
>> I don't think I'll have time to do that anytime soon. Not that I know what
>> exactly to look for in the first place.
> 
> I can confirm that there is a problem with mass storage devices and these
> local memory patches.
> 

Thanks for the confirmation. Do you see the problem only with the
ohci-sm501 driver or also with others ?

Thanks,
Guenter

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

[Fredrik Noring]

Hi Guenter,

> Thanks for the confirmation. Do you see the problem only with the
> ohci-sm501 driver or also with others ?

All are likely affected, but it depends, because I believe the problem is
that the USB subsystem runs out of memory. Please try the attached patch!

The pool assumed 4096 byte page alignment for every allocation, which is
excessive given that many requests are for 16 and 32 bytes. In the patch
below, I have turned down the order to 5, which is good enough for the ED
and TD structures of the OHCI, but not enough for the HCCA that needs 256
byte alignment. With some luck, the WARN_ON_ONCE will not trigger in your
test, though. If it does, you may try to increase the order from 5 to 8.

I have observed strange things happen when the USB subsystem runs out of
memory. The mass storage drivers often seem to busy-wait on -ENOMEM,
consuming a lot of processor resources. It would be much more efficient
to sleep waiting for memory to become available.

In your case I suspect that allocation failures are not correctly
attributed. Certain kinds of temporary freezes may also occur, as the
various devices are reset due to host memory allocation errors.

Fredrik

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -3011,7 +3011,7 @@ int usb_hcd_setup_local_mem(struct usb_hcd *hcd, phys_addr_t phys_addr,
 	int err;
 	void __iomem *local_mem;
 
-	hcd->localmem_pool = devm_gen_pool_create(hcd->self.sysdev, PAGE_SHIFT,
+	hcd->localmem_pool = devm_gen_pool_create(hcd->self.sysdev, 5,
 						  dev_to_node(hcd->self.sysdev),
 						  dev_name(hcd->self.sysdev));
 	if (IS_ERR(hcd->localmem_pool))
diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c
--- a/drivers/usb/host/ohci-hcd.c
+++ b/drivers/usb/host/ohci-hcd.c
@@ -517,6 +517,7 @@ static int ohci_init (struct ohci_hcd *ohci)
 						GFP_KERNEL);
 	if (!ohci->hcca)
 		return -ENOMEM;
+	WARN_ON_ONCE(ohci->hcca_dma & 0xff);
 
 	if ((ret = ohci_mem_init (ohci)) < 0)
 		ohci_stop (hcd);

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

[Guenter Roeck]

On 6/13/19 8:34 AM, Fredrik Noring wrote:
> Hi Guenter,
> 
>> Thanks for the confirmation. Do you see the problem only with the
>> ohci-sm501 driver or also with others ?
> 
> All are likely affected, but it depends, because I believe the problem is
> that the USB subsystem runs out of memory. Please try the attached patch!
> 
> The pool assumed 4096 byte page alignment for every allocation, which is
> excessive given that many requests are for 16 and 32 bytes. In the patch
> below, I have turned down the order to 5, which is good enough for the ED
> and TD structures of the OHCI, but not enough for the HCCA that needs 256
> byte alignment. With some luck, the WARN_ON_ONCE will not trigger in your
> test, though. If it does, you may try to increase the order from 5 to 8.
> 

You are right, the patch below fixes the problem. I did not get the warning
with order==5. Nevertheless, I also tested with order==8; that works as well.

Thanks a lot for tracking this down!
Guenter

> I have observed strange things happen when the USB subsystem runs out of
> memory. The mass storage drivers often seem to busy-wait on -ENOMEM,
> consuming a lot of processor resources. It would be much more efficient
> to sleep waiting for memory to become available.
> 
> In your case I suspect that allocation failures are not correctly
> attributed. Certain kinds of temporary freezes may also occur, as the
> various devices are reset due to host memory allocation errors.
>  > Fredrik
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -3011,7 +3011,7 @@ int usb_hcd_setup_local_mem(struct usb_hcd *hcd, phys_addr_t phys_addr,
>   	int err;
>   	void __iomem *local_mem;
>   
> -	hcd->localmem_pool = devm_gen_pool_create(hcd->self.sysdev, PAGE_SHIFT,
> +	hcd->localmem_pool = devm_gen_pool_create(hcd->self.sysdev, 5,
>   						  dev_to_node(hcd->self.sysdev),
>   						  dev_name(hcd->self.sysdev));
>   	if (IS_ERR(hcd->localmem_pool))
> diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c
> --- a/drivers/usb/host/ohci-hcd.c
> +++ b/drivers/usb/host/ohci-hcd.c
> @@ -517,6 +517,7 @@ static int ohci_init (struct ohci_hcd *ohci)
>   						GFP_KERNEL);
>   	if (!ohci->hcca)
>   		return -ENOMEM;
> +	WARN_ON_ONCE(ohci->hcca_dma & 0xff);
>   
>   	if ((ret = ohci_mem_init (ohci)) < 0)
>   		ohci_stop (hcd);
> 

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

[Fredrik Noring]

Hi Guenter,

> You are right, the patch below fixes the problem. I did not get the warning
> with order==5. Nevertheless, I also tested with order==8; that works as well.
> 
> Thanks a lot for tracking this down!

You are welcome, and thanks for your report!

This patch series needs some redesign, I think, because the problem you
reported would come back if one attaches two or more devices to the
system. Local memory devices are typically memory constrained and so it
has to be used efficiently. I believe there are four kinds of alignments
to consider when memory is allocated in the pool:

	- 256 bytes for the host controller communication area (HCCA);
	- 32 bytes for the general transfer descriptors (TDs);
	- 16 bytes for the endpoint descriptors (EDs);
	- buffer alignment for data.

Using the greatest common alignment for all is clearly an undesirable
regression. The TDs and EDs could have their own subpools, perhaps, as
they are abundant. There is only one instance of the HCCA.

As mentioned, the USB subsystem could be improved to properly report
allocation failures, and the logic to retry allocations could be more
efficient by avoiding polling loops.

Fredrik

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

[Christoph Hellwig]

Can you send me the patch formally so that I can queue it up for the
dma-mapping tree?

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

[Fredrik Noring]

Hi Christoph,

> Can you send me the patch formally so that I can queue it up for the
> dma-mapping tree?

That patch would be detrimental to local memory devices, as previously
discussed, so I would like to suggest a much better approach, as shown below,
where allocations are aligned as required but not necessarily much more than
that.

Fredrik

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -3014,7 +3014,7 @@ int usb_hcd_setup_local_mem(struct usb_hcd *hcd, phys_addr_t phys_addr,
 	int err;
 	void __iomem *local_mem;
 
-	hcd->localmem_pool = devm_gen_pool_create(hcd->self.sysdev, PAGE_SHIFT,
+	hcd->localmem_pool = devm_gen_pool_create(hcd->self.sysdev, 4,
 						  dev_to_node(hcd->self.sysdev),
 						  dev_name(hcd->self.sysdev));
 	if (IS_ERR(hcd->localmem_pool))
diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c
--- a/drivers/usb/host/ohci-hcd.c
+++ b/drivers/usb/host/ohci-hcd.c
@@ -507,9 +507,9 @@ static int ohci_init (struct ohci_hcd *ohci)
 	ohci->prev_frame_no = IO_WATCHDOG_OFF;
 
 	if (hcd->localmem_pool)
-		ohci->hcca = gen_pool_dma_alloc(hcd->localmem_pool,
+		ohci->hcca = gen_pool_dma_alloc_align(hcd->localmem_pool,
 						sizeof(*ohci->hcca),
-						&ohci->hcca_dma);
+						&ohci->hcca_dma, 256);
 	else
 		ohci->hcca = dma_alloc_coherent(hcd->self.controller,
 						sizeof(*ohci->hcca),
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
@@ -94,7 +94,8 @@ td_alloc (struct ohci_hcd *hc, gfp_t mem_flags)
 	struct usb_hcd	*hcd = ohci_to_hcd(hc);
 
 	if (hcd->localmem_pool)
-		td = gen_pool_dma_zalloc(hcd->localmem_pool, sizeof(*td), &dma);
+		td = gen_pool_dma_zalloc_align(hcd->localmem_pool,
+				sizeof(*td), &dma, 32);
 	else
 		td = dma_pool_zalloc(hc->td_cache, mem_flags, &dma);
 	if (td) {
@@ -137,7 +138,8 @@ ed_alloc (struct ohci_hcd *hc, gfp_t mem_flags)
 	struct usb_hcd	*hcd = ohci_to_hcd(hc);
 
 	if (hcd->localmem_pool)
-		ed = gen_pool_dma_zalloc(hcd->localmem_pool, sizeof(*ed), &dma);
+		ed = gen_pool_dma_zalloc_align(hcd->localmem_pool,
+				sizeof(*ed), &dma, 16);
 	else
 		ed = dma_pool_zalloc(hc->ed_cache, mem_flags, &dma);
 	if (ed) {
diff --git a/include/linux/genalloc.h b/include/linux/genalloc.h
--- a/include/linux/genalloc.h
+++ b/include/linux/genalloc.h
@@ -121,7 +121,15 @@ 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);
-void *gen_pool_dma_zalloc(struct gen_pool *pool, size_t size, dma_addr_t *dma);
+extern void *gen_pool_dma_alloc_algo(struct gen_pool *pool, size_t size,
+		dma_addr_t *dma, genpool_algo_t algo, void *data);
+extern void *gen_pool_dma_alloc_align(struct gen_pool *pool, size_t size,
+		dma_addr_t *dma, int align);
+extern void *gen_pool_dma_zalloc(struct gen_pool *pool, size_t size, dma_addr_t *dma);
+extern void *gen_pool_dma_zalloc_algo(struct gen_pool *pool, size_t size,
+		dma_addr_t *dma, genpool_algo_t algo, void *data);
+extern void *gen_pool_dma_zalloc_align(struct gen_pool *pool, size_t size,
+		dma_addr_t *dma, int align);
 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
@@ -347,13 +347,33 @@ EXPORT_SYMBOL(gen_pool_alloc_algo);
  * 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)
+{
+	return gen_pool_dma_alloc_algo(pool, size, dma, pool->algo, pool->data);
+}
+EXPORT_SYMBOL(gen_pool_dma_alloc);
+
+/**
+ * gen_pool_dma_alloc_algo - allocate special memory from the pool for DMA
+ * usage with the given pool algorithm
+ * @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.
+ * @algo: algorithm passed from caller
+ * @data: data passed to algorithm
+ *
+ * Allocate the requested number of bytes from the specified pool. Uses the
+ * given pool allocation function. Can not be used in NMI handler on
+ * architectures without NMI-safe cmpxchg implementation.
+ */
+void *gen_pool_dma_alloc_algo(struct gen_pool *pool, size_t size,
+		dma_addr_t *dma, genpool_algo_t algo, void *data)
 {
 	unsigned long vaddr;
 
 	if (!pool)
 		return NULL;
 
-	vaddr = gen_pool_alloc(pool, size);
+	vaddr = gen_pool_alloc_algo(pool, size, algo, data);
 	if (!vaddr)
 		return NULL;
 
@@ -362,7 +382,31 @@ void *gen_pool_dma_alloc(struct gen_pool *pool, size_t size, dma_addr_t *dma)
 
 	return (void *)vaddr;
 }
-EXPORT_SYMBOL(gen_pool_dma_alloc);
+EXPORT_SYMBOL(gen_pool_dma_alloc_algo);
+
+/**
+ * gen_pool_dma_zalloc_align - allocate special from the pool for DMA usage
+ * with the given alignment
+ * @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.
+ * @align: alignment in bytes for starting address
+ *
+ * Allocate the requested number bytes from the specified pool, with the given
+ * alignment restriction. 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_align(struct gen_pool *pool, size_t size,
+		dma_addr_t *dma, int align)
+{
+	struct genpool_data_align data = { .align = align };
+
+	return gen_pool_dma_alloc_algo(pool, size, dma,
+			gen_pool_first_fit_align, &data);
+}
+EXPORT_SYMBOL(gen_pool_dma_alloc_align);
 
 /**
  * gen_pool_dma_zalloc - allocate special zeroed memory from the pool for
@@ -380,14 +424,60 @@ 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);
+	return gen_pool_dma_zalloc_algo(pool, size, dma, pool->algo, pool->data);
+}
+EXPORT_SYMBOL(gen_pool_dma_zalloc);
+
+/**
+ * gen_pool_dma_zalloc_algo - allocate special zeroed memory from the pool for
+ * DMA usage with the given pool algorithm
+ * @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.
+ * @algo: algorithm passed from caller
+ * @data: data passed to algorithm
+ *
+ * Allocate the requested number of zeroed bytes from the specified pool. Uses
+ * the pool allocation function. 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_algo(struct gen_pool *pool, size_t size,
+		dma_addr_t *dma, genpool_algo_t algo, void *data)
+{
+	void *vaddr = gen_pool_dma_alloc_algo(pool, size, dma, algo, data);
 
 	if (vaddr)
 		memset(vaddr, 0, size);
 
 	return vaddr;
 }
-EXPORT_SYMBOL(gen_pool_dma_zalloc);
+EXPORT_SYMBOL(gen_pool_dma_zalloc_algo);
+
+/**
+ * gen_pool_dma_zalloc_align - allocate special zeroed memory from the pool for
+ * DMA usage with the given alignment
+ * @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.
+ * @align: alignment in bytes for starting address
+ *
+ * Allocate the requested number of zeroed bytes from the specified pool,
+ * with the given alignment restriction. 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_align(struct gen_pool *pool, size_t size,
+		dma_addr_t *dma, int align)
+{
+	struct genpool_data_align data = { .align = align };
+
+	return gen_pool_dma_zalloc_algo(pool, size, dma,
+			gen_pool_first_fit_align, &data);
+}
+EXPORT_SYMBOL(gen_pool_dma_zalloc_align);
 
 /**
  * gen_pool_free - free allocated special memory back to the pool

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

[Christoph Hellwig]

On Mon, Jun 24, 2019 at 02:59:16PM +0200, Fredrik Noring wrote:
> Hi Christoph,
> 
> > Can you send me the patch formally so that I can queue it up for the
> > dma-mapping tree?
> 
> That patch would be detrimental to local memory devices, as previously
> discussed, so I would like to suggest a much better approach, as shown below,
> where allocations are aligned as required but not necessarily much more than
> that.

This looks sensible to me.  Can you submit it with a proper patch
description and split into a separate patch for genalloc vs the user of
the new interface?

[PATCH 1/2] lib/genalloc.c: Add algorithm, align and zeroed family of DMA allocators

[Fredrik Noring]

Provide the algorithm option to DMA allocators as well, along with
convenience variants for zeroed and aligned memory. The following
four functions are added:

- gen_pool_dma_alloc_algo()
- gen_pool_dma_alloc_align()
- gen_pool_dma_zalloc_algo()
- gen_pool_dma_zalloc_align()

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

This patch is based on my v5.0.21 branch, with Laurentiu Tudor's other
local memory changes.

Fredrik
---
 include/linux/genalloc.h |  10 +++-
 lib/genalloc.c           | 100 +++++++++++++++++++++++++++++++++++++--
 2 files changed, 105 insertions(+), 5 deletions(-)

diff --git a/include/linux/genalloc.h b/include/linux/genalloc.h
--- a/include/linux/genalloc.h
+++ b/include/linux/genalloc.h
@@ -121,7 +121,15 @@ 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);
-void *gen_pool_dma_zalloc(struct gen_pool *pool, size_t size, dma_addr_t *dma);
+extern void *gen_pool_dma_alloc_algo(struct gen_pool *pool, size_t size,
+		dma_addr_t *dma, genpool_algo_t algo, void *data);
+extern void *gen_pool_dma_alloc_align(struct gen_pool *pool, size_t size,
+		dma_addr_t *dma, int align);
+extern void *gen_pool_dma_zalloc(struct gen_pool *pool, size_t size, dma_addr_t *dma);
+extern void *gen_pool_dma_zalloc_algo(struct gen_pool *pool, size_t size,
+		dma_addr_t *dma, genpool_algo_t algo, void *data);
+extern void *gen_pool_dma_zalloc_align(struct gen_pool *pool, size_t size,
+		dma_addr_t *dma, int align);
 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
@@ -347,13 +347,35 @@ EXPORT_SYMBOL(gen_pool_alloc_algo);
  * 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)
+{
+	return gen_pool_dma_alloc_algo(pool, size, dma, pool->algo, pool->data);
+}
+EXPORT_SYMBOL(gen_pool_dma_alloc);
+
+/**
+ * gen_pool_dma_alloc_algo - allocate special memory from the pool for DMA
+ * usage with the given pool algorithm
+ * @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.
+ * @algo: algorithm passed from caller
+ * @data: data passed to algorithm
+ *
+ * Allocate the requested number of bytes from the specified pool. Uses the
+ * given pool allocation function. 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_algo(struct gen_pool *pool, size_t size,
+		dma_addr_t *dma, genpool_algo_t algo, void *data)
 {
 	unsigned long vaddr;
 
 	if (!pool)
 		return NULL;
 
-	vaddr = gen_pool_alloc(pool, size);
+	vaddr = gen_pool_alloc_algo(pool, size, algo, data);
 	if (!vaddr)
 		return NULL;
 
@@ -362,7 +384,31 @@ void *gen_pool_dma_alloc(struct gen_pool *pool, size_t size, dma_addr_t *dma)
 
 	return (void *)vaddr;
 }
-EXPORT_SYMBOL(gen_pool_dma_alloc);
+EXPORT_SYMBOL(gen_pool_dma_alloc_algo);
+
+/**
+ * gen_pool_dma_alloc_align - allocate special memory from the pool for DMA
+ * usage with the given alignment
+ * @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.
+ * @align: alignment in bytes for starting address
+ *
+ * Allocate the requested number bytes from the specified pool, with the given
+ * alignment restriction. 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_align(struct gen_pool *pool, size_t size,
+		dma_addr_t *dma, int align)
+{
+	struct genpool_data_align data = { .align = align };
+
+	return gen_pool_dma_alloc_algo(pool, size, dma,
+			gen_pool_first_fit_align, &data);
+}
+EXPORT_SYMBOL(gen_pool_dma_alloc_align);
 
 /**
  * gen_pool_dma_zalloc - allocate special zeroed memory from the pool for
@@ -380,14 +426,60 @@ 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);
+	return gen_pool_dma_zalloc_algo(pool, size, dma, pool->algo, pool->data);
+}
+EXPORT_SYMBOL(gen_pool_dma_zalloc);
+
+/**
+ * gen_pool_dma_zalloc_algo - allocate special zeroed memory from the pool for
+ * DMA usage with the given pool algorithm
+ * @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.
+ * @algo: algorithm passed from caller
+ * @data: data passed to algorithm
+ *
+ * Allocate the requested number of zeroed bytes from the specified pool. Uses
+ * the given pool allocation function. 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_algo(struct gen_pool *pool, size_t size,
+		dma_addr_t *dma, genpool_algo_t algo, void *data)
+{
+	void *vaddr = gen_pool_dma_alloc_algo(pool, size, dma, algo, data);
 
 	if (vaddr)
 		memset(vaddr, 0, size);
 
 	return vaddr;
 }
-EXPORT_SYMBOL(gen_pool_dma_zalloc);
+EXPORT_SYMBOL(gen_pool_dma_zalloc_algo);
+
+/**
+ * gen_pool_dma_zalloc_align - allocate special zeroed memory from the pool for
+ * DMA usage with the given alignment
+ * @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.
+ * @align: alignment in bytes for starting address
+ *
+ * Allocate the requested number of zeroed bytes from the specified pool,
+ * with the given alignment restriction. 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_align(struct gen_pool *pool, size_t size,
+		dma_addr_t *dma, int align)
+{
+	struct genpool_data_align data = { .align = align };
+
+	return gen_pool_dma_zalloc_algo(pool, size, dma,
+			gen_pool_first_fit_align, &data);
+}
+EXPORT_SYMBOL(gen_pool_dma_zalloc_align);
 
 /**
  * gen_pool_free - free allocated special memory back to the pool
-- 
2.21.0

[PATCH 2/2] usb: host: Fix excessive alignment restriction for local memory allocations

[Fredrik Noring]

The PAGE_SHIFT alignment restriction to devm_gen_pool_create() quickly
exhaust local memory because most allocations are much smaller than
PAGE_SIZE. This causes USB device failures such as

	usb 1-2.1: reset full-speed USB device number 4 using sm501-usb
	sd 1:0:0:0: [sda] tag#0 UNKNOWN(0x2003) Result: hostbyte=0x03 driverbyte=0x00
	sd 1:0:0:0: [sda] tag#0 CDB: opcode=0x28 28 00 00 00 08 7c 00 00 f0 00
	print_req_error: I/O error, dev sda, sector 2172 flags 80700

when trying to boot from the SM501 USB controller on SH4 with QEMU.

Align allocations as required but not necessarily much more than that.
The HCCA, TD and ED structures align with 256, 32 and 16 byte memory
boundaries, as specified by the Open HCI[1]. The min_alloc_order argument
to devm_gen_pool_create is now somewhat arbitrarily set to 4 (16 bytes).
Perhaps it could be somewhat lower for general buffer allocations.

Reference:

[1] "Open Host Controller Interface Specification for USB",
    release 1.0a, Compaq, Microsoft, National Semiconductor, 1999,
    pp. 16, 19, 33.

Reported-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Fredrik Noring <noring@nocrew.org>
---
 drivers/usb/core/hcd.c      | 2 +-
 drivers/usb/host/ohci-hcd.c | 4 ++--
 drivers/usb/host/ohci-mem.c | 6 ++++--
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index b2362303d32f..48483fa71854 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -3014,7 +3014,7 @@ int usb_hcd_setup_local_mem(struct usb_hcd *hcd, phys_addr_t phys_addr,
 	int err;
 	void __iomem *local_mem;
 
-	hcd->localmem_pool = devm_gen_pool_create(hcd->self.sysdev, PAGE_SHIFT,
+	hcd->localmem_pool = devm_gen_pool_create(hcd->self.sysdev, 4,
 						  dev_to_node(hcd->self.sysdev),
 						  dev_name(hcd->self.sysdev));
 	if (IS_ERR(hcd->localmem_pool))
diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c
index 5801858d867e..b457fdaff297 100644
--- a/drivers/usb/host/ohci-hcd.c
+++ b/drivers/usb/host/ohci-hcd.c
@@ -507,9 +507,9 @@ static int ohci_init (struct ohci_hcd *ohci)
 	ohci->prev_frame_no = IO_WATCHDOG_OFF;
 
 	if (hcd->localmem_pool)
-		ohci->hcca = gen_pool_dma_alloc(hcd->localmem_pool,
+		ohci->hcca = gen_pool_dma_alloc_align(hcd->localmem_pool,
 						sizeof(*ohci->hcca),
-						&ohci->hcca_dma);
+						&ohci->hcca_dma, 256);
 	else
 		ohci->hcca = dma_alloc_coherent(hcd->self.controller,
 						sizeof(*ohci->hcca),
diff --git a/drivers/usb/host/ohci-mem.c b/drivers/usb/host/ohci-mem.c
index 4afe27cc7e46..1425335c6baf 100644
--- a/drivers/usb/host/ohci-mem.c
+++ b/drivers/usb/host/ohci-mem.c
@@ -94,7 +94,8 @@ td_alloc (struct ohci_hcd *hc, gfp_t mem_flags)
 	struct usb_hcd	*hcd = ohci_to_hcd(hc);
 
 	if (hcd->localmem_pool)
-		td = gen_pool_dma_zalloc(hcd->localmem_pool, sizeof(*td), &dma);
+		td = gen_pool_dma_zalloc_align(hcd->localmem_pool,
+				sizeof(*td), &dma, 32);
 	else
 		td = dma_pool_zalloc(hc->td_cache, mem_flags, &dma);
 	if (td) {
@@ -137,7 +138,8 @@ ed_alloc (struct ohci_hcd *hc, gfp_t mem_flags)
 	struct usb_hcd	*hcd = ohci_to_hcd(hc);
 
 	if (hcd->localmem_pool)
-		ed = gen_pool_dma_zalloc(hcd->localmem_pool, sizeof(*ed), &dma);
+		ed = gen_pool_dma_zalloc_align(hcd->localmem_pool,
+				sizeof(*ed), &dma, 16);
 	else
 		ed = dma_pool_zalloc(hc->ed_cache, mem_flags, &dma);
 	if (ed) {
-- 
2.21.0

Re: [PATCH 2/2] usb: host: Fix excessive alignment restriction for local memory allocations

[Guenter Roeck]

On Tue, Jun 25, 2019 at 05:08:23PM +0200, Fredrik Noring wrote:
> The PAGE_SHIFT alignment restriction to devm_gen_pool_create() quickly
> exhaust local memory because most allocations are much smaller than
> PAGE_SIZE. This causes USB device failures such as
> 
> 	usb 1-2.1: reset full-speed USB device number 4 using sm501-usb
> 	sd 1:0:0:0: [sda] tag#0 UNKNOWN(0x2003) Result: hostbyte=0x03 driverbyte=0x00
> 	sd 1:0:0:0: [sda] tag#0 CDB: opcode=0x28 28 00 00 00 08 7c 00 00 f0 00
> 	print_req_error: I/O error, dev sda, sector 2172 flags 80700
> 
> when trying to boot from the SM501 USB controller on SH4 with QEMU.
> 
> Align allocations as required but not necessarily much more than that.
> The HCCA, TD and ED structures align with 256, 32 and 16 byte memory
> boundaries, as specified by the Open HCI[1]. The min_alloc_order argument
> to devm_gen_pool_create is now somewhat arbitrarily set to 4 (16 bytes).
> Perhaps it could be somewhat lower for general buffer allocations.
> 
> Reference:
> 
> [1] "Open Host Controller Interface Specification for USB",
>     release 1.0a, Compaq, Microsoft, National Semiconductor, 1999,
>     pp. 16, 19, 33.
> 
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Fredrik Noring <noring@nocrew.org>

Tested-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  drivers/usb/core/hcd.c      | 2 +-
>  drivers/usb/host/ohci-hcd.c | 4 ++--
>  drivers/usb/host/ohci-mem.c | 6 ++++--
>  3 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index b2362303d32f..48483fa71854 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -3014,7 +3014,7 @@ int usb_hcd_setup_local_mem(struct usb_hcd *hcd, phys_addr_t phys_addr,
>  	int err;
>  	void __iomem *local_mem;
>  
> -	hcd->localmem_pool = devm_gen_pool_create(hcd->self.sysdev, PAGE_SHIFT,
> +	hcd->localmem_pool = devm_gen_pool_create(hcd->self.sysdev, 4,
>  						  dev_to_node(hcd->self.sysdev),
>  						  dev_name(hcd->self.sysdev));
>  	if (IS_ERR(hcd->localmem_pool))
> diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c
> index 5801858d867e..b457fdaff297 100644
> --- a/drivers/usb/host/ohci-hcd.c
> +++ b/drivers/usb/host/ohci-hcd.c
> @@ -507,9 +507,9 @@ static int ohci_init (struct ohci_hcd *ohci)
>  	ohci->prev_frame_no = IO_WATCHDOG_OFF;
>  
>  	if (hcd->localmem_pool)
> -		ohci->hcca = gen_pool_dma_alloc(hcd->localmem_pool,
> +		ohci->hcca = gen_pool_dma_alloc_align(hcd->localmem_pool,
>  						sizeof(*ohci->hcca),
> -						&ohci->hcca_dma);
> +						&ohci->hcca_dma, 256);
>  	else
>  		ohci->hcca = dma_alloc_coherent(hcd->self.controller,
>  						sizeof(*ohci->hcca),
> diff --git a/drivers/usb/host/ohci-mem.c b/drivers/usb/host/ohci-mem.c
> index 4afe27cc7e46..1425335c6baf 100644
> --- a/drivers/usb/host/ohci-mem.c
> +++ b/drivers/usb/host/ohci-mem.c
> @@ -94,7 +94,8 @@ td_alloc (struct ohci_hcd *hc, gfp_t mem_flags)
>  	struct usb_hcd	*hcd = ohci_to_hcd(hc);
>  
>  	if (hcd->localmem_pool)
> -		td = gen_pool_dma_zalloc(hcd->localmem_pool, sizeof(*td), &dma);
> +		td = gen_pool_dma_zalloc_align(hcd->localmem_pool,
> +				sizeof(*td), &dma, 32);
>  	else
>  		td = dma_pool_zalloc(hc->td_cache, mem_flags, &dma);
>  	if (td) {
> @@ -137,7 +138,8 @@ ed_alloc (struct ohci_hcd *hc, gfp_t mem_flags)
>  	struct usb_hcd	*hcd = ohci_to_hcd(hc);
>  
>  	if (hcd->localmem_pool)
> -		ed = gen_pool_dma_zalloc(hcd->localmem_pool, sizeof(*ed), &dma);
> +		ed = gen_pool_dma_zalloc_align(hcd->localmem_pool,
> +				sizeof(*ed), &dma, 16);
>  	else
>  		ed = dma_pool_zalloc(hc->ed_cache, mem_flags, &dma);
>  	if (ed) {
> -- 
> 2.21.0
> 

Re: [PATCH 1/2] lib/genalloc.c: Add algorithm, align and zeroed family of DMA allocators

[Guenter Roeck]

On Tue, Jun 25, 2019 at 05:05:58PM +0200, Fredrik Noring wrote:
> Provide the algorithm option to DMA allocators as well, along with
> convenience variants for zeroed and aligned memory. The following
> four functions are added:
> 
> - gen_pool_dma_alloc_algo()
> - gen_pool_dma_alloc_align()
> - gen_pool_dma_zalloc_algo()
> - gen_pool_dma_zalloc_align()
> 
> Signed-off-by: Fredrik Noring <noring@nocrew.org>

The series fixes the problem I had observed in linux-next.

Tested-by: Guenter Roeck <linux@roeck-us.net>

Guenter

> ---
> Hi Christoph,
> 
> This patch is based on my v5.0.21 branch, with Laurentiu Tudor's other
> local memory changes.
> 
> Fredrik
> ---
>  include/linux/genalloc.h |  10 +++-
>  lib/genalloc.c           | 100 +++++++++++++++++++++++++++++++++++++--
>  2 files changed, 105 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/genalloc.h b/include/linux/genalloc.h
> --- a/include/linux/genalloc.h
> +++ b/include/linux/genalloc.h
> @@ -121,7 +121,15 @@ 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);
> -void *gen_pool_dma_zalloc(struct gen_pool *pool, size_t size, dma_addr_t *dma);
> +extern void *gen_pool_dma_alloc_algo(struct gen_pool *pool, size_t size,
> +		dma_addr_t *dma, genpool_algo_t algo, void *data);
> +extern void *gen_pool_dma_alloc_align(struct gen_pool *pool, size_t size,
> +		dma_addr_t *dma, int align);
> +extern void *gen_pool_dma_zalloc(struct gen_pool *pool, size_t size, dma_addr_t *dma);
> +extern void *gen_pool_dma_zalloc_algo(struct gen_pool *pool, size_t size,
> +		dma_addr_t *dma, genpool_algo_t algo, void *data);
> +extern void *gen_pool_dma_zalloc_align(struct gen_pool *pool, size_t size,
> +		dma_addr_t *dma, int align);
>  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
> @@ -347,13 +347,35 @@ EXPORT_SYMBOL(gen_pool_alloc_algo);
>   * 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)
> +{
> +	return gen_pool_dma_alloc_algo(pool, size, dma, pool->algo, pool->data);
> +}
> +EXPORT_SYMBOL(gen_pool_dma_alloc);
> +
> +/**
> + * gen_pool_dma_alloc_algo - allocate special memory from the pool for DMA
> + * usage with the given pool algorithm
> + * @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.
> + * @algo: algorithm passed from caller
> + * @data: data passed to algorithm
> + *
> + * Allocate the requested number of bytes from the specified pool. Uses the
> + * given pool allocation function. 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_algo(struct gen_pool *pool, size_t size,
> +		dma_addr_t *dma, genpool_algo_t algo, void *data)
>  {
>  	unsigned long vaddr;
>  
>  	if (!pool)
>  		return NULL;
>  
> -	vaddr = gen_pool_alloc(pool, size);
> +	vaddr = gen_pool_alloc_algo(pool, size, algo, data);
>  	if (!vaddr)
>  		return NULL;
>  
> @@ -362,7 +384,31 @@ void *gen_pool_dma_alloc(struct gen_pool *pool, size_t size, dma_addr_t *dma)
>  
>  	return (void *)vaddr;
>  }
> -EXPORT_SYMBOL(gen_pool_dma_alloc);
> +EXPORT_SYMBOL(gen_pool_dma_alloc_algo);
> +
> +/**
> + * gen_pool_dma_alloc_align - allocate special memory from the pool for DMA
> + * usage with the given alignment
> + * @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.
> + * @align: alignment in bytes for starting address
> + *
> + * Allocate the requested number bytes from the specified pool, with the given
> + * alignment restriction. 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_align(struct gen_pool *pool, size_t size,
> +		dma_addr_t *dma, int align)
> +{
> +	struct genpool_data_align data = { .align = align };
> +
> +	return gen_pool_dma_alloc_algo(pool, size, dma,
> +			gen_pool_first_fit_align, &data);
> +}
> +EXPORT_SYMBOL(gen_pool_dma_alloc_align);
>  
>  /**
>   * gen_pool_dma_zalloc - allocate special zeroed memory from the pool for
> @@ -380,14 +426,60 @@ 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);
> +	return gen_pool_dma_zalloc_algo(pool, size, dma, pool->algo, pool->data);
> +}
> +EXPORT_SYMBOL(gen_pool_dma_zalloc);
> +
> +/**
> + * gen_pool_dma_zalloc_algo - allocate special zeroed memory from the pool for
> + * DMA usage with the given pool algorithm
> + * @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.
> + * @algo: algorithm passed from caller
> + * @data: data passed to algorithm
> + *
> + * Allocate the requested number of zeroed bytes from the specified pool. Uses
> + * the given pool allocation function. 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_algo(struct gen_pool *pool, size_t size,
> +		dma_addr_t *dma, genpool_algo_t algo, void *data)
> +{
> +	void *vaddr = gen_pool_dma_alloc_algo(pool, size, dma, algo, data);
>  
>  	if (vaddr)
>  		memset(vaddr, 0, size);
>  
>  	return vaddr;
>  }
> -EXPORT_SYMBOL(gen_pool_dma_zalloc);
> +EXPORT_SYMBOL(gen_pool_dma_zalloc_algo);
> +
> +/**
> + * gen_pool_dma_zalloc_align - allocate special zeroed memory from the pool for
> + * DMA usage with the given alignment
> + * @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.
> + * @align: alignment in bytes for starting address
> + *
> + * Allocate the requested number of zeroed bytes from the specified pool,
> + * with the given alignment restriction. 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_align(struct gen_pool *pool, size_t size,
> +		dma_addr_t *dma, int align)
> +{
> +	struct genpool_data_align data = { .align = align };
> +
> +	return gen_pool_dma_zalloc_algo(pool, size, dma,
> +			gen_pool_first_fit_align, &data);
> +}
> +EXPORT_SYMBOL(gen_pool_dma_zalloc_align);
>  
>  /**
>   * gen_pool_free - free allocated special memory back to the pool
> -- 
> 2.21.0
> 

Re: [PATCH 1/2] lib/genalloc.c: Add algorithm, align and zeroed family of DMA allocators

[Christoph Hellwig]

Thanks,

I've added both patches to the dma-mapping for-next tree.

RE: [PATCH v7 3/5] usb: host: ohci-sm501: init genalloc for local memory

[Laurentiu Tudor]

Hello,

> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Thursday, June 13, 2019 9:06 PM
> 
> On 6/13/19 8:34 AM, Fredrik Noring wrote:
> > Hi Guenter,
> >
> >> Thanks for the confirmation. Do you see the problem only with the
> >> ohci-sm501 driver or also with others ?
> >
> > All are likely affected, but it depends, because I believe the problem
> is
> > that the USB subsystem runs out of memory. Please try the attached patch!
> >
> > The pool assumed 4096 byte page alignment for every allocation, which is
> > excessive given that many requests are for 16 and 32 bytes. In the patch
> > below, I have turned down the order to 5, which is good enough for the
> ED
> > and TD structures of the OHCI, but not enough for the HCCA that needs
> 256
> > byte alignment. With some luck, the WARN_ON_ONCE will not trigger in
> your
> > test, though. If it does, you may try to increase the order from 5 to 8.
> >
> 
> You are right, the patch below fixes the problem. I did not get the
> warning
> with order==5. Nevertheless, I also tested with order==8; that works as
> well.

Sorry for the late reply, I was OOO for past week+ and many thanks for taking a look at this.
So if my understanding is correct, an order of PAGE_SHIFT is too large and leads to waste of memory and in the end to an out of memory condition. This leaves me wondering what a safe order value would be.
I'll try to look into the legacy dma coherent allocation code maybe I can gather some info on the subject.

---
Best Regards, Laurentiu

> 
> > I have observed strange things happen when the USB subsystem runs out of
> > memory. The mass storage drivers often seem to busy-wait on -ENOMEM,
> > consuming a lot of processor resources. It would be much more efficient
> > to sleep waiting for memory to become available.
> >
> > In your case I suspect that allocation failures are not correctly
> > attributed. Certain kinds of temporary freezes may also occur, as the
> > various devices are reset due to host memory allocation errors.
> >  > Fredrik
> >
> > diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> > --- a/drivers/usb/core/hcd.c
> > +++ b/drivers/usb/core/hcd.c
> > @@ -3011,7 +3011,7 @@ int usb_hcd_setup_local_mem(struct usb_hcd *hcd,
> phys_addr_t phys_addr,
> >   	int err;
> >   	void __iomem *local_mem;
> >
> > -	hcd->localmem_pool = devm_gen_pool_create(hcd->self.sysdev,
> PAGE_SHIFT,
> > +	hcd->localmem_pool = devm_gen_pool_create(hcd->self.sysdev, 5,
> >   						  dev_to_node(hcd->self.sysdev),
> >   						  dev_name(hcd->self.sysdev));
> >   	if (IS_ERR(hcd->localmem_pool))
> > diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c
> > --- a/drivers/usb/host/ohci-hcd.c
> > +++ b/drivers/usb/host/ohci-hcd.c
> > @@ -517,6 +517,7 @@ static int ohci_init (struct ohci_hcd *ohci)
> >   						GFP_KERNEL);
> >   	if (!ohci->hcca)
> >   		return -ENOMEM;
> > +	WARN_ON_ONCE(ohci->hcca_dma & 0xff);
> >
> >   	if ((ret = ohci_mem_init (ohci)) < 0)
> >   		ohci_stop (hcd);
> >

[PATCH v7 4/5] 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 | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/host/ohci-tmio.c b/drivers/usb/host/ohci-tmio.c
index f88a0370659f..3b84ce0c3f29 100644
--- a/drivers/usb/host/ohci-tmio.c
+++ b/drivers/usb/host/ohci-tmio.c
@@ -224,11 +224,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 +234,11 @@ static int ohci_hcd_tmio_drv_probe(struct platform_device *dev)
 	ohci = hcd_to_ohci(hcd);
 	ohci_hcd_init(ohci);
 
+	ret = usb_hcd_setup_local_mem(hcd, sram->start, sram->start,
+				      resource_size(sram));
+	if (ret < 0)
+		goto err_enable;
+
 	ret = usb_add_hcd(hcd, irq, 0);
 	if (ret)
 		goto err_add_hcd;
@@ -254,8 +254,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 +274,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

[PATCH v7 5/5] USB: drop HCD_LOCAL_MEM flag

[laurentiu.tudor]

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

With the addition of the local memory allocator, the HCD_LOCAL_MEM
flag can be dropped and the checks against it replaced with a check
for the localmem_pool ptr being initialized.

Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com>
Tested-by: Fredrik Noring <noring@nocrew.org>
---
 drivers/usb/core/buffer.c      |  8 +++-----
 drivers/usb/core/hcd.c         | 15 ++++++---------
 drivers/usb/host/ehci-hcd.c    |  2 +-
 drivers/usb/host/fotg210-hcd.c |  2 +-
 drivers/usb/host/ohci-hcd.c    |  2 +-
 drivers/usb/host/ohci-sm501.c  |  5 +++--
 drivers/usb/host/ohci-tmio.c   |  2 +-
 drivers/usb/host/uhci-hcd.c    |  2 +-
 include/linux/usb/hcd.h        |  1 -
 9 files changed, 17 insertions(+), 22 deletions(-)

diff --git a/drivers/usb/core/buffer.c b/drivers/usb/core/buffer.c
index d2064ad7ad14..1359b78a624e 100644
--- a/drivers/usb/core/buffer.c
+++ b/drivers/usb/core/buffer.c
@@ -68,7 +68,7 @@ int hcd_buffer_create(struct usb_hcd *hcd)
 
 	if (!IS_ENABLED(CONFIG_HAS_DMA) ||
 	    (!is_device_dma_capable(hcd->self.sysdev) &&
-	     !(hcd->driver->flags & HCD_LOCAL_MEM)))
+	     !hcd->localmem_pool))
 		return 0;
 
 	for (i = 0; i < HCD_BUFFER_POOLS; i++) {
@@ -130,8 +130,7 @@ void *hcd_buffer_alloc(
 
 	/* 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);
 	}
@@ -162,8 +161,7 @@ void hcd_buffer_free(
 	}
 
 	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/core/hcd.c b/drivers/usb/core/hcd.c
index 29b96e5e8621..fe631d18c1ed 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1347,14 +1347,14 @@ EXPORT_SYMBOL_GPL(usb_hcd_unlink_urb_from_ep);
  * using regular system memory - like pci devices doing bus mastering.
  *
  * To support host controllers with limited dma capabilities we provide dma
- * bounce buffers. This feature can be enabled using the HCD_LOCAL_MEM flag.
+ * bounce buffers. This feature can be enabled by initializing
+ * hcd->localmem_pool using usb_hcd_setup_local_mem().
  * For this to work properly the host controller code must first use the
  * function dma_declare_coherent_memory() to point out which memory area
  * that should be used for dma allocations.
  *
- * The HCD_LOCAL_MEM flag then tells the usb code to allocate all data for
- * dma using dma_alloc_coherent() which in turn allocates from the memory
- * area pointed out with dma_declare_coherent_memory().
+ * The initialized hcd->localmem_pool then tells the usb code to allocate all
+ * data for dma using the genalloc API.
  *
  * So, to summarize...
  *
@@ -1364,9 +1364,6 @@ EXPORT_SYMBOL_GPL(usb_hcd_unlink_urb_from_ep);
  *   (a) "normal" kernel memory is no good, and
  *   (b) there's not enough to share
  *
- * - The only *portable* hook for such stuff in the
- *   DMA framework is dma_declare_coherent_memory()
- *
  * - So we use that, even though the primary requirement
  *   is that the memory be "local" (hence addressable
  *   by that device), not "coherent".
@@ -1533,7 +1530,7 @@ int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
 						urb->setup_dma))
 				return -EAGAIN;
 			urb->transfer_flags |= URB_SETUP_MAP_SINGLE;
-		} else if (hcd->driver->flags & HCD_LOCAL_MEM) {
+		} else if (hcd->localmem_pool) {
 			ret = hcd_alloc_coherent(
 					urb->dev->bus, mem_flags,
 					&urb->setup_dma,
@@ -1603,7 +1600,7 @@ int usb_hcd_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
 				else
 					urb->transfer_flags |= URB_DMA_MAP_SINGLE;
 			}
-		} else if (hcd->driver->flags & HCD_LOCAL_MEM) {
+		} else if (hcd->localmem_pool) {
 			ret = hcd_alloc_coherent(
 					urb->dev->bus, mem_flags,
 					&urb->transfer_dma,
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index cdafa97f632d..9da7e22848c9 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -559,7 +559,7 @@ static int ehci_init(struct usb_hcd *hcd)
 	ehci->command = temp;
 
 	/* Accept arbitrarily long scatter-gather lists */
-	if (!(hcd->driver->flags & HCD_LOCAL_MEM))
+	if (!hcd->localmem_pool)
 		hcd->self.sg_tablesize = ~0;
 
 	/* Prepare for unlinking active QHs */
diff --git a/drivers/usb/host/fotg210-hcd.c b/drivers/usb/host/fotg210-hcd.c
index 0da68df259c8..5d74ff61fa4c 100644
--- a/drivers/usb/host/fotg210-hcd.c
+++ b/drivers/usb/host/fotg210-hcd.c
@@ -4995,7 +4995,7 @@ static int hcd_fotg210_init(struct usb_hcd *hcd)
 	fotg210->command = temp;
 
 	/* Accept arbitrarily long scatter-gather lists */
-	if (!(hcd->driver->flags & HCD_LOCAL_MEM))
+	if (!hcd->localmem_pool)
 		hcd->self.sg_tablesize = ~0;
 	return 0;
 }
diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c
index b200b19b44fa..5801858d867e 100644
--- a/drivers/usb/host/ohci-hcd.c
+++ b/drivers/usb/host/ohci-hcd.c
@@ -448,7 +448,7 @@ static int ohci_init (struct ohci_hcd *ohci)
 	struct usb_hcd *hcd = ohci_to_hcd(ohci);
 
 	/* Accept arbitrarily long scatter-gather lists */
-	if (!(hcd->driver->flags & HCD_LOCAL_MEM))
+	if (!hcd->localmem_pool)
 		hcd->self.sg_tablesize = ~0;
 
 	if (distrust_firmware)
diff --git a/drivers/usb/host/ohci-sm501.c b/drivers/usb/host/ohci-sm501.c
index b710e100aec9..c158cda9e4b9 100644
--- a/drivers/usb/host/ohci-sm501.c
+++ b/drivers/usb/host/ohci-sm501.c
@@ -49,7 +49,7 @@ static const struct hc_driver ohci_sm501_hc_driver = {
 	 * generic hardware linkage
 	 */
 	.irq =			ohci_irq,
-	.flags =		HCD_USB11 | HCD_MEMORY | HCD_LOCAL_MEM,
+	.flags =		HCD_USB11 | HCD_MEMORY,
 
 	/*
 	 * basic lifecycle operations
@@ -153,7 +153,8 @@ static int ohci_hcd_sm501_drv_probe(struct platform_device *pdev)
 	 * 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.
+	 * regular memory. A non-null hcd->localmem_pool initialized by the
+	 * the call to usb_hcd_setup_local_mem() below does just that.
 	 */
 
 	if (usb_hcd_setup_local_mem(hcd, mem->start,
diff --git a/drivers/usb/host/ohci-tmio.c b/drivers/usb/host/ohci-tmio.c
index 3b84ce0c3f29..d5a293a707b6 100644
--- a/drivers/usb/host/ohci-tmio.c
+++ b/drivers/usb/host/ohci-tmio.c
@@ -153,7 +153,7 @@ static const struct hc_driver ohci_tmio_hc_driver = {
 
 	/* generic hardware linkage */
 	.irq =			ohci_irq,
-	.flags =		HCD_USB11 | HCD_MEMORY | HCD_LOCAL_MEM,
+	.flags =		HCD_USB11 | HCD_MEMORY,
 
 	/* basic lifecycle operations */
 	.start =		ohci_tmio_start,
diff --git a/drivers/usb/host/uhci-hcd.c b/drivers/usb/host/uhci-hcd.c
index 98deb5f64268..03bc59755123 100644
--- a/drivers/usb/host/uhci-hcd.c
+++ b/drivers/usb/host/uhci-hcd.c
@@ -581,7 +581,7 @@ static int uhci_start(struct usb_hcd *hcd)
 
 	hcd->uses_new_polling = 1;
 	/* Accept arbitrarily long scatter-gather lists */
-	if (!(hcd->driver->flags & HCD_LOCAL_MEM))
+	if (!hcd->localmem_pool)
 		hcd->self.sg_tablesize = ~0;
 
 	spin_lock_init(&uhci->lock);
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 127560a4bfa0..bab27ccc8ff5 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -256,7 +256,6 @@ struct hc_driver {
 
 	int	flags;
 #define	HCD_MEMORY	0x0001		/* HC regs use memory (else I/O) */
-#define	HCD_LOCAL_MEM	0x0002		/* HC needs local memory */
 #define	HCD_SHARED	0x0004		/* Two (or more) usb_hcds share HW */
 #define	HCD_USB11	0x0010		/* USB 1.1 */
 #define	HCD_USB2	0x0020		/* USB 2.0 */
-- 
2.17.1

Re: [PATCH v7 0/5] prerequisites for device reserved local mem rework

[Greg KH]

On Wed, May 29, 2019 at 01:28:38PM +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.
> 
> 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

All look good to me:

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Christoph, this is going through your tree, right?

thanks,

greg k-h

Re: [PATCH v7 0/5] prerequisites for device reserved local mem rework

[Christoph Hellwig]

On Wed, May 29, 2019 at 04:34:27AM -0700, Greg KH wrote:
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> Christoph, this is going through your tree, right?

Yes, I'll pіck it up.

Re: [PATCH v7 0/5] prerequisites for device reserved local mem rework

[Laurentiu Tudor]

Hi Christoph,

On 29.05.2019 14:37, Christoph Hellwig wrote:
> On Wed, May 29, 2019 at 04:34:27AM -0700, Greg KH wrote:
>> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>
>> Christoph, this is going through your tree, right?
> 
> Yes, I'll pіck it up.
> 

Thanks, hope this time everything is fine.
When you get the time, please let me know your ideas on the next steps.

---
Best Regards, Laurentiu

Re: [PATCH v7 0/5] prerequisites for device reserved local mem rework

[Christoph Hellwig]

On Wed, May 29, 2019 at 02:06:12PM +0000, Laurentiu Tudor wrote:
> Thanks, hope this time everything is fine.

I've applied it to the dma-mapping tree now.

> When you get the time, please let me know your ideas on the next steps.

I think the next step is to move the call to
dma_alloc_from_dev_coherent from dma_alloc_attrs into the ->alloc
instances.  The only onces that really need it for now are the
generic and legacy arm dma-direct code, and drivers/iommu/dma-iommu.c
as well as the ARM DMA API code, as those are the ones use for
architectures that declare coherent regions.  The other iommus are
not used on OF platforms (at least that's what my analysis said a while
ago, feel free to double check it)

RE: [PATCH v7 0/5] prerequisites for device reserved local mem rework

[Laurentiu Tudor]

> -----Original Message-----
> From: Christoph Hellwig <hch@lst.de>
> Sent: Friday, May 31, 2019 7:43 PM
> 
> On Wed, May 29, 2019 at 02:06:12PM +0000, Laurentiu Tudor wrote:
> > Thanks, hope this time everything is fine.
> 
> I've applied it to the dma-mapping tree now.
> 
> > When you get the time, please let me know your ideas on the next steps.
> 
> I think the next step is to move the call to
> dma_alloc_from_dev_coherent from dma_alloc_attrs into the ->alloc
> instances.  The only onces that really need it for now are the
> generic and legacy arm dma-direct code, and drivers/iommu/dma-iommu.c
> as well as the ARM DMA API code, as those are the ones use for
> architectures that declare coherent regions.  The other iommus are
> not used on OF platforms (at least that's what my analysis said a while
> ago, feel free to double check it)

Thanks. I'll start looking into it starting next week. Keep in touch.

---
Best Regards, Laurentiu

Re: [PATCH v7 0/5] prerequisites for device reserved local mem rework

[Laurentiu Tudor]

On 31.05.2019 19:43, Christoph Hellwig wrote:
> On Wed, May 29, 2019 at 02:06:12PM +0000, Laurentiu Tudor wrote:
>> Thanks, hope this time everything is fine.
> 
> I've applied it to the dma-mapping tree now.
> 
>> When you get the time, please let me know your ideas on the next steps.
> 
> I think the next step is to move the call to
> dma_alloc_from_dev_coherent from dma_alloc_attrs into the ->alloc
> instances.  The only onces that really need it for now are the
> generic and legacy arm dma-direct code, and drivers/iommu/dma-iommu.c
> as well as the ARM DMA API code, as those are the ones use for
> architectures that declare coherent regions.  The other iommus are
> not used on OF platforms (at least that's what my analysis said a while
> ago, feel free to double check it)

Thanks for the details. I'll be OOO next week so probably I'll start 
looking into it next next week.

---
Best Regards, Laurentiu