[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 at socionext.com> wrote:
> 2015-07-13 19:55 GMT+09:00 Albert ARIBAUD <albert.u.boot at aribaud.net>:
> > Hello Masahiro,
> >
> > On Mon, 13 Jul 2015 16:39:45 +0900, Masahiro Yamada
> > <yamada.masahiro at socionext.com> wrote:
> >> Hi Albert,
> >>
> >>
> >> 2015-07-13 15:51 GMT+09:00 Albert ARIBAUD <albert.u.boot at aribaud.net>:
> >> > Hello Masahiro,
> >> >
> >> > On Mon, 13 Jul 2015 13:17:03 +0900, Masahiro Yamada
> >> > <yamada.masahiro at 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.