| Message ID | 20240828032343.1218749-1-ruanjinjie@huawei.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | net: Simplified with scoped function | expand |
On Wed, Aug 28, 2024 at 11:23:30AM +0800, Jinjie Ruan wrote: > Simplify with scoped for each OF child loop and __free(), as well as > dev_err_probe(). > > Changes in v2: > - Subject prefix: next -> net-next. > - Split __free() from scoped for each OF child loop clean. > - Fix use of_node_put() instead of __free() for the 5th patch. I personally think all these __free() are ugly and magical. Can it somehow be made part of of_get_child_by_name()? Add an of_get_child_by_name_func_ref() which holds a reference to the node for the scope of the function? for_each_available_child_of_node_scoped() is fine. Once you have fixed all the reverse christmas tree, please submit them. But i would like to see alternatives to __free(), once which are less ugly. Andrew --- pw-bot: cr
On 28/08/2024 16:32, Andrew Lunn wrote: > On Wed, Aug 28, 2024 at 11:23:30AM +0800, Jinjie Ruan wrote: >> Simplify with scoped for each OF child loop and __free(), as well as >> dev_err_probe(). >> >> Changes in v2: >> - Subject prefix: next -> net-next. >> - Split __free() from scoped for each OF child loop clean. >> - Fix use of_node_put() instead of __free() for the 5th patch. > > I personally think all these __free() are ugly and magical. Can it It is code readability so quite subjective. Jakub also rejected one of such __free() changes, so maybe all these cases here should be rejected? > somehow be made part of of_get_child_by_name()? Add an > of_get_child_by_name_func_ref() which holds a reference to the node > for the scope of the function? That's interesting, scoped-wrapper. I am afraid we would need quite a lot of them, though, for every of_get_xxx call. Best regards, Krzysztof
On Wed, Aug 28, 2024 at 04:45:32PM +0200, Krzysztof Kozlowski wrote: > On 28/08/2024 16:32, Andrew Lunn wrote: > > On Wed, Aug 28, 2024 at 11:23:30AM +0800, Jinjie Ruan wrote: > >> Simplify with scoped for each OF child loop and __free(), as well as > >> dev_err_probe(). > >> > >> Changes in v2: > >> - Subject prefix: next -> net-next. > >> - Split __free() from scoped for each OF child loop clean. > >> - Fix use of_node_put() instead of __free() for the 5th patch. > > > > I personally think all these __free() are ugly and magical. Can it > > It is code readability so quite subjective. Try. But the __ is also a red flag. Anything starting with _ or __ in general should not be used in common code. That prefix is supposed to indicate it is internal plumbing which should be hidden away, out of sight, not to be used directly. Yet here it is, being scattered everywhere. I also wounder if this is lipstick on a pig. I suspect the reference counting on DT object is broken everywhere, because it is almost never used. In general, DT blobs exist from boot to shutdown. They don't go away, so these reference counts are never used. DT overlays do exist, but account for what, 1% of DT objects? And how often does an overlay actually get unloaded? Has anybody written a fuzzer to try unloading parts of DT blobs? I suspect we would quickly drown in bug reports. Adding missing of_node_put() seems to be high on the list of bot driven patches, which cause a lot of maintainer effort for no real gain. And those submitting the patches probably have little understanding of what they are doing, other than making the bot happy. Do we really want to be adding ugly code, probably with a few additional bugs thrown in, just to make a bot happy, but probably no real benefit? Andrew
On Wed, 28 Aug 2024 16:45:32 +0200 Krzysztof Kozlowski wrote: > On 28/08/2024 16:32, Andrew Lunn wrote: > > On Wed, Aug 28, 2024 at 11:23:30AM +0800, Jinjie Ruan wrote: > >> Simplify with scoped for each OF child loop and __free(), as well as > >> dev_err_probe(). > >> > >> Changes in v2: > >> - Subject prefix: next -> net-next. > >> - Split __free() from scoped for each OF child loop clean. > >> - Fix use of_node_put() instead of __free() for the 5th patch. > > > > I personally think all these __free() are ugly and magical. Can it > > It is code readability so quite subjective. Jakub also rejected one of > such __free() changes, so maybe all these cases here should be rejected? Andrew's comments on refcounting on DT objects notwithstanding -- I do like the _scoped() iterator quite a bit, FWIW. I think it's one of the better uses of the auto-cleanup and local variable declarations. Direct uses of __free() are more questionable in my opinion. Using advanced constructs to build clean subsystem APIs is great, sprinkling unreadable constructs to save LoC is what C++ is for. (Lets see how many people this offends ;))
On 2024/8/28 22:32, Andrew Lunn wrote: > On Wed, Aug 28, 2024 at 11:23:30AM +0800, Jinjie Ruan wrote: >> Simplify with scoped for each OF child loop and __free(), as well as >> dev_err_probe(). >> >> Changes in v2: >> - Subject prefix: next -> net-next. >> - Split __free() from scoped for each OF child loop clean. >> - Fix use of_node_put() instead of __free() for the 5th patch. > > I personally think all these __free() are ugly and magical. Can it > somehow be made part of of_get_child_by_name()? Add an > of_get_child_by_name_func_ref() which holds a reference to the node > for the scope of the function? Yes, that is a good idea, and the __free() doesn't look as readable as or_each_*child_of_node_scoped(). > > for_each_available_child_of_node_scoped() is fine. Once you have fixed > all the reverse christmas tree, please submit them. But i would like > to see alternatives to __free(), once which are less ugly. Thank you! > > Andrew > > --- > pw-bot: cr >