[U-Boot] [PATCH v2 00/14] Devres (Managed Device Resource) for U-Boot

[U-Boot] [PATCH v2 00/14] Devres (Managed Device Resource) for U-Boot

[Masahiro Yamada]

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

-- 
1.9.1

[U-Boot] [PATCH v2 01/14] x86: delete unneeded declarations of disable_irq() and enable_irq()

[Masahiro Yamada]

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

[U-Boot] [PATCH v2 01/14] x86: delete unneeded declarations of disable_irq() and enable_irq()

[Simon Glass]

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!

[U-Boot] [PATCH v2 02/14] linux_compat: remove cpu_relax() define

[Masahiro Yamada]

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

[U-Boot] [PATCH v2 02/14] linux_compat: remove cpu_relax() define

[Lukasz Majewski]

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>

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] [PATCH v2 02/14] linux_compat: remove cpu_relax() define

[Simon Glass]

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!

[U-Boot] [PATCH v2 03/14] linux_compat: move vzalloc() to header file as an inline function

[Masahiro Yamada]

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

[U-Boot] [PATCH v2 03/14] linux_compat: move vzalloc() to header file as an inline function

[Simon Glass]

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!

[U-Boot] [PATCH v2 04/14] linux_compat: handle __GFP_ZERO in kmalloc()

[Masahiro Yamada]

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

[U-Boot] [PATCH v2 04/14] linux_compat: handle __GFP_ZERO in kmalloc()

[Lukasz Majewski]

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>

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] [PATCH v2 04/14] linux_compat: handle __GFP_ZERO in kmalloc()

[Simon Glass]

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!

[U-Boot] [PATCH v2 05/14] dm: add DM_FLAG_BOUND flag

[Masahiro Yamada]

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
  *
-- 
1.9.1

[U-Boot] [PATCH v2 06/14] devres: introduce Devres (Managed Device Resource) framework

[Masahiro Yamada]

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
-- 
1.9.1

[U-Boot] [PATCH v2 06/14] devres: introduce Devres (Managed Device Resource) framework

[Simon Glass]

+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 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
> --
> 1.9.1
>

[U-Boot] [PATCH v2 07/14] devres: add devm_kmalloc() and friends (managed memory allocators)

[Masahiro Yamada]

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
-- 
1.9.1

[U-Boot] [PATCH v2 08/14] dm: refactor device_bind() and device_unbind() with devm_kzalloc()

[Masahiro Yamada]

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

[U-Boot] [PATCH v2 09/14] dm: merge fail_alloc labels

[Masahiro Yamada]

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

[U-Boot] [PATCH v2 10/14] linux_compat: introduce GFP_DMA flag for kmalloc()

[Masahiro Yamada]

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

[U-Boot] [PATCH v2 10/14] linux_compat: introduce GFP_DMA flag for kmalloc()

[Lukasz Majewski]

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

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[U-Boot] [PATCH v2 10/14] linux_compat: introduce GFP_DMA flag for kmalloc()

[Masahiro Yamada]

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);
>> +     else
>> +             p = 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.




-- 
Best Regards
Masahiro Yamada

[U-Boot] [PATCH v2 11/14] dm: refactor device_probe() and device_remove() with devm_kzalloc()

[Masahiro Yamada]

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

[U-Boot] [PATCH v2 12/14] devres: add debug command to dump device resources

[Masahiro Yamada]

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 at 54006800
    0xbfb541e8 (8 byte) devm_kmalloc_release  BIND
    0xbfb54440 (4 byte) devm_kmalloc_release  PROBE
    0xbfb54460 (4 byte) devm_kmalloc_release  PROBE
- serial at 54006900
    0xbfb54270 (8 byte) devm_kmalloc_release  BIND
- gpio at 55000000
- i2c at 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
  */
-- 
1.9.1

[U-Boot] [PATCH v2 13/14] dm: remove redundant CONFIG_DM from driver/core/Makefile

[Masahiro Yamada]

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
-- 
1.9.1

[U-Boot] [PATCH v2 13/14] dm: remove redundant CONFIG_DM from driver/core/Makefile

[Simon Glass]

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
>

[U-Boot] [PATCH v2 13/14] dm: remove redundant CONFIG_DM from driver/core/Makefile

[Simon Glass]

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!

[U-Boot] [PATCH v2 14/14] devres: compile Devres iif CONFIG_DM_DEVICE_REMOVE is enabled

[Masahiro Yamada]

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
-- 
1.9.1

[U-Boot] [PATCH v2 00/14] Devres (Managed Device Resource) for U-Boot

[Albert ARIBAUD]

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,
-- 
Albert.

[U-Boot] [PATCH v2 00/14] Devres (Managed Device Resource) for U-Boot

[Masahiro Yamada]

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.


-- 
Best Regards
Masahiro Yamada

[U-Boot] [PATCH v2 00/14] Devres (Managed Device Resource) for U-Boot

[Albert ARIBAUD]

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,
-- 
Albert.

[U-Boot] [PATCH v2 00/14] Devres (Managed Device Resource) for U-Boot

[Masahiro Yamada]

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.



-- 
Best Regards
Masahiro Yamada

[U-Boot] [PATCH v2 00/14] Devres (Managed Device Resource) for U-Boot

[Albert ARIBAUD]

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,
-- 
Albert.

[U-Boot] [PATCH v2 00/14] Devres (Managed Device Resource) for U-Boot

[Simon Glass]

+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

[U-Boot] [PATCH v2 00/14] Devres (Managed Device Resource) for U-Boot

[Masahiro Yamada]

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



> 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


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.



-- 
Best Regards
Masahiro Yamada

[U-Boot] [PATCH v2 00/14] Devres (Managed Device Resource) for U-Boot

[Simon Glass]

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

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.

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

[U-Boot] [PATCH v2 00/14] Devres (Managed Device Resource) for U-Boot

[Masahiro Yamada]

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



-- 
Best Regards
Masahiro Yamada