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

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

[Brian Norris]

Hi,

Currently, we assume that all of the resources used by partition parsers can be
cleaned up with a single kfree(), but that can be burdensome to work around.
This series (particularly, patch 3) supports a cleanup() callback for parsers.

Brian

Brian Norris (3):
  mtd: rename MTD parser get/put
  mtd: untangle error codes and number of partitions
  mtd: support a cleanup callback for partition parsers

 drivers/mtd/mtdcore.c          | 22 ++++++++++++++++------
 drivers/mtd/mtdcore.h          | 25 ++++++++++++++++++++++++-
 drivers/mtd/mtdpart.c          | 19 +++++++++++--------
 include/linux/mtd/partitions.h |  1 +
 4 files changed, 52 insertions(+), 15 deletions(-)

-- 
2.6.0.rc2.230.g3dd15c0

[PATCH 1/3] mtd: rename MTD parser get/put

[Brian Norris]

We're going to use put_partition_parser() outside of this file, so let's
fix up the prefix naming a bit, to hopefully be more consistent.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/mtdcore.h |  7 +++++++
 drivers/mtd/mtdpart.c | 10 ++++------
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/mtdcore.h b/drivers/mtd/mtdcore.h
index 7b0353399a10..102cdefa07b5 100644
--- a/drivers/mtd/mtdcore.h
+++ b/drivers/mtd/mtdcore.h
@@ -1,3 +1,5 @@
+#include <linux/mtd/partitions.h>
+
 /*
  * These are exported solely for the purpose of mtd_blkdevs.c and mtdchar.c.
  * You should not use them for _anything_ else.
@@ -14,6 +16,11 @@ int parse_mtd_partitions(struct mtd_info *master, const char * const *types,
 			 struct mtd_partition **pparts,
 			 struct mtd_part_parser_data *data);
 
+static inline void mtd_part_parser_put(struct mtd_part_parser *p)
+{
+	module_put(p->owner);
+}
+
 int __init init_mtdchar(void);
 void __exit cleanup_mtdchar(void);
 
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index 1fa3ca95d9c1..47afef3f4e25 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -684,7 +684,7 @@ int add_mtd_partitions(struct mtd_info *master,
 static DEFINE_SPINLOCK(part_parser_lock);
 static LIST_HEAD(part_parsers);
 
-static struct mtd_part_parser *get_partition_parser(const char *name)
+static struct mtd_part_parser *mtd_part_parser_get(const char *name)
 {
 	struct mtd_part_parser *p, *ret = NULL;
 
@@ -701,8 +701,6 @@ static struct mtd_part_parser *get_partition_parser(const char *name)
 	return ret;
 }
 
-#define put_partition_parser(p) do { module_put((p)->owner); } while (0)
-
 int __register_mtd_parser(struct mtd_part_parser *p, struct module *owner)
 {
 	p->owner = owner;
@@ -765,9 +763,9 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
 
 	for ( ; *types; types++) {
 		pr_debug("%s: parsing partitions %s\n", master->name, *types);
-		parser = get_partition_parser(*types);
+		parser = mtd_part_parser_get(*types);
 		if (!parser && !request_module("%s", *types))
-			parser = get_partition_parser(*types);
+			parser = mtd_part_parser_get(*types);
 		pr_debug("%s: got parser %s\n", master->name,
 			 parser ? parser->name : NULL);
 		if (!parser)
@@ -775,7 +773,7 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
 		ret = (*parser->parse_fn)(master, pparts, data);
 		pr_debug("%s: parser %s: %i\n",
 			 master->name, parser->name, ret);
-		put_partition_parser(parser);
+		mtd_part_parser_put(parser);
 		if (ret > 0) {
 			printk(KERN_NOTICE "%d %s partitions found on MTD device %s\n",
 			       ret, parser->name, master->name);
-- 
2.6.0.rc2.230.g3dd15c0

Re: [PATCH 1/3] mtd: rename MTD parser get/put

[Boris Brezillon]

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

> We're going to use put_partition_parser() outside of this file, so let's
> fix up the prefix naming a bit, to hopefully be more consistent.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>

Reviewed-by: Boris Brezillon <boris.brezillon@free-electrons.com>

> ---
>  drivers/mtd/mtdcore.h |  7 +++++++
>  drivers/mtd/mtdpart.c | 10 ++++------
>  2 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mtd/mtdcore.h b/drivers/mtd/mtdcore.h
> index 7b0353399a10..102cdefa07b5 100644
> --- a/drivers/mtd/mtdcore.h
> +++ b/drivers/mtd/mtdcore.h
> @@ -1,3 +1,5 @@
> +#include <linux/mtd/partitions.h>
> +
>  /*
>   * These are exported solely for the purpose of mtd_blkdevs.c and mtdchar.c.
>   * You should not use them for _anything_ else.
> @@ -14,6 +16,11 @@ int parse_mtd_partitions(struct mtd_info *master, const char * const *types,
>  			 struct mtd_partition **pparts,
>  			 struct mtd_part_parser_data *data);
>  
> +static inline void mtd_part_parser_put(struct mtd_part_parser *p)
> +{
> +	module_put(p->owner);
> +}
> +
>  int __init init_mtdchar(void);
>  void __exit cleanup_mtdchar(void);
>  
> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> index 1fa3ca95d9c1..47afef3f4e25 100644
> --- a/drivers/mtd/mtdpart.c
> +++ b/drivers/mtd/mtdpart.c
> @@ -684,7 +684,7 @@ int add_mtd_partitions(struct mtd_info *master,
>  static DEFINE_SPINLOCK(part_parser_lock);
>  static LIST_HEAD(part_parsers);
>  
> -static struct mtd_part_parser *get_partition_parser(const char *name)
> +static struct mtd_part_parser *mtd_part_parser_get(const char *name)
>  {
>  	struct mtd_part_parser *p, *ret = NULL;
>  
> @@ -701,8 +701,6 @@ static struct mtd_part_parser *get_partition_parser(const char *name)
>  	return ret;
>  }
>  
> -#define put_partition_parser(p) do { module_put((p)->owner); } while (0)
> -
>  int __register_mtd_parser(struct mtd_part_parser *p, struct module *owner)
>  {
>  	p->owner = owner;
> @@ -765,9 +763,9 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
>  
>  	for ( ; *types; types++) {
>  		pr_debug("%s: parsing partitions %s\n", master->name, *types);
> -		parser = get_partition_parser(*types);
> +		parser = mtd_part_parser_get(*types);
>  		if (!parser && !request_module("%s", *types))
> -			parser = get_partition_parser(*types);
> +			parser = mtd_part_parser_get(*types);
>  		pr_debug("%s: got parser %s\n", master->name,
>  			 parser ? parser->name : NULL);
>  		if (!parser)
> @@ -775,7 +773,7 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
>  		ret = (*parser->parse_fn)(master, pparts, data);
>  		pr_debug("%s: parser %s: %i\n",
>  			 master->name, parser->name, ret);
> -		put_partition_parser(parser);
> +		mtd_part_parser_put(parser);
>  		if (ret > 0) {
>  			printk(KERN_NOTICE "%d %s partitions found on MTD device %s\n",
>  			       ret, parser->name, master->name);



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH 2/3] mtd: untangle error codes and number of partitions

[Brian Norris]

We're going to need to know how many partitions were registered, so
let's disentangle the 'ret' and 'nr_parts' variables, so that nr_parts
always represents the number of partitions we're registering.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/mtdcore.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 62f83b050978..c8d54948bbc1 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -592,23 +592,25 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
 	struct mtd_partition *real_parts = NULL;
 
 	ret = parse_mtd_partitions(mtd, types, &real_parts, parser_data);
-	if (ret <= 0 && nr_parts && parts) {
+	if (ret > 0) {
+		nr_parts = ret;
+	} else if (ret <= 0 && nr_parts && parts) {
 		real_parts = kmemdup(parts, sizeof(*parts) * nr_parts,
 				     GFP_KERNEL);
 		if (!real_parts)
 			ret = -ENOMEM;
 		else
-			ret = nr_parts;
+			ret = 0;
 	}
 	/* Didn't come up with either parsed OR fallback partitions */
 	if (ret < 0) {
 		pr_info("mtd: failed to find partitions; one or more parsers reports errors (%d)\n",
 			ret);
 		/* Don't abort on errors; we can still use unpartitioned MTD */
-		ret = 0;
+		nr_parts = 0;
 	}
 
-	ret = mtd_add_device_partitions(mtd, real_parts, ret);
+	ret = mtd_add_device_partitions(mtd, real_parts, nr_parts);
 	if (ret)
 		goto out;
 
-- 
2.6.0.rc2.230.g3dd15c0

Re: [PATCH 2/3] mtd: untangle error codes and number of partitions

[Boris Brezillon]

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

> We're going to need to know how many partitions were registered, so
> let's disentangle the 'ret' and 'nr_parts' variables, so that nr_parts
> always represents the number of partitions we're registering.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>

Reviewed-by: Boris Brezillon <boris.brezillon@free-electrons.com>

> ---
>  drivers/mtd/mtdcore.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 62f83b050978..c8d54948bbc1 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -592,23 +592,25 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
>  	struct mtd_partition *real_parts = NULL;
>  
>  	ret = parse_mtd_partitions(mtd, types, &real_parts, parser_data);
> -	if (ret <= 0 && nr_parts && parts) {
> +	if (ret > 0) {
> +		nr_parts = ret;
> +	} else if (ret <= 0 && nr_parts && parts) {
>  		real_parts = kmemdup(parts, sizeof(*parts) * nr_parts,
>  				     GFP_KERNEL);
>  		if (!real_parts)
>  			ret = -ENOMEM;
>  		else
> -			ret = nr_parts;
> +			ret = 0;
>  	}
>  	/* Didn't come up with either parsed OR fallback partitions */
>  	if (ret < 0) {
>  		pr_info("mtd: failed to find partitions; one or more parsers reports errors (%d)\n",
>  			ret);
>  		/* Don't abort on errors; we can still use unpartitioned MTD */
> -		ret = 0;
> +		nr_parts = 0;
>  	}
>  
> -	ret = mtd_add_device_partitions(mtd, real_parts, ret);
> +	ret = mtd_add_device_partitions(mtd, real_parts, nr_parts);
>  	if (ret)
>  		goto out;
>  



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

[Brian Norris]

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

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/mtdcore.c          | 12 ++++++++++--
 drivers/mtd/mtdcore.h          | 18 +++++++++++++++++-
 drivers/mtd/mtdpart.c          | 11 ++++++++---
 include/linux/mtd/partitions.h |  1 +
 4 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index c8d54948bbc1..c366de5e0124 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -590,8 +590,10 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
 {
 	int ret;
 	struct mtd_partition *real_parts = NULL;
+	struct mtd_part_parser *parser = NULL;
 
-	ret = parse_mtd_partitions(mtd, types, &real_parts, parser_data);
+	ret = parse_mtd_partitions(mtd, types, &real_parts, parser_data,
+				   &parser);
 	if (ret > 0) {
 		nr_parts = ret;
 	} else if (ret <= 0 && nr_parts && parts) {
@@ -630,7 +632,13 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
 	}
 
 out:
-	kfree(real_parts);
+	if (parser) {
+		mtd_part_parser_cleanup(parser, real_parts, nr_parts);
+		mtd_part_parser_put(parser);
+	} else {
+		kfree(real_parts);
+	}
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(mtd_device_parse_register);
diff --git a/drivers/mtd/mtdcore.h b/drivers/mtd/mtdcore.h
index 102cdefa07b5..2cacad7cfbdc 100644
--- a/drivers/mtd/mtdcore.h
+++ b/drivers/mtd/mtdcore.h
@@ -14,13 +14,29 @@ int add_mtd_partitions(struct mtd_info *, const struct mtd_partition *, int);
 int del_mtd_partitions(struct mtd_info *);
 int parse_mtd_partitions(struct mtd_info *master, const char * const *types,
 			 struct mtd_partition **pparts,
-			 struct mtd_part_parser_data *data);
+			 struct mtd_part_parser_data *data,
+			 struct mtd_part_parser **ret_parser);
 
 static inline void mtd_part_parser_put(struct mtd_part_parser *p)
 {
 	module_put(p->owner);
 }
 
+static inline void mtd_part_parser_cleanup(struct mtd_part_parser *parser,
+					   struct mtd_partition *pparts,
+					   int nrparts)
+{
+	/* Some parsers provide their own cleanup function */
+	if (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);
+}
+
 int __init init_mtdchar(void);
 void __exit cleanup_mtdchar(void);
 
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index 47afef3f4e25..5146c4e71a47 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -737,6 +737,7 @@ static const char * const default_mtd_part_types[] = {
  * @types: names of partition parsers to try or %NULL
  * @pparts: array of partitions found is returned here
  * @data: MTD partition parser-specific data
+ * @ret_parser: MTD parser used; assigned only on successful parsing
  *
  * This function tries to find partition on MTD device @master. It uses MTD
  * partition parsers, specified in @types. However, if @types is %NULL, then
@@ -749,11 +750,14 @@ static const char * const default_mtd_part_types[] = {
  * o a negative error code in case of failure
  * o zero if no partitions were found
  * o a positive number of found partitions, in which case on exit @pparts will
- *   point to an array containing this number of &struct mtd_info objects.
+ *   point to an array containing this number of &struct mtd_info objects, and
+ *   @ret_parser will point to the parser used. Note that the caller must call
+ *   'mtd_part_parser_put()' on this struct when finished with it.
  */
 int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
 			 struct mtd_partition **pparts,
-			 struct mtd_part_parser_data *data)
+			 struct mtd_part_parser_data *data,
+			 struct mtd_part_parser **ret_parser)
 {
 	struct mtd_part_parser *parser;
 	int ret, err = 0;
@@ -773,12 +777,13 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
 		ret = (*parser->parse_fn)(master, pparts, data);
 		pr_debug("%s: parser %s: %i\n",
 			 master->name, parser->name, ret);
-		mtd_part_parser_put(parser);
 		if (ret > 0) {
 			printk(KERN_NOTICE "%d %s partitions found on MTD device %s\n",
 			       ret, parser->name, master->name);
+			*ret_parser = parser;
 			return ret;
 		}
+		mtd_part_parser_put(parser);
 		/*
 		 * Stash the first error we see; only report it if no parser
 		 * succeeds
diff --git a/include/linux/mtd/partitions.h b/include/linux/mtd/partitions.h
index d002d9b5d797..d1fd7edbfcfe 100644
--- a/include/linux/mtd/partitions.h
+++ b/include/linux/mtd/partitions.h
@@ -71,6 +71,7 @@ struct mtd_part_parser {
 	const char *name;
 	int (*parse_fn)(struct mtd_info *, struct mtd_partition **,
 			struct mtd_part_parser_data *);
+	void (*cleanup)(struct mtd_partition *pparts, int nr_parts);
 };
 
 extern int __register_mtd_parser(struct mtd_part_parser *parser,
-- 
2.6.0.rc2.230.g3dd15c0

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

[Boris Brezillon]

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

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);
 	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);
+
+	/* 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);
 
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index 47afef3..2452a57 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -773,12 +773,13 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
 		ret = (*parser->parse_fn)(master, pparts, data);
 		pr_debug("%s: parser %s: %i\n",
 			 master->name, parser->name, ret);
-		mtd_part_parser_put(parser);
 		if (ret > 0) {
 			printk(KERN_NOTICE "%d %s partitions found on MTD device %s\n",
 			       ret, parser->name, master->name);
+			(*pparts)->parser = parser;
 			return ret;
 		}
+		mtd_part_parser_put(parser);
 		/*
 		 * Stash the first error we see; only report it if no parser
 		 * succeeds
diff --git a/include/linux/mtd/partitions.h b/include/linux/mtd/partitions.h
index d002d9b..5ba07f2 100644
--- a/include/linux/mtd/partitions.h
+++ b/include/linux/mtd/partitions.h
@@ -11,6 +11,7 @@
 
 #include <linux/types.h>
 
+struct mtd_part_parser;
 
 /*
  * Partition definition structure:
@@ -31,6 +32,8 @@
  * 	master MTD flag set for the corresponding MTD partition.
  * 	For example, to force a read-only partition, simply adding
  * 	MTD_WRITEABLE to the mask_flags will do the trick.
+ * parser: partition parser that created this partition. Only set in the
+ *	   the first entry of the partiton array.
  *
  * Note: writeable partitions require their size and offset be
  * erasesize aligned (e.g. use MTDPART_OFS_NEXTBLK).
@@ -41,6 +44,7 @@ struct mtd_partition {
 	uint64_t size;			/* partition size */
 	uint64_t offset;		/* offset within the master MTD space */
 	uint32_t mask_flags;		/* master MTD flags to mask out for this partition */
+	struct mtd_part_parser *parser;
 };
 
 #define MTDPART_OFS_RETAIN	(-3)
@@ -71,6 +75,7 @@ struct mtd_part_parser {
 	const char *name;
 	int (*parse_fn)(struct mtd_info *, struct mtd_partition **,
 			struct mtd_part_parser_data *);
+	void (*cleanup)(struct mtd_partition *pparts, int nr_parts);
 };
 
 extern int __register_mtd_parser(struct mtd_part_parser *parser,


-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

[Brian Norris]

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

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

[Boris Brezillon]

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

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

[Brian Norris]

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

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

[Boris Brezillon]

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

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com