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. 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>)).
The direct effect of such unbalanced reference counting is visible by rmmod'ing any module that would have added MTD partitions:
OF: ERROR: Bad of_node_put() on <of_path_to_partition>
As it seems normal to get a reference on the OF node during the of_property_for_each_string() that follows, add a call to of_node_get() when relevant.
Fixes: 76a832254ab0 ("mtd: partitions: use DT info for parsing partitions with "compatible" prop") Cc: stable@vger.kernel.org Signed-off-by: Miquel Raynal miquel.raynal@bootlin.com --- drivers/mtd/mtdpart.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c index 52e2cb35fc79..99c460facd5e 100644 --- a/drivers/mtd/mtdpart.c +++ b/drivers/mtd/mtdpart.c @@ -873,8 +873,11 @@ static int mtd_part_of_parse(struct mtd_info *master, int ret, err = 0;
np = mtd_get_of_node(master); - if (!mtd_is_partition(master)) + if (mtd_is_partition(master)) + of_node_get(np); + else np = of_get_child_by_name(np, "partitions"); + of_property_for_each_string(np, "compatible", prop, compat) { parser = mtd_part_get_compatible_parser(compat); if (!parser)
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. 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>)).The direct effect of such unbalanced reference counting is visible by rmmod'ing any module that would have added MTD partitions:
OF: ERROR: Bad of_node_put() on <of_path_to_partition>As it seems normal to get a reference on the OF node during the of_property_for_each_string() that follows, add a call to of_node_get() when relevant.
Fixes: 76a832254ab0 ("mtd: partitions: use DT info for parsing partitions with "compatible" prop") Cc: stable@vger.kernel.org Signed-off-by: Miquel Raynal miquel.raynal@bootlin.com
drivers/mtd/mtdpart.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c index 52e2cb35fc79..99c460facd5e 100644 --- a/drivers/mtd/mtdpart.c +++ b/drivers/mtd/mtdpart.c @@ -873,8 +873,11 @@ static int mtd_part_of_parse(struct mtd_info *master, int ret, err = 0; np = mtd_get_of_node(master);
- if (!mtd_is_partition(master))
- if (mtd_is_partition(master))
of_node_get(np);- else np = of_get_child_by_name(np, "partitions");
- of_property_for_each_string(np, "compatible", prop, compat) { parser = mtd_part_get_compatible_parser(compat); if (!parser)
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.
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?
The direct effect of such unbalanced reference counting is visible by rmmod'ing any module that would have added MTD partitions:
OF: ERROR: Bad of_node_put() on <of_path_to_partition>As it seems normal to get a reference on the OF node during the of_property_for_each_string() that follows, add a call to of_node_get() when relevant.
Fixes: 76a832254ab0 ("mtd: partitions: use DT info for parsing partitions with "compatible" prop") Cc: stable@vger.kernel.org
You can drop the above line since this patch is fixing a bug introduced in 4.19-rc1.
Signed-off-by: Miquel Raynal miquel.raynal@bootlin.com
drivers/mtd/mtdpart.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c index 52e2cb35fc79..99c460facd5e 100644 --- a/drivers/mtd/mtdpart.c +++ b/drivers/mtd/mtdpart.c @@ -873,8 +873,11 @@ static int mtd_part_of_parse(struct mtd_info *master, int ret, err = 0; np = mtd_get_of_node(master);
- if (!mtd_is_partition(master))
- if (mtd_is_partition(master))
of_node_get(np);- else np = of_get_child_by_name(np, "partitions");
- of_property_for_each_string(np, "compatible", prop, compat) { parser = mtd_part_get_compatible_parser(compat); if (!parser)
The patch itself looks correct, but I'd like some clarification about the commit message before applying it.
Thanks,
Boris
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?
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. I think if the helper was named "mtd_to_of_node()" that would be much clearer for everyone and of_node_get(mtd_to_of_node(mtd)) would be the way to retain a reference on the OF node.
I don't think creating a helper for that would be better because I really prefer seeing the of_node_get() in the code, meaning an of_node_put() will be needed at some point.
The direct effect of such unbalanced reference counting is visible by rmmod'ing any module that would have added MTD partitions:
OF: ERROR: Bad of_node_put() on <of_path_to_partition>As it seems normal to get a reference on the OF node during the of_property_for_each_string() that follows, add a call to of_node_get() when relevant.
Fixes: 76a832254ab0 ("mtd: partitions: use DT info for parsing partitions with "compatible" prop") Cc: stable@vger.kernel.org
You can drop the above line since this patch is fixing a bug introduced in 4.19-rc1.
Right!
Signed-off-by: Miquel Raynal miquel.raynal@bootlin.com
drivers/mtd/mtdpart.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c index 52e2cb35fc79..99c460facd5e 100644 --- a/drivers/mtd/mtdpart.c +++ b/drivers/mtd/mtdpart.c @@ -873,8 +873,11 @@ static int mtd_part_of_parse(struct mtd_info *master, int ret, err = 0; np = mtd_get_of_node(master);
- if (!mtd_is_partition(master))
- if (mtd_is_partition(master))
of_node_get(np);- else np = of_get_child_by_name(np, "partitions");
- of_property_for_each_string(np, "compatible", prop, compat) { parser = mtd_part_get_compatible_parser(compat); if (!parser)
The patch itself looks correct, but I'd like some clarification about the commit message before applying it.
Thanks,
Boris
Thanks, Miquèl
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.
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.
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.
I think if the helper was named "mtd_to_of_node()" that would be much clearer for everyone and of_node_get(mtd_to_of_node(mtd)) would be the way to retain a reference on the OF node.
I don't think creating a helper for that would be better because I really prefer seeing the of_node_get() in the code, meaning an of_node_put() will be needed at some point.
Again, it's mainly a matter of consistency. If people are used to call of_node_put() when a function returns a device_node object, then it's better to do the same in the MTD framework.
On Mon, 10 Sep 2018 15:25:51 +0200 Boris Brezillon boris.brezillon@bootlin.com wrote:
I think if the helper was named "mtd_to_of_node()" that would be much clearer for everyone and of_node_get(mtd_to_of_node(mtd)) would be the way to retain a reference on the OF node.
I don't think creating a helper for that would be better because I really prefer seeing the of_node_get() in the code, meaning an of_node_put() will be needed at some point.
Again, it's mainly a matter of consistency. If people are used to call of_node_put() when a function returns a device_node object, then it's better to do the same in the MTD framework.
Just to be clear. I still want to queue this patch for 4.19-rc4. All I'm saying is that we should consider changing the behavior of mtd_get_of_node() (and friends) at some point, otherwise we might see other bugs like this one in the future.
On a side note, we should probably also call of_node_get/put() when someone updates mtd->dev.of_node through the mtd_set_of_node() helper, and call of_node_put() when an mtd device is unregistered.
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
On Mon, 10 Sep 2018 15:38:31 +0200 Miquel Raynal miquel.raynal@bootlin.com wrote:
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.
Or you just say that mtd_get_of_node() does not retain a reference to the device_node object it returns and that should be enough ;-).
Hi Boris,
Boris Brezillon boris.brezillon@bootlin.com wrote on Mon, 10 Sep 2018 15:42:19 +0200:
On Mon, 10 Sep 2018 15:38:31 +0200 Miquel Raynal miquel.raynal@bootlin.com wrote:
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.
Or you just say that mtd_get_of_node() does not retain a reference to the device_node object it returns and that should be enough ;-).
Fine by me, you can apply it and modify in place. If you want a new version, just ask.
I will soon send a patch cleaning the mtd_of_get/put() so that it does retain a reference (as most of_ helpers do), as discussed earlier.
Thanks, Miquèl
On Mon, 17 Sep 2018 11:55:20 +0200 Miquel Raynal miquel.raynal@bootlin.com wrote:
Or you just say that mtd_get_of_node() does not retain a reference to the device_node object it returns and that should be enough ;-).
Fine by me, you can apply it and modify in place.
Here is the new commit message:
" mtd: partitions: fix unbalanced of_node_get/put()
While at first mtd_part_of_parse() would just call of_get_chil_by_name(), it has been patched to deal with sub-partitions and will now directly manipulate the node returned mtd_get_of_node() if the MTD device is a partition.
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. It is a simple helper hiding some pointer logic to retrieve the OF node related to an MTD device.
The direct effect of such unbalanced reference counting is visible by rmmod'ing any module that would have added MTD partitions:
OF: ERROR: Bad of_node_put() on <of_path_to_partition>
As it seems normal to get a reference on the OF node during the of_property_for_each_string() that follows, add a call to of_node_get() when relevant.
Fixes: 76a832254ab0 ("mtd: partitions: use DT info for parsing partitions with "compatible" prop") Cc: stable@vger.kernel.org Signed-off-by: Miquel Raynal miquel.raynal@bootlin.com Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com "
Let me know if you're okay with this update.
Thanks,
Boris
Hi Boris,
Boris Brezillon boris.brezillon@bootlin.com wrote on Mon, 17 Sep 2018 15:51:40 +0200:
On Mon, 17 Sep 2018 11:55:20 +0200 Miquel Raynal miquel.raynal@bootlin.com wrote:
Or you just say that mtd_get_of_node() does not retain a reference to the device_node object it returns and that should be enough ;-).
Fine by me, you can apply it and modify in place.
Here is the new commit message:
" mtd: partitions: fix unbalanced of_node_get/put()
While at first mtd_part_of_parse() would just call of_get_chil_by_name(), it has been patched to deal with sub-partitions and will now directly manipulate the node returned mtd_get_of_node() if
^by
the MTD device is a partition.
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. It is a simple helper hiding some pointer logic to retrieve the OF node related to an MTD device. The direct effect of such unbalanced reference counting is visible by rmmod'ing any module that would have added MTD partitions:
OF: ERROR: Bad of_node_put() on <of_path_to_partition>
As it seems normal to get a reference on the OF node during the of_property_for_each_string() that follows, add a call to of_node_get() when relevant. Fixes: 76a832254ab0 ("mtd: partitions: use DT info for parsing partitions with "compatible" prop") Cc: stable@vger.kernel.org Signed-off-by: Miquel Raynal miquel.raynal@bootlin.com Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com "
Let me know if you're okay with this update.
One missing work, otherwise I'm ok.
Thanks, Miquèl
On Mon, 17 Sep 2018 16:03:18 +0200 Miquel Raynal miquel.raynal@bootlin.com wrote:
Hi Boris,
Boris Brezillon boris.brezillon@bootlin.com wrote on Mon, 17 Sep 2018 15:51:40 +0200:
On Mon, 17 Sep 2018 11:55:20 +0200 Miquel Raynal miquel.raynal@bootlin.com wrote:
Or you just say that mtd_get_of_node() does not retain a reference to the device_node object it returns and that should be enough ;-).
Fine by me, you can apply it and modify in place.
Here is the new commit message:
" mtd: partitions: fix unbalanced of_node_get/put()
While at first mtd_part_of_parse() would just call of_get_chil_by_name(), it has been patched to deal with sub-partitions and will now directly manipulate the node returned mtd_get_of_node() if
^bythe MTD device is a partition.
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. It is a simple helper hiding some pointer logic to retrieve the OF node related to an MTD device. The direct effect of such unbalanced reference counting is visible by rmmod'ing any module that would have added MTD partitions:
OF: ERROR: Bad of_node_put() on <of_path_to_partition>
As it seems normal to get a reference on the OF node during the of_property_for_each_string() that follows, add a call to of_node_get() when relevant. Fixes: 76a832254ab0 ("mtd: partitions: use DT info for parsing partitions with "compatible" prop") Cc: stable@vger.kernel.org Signed-off-by: Miquel Raynal miquel.raynal@bootlin.com Signed-off-by: Boris Brezillon boris.brezillon@bootlin.com "
Let me know if you're okay with this update.
One missing work, otherwise I'm ok.
Fixed and pushed.
Thanks,
Boris
linux-stable-mirror@lists.linaro.org
-
Boris Brezillon -
Miquel Raynal