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


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