Re: [PATCH 1/3] backlight: as3711_bl: fix device-tree node lookup

On Wed, Nov 15, 2017 at 02:32:11PM +0000, Lee Jones wrote:

On Wed, 15 Nov 2017, Johan Hovold wrote:

quoted

On Tue, Nov 14, 2017 at 07:48:09PM +0000, Daniel Thompson wrote:

quoted

On 14/11/17 18:05, Johan Hovold wrote:

quoted

On Mon, Nov 13, 2017 at 02:16:09PM +0000, Daniel Thompson wrote:

quoted

On 13/11/17 10:20, Johan Hovold wrote:

quoted

Fix child-node lookup during probe, which ended up searching the whole
device tree depth-first starting at the parent rather than just matching
on its children.

To make things worse, the parent mfd node was also prematurely freed.

Note that the nodes returned from the two calls to of_parse_phandle()
are also leaking, but fixing that is a bit more involved as pointers to
node fields are being stored for later use.

Is using a devm_kstrdup() to remember the full_name sufficient so get
each of the FIXMEs cleaned up as well?

Yeah, that may be sufficient, but looking closer at this now, it seems
the name pointers (su1_fb and su2_fb) are only used as booleans, and the
fb_name pointer in struct as3711_bl_data is never used at all.

So cleaning that up somehow (e.g. and maybe even dropping non-dt
probing) would also work.

But since this is a separate, and less critical issue, I think it needs
to be done as a follow up to this one.

To be honest it was adding the separate and less critical FIXMEs into 
the patches that attracted my attention in the first place. ;-)

Heh. Since I was touching those error paths, I at least wanted to record
somehow there were further issues to be addressed. But feel free to drop
the FIXMEs if you prefer.

In my experience FIXME's tend not to get addressed:

$ git grep -i fixme | wc -l
4431

Submit patches instead. :)

There may be some truth to that, but I still think it's better to mark
what is broken (especially since a leaked node is no big deal in this
case) than to just ignore and forget about it.

Johan