[3/3] mtd: support a cleanup callback for partition parsers

Hi Brian,

On Thu, 19 Nov 2015 19:26:37 -0800
Brian Norris <computersforpeace@gmail.com> wrote:

> If partition parsers need to clean up their resources, we shouldn't
> assume that all memory will fit in a single kmalloc() that the caller
> can kfree(). We should allow the parser to provide a proper cleanup
> routine.
> 
> Note that this means we need to keep a hold on the parser's module for a
> bit longer, and release it later with mtd_part_parser_put().

I like the general idea behind this patch but I would have done it
sightly differently. Here you are keeping the parser around and the
parse_mtd_partitions() caller is responsible for calling the
appropriate cleanup method and releasing the parser reference if any.

How about simplifying callers life by doing all this behind the scene
and keeping the parser reference directly inside the mtd_partition
object (see the following diff).
Of course this implies adding an extra ->parser field to all partitions
while all we need is one parser reference per partition array, but
IMHO, it also keeps the code more readable (I guess it's a matter of
taste).
Another solution would be to declare an mtd_partitions struct
containing the number of partitions, the partition array and a
reference to the partition parser, which would even further simplify
the caller logic (nr_parts would be directly available in the
mtd_partitions struct).

What do you think?

Best Regards,

Boris

Hi Boris,

On Mon, Nov 30, 2015 at 07:36:24PM +0100, Boris Brezillon wrote:
> On Thu, 19 Nov 2015 19:26:37 -0800
> Brian Norris <computersforpeace@gmail.com> wrote:
> 
> > If partition parsers need to clean up their resources, we shouldn't
> > assume that all memory will fit in a single kmalloc() that the caller
> > can kfree(). We should allow the parser to provide a proper cleanup
> > routine.
> > 
> > Note that this means we need to keep a hold on the parser's module for a
> > bit longer, and release it later with mtd_part_parser_put().
> 
> I like the general idea behind this patch but I would have done it
> sightly differently. Here you are keeping the parser around and the
> parse_mtd_partitions() caller is responsible for calling the
> appropriate cleanup method and releasing the parser reference if any.
> 
> How about simplifying callers life by doing all this behind the scene
> and keeping the parser reference directly inside the mtd_partition
> object (see the following diff).
> Of course this implies adding an extra ->parser field to all partitions
> while all we need is one parser reference per partition array, but
> IMHO, it also keeps the code more readable (I guess it's a matter of
> taste).
> Another solution would be to declare an mtd_partitions struct
> containing the number of partitions, the partition array and a
> reference to the partition parser, which would even further simplify
> the caller logic (nr_parts would be directly available in the
> mtd_partitions struct).
> 
> What do you think?

I guess I do like the idea of hiding the handling of the parser
reference so mtd_device_parse_register() doesn't have to track the
parser directly. I'll admit I didn't like yet another
return-by-pointer-argument, but I didn't bother finding a better
solution at the time.

About the extra parser field: it's awkward that you assume the first
partition has the reference, making all the other instances of that
field pointless. Maybe a new mtd_partitions struct would be nice for
encapsulating everything properly.

> Best Regards,
> 
> Boris
> 
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index c8d5494..e0bd54d 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -630,7 +630,7 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
>  	}
>  
>  out:
> -	kfree(real_parts);
> +	mtd_part_cleanup(real_parts, nr_parts);

I purposely distinguished the parsed partitions case from the "provided
by driver" partitions. With your patch, you're letting
mtd_part_cleanup() handle even the case where the parsing code did not
generate 'real_parts'. Though that works for now, I think it's a bad
choice. So we should still have something like:

	if (parser /* or some other equivalent condition */ )
		mtd_part_cleanup( /* stuff allocated in parse_mtd_partitions() */ );
	else
		kfree( /* stuff we allocated in mtd_device_parse_register() */ );

But wait...why do we even kmemdup() anything in
mtd_device_parse_register() at all? add_mtd_partitions() already makes
sure to copy any relevant info, and it passes everything around as
'const'. We should just drop the kmemdup() entirely.

>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(mtd_device_parse_register);
> diff --git a/drivers/mtd/mtdcore.h b/drivers/mtd/mtdcore.h
> index 102cdef..b8a6e07 100644
> --- a/drivers/mtd/mtdcore.h
> +++ b/drivers/mtd/mtdcore.h
> @@ -21,6 +21,25 @@ static inline void mtd_part_parser_put(struct mtd_part_parser *p)
>  	module_put(p->owner);
>  }
>  
> +static inline void mtd_part_cleanup(struct mtd_partition *pparts, int nrparts)
> +{
> +	struct mtd_part_parser *parser = pparts->parser;
> +
> +	/* Some parsers provide their own cleanup function */
> +	if (parser && parser->cleanup)
> +		parser->cleanup(pparts, nrparts);
> +	/*
> +	 * Others have historically relied on the core to kfree() their data.
> +	 * Retain this behavior for legacy.
> +	 */
> +	else
> +		kfree(pparts);

Now that I'm looking at my code again, I'm thinking this could work
better as a default cleanup function. i.e., have __register_mtd_parser()
assign parser->cleanup to something that just calls kfree().

> +
> +	/* Release the reference to the partition parser if any */
> +	if (parser)
> +		mtd_part_parser_put(parser);
> +}
> +
>  int __init init_mtdchar(void);
>  void __exit cleanup_mtdchar(void);
>  

[snip]

Brian

Hi Brian,

On Mon, 30 Nov 2015 15:53:40 -0800
Brian Norris <computersforpeace@gmail.com> wrote:

> Hi Boris,
> 
> On Mon, Nov 30, 2015 at 07:36:24PM +0100, Boris Brezillon wrote:
> > On Thu, 19 Nov 2015 19:26:37 -0800
> > Brian Norris <computersforpeace@gmail.com> wrote:
> > 
> > > If partition parsers need to clean up their resources, we shouldn't
> > > assume that all memory will fit in a single kmalloc() that the caller
> > > can kfree(). We should allow the parser to provide a proper cleanup
> > > routine.
> > > 
> > > Note that this means we need to keep a hold on the parser's module for a
> > > bit longer, and release it later with mtd_part_parser_put().
> > 
> > I like the general idea behind this patch but I would have done it
> > sightly differently. Here you are keeping the parser around and the
> > parse_mtd_partitions() caller is responsible for calling the
> > appropriate cleanup method and releasing the parser reference if any.
> > 
> > How about simplifying callers life by doing all this behind the scene
> > and keeping the parser reference directly inside the mtd_partition
> > object (see the following diff).
> > Of course this implies adding an extra ->parser field to all partitions
> > while all we need is one parser reference per partition array, but
> > IMHO, it also keeps the code more readable (I guess it's a matter of
> > taste).
> > Another solution would be to declare an mtd_partitions struct
> > containing the number of partitions, the partition array and a
> > reference to the partition parser, which would even further simplify
> > the caller logic (nr_parts would be directly available in the
> > mtd_partitions struct).
> > 
> > What do you think?
> 
> I guess I do like the idea of hiding the handling of the parser
> reference so mtd_device_parse_register() doesn't have to track the
> parser directly. I'll admit I didn't like yet another
> return-by-pointer-argument, but I didn't bother finding a better
> solution at the time.
> 
> About the extra parser field: it's awkward that you assume the first
> partition has the reference, making all the other instances of that
> field pointless. Maybe a new mtd_partitions struct would be nice for
> encapsulating everything properly.

Agreed. I just wanted to show that with a minimal amount of changes we
could have a simpler implementation, but I clearly prefer the
mtd_partitions approach.

> 
> > Best Regards,
> > 
> > Boris
> > 
> > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> > index c8d5494..e0bd54d 100644
> > --- a/drivers/mtd/mtdcore.c
> > +++ b/drivers/mtd/mtdcore.c
> > @@ -630,7 +630,7 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
> >  	}
> >  
> >  out:
> > -	kfree(real_parts);
> > +	mtd_part_cleanup(real_parts, nr_parts);
> 
> I purposely distinguished the parsed partitions case from the "provided
> by driver" partitions. With your patch, you're letting
> mtd_part_cleanup() handle even the case where the parsing code did not
> generate 'real_parts'. Though that works for now, I think it's a bad
> choice. So we should still have something like:
> 
> 	if (parser /* or some other equivalent condition */ )
> 		mtd_part_cleanup( /* stuff allocated in parse_mtd_partitions() */ );
> 	else
> 		kfree( /* stuff we allocated in mtd_device_parse_register() */ );
> 
> But wait...why do we even kmemdup() anything in
> mtd_device_parse_register() at all? add_mtd_partitions() already makes
> sure to copy any relevant info, and it passes everything around as
> 'const'. We should just drop the kmemdup() entirely.

That's true. I guess this was done to avoid differentiating the 2 cases
in the cleanup path, but maybe we can do better if a default ->cleanup()
is provided (a simple cleanup() function calling kfree()) ...

> 
> >  	return ret;
> >  }
> >  EXPORT_SYMBOL_GPL(mtd_device_parse_register);
> > diff --git a/drivers/mtd/mtdcore.h b/drivers/mtd/mtdcore.h
> > index 102cdef..b8a6e07 100644
> > --- a/drivers/mtd/mtdcore.h
> > +++ b/drivers/mtd/mtdcore.h
> > @@ -21,6 +21,25 @@ static inline void mtd_part_parser_put(struct mtd_part_parser *p)
> >  	module_put(p->owner);
> >  }
> >  
> > +static inline void mtd_part_cleanup(struct mtd_partition *pparts, int nrparts)
> > +{
> > +	struct mtd_part_parser *parser = pparts->parser;
> > +
> > +	/* Some parsers provide their own cleanup function */
> > +	if (parser && parser->cleanup)
> > +		parser->cleanup(pparts, nrparts);
> > +	/*
> > +	 * Others have historically relied on the core to kfree() their data.
> > +	 * Retain this behavior for legacy.
> > +	 */
> > +	else
> > +		kfree(pparts);
> 
> Now that I'm looking at my code again, I'm thinking this could work
> better as a default cleanup function. i.e., have __register_mtd_parser()
> assign parser->cleanup to something that just calls kfree().

... as you suggest here.

Best Regards,

Boris

On Tue, Dec 01, 2015 at 01:37:32PM +0100, Boris Brezillon wrote:
> On Mon, 30 Nov 2015 15:53:40 -0800
> Brian Norris <computersforpeace@gmail.com> wrote:
> > On Mon, Nov 30, 2015 at 07:36:24PM +0100, Boris Brezillon wrote:
> > > What do you think?
> > 
> > I guess I do like the idea of hiding the handling of the parser
> > reference so mtd_device_parse_register() doesn't have to track the
> > parser directly. I'll admit I didn't like yet another
> > return-by-pointer-argument, but I didn't bother finding a better
> > solution at the time.
> > 
> > About the extra parser field: it's awkward that you assume the first
> > partition has the reference, making all the other instances of that
> > field pointless. Maybe a new mtd_partitions struct would be nice for
> > encapsulating everything properly.
> 
> Agreed. I just wanted to show that with a minimal amount of changes we
> could have a simpler implementation, but I clearly prefer the
> mtd_partitions approach.

OK, do you want to roll up all the suggestions into a new patch series,
or should I?

Brian

On Tue, 1 Dec 2015 19:12:09 -0800
Brian Norris <computersforpeace@gmail.com> wrote:

> On Tue, Dec 01, 2015 at 01:37:32PM +0100, Boris Brezillon wrote:
> > On Mon, 30 Nov 2015 15:53:40 -0800
> > Brian Norris <computersforpeace@gmail.com> wrote:
> > > On Mon, Nov 30, 2015 at 07:36:24PM +0100, Boris Brezillon wrote:
> > > > What do you think?
> > > 
> > > I guess I do like the idea of hiding the handling of the parser
> > > reference so mtd_device_parse_register() doesn't have to track the
> > > parser directly. I'll admit I didn't like yet another
> > > return-by-pointer-argument, but I didn't bother finding a better
> > > solution at the time.
> > > 
> > > About the extra parser field: it's awkward that you assume the first
> > > partition has the reference, making all the other instances of that
> > > field pointless. Maybe a new mtd_partitions struct would be nice for
> > > encapsulating everything properly.
> > 
> > Agreed. I just wanted to show that with a minimal amount of changes we
> > could have a simpler implementation, but I clearly prefer the
> > mtd_partitions approach.
> 
> OK, do you want to roll up all the suggestions into a new patch series,
> or should I?

If you don't mind and have some time I'll let you do it ;-).

Thanks,

Boris