Hi Boris,
Boris Brezillon boris.brezillon@bootlin.com wrote on Mon, 10 Sep 2018 15:25:51 +0200:
On Mon, 10 Sep 2018 15:14:23 +0200 Miquel Raynal miquel.raynal@bootlin.com wrote:
Hi Boris,
Boris Brezillon boris.brezillon@bootlin.com wrote on Mon, 10 Sep 2018 14:53:12 +0200:
Hi Miquel,
On Fri, 7 Sep 2018 16:38:24 +0200 Miquel Raynal miquel.raynal@bootlin.com wrote:
Hello,
I forgot to add Rafal which I know worked a lot on the parsers.
Miquel Raynal miquel.raynal@bootlin.com wrote on Fri, 7 Sep 2018 16:35:54 +0200:
While at first mtd_part_of_parse() would just call of_get_chil_by_name(), it has been edited to first try to get the OF node thanks to mtd_get_of_node() and fallback on of_get_child_by_name().
A of_node_put() was a bit below in the code, to balance the of_get_child_by_name(). However, despite its name, mtd_get_of_node() does not take a reference on the OF node.
That's probably something we should patch at some point, but that implies patching all mtd_get_of_node() users at the same time, so let's keep that for later.
BTW, if mtd_get_of_node() was actually retaining a reference, you would miss an of_node_put() in the !mtd_is_partition(master) case.
I think there is a misunderstanding here: mtd_get_of_node() is not retaining a reference, and I do not think it should! It is by design a helper to shortcut from the MTD device to the related FW node. Maybe calling it differently than "get" would be definitely less prone to errors. Maybe mtd_to_of_node() would be better?
Yes, the name is misleading for sure. But consistency is good, and (almost?) all DT helpers that return a device_node retain a reference to this node before returning it, so I think it would be a good thing to do the same in the MTD framework.
Fine by me, I'll patch all the places where it's used, but you can still take this patch as a fix for now.
Also, I'm not a big fan of the mtd_to_of_node() for this kind of function. It seems to imply that the mtd device is inheriting from device_node, which is not really the case, it's just an association relationship.
Ok
It is a simple helper hiding some pointer logic to retrieve the OF node related to an MTD device. People often used it this way:
of_node_put(mtd_get_of_node(<mtd>)).I don't get your point. Are you saying other places in the code are doing the wrong thing? Should we fix them too?
No, other places are doing the right thing.
Hm, okay. Then your example is not well chosen, because you seem to put the return of mtd_get_of_node(<mtd>), which contradicts what you explain in the previous sentence. I guess somewhere in the same path you have an of_node_get(mtd_get_of_node(<mtd>)) which retains the reference and explains why calling of_node_put(mtd_get_of_node(<mtd>)) is required.
Maybe you can just drop this example.
Mhhh. Maybe I should s/of_node_put/of_node_get/ in the example? I want to show why (currently) no of_node_put() is needed after a mtd_get_of_node().
This examples shows what people do with this helper, ie. calling of_node_get() on the returned OF node to actually retain a reference of the retrieve object.
Thanks, Miquèl