On Wed, Aug 28, 2024 at 04:45:32PM +0200, Krzysztof Kozlowski wrote:

On 28/08/2024 16:32, Andrew Lunn wrote:

quoted

On Wed, Aug 28, 2024 at 11:23:30AM +0800, Jinjie Ruan wrote:

quoted

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