Devres (Managed Device Resource) for U-Boot

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 <[email protected]>

Reviewed-by Bin Meng <[email protected]>

Acked-by Simon Glass <[email protected]>

---

Changes in v2: None

 arch/x86/include/asm/interrupt.h | 4 ----
 1 file changed, 4 deletions(-)

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 <[email protected]>

Reviewed-by Heiko Schocher <[email protected]>

Acked-by Simon Glass <[email protected]>

---

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(-)

(quoted diff) +0 -0

b/drivers/usb/musb-new/musb_gadget_ep0.c index 5a71501..415a9f2 100644

drivers/usb/musb-new/musb_gadget_ep0.c +1 -0

@@ -43,6 +43,7 @@
 #else
 #include <common.h>
 #include "linux-compat.h"
+#include <asm/processor.h>
 #endif
 
 #include "musb_core.h"

include/linux/compat.h +0 -0

index 6ff3915..da1420f 100644

Hi Masahiro,

Quoted 26 lines

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 <[email protected]>

Reviewed-by Heiko Schocher <[email protected]>

Acked-by Simon Glass <[email protected]>

---

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(-)

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 <[email protected]>

Reviewed-by Heiko Schocher <[email protected]>

Acked-by Simon Glass <[email protected]>

---

Changes in v2: None

 include/linux/compat.h | 6 ++++--
 lib/linux_compat.c     | 5 -----
 2 files changed, 4 insertions(+), 7 deletions(-)

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 <[email protected]>

Reviewed-by Heiko Schocher <[email protected]>

Acked-by Simon Glass <[email protected]>

---

Changes in v2: None

 include/linux/compat.h | 20 ++++++++++++--------
 lib/linux_compat.c     | 13 ++++++-------
 2 files changed, 18 insertions(+), 15 deletions(-)

include/linux/compat.h +6 -0

index a3d136b..fbebf91 100644
@@ -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);

include/linux/compat.h +5 -1

-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(...)

include/linux/compat.h +0 -7

-/* 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 {};

lib/linux_compat.c +1 -5

index 8c7a7b5..a936a7e 100644
@@ -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);

Hi Masahiro,

Quoted 20 lines

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 <[email protected]>

Reviewed-by Heiko Schocher <[email protected]>

Acked-by Simon Glass <[email protected]>

---

Changes in v2: None

 include/linux/compat.h | 20 ++++++++++++--------
 lib/linux_compat.c     | 13 ++++++-------
 2 files changed, 18 insertions(+), 15 deletions(-)

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 <[email protected]>

---

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

drivers/core/Kconfig +10 -0

index 2861b43..5966801 100644
@@ -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.

drivers/core/Makefile +1 -1

index a3fec38..cd8c104 100644
@@ -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

drivers/core/device-remove.c +5 -0

index 20b56f9..e1714b2 100644
@@ -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)

drivers/core/device.c +3 -0

index 39cc2f3..83b47d8 100644
@@ -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;

drivers/core/devres.c new file +187 -0

new file mode 100644
index 0000000..e7330b3
@@ -0,0 +1,187 @@
+/*
+ * Copyright (C) 2015 Masahiro Yamada <[email protected]>
+ *
+ * Based on the original work in Linux by
+ * Copyright (c) 2006  SUSE Linux Products GmbH
+ * Copyright (c) 2006  Tejun Heo <[email protected]>
+ *
+ * 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);
+}

include/dm/device-internal.h +19 -0

index 687462b..409c687 100644
@@ -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

include/dm/device.h +140 -1

index 3674d19..c266c7d 100644
@@ -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

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 <[email protected]>

---

Changes in v2: None

 include/linux/compat.h | 1 +
 lib/linux_compat.c     | 5 ++++-
 2 files changed, 5 insertions(+), 1 deletion(-)

include/linux/compat.h +1 -0

index fbebf91..13386c9 100644
@@ -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);

lib/linux_compat.c +0 -0

index a936a7e..6da0cfa 100644
@@ -18,7 +18,10 @@ void *kmalloc(size_t size, int flags)
 {

Hi Masahiro,

Quoted 34 lines

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 <[email protected]>

---

Changes in v2: None

 include/linux/compat.h | 1 +
 lib/linux_compat.c     | 5 ++++-
 2 files changed, 5 insertions(+), 1 deletion(-)

include/linux/compat.h +1 -0

index fbebf91..13386c9 100644
@@ -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);

lib/linux_compat.c +0 -0

index a936a7e..6da0cfa 100644
@@ -18,7 +18,10 @@ void *kmalloc(size_t size, int flags)
 {

lib/linux_compat.c void *kmalloc(size_t size, int flags) · line 27

Quoted 10 lines

lib/linux_compat.c +4 -1

      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);
 

Reviewed-by Lukasz Majewski <[email protected]>

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 you see in driver/Makefile, Kbuild descends into the driver/core/ directory only when CONFIG_DM is enabled.

Signed-off-by Masahiro Yamada <[email protected]>

---

Changes in v2:
- Newly added

 drivers/core/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/core/Makefile +1 -2

index cd8c104..5f019e8 100644
@@ -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

Quoted 4 lines · Masahiro Yamada

As you see in driver/Makefile, Kbuild descends into the driver/core/ directory only when CONFIG_DM is enabled.

Quoted 8 lines

Signed-off-by Masahiro Yamada <[email protected]>

---

Changes in v2:
- Newly added

 drivers/core/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Acked-by Simon Glass <[email protected]>

Quoted 9 lines

drivers/core/Makefile +0 -0

index cd8c104..5f019e8 100644
@@ -4,7 +4,7 @@
 # SPDX-License-Identifier:     GPL-2.0+
 #
 

Quoted 8 lines

drivers/core/Makefile +1 -2

-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

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

Hello Masahiro,

Quoted 3 lines · Masahiro Yamada

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.

Quoted 7 lines

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

Quoted 8 lines

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/[email protected]/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.

Hi Albert,

2015-07-13 15:51 GMT+09:00 Albert ARIBAUD <[email protected]>:

Quoted 38 lines

Hello Masahiro,

Quoted 3 lines · Masahiro Yamada

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.

Quoted 7 lines

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

Quoted 8 lines

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/[email protected]/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.

Quoted 4 lines

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.

Signature · 2 lines

Best Regards
Masahiro Yamada

Hello Masahiro,

Quoted 63 lines · Masahiro Yamada

Hi Albert,

2015-07-13 15:51 GMT+09:00 Albert ARIBAUD <[email protected]>:

Quoted 38 lines

Hello Masahiro,

Quoted 3 lines · Masahiro Yamada

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.

Quoted 7 lines

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

Quoted 8 lines

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/[email protected]/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.

Quoted 4 lines

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

Quoted 3 lines

[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).

Quoted 2 lines

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?

Quoted 5 lines

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?

Quoted 6 lines

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.

Quoted 2 lines

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 ?

2015-07-13 19:55 GMT+09:00 Albert ARIBAUD <[email protected]>:

Quoted 73 lines

Hello Masahiro,

Quoted 63 lines · Masahiro Yamada

Hi Albert,

2015-07-13 15:51 GMT+09:00 Albert ARIBAUD <[email protected]>:

Quoted 38 lines

Hello Masahiro,

Quoted 3 lines · Masahiro Yamada

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.

Quoted 7 lines

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

Quoted 8 lines

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/[email protected]/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.

Quoted 4 lines

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

Quoted 3 lines

[1] and [2] are completely differently approach,
but what they solve is the same: easier memory (resource) management without leak.

Understood.

Quoted 7 lines

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.

Quoted 4 lines

Quoted 2 lines

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.

Quoted 8 lines

Quoted 5 lines

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.

Quoted 6 lines

Quoted 6 lines

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.

Quoted 6 lines

Quoted 2 lines

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.

Signature · 2 lines

Best Regards
Masahiro Yamada

Hello Masahiro,

Quoted 82 lines · Masahiro Yamada

2015-07-13 19:55 GMT+09:00 Albert ARIBAUD <[email protected]>:

Quoted 73 lines

Hello Masahiro,

Quoted 63 lines · Masahiro Yamada

Hi Albert,

2015-07-13 15:51 GMT+09:00 Albert ARIBAUD <[email protected]>:

Quoted 38 lines

Hello Masahiro,

Quoted 3 lines · Masahiro Yamada

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.

Quoted 7 lines

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

Quoted 8 lines

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/[email protected]/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.

Quoted 4 lines

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

Quoted 3 lines

[1] and [2] are completely differently approach,
but what they solve is the same: easier memory (resource) management without leak.

Understood.

Quoted 7 lines

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).

Quoted 4 lines

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.

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

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.

Quoted 9 lines

Quoted 8 lines

Quoted 5 lines

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?

Quoted 7 lines

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.

Quoted 13 lines

Quoted 6 lines

Quoted 6 lines

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.

Quoted 6 lines

Quoted 2 lines

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 ?

Quoted 8 lines

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.

+Hans

Hi,

Quoted 183 lines · Albert ARIBAUD

Hello Masahiro,

Quoted 82 lines · Masahiro Yamada

2015-07-13 19:55 GMT+09:00 Albert ARIBAUD <[email protected]>:

Quoted 73 lines

Hello Masahiro,

Quoted 63 lines · Masahiro Yamada

Hi Albert,

2015-07-13 15:51 GMT+09:00 Albert ARIBAUD <[email protected]>:

Quoted 38 lines

Hello Masahiro,

Quoted 3 lines · Masahiro Yamada

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.

Quoted 7 lines

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

Quoted 8 lines

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/[email protected]/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.

Quoted 4 lines

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

Quoted 3 lines

[1] and [2] are completely differently approach,
but what they solve is the same: easier memory (resource) management without leak.

Understood.

Quoted 7 lines

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).

Quoted 4 lines

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.

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

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.

Quoted 9 lines

Quoted 8 lines

Quoted 5 lines

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?

Quoted 7 lines

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.

Quoted 13 lines

Quoted 6 lines

Quoted 6 lines

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.

Quoted 6 lines

Quoted 2 lines

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 ?

Quoted 8 lines

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).

Quoted 4 lines

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 <[email protected]>:

Quoted 239 lines

+Hans

Hi,

Quoted 183 lines · Albert ARIBAUD

Hello Masahiro,

Quoted 82 lines · Masahiro Yamada

2015-07-13 19:55 GMT+09:00 Albert ARIBAUD <[email protected]>:

Quoted 73 lines

Hello Masahiro,

Quoted 63 lines · Masahiro Yamada

Hi Albert,

2015-07-13 15:51 GMT+09:00 Albert ARIBAUD <[email protected]>:

Quoted 38 lines

Hello Masahiro,

Quoted 3 lines · Masahiro Yamada

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.

Quoted 7 lines

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

Quoted 8 lines

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/[email protected]/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.

Quoted 4 lines

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

Quoted 3 lines

[1] and [2] are completely differently approach,
but what they solve is the same: easier memory (resource) management without leak.

Understood.

Quoted 7 lines

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).

Quoted 4 lines

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.

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

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.

Quoted 9 lines

Quoted 8 lines

Quoted 5 lines

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?

Quoted 7 lines

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.

Quoted 13 lines

Quoted 6 lines

Quoted 6 lines

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.

Quoted 6 lines

Quoted 2 lines

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 ?

Quoted 8 lines

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).

Quoted 4 lines

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:

Quoted 8 lines

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.

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.

Quoted 12 lines

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!

Quoted 8 lines

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

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,

Quoted 259 lines · Masahiro Yamada

Hi Simon, Albert.

2015-07-18 23:37 GMT+09:00 Simon Glass <[email protected]>:

Quoted 239 lines

+Hans

Hi,

Quoted 183 lines · Albert ARIBAUD

Hello Masahiro,

Quoted 82 lines · Masahiro Yamada

2015-07-13 19:55 GMT+09:00 Albert ARIBAUD <[email protected]>:

Quoted 73 lines

Hello Masahiro,

Quoted 63 lines · Masahiro Yamada

Hi Albert,

2015-07-13 15:51 GMT+09:00 Albert ARIBAUD <[email protected]>:

Quoted 38 lines

Hello Masahiro,

Quoted 3 lines · Masahiro Yamada

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.

Quoted 7 lines

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

Quoted 8 lines

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/[email protected]/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.

Quoted 4 lines

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

Quoted 3 lines

[1] and [2] are completely differently approach,
but what they solve is the same: easier memory (resource) management without leak.

Understood.

Quoted 7 lines

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).

Quoted 4 lines

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.

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

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.

Quoted 9 lines

Quoted 8 lines

Quoted 5 lines

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?

Quoted 7 lines

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.

Quoted 13 lines

Quoted 6 lines

Quoted 6 lines

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.

Quoted 6 lines

Quoted 2 lines

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 ?

Quoted 8 lines

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).

Quoted 4 lines

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:

Quoted 8 lines

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.

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.

Quoted 4 lines

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.

Quoted 15 lines

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.

Quoted 5 lines

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.

Quoted 22 lines

Quoted 12 lines

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!

Quoted 8 lines

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

Quoted 7 lines

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.