2019-05-21 14:10:51

by Laurentiu Tudor

[permalink] [raw]
Subject: [PATCH v5 0/5] prerequisites for device reserved local mem rework

From: Laurentiu Tudor <[email protected]>

For HCs that have local memory, replace the current DMA API usage
with a genalloc generic allocator to manage the mappings for these
devices.
This is in preparation for dropping the existing "coherent" dma
mem declaration APIs. Current implementation was relying on a short
circuit in the DMA API that in the end, was acting as an allocator
for these type of devices.

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 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/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 | 34 ++++++++++++++++++++---
drivers/usb/host/ohci-sm501.c | 50 +++++++++++++++------------------
drivers/usb/host/ohci-tmio.c | 15 ++++------
drivers/usb/host/uhci-hcd.c | 2 +-
include/linux/genalloc.h | 1 +
include/linux/usb/hcd.h | 6 +++-
lib/genalloc.c | 29 ++++++++++++++++++-
12 files changed, 168 insertions(+), 66 deletions(-)

--
2.17.1



2019-05-21 14:11:38

by Laurentiu Tudor

[permalink] [raw]
Subject: [PATCH v5 2/5] USB: use genalloc for USB HCs with local memory

From: Laurentiu Tudor <[email protected]>

For HCs that have local memory, replace the current DMA API usage
with a genalloc generic allocator to manage the mappings for these
devices. 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 context, see thread here: https://lkml.org/lkml/2019/4/22/357

Signed-off-by: Laurentiu Tudor <[email protected]>
Signed-off-by: Fredrik Noring <[email protected]>
---
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 | 34 ++++++++++++++++++++++++++++++----
include/linux/usb/hcd.h | 5 +++++
5 files changed, 98 insertions(+), 9 deletions(-)

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..a1aa54f2db20 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 __iomem *local_mem;
+
+ hcd->localmem_pool = devm_gen_pool_create(hcd->self.sysdev, PAGE_SHIFT,
+ dev_to_node(hcd->self.sysdev),
+ "ohci-hcd");
+ 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..1805f934ee97 100644
--- a/drivers/usb/host/ohci-mem.c
+++ b/drivers/usb/host/ohci-mem.c
@@ -36,6 +36,12 @@ static void ohci_hcd_init (struct ohci_hcd *ohci)

static int ohci_mem_init (struct ohci_hcd *ohci)
{
+ if (ohci_to_hcd(ohci)->localmem_pool) {
+ ohci->td_cache = NULL;
+ ohci->ed_cache = NULL;
+ return 0;
+ }
+
ohci->td_cache = dma_pool_create ("ohci_td",
ohci_to_hcd(ohci)->self.controller,
sizeof (struct td),
@@ -84,8 +90,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 +109,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 +117,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 +133,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 +149,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/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index bb57b5af4700..292016553760 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -216,6 +216,9 @@ struct usb_hcd {
#define HC_IS_RUNNING(state) ((state) & __ACTIVE)
#define HC_IS_SUSPENDED(state) ((state) & __SUSPEND)

+ /* allocator for HCs having local memory */
+ struct gen_pool *localmem_pool;
+
/* more shared queuing code would be good; it should support
* smarter scheduling, handle transaction translators, etc;
* input size of periodic table to an interrupt scheduler.
@@ -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


2019-05-21 15:30:15

by Fredrik Noring

[permalink] [raw]
Subject: Re: [PATCH v5 2/5] USB: use genalloc for USB HCs with local memory

Thanks Laurentiu!

> --- a/include/linux/usb/hcd.h
> +++ b/include/linux/usb/hcd.h
> @@ -216,6 +216,9 @@ struct usb_hcd {
> #define HC_IS_RUNNING(state) ((state) & __ACTIVE)
> #define HC_IS_SUSPENDED(state) ((state) & __SUSPEND)
>
> + /* allocator for HCs having local memory */
> + struct gen_pool *localmem_pool;
> +

I have tested patches 1, 2 and 5, and they seem to work. Nice! May I
suggest clarifying the NULL pointers? I think that may help someone
debugging or maintaining this in the future. Something like this:

--- 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;
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -211,7 +211,7 @@ struct usb_hcd {
#define HC_IS_RUNNING(state) ((state) & __ACTIVE)
#define HC_IS_SUSPENDED(state) ((state) & __SUSPEND)

- /* allocator for HCs having local memory */
+ /* allocator for HCs having local memory, or %NULL */
struct gen_pool *localmem_pool;

/* more shared queuing code would be good; it should support

Fredrik

2019-05-21 17:22:04

by Alan Stern

[permalink] [raw]
Subject: Re: [PATCH v5 2/5] USB: use genalloc for USB HCs with local memory

On Tue, 21 May 2019 [email protected] wrote:

> From: Laurentiu Tudor <[email protected]>
>
> For HCs that have local memory, replace the current DMA API usage
> with a genalloc generic allocator to manage the mappings for these
> devices. 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 context, see thread here: https://lkml.org/lkml/2019/4/22/357
>
> Signed-off-by: Laurentiu Tudor <[email protected]>
> Signed-off-by: Fredrik Noring <[email protected]>
> ---

> --- 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 __iomem *local_mem;
> +
> + hcd->localmem_pool = devm_gen_pool_create(hcd->self.sysdev, PAGE_SHIFT,
> + dev_to_node(hcd->self.sysdev),
> + "ohci-hcd");

Surely that string is a mistake. You could use
dev_name(hcd->self.sysdev) or a name passed by the caller.

> + 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 you have a usb_hcd_setup_local_mem() function then you should also
have a usb_hcd_remove_local_mem() function.

> --- a/drivers/usb/host/ohci-mem.c
> +++ b/drivers/usb/host/ohci-mem.c
> @@ -36,6 +36,12 @@ static void ohci_hcd_init (struct ohci_hcd *ohci)
>
> static int ohci_mem_init (struct ohci_hcd *ohci)
> {
> + if (ohci_to_hcd(ohci)->localmem_pool) {
> + ohci->td_cache = NULL;
> + ohci->ed_cache = NULL;
> + return 0;
> + }

This really isn't necessary. The entire ohci_hcd structure is
initialized to 0 when it is first allocated.

> --- a/include/linux/usb/hcd.h
> +++ b/include/linux/usb/hcd.h
> @@ -216,6 +216,9 @@ struct usb_hcd {
> #define HC_IS_RUNNING(state) ((state) & __ACTIVE)
> #define HC_IS_SUSPENDED(state) ((state) & __SUSPEND)
>
> + /* allocator for HCs having local memory */

"allocator" is the wrong word -- an allocator is something that
allocates. Perhaps "memory area" or something along those lines.

Alan Stern


2019-05-22 10:47:13

by Laurentiu Tudor

[permalink] [raw]
Subject: Re: [PATCH v5 2/5] USB: use genalloc for USB HCs with local memory

Hello Alan,

On 21.05.2019 20:20, Alan Stern wrote:
> On Tue, 21 May 2019 [email protected] wrote:
>
>> From: Laurentiu Tudor <[email protected]>
>>
>> For HCs that have local memory, replace the current DMA API usage
>> with a genalloc generic allocator to manage the mappings for these
>> devices. 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 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%7Cdc4a76b04b874b75bd5408d6de109d86%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636940560281957207&amp;sdata=JQ1iJc%2Fw%2BwHdTFvaOT%2FYHBx%2BVQlpHWlf346q7sclfcg%3D&amp;reserved=0
>>
>> Signed-off-by: Laurentiu Tudor <[email protected]>
>> Signed-off-by: Fredrik Noring <[email protected]>
>> ---
>
>> --- 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 __iomem *local_mem;
>> +
>> + hcd->localmem_pool = devm_gen_pool_create(hcd->self.sysdev, PAGE_SHIFT,
>> + dev_to_node(hcd->self.sysdev),
>> + "ohci-hcd");
>
> Surely that string is a mistake. You could use
> dev_name(hcd->self.sysdev) or a name passed by the caller.

Sorry, missed that. Will go with dev_name() if that's ok with you.

>> + 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 you have a usb_hcd_setup_local_mem() function then you should also
> have a usb_hcd_remove_local_mem() function.

Even if all resources that are allocated are device managed?

>> --- a/drivers/usb/host/ohci-mem.c
>> +++ b/drivers/usb/host/ohci-mem.c
>> @@ -36,6 +36,12 @@ static void ohci_hcd_init (struct ohci_hcd *ohci)
>>
>> static int ohci_mem_init (struct ohci_hcd *ohci)
>> {
>> + if (ohci_to_hcd(ohci)->localmem_pool) {
>> + ohci->td_cache = NULL;
>> + ohci->ed_cache = NULL;
>> + return 0;
>> + }
>
> This really isn't necessary. The entire ohci_hcd structure is
> initialized to 0 when it is first allocated.

Will drop.

>> --- a/include/linux/usb/hcd.h
>> +++ b/include/linux/usb/hcd.h
>> @@ -216,6 +216,9 @@ struct usb_hcd {
>> #define HC_IS_RUNNING(state) ((state) & __ACTIVE)
>> #define HC_IS_SUSPENDED(state) ((state) & __SUSPEND)
>>
>> + /* allocator for HCs having local memory */
>
> "allocator" is the wrong word -- an allocator is something that
> allocates. Perhaps "memory area" or something along those lines.

Alright, I will rework.

---
Thanks & Best Regards, Laurentiu

2019-05-22 10:56:25

by Laurentiu Tudor

[permalink] [raw]
Subject: Re: [PATCH v5 2/5] USB: use genalloc for USB HCs with local memory


On 21.05.2019 18:27, Fredrik Noring wrote:
> Thanks Laurentiu!
>
>> --- a/include/linux/usb/hcd.h
>> +++ b/include/linux/usb/hcd.h
>> @@ -216,6 +216,9 @@ struct usb_hcd {
>> #define HC_IS_RUNNING(state) ((state) & __ACTIVE)
>> #define HC_IS_SUSPENDED(state) ((state) & __SUSPEND)
>>
>> + /* allocator for HCs having local memory */
>> + struct gen_pool *localmem_pool;
>> +
>
> I have tested patches 1, 2 and 5, and they seem to work. Nice!

Great! Thanks for testing. I'll add your Tested-By tags on the patches
you mentioned.

> May I suggest clarifying the NULL pointers? I think that may help someone
> debugging or maintaining this in the future. Something like this:

Sure, will include in v6.

---
Best Regards, Laurentiu

2019-05-22 14:59:35

by Alan Stern

[permalink] [raw]
Subject: Re: [PATCH v5 2/5] USB: use genalloc for USB HCs with local memory

On Wed, 22 May 2019, Laurentiu Tudor wrote:

> >> +EXPORT_SYMBOL_GPL(usb_hcd_setup_local_mem);
> >
> > If you have a usb_hcd_setup_local_mem() function then you should also
> > have a usb_hcd_remove_local_mem() function.
>
> Even if all resources that are allocated are device managed?

Ah, I missed that fact. Okay, that's all right.

Alan Stern