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

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

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