[U-Boot] [PATCH v2 00/14] Devres (Managed Device Resource) for U-Boot
Please refer to the commit message of 06/14 for what this series wants to do.
Masahiro Yamada (14): x86: delete unneeded declarations of disable_irq() and enable_irq() linux_compat: remove cpu_relax() define linux_compat: move vzalloc() to header file as an inline function linux_compat: handle __GFP_ZERO in kmalloc() dm: add DM_FLAG_BOUND flag devres: introduce Devres (Managed Device Resource) framework devres: add devm_kmalloc() and friends (managed memory allocators) dm: refactor device_bind() and device_unbind() with devm_kzalloc() dm: merge fail_alloc labels linux_compat: introduce GFP_DMA flag for kmalloc() dm: refactor device_probe() and device_remove() with devm_kzalloc() devres: add debug command to dump device resources dm: remove redundant CONFIG_DM from driver/core/Makefile devres: compile Devres iif CONFIG_DM_DEVICE_REMOVE is enabled
arch/x86/include/asm/interrupt.h | 4 - drivers/core/Kconfig | 16 ++ drivers/core/Makefile | 4 +- drivers/core/device-remove.c | 41 +---- drivers/core/device.c | 64 +++----- drivers/core/devres.c | 258 ++++++++++++++++++++++++++++++ drivers/usb/musb-new/musb_gadget_ep0.c | 1 + include/dm/device-internal.h | 32 ++++ include/dm/device.h | 280 +++++++++++++++++++++++++++++++-- include/linux/compat.h | 29 ++-- lib/linux_compat.c | 19 +-- 11 files changed, 634 insertions(+), 114 deletions(-) create mode 100644 drivers/core/devres.c
These two declarations in arch/x86/include/asm/interrupt.h conflict with ones in include/linux/compat.h, so x86 boards cannot include <linux/compat.h>.
The comment /* arch/x86/lib/interrupts.c */ is bogus now, and we do not see any definitions of disable_irq() and enable_irq() in there.
Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com Reviewed-by: Bin Meng bmeng.cn@gmail.com Acked-by: Simon Glass sjg@chromium.org ---
Changes in v2: None
arch/x86/include/asm/interrupt.h | 4 ---- 1 file changed, 4 deletions(-)
diff --git a/arch/x86/include/asm/interrupt.h b/arch/x86/include/asm/interrupt.h index 0a75f89..00cbe07 100644 --- a/arch/x86/include/asm/interrupt.h +++ b/arch/x86/include/asm/interrupt.h @@ -16,10 +16,6 @@ /* arch/x86/cpu/interrupts.c */ void set_vector(u8 intnum, void *routine);
-/* arch/x86/lib/interrupts.c */ -void disable_irq(int irq); -void enable_irq(int irq); - /* Architecture specific functions */ void mask_irq(int irq); void unmask_irq(int irq);
On 12 July 2015 at 22:17, Masahiro Yamada yamada.masahiro@socionext.com wrote:
These two declarations in arch/x86/include/asm/interrupt.h conflict with ones in include/linux/compat.h, so x86 boards cannot include <linux/compat.h>.
The comment /* arch/x86/lib/interrupts.c */ is bogus now, and we do not see any definitions of disable_irq() and enable_irq() in there.
Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com Reviewed-by: Bin Meng bmeng.cn@gmail.com Acked-by: Simon Glass sjg@chromium.org
Changes in v2: None
arch/x86/include/asm/interrupt.h | 4 ---- 1 file changed, 4 deletions(-)
Applied to u-boot-dm, thanks!
The macro cpu_relax() is defined by several headers in different ways.
arch/{arm,avr32,mips}/include/asm/processor.h defines it as follows: #define cpu_relax() barrier()
On the other hand, include/linux/compat.h defines it as follows: #define cpu_relax() do {} while (0)
If both headers are included from the same source file, the warning warning: "cpu_relax" redefined [enabled by default] is displayed.
It effectively makes it impossible to include <linux/compat.h> from some sources. Drop the latter.
Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com Reviewed-by: Heiko Schocher hs@denx.de Acked-by: Simon Glass sjg@chromium.org ---
Changes in v2: None
drivers/usb/musb-new/musb_gadget_ep0.c | 1 + include/linux/compat.h | 2 -- 2 files changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/usb/musb-new/musb_gadget_ep0.c b/drivers/usb/musb-new/musb_gadget_ep0.c index 5a71501..415a9f2 100644 --- a/drivers/usb/musb-new/musb_gadget_ep0.c +++ b/drivers/usb/musb-new/musb_gadget_ep0.c @@ -43,6 +43,7 @@ #else #include <common.h> #include "linux-compat.h" +#include <asm/processor.h> #endif
#include "musb_core.h" diff --git a/include/linux/compat.h b/include/linux/compat.h index 6ff3915..da1420f 100644 --- a/include/linux/compat.h +++ b/include/linux/compat.h @@ -315,8 +315,6 @@ struct notifier_block {};
typedef unsigned long dmaaddr_t;
-#define cpu_relax() do {} while (0) - #define pm_runtime_get_sync(dev) do {} while (0) #define pm_runtime_put(dev) do {} while (0) #define pm_runtime_put_sync(dev) do {} while (0)
Hi Masahiro,
The macro cpu_relax() is defined by several headers in different ways.
arch/{arm,avr32,mips}/include/asm/processor.h defines it as follows: #define cpu_relax() barrier()
On the other hand, include/linux/compat.h defines it as follows: #define cpu_relax() do {} while (0)
If both headers are included from the same source file, the warning warning: "cpu_relax" redefined [enabled by default] is displayed.
It effectively makes it impossible to include <linux/compat.h> from some sources. Drop the latter.
Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com Reviewed-by: Heiko Schocher hs@denx.de Acked-by: Simon Glass sjg@chromium.org
Changes in v2: None
drivers/usb/musb-new/musb_gadget_ep0.c | 1 + include/linux/compat.h | 2 -- 2 files changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/usb/musb-new/musb_gadget_ep0.c b/drivers/usb/musb-new/musb_gadget_ep0.c index 5a71501..415a9f2 100644 --- a/drivers/usb/musb-new/musb_gadget_ep0.c +++ b/drivers/usb/musb-new/musb_gadget_ep0.c @@ -43,6 +43,7 @@ #else #include <common.h> #include "linux-compat.h" +#include <asm/processor.h> #endif
#include "musb_core.h" diff --git a/include/linux/compat.h b/include/linux/compat.h index 6ff3915..da1420f 100644 --- a/include/linux/compat.h +++ b/include/linux/compat.h @@ -315,8 +315,6 @@ struct notifier_block {};
typedef unsigned long dmaaddr_t;
-#define cpu_relax() do {} while (0)
#define pm_runtime_get_sync(dev) do {} while (0) #define pm_runtime_put(dev) do {} while (0) #define pm_runtime_put_sync(dev) do {} while (0)
Reviewed-by: Lukasz Majewski l.majewski@samsung.com
On 13 July 2015 at 01:38, Lukasz Majewski l.majewski@samsung.com wrote:
Hi Masahiro,
The macro cpu_relax() is defined by several headers in different ways.
arch/{arm,avr32,mips}/include/asm/processor.h defines it as follows: #define cpu_relax() barrier()
On the other hand, include/linux/compat.h defines it as follows: #define cpu_relax() do {} while (0)
If both headers are included from the same source file, the warning warning: "cpu_relax" redefined [enabled by default] is displayed.
It effectively makes it impossible to include <linux/compat.h> from some sources. Drop the latter.
Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com Reviewed-by: Heiko Schocher hs@denx.de Acked-by: Simon Glass sjg@chromium.org
Changes in v2: None
drivers/usb/musb-new/musb_gadget_ep0.c | 1 + include/linux/compat.h | 2 -- 2 files changed, 1 insertion(+), 2 deletions(-)
Applied to u-boot-dm, thanks!
The vzalloc(size) is equivalent to kzalloc(size, 0). Move it to include/linux/compat.h as an inline function in order to avoid the function call overhead.
Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com Reviewed-by: Heiko Schocher hs@denx.de Acked-by: Simon Glass sjg@chromium.org ---
Changes in v2: None
include/linux/compat.h | 6 ++++-- lib/linux_compat.c | 5 ----- 2 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/include/linux/compat.h b/include/linux/compat.h index da1420f..a3d136b 100644 --- a/include/linux/compat.h +++ b/include/linux/compat.h @@ -40,6 +40,10 @@ void *kmalloc(size_t size, int flags); void *kzalloc(size_t size, int flags); #define vmalloc(size) kmalloc(size, 0) #define __vmalloc(size, flags, pgsz) kmalloc(size, flags) +static inline void *vzalloc(unsigned long size) +{ + return kzalloc(size, 0); +} #define kfree(ptr) free(ptr) #define vfree(ptr) free(ptr)
@@ -189,8 +193,6 @@ struct work_struct {}; unsigned long copy_from_user(void *dest, const void *src, unsigned long count);
-void *vzalloc(unsigned long size); - typedef unused_t spinlock_t; typedef int wait_queue_head_t;
diff --git a/lib/linux_compat.c b/lib/linux_compat.c index a3d4675..8c7a7b5 100644 --- a/lib/linux_compat.c +++ b/lib/linux_compat.c @@ -26,11 +26,6 @@ void *kzalloc(size_t size, int flags) return ptr; }
-void *vzalloc(unsigned long size) -{ - return kzalloc(size, 0); -} - struct kmem_cache *get_mem(int element_sz) { struct kmem_cache *ret;
On 12 July 2015 at 22:17, Masahiro Yamada yamada.masahiro@socionext.com wrote:
The vzalloc(size) is equivalent to kzalloc(size, 0). Move it to include/linux/compat.h as an inline function in order to avoid the function call overhead.
Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com Reviewed-by: Heiko Schocher hs@denx.de Acked-by: Simon Glass sjg@chromium.org
Changes in v2: None
include/linux/compat.h | 6 ++++-- lib/linux_compat.c | 5 ----- 2 files changed, 4 insertions(+), 7 deletions(-)
Applied to u-boot-dm, thanks!
Currently, kzalloc() returns zero-filled memory, while kmalloc() simply ignores the second argument and never fills the memory area with zeros.
I want kmalloc(size, __GFP_ZERO) to behave as kzalloc() does, which will make it easier to add more memory allocator variants.
With the introduction of __GFP_ZERO flag, going forward, kzmalloc() variants can fall back to kmalloc() enabling the __GFP_ZERO flag.
Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com Reviewed-by: Heiko Schocher hs@denx.de Acked-by: Simon Glass sjg@chromium.org ---
Changes in v2: None
include/linux/compat.h | 20 ++++++++++++-------- lib/linux_compat.c | 13 ++++++------- 2 files changed, 18 insertions(+), 15 deletions(-)
diff --git a/include/linux/compat.h b/include/linux/compat.h index a3d136b..fbebf91 100644 --- a/include/linux/compat.h +++ b/include/linux/compat.h @@ -36,8 +36,19 @@ extern struct p_current *current; #define KERN_INFO #define KERN_DEBUG
+#define GFP_ATOMIC ((gfp_t) 0) +#define GFP_KERNEL ((gfp_t) 0) +#define GFP_NOFS ((gfp_t) 0) +#define GFP_USER ((gfp_t) 0) +#define __GFP_NOWARN ((gfp_t) 0) +#define __GFP_ZERO ((__force gfp_t)0x8000u) /* Return zeroed page on success */ + void *kmalloc(size_t size, int flags); -void *kzalloc(size_t size, int flags); + +static inline void *kzalloc(size_t size, gfp_t flags) +{ + return kmalloc(size, flags | __GFP_ZERO); +} #define vmalloc(size) kmalloc(size, 0) #define __vmalloc(size, flags, pgsz) kmalloc(size, flags) static inline void *vzalloc(unsigned long size) @@ -77,13 +88,6 @@ void *kmem_cache_alloc(struct kmem_cache *obj, int flag); /* drivers/char/random.c */ #define get_random_bytes(...)
-/* idr.c */ -#define GFP_ATOMIC ((gfp_t) 0) -#define GFP_KERNEL ((gfp_t) 0) -#define GFP_NOFS ((gfp_t) 0) -#define GFP_USER ((gfp_t) 0) -#define __GFP_NOWARN ((gfp_t) 0) - /* include/linux/leds.h */ struct led_trigger {};
diff --git a/lib/linux_compat.c b/lib/linux_compat.c index 8c7a7b5..a936a7e 100644 --- a/lib/linux_compat.c +++ b/lib/linux_compat.c @@ -16,14 +16,13 @@ unsigned long copy_from_user(void *dest, const void *src,
void *kmalloc(size_t size, int flags) { - return memalign(ARCH_DMA_MINALIGN, size); -} + void *p;
-void *kzalloc(size_t size, int flags) -{ - void *ptr = kmalloc(size, flags); - memset(ptr, 0, size); - return ptr; + p = memalign(ARCH_DMA_MINALIGN, size); + if (flags & __GFP_ZERO) + memset(p, 0, size); + + return p; }
struct kmem_cache *get_mem(int element_sz)
Hi Masahiro,
Currently, kzalloc() returns zero-filled memory, while kmalloc() simply ignores the second argument and never fills the memory area with zeros.
I want kmalloc(size, __GFP_ZERO) to behave as kzalloc() does, which will make it easier to add more memory allocator variants.
With the introduction of __GFP_ZERO flag, going forward, kzmalloc() variants can fall back to kmalloc() enabling the __GFP_ZERO flag.
Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com Reviewed-by: Heiko Schocher hs@denx.de Acked-by: Simon Glass sjg@chromium.org
Changes in v2: None
include/linux/compat.h | 20 ++++++++++++-------- lib/linux_compat.c | 13 ++++++------- 2 files changed, 18 insertions(+), 15 deletions(-)
diff --git a/include/linux/compat.h b/include/linux/compat.h index a3d136b..fbebf91 100644 --- a/include/linux/compat.h +++ b/include/linux/compat.h @@ -36,8 +36,19 @@ extern struct p_current *current; #define KERN_INFO #define KERN_DEBUG
+#define GFP_ATOMIC ((gfp_t) 0) +#define GFP_KERNEL ((gfp_t) 0) +#define GFP_NOFS ((gfp_t) 0) +#define GFP_USER ((gfp_t) 0) +#define __GFP_NOWARN ((gfp_t) 0) +#define __GFP_ZERO ((__force gfp_t)0x8000u) /* Return zeroed page on success */ + void *kmalloc(size_t size, int flags); -void *kzalloc(size_t size, int flags);
+static inline void *kzalloc(size_t size, gfp_t flags) +{
- return kmalloc(size, flags | __GFP_ZERO);
+} #define vmalloc(size) kmalloc(size, 0) #define __vmalloc(size, flags, pgsz) kmalloc(size, flags) static inline void *vzalloc(unsigned long size) @@ -77,13 +88,6 @@ void *kmem_cache_alloc(struct kmem_cache *obj, int flag); /* drivers/char/random.c */ #define get_random_bytes(...)
-/* idr.c */ -#define GFP_ATOMIC ((gfp_t) 0) -#define GFP_KERNEL ((gfp_t) 0) -#define GFP_NOFS ((gfp_t) 0) -#define GFP_USER ((gfp_t) 0) -#define __GFP_NOWARN ((gfp_t) 0)
/* include/linux/leds.h */ struct led_trigger {};
diff --git a/lib/linux_compat.c b/lib/linux_compat.c index 8c7a7b5..a936a7e 100644 --- a/lib/linux_compat.c +++ b/lib/linux_compat.c @@ -16,14 +16,13 @@ unsigned long copy_from_user(void *dest, const void *src, void *kmalloc(size_t size, int flags) {
- return memalign(ARCH_DMA_MINALIGN, size);
-}
- void *p;
-void *kzalloc(size_t size, int flags) -{
- void *ptr = kmalloc(size, flags);
- memset(ptr, 0, size);
- return ptr;
- p = memalign(ARCH_DMA_MINALIGN, size);
- if (flags & __GFP_ZERO)
memset(p, 0, size);- return p;
}
struct kmem_cache *get_mem(int element_sz)
Reviewed-by: Lukasz Majewski l.majewski@samsung.com
On 13 July 2015 at 01:47, Lukasz Majewski l.majewski@samsung.com wrote:
Hi Masahiro,
Currently, kzalloc() returns zero-filled memory, while kmalloc() simply ignores the second argument and never fills the memory area with zeros.
I want kmalloc(size, __GFP_ZERO) to behave as kzalloc() does, which will make it easier to add more memory allocator variants.
With the introduction of __GFP_ZERO flag, going forward, kzmalloc() variants can fall back to kmalloc() enabling the __GFP_ZERO flag.
Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com Reviewed-by: Heiko Schocher hs@denx.de Acked-by: Simon Glass sjg@chromium.org
Changes in v2: None
include/linux/compat.h | 20 ++++++++++++-------- lib/linux_compat.c | 13 ++++++------- 2 files changed, 18 insertions(+), 15 deletions(-)
Applied to u-boot-dm, thanks!
Currently, we only have DM_FLAG_ACTIVATED to indicate the device status, but we still cannot know in which stage is in progress, binding or probing.
This commit introduces a new flag, DM_FLAG_BOUND, which is set when the device is really bound, and cleared when it is unbound.
Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com ---
Changes in v2: None
drivers/core/device-remove.c | 3 +++ drivers/core/device.c | 2 ++ include/dm/device.h | 3 +++ 3 files changed, 8 insertions(+)
diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c index 6a16b4f..20b56f9 100644 --- a/drivers/core/device-remove.c +++ b/drivers/core/device-remove.c @@ -75,6 +75,9 @@ int device_unbind(struct udevice *dev) if (dev->flags & DM_FLAG_ACTIVATED) return -EINVAL;
+ if (!(dev->flags & DM_FLAG_BOUND)) + return -EINVAL; + drv = dev->driver; assert(drv);
diff --git a/drivers/core/device.c b/drivers/core/device.c index 46bf388..39cc2f3 100644 --- a/drivers/core/device.c +++ b/drivers/core/device.c @@ -132,6 +132,8 @@ int device_bind(struct udevice *parent, const struct driver *drv, dm_dbg("Bound device %s to %s\n", dev->name, parent->name); *devp = dev;
+ dev->flags |= DM_FLAG_BOUND; + return 0;
fail_child_post_bind: diff --git a/include/dm/device.h b/include/dm/device.h index 18296bb..3674d19 100644 --- a/include/dm/device.h +++ b/include/dm/device.h @@ -36,6 +36,9 @@ struct driver_info; /* Allocate driver private data on a DMA boundary */ #define DM_FLAG_ALLOC_PRIV_DMA (1 << 5)
+/* Device is bound */ +#define DM_FLAG_BOUND (1 << 6) + /** * struct udevice - An instance of a driver *
In U-Boot's driver model, memory is basically allocated and freed in the core framework. So, low level drivers generally only have to specify the size of needed memory with .priv_auto_alloc_size, .platdata_auto_alloc_size, etc. Nevertheless, some drivers still need to allocate memory on their own in case they cannot statically know how much memory is needed. Moreover, I am afraid the failure paths of driver model core parts are getting messier as more and more memory size members are supported, .per_child_auto_alloc_size, .per_child_platdata_auto_alloc_size... So, I believe it is reasonable enough to port Devres into U-boot.
As you know, Devres, which originates in Linux, manages device resources for each device and automatically releases them on driver detach. With devres, device resources are guaranteed to be freed whether initialization fails half-way or the device gets detached.
The basic idea is totally the same to that of Linux, but I tweaked it a bit so that it fits in U-Boot's driver model.
In U-Boot, drivers are activated in two steps: binding and probing. Binding puts a driver and a device together. It is just data manipulation on the system memory, so nothing has happened on the hardware device at this moment. When the device is really used, it is probed. Probing initializes the real hardware device to make it really ready for use.
So, the resources acquired during the probing process must be freed when the device is removed. Likewise, what has been allocated in binding should be released when the device is unbound. The struct devres has a member "probe" to remember when the resource was allocated.
CONFIG_DEBUG_DEVRES is also supported for easier debugging. If enabled, debug messages are printed each time a resource is allocated/freed.
Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com ---
Changes in v2: - Add more APIs: _free, _find, _get, _remove, _destroy, _release - Move devres_release_probe() and devres_release_all() decrlarations to dm/device-internal.h - Move comments to headers
drivers/core/Kconfig | 10 +++ drivers/core/Makefile | 2 +- drivers/core/device-remove.c | 5 ++ drivers/core/device.c | 3 + drivers/core/devres.c | 187 +++++++++++++++++++++++++++++++++++++++++++ include/dm/device-internal.h | 19 +++++ include/dm/device.h | 140 ++++++++++++++++++++++++++++++++ 7 files changed, 365 insertions(+), 1 deletion(-) create mode 100644 drivers/core/devres.c
diff --git a/drivers/core/Kconfig b/drivers/core/Kconfig index 2861b43..5966801 100644 --- a/drivers/core/Kconfig +++ b/drivers/core/Kconfig @@ -55,3 +55,13 @@ config DM_SEQ_ALIAS Most boards will have a '/aliases' node containing the path to numbered devices (e.g. serial0 = &serial0). This feature can be disabled if it is not required, to save code space in SPL. + +config DEBUG_DEVRES + bool "Managed device resources verbose debug messages" + depends on DM + help + If this option is enabled, devres debug messages are printed. + Select this if you are having a problem with devres or want to + debug resource management for a managed device. + + If you are unsure about this, Say N here. diff --git a/drivers/core/Makefile b/drivers/core/Makefile index a3fec38..cd8c104 100644 --- a/drivers/core/Makefile +++ b/drivers/core/Makefile @@ -4,7 +4,7 @@ # SPDX-License-Identifier: GPL-2.0+ #
-obj-$(CONFIG_DM) += device.o lists.o root.o uclass.o util.o +obj-$(CONFIG_DM) += device.o lists.o root.o uclass.o util.o devres.o ifndef CONFIG_SPL_BUILD obj-$(CONFIG_OF_CONTROL) += simple-bus.o endif diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c index 20b56f9..e1714b2 100644 --- a/drivers/core/device-remove.c +++ b/drivers/core/device-remove.c @@ -109,6 +109,9 @@ int device_unbind(struct udevice *dev)
if (dev->parent) list_del(&dev->sibling_node); + + devres_release_all(dev); + free(dev);
return 0; @@ -142,6 +145,8 @@ void device_free(struct udevice *dev) dev->parent_priv = NULL; } } + + devres_release_probe(dev); }
int device_remove(struct udevice *dev) diff --git a/drivers/core/device.c b/drivers/core/device.c index 39cc2f3..83b47d8 100644 --- a/drivers/core/device.c +++ b/drivers/core/device.c @@ -47,6 +47,7 @@ int device_bind(struct udevice *parent, const struct driver *drv, INIT_LIST_HEAD(&dev->sibling_node); INIT_LIST_HEAD(&dev->child_head); INIT_LIST_HEAD(&dev->uclass_node); + INIT_LIST_HEAD(&dev->devres_head); dev->platdata = platdata; dev->name = name; dev->of_offset = of_offset; @@ -170,6 +171,8 @@ fail_alloc2: dev->platdata = NULL; } fail_alloc1: + devres_release_all(dev); + free(dev);
return ret; diff --git a/drivers/core/devres.c b/drivers/core/devres.c new file mode 100644 index 0000000..e7330b3 --- /dev/null +++ b/drivers/core/devres.c @@ -0,0 +1,187 @@ +/* + * Copyright (C) 2015 Masahiro Yamada yamada.masahiro@socionext.com + * + * Based on the original work in Linux by + * Copyright (c) 2006 SUSE Linux Products GmbH + * Copyright (c) 2006 Tejun Heo teheo@suse.de + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +#include <common.h> +#include <linux/compat.h> +#include <linux/kernel.h> +#include <linux/list.h> +#include <dm/device.h> + +struct devres { + struct list_head entry; + dr_release_t release; + bool probe; +#ifdef CONFIG_DEBUG_DEVRES + const char *name; + size_t size; +#endif + unsigned long long data[]; +}; + +#ifdef CONFIG_DEBUG_DEVRES + +static void set_node_dbginfo(struct devres *dr, const char *name, size_t size) +{ + dr->name = name; + dr->size = size; +} + +static void devres_log(struct udevice *dev, struct devres *dr, + const char *op) +{ + printf("%s: DEVRES %3s %p %s (%lu bytes)\n", + dev->name, op, dr, dr->name, (unsigned long)dr->size); +} +#else /* CONFIG_DEBUG_DEVRES */ +#define set_node_dbginfo(dr, n, s) do {} while (0) +#define devres_log(dev, dr, op) do {} while (0) +#endif + +#if CONFIG_DEBUG_DEVRES +void *__devres_alloc(dr_release_t release, size_t size, gfp_t gfp, + const char *name) +#else +void *_devres_alloc(dr_release_t release, size_t size, gfp_t gfp) +#endif +{ + size_t tot_size = sizeof(struct devres) + size; + struct devres *dr; + + dr = kmalloc(tot_size, gfp); + if (unlikely(!dr)) + return NULL; + + INIT_LIST_HEAD(&dr->entry); + dr->release = release; + set_node_dbginfo(dr, name, size); + + return dr->data; +} + +void devres_free(void *res) +{ + if (res) { + struct devres *dr = container_of(res, struct devres, data); + + BUG_ON(!list_empty(&dr->entry)); + kfree(dr); + } +} + +void devres_add(struct udevice *dev, void *res) +{ + struct devres *dr = container_of(res, struct devres, data); + + devres_log(dev, dr, "ADD"); + BUG_ON(!list_empty(&dr->entry)); + dr->probe = dev->flags & DM_FLAG_BOUND ? true : false; + list_add_tail(&dr->entry, &dev->devres_head); +} + +void *devres_find(struct udevice *dev, dr_release_t release, + dr_match_t match, void *match_data) +{ + struct devres *dr; + + list_for_each_entry_reverse(dr, &dev->devres_head, entry) { + if (dr->release != release) + continue; + if (match && !match(dev, dr->data, match_data)) + continue; + return dr->data; + } + + return NULL; +} + +void *devres_get(struct udevice *dev, void *new_res, + dr_match_t match, void *match_data) +{ + struct devres *new_dr = container_of(new_res, struct devres, data); + void *res; + + res = devres_find(dev, new_dr->release, match, match_data); + if (!res) { + devres_add(dev, new_res); + res = new_res; + new_res = NULL; + } + devres_free(new_res); + + return res; +} + +void *devres_remove(struct udevice *dev, dr_release_t release, + dr_match_t match, void *match_data) +{ + void *res; + + res = devres_find(dev, release, match, match_data); + if (res) { + struct devres *dr = container_of(res, struct devres, data); + + list_del_init(&dr->entry); + devres_log(dev, dr, "REM"); + } + + return res; +} + +int devres_destroy(struct udevice *dev, dr_release_t release, + dr_match_t match, void *match_data) +{ + void *res; + + res = devres_remove(dev, release, match, match_data); + if (unlikely(!res)) + return -ENOENT; + + devres_free(res); + return 0; +} + +int devres_release(struct udevice *dev, dr_release_t release, + dr_match_t match, void *match_data) +{ + void *res; + + res = devres_remove(dev, release, match, match_data); + if (unlikely(!res)) + return -ENOENT; + + (*release)(dev, res); + devres_free(res); + return 0; +} + +static void release_nodes(struct udevice *dev, struct list_head *head, + bool probe_only) +{ + struct devres *dr, *tmp; + + list_for_each_entry_safe_reverse(dr, tmp, head, entry) { + if (probe_only && !dr->probe) + break; + devres_log(dev, dr, "REL"); + dr->release(dev, dr->data); + list_del(&dr->entry); + kfree(dr); + } +} + +void devres_release_probe(struct udevice *dev) +{ + release_nodes(dev, &dev->devres_head, true); +} + +void devres_release_all(struct udevice *dev) +{ + release_nodes(dev, &dev->devres_head, false); +} diff --git a/include/dm/device-internal.h b/include/dm/device-internal.h index 687462b..409c687 100644 --- a/include/dm/device-internal.h +++ b/include/dm/device-internal.h @@ -117,4 +117,23 @@ static inline void device_free(struct udevice *dev) {} #define DM_ROOT_NON_CONST (((gd_t *)gd)->dm_root) #define DM_UCLASS_ROOT_NON_CONST (((gd_t *)gd)->uclass_root)
+/* device resource management */ +/** + * devres_release_probe - Release managed resources allocated after probing + * @dev: Device to release resources for + * + * Release all resources allocated for @dev when it was probed or later. + * This function is called on driver removal. + */ +void devres_release_probe(struct udevice *dev); + +/** + * devres_release_all - Release all managed resources + * @dev: Device to release resources for + * + * Release all resources associated with @dev. This function is + * called on driver unbinding. + */ +void devres_release_all(struct udevice *dev); + #endif diff --git a/include/dm/device.h b/include/dm/device.h index 3674d19..c266c7d 100644 --- a/include/dm/device.h +++ b/include/dm/device.h @@ -96,6 +96,7 @@ struct udevice { uint32_t flags; int req_seq; int seq; + struct list_head devres_head; };
/* Maximum sequence number supported */ @@ -449,4 +450,143 @@ bool device_has_active_children(struct udevice *dev); */ bool device_is_last_sibling(struct udevice *dev);
+/* device resource management */ +typedef void (*dr_release_t)(struct udevice *dev, void *res); +typedef int (*dr_match_t)(struct udevice *dev, void *res, void *match_data); + +#ifdef CONFIG_DEBUG_DEVRES +void *__devres_alloc(dr_release_t release, size_t size, gfp_t gfp, + const char *name); +#define _devres_alloc(release, size, gfp) \ + __devres_alloc(release, size, gfp, #release) +#else +void *_devres_alloc(dr_release_t release, size_t size, gfp_t gfp); +#endif + +/** + * devres_alloc - Allocate device resource data + * @release: Release function devres will be associated with + * @size: Allocation size + * @gfp: Allocation flags + * + * Allocate devres of @size bytes. The allocated area is associated + * with @release. The returned pointer can be passed to + * other devres_*() functions. + * + * RETURNS: + * Pointer to allocated devres on success, NULL on failure. + */ +#define devres_alloc(release, size, gfp) \ + _devres_alloc(release, size, gfp | __GFP_ZERO) + +/** + * devres_free - Free device resource data + * @res: Pointer to devres data to free + * + * Free devres created with devres_alloc(). + */ +void devres_free(void *res); + +/** + * devres_add - Register device resource + * @dev: Device to add resource to + * @res: Resource to register + * + * Register devres @res to @dev. @res should have been allocated + * using devres_alloc(). On driver detach, the associated release + * function will be invoked and devres will be freed automatically. + */ +void devres_add(struct udevice *dev, void *res); + +/** + * devres_find - Find device resource + * @dev: Device to lookup resource from + * @release: Look for resources associated with this release function + * @match: Match function (optional) + * @match_data: Data for the match function + * + * Find the latest devres of @dev which is associated with @release + * and for which @match returns 1. If @match is NULL, it's considered + * to match all. + * + * RETURNS: + * Pointer to found devres, NULL if not found. + */ +void *devres_find(struct udevice *dev, dr_release_t release, + dr_match_t match, void *match_data); + +/** + * devres_get - Find devres, if non-existent, add one atomically + * @dev: Device to lookup or add devres for + * @new_res: Pointer to new initialized devres to add if not found + * @match: Match function (optional) + * @match_data: Data for the match function + * + * Find the latest devres of @dev which has the same release function + * as @new_res and for which @match return 1. If found, @new_res is + * freed; otherwise, @new_res is added atomically. + * + * RETURNS: + * Pointer to found or added devres. + */ +void *devres_get(struct udevice *dev, void *new_res, + dr_match_t match, void *match_data); + +/** + * devres_remove - Find a device resource and remove it + * @dev: Device to find resource from + * @release: Look for resources associated with this release function + * @match: Match function (optional) + * @match_data: Data for the match function + * + * Find the latest devres of @dev associated with @release and for + * which @match returns 1. If @match is NULL, it's considered to + * match all. If found, the resource is removed atomically and + * returned. + * + * RETURNS: + * Pointer to removed devres on success, NULL if not found. + */ +void *devres_remove(struct udevice *dev, dr_release_t release, + dr_match_t match, void *match_data); + +/** + * devres_destroy - Find a device resource and destroy it + * @dev: Device to find resource from + * @release: Look for resources associated with this release function + * @match: Match function (optional) + * @match_data: Data for the match function + * + * Find the latest devres of @dev associated with @release and for + * which @match returns 1. If @match is NULL, it's considered to + * match all. If found, the resource is removed atomically and freed. + * + * Note that the release function for the resource will not be called, + * only the devres-allocated data will be freed. The caller becomes + * responsible for freeing any other data. + * + * RETURNS: + * 0 if devres is found and freed, -ENOENT if not found. + */ +int devres_destroy(struct udevice *dev, dr_release_t release, + dr_match_t match, void *match_data); + +/** + * devres_release - Find a device resource and destroy it, calling release + * @dev: Device to find resource from + * @release: Look for resources associated with this release function + * @match: Match function (optional) + * @match_data: Data for the match function + * + * Find the latest devres of @dev associated with @release and for + * which @match returns 1. If @match is NULL, it's considered to + * match all. If found, the resource is removed atomically, the + * release function called and the resource freed. + * + * RETURNS: + * 0 if devres is found and freed, -ENOENT if not found. + */ +int devres_release(struct udevice *dev, dr_release_t release, + dr_match_t match, void *match_data); + #endif
+Albert
Hi Masahiro,
On 12 July 2015 at 22:17, Masahiro Yamada yamada.masahiro@socionext.com wrote:
In U-Boot's driver model, memory is basically allocated and freed in the core framework. So, low level drivers generally only have to specify the size of needed memory with .priv_auto_alloc_size, .platdata_auto_alloc_size, etc. Nevertheless, some drivers still need to allocate memory on their own in case they cannot statically know how much memory is needed. Moreover, I am afraid the failure paths of driver model core parts are getting messier as more and more memory size members are supported, .per_child_auto_alloc_size, .per_child_platdata_auto_alloc_size... So, I believe it is reasonable enough to port Devres into U-boot.
As you know, Devres, which originates in Linux, manages device resources for each device and automatically releases them on driver detach. With devres, device resources are guaranteed to be freed whether initialization fails half-way or the device gets detached.
The basic idea is totally the same to that of Linux, but I tweaked it a bit so that it fits in U-Boot's driver model.
In U-Boot, drivers are activated in two steps: binding and probing. Binding puts a driver and a device together. It is just data manipulation on the system memory, so nothing has happened on the hardware device at this moment. When the device is really used, it is probed. Probing initializes the real hardware device to make it really ready for use.
So, the resources acquired during the probing process must be freed when the device is removed. Likewise, what has been allocated in binding should be released when the device is unbound. The struct devres has a member "probe" to remember when the resource was allocated.
CONFIG_DEBUG_DEVRES is also supported for easier debugging. If enabled, debug messages are printed each time a resource is allocated/freed.
Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com
Changes in v2:
- Add more APIs: _free, _find, _get, _remove, _destroy, _release
- Move devres_release_probe() and devres_release_all() decrlarations to dm/device-internal.h
- Move comments to headers
I've read the other thread
http://patchwork.ozlabs.org/patch/492715/
drivers/core/Kconfig | 10 +++ drivers/core/Makefile | 2 +- drivers/core/device-remove.c | 5 ++ drivers/core/device.c | 3 + drivers/core/devres.c | 187 +++++++++++++++++++++++++++++++++++++++++++ include/dm/device-internal.h | 19 +++++ include/dm/device.h | 140 ++++++++++++++++++++++++++++++++ 7 files changed, 365 insertions(+), 1 deletion(-) create mode 100644 drivers/core/devres.c
diff --git a/drivers/core/Kconfig b/drivers/core/Kconfig index 2861b43..5966801 100644 --- a/drivers/core/Kconfig +++ b/drivers/core/Kconfig @@ -55,3 +55,13 @@ config DM_SEQ_ALIAS Most boards will have a '/aliases' node containing the path to numbered devices (e.g. serial0 = &serial0). This feature can be disabled if it is not required, to save code space in SPL.
+config DEBUG_DEVRES
bool "Managed device resources verbose debug messages"depends on DMhelpIf this option is enabled, devres debug messages are printed.Select this if you are having a problem with devres or want todebug resource management for a managed device.If you are unsure about this, Say N here.diff --git a/drivers/core/Makefile b/drivers/core/Makefile index a3fec38..cd8c104 100644 --- a/drivers/core/Makefile +++ b/drivers/core/Makefile @@ -4,7 +4,7 @@ # SPDX-License-Identifier: GPL-2.0+ #
-obj-$(CONFIG_DM) += device.o lists.o root.o uclass.o util.o +obj-$(CONFIG_DM) += device.o lists.o root.o uclass.o util.o devres.o ifndef CONFIG_SPL_BUILD obj-$(CONFIG_OF_CONTROL) += simple-bus.o endif diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c index 20b56f9..e1714b2 100644 --- a/drivers/core/device-remove.c +++ b/drivers/core/device-remove.c @@ -109,6 +109,9 @@ int device_unbind(struct udevice *dev)
if (dev->parent) list_del(&dev->sibling_node);
devres_release_all(dev);free(dev); return 0;@@ -142,6 +145,8 @@ void device_free(struct udevice *dev) dev->parent_priv = NULL; } }
devres_release_probe(dev);}
int device_remove(struct udevice *dev) diff --git a/drivers/core/device.c b/drivers/core/device.c index 39cc2f3..83b47d8 100644 --- a/drivers/core/device.c +++ b/drivers/core/device.c @@ -47,6 +47,7 @@ int device_bind(struct udevice *parent, const struct driver *drv, INIT_LIST_HEAD(&dev->sibling_node); INIT_LIST_HEAD(&dev->child_head); INIT_LIST_HEAD(&dev->uclass_node);
INIT_LIST_HEAD(&dev->devres_head); dev->platdata = platdata; dev->name = name; dev->of_offset = of_offset;@@ -170,6 +171,8 @@ fail_alloc2: dev->platdata = NULL; } fail_alloc1:
devres_release_all(dev);free(dev); return ret;diff --git a/drivers/core/devres.c b/drivers/core/devres.c new file mode 100644 index 0000000..e7330b3 --- /dev/null +++ b/drivers/core/devres.c @@ -0,0 +1,187 @@ +/*
- Copyright (C) 2015 Masahiro Yamada yamada.masahiro@socionext.com
- Based on the original work in Linux by
- Copyright (c) 2006 SUSE Linux Products GmbH
- Copyright (c) 2006 Tejun Heo teheo@suse.de
- SPDX-License-Identifier: GPL-2.0+
- */
+#include <common.h> +#include <linux/compat.h> +#include <linux/kernel.h> +#include <linux/list.h> +#include <dm/device.h>
+struct devres {
struct list_head entry;dr_release_t release;bool probe;+#ifdef CONFIG_DEBUG_DEVRES
const char *name;size_t size;+#endif
unsigned long long data[];+};
+#ifdef CONFIG_DEBUG_DEVRES
+static void set_node_dbginfo(struct devres *dr, const char *name, size_t size) +{
dr->name = name;dr->size = size;+}
+static void devres_log(struct udevice *dev, struct devres *dr,
const char *op)+{
printf("%s: DEVRES %3s %p %s (%lu bytes)\n",dev->name, op, dr, dr->name, (unsigned long)dr->size);+} +#else /* CONFIG_DEBUG_DEVRES */ +#define set_node_dbginfo(dr, n, s) do {} while (0) +#define devres_log(dev, dr, op) do {} while (0) +#endif
+#if CONFIG_DEBUG_DEVRES +void *__devres_alloc(dr_release_t release, size_t size, gfp_t gfp,
const char *name)+#else +void *_devres_alloc(dr_release_t release, size_t size, gfp_t gfp) +#endif +{
size_t tot_size = sizeof(struct devres) + size;struct devres *dr;dr = kmalloc(tot_size, gfp);if (unlikely(!dr))return NULL;INIT_LIST_HEAD(&dr->entry);dr->release = release;set_node_dbginfo(dr, name, size);return dr->data;+}
+void devres_free(void *res) +{
if (res) {struct devres *dr = container_of(res, struct devres, data);BUG_ON(!list_empty(&dr->entry));kfree(dr);}+}
+void devres_add(struct udevice *dev, void *res) +{
struct devres *dr = container_of(res, struct devres, data);devres_log(dev, dr, "ADD");BUG_ON(!list_empty(&dr->entry));dr->probe = dev->flags & DM_FLAG_BOUND ? true : false;list_add_tail(&dr->entry, &dev->devres_head);+}
+void *devres_find(struct udevice *dev, dr_release_t release,
dr_match_t match, void *match_data)+{
struct devres *dr;list_for_each_entry_reverse(dr, &dev->devres_head, entry) {if (dr->release != release)continue;if (match && !match(dev, dr->data, match_data))continue;return dr->data;}return NULL;+}
+void *devres_get(struct udevice *dev, void *new_res,
dr_match_t match, void *match_data)+{
struct devres *new_dr = container_of(new_res, struct devres, data);void *res;res = devres_find(dev, new_dr->release, match, match_data);if (!res) {devres_add(dev, new_res);res = new_res;new_res = NULL;}devres_free(new_res);return res;+}
+void *devres_remove(struct udevice *dev, dr_release_t release,
dr_match_t match, void *match_data)+{
void *res;res = devres_find(dev, release, match, match_data);if (res) {struct devres *dr = container_of(res, struct devres, data);list_del_init(&dr->entry);devres_log(dev, dr, "REM");}return res;+}
+int devres_destroy(struct udevice *dev, dr_release_t release,
dr_match_t match, void *match_data)+{
void *res;res = devres_remove(dev, release, match, match_data);if (unlikely(!res))return -ENOENT;devres_free(res);return 0;+}
+int devres_release(struct udevice *dev, dr_release_t release,
dr_match_t match, void *match_data)+{
void *res;res = devres_remove(dev, release, match, match_data);if (unlikely(!res))return -ENOENT;(*release)(dev, res);devres_free(res);return 0;+}
+static void release_nodes(struct udevice *dev, struct list_head *head,
bool probe_only)+{
struct devres *dr, *tmp;list_for_each_entry_safe_reverse(dr, tmp, head, entry) {if (probe_only && !dr->probe)break;devres_log(dev, dr, "REL");dr->release(dev, dr->data);list_del(&dr->entry);kfree(dr);}+}
+void devres_release_probe(struct udevice *dev) +{
release_nodes(dev, &dev->devres_head, true);+}
+void devres_release_all(struct udevice *dev) +{
release_nodes(dev, &dev->devres_head, false);+} diff --git a/include/dm/device-internal.h b/include/dm/device-internal.h index 687462b..409c687 100644 --- a/include/dm/device-internal.h +++ b/include/dm/device-internal.h @@ -117,4 +117,23 @@ static inline void device_free(struct udevice *dev) {} #define DM_ROOT_NON_CONST (((gd_t *)gd)->dm_root) #define DM_UCLASS_ROOT_NON_CONST (((gd_t *)gd)->uclass_root)
+/* device resource management */ +/**
- devres_release_probe - Release managed resources allocated after probing
- @dev: Device to release resources for
- Release all resources allocated for @dev when it was probed or later.
- This function is called on driver removal.
- */
+void devres_release_probe(struct udevice *dev);
+/**
- devres_release_all - Release all managed resources
- @dev: Device to release resources for
- Release all resources associated with @dev. This function is
- called on driver unbinding.
- */
+void devres_release_all(struct udevice *dev);
#endif diff --git a/include/dm/device.h b/include/dm/device.h index 3674d19..c266c7d 100644 --- a/include/dm/device.h +++ b/include/dm/device.h @@ -96,6 +96,7 @@ struct udevice { uint32_t flags; int req_seq; int seq;
struct list_head devres_head;};
/* Maximum sequence number supported */ @@ -449,4 +450,143 @@ bool device_has_active_children(struct udevice *dev); */ bool device_is_last_sibling(struct udevice *dev);
+/* device resource management */ +typedef void (*dr_release_t)(struct udevice *dev, void *res); +typedef int (*dr_match_t)(struct udevice *dev, void *res, void *match_data);
+#ifdef CONFIG_DEBUG_DEVRES +void *__devres_alloc(dr_release_t release, size_t size, gfp_t gfp,
const char *name);+#define _devres_alloc(release, size, gfp) \
__devres_alloc(release, size, gfp, #release)+#else +void *_devres_alloc(dr_release_t release, size_t size, gfp_t gfp); +#endif
+/**
- devres_alloc - Allocate device resource data
- @release: Release function devres will be associated with
- @size: Allocation size
- @gfp: Allocation flags
- Allocate devres of @size bytes. The allocated area is associated
- with @release. The returned pointer can be passed to
- other devres_*() functions.
- RETURNS:
- Pointer to allocated devres on success, NULL on failure.
- */
+#define devres_alloc(release, size, gfp) \
_devres_alloc(release, size, gfp | __GFP_ZERO)+/**
- devres_free - Free device resource data
- @res: Pointer to devres data to free
- Free devres created with devres_alloc().
- */
+void devres_free(void *res);
+/**
- devres_add - Register device resource
- @dev: Device to add resource to
- @res: Resource to register
- Register devres @res to @dev. @res should have been allocated
- using devres_alloc(). On driver detach, the associated release
- function will be invoked and devres will be freed automatically.
- */
+void devres_add(struct udevice *dev, void *res);
+/**
- devres_find - Find device resource
- @dev: Device to lookup resource from
- @release: Look for resources associated with this release function
- @match: Match function (optional)
- @match_data: Data for the match function
- Find the latest devres of @dev which is associated with @release
- and for which @match returns 1. If @match is NULL, it's considered
- to match all.
- RETURNS:
- Pointer to found devres, NULL if not found.
- */
+void *devres_find(struct udevice *dev, dr_release_t release,
dr_match_t match, void *match_data);+/**
- devres_get - Find devres, if non-existent, add one atomically
- @dev: Device to lookup or add devres for
- @new_res: Pointer to new initialized devres to add if not found
- @match: Match function (optional)
- @match_data: Data for the match function
- Find the latest devres of @dev which has the same release function
- as @new_res and for which @match return 1. If found, @new_res is
- freed; otherwise, @new_res is added atomically.
- RETURNS:
- Pointer to found or added devres.
- */
+void *devres_get(struct udevice *dev, void *new_res,
dr_match_t match, void *match_data);+/**
- devres_remove - Find a device resource and remove it
- @dev: Device to find resource from
- @release: Look for resources associated with this release function
- @match: Match function (optional)
- @match_data: Data for the match function
- Find the latest devres of @dev associated with @release and for
- which @match returns 1. If @match is NULL, it's considered to
- match all. If found, the resource is removed atomically and
- returned.
- RETURNS:
- Pointer to removed devres on success, NULL if not found.
- */
+void *devres_remove(struct udevice *dev, dr_release_t release,
dr_match_t match, void *match_data);+/**
- devres_destroy - Find a device resource and destroy it
- @dev: Device to find resource from
- @release: Look for resources associated with this release function
- @match: Match function (optional)
- @match_data: Data for the match function
- Find the latest devres of @dev associated with @release and for
- which @match returns 1. If @match is NULL, it's considered to
- match all. If found, the resource is removed atomically and freed.
- Note that the release function for the resource will not be called,
- only the devres-allocated data will be freed. The caller becomes
- responsible for freeing any other data.
- RETURNS:
- 0 if devres is found and freed, -ENOENT if not found.
- */
+int devres_destroy(struct udevice *dev, dr_release_t release,
dr_match_t match, void *match_data);+/**
- devres_release - Find a device resource and destroy it, calling release
- @dev: Device to find resource from
- @release: Look for resources associated with this release function
- @match: Match function (optional)
- @match_data: Data for the match function
- Find the latest devres of @dev associated with @release and for
- which @match returns 1. If @match is NULL, it's considered to
- match all. If found, the resource is removed atomically, the
- release function called and the resource freed.
- RETURNS:
- 0 if devres is found and freed, -ENOENT if not found.
- */
+int devres_release(struct udevice *dev, dr_release_t release,
dr_match_t match, void *match_data);#endif
1.9.1
devm_kmalloc() is identical to kmalloc() except that the memory allocated with it is managed and will be automatically released when the device is removed/unbound.
Likewise for the other variants.
Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com Acked-by: Simon Glass sjg@chromium.org ---
Changes in v2: - Rip off "extern" from the func declarations - Add comments in headers - Add devm_kfree() - Do not force devm_kmalloc() zero-filling
drivers/core/devres.c | 34 ++++++++++++++++++++++++++++++++++ include/dm/device.h | 44 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+)
diff --git a/drivers/core/devres.c b/drivers/core/devres.c index e7330b3..ae0c191 100644 --- a/drivers/core/devres.c +++ b/drivers/core/devres.c @@ -185,3 +185,37 @@ void devres_release_all(struct udevice *dev) { release_nodes(dev, &dev->devres_head, false); } + +/* + * Managed kmalloc/kfree + */ +static void devm_kmalloc_release(struct udevice *dev, void *res) +{ + /* noop */ +} + +static int devm_kmalloc_match(struct udevice *dev, void *res, void *data) +{ + return res == data; +} + +void *devm_kmalloc(struct udevice *dev, size_t size, gfp_t gfp) +{ + void *data; + + data = _devres_alloc(devm_kmalloc_release, size, gfp); + if (unlikely(!data)) + return NULL; + + devres_add(dev, data); + + return data; +} + +void devm_kfree(struct udevice *dev, void *p) +{ + int rc; + + rc = devres_destroy(dev, devm_kmalloc_release, devm_kmalloc_match, p); + WARN_ON(rc); +} diff --git a/include/dm/device.h b/include/dm/device.h index c266c7d..ae98067 100644 --- a/include/dm/device.h +++ b/include/dm/device.h @@ -14,6 +14,8 @@ #include <dm/uclass-id.h> #include <fdtdec.h> #include <linker_lists.h> +#include <linux/compat.h> +#include <linux/kernel.h> #include <linux/list.h>
struct driver_info; @@ -589,4 +591,46 @@ int devres_destroy(struct udevice *dev, dr_release_t release, int devres_release(struct udevice *dev, dr_release_t release, dr_match_t match, void *match_data);
+/* managed devm_k.alloc/kfree for device drivers */ +/** + * devm_kmalloc - Resource-managed kmalloc + * @dev: Device to allocate memory for + * @size: Allocation size + * @gfp: Allocation gfp flags + * + * Managed kmalloc. Memory allocated with this function is + * automatically freed on driver detach. Like all other devres + * resources, guaranteed alignment is unsigned long long. + * + * RETURNS: + * Pointer to allocated memory on success, NULL on failure. + */ +void *devm_kmalloc(struct udevice *dev, size_t size, gfp_t gfp); +static inline void *devm_kzalloc(struct udevice *dev, size_t size, gfp_t gfp) +{ + return devm_kmalloc(dev, size, gfp | __GFP_ZERO); +} +static inline void *devm_kmalloc_array(struct udevice *dev, + size_t n, size_t size, gfp_t flags) +{ + if (size != 0 && n > SIZE_MAX / size) + return NULL; + return devm_kmalloc(dev, n * size, flags); +} +static inline void *devm_kcalloc(struct udevice *dev, + size_t n, size_t size, gfp_t flags) +{ + return devm_kmalloc_array(dev, n, size, flags | __GFP_ZERO); +} + +/** + * devm_kfree - Resource-managed kfree + * @dev: Device this memory belongs to + * @p: Memory to free + * + * Free memory allocated with devm_kmalloc(). + */ +void devm_kfree(struct udevice *dev, void *p); + + #endif
With devm_kzalloc(), device_unbind() and the failure path in device_bind() can be much cleaner.
We no longer need such flags as DM_FLAG_ALLOC_PDATA etc. because we do not have to remember if the memory has been really allocated.
The memory is freed when the device is unbind.
Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com ---
Changes in v2: None
drivers/core/device-remove.c | 12 ------------ drivers/core/device.c | 24 ++++++------------------ include/dm/device.h | 9 --------- 3 files changed, 6 insertions(+), 39 deletions(-)
diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c index e1714b2..0417535 100644 --- a/drivers/core/device-remove.c +++ b/drivers/core/device-remove.c @@ -91,18 +91,6 @@ int device_unbind(struct udevice *dev) if (ret) return ret;
- if (dev->flags & DM_FLAG_ALLOC_PDATA) { - free(dev->platdata); - dev->platdata = NULL; - } - if (dev->flags & DM_FLAG_ALLOC_UCLASS_PDATA) { - free(dev->uclass_platdata); - dev->uclass_platdata = NULL; - } - if (dev->flags & DM_FLAG_ALLOC_PARENT_PDATA) { - free(dev->parent_platdata); - dev->parent_platdata = NULL; - } ret = uclass_unbind_device(dev); if (ret) return ret; diff --git a/drivers/core/device.c b/drivers/core/device.c index 83b47d8..65b8a14 100644 --- a/drivers/core/device.c +++ b/drivers/core/device.c @@ -75,8 +75,9 @@ int device_bind(struct udevice *parent, const struct driver *drv, }
if (!dev->platdata && drv->platdata_auto_alloc_size) { - dev->flags |= DM_FLAG_ALLOC_PDATA; - dev->platdata = calloc(1, drv->platdata_auto_alloc_size); + dev->platdata = devm_kzalloc(dev, + drv->platdata_auto_alloc_size, + GFP_KERNEL); if (!dev->platdata) { ret = -ENOMEM; goto fail_alloc1; @@ -85,8 +86,7 @@ int device_bind(struct udevice *parent, const struct driver *drv,
size = uc->uc_drv->per_device_platdata_auto_alloc_size; if (size) { - dev->flags |= DM_FLAG_ALLOC_UCLASS_PDATA; - dev->uclass_platdata = calloc(1, size); + dev->uclass_platdata = devm_kzalloc(dev, size, GFP_KERNEL); if (!dev->uclass_platdata) { ret = -ENOMEM; goto fail_alloc2; @@ -100,8 +100,8 @@ int device_bind(struct udevice *parent, const struct driver *drv, per_child_platdata_auto_alloc_size; } if (size) { - dev->flags |= DM_FLAG_ALLOC_PARENT_PDATA; - dev->parent_platdata = calloc(1, size); + dev->parent_platdata = devm_kzalloc(dev, size, + GFP_KERNEL); if (!dev->parent_platdata) { ret = -ENOMEM; goto fail_alloc3; @@ -155,21 +155,9 @@ fail_bind: fail_uclass_bind: if (IS_ENABLED(CONFIG_DM_DEVICE_REMOVE)) { list_del(&dev->sibling_node); - if (dev->flags & DM_FLAG_ALLOC_PARENT_PDATA) { - free(dev->parent_platdata); - dev->parent_platdata = NULL; - } } fail_alloc3: - if (dev->flags & DM_FLAG_ALLOC_UCLASS_PDATA) { - free(dev->uclass_platdata); - dev->uclass_platdata = NULL; - } fail_alloc2: - if (dev->flags & DM_FLAG_ALLOC_PDATA) { - free(dev->platdata); - dev->platdata = NULL; - } fail_alloc1: devres_release_all(dev);
diff --git a/include/dm/device.h b/include/dm/device.h index ae98067..1a832a7 100644 --- a/include/dm/device.h +++ b/include/dm/device.h @@ -23,18 +23,9 @@ struct driver_info; /* Driver is active (probed). Cleared when it is removed */ #define DM_FLAG_ACTIVATED (1 << 0)
-/* DM is responsible for allocating and freeing platdata */ -#define DM_FLAG_ALLOC_PDATA (1 << 1) - /* DM should init this device prior to relocation */ #define DM_FLAG_PRE_RELOC (1 << 2)
-/* DM is responsible for allocating and freeing parent_platdata */ -#define DM_FLAG_ALLOC_PARENT_PDATA (1 << 3) - -/* DM is responsible for allocating and freeing uclass_platdata */ -#define DM_FLAG_ALLOC_UCLASS_PDATA (1 << 4) - /* Allocate driver private data on a DMA boundary */ #define DM_FLAG_ALLOC_PRIV_DMA (1 << 5)
A little more clean up made possible by devm_kzalloc().
Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com ---
Changes in v2: None
drivers/core/device.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/core/device.c b/drivers/core/device.c index 65b8a14..bcae3ba 100644 --- a/drivers/core/device.c +++ b/drivers/core/device.c @@ -80,7 +80,7 @@ int device_bind(struct udevice *parent, const struct driver *drv, GFP_KERNEL); if (!dev->platdata) { ret = -ENOMEM; - goto fail_alloc1; + goto fail_alloc; } }
@@ -89,7 +89,7 @@ int device_bind(struct udevice *parent, const struct driver *drv, dev->uclass_platdata = devm_kzalloc(dev, size, GFP_KERNEL); if (!dev->uclass_platdata) { ret = -ENOMEM; - goto fail_alloc2; + goto fail_alloc; } }
@@ -104,7 +104,7 @@ int device_bind(struct udevice *parent, const struct driver *drv, GFP_KERNEL); if (!dev->parent_platdata) { ret = -ENOMEM; - goto fail_alloc3; + goto fail_alloc; } } } @@ -156,9 +156,7 @@ fail_uclass_bind: if (IS_ENABLED(CONFIG_DM_DEVICE_REMOVE)) { list_del(&dev->sibling_node); } -fail_alloc3: -fail_alloc2: -fail_alloc1: +fail_alloc: devres_release_all(dev);
free(dev);
It does not seem efficient to always return cache-aligned memory. Return aligned memory only when GFP_DMA flag is given.
My main motivation for this commit is to refactor device_probe() and device_free() in the next commit. DM_FLAG_ALLOC_PRIV_DMA should be handled more easily.
Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com ---
Changes in v2: None
include/linux/compat.h | 1 + lib/linux_compat.c | 5 ++++- 2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/include/linux/compat.h b/include/linux/compat.h index fbebf91..13386c9 100644 --- a/include/linux/compat.h +++ b/include/linux/compat.h @@ -41,6 +41,7 @@ extern struct p_current *current; #define GFP_NOFS ((gfp_t) 0) #define GFP_USER ((gfp_t) 0) #define __GFP_NOWARN ((gfp_t) 0) +#define GFP_DMA ((__force gfp_t)0x01u) #define __GFP_ZERO ((__force gfp_t)0x8000u) /* Return zeroed page on success */
void *kmalloc(size_t size, int flags); diff --git a/lib/linux_compat.c b/lib/linux_compat.c index a936a7e..6da0cfa 100644 --- a/lib/linux_compat.c +++ b/lib/linux_compat.c @@ -18,7 +18,10 @@ void *kmalloc(size_t size, int flags) { void *p;
- p = memalign(ARCH_DMA_MINALIGN, size); + if (flags & GFP_DMA) + p = memalign(ARCH_DMA_MINALIGN, size); + else + p = malloc(size); if (flags & __GFP_ZERO) memset(p, 0, size);
Hi Masahiro,
It does not seem efficient to always return cache-aligned memory. Return aligned memory only when GFP_DMA flag is given.
My main motivation for this commit is to refactor device_probe() and device_free() in the next commit. DM_FLAG_ALLOC_PRIV_DMA should be handled more easily.
Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com
Changes in v2: None
include/linux/compat.h | 1 + lib/linux_compat.c | 5 ++++- 2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/include/linux/compat.h b/include/linux/compat.h index fbebf91..13386c9 100644 --- a/include/linux/compat.h +++ b/include/linux/compat.h @@ -41,6 +41,7 @@ extern struct p_current *current; #define GFP_NOFS ((gfp_t) 0) #define GFP_USER ((gfp_t) 0) #define __GFP_NOWARN ((gfp_t) 0) +#define GFP_DMA ((__force gfp_t)0x01u) #define __GFP_ZERO ((__force gfp_t)0x8000u) /* Return zeroed page on success */ void *kmalloc(size_t size, int flags); diff --git a/lib/linux_compat.c b/lib/linux_compat.c index a936a7e..6da0cfa 100644 --- a/lib/linux_compat.c +++ b/lib/linux_compat.c @@ -18,7 +18,10 @@ void *kmalloc(size_t size, int flags) { void *p;
- p = memalign(ARCH_DMA_MINALIGN, size);
- if (flags & GFP_DMA)
p = memalign(ARCH_DMA_MINALIGN, size);- else
p = malloc(size);
Reviewed-by: Lukasz Majewski l.majewski@samsung.com
I hope that all places where kmalloc without GFP_DMA set is called and implicitly require memalign'ed memory would be changed accordingly.
Otherwise we would have nasty, hard to find bugs.
2015-07-13 16:52 GMT+09:00 Lukasz Majewski l.majewski@samsung.com:
Hi Masahiro,
It does not seem efficient to always return cache-aligned memory. Return aligned memory only when GFP_DMA flag is given.
My main motivation for this commit is to refactor device_probe() and device_free() in the next commit. DM_FLAG_ALLOC_PRIV_DMA should be handled more easily.
Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com
Changes in v2: None
include/linux/compat.h | 1 + lib/linux_compat.c | 5 ++++- 2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/include/linux/compat.h b/include/linux/compat.h index fbebf91..13386c9 100644 --- a/include/linux/compat.h +++ b/include/linux/compat.h @@ -41,6 +41,7 @@ extern struct p_current *current; #define GFP_NOFS ((gfp_t) 0) #define GFP_USER ((gfp_t) 0) #define __GFP_NOWARN ((gfp_t) 0) +#define GFP_DMA ((__force gfp_t)0x01u) #define __GFP_ZERO ((__force gfp_t)0x8000u) /* Return zeroed page on success */ void *kmalloc(size_t size, int flags); diff --git a/lib/linux_compat.c b/lib/linux_compat.c index a936a7e..6da0cfa 100644 --- a/lib/linux_compat.c +++ b/lib/linux_compat.c @@ -18,7 +18,10 @@ void *kmalloc(size_t size, int flags) { void *p;
p = memalign(ARCH_DMA_MINALIGN, size);
if (flags & GFP_DMA)p = memalign(ARCH_DMA_MINALIGN, size);elsep = malloc(size); if (flags & __GFP_ZERO) memset(p, 0, size);Reviewed-by: Lukasz Majewski l.majewski@samsung.com
I hope that all places where kmalloc without GFP_DMA set is called and implicitly require memalign'ed memory would be changed accordingly.
Otherwise we would have nasty, hard to find bugs.
As Lukasz and Heiko pointed out, I admit this commit is not safe.
For now, we should consider keeping the cache-alignment for all the cases.
The memory is automatically released on driver removal, so we do not need to do so explicitly in device_free().
The helper function alloc_priv() is no longer needed because devm_kzalloc() now understands the GFP_DMA flag.
Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com Reviewed-by: Heiko Schocher hs@denx.de ---
Changes in v2: None
drivers/core/device-remove.c | 23 ----------------------- drivers/core/device.c | 25 +++++++------------------ 2 files changed, 7 insertions(+), 41 deletions(-)
diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c index 0417535..78551e4 100644 --- a/drivers/core/device-remove.c +++ b/drivers/core/device-remove.c @@ -111,29 +111,6 @@ int device_unbind(struct udevice *dev) */ void device_free(struct udevice *dev) { - int size; - - if (dev->driver->priv_auto_alloc_size) { - free(dev->priv); - dev->priv = NULL; - } - size = dev->uclass->uc_drv->per_device_auto_alloc_size; - if (size) { - free(dev->uclass_priv); - dev->uclass_priv = NULL; - } - if (dev->parent) { - size = dev->parent->driver->per_child_auto_alloc_size; - if (!size) { - size = dev->parent->uclass->uc_drv-> - per_child_auto_alloc_size; - } - if (size) { - free(dev->parent_priv); - dev->parent_priv = NULL; - } - } - devres_release_probe(dev); }
diff --git a/drivers/core/device.c b/drivers/core/device.c index bcae3ba..e4097c9 100644 --- a/drivers/core/device.c +++ b/drivers/core/device.c @@ -179,27 +179,13 @@ int device_bind_by_name(struct udevice *parent, bool pre_reloc_only, -1, devp); }
-static void *alloc_priv(int size, uint flags) -{ - void *priv; - - if (flags & DM_FLAG_ALLOC_PRIV_DMA) { - priv = memalign(ARCH_DMA_MINALIGN, size); - if (priv) - memset(priv, '\0', size); - } else { - priv = calloc(1, size); - } - - return priv; -} - int device_probe_child(struct udevice *dev, void *parent_priv) { const struct driver *drv; int size = 0; int ret; int seq; + gfp_t flags = GFP_KERNEL;
if (!dev) return -EINVAL; @@ -210,9 +196,12 @@ int device_probe_child(struct udevice *dev, void *parent_priv) drv = dev->driver; assert(drv);
+ if (drv->flags & DM_FLAG_ALLOC_PRIV_DMA) + flags |= GFP_DMA; + /* Allocate private data if requested */ if (drv->priv_auto_alloc_size) { - dev->priv = alloc_priv(drv->priv_auto_alloc_size, drv->flags); + dev->priv = devm_kzalloc(dev, drv->priv_auto_alloc_size, flags); if (!dev->priv) { ret = -ENOMEM; goto fail; @@ -221,7 +210,7 @@ int device_probe_child(struct udevice *dev, void *parent_priv) /* Allocate private data if requested */ size = dev->uclass->uc_drv->per_device_auto_alloc_size; if (size) { - dev->uclass_priv = calloc(1, size); + dev->uclass_priv = devm_kzalloc(dev, size, GFP_KERNEL); if (!dev->uclass_priv) { ret = -ENOMEM; goto fail; @@ -236,7 +225,7 @@ int device_probe_child(struct udevice *dev, void *parent_priv) per_child_auto_alloc_size; } if (size) { - dev->parent_priv = alloc_priv(size, drv->flags); + dev->parent_priv = devm_kzalloc(dev, size, flags); if (!dev->parent_priv) { ret = -ENOMEM; goto fail;
This new command can dump all device resources associated to each device. The fields in every line shows: - The address of the resource - The size of the resource - The name of the release function - The stage in which the resource has been acquired (BIND/PROBE)
The output looks like this:
=> devres - root_driver - soc - extbus - serial@54006800 0xbfb541e8 (8 byte) devm_kmalloc_release BIND 0xbfb54440 (4 byte) devm_kmalloc_release PROBE 0xbfb54460 (4 byte) devm_kmalloc_release PROBE - serial@54006900 0xbfb54270 (8 byte) devm_kmalloc_release BIND - gpio@55000000 - i2c@58780000 0xbfb5bce8 (12 byte) devm_kmalloc_release PROBE 0xbfb5bd10 (4 byte) devm_kmalloc_release PROBE - eeprom 0xbfb54418 (12 byte) devm_kmalloc_release BIND
Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com ---
Changes in v2: - add static to dump_resources()
drivers/core/Kconfig | 6 ++++++ drivers/core/devres.c | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+)
diff --git a/drivers/core/Kconfig b/drivers/core/Kconfig index 5966801..edaf3fd 100644 --- a/drivers/core/Kconfig +++ b/drivers/core/Kconfig @@ -65,3 +65,9 @@ config DEBUG_DEVRES debug resource management for a managed device.
If you are unsure about this, Say N here. + +config CMD_DEVRES + bool "Managed device resources dump command" + depends on DEBUG_DEVRES + help + This command displays all resources allociated to each device. diff --git a/drivers/core/devres.c b/drivers/core/devres.c index ae0c191..77f39a5 100644 --- a/drivers/core/devres.c +++ b/drivers/core/devres.c @@ -13,6 +13,7 @@ #include <linux/kernel.h> #include <linux/list.h> #include <dm/device.h> +#include <dm/root.h>
struct devres { struct list_head entry; @@ -186,6 +187,42 @@ void devres_release_all(struct udevice *dev) release_nodes(dev, &dev->devres_head, false); }
+#ifdef CONFIG_CMD_DEVRES +static void dump_resources(struct udevice *dev, int depth) +{ + struct devres *dr; + struct udevice *child; + + printf("- %s\n", dev->name); + + list_for_each_entry(dr, &dev->devres_head, entry) + printf(" 0x%p (%lu byte) %s %s\n", dr, + (unsigned long)dr->size, dr->name, + dr->probe ? "PROBE" : "BIND"); + + list_for_each_entry(child, &dev->child_head, sibling_node) + dump_resources(child, depth + 1); +} + +static int do_devres(cmd_tbl_t *cmdtp, int flag, int argc, + char * const argv[]) +{ + struct udevice *root; + + root = dm_root(); + if (root) + dump_resources(root, 0); + + return 0; +} + +U_BOOT_CMD( + devres, 1, 1, do_devres, + "show device resources", + "" +); +#endif + /* * Managed kmalloc/kfree */
As you see in driver/Makefile, Kbuild descends into the driver/core/ directory only when CONFIG_DM is enabled.
Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com ---
Changes in v2: - Newly added
drivers/core/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/core/Makefile b/drivers/core/Makefile index cd8c104..5f019e8 100644 --- a/drivers/core/Makefile +++ b/drivers/core/Makefile @@ -4,7 +4,7 @@ # SPDX-License-Identifier: GPL-2.0+ #
-obj-$(CONFIG_DM) += device.o lists.o root.o uclass.o util.o devres.o +obj-y += device.o lists.o root.o uclass.o util.o devres.o ifndef CONFIG_SPL_BUILD obj-$(CONFIG_OF_CONTROL) += simple-bus.o endif
On 12 July 2015 at 22:17, Masahiro Yamada yamada.masahiro@socionext.com wrote:
As you see in driver/Makefile, Kbuild descends into the driver/core/ directory only when CONFIG_DM is enabled.
Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com
Changes in v2:
- Newly added
drivers/core/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Acked-by: Simon Glass sjg@chromium.org
diff --git a/drivers/core/Makefile b/drivers/core/Makefile index cd8c104..5f019e8 100644 --- a/drivers/core/Makefile +++ b/drivers/core/Makefile @@ -4,7 +4,7 @@ # SPDX-License-Identifier: GPL-2.0+ #
-obj-$(CONFIG_DM) += device.o lists.o root.o uclass.o util.o devres.o +obj-y += device.o lists.o root.o uclass.o util.o devres.o ifndef CONFIG_SPL_BUILD obj-$(CONFIG_OF_CONTROL) += simple-bus.o endif -- 1.9.1
On 15 July 2015 at 18:59, Simon Glass sjg@chromium.org wrote:
On 12 July 2015 at 22:17, Masahiro Yamada yamada.masahiro@socionext.com wrote:
As you see in driver/Makefile, Kbuild descends into the driver/core/ directory only when CONFIG_DM is enabled.
Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com
Changes in v2:
- Newly added
drivers/core/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Acked-by: Simon Glass sjg@chromium.org
diff --git a/drivers/core/Makefile b/drivers/core/Makefile index cd8c104..5f019e8 100644 --- a/drivers/core/Makefile +++ b/drivers/core/Makefile @@ -4,7 +4,7 @@ # SPDX-License-Identifier: GPL-2.0+ #
-obj-$(CONFIG_DM) += device.o lists.o root.o uclass.o util.o devres.o +obj-y += device.o lists.o root.o uclass.o util.o devres.o ifndef CONFIG_SPL_BUILD obj-$(CONFIG_OF_CONTROL) += simple-bus.o endif -- 1.9.1
Applied to u-boot-dm, thanks!
Currently, Devres requires additional 16 byte for each allocation, which is not so insignificant for SPL in some cases.
If CONFIG_DM_DEVICE_REMOVE is disabled, we never remove devices, so we do not have to manage resources in the first place. In this case, devres_alloc() can fall back to the simple kzalloc(), and likewise, devm_*() to non-managed variants.
Signed-off-by: Masahiro Yamada yamada.masahiro@socionext.com Suggested-by: Simon Glass sjg@chromium.org ---
Changes in v2: None
drivers/core/Makefile | 4 +-- include/dm/device-internal.h | 13 +++++++ include/dm/device.h | 84 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 99 insertions(+), 2 deletions(-)
diff --git a/drivers/core/Makefile b/drivers/core/Makefile index 5f019e8..37d64c6 100644 --- a/drivers/core/Makefile +++ b/drivers/core/Makefile @@ -4,8 +4,8 @@ # SPDX-License-Identifier: GPL-2.0+ #
-obj-y += device.o lists.o root.o uclass.o util.o devres.o +obj-y += device.o lists.o root.o uclass.o util.o ifndef CONFIG_SPL_BUILD obj-$(CONFIG_OF_CONTROL) += simple-bus.o endif -obj-$(CONFIG_DM_DEVICE_REMOVE) += device-remove.o +obj-$(CONFIG_DM_DEVICE_REMOVE) += device-remove.o devres.o diff --git a/include/dm/device-internal.h b/include/dm/device-internal.h index 409c687..2c8446e 100644 --- a/include/dm/device-internal.h +++ b/include/dm/device-internal.h @@ -118,6 +118,8 @@ static inline void device_free(struct udevice *dev) {} #define DM_UCLASS_ROOT_NON_CONST (((gd_t *)gd)->uclass_root)
/* device resource management */ +#ifdef CONFIG_DM_DEVICE_REMOVE + /** * devres_release_probe - Release managed resources allocated after probing * @dev: Device to release resources for @@ -136,4 +138,15 @@ void devres_release_probe(struct udevice *dev); */ void devres_release_all(struct udevice *dev);
+#else /* !CONFIG_DM_DEVICE_REMOVE */ + +static inline void devres_release_probe(struct udevice *dev) +{ +} + +static inline void devres_release_all(struct udevice *dev) +{ +} + +#endif /* CONFIG_DM_DEVICE_REMOVE */ #endif diff --git a/include/dm/device.h b/include/dm/device.h index 1a832a7..33dc470 100644 --- a/include/dm/device.h +++ b/include/dm/device.h @@ -447,6 +447,8 @@ bool device_is_last_sibling(struct udevice *dev); typedef void (*dr_release_t)(struct udevice *dev, void *res); typedef int (*dr_match_t)(struct udevice *dev, void *res, void *match_data);
+#ifdef CONFIG_DM_DEVICE_REMOVE + #ifdef CONFIG_DEBUG_DEVRES void *__devres_alloc(dr_release_t release, size_t size, gfp_t gfp, const char *name); @@ -623,5 +625,87 @@ static inline void *devm_kcalloc(struct udevice *dev, */ void devm_kfree(struct udevice *dev, void *p);
+#else /* !CONFIG_DM_DEVICE_REMOVE */ + +/* + * If CONFIG_DM_DEVICE_REMOVE is not defined, we need not manage resources. + * The devres functions fall back to normal allocators. + */ +static inline void *devres_alloc(dr_release_t release, size_t size, gfp_t gfp) +{ + return kzalloc(size, gfp); +} + +static inline void devres_free(void *res) +{ + kfree(res); +} + +static inline void devres_add(struct udevice *dev, void *res) +{ +} + +static inline void *devres_find(struct udevice *dev, dr_release_t release, + dr_match_t match, void *match_data) +{ + return NULL; +} + +static inline void *devres_get(struct udevice *dev, void *new_res, + dr_match_t match, void *match_data) +{ + return NULL; +} + +static inline void *devres_remove(struct udevice *dev, dr_release_t release, + dr_match_t match, void *match_data) +{ + return NULL; +} + +static inline int devres_destroy(struct udevice *dev, dr_release_t release, + dr_match_t match, void *match_data) +{ + return 0; +} + +static inline int devres_release(struct udevice *dev, dr_release_t release, + dr_match_t match, void *match_data) +{ + return 0; +} + +static inline void *devm_kmalloc(struct udevice *dev, size_t size, gfp_t gfp) +{ + return kmalloc(size, gfp); +} + +static inline void *devm_kzalloc(struct udevice *dev, size_t size, gfp_t gfp) +{ + return kzalloc(size, gfp); +} + +static inline void *devm_kmaloc_array(struct udevice *dev, + size_t n, size_t size, gfp_t flags) +{ + /* TODO: add kmalloc_array() to linux/compat.h */ + if (size != 0 && n > SIZE_MAX / size) + return NULL; + return kmalloc(n * size, flags); +} + +static inline void *devm_kcalloc(struct udevice *dev, + size_t n, size_t size, gfp_t flags) +{ + /* TODO: add kcalloc() to linux/compat.h */ + return kmalloc(n * size, flags | __GFP_ZERO); +} + +static inline void devm_kfree(struct udevice *dev, void *p) +{ + kfree(p); +} + +#endif /* CONFIG_DM_DEVICE_REMOVE */
#endif
Hello Masahiro,
On Mon, 13 Jul 2015 13:17:03 +0900, Masahiro Yamada yamada.masahiro@socionext.com wrote:
Please refer to the commit message of 06/14 for what this series wants to do.
Remark: you could use "Series-notes:" in 6/14 to have patman directly include said notes here instead of referring the reader to the patch.
Masahiro Yamada (14): x86: delete unneeded declarations of disable_irq() and enable_irq() linux_compat: remove cpu_relax() define linux_compat: move vzalloc() to header file as an inline function linux_compat: handle __GFP_ZERO in kmalloc() dm: add DM_FLAG_BOUND flag devres: introduce Devres (Managed Device Resource) framework devres: add devm_kmalloc() and friends (managed memory allocators) dm: refactor device_bind() and device_unbind() with devm_kzalloc() dm: merge fail_alloc labels linux_compat: introduce GFP_DMA flag for kmalloc() dm: refactor device_probe() and device_remove() with devm_kzalloc() devres: add debug command to dump device resources dm: remove redundant CONFIG_DM from driver/core/Makefile devres: compile Devres iif CONFIG_DM_DEVICE_REMOVE is enabled
I am still unsure why this is necessary. I had a discussion on the list with Simon, see last message here:
https://www.mail-archive.com/u-boot@lists.denx.de/msg177031.html
Unless I'm mistaken, the only case where we actually have a leak that this series would fix is in some cases of binding USB devices / drivers multiple times, and even then, it would take a considerable amount of repeated bindings before U-Boot could become unable to boot a payload; a scenario that I find unlikely.
I do understand the general goal of fixing bugs when we ind them; but I question the global benefit of fixing this specific leak bug by adding a whole new feature with a lot of code to U-Boot, as opposed to fixing it in a more ad hoc way with much less less code volume and complexity.
Amicalement,
Hi Albert,
2015-07-13 15:51 GMT+09:00 Albert ARIBAUD albert.u.boot@aribaud.net:
Hello Masahiro,
On Mon, 13 Jul 2015 13:17:03 +0900, Masahiro Yamada yamada.masahiro@socionext.com wrote:
Please refer to the commit message of 06/14 for what this series wants to do.
Remark: you could use "Series-notes:" in 6/14 to have patman directly include said notes here instead of referring the reader to the patch.
Masahiro Yamada (14): x86: delete unneeded declarations of disable_irq() and enable_irq() linux_compat: remove cpu_relax() define linux_compat: move vzalloc() to header file as an inline function linux_compat: handle __GFP_ZERO in kmalloc() dm: add DM_FLAG_BOUND flag devres: introduce Devres (Managed Device Resource) framework devres: add devm_kmalloc() and friends (managed memory allocators) dm: refactor device_bind() and device_unbind() with devm_kzalloc() dm: merge fail_alloc labels linux_compat: introduce GFP_DMA flag for kmalloc() dm: refactor device_probe() and device_remove() with devm_kzalloc() devres: add debug command to dump device resources dm: remove redundant CONFIG_DM from driver/core/Makefile devres: compile Devres iif CONFIG_DM_DEVICE_REMOVE is enabled
I am still unsure why this is necessary. I had a discussion on the list with Simon, see last message here:
https://www.mail-archive.com/u-boot@lists.denx.de/msg177031.html
Unless I'm mistaken, the only case where we actually have a leak that this series would fix is in some cases of binding USB devices / drivers multiple times, and even then, it would take a considerable amount of repeated bindings before U-Boot could become unable to boot a payload; a scenario that I find unlikely.
I do understand the general goal of fixing bugs when we ind them; but I question the global benefit of fixing this specific leak bug by adding a whole new feature with a lot of code to U-Boot, as opposed to fixing it in a more ad hoc way with much less less code volume and complexity.
You have a point.
What we really want to avoid is to make low-level drivers too complicated by leaving the correct memory management to each of them.
After all, there turned out to be two options to solve it.
[1] Simon's driver model: move allocating/freeing memory to the driver core by having only the needed memory size specified in each driver [2] Devres: we still have to allocate memory in each driver, but we need not free it explicitly, making our driver work much easier
[1] and [2] are completely differently approach, but what they solve is the same: easier memory (resource) management without leak.
The only problem I see in [1] is that it is not controllable at run-time. The memory size for the auto-allocation must be specified at the compile time.
So, we need calloc() and free() in some low-level drivers. Simon might say they are only a few "exceptions", (my impression is I often hear the logic such as "it is not a problem because we do not have many.") Anyway, we had already lost the consistency as for memory allocation.
I imagined if [2] had been supported earlier, we would not have needed [1]. (at least, [2] seems more flexible to me.)
We already have many DM-based drivers, and I think we can live with [1] despite some exceptional drivers allocating memory on their own.
So, if Simon (and other developers) does not feel comfortable with this series, I do not mind discarding it.
Hello Masahiro,
On Mon, 13 Jul 2015 16:39:45 +0900, Masahiro Yamada yamada.masahiro@socionext.com wrote:
Hi Albert,
2015-07-13 15:51 GMT+09:00 Albert ARIBAUD albert.u.boot@aribaud.net:
Hello Masahiro,
On Mon, 13 Jul 2015 13:17:03 +0900, Masahiro Yamada yamada.masahiro@socionext.com wrote:
Please refer to the commit message of 06/14 for what this series wants to do.
Remark: you could use "Series-notes:" in 6/14 to have patman directly include said notes here instead of referring the reader to the patch.
Masahiro Yamada (14): x86: delete unneeded declarations of disable_irq() and enable_irq() linux_compat: remove cpu_relax() define linux_compat: move vzalloc() to header file as an inline function linux_compat: handle __GFP_ZERO in kmalloc() dm: add DM_FLAG_BOUND flag devres: introduce Devres (Managed Device Resource) framework devres: add devm_kmalloc() and friends (managed memory allocators) dm: refactor device_bind() and device_unbind() with devm_kzalloc() dm: merge fail_alloc labels linux_compat: introduce GFP_DMA flag for kmalloc() dm: refactor device_probe() and device_remove() with devm_kzalloc() devres: add debug command to dump device resources dm: remove redundant CONFIG_DM from driver/core/Makefile devres: compile Devres iif CONFIG_DM_DEVICE_REMOVE is enabled
I am still unsure why this is necessary. I had a discussion on the list with Simon, see last message here:
https://www.mail-archive.com/u-boot@lists.denx.de/msg177031.html
Unless I'm mistaken, the only case where we actually have a leak that this series would fix is in some cases of binding USB devices / drivers multiple times, and even then, it would take a considerable amount of repeated bindings before U-Boot could become unable to boot a payload; a scenario that I find unlikely.
I do understand the general goal of fixing bugs when we ind them; but I question the global benefit of fixing this specific leak bug by adding a whole new feature with a lot of code to U-Boot, as opposed to fixing it in a more ad hoc way with much less less code volume and complexity.
You have a point.
What we really want to avoid is to make low-level drivers too complicated by leaving the correct memory management to each of them.
After all, there turned out to be two options to solve it.
[1] Simon's driver model: move allocating/freeing memory to the driver core by having only the needed memory size specified in each driver [2] Devres: we still have to allocate memory in each driver, but we need not free it explicitly, making our driver work much easier
[1] and [2] are completely differently approach, but what they solve is the same: easier memory (resource) management without leak.
Understood.
IIUC, this adds a new leak scenario beside the multiple inits one such as for USB, but this new scenarion which is in failure paths where already allocated memory must be released, seems to me no more critical than the one I'd discussed with Simon, i.e., I'm not convinced we need a fix, and I'm not convinced we need a general memory management feature for that -- which does not mean I can't be convinced, though; I just need (more) convincing arguments (than I can currently read).
The only problem I see in [1] is that it is not controllable at run-time. The memory size for the auto-allocation must be specified at the compile time.
How in practice is that a problem?
So, we need calloc() and free() in some low-level drivers. Simon might say they are only a few "exceptions", (my impression is I often hear the logic such as "it is not a problem because we do not have many.") Anyway, we had already lost the consistency as for memory allocation.
Not sure I understand that last sentence. Which consistency exactly have we lost?
I imagined if [2] had been supported earlier, we would not have needed [1]. (at least, [2] seems more flexible to me.)
We already have many DM-based drivers, and I think we can live with [1] despite some exceptional drivers allocating memory on their own.
So, if Simon (and other developers) does not feel comfortable with this series, I do not mind discarding it.
Note that I'm not saying your patch series does not solve the issue of leaks. What I'm saying is i) do we need to solve this issue, and ii) if we do, is your series the best option ?
-- Best Regards Masahiro Yamada
Amicalement,
2015-07-13 19:55 GMT+09:00 Albert ARIBAUD albert.u.boot@aribaud.net:
Hello Masahiro,
On Mon, 13 Jul 2015 16:39:45 +0900, Masahiro Yamada yamada.masahiro@socionext.com wrote:
Hi Albert,
2015-07-13 15:51 GMT+09:00 Albert ARIBAUD albert.u.boot@aribaud.net:
Hello Masahiro,
On Mon, 13 Jul 2015 13:17:03 +0900, Masahiro Yamada yamada.masahiro@socionext.com wrote:
Please refer to the commit message of 06/14 for what this series wants to do.
Remark: you could use "Series-notes:" in 6/14 to have patman directly include said notes here instead of referring the reader to the patch.
Masahiro Yamada (14): x86: delete unneeded declarations of disable_irq() and enable_irq() linux_compat: remove cpu_relax() define linux_compat: move vzalloc() to header file as an inline function linux_compat: handle __GFP_ZERO in kmalloc() dm: add DM_FLAG_BOUND flag devres: introduce Devres (Managed Device Resource) framework devres: add devm_kmalloc() and friends (managed memory allocators) dm: refactor device_bind() and device_unbind() with devm_kzalloc() dm: merge fail_alloc labels linux_compat: introduce GFP_DMA flag for kmalloc() dm: refactor device_probe() and device_remove() with devm_kzalloc() devres: add debug command to dump device resources dm: remove redundant CONFIG_DM from driver/core/Makefile devres: compile Devres iif CONFIG_DM_DEVICE_REMOVE is enabled
I am still unsure why this is necessary. I had a discussion on the list with Simon, see last message here:
https://www.mail-archive.com/u-boot@lists.denx.de/msg177031.html
Unless I'm mistaken, the only case where we actually have a leak that this series would fix is in some cases of binding USB devices / drivers multiple times, and even then, it would take a considerable amount of repeated bindings before U-Boot could become unable to boot a payload; a scenario that I find unlikely.
I do understand the general goal of fixing bugs when we ind them; but I question the global benefit of fixing this specific leak bug by adding a whole new feature with a lot of code to U-Boot, as opposed to fixing it in a more ad hoc way with much less less code volume and complexity.
You have a point.
What we really want to avoid is to make low-level drivers too complicated by leaving the correct memory management to each of them.
After all, there turned out to be two options to solve it.
[1] Simon's driver model: move allocating/freeing memory to the driver core by having only the needed memory size specified in each driver [2] Devres: we still have to allocate memory in each driver, but we need not free it explicitly, making our driver work much easier
[1] and [2] are completely differently approach, but what they solve is the same: easier memory (resource) management without leak.
Understood.
IIUC, this adds a new leak scenario beside the multiple inits one such as for USB, but this new scenarion which is in failure paths where already allocated memory must be released, seems to me no more critical than the one I'd discussed with Simon, i.e., I'm not convinced we need a fix, and I'm not convinced we need a general memory management feature for that -- which does not mean I can't be convinced, though; I just need (more) convincing arguments (than I can currently read).
BTW, I am not following the USB-discussion between Simon and you. I have not checked the USB changes recently, so I am not familiar enough with it to add my comment.
The only problem I see in [1] is that it is not controllable at run-time. The memory size for the auto-allocation must be specified at the compile time.
How in practice is that a problem?
At first, I thought (or expected) that .priv_auto_alloc_size and friends were generic enough to cover all the cases, but they were not.
So, we need calloc() and free() in some low-level drivers. Simon might say they are only a few "exceptions", (my impression is I often hear the logic such as "it is not a problem because we do not have many.") Anyway, we had already lost the consistency as for memory allocation.
Not sure I understand that last sentence. Which consistency exactly have we lost?
When I write drivers for Linux, I usually allocate memory for driver data with devm_kzalloc().
But, in U-boot, sometimes we specify .priv_auto_alloc_size, and sometimes we call calloc() and free().
This is what I called lost-consistency.
I imagined if [2] had been supported earlier, we would not have needed [1]. (at least, [2] seems more flexible to me.)
We already have many DM-based drivers, and I think we can live with [1] despite some exceptional drivers allocating memory on their own.
So, if Simon (and other developers) does not feel comfortable with this series, I do not mind discarding it.
Note that I'm not saying your patch series does not solve the issue of leaks. What I'm saying is i) do we need to solve this issue, and ii) if we do, is your series the best option ?
Sorry, if I am misunderstanding your questions.
If you are talking about the generic program guideline, I think leaks should be eliminated and this series should be helpful (but I wouldn't say as far as it is the best).
If you are asking if this series is the best for solving some particular issues, sorry, I can not comment on what I am not familiar with.
Hello Masahiro,
On Mon, 13 Jul 2015 20:42:15 +0900, Masahiro Yamada yamada.masahiro@socionext.com wrote:
2015-07-13 19:55 GMT+09:00 Albert ARIBAUD albert.u.boot@aribaud.net:
Hello Masahiro,
On Mon, 13 Jul 2015 16:39:45 +0900, Masahiro Yamada yamada.masahiro@socionext.com wrote:
Hi Albert,
2015-07-13 15:51 GMT+09:00 Albert ARIBAUD albert.u.boot@aribaud.net:
Hello Masahiro,
On Mon, 13 Jul 2015 13:17:03 +0900, Masahiro Yamada yamada.masahiro@socionext.com wrote:
Please refer to the commit message of 06/14 for what this series wants to do.
Remark: you could use "Series-notes:" in 6/14 to have patman directly include said notes here instead of referring the reader to the patch.
Masahiro Yamada (14): x86: delete unneeded declarations of disable_irq() and enable_irq() linux_compat: remove cpu_relax() define linux_compat: move vzalloc() to header file as an inline function linux_compat: handle __GFP_ZERO in kmalloc() dm: add DM_FLAG_BOUND flag devres: introduce Devres (Managed Device Resource) framework devres: add devm_kmalloc() and friends (managed memory allocators) dm: refactor device_bind() and device_unbind() with devm_kzalloc() dm: merge fail_alloc labels linux_compat: introduce GFP_DMA flag for kmalloc() dm: refactor device_probe() and device_remove() with devm_kzalloc() devres: add debug command to dump device resources dm: remove redundant CONFIG_DM from driver/core/Makefile devres: compile Devres iif CONFIG_DM_DEVICE_REMOVE is enabled
I am still unsure why this is necessary. I had a discussion on the list with Simon, see last message here:
https://www.mail-archive.com/u-boot@lists.denx.de/msg177031.html
Unless I'm mistaken, the only case where we actually have a leak that this series would fix is in some cases of binding USB devices / drivers multiple times, and even then, it would take a considerable amount of repeated bindings before U-Boot could become unable to boot a payload; a scenario that I find unlikely.
I do understand the general goal of fixing bugs when we ind them; but I question the global benefit of fixing this specific leak bug by adding a whole new feature with a lot of code to U-Boot, as opposed to fixing it in a more ad hoc way with much less less code volume and complexity.
You have a point.
What we really want to avoid is to make low-level drivers too complicated by leaving the correct memory management to each of them.
After all, there turned out to be two options to solve it.
[1] Simon's driver model: move allocating/freeing memory to the driver core by having only the needed memory size specified in each driver [2] Devres: we still have to allocate memory in each driver, but we need not free it explicitly, making our driver work much easier
[1] and [2] are completely differently approach, but what they solve is the same: easier memory (resource) management without leak.
Understood.
IIUC, this adds a new leak scenario beside the multiple inits one such as for USB, but this new scenarion which is in failure paths where already allocated memory must be released, seems to me no more critical than the one I'd discussed with Simon, i.e., I'm not convinced we need a fix, and I'm not convinced we need a general memory management feature for that -- which does not mean I can't be convinced, though; I just need (more) convincing arguments (than I can currently read).
BTW, I am not following the USB-discussion between Simon and you. I have not checked the USB changes recently, so I am not familiar enough with it to add my comment.
It was not actually a USB discussion. It was a discussion within v1 of this patch series. I'd asked when exactly there could be leaks in our current driver user scenarios and Simon said there was no leak case except in USB which could be bound several times in U-Boot between power-on and OS boot.
The only problem I see in [1] is that it is not controllable at run-time. The memory size for the auto-allocation must be specified at the compile time.
How in practice is that a problem?
At first, I thought (or expected) that .priv_auto_alloc_size and friends were generic enough to cover all the cases, but they were not.
I'm afraid I am still not seeing the problem.
So, we need calloc() and free() in some low-level drivers. Simon might say they are only a few "exceptions", (my impression is I often hear the logic such as "it is not a problem because we do not have many.") Anyway, we had already lost the consistency as for memory allocation.
Not sure I understand that last sentence. Which consistency exactly have we lost?
When I write drivers for Linux, I usually allocate memory for driver data with devm_kzalloc().
But, in U-boot, sometimes we specify .priv_auto_alloc_size, and sometimes we call calloc() and free().
This is what I called lost-consistency.
Ok, now I get this point.
I imagined if [2] had been supported earlier, we would not have needed [1]. (at least, [2] seems more flexible to me.)
We already have many DM-based drivers, and I think we can live with [1] despite some exceptional drivers allocating memory on their own.
So, if Simon (and other developers) does not feel comfortable with this series, I do not mind discarding it.
Note that I'm not saying your patch series does not solve the issue of leaks. What I'm saying is i) do we need to solve this issue, and ii) if we do, is your series the best option ?
Sorry, if I am misunderstanding your questions.
If you are talking about the generic program guideline, I think leaks should be eliminated and this series should be helpful (but I wouldn't say as far as it is the best).
If you are asking if this series is the best for solving some particular issues, sorry, I can not comment on what I am not familiar with.
Let me recap :) as a series of simple yes/no sentences.
- Sometimes in driver code there are memory leaks.
- These leaks are bugs.
- These leaks do not prevent U-Boot from functioning properly.
- There are two methods to eliminate these leaks: Simon's method of allocating / freeing driver/device memory outside driver code per se, and your proposed method of dynamically tracking memory allocated by a driver / device, and freeing it when the driver gets 'unloaded' -- akin to freeing a process' resources when it terminates.
- Each method has advantages and disadvantages.
My opinion for now is that:
- We do not /need/ to fix the leaks, we /would like/ to.
- since we don't /need/ to fix the leaks, we can afford to be picky about how we will fix them, or even whether we will fix them at all if no solution pleases us.
- 'being picky' means we should consider the pros and cons of available methods, not only wrt the fix itself, but more generally too: Does it fix other issues? Does it hinder or encourage best practices? how much source code does it bring in? etc.
Right now, I'm not even certain we have a problem at all, in the sense that I don't recall seeing reports of a target failing to boot because of memory exhaustion in U-Boot -- which means that, to me, the 'pros' of any leak fix solution would start thin since they would solve a non-problem, or more precisely, a not-really-a-problem, and I fail to see the other pros (OTOH, the cons are not /that/ many either).
But I might be mistaken, so I'm asking for actual scenarios where a target did have problems doing its job due to memory allocation issues, and scenarios for other pros as well, and comments from anyone who would have a good idea of the cons and pros.
-- Best Regards Masahiro Yamada
Amicalement,
+Hans
Hi,
On 13 July 2015 at 11:16, Albert ARIBAUD albert.u.boot@aribaud.net wrote:
Hello Masahiro,
On Mon, 13 Jul 2015 20:42:15 +0900, Masahiro Yamada yamada.masahiro@socionext.com wrote:
2015-07-13 19:55 GMT+09:00 Albert ARIBAUD albert.u.boot@aribaud.net:
Hello Masahiro,
On Mon, 13 Jul 2015 16:39:45 +0900, Masahiro Yamada yamada.masahiro@socionext.com wrote:
Hi Albert,
2015-07-13 15:51 GMT+09:00 Albert ARIBAUD albert.u.boot@aribaud.net:
Hello Masahiro,
On Mon, 13 Jul 2015 13:17:03 +0900, Masahiro Yamada yamada.masahiro@socionext.com wrote:
Please refer to the commit message of 06/14 for what this series wants to do.
Remark: you could use "Series-notes:" in 6/14 to have patman directly include said notes here instead of referring the reader to the patch.
Masahiro Yamada (14): x86: delete unneeded declarations of disable_irq() and enable_irq() linux_compat: remove cpu_relax() define linux_compat: move vzalloc() to header file as an inline function linux_compat: handle __GFP_ZERO in kmalloc() dm: add DM_FLAG_BOUND flag devres: introduce Devres (Managed Device Resource) framework devres: add devm_kmalloc() and friends (managed memory allocators) dm: refactor device_bind() and device_unbind() with devm_kzalloc() dm: merge fail_alloc labels linux_compat: introduce GFP_DMA flag for kmalloc() dm: refactor device_probe() and device_remove() with devm_kzalloc() devres: add debug command to dump device resources dm: remove redundant CONFIG_DM from driver/core/Makefile devres: compile Devres iif CONFIG_DM_DEVICE_REMOVE is enabled
I am still unsure why this is necessary. I had a discussion on the list with Simon, see last message here:
https://www.mail-archive.com/u-boot@lists.denx.de/msg177031.html
Unless I'm mistaken, the only case where we actually have a leak that this series would fix is in some cases of binding USB devices / drivers multiple times, and even then, it would take a considerable amount of repeated bindings before U-Boot could become unable to boot a payload; a scenario that I find unlikely.
I do understand the general goal of fixing bugs when we ind them; but I question the global benefit of fixing this specific leak bug by adding a whole new feature with a lot of code to U-Boot, as opposed to fixing it in a more ad hoc way with much less less code volume and complexity.
You have a point.
What we really want to avoid is to make low-level drivers too complicated by leaving the correct memory management to each of them.
After all, there turned out to be two options to solve it.
[1] Simon's driver model: move allocating/freeing memory to the driver core by having only the needed memory size specified in each driver [2] Devres: we still have to allocate memory in each driver, but we need not free it explicitly, making our driver work much easier
[1] and [2] are completely differently approach, but what they solve is the same: easier memory (resource) management without leak.
Understood.
IIUC, this adds a new leak scenario beside the multiple inits one such as for USB, but this new scenarion which is in failure paths where already allocated memory must be released, seems to me no more critical than the one I'd discussed with Simon, i.e., I'm not convinced we need a fix, and I'm not convinced we need a general memory management feature for that -- which does not mean I can't be convinced, though; I just need (more) convincing arguments (than I can currently read).
BTW, I am not following the USB-discussion between Simon and you. I have not checked the USB changes recently, so I am not familiar enough with it to add my comment.
It was not actually a USB discussion. It was a discussion within v1 of this patch series. I'd asked when exactly there could be leaks in our current driver user scenarios and Simon said there was no leak case except in USB which could be bound several times in U-Boot between power-on and OS boot.
The only problem I see in [1] is that it is not controllable at run-time. The memory size for the auto-allocation must be specified at the compile time.
How in practice is that a problem?
At first, I thought (or expected) that .priv_auto_alloc_size and friends were generic enough to cover all the cases, but they were not.
I'm afraid I am still not seeing the problem.
So, we need calloc() and free() in some low-level drivers. Simon might say they are only a few "exceptions", (my impression is I often hear the logic such as "it is not a problem because we do not have many.") Anyway, we had already lost the consistency as for memory allocation.
Not sure I understand that last sentence. Which consistency exactly have we lost?
When I write drivers for Linux, I usually allocate memory for driver data with devm_kzalloc().
But, in U-boot, sometimes we specify .priv_auto_alloc_size, and sometimes we call calloc() and free().
This is what I called lost-consistency.
Ok, now I get this point.
I imagined if [2] had been supported earlier, we would not have needed [1]. (at least, [2] seems more flexible to me.)
We already have many DM-based drivers, and I think we can live with [1] despite some exceptional drivers allocating memory on their own.
So, if Simon (and other developers) does not feel comfortable with this series, I do not mind discarding it.
Note that I'm not saying your patch series does not solve the issue of leaks. What I'm saying is i) do we need to solve this issue, and ii) if we do, is your series the best option ?
Sorry, if I am misunderstanding your questions.
If you are talking about the generic program guideline, I think leaks should be eliminated and this series should be helpful (but I wouldn't say as far as it is the best).
If you are asking if this series is the best for solving some particular issues, sorry, I can not comment on what I am not familiar with.
Let me recap :) as a series of simple yes/no sentences.
Sometimes in driver code there are memory leaks.
These leaks are bugs.
These leaks do not prevent U-Boot from functioning properly.
There are two methods to eliminate these leaks: Simon's method of allocating / freeing driver/device memory outside driver code per se, and your proposed method of dynamically tracking memory allocated by a driver / device, and freeing it when the driver gets 'unloaded' -- akin to freeing a process' resources when it terminates.
Each method has advantages and disadvantages.
My opinion for now is that:
We do not /need/ to fix the leaks, we /would like/ to.
since we don't /need/ to fix the leaks, we can afford to be picky about how we will fix them, or even whether we will fix them at all if no solution pleases us.
'being picky' means we should consider the pros and cons of available methods, not only wrt the fix itself, but more generally too: Does it fix other issues? Does it hinder or encourage best practices? how much source code does it bring in? etc.
Right now, I'm not even certain we have a problem at all, in the sense that I don't recall seeing reports of a target failing to boot because of memory exhaustion in U-Boot -- which means that, to me, the 'pros' of any leak fix solution would start thin since they would solve a non-problem, or more precisely, a not-really-a-problem, and I fail to see the other pros (OTOH, the cons are not /that/ many either).
But I might be mistaken, so I'm asking for actual scenarios where a target did have problems doing its job due to memory allocation issues, and scenarios for other pros as well, and comments from anyone who would have a good idea of the cons and pros.
It's a valuable point of view. My instinct is often to bring things in and expand the platform. But there needs to be a strong benefit.
As things stand I am also unsure of this. Driver model was designed to put memory allocation inside the core code (driver/core/device.c). Very few devices allocate memory at present () and perhaps even fewer need to. Here's a list of:
$ grep "alloc(" `git grep -l U_BOOT_DRIVER` arch/x86/cpu/ivybridge/bd82x6x.c: cpu = calloc(1, sizeof(*cpu)); drivers/gpio/mxc_gpio.c: plat = calloc(1, sizeof(*plat)); drivers/gpio/s5p_gpio.c: plat = calloc(1, sizeof(*plat)); drivers/gpio/sandbox.c: dev->priv = calloc(sizeof(struct gpio_state), uc_priv->gpio_count); drivers/gpio/sunxi_gpio.c: name = malloc(3); drivers/gpio/sunxi_gpio.c: plat = calloc(1, sizeof(*plat)); drivers/gpio/tegra_gpio.c: name = malloc(3); drivers/gpio/tegra_gpio.c: plat = calloc(1, sizeof(*plat)); drivers/gpio/vybrid_gpio.c: plat = calloc(1, sizeof(*plat)); drivers/i2c/i2c-uclass.c: * Use the stack for small messages, malloc() for larger ones. We drivers/i2c/i2c-uclass.c: buf = malloc(I2C_MAX_OFFSET_LEN + len); drivers/misc/cros_ec_sandbox.c: ec->flash_data = os_malloc(len); drivers/misc/cros_ec_sandbox.c: ec->matrix = calloc(ec->matrix_count, sizeof(*ec->matrix)); drivers/misc/cros_ec_sandbox.c: ec->flash_data = os_malloc(ec->flash_data_len); drivers/misc/i2c_eeprom_emul.c: priv->data = calloc(1, plat->size); drivers/mtd/spi/sf_probe.c: flash = calloc(1, sizeof(*flash)); drivers/net/designware.c: struct mii_dev *bus = mdio_alloc(); drivers/net/designware.c: dev = (struct eth_device *) malloc(sizeof(struct eth_device)); drivers/net/sunxi_emac.c: priv->bus = mdio_alloc(); drivers/spi/sandbox_spi.c: tx = malloc(bytes); drivers/spi/sandbox_spi.c: rx = malloc(bytes); test/dm/test-driver.c: dev->priv = calloc(1, sizeof(struct dm_test_priv));
Most of those are temporary allocations are unnecessary but some could use devres.
The v2 series solves the SPL size issue, except that with Han's USB changes we have to device DM_REMOVE when we use USB. I am seriously reconsidering that as a limitation that might be best avoided.
But we now require devres as a core feature - this pushes up the overhead of driver rmodel. I really like this patch:
http://patchwork.ozlabs.org/patch/494216/
but I don't think it goes far enough.
I'd like to figure this out before moving to v3. Here is my proposal:
1. Add CONFIG_DEVRES as an option which can be enabled if wanted, but is not required for things to work.
2. Drop the use of devres to remove()/unbind() devices. The core DM code can stick with its existing manual free() calls.
3. devres_head should only exist in struct device when CONFIG_DEVRES is set.
4. Convert over a driver to use devres in sandbox and enable it. One option would be cros_ec_sandbox. It reads the device tree key map and it is variable size so we can't use the automatic memory allocation. Need to add a remove() method!
5. See how much use devres gets over the next year. We haven't lost any efficiency and we gain a useful feature to track device memory allocation.
Masahiro, Albert what do you think?
Regards, Simon
Hi Simon, Albert.
2015-07-18 23:37 GMT+09:00 Simon Glass sjg@chromium.org:
+Hans
Hi,
On 13 July 2015 at 11:16, Albert ARIBAUD albert.u.boot@aribaud.net wrote:
Hello Masahiro,
On Mon, 13 Jul 2015 20:42:15 +0900, Masahiro Yamada yamada.masahiro@socionext.com wrote:
2015-07-13 19:55 GMT+09:00 Albert ARIBAUD albert.u.boot@aribaud.net:
Hello Masahiro,
On Mon, 13 Jul 2015 16:39:45 +0900, Masahiro Yamada yamada.masahiro@socionext.com wrote:
Hi Albert,
2015-07-13 15:51 GMT+09:00 Albert ARIBAUD albert.u.boot@aribaud.net:
Hello Masahiro,
On Mon, 13 Jul 2015 13:17:03 +0900, Masahiro Yamada yamada.masahiro@socionext.com wrote: > > Please refer to the commit message of 06/14 > for what this series wants to do.
Remark: you could use "Series-notes:" in 6/14 to have patman directly include said notes here instead of referring the reader to the patch.
> Masahiro Yamada (14): > x86: delete unneeded declarations of disable_irq() and enable_irq() > linux_compat: remove cpu_relax() define > linux_compat: move vzalloc() to header file as an inline function > linux_compat: handle __GFP_ZERO in kmalloc() > dm: add DM_FLAG_BOUND flag > devres: introduce Devres (Managed Device Resource) framework > devres: add devm_kmalloc() and friends (managed memory allocators) > dm: refactor device_bind() and device_unbind() with devm_kzalloc() > dm: merge fail_alloc labels > linux_compat: introduce GFP_DMA flag for kmalloc() > dm: refactor device_probe() and device_remove() with devm_kzalloc() > devres: add debug command to dump device resources > dm: remove redundant CONFIG_DM from driver/core/Makefile > devres: compile Devres iif CONFIG_DM_DEVICE_REMOVE is enabled
I am still unsure why this is necessary. I had a discussion on the list with Simon, see last message here:
https://www.mail-archive.com/u-boot@lists.denx.de/msg177031.html
Unless I'm mistaken, the only case where we actually have a leak that this series would fix is in some cases of binding USB devices / drivers multiple times, and even then, it would take a considerable amount of repeated bindings before U-Boot could become unable to boot a payload; a scenario that I find unlikely.
I do understand the general goal of fixing bugs when we ind them; but I question the global benefit of fixing this specific leak bug by adding a whole new feature with a lot of code to U-Boot, as opposed to fixing it in a more ad hoc way with much less less code volume and complexity.
You have a point.
What we really want to avoid is to make low-level drivers too complicated by leaving the correct memory management to each of them.
After all, there turned out to be two options to solve it.
[1] Simon's driver model: move allocating/freeing memory to the driver core by having only the needed memory size specified in each driver [2] Devres: we still have to allocate memory in each driver, but we need not free it explicitly, making our driver work much easier
[1] and [2] are completely differently approach, but what they solve is the same: easier memory (resource) management without leak.
Understood.
IIUC, this adds a new leak scenario beside the multiple inits one such as for USB, but this new scenarion which is in failure paths where already allocated memory must be released, seems to me no more critical than the one I'd discussed with Simon, i.e., I'm not convinced we need a fix, and I'm not convinced we need a general memory management feature for that -- which does not mean I can't be convinced, though; I just need (more) convincing arguments (than I can currently read).
BTW, I am not following the USB-discussion between Simon and you. I have not checked the USB changes recently, so I am not familiar enough with it to add my comment.
It was not actually a USB discussion. It was a discussion within v1 of this patch series. I'd asked when exactly there could be leaks in our current driver user scenarios and Simon said there was no leak case except in USB which could be bound several times in U-Boot between power-on and OS boot.
The only problem I see in [1] is that it is not controllable at run-time. The memory size for the auto-allocation must be specified at the compile time.
How in practice is that a problem?
At first, I thought (or expected) that .priv_auto_alloc_size and friends were generic enough to cover all the cases, but they were not.
I'm afraid I am still not seeing the problem.
So, we need calloc() and free() in some low-level drivers. Simon might say they are only a few "exceptions", (my impression is I often hear the logic such as "it is not a problem because we do not have many.") Anyway, we had already lost the consistency as for memory allocation.
Not sure I understand that last sentence. Which consistency exactly have we lost?
When I write drivers for Linux, I usually allocate memory for driver data with devm_kzalloc().
But, in U-boot, sometimes we specify .priv_auto_alloc_size, and sometimes we call calloc() and free().
This is what I called lost-consistency.
Ok, now I get this point.
I imagined if [2] had been supported earlier, we would not have needed [1]. (at least, [2] seems more flexible to me.)
We already have many DM-based drivers, and I think we can live with [1] despite some exceptional drivers allocating memory on their own.
So, if Simon (and other developers) does not feel comfortable with this series, I do not mind discarding it.
Note that I'm not saying your patch series does not solve the issue of leaks. What I'm saying is i) do we need to solve this issue, and ii) if we do, is your series the best option ?
Sorry, if I am misunderstanding your questions.
If you are talking about the generic program guideline, I think leaks should be eliminated and this series should be helpful (but I wouldn't say as far as it is the best).
If you are asking if this series is the best for solving some particular issues, sorry, I can not comment on what I am not familiar with.
Let me recap :) as a series of simple yes/no sentences.
Sometimes in driver code there are memory leaks.
These leaks are bugs.
These leaks do not prevent U-Boot from functioning properly.
There are two methods to eliminate these leaks: Simon's method of allocating / freeing driver/device memory outside driver code per se, and your proposed method of dynamically tracking memory allocated by a driver / device, and freeing it when the driver gets 'unloaded' -- akin to freeing a process' resources when it terminates.
Each method has advantages and disadvantages.
My opinion for now is that:
We do not /need/ to fix the leaks, we /would like/ to.
since we don't /need/ to fix the leaks, we can afford to be picky about how we will fix them, or even whether we will fix them at all if no solution pleases us.
'being picky' means we should consider the pros and cons of available methods, not only wrt the fix itself, but more generally too: Does it fix other issues? Does it hinder or encourage best practices? how much source code does it bring in? etc.
Right now, I'm not even certain we have a problem at all, in the sense that I don't recall seeing reports of a target failing to boot because of memory exhaustion in U-Boot -- which means that, to me, the 'pros' of any leak fix solution would start thin since they would solve a non-problem, or more precisely, a not-really-a-problem, and I fail to see the other pros (OTOH, the cons are not /that/ many either).
But I might be mistaken, so I'm asking for actual scenarios where a target did have problems doing its job due to memory allocation issues, and scenarios for other pros as well, and comments from anyone who would have a good idea of the cons and pros.
It's a valuable point of view. My instinct is often to bring things in and expand the platform. But there needs to be a strong benefit.
As things stand I am also unsure of this. Driver model was designed to put memory allocation inside the core code (driver/core/device.c). Very few devices allocate memory at present () and perhaps even fewer need to. Here's a list of:
$ grep "alloc(" `git grep -l U_BOOT_DRIVER` arch/x86/cpu/ivybridge/bd82x6x.c: cpu = calloc(1, sizeof(*cpu)); drivers/gpio/mxc_gpio.c: plat = calloc(1, sizeof(*plat)); drivers/gpio/s5p_gpio.c: plat = calloc(1, sizeof(*plat)); drivers/gpio/sandbox.c: dev->priv = calloc(sizeof(struct gpio_state), uc_priv->gpio_count); drivers/gpio/sunxi_gpio.c: name = malloc(3); drivers/gpio/sunxi_gpio.c: plat = calloc(1, sizeof(*plat)); drivers/gpio/tegra_gpio.c: name = malloc(3); drivers/gpio/tegra_gpio.c: plat = calloc(1, sizeof(*plat)); drivers/gpio/vybrid_gpio.c: plat = calloc(1, sizeof(*plat)); drivers/i2c/i2c-uclass.c: * Use the stack for small messages, malloc() for larger ones. We drivers/i2c/i2c-uclass.c: buf = malloc(I2C_MAX_OFFSET_LEN + len); drivers/misc/cros_ec_sandbox.c: ec->flash_data = os_malloc(len); drivers/misc/cros_ec_sandbox.c: ec->matrix = calloc(ec->matrix_count, sizeof(*ec->matrix)); drivers/misc/cros_ec_sandbox.c: ec->flash_data = os_malloc(ec->flash_data_len); drivers/misc/i2c_eeprom_emul.c: priv->data = calloc(1, plat->size); drivers/mtd/spi/sf_probe.c: flash = calloc(1, sizeof(*flash)); drivers/net/designware.c: struct mii_dev *bus = mdio_alloc(); drivers/net/designware.c: dev = (struct eth_device *) malloc(sizeof(struct eth_device)); drivers/net/sunxi_emac.c: priv->bus = mdio_alloc(); drivers/spi/sandbox_spi.c: tx = malloc(bytes); drivers/spi/sandbox_spi.c: rx = malloc(bytes); test/dm/test-driver.c: dev->priv = calloc(1, sizeof(struct dm_test_priv));
Most of those are temporary allocations are unnecessary but some could use devres.
The v2 series solves the SPL size issue, except that with Han's USB changes we have to device DM_REMOVE when we use USB. I am seriously reconsidering that as a limitation that might be best avoided.
But we now require devres as a core feature - this pushes up the overhead of driver rmodel. I really like this patch:
http://patchwork.ozlabs.org/patch/494216/
but I don't think it goes far enough.
I'd like to figure this out before moving to v3. Here is my proposal:
- Add CONFIG_DEVRES as an option which can be enabled if wanted, but
is not required for things to work.
This is a boot-loader, so it is a quite rare scenario to attach and detach drivers over and over again. Memory exhaustion would never happen (at least in normal usage) even if memory is never freed.
Things always work well, in other words, there is no use-case where precise resource management is our requirement.
So, CONFIG_DEVRES can be optional.
Memory may leak if CONFIG_DEVRES is disabled, but that is OK. That is not what we care about very much.
This is what I understood so far.
If so, CONFIG_DEVRES and CONFIG_DM_REMOVE eventually have similar concept: We prepared these features just in case, but we actually do not need them, so they are the first things we want to disable in memory-limited cases.
We had already disabled CONFIG_DM_REMOVE where we really want to save memory footprint, such as SPL. I do not think we will get further benefit by adding CONFIG_DEVRES here.
Also, another similar concept we have is, malloc_simple, where free() does nothing, we cannot retrieve freed memory.
I think too many choices will result in making things messy.
- Drop the use of devres to remove()/unbind() devices. The core DM
code can stick with its existing manual free() calls.
devres_head should only exist in struct device when CONFIG_DEVRES is set.
Convert over a driver to use devres in sandbox and enable it. One
option would be cros_ec_sandbox. It reads the device tree key map and it is variable size so we can't use the automatic memory allocation. Need to add a remove() method!
- See how much use devres gets over the next year. We haven't lost
any efficiency and we gain a useful feature to track device memory allocation.
Masahiro, Albert what do you think?
Regards, Simon
From our discussion so far, my frank feeling is, this series is unwelcome.
Rather than pulling in what we are unsure about the necessity, won't it be better to defer it to see if it is really useful or not? (i.e. how many drivers want to do malloc on their own.)
Anyway, we can dig it in Patchwork anytime we find it necessary.
Hi Masahiro,
On 20 July 2015 at 00:21, Masahiro Yamada yamada.masahiro@socionext.com wrote:
Hi Simon, Albert.
2015-07-18 23:37 GMT+09:00 Simon Glass sjg@chromium.org:
+Hans
Hi,
On 13 July 2015 at 11:16, Albert ARIBAUD albert.u.boot@aribaud.net wrote:
Hello Masahiro,
On Mon, 13 Jul 2015 20:42:15 +0900, Masahiro Yamada yamada.masahiro@socionext.com wrote:
2015-07-13 19:55 GMT+09:00 Albert ARIBAUD albert.u.boot@aribaud.net:
Hello Masahiro,
On Mon, 13 Jul 2015 16:39:45 +0900, Masahiro Yamada yamada.masahiro@socionext.com wrote:
Hi Albert,
2015-07-13 15:51 GMT+09:00 Albert ARIBAUD albert.u.boot@aribaud.net: > Hello Masahiro, > > On Mon, 13 Jul 2015 13:17:03 +0900, Masahiro Yamada > yamada.masahiro@socionext.com wrote: >> >> Please refer to the commit message of 06/14 >> for what this series wants to do. > > Remark: you could use "Series-notes:" in 6/14 to have patman directly > include said notes here instead of referring the reader to the patch. > >> Masahiro Yamada (14): >> x86: delete unneeded declarations of disable_irq() and enable_irq() >> linux_compat: remove cpu_relax() define >> linux_compat: move vzalloc() to header file as an inline function >> linux_compat: handle __GFP_ZERO in kmalloc() >> dm: add DM_FLAG_BOUND flag >> devres: introduce Devres (Managed Device Resource) framework >> devres: add devm_kmalloc() and friends (managed memory allocators) >> dm: refactor device_bind() and device_unbind() with devm_kzalloc() >> dm: merge fail_alloc labels >> linux_compat: introduce GFP_DMA flag for kmalloc() >> dm: refactor device_probe() and device_remove() with devm_kzalloc() >> devres: add debug command to dump device resources >> dm: remove redundant CONFIG_DM from driver/core/Makefile >> devres: compile Devres iif CONFIG_DM_DEVICE_REMOVE is enabled > > I am still unsure why this is necessary. I had a discussion on the > list with Simon, see last message here: > > https://www.mail-archive.com/u-boot@lists.denx.de/msg177031.html > > Unless I'm mistaken, the only case where we actually have a leak that > this series would fix is in some cases of binding USB devices / drivers > multiple times, and even then, it would take a considerable amount of > repeated bindings before U-Boot could become unable to boot a payload; > a scenario that I find unlikely. > > I do understand the general goal of fixing bugs when we ind them; but I > question the global benefit of fixing this specific leak bug by adding a > whole new feature with a lot of code to U-Boot, as opposed to fixing > it in a more ad hoc way with much less less code volume and complexity.
You have a point.
What we really want to avoid is to make low-level drivers too complicated by leaving the correct memory management to each of them.
After all, there turned out to be two options to solve it.
[1] Simon's driver model: move allocating/freeing memory to the driver core by having only the needed memory size specified in each driver [2] Devres: we still have to allocate memory in each driver, but we need not free it explicitly, making our driver work much easier
[1] and [2] are completely differently approach, but what they solve is the same: easier memory (resource) management without leak.
Understood.
IIUC, this adds a new leak scenario beside the multiple inits one such as for USB, but this new scenarion which is in failure paths where already allocated memory must be released, seems to me no more critical than the one I'd discussed with Simon, i.e., I'm not convinced we need a fix, and I'm not convinced we need a general memory management feature for that -- which does not mean I can't be convinced, though; I just need (more) convincing arguments (than I can currently read).
BTW, I am not following the USB-discussion between Simon and you. I have not checked the USB changes recently, so I am not familiar enough with it to add my comment.
It was not actually a USB discussion. It was a discussion within v1 of this patch series. I'd asked when exactly there could be leaks in our current driver user scenarios and Simon said there was no leak case except in USB which could be bound several times in U-Boot between power-on and OS boot.
The only problem I see in [1] is that it is not controllable at run-time. The memory size for the auto-allocation must be specified at the compile time.
How in practice is that a problem?
At first, I thought (or expected) that .priv_auto_alloc_size and friends were generic enough to cover all the cases, but they were not.
I'm afraid I am still not seeing the problem.
So, we need calloc() and free() in some low-level drivers. Simon might say they are only a few "exceptions", (my impression is I often hear the logic such as "it is not a problem because we do not have many.") Anyway, we had already lost the consistency as for memory allocation.
Not sure I understand that last sentence. Which consistency exactly have we lost?
When I write drivers for Linux, I usually allocate memory for driver data with devm_kzalloc().
But, in U-boot, sometimes we specify .priv_auto_alloc_size, and sometimes we call calloc() and free().
This is what I called lost-consistency.
Ok, now I get this point.
I imagined if [2] had been supported earlier, we would not have needed [1]. (at least, [2] seems more flexible to me.)
We already have many DM-based drivers, and I think we can live with [1] despite some exceptional drivers allocating memory on their own.
So, if Simon (and other developers) does not feel comfortable with this series, I do not mind discarding it.
Note that I'm not saying your patch series does not solve the issue of leaks. What I'm saying is i) do we need to solve this issue, and ii) if we do, is your series the best option ?
Sorry, if I am misunderstanding your questions.
If you are talking about the generic program guideline, I think leaks should be eliminated and this series should be helpful (but I wouldn't say as far as it is the best).
If you are asking if this series is the best for solving some particular issues, sorry, I can not comment on what I am not familiar with.
Let me recap :) as a series of simple yes/no sentences.
Sometimes in driver code there are memory leaks.
These leaks are bugs.
These leaks do not prevent U-Boot from functioning properly.
There are two methods to eliminate these leaks: Simon's method of allocating / freeing driver/device memory outside driver code per se, and your proposed method of dynamically tracking memory allocated by a driver / device, and freeing it when the driver gets 'unloaded' -- akin to freeing a process' resources when it terminates.
Each method has advantages and disadvantages.
My opinion for now is that:
We do not /need/ to fix the leaks, we /would like/ to.
since we don't /need/ to fix the leaks, we can afford to be picky about how we will fix them, or even whether we will fix them at all if no solution pleases us.
'being picky' means we should consider the pros and cons of available methods, not only wrt the fix itself, but more generally too: Does it fix other issues? Does it hinder or encourage best practices? how much source code does it bring in? etc.
Right now, I'm not even certain we have a problem at all, in the sense that I don't recall seeing reports of a target failing to boot because of memory exhaustion in U-Boot -- which means that, to me, the 'pros' of any leak fix solution would start thin since they would solve a non-problem, or more precisely, a not-really-a-problem, and I fail to see the other pros (OTOH, the cons are not /that/ many either).
But I might be mistaken, so I'm asking for actual scenarios where a target did have problems doing its job due to memory allocation issues, and scenarios for other pros as well, and comments from anyone who would have a good idea of the cons and pros.
It's a valuable point of view. My instinct is often to bring things in and expand the platform. But there needs to be a strong benefit.
As things stand I am also unsure of this. Driver model was designed to put memory allocation inside the core code (driver/core/device.c). Very few devices allocate memory at present () and perhaps even fewer need to. Here's a list of:
$ grep "alloc(" `git grep -l U_BOOT_DRIVER` arch/x86/cpu/ivybridge/bd82x6x.c: cpu = calloc(1, sizeof(*cpu)); drivers/gpio/mxc_gpio.c: plat = calloc(1, sizeof(*plat)); drivers/gpio/s5p_gpio.c: plat = calloc(1, sizeof(*plat)); drivers/gpio/sandbox.c: dev->priv = calloc(sizeof(struct gpio_state), uc_priv->gpio_count); drivers/gpio/sunxi_gpio.c: name = malloc(3); drivers/gpio/sunxi_gpio.c: plat = calloc(1, sizeof(*plat)); drivers/gpio/tegra_gpio.c: name = malloc(3); drivers/gpio/tegra_gpio.c: plat = calloc(1, sizeof(*plat)); drivers/gpio/vybrid_gpio.c: plat = calloc(1, sizeof(*plat)); drivers/i2c/i2c-uclass.c: * Use the stack for small messages, malloc() for larger ones. We drivers/i2c/i2c-uclass.c: buf = malloc(I2C_MAX_OFFSET_LEN + len); drivers/misc/cros_ec_sandbox.c: ec->flash_data = os_malloc(len); drivers/misc/cros_ec_sandbox.c: ec->matrix = calloc(ec->matrix_count, sizeof(*ec->matrix)); drivers/misc/cros_ec_sandbox.c: ec->flash_data = os_malloc(ec->flash_data_len); drivers/misc/i2c_eeprom_emul.c: priv->data = calloc(1, plat->size); drivers/mtd/spi/sf_probe.c: flash = calloc(1, sizeof(*flash)); drivers/net/designware.c: struct mii_dev *bus = mdio_alloc(); drivers/net/designware.c: dev = (struct eth_device *) malloc(sizeof(struct eth_device)); drivers/net/sunxi_emac.c: priv->bus = mdio_alloc(); drivers/spi/sandbox_spi.c: tx = malloc(bytes); drivers/spi/sandbox_spi.c: rx = malloc(bytes); test/dm/test-driver.c: dev->priv = calloc(1, sizeof(struct dm_test_priv));
Most of those are temporary allocations are unnecessary but some could use devres.
The v2 series solves the SPL size issue, except that with Han's USB changes we have to device DM_REMOVE when we use USB. I am seriously reconsidering that as a limitation that might be best avoided.
But we now require devres as a core feature - this pushes up the overhead of driver rmodel. I really like this patch:
http://patchwork.ozlabs.org/patch/494216/
but I don't think it goes far enough.
I'd like to figure this out before moving to v3. Here is my proposal:
- Add CONFIG_DEVRES as an option which can be enabled if wanted, but
is not required for things to work.
This is a boot-loader, so it is a quite rare scenario to attach and detach drivers over and over again. Memory exhaustion would never happen (at least in normal usage) even if memory is never freed.
Things always work well, in other words, there is no use-case where precise resource management is our requirement.
So, CONFIG_DEVRES can be optional.
I agree - the main thing I can think of is USB but even then it would mostly be a human fiddling with it. A normal device boot sequence would not rescan the bus multiple times.
Memory may leak if CONFIG_DEVRES is disabled, but that is OK. That is not what we care about very much.
This is what I understood so far.
If so, CONFIG_DEVRES and CONFIG_DM_REMOVE eventually have similar concept: We prepared these features just in case, but we actually do not need them, so they are the first things we want to disable in memory-limited cases.
We had already disabled CONFIG_DM_REMOVE where we really want to save memory footprint, such as SPL. I do not think we will get further benefit by adding CONFIG_DEVRES here.
Also, another similar concept we have is, malloc_simple, where free() does nothing, we cannot retrieve freed memory.
I think too many choices will result in making things messy.
We have found a case where we want CONFIG_DM_REMOVE in normal operation - shutting down USB before booting the OS.
Extra options introduce extra cases, but they are all different:
- malloc_simple - reduced code size for supporting malloc() - DM_REMOVE - drops all remove/unbind code (actually it doesn't drop it in drivers, which could be fixed) - DEVRES - supports manual memory allocation in drivers with automatic free
So I don't see any conflict here. Also I like the memory allocation improvements you have added.
- Drop the use of devres to remove()/unbind() devices. The core DM
code can stick with its existing manual free() calls.
devres_head should only exist in struct device when CONFIG_DEVRES is set.
Convert over a driver to use devres in sandbox and enable it. One
option would be cros_ec_sandbox. It reads the device tree key map and it is variable size so we can't use the automatic memory allocation. Need to add a remove() method!
- See how much use devres gets over the next year. We haven't lost
any efficiency and we gain a useful feature to track device memory allocation.
Masahiro, Albert what do you think?
Regards, Simon
From our discussion so far, my frank feeling is, this series is unwelcome.
Rather than pulling in what we are unsure about the necessity, won't it be better to defer it to see if it is really useful or not? (i.e. how many drivers want to do malloc on their own.)
Anyway, we can dig it in Patchwork anytime we find it necessary.
If we apply it then we can tell people to always use it when allocating memory manually in a driver. We can go through and make sure all existing cases use it.
Then we have options in the future when we need to enable it. If we don't apply it, we build up code that would need to be refactored later in this case.
Also devres is a concept understood in the kernel, and if we can bring it in without adding mandatory overhead, that seems like a win to me.
My preference is to apply it, but as an option. So long as sandbox uses it, this will ensure adequate test coverage. BTW we already have memory leak tests but I'm not sure if they will be enough to test devres.
Regards, Simon
Hi Simon,
2015-07-20 23:16 GMT+09:00 Simon Glass sjg@chromium.org:
Hi Masahiro,
On 20 July 2015 at 00:21, Masahiro Yamada yamada.masahiro@socionext.com wrote:
Hi Simon, Albert.
2015-07-18 23:37 GMT+09:00 Simon Glass sjg@chromium.org:
+Hans
Hi,
On 13 July 2015 at 11:16, Albert ARIBAUD albert.u.boot@aribaud.net wrote:
Hello Masahiro,
On Mon, 13 Jul 2015 20:42:15 +0900, Masahiro Yamada yamada.masahiro@socionext.com wrote:
2015-07-13 19:55 GMT+09:00 Albert ARIBAUD albert.u.boot@aribaud.net:
Hello Masahiro,
On Mon, 13 Jul 2015 16:39:45 +0900, Masahiro Yamada yamada.masahiro@socionext.com wrote: > Hi Albert, > > > 2015-07-13 15:51 GMT+09:00 Albert ARIBAUD albert.u.boot@aribaud.net: > > Hello Masahiro, > > > > On Mon, 13 Jul 2015 13:17:03 +0900, Masahiro Yamada > > yamada.masahiro@socionext.com wrote: > >> > >> Please refer to the commit message of 06/14 > >> for what this series wants to do. > > > > Remark: you could use "Series-notes:" in 6/14 to have patman directly > > include said notes here instead of referring the reader to the patch. > > > >> Masahiro Yamada (14): > >> x86: delete unneeded declarations of disable_irq() and enable_irq() > >> linux_compat: remove cpu_relax() define > >> linux_compat: move vzalloc() to header file as an inline function > >> linux_compat: handle __GFP_ZERO in kmalloc() > >> dm: add DM_FLAG_BOUND flag > >> devres: introduce Devres (Managed Device Resource) framework > >> devres: add devm_kmalloc() and friends (managed memory allocators) > >> dm: refactor device_bind() and device_unbind() with devm_kzalloc() > >> dm: merge fail_alloc labels > >> linux_compat: introduce GFP_DMA flag for kmalloc() > >> dm: refactor device_probe() and device_remove() with devm_kzalloc() > >> devres: add debug command to dump device resources > >> dm: remove redundant CONFIG_DM from driver/core/Makefile > >> devres: compile Devres iif CONFIG_DM_DEVICE_REMOVE is enabled > > > > I am still unsure why this is necessary. I had a discussion on the > > list with Simon, see last message here: > > > > https://www.mail-archive.com/u-boot@lists.denx.de/msg177031.html > > > > Unless I'm mistaken, the only case where we actually have a leak that > > this series would fix is in some cases of binding USB devices / drivers > > multiple times, and even then, it would take a considerable amount of > > repeated bindings before U-Boot could become unable to boot a payload; > > a scenario that I find unlikely. > > > > I do understand the general goal of fixing bugs when we ind them; but I > > question the global benefit of fixing this specific leak bug by adding a > > whole new feature with a lot of code to U-Boot, as opposed to fixing > > it in a more ad hoc way with much less less code volume and complexity. > > > You have a point. > > What we really want to avoid is to make low-level drivers too complicated > by leaving the correct memory management to each of them. > > After all, there turned out to be two options to solve it. > > [1] Simon's driver model: move allocating/freeing memory to the driver core > by having only the needed memory size > specified in each driver > [2] Devres: we still have to allocate memory in each driver, but we > need not free it explicitly, > making our driver work much easier > > > [1] and [2] are completely differently approach, > but what they solve is the same: easier memory (resource) management > without leak.
Understood.
IIUC, this adds a new leak scenario beside the multiple inits one such as for USB, but this new scenarion which is in failure paths where already allocated memory must be released, seems to me no more critical than the one I'd discussed with Simon, i.e., I'm not convinced we need a fix, and I'm not convinced we need a general memory management feature for that -- which does not mean I can't be convinced, though; I just need (more) convincing arguments (than I can currently read).
BTW, I am not following the USB-discussion between Simon and you. I have not checked the USB changes recently, so I am not familiar enough with it to add my comment.
It was not actually a USB discussion. It was a discussion within v1 of this patch series. I'd asked when exactly there could be leaks in our current driver user scenarios and Simon said there was no leak case except in USB which could be bound several times in U-Boot between power-on and OS boot.
> The only problem I see in [1] is that it is not controllable at run-time. > The memory size for the auto-allocation must be specified at the compile time.
How in practice is that a problem?
At first, I thought (or expected) that .priv_auto_alloc_size and friends were generic enough to cover all the cases, but they were not.
I'm afraid I am still not seeing the problem.
> So, we need calloc() and free() in some low-level drivers. > Simon might say they are only a few "exceptions", > (my impression is I often hear the logic such as "it is not a problem > because we do not have many.") > Anyway, we had already lost the consistency as for memory allocation.
Not sure I understand that last sentence. Which consistency exactly have we lost?
When I write drivers for Linux, I usually allocate memory for driver data with devm_kzalloc().
But, in U-boot, sometimes we specify .priv_auto_alloc_size, and sometimes we call calloc() and free().
This is what I called lost-consistency.
Ok, now I get this point.
> I imagined if [2] had been supported earlier, we would not have needed [1]. > (at least, [2] seems more flexible to me.) > > We already have many DM-based drivers, and I think we can live with [1] > despite some exceptional drivers allocating memory on their own. > > So, if Simon (and other developers) does not feel comfortable with this series, > I do not mind discarding it.
Note that I'm not saying your patch series does not solve the issue of leaks. What I'm saying is i) do we need to solve this issue, and ii) if we do, is your series the best option ?
Sorry, if I am misunderstanding your questions.
If you are talking about the generic program guideline, I think leaks should be eliminated and this series should be helpful (but I wouldn't say as far as it is the best).
If you are asking if this series is the best for solving some particular issues, sorry, I can not comment on what I am not familiar with.
Let me recap :) as a series of simple yes/no sentences.
Sometimes in driver code there are memory leaks.
These leaks are bugs.
These leaks do not prevent U-Boot from functioning properly.
There are two methods to eliminate these leaks: Simon's method of allocating / freeing driver/device memory outside driver code per se, and your proposed method of dynamically tracking memory allocated by a driver / device, and freeing it when the driver gets 'unloaded' -- akin to freeing a process' resources when it terminates.
Each method has advantages and disadvantages.
My opinion for now is that:
We do not /need/ to fix the leaks, we /would like/ to.
since we don't /need/ to fix the leaks, we can afford to be picky about how we will fix them, or even whether we will fix them at all if no solution pleases us.
'being picky' means we should consider the pros and cons of available methods, not only wrt the fix itself, but more generally too: Does it fix other issues? Does it hinder or encourage best practices? how much source code does it bring in? etc.
Right now, I'm not even certain we have a problem at all, in the sense that I don't recall seeing reports of a target failing to boot because of memory exhaustion in U-Boot -- which means that, to me, the 'pros' of any leak fix solution would start thin since they would solve a non-problem, or more precisely, a not-really-a-problem, and I fail to see the other pros (OTOH, the cons are not /that/ many either).
But I might be mistaken, so I'm asking for actual scenarios where a target did have problems doing its job due to memory allocation issues, and scenarios for other pros as well, and comments from anyone who would have a good idea of the cons and pros.
It's a valuable point of view. My instinct is often to bring things in and expand the platform. But there needs to be a strong benefit.
As things stand I am also unsure of this. Driver model was designed to put memory allocation inside the core code (driver/core/device.c). Very few devices allocate memory at present () and perhaps even fewer need to. Here's a list of:
$ grep "alloc(" `git grep -l U_BOOT_DRIVER` arch/x86/cpu/ivybridge/bd82x6x.c: cpu = calloc(1, sizeof(*cpu)); drivers/gpio/mxc_gpio.c: plat = calloc(1, sizeof(*plat)); drivers/gpio/s5p_gpio.c: plat = calloc(1, sizeof(*plat)); drivers/gpio/sandbox.c: dev->priv = calloc(sizeof(struct gpio_state), uc_priv->gpio_count); drivers/gpio/sunxi_gpio.c: name = malloc(3); drivers/gpio/sunxi_gpio.c: plat = calloc(1, sizeof(*plat)); drivers/gpio/tegra_gpio.c: name = malloc(3); drivers/gpio/tegra_gpio.c: plat = calloc(1, sizeof(*plat)); drivers/gpio/vybrid_gpio.c: plat = calloc(1, sizeof(*plat)); drivers/i2c/i2c-uclass.c: * Use the stack for small messages, malloc() for larger ones. We drivers/i2c/i2c-uclass.c: buf = malloc(I2C_MAX_OFFSET_LEN + len); drivers/misc/cros_ec_sandbox.c: ec->flash_data = os_malloc(len); drivers/misc/cros_ec_sandbox.c: ec->matrix = calloc(ec->matrix_count, sizeof(*ec->matrix)); drivers/misc/cros_ec_sandbox.c: ec->flash_data = os_malloc(ec->flash_data_len); drivers/misc/i2c_eeprom_emul.c: priv->data = calloc(1, plat->size); drivers/mtd/spi/sf_probe.c: flash = calloc(1, sizeof(*flash)); drivers/net/designware.c: struct mii_dev *bus = mdio_alloc(); drivers/net/designware.c: dev = (struct eth_device *) malloc(sizeof(struct eth_device)); drivers/net/sunxi_emac.c: priv->bus = mdio_alloc(); drivers/spi/sandbox_spi.c: tx = malloc(bytes); drivers/spi/sandbox_spi.c: rx = malloc(bytes); test/dm/test-driver.c: dev->priv = calloc(1, sizeof(struct dm_test_priv));
Most of those are temporary allocations are unnecessary but some could use devres.
The v2 series solves the SPL size issue, except that with Han's USB changes we have to device DM_REMOVE when we use USB. I am seriously reconsidering that as a limitation that might be best avoided.
But we now require devres as a core feature - this pushes up the overhead of driver rmodel. I really like this patch:
http://patchwork.ozlabs.org/patch/494216/
but I don't think it goes far enough.
I'd like to figure this out before moving to v3. Here is my proposal:
- Add CONFIG_DEVRES as an option which can be enabled if wanted, but
is not required for things to work.
This is a boot-loader, so it is a quite rare scenario to attach and detach drivers over and over again. Memory exhaustion would never happen (at least in normal usage) even if memory is never freed.
Things always work well, in other words, there is no use-case where precise resource management is our requirement.
So, CONFIG_DEVRES can be optional.
I agree - the main thing I can think of is USB but even then it would mostly be a human fiddling with it. A normal device boot sequence would not rescan the bus multiple times.
Memory may leak if CONFIG_DEVRES is disabled, but that is OK. That is not what we care about very much.
This is what I understood so far.
If so, CONFIG_DEVRES and CONFIG_DM_REMOVE eventually have similar concept: We prepared these features just in case, but we actually do not need them, so they are the first things we want to disable in memory-limited cases.
We had already disabled CONFIG_DM_REMOVE where we really want to save memory footprint, such as SPL. I do not think we will get further benefit by adding CONFIG_DEVRES here.
Also, another similar concept we have is, malloc_simple, where free() does nothing, we cannot retrieve freed memory.
I think too many choices will result in making things messy.
We have found a case where we want CONFIG_DM_REMOVE in normal operation - shutting down USB before booting the OS.
Extra options introduce extra cases, but they are all different:
- malloc_simple - reduced code size for supporting malloc()
- DM_REMOVE - drops all remove/unbind code (actually it doesn't drop
it in drivers, which could be fixed)
- DEVRES - supports manual memory allocation in drivers with automatic free
So I don't see any conflict here. Also I like the memory allocation improvements you have added.
- Drop the use of devres to remove()/unbind() devices. The core DM
code can stick with its existing manual free() calls.
devres_head should only exist in struct device when CONFIG_DEVRES is set.
Convert over a driver to use devres in sandbox and enable it. One
option would be cros_ec_sandbox. It reads the device tree key map and it is variable size so we can't use the automatic memory allocation. Need to add a remove() method!
- See how much use devres gets over the next year. We haven't lost
any efficiency and we gain a useful feature to track device memory allocation.
Masahiro, Albert what do you think?
Regards, Simon
From our discussion so far, my frank feeling is, this series is unwelcome.
Rather than pulling in what we are unsure about the necessity, won't it be better to defer it to see if it is really useful or not? (i.e. how many drivers want to do malloc on their own.)
Anyway, we can dig it in Patchwork anytime we find it necessary.
If we apply it then we can tell people to always use it when allocating memory manually in a driver. We can go through and make sure all existing cases use it.
Then we have options in the future when we need to enable it. If we don't apply it, we build up code that would need to be refactored later in this case.
Also devres is a concept understood in the kernel, and if we can bring it in without adding mandatory overhead, that seems like a win to me.
My preference is to apply it, but as an option. So long as sandbox uses it, this will ensure adequate test coverage. BTW we already have memory leak tests but I'm not sure if they will be enough to test devres.
OK, I will try v3, making it optional with CONFIG_DEVRES.
participants (4)
-
Albert ARIBAUD -
Lukasz Majewski -
Masahiro Yamada -
Simon Glass