[PATCH] mtd: partitions: fix of_node_get/put balance in parser

[PATCH] mtd: partitions: fix of_node_get/put balance in parser

[Miquel Raynal]

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)
-- 
2.17.1

Re: [PATCH] mtd: partitions: fix of_node_get/put balance in parser

[Miquel Raynal]

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)

Re: [PATCH] mtd: partitions: fix of_node_get/put balance in parser

[Boris Brezillon]

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

-- 
Boris Brezillon, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] mtd: partitions: fix of_node_get/put balance in parser

[Miquel Raynal]

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

Re: [PATCH] mtd: partitions: fix of_node_get/put balance in parser

[Boris Brezillon]

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.

Re: [PATCH] mtd: partitions: fix of_node_get/put balance in parser

[Boris Brezillon]

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.

Re: [PATCH] mtd: partitions: fix of_node_get/put balance in parser

[Miquel Raynal]

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

Re: [PATCH] mtd: partitions: fix of_node_get/put balance in parser

[Boris Brezillon]

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 ;-).

Re: [PATCH] mtd: partitions: fix of_node_get/put balance in parser

[Miquel Raynal]

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

Re: [PATCH] mtd: partitions: fix of_node_get/put balance in parser

[Boris Brezillon]

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

Re: [PATCH] mtd: partitions: fix of_node_get/put balance in parser

[Miquel Raynal]

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

Re: [PATCH] mtd: partitions: fix of_node_get/put balance in parser

[Boris Brezillon]

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  
> 
>                                                     ^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.

Fixed and pushed.

Thanks,

Boris