2015-07-13 19:55 GMT+09:00 Albert ARIBAUD <albert.u.b...@aribaud.net>: > Hello Masahiro, > > On Mon, 13 Jul 2015 16:39:45 +0900, Masahiro Yamada > <yamada.masah...@socionext.com> wrote: >> Hi Albert, >> >> >> 2015-07-13 15:51 GMT+09:00 Albert ARIBAUD <albert.u.b...@aribaud.net>: >> > Hello Masahiro, >> > >> > On Mon, 13 Jul 2015 13:17:03 +0900, Masahiro Yamada >> > <yamada.masah...@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 mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
