Devres (Managed Device Resource) for U-Boot

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

Quoted 73 lines

Hello Masahiro,

Quoted 63 lines · Masahiro Yamada

Hi Albert,

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

Quoted 38 lines

Hello Masahiro,

Quoted 3 lines · Masahiro Yamada

Please refer to the commit message of 06/14
for what this series wants to do.

Remark: you could use "Series-notes:" in 6/14 to have patman directly include said notes here instead of referring the reader to the patch.

Quoted 7 lines

Masahiro Yamada (14):
x86: delete unneeded declarations of disable_irq() and enable_irq()
linux_compat: remove cpu_relax() define
linux_compat: move vzalloc() to header file as an inline function
linux_compat: handle __GFP_ZERO in kmalloc()
dm: add DM_FLAG_BOUND flag
devres: introduce Devres (Managed Device Resource) framework

Quoted 8 lines

devres: add devm_kmalloc() and friends (managed memory allocators)
dm: refactor device_bind() and device_unbind() with devm_kzalloc()
dm: merge fail_alloc labels
linux_compat: introduce GFP_DMA flag for kmalloc()
dm: refactor device_probe() and device_remove() with devm_kzalloc()
devres: add debug command to dump device resources
dm: remove redundant CONFIG_DM from driver/core/Makefile
devres: compile Devres iif CONFIG_DM_DEVICE_REMOVE is enabled

I am still unsure why this is necessary. I had a discussion on the list with Simon, see last message here:

<https://www.mail-archive.com/[email protected]/msg177031.html>

Unless I'm mistaken, the only case where we actually have a leak that this series would fix is in some cases of binding USB devices / drivers multiple times, and even then, it would take a considerable amount of repeated bindings before U-Boot could become unable to boot a payload; a scenario that I find unlikely.

Quoted 4 lines

I do understand the general goal of fixing bugs when we ind them; but I question the global benefit of fixing this specific leak bug by adding a whole new feature with a lot of code to U-Boot, as opposed to fixing it in a more ad hoc way with much less less code volume and complexity.

You have a point.

What we really want to avoid is to make low-level drivers too complicated by leaving the correct memory management to each of them.

After all, there turned out to be two options to solve it.

[1] Simon's driver model: move allocating/freeing memory to the driver core by having only the needed memory size
specified in each driver
[2] Devres: we still have to allocate memory in each driver, but we
need not free it explicitly,
making our driver work much easier

Quoted 3 lines

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

Understood.

Quoted 7 lines

IIUC, this adds a new leak scenario beside the multiple inits one such as for USB, but this new scenarion which is in failure paths where already allocated memory must be released, seems to me no more critical than the one I'd discussed with Simon, i.e., I'm not convinced we need a fix, and I'm not convinced we need a general memory management feature for that -- which does not mean I can't be convinced, though; I just need (more) convincing arguments (than I can currently read).

BTW, I am not following the USB-discussion between Simon and you. I have not checked the USB changes recently,
so I am not familiar enough with it to add my comment.

Quoted 4 lines

Quoted 2 lines

The only problem I see in [1] is that it is not controllable at run-time. The memory size for the auto-allocation must be specified at the compile time.

How in practice is that a problem?

At first, I thought (or expected) that .priv_auto_alloc_size and friends were generic enough to cover all the cases, but they were not.

Quoted 8 lines

Quoted 5 lines

So, we need calloc() and free() in some low-level drivers. Simon might say they are only a few "exceptions",
(my impression is I often hear the logic such as "it is not a problem because we do not have many.")
Anyway, we had already lost the consistency as for memory allocation.

Not sure I understand that last sentence. Which consistency exactly have we lost?

When I write drivers for Linux, I usually allocate memory for driver data with devm_kzalloc().

But, in U-boot, sometimes we specify .priv_auto_alloc_size, and sometimes we call calloc() and free().

This is what I called lost-consistency.

Quoted 6 lines

Quoted 6 lines

I imagined if [2] had been supported earlier, we would not have needed [1]. (at least, [2] seems more flexible to me.)

We already have many DM-based drivers, and I think we can live with [1] despite some exceptional drivers allocating memory on their own.

Quoted 6 lines

Quoted 2 lines

So, if Simon (and other developers) does not feel comfortable with this series, I do not mind discarding it.

Note that I'm not saying your patch series does not solve the issue of leaks. What I'm saying is i) do we need to solve this issue, and ii) if we do, is your series the best option ?

Sorry, if I am misunderstanding your questions.

If you are talking about the generic program guideline, I think leaks should be eliminated and this series should be helpful (but I wouldn't say as far as it is the best).

If you are asking if this series is the best for solving some particular issues, sorry, I can not comment on what I am not familiar with.

Signature · 2 lines

Best Regards
Masahiro Yamada