[PATCH v2 0/6] mtd: partitions: support cleanup callback for parsers

[PATCH v2 0/6] mtd: partitions: support cleanup callback for parsers

[Brian Norris]

Hi,

Here's v2, which should address the comments made on v1.

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 6) supports a cleanup() callback for parsers.

v1 -> v2:
 * add "mtd_partitions" struct to hold info about the array of parsed partitions
 * const-ify some arguments, which requires some small refactoring of the
   existing partitions parsers
 * remove the kmemdup() in mtd_device_parse_register()
 * provide default cleanup routine for parsers that don't have one (all
   parsers, ATM)
 * put more common logic in mtd_part_parser_cleanup(), to avoid making the
   caller worry about some of the reference counting
 * (hopefully) less convoluted error handling in mtd_device_parse_register()

Brian

Brian Norris (6):
  mtd: ofpart: assign return argument exactly once
  mtd: partitions: make parsers return 'const' partition arrays
  mtd: partitions: rename MTD parser get/put
  mtd: partitions: remove kmemdup()
  mtd: partitions: pass around 'mtd_partitions' wrapper struct
  mtd: partitions: support a cleanup callback for parsers

 drivers/mtd/afs.c              |  2 +-
 drivers/mtd/ar7part.c          |  2 +-
 drivers/mtd/bcm47xxpart.c      |  2 +-
 drivers/mtd/bcm63xxpart.c      |  2 +-
 drivers/mtd/cmdlinepart.c      |  2 +-
 drivers/mtd/mtdcore.c          | 39 +++++++++++++++-------------
 drivers/mtd/mtdcore.h          |  7 ++++-
 drivers/mtd/mtdpart.c          | 59 +++++++++++++++++++++++++++++++++---------
 drivers/mtd/ofpart.c           | 39 +++++++++++++++-------------
 drivers/mtd/redboot.c          |  2 +-
 include/linux/mtd/partitions.h | 10 ++++++-
 11 files changed, 110 insertions(+), 56 deletions(-)

-- 
2.6.0.rc2.230.g3dd15c0

[PATCH v2 1/6] mtd: ofpart: assign return argument exactly once

[Brian Norris]

It's easier to refactor these parsers if the return value gets assigned
only once, just like every other MTD partition parser.

This prepares for making the second arg to the parse_fn() const. This is
OK if we construct the partitions completely first, and assign them to
the return pointer only after we're done modifying them.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
New in v2

 drivers/mtd/ofpart.c | 35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/drivers/mtd/ofpart.c b/drivers/mtd/ofpart.c
index 478538100ddd..4800fecaf8cf 100644
--- a/drivers/mtd/ofpart.c
+++ b/drivers/mtd/ofpart.c
@@ -29,6 +29,7 @@ static int parse_ofpart_partitions(struct mtd_info *master,
 				   struct mtd_partition **pparts,
 				   struct mtd_part_parser_data *data)
 {
+	struct mtd_partition *parts;
 	struct device_node *mtd_node;
 	struct device_node *ofpart_node;
 	const char *partname;
@@ -62,8 +63,8 @@ static int parse_ofpart_partitions(struct mtd_info *master,
 	if (nr_parts == 0)
 		return 0;
 
-	*pparts = kzalloc(nr_parts * sizeof(**pparts), GFP_KERNEL);
-	if (!*pparts)
+	parts = kzalloc(nr_parts * sizeof(*parts), GFP_KERNEL);
+	if (!parts)
 		return -ENOMEM;
 
 	i = 0;
@@ -97,19 +98,19 @@ static int parse_ofpart_partitions(struct mtd_info *master,
 			goto ofpart_fail;
 		}
 
-		(*pparts)[i].offset = of_read_number(reg, a_cells);
-		(*pparts)[i].size = of_read_number(reg + a_cells, s_cells);
+		parts[i].offset = of_read_number(reg, a_cells);
+		parts[i].size = of_read_number(reg + a_cells, s_cells);
 
 		partname = of_get_property(pp, "label", &len);
 		if (!partname)
 			partname = of_get_property(pp, "name", &len);
-		(*pparts)[i].name = partname;
+		parts[i].name = partname;
 
 		if (of_get_property(pp, "read-only", &len))
-			(*pparts)[i].mask_flags |= MTD_WRITEABLE;
+			parts[i].mask_flags |= MTD_WRITEABLE;
 
 		if (of_get_property(pp, "lock", &len))
-			(*pparts)[i].mask_flags |= MTD_POWERUP_LOCK;
+			parts[i].mask_flags |= MTD_POWERUP_LOCK;
 
 		i++;
 	}
@@ -117,6 +118,7 @@ static int parse_ofpart_partitions(struct mtd_info *master,
 	if (!nr_parts)
 		goto ofpart_none;
 
+	*pparts = parts;
 	return nr_parts;
 
 ofpart_fail:
@@ -125,8 +127,7 @@ ofpart_fail:
 	ret = -EINVAL;
 ofpart_none:
 	of_node_put(pp);
-	kfree(*pparts);
-	*pparts = NULL;
+	kfree(parts);
 	return ret;
 }
 
@@ -139,6 +140,7 @@ static int parse_ofoldpart_partitions(struct mtd_info *master,
 				      struct mtd_partition **pparts,
 				      struct mtd_part_parser_data *data)
 {
+	struct mtd_partition *parts;
 	struct device_node *dp;
 	int i, plen, nr_parts;
 	const struct {
@@ -160,32 +162,33 @@ static int parse_ofoldpart_partitions(struct mtd_info *master,
 
 	nr_parts = plen / sizeof(part[0]);
 
-	*pparts = kzalloc(nr_parts * sizeof(*(*pparts)), GFP_KERNEL);
-	if (!*pparts)
+	parts = kzalloc(nr_parts * sizeof(*parts), GFP_KERNEL);
+	if (!parts)
 		return -ENOMEM;
 
 	names = of_get_property(dp, "partition-names", &plen);
 
 	for (i = 0; i < nr_parts; i++) {
-		(*pparts)[i].offset = be32_to_cpu(part->offset);
-		(*pparts)[i].size   = be32_to_cpu(part->len) & ~1;
+		parts[i].offset = be32_to_cpu(part->offset);
+		parts[i].size   = be32_to_cpu(part->len) & ~1;
 		/* bit 0 set signifies read only partition */
 		if (be32_to_cpu(part->len) & 1)
-			(*pparts)[i].mask_flags = MTD_WRITEABLE;
+			parts[i].mask_flags = MTD_WRITEABLE;
 
 		if (names && (plen > 0)) {
 			int len = strlen(names) + 1;
 
-			(*pparts)[i].name = names;
+			parts[i].name = names;
 			plen -= len;
 			names += len;
 		} else {
-			(*pparts)[i].name = "unnamed";
+			parts[i].name = "unnamed";
 		}
 
 		part++;
 	}
 
+	*pparts = parts;
 	return nr_parts;
 }
 
-- 
2.6.0.rc2.230.g3dd15c0

[PATCH v2 2/6] mtd: partitions: make parsers return 'const' partition arrays

[Brian Norris]

We only want to modify these arrays inside the parser "drivers", so the
drivers should construct them however they like, then return them as
immutable arrays.

This will make other refactorings easier.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
New in v2

 drivers/mtd/afs.c              | 2 +-
 drivers/mtd/ar7part.c          | 2 +-
 drivers/mtd/bcm47xxpart.c      | 2 +-
 drivers/mtd/bcm63xxpart.c      | 2 +-
 drivers/mtd/cmdlinepart.c      | 2 +-
 drivers/mtd/ofpart.c           | 4 ++--
 drivers/mtd/redboot.c          | 2 +-
 include/linux/mtd/partitions.h | 2 +-
 8 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/mtd/afs.c b/drivers/mtd/afs.c
index e02dae3b739b..d61b7edfc938 100644
--- a/drivers/mtd/afs.c
+++ b/drivers/mtd/afs.c
@@ -162,7 +162,7 @@ afs_read_iis_v1(struct mtd_info *mtd, struct image_info_v1 *iis, u_int ptr)
 }
 
 static int parse_afs_partitions(struct mtd_info *mtd,
-				struct mtd_partition **pparts,
+				const struct mtd_partition **pparts,
 				struct mtd_part_parser_data *data)
 {
 	struct mtd_partition *parts;
diff --git a/drivers/mtd/ar7part.c b/drivers/mtd/ar7part.c
index 9203b96fd789..90575deff0ae 100644
--- a/drivers/mtd/ar7part.c
+++ b/drivers/mtd/ar7part.c
@@ -43,7 +43,7 @@ struct ar7_bin_rec {
 };
 
 static int create_mtd_partitions(struct mtd_info *master,
-				 struct mtd_partition **pparts,
+				 const struct mtd_partition **pparts,
 				 struct mtd_part_parser_data *data)
 {
 	struct ar7_bin_rec header;
diff --git a/drivers/mtd/bcm47xxpart.c b/drivers/mtd/bcm47xxpart.c
index 92a6dd18198b..8282f47bcf5d 100644
--- a/drivers/mtd/bcm47xxpart.c
+++ b/drivers/mtd/bcm47xxpart.c
@@ -82,7 +82,7 @@ out_default:
 }
 
 static int bcm47xxpart_parse(struct mtd_info *master,
-			     struct mtd_partition **pparts,
+			     const struct mtd_partition **pparts,
 			     struct mtd_part_parser_data *data)
 {
 	struct mtd_partition *parts;
diff --git a/drivers/mtd/bcm63xxpart.c b/drivers/mtd/bcm63xxpart.c
index cf02135320bc..440936998593 100644
--- a/drivers/mtd/bcm63xxpart.c
+++ b/drivers/mtd/bcm63xxpart.c
@@ -68,7 +68,7 @@ static int bcm63xx_detect_cfe(struct mtd_info *master)
 }
 
 static int bcm63xx_parse_cfe_partitions(struct mtd_info *master,
-					struct mtd_partition **pparts,
+					const struct mtd_partition **pparts,
 					struct mtd_part_parser_data *data)
 {
 	/* CFE, NVRAM and global Linux are always present */
diff --git a/drivers/mtd/cmdlinepart.c b/drivers/mtd/cmdlinepart.c
index 420489864bc2..fbd5affc0acf 100644
--- a/drivers/mtd/cmdlinepart.c
+++ b/drivers/mtd/cmdlinepart.c
@@ -304,7 +304,7 @@ static int mtdpart_setup_real(char *s)
  * the first one in the chain if a NULL mtd_id is passed in.
  */
 static int parse_cmdline_partitions(struct mtd_info *master,
-				    struct mtd_partition **pparts,
+				    const struct mtd_partition **pparts,
 				    struct mtd_part_parser_data *data)
 {
 	unsigned long long offset;
diff --git a/drivers/mtd/ofpart.c b/drivers/mtd/ofpart.c
index 4800fecaf8cf..8a027a71e662 100644
--- a/drivers/mtd/ofpart.c
+++ b/drivers/mtd/ofpart.c
@@ -26,7 +26,7 @@ static bool node_has_compatible(struct device_node *pp)
 }
 
 static int parse_ofpart_partitions(struct mtd_info *master,
-				   struct mtd_partition **pparts,
+				   const struct mtd_partition **pparts,
 				   struct mtd_part_parser_data *data)
 {
 	struct mtd_partition *parts;
@@ -137,7 +137,7 @@ static struct mtd_part_parser ofpart_parser = {
 };
 
 static int parse_ofoldpart_partitions(struct mtd_info *master,
-				      struct mtd_partition **pparts,
+				      const struct mtd_partition **pparts,
 				      struct mtd_part_parser_data *data)
 {
 	struct mtd_partition *parts;
diff --git a/drivers/mtd/redboot.c b/drivers/mtd/redboot.c
index 11c3447eb8ff..7623ac5fc586 100644
--- a/drivers/mtd/redboot.c
+++ b/drivers/mtd/redboot.c
@@ -57,7 +57,7 @@ static inline int redboot_checksum(struct fis_image_desc *img)
 }
 
 static int parse_redboot_partitions(struct mtd_info *master,
-				    struct mtd_partition **pparts,
+				    const struct mtd_partition **pparts,
 				    struct mtd_part_parser_data *data)
 {
 	int nrparts = 0;
diff --git a/include/linux/mtd/partitions.h b/include/linux/mtd/partitions.h
index d002d9b5d797..6185536daacc 100644
--- a/include/linux/mtd/partitions.h
+++ b/include/linux/mtd/partitions.h
@@ -69,7 +69,7 @@ struct mtd_part_parser {
 	struct list_head list;
 	struct module *owner;
 	const char *name;
-	int (*parse_fn)(struct mtd_info *, struct mtd_partition **,
+	int (*parse_fn)(struct mtd_info *, const struct mtd_partition **,
 			struct mtd_part_parser_data *);
 };
 
-- 
2.6.0.rc2.230.g3dd15c0

[PATCH v2 3/6] mtd: partitions: rename MTD parser get/put

[Brian Norris]

We're going to reuse put_partition_parser(), so let's fix up the prefix
naming a bit, to hopefully be more consistent. Also make convert to a
true C function instead of a macro.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
v2: keep mtd_part_parser_put() in the *.c file instead of the header, since
    we'll only use it in a wrapper now instead of exporting it directly

 drivers/mtd/mtdpart.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index 1fa3ca95d9c1..c6fd4b24c822 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,7 +701,10 @@ 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)
+static inline void mtd_part_parser_put(const struct mtd_part_parser *p)
+{
+	module_put(p->owner);
+}
 
 int __register_mtd_parser(struct mtd_part_parser *p, struct module *owner)
 {
@@ -765,9 +768,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 +778,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

[PATCH v2 4/6] mtd: partitions: remove kmemdup()

[Brian Norris]

The use of kmemdup() complicates the error handling a bit. We don't
actually need to allocate new memory, since this reference is treated as
const, and it is copied into new memory by the partition registration
code anyway. So remove it.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
New in v2

 drivers/mtd/mtdcore.c | 16 +++++++---------
 drivers/mtd/mtdcore.h |  2 +-
 drivers/mtd/mtdpart.c |  2 +-
 3 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 62f83b050978..868ee52d5063 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -532,7 +532,7 @@ out_error:
 }
 
 static int mtd_add_device_partitions(struct mtd_info *mtd,
-				     struct mtd_partition *real_parts,
+				     const struct mtd_partition *real_parts,
 				     int nbparts)
 {
 	int ret;
@@ -589,16 +589,12 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
 			      int nr_parts)
 {
 	int ret;
-	struct mtd_partition *real_parts = NULL;
+	const struct mtd_partition *real_parts = NULL;
 
 	ret = parse_mtd_partitions(mtd, types, &real_parts, parser_data);
 	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;
+		real_parts = parts;
+		ret = nr_parts;
 	}
 	/* Didn't come up with either parsed OR fallback partitions */
 	if (ret < 0) {
@@ -628,7 +624,9 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
 	}
 
 out:
-	kfree(real_parts);
+	/* Cleanup any parsed partitions */
+	if (real_parts != parts)
+		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 7b0353399a10..537ec66f9cfd 100644
--- a/drivers/mtd/mtdcore.h
+++ b/drivers/mtd/mtdcore.h
@@ -11,7 +11,7 @@ int del_mtd_device(struct mtd_info *mtd);
 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,
+			 const struct mtd_partition **pparts,
 			 struct mtd_part_parser_data *data);
 
 int __init init_mtdchar(void);
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index c6fd4b24c822..86691a5c68b9 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -757,7 +757,7 @@ static const char * const default_mtd_part_types[] = {
  *   point to an array containing this number of &struct mtd_info objects.
  */
 int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
-			 struct mtd_partition **pparts,
+			 const struct mtd_partition **pparts,
 			 struct mtd_part_parser_data *data)
 {
 	struct mtd_part_parser *parser;
-- 
2.6.0.rc2.230.g3dd15c0

[PATCH v2 5/6] mtd: partitions: pass around 'mtd_partitions' wrapper struct

[Brian Norris]

For some of the core partitioning code, it helps to keep info about the
parsed partition (and who parsed them) together in one place.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
New in v2

 drivers/mtd/mtdcore.c          | 33 +++++++++++++++++++--------------
 drivers/mtd/mtdcore.h          |  5 ++++-
 drivers/mtd/mtdpart.c          | 15 ++++++++-------
 include/linux/mtd/partitions.h |  7 +++++++
 4 files changed, 38 insertions(+), 22 deletions(-)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 868ee52d5063..20b2b38247b6 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -532,9 +532,10 @@ out_error:
 }
 
 static int mtd_add_device_partitions(struct mtd_info *mtd,
-				     const struct mtd_partition *real_parts,
-				     int nbparts)
+				     struct mtd_partitions *parts)
 {
+	const struct mtd_partition *real_parts = parts->parts;
+	int nbparts = parts->nr_parts;
 	int ret;
 
 	if (nbparts == 0 || IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) {
@@ -588,23 +589,27 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
 			      const struct mtd_partition *parts,
 			      int nr_parts)
 {
+	struct mtd_partitions parsed;
 	int ret;
-	const struct mtd_partition *real_parts = NULL;
 
-	ret = parse_mtd_partitions(mtd, types, &real_parts, parser_data);
-	if (ret <= 0 && nr_parts && parts) {
-		real_parts = parts;
-		ret = nr_parts;
-	}
-	/* Didn't come up with either parsed OR fallback partitions */
-	if (ret < 0) {
+	memset(&parsed, 0, sizeof(parsed));
+
+	ret = parse_mtd_partitions(mtd, types, &parsed, parser_data);
+	if ((ret < 0 || parsed.nr_parts == 0) && parts && nr_parts) {
+		/* Fall back to driver-provided partitions */
+		parsed = (struct mtd_partitions){
+			.parts		= parts,
+			.nr_parts	= nr_parts,
+		};
+	} else if (ret < 0) {
+		/* Didn't come up with parsed OR fallback partitions */
 		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;
+		memset(&parsed, 0, sizeof(parsed));
 	}
 
-	ret = mtd_add_device_partitions(mtd, real_parts, ret);
+	ret = mtd_add_device_partitions(mtd, &parsed);
 	if (ret)
 		goto out;
 
@@ -625,8 +630,8 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
 
 out:
 	/* Cleanup any parsed partitions */
-	if (real_parts != parts)
-		kfree(real_parts);
+	if (parsed.parser)
+		kfree(parsed.parts);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(mtd_device_parse_register);
diff --git a/drivers/mtd/mtdcore.h b/drivers/mtd/mtdcore.h
index 537ec66f9cfd..ce81cc2002f4 100644
--- a/drivers/mtd/mtdcore.h
+++ b/drivers/mtd/mtdcore.h
@@ -10,8 +10,11 @@ int add_mtd_device(struct mtd_info *mtd);
 int del_mtd_device(struct mtd_info *mtd);
 int add_mtd_partitions(struct mtd_info *, const struct mtd_partition *, int);
 int del_mtd_partitions(struct mtd_info *);
+
+struct mtd_partitions;
+
 int parse_mtd_partitions(struct mtd_info *master, const char * const *types,
-			 const struct mtd_partition **pparts,
+			 struct mtd_partitions *pparts,
 			 struct mtd_part_parser_data *data);
 
 int __init init_mtdchar(void);
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index 86691a5c68b9..c5108e0efe88 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -740,7 +740,7 @@ static const char * const default_mtd_part_types[] = {
  * parse_mtd_partitions - parse MTD partitions
  * @master: the master partition (describes whole MTD device)
  * @types: names of partition parsers to try or %NULL
- * @pparts: array of partitions found is returned here
+ * @pparts: info about partitions found is returned here
  * @data: MTD partition parser-specific data
  *
  * This function tries to find partition on MTD device @master. It uses MTD
@@ -752,12 +752,11 @@ static const char * const default_mtd_part_types[] = {
  *
  * This function may return:
  * 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.
+ * o zero otherwise, and @pparts will describe the partitions, number of
+ *   partitions, and the parser which parsed them
  */
 int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
-			 const struct mtd_partition **pparts,
+			 struct mtd_partitions *pparts,
 			 struct mtd_part_parser_data *data)
 {
 	struct mtd_part_parser *parser;
@@ -775,14 +774,16 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
 			 parser ? parser->name : NULL);
 		if (!parser)
 			continue;
-		ret = (*parser->parse_fn)(master, pparts, data);
+		ret = (*parser->parse_fn)(master, &pparts->parts, 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);
-			return ret;
+			pparts->nr_parts = ret;
+			pparts->parser = parser;
+			return 0;
 		}
 		/*
 		 * Stash the first error we see; only report it if no parser
diff --git a/include/linux/mtd/partitions.h b/include/linux/mtd/partitions.h
index 6185536daacc..cceaf7bd1537 100644
--- a/include/linux/mtd/partitions.h
+++ b/include/linux/mtd/partitions.h
@@ -73,6 +73,13 @@ struct mtd_part_parser {
 			struct mtd_part_parser_data *);
 };
 
+/* Container for passing around a set of parsed partitions */
+struct mtd_partitions {
+	const struct mtd_partition *parts;
+	int nr_parts;
+	const struct mtd_part_parser *parser;
+};
+
 extern int __register_mtd_parser(struct mtd_part_parser *parser,
 				 struct module *owner);
 #define register_mtd_parser(parser) __register_mtd_parser(parser, THIS_MODULE)
-- 
2.6.0.rc2.230.g3dd15c0

[PATCH v2 6/6] mtd: partitions: support a cleanup callback for 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().

Alongside this, define a default callback that we'll automatically use
if the parser doesn't provide one, so we can still retain the old
behavior.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
v2:
 * put more common logic in mtd_part_parser_cleanup()
 * provide default cleanup routine (i.e., kfree()) for parsers

 drivers/mtd/mtdcore.c          |  2 +-
 drivers/mtd/mtdcore.h          |  2 ++
 drivers/mtd/mtdpart.c          | 35 +++++++++++++++++++++++++++++++++--
 include/linux/mtd/partitions.h |  1 +
 4 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 20b2b38247b6..466cb0da7c08 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -631,7 +631,7 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
 out:
 	/* Cleanup any parsed partitions */
 	if (parsed.parser)
-		kfree(parsed.parts);
+		mtd_part_parser_cleanup(&parsed);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(mtd_device_parse_register);
diff --git a/drivers/mtd/mtdcore.h b/drivers/mtd/mtdcore.h
index ce81cc2002f4..55fdb8e1fd2a 100644
--- a/drivers/mtd/mtdcore.h
+++ b/drivers/mtd/mtdcore.h
@@ -17,6 +17,8 @@ int parse_mtd_partitions(struct mtd_info *master, const char * const *types,
 			 struct mtd_partitions *pparts,
 			 struct mtd_part_parser_data *data);
 
+void mtd_part_parser_cleanup(struct mtd_partitions *parts);
+
 int __init init_mtdchar(void);
 void __exit cleanup_mtdchar(void);
 
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index c5108e0efe88..7ab6b313add4 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -706,10 +706,23 @@ static inline void mtd_part_parser_put(const struct mtd_part_parser *p)
 	module_put(p->owner);
 }
 
+/*
+ * Many partition parsers just expected the core to kfree() all their data in
+ * one chunk. Do that by default.
+ */
+static void mtd_part_parser_cleanup_default(const struct mtd_partition *pparts,
+					    int nr_parts)
+{
+	kfree(pparts);
+}
+
 int __register_mtd_parser(struct mtd_part_parser *p, struct module *owner)
 {
 	p->owner = owner;
 
+	if (!p->cleanup)
+		p->cleanup = &mtd_part_parser_cleanup_default;
+
 	spin_lock(&part_parser_lock);
 	list_add(&p->list, &part_parsers);
 	spin_unlock(&part_parser_lock);
@@ -753,7 +766,9 @@ static const char * const default_mtd_part_types[] = {
  * This function may return:
  * o a negative error code in case of failure
  * o zero otherwise, and @pparts will describe the partitions, number of
- *   partitions, and the parser which parsed them
+ *   partitions, and the parser which parsed them. Caller must release
+ *   resources with mtd_part_parser_cleanup() when finished with the returned
+ *   data.
  */
 int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
 			 struct mtd_partitions *pparts,
@@ -777,7 +792,6 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
 		ret = (*parser->parse_fn)(master, &pparts->parts, 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);
@@ -785,6 +799,7 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
 			pparts->parser = parser;
 			return 0;
 		}
+		mtd_part_parser_put(parser);
 		/*
 		 * Stash the first error we see; only report it if no parser
 		 * succeeds
@@ -795,6 +810,22 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
 	return err;
 }
 
+void mtd_part_parser_cleanup(struct mtd_partitions *parts)
+{
+	const struct mtd_part_parser *parser;
+
+	if (!parts)
+		return;
+
+	parser = parts->parser;
+	if (parser) {
+		if (parser->cleanup)
+			parser->cleanup(parts->parts, parts->nr_parts);
+
+		mtd_part_parser_put(parser);
+	}
+}
+
 int mtd_is_partition(const struct mtd_info *mtd)
 {
 	struct mtd_part *part;
diff --git a/include/linux/mtd/partitions.h b/include/linux/mtd/partitions.h
index cceaf7bd1537..70736e1e6c8f 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 *, const struct mtd_partition **,
 			struct mtd_part_parser_data *);
+	void (*cleanup)(const struct mtd_partition *pparts, int nr_parts);
 };
 
 /* Container for passing around a set of parsed partitions */
-- 
2.6.0.rc2.230.g3dd15c0

Re: [PATCH v2 1/6] mtd: ofpart: assign return argument exactly once

[Boris Brezillon]

On Fri,  4 Dec 2015 15:25:13 -0800
Brian Norris <computersforpeace@gmail.com> wrote:

> It's easier to refactor these parsers if the return value gets assigned
> only once, just like every other MTD partition parser.
> 
> This prepares for making the second arg to the parse_fn() const. This is
> OK if we construct the partitions completely first, and assign them to
> the return pointer only after we're done modifying them.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>

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

> ---
> New in v2
> 
>  drivers/mtd/ofpart.c | 35 +++++++++++++++++++----------------
>  1 file changed, 19 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/mtd/ofpart.c b/drivers/mtd/ofpart.c
> index 478538100ddd..4800fecaf8cf 100644
> --- a/drivers/mtd/ofpart.c
> +++ b/drivers/mtd/ofpart.c
> @@ -29,6 +29,7 @@ static int parse_ofpart_partitions(struct mtd_info *master,
>  				   struct mtd_partition **pparts,
>  				   struct mtd_part_parser_data *data)
>  {
> +	struct mtd_partition *parts;
>  	struct device_node *mtd_node;
>  	struct device_node *ofpart_node;
>  	const char *partname;
> @@ -62,8 +63,8 @@ static int parse_ofpart_partitions(struct mtd_info *master,
>  	if (nr_parts == 0)
>  		return 0;
>  
> -	*pparts = kzalloc(nr_parts * sizeof(**pparts), GFP_KERNEL);
> -	if (!*pparts)
> +	parts = kzalloc(nr_parts * sizeof(*parts), GFP_KERNEL);
> +	if (!parts)
>  		return -ENOMEM;
>  
>  	i = 0;
> @@ -97,19 +98,19 @@ static int parse_ofpart_partitions(struct mtd_info *master,
>  			goto ofpart_fail;
>  		}
>  
> -		(*pparts)[i].offset = of_read_number(reg, a_cells);
> -		(*pparts)[i].size = of_read_number(reg + a_cells, s_cells);
> +		parts[i].offset = of_read_number(reg, a_cells);
> +		parts[i].size = of_read_number(reg + a_cells, s_cells);
>  
>  		partname = of_get_property(pp, "label", &len);
>  		if (!partname)
>  			partname = of_get_property(pp, "name", &len);
> -		(*pparts)[i].name = partname;
> +		parts[i].name = partname;
>  
>  		if (of_get_property(pp, "read-only", &len))
> -			(*pparts)[i].mask_flags |= MTD_WRITEABLE;
> +			parts[i].mask_flags |= MTD_WRITEABLE;
>  
>  		if (of_get_property(pp, "lock", &len))
> -			(*pparts)[i].mask_flags |= MTD_POWERUP_LOCK;
> +			parts[i].mask_flags |= MTD_POWERUP_LOCK;
>  
>  		i++;
>  	}
> @@ -117,6 +118,7 @@ static int parse_ofpart_partitions(struct mtd_info *master,
>  	if (!nr_parts)
>  		goto ofpart_none;
>  
> +	*pparts = parts;
>  	return nr_parts;
>  
>  ofpart_fail:
> @@ -125,8 +127,7 @@ ofpart_fail:
>  	ret = -EINVAL;
>  ofpart_none:
>  	of_node_put(pp);
> -	kfree(*pparts);
> -	*pparts = NULL;
> +	kfree(parts);
>  	return ret;
>  }
>  
> @@ -139,6 +140,7 @@ static int parse_ofoldpart_partitions(struct mtd_info *master,
>  				      struct mtd_partition **pparts,
>  				      struct mtd_part_parser_data *data)
>  {
> +	struct mtd_partition *parts;
>  	struct device_node *dp;
>  	int i, plen, nr_parts;
>  	const struct {
> @@ -160,32 +162,33 @@ static int parse_ofoldpart_partitions(struct mtd_info *master,
>  
>  	nr_parts = plen / sizeof(part[0]);
>  
> -	*pparts = kzalloc(nr_parts * sizeof(*(*pparts)), GFP_KERNEL);
> -	if (!*pparts)
> +	parts = kzalloc(nr_parts * sizeof(*parts), GFP_KERNEL);
> +	if (!parts)
>  		return -ENOMEM;
>  
>  	names = of_get_property(dp, "partition-names", &plen);
>  
>  	for (i = 0; i < nr_parts; i++) {
> -		(*pparts)[i].offset = be32_to_cpu(part->offset);
> -		(*pparts)[i].size   = be32_to_cpu(part->len) & ~1;
> +		parts[i].offset = be32_to_cpu(part->offset);
> +		parts[i].size   = be32_to_cpu(part->len) & ~1;
>  		/* bit 0 set signifies read only partition */
>  		if (be32_to_cpu(part->len) & 1)
> -			(*pparts)[i].mask_flags = MTD_WRITEABLE;
> +			parts[i].mask_flags = MTD_WRITEABLE;
>  
>  		if (names && (plen > 0)) {
>  			int len = strlen(names) + 1;
>  
> -			(*pparts)[i].name = names;
> +			parts[i].name = names;
>  			plen -= len;
>  			names += len;
>  		} else {
> -			(*pparts)[i].name = "unnamed";
> +			parts[i].name = "unnamed";
>  		}
>  
>  		part++;
>  	}
>  
> +	*pparts = parts;
>  	return nr_parts;
>  }
>  



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

Re: [PATCH v2 2/6] mtd: partitions: make parsers return 'const' partition arrays

[Boris Brezillon]

On Fri,  4 Dec 2015 15:25:14 -0800
Brian Norris <computersforpeace@gmail.com> wrote:

> We only want to modify these arrays inside the parser "drivers", so the
> drivers should construct them however they like, then return them as
> immutable arrays.
> 
> This will make other refactorings easier.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>

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

> ---
> New in v2
> 
>  drivers/mtd/afs.c              | 2 +-
>  drivers/mtd/ar7part.c          | 2 +-
>  drivers/mtd/bcm47xxpart.c      | 2 +-
>  drivers/mtd/bcm63xxpart.c      | 2 +-
>  drivers/mtd/cmdlinepart.c      | 2 +-
>  drivers/mtd/ofpart.c           | 4 ++--
>  drivers/mtd/redboot.c          | 2 +-
>  include/linux/mtd/partitions.h | 2 +-
>  8 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/mtd/afs.c b/drivers/mtd/afs.c
> index e02dae3b739b..d61b7edfc938 100644
> --- a/drivers/mtd/afs.c
> +++ b/drivers/mtd/afs.c
> @@ -162,7 +162,7 @@ afs_read_iis_v1(struct mtd_info *mtd, struct image_info_v1 *iis, u_int ptr)
>  }
>  
>  static int parse_afs_partitions(struct mtd_info *mtd,
> -				struct mtd_partition **pparts,
> +				const struct mtd_partition **pparts,
>  				struct mtd_part_parser_data *data)
>  {
>  	struct mtd_partition *parts;
> diff --git a/drivers/mtd/ar7part.c b/drivers/mtd/ar7part.c
> index 9203b96fd789..90575deff0ae 100644
> --- a/drivers/mtd/ar7part.c
> +++ b/drivers/mtd/ar7part.c
> @@ -43,7 +43,7 @@ struct ar7_bin_rec {
>  };
>  
>  static int create_mtd_partitions(struct mtd_info *master,
> -				 struct mtd_partition **pparts,
> +				 const struct mtd_partition **pparts,
>  				 struct mtd_part_parser_data *data)
>  {
>  	struct ar7_bin_rec header;
> diff --git a/drivers/mtd/bcm47xxpart.c b/drivers/mtd/bcm47xxpart.c
> index 92a6dd18198b..8282f47bcf5d 100644
> --- a/drivers/mtd/bcm47xxpart.c
> +++ b/drivers/mtd/bcm47xxpart.c
> @@ -82,7 +82,7 @@ out_default:
>  }
>  
>  static int bcm47xxpart_parse(struct mtd_info *master,
> -			     struct mtd_partition **pparts,
> +			     const struct mtd_partition **pparts,
>  			     struct mtd_part_parser_data *data)
>  {
>  	struct mtd_partition *parts;
> diff --git a/drivers/mtd/bcm63xxpart.c b/drivers/mtd/bcm63xxpart.c
> index cf02135320bc..440936998593 100644
> --- a/drivers/mtd/bcm63xxpart.c
> +++ b/drivers/mtd/bcm63xxpart.c
> @@ -68,7 +68,7 @@ static int bcm63xx_detect_cfe(struct mtd_info *master)
>  }
>  
>  static int bcm63xx_parse_cfe_partitions(struct mtd_info *master,
> -					struct mtd_partition **pparts,
> +					const struct mtd_partition **pparts,
>  					struct mtd_part_parser_data *data)
>  {
>  	/* CFE, NVRAM and global Linux are always present */
> diff --git a/drivers/mtd/cmdlinepart.c b/drivers/mtd/cmdlinepart.c
> index 420489864bc2..fbd5affc0acf 100644
> --- a/drivers/mtd/cmdlinepart.c
> +++ b/drivers/mtd/cmdlinepart.c
> @@ -304,7 +304,7 @@ static int mtdpart_setup_real(char *s)
>   * the first one in the chain if a NULL mtd_id is passed in.
>   */
>  static int parse_cmdline_partitions(struct mtd_info *master,
> -				    struct mtd_partition **pparts,
> +				    const struct mtd_partition **pparts,
>  				    struct mtd_part_parser_data *data)
>  {
>  	unsigned long long offset;
> diff --git a/drivers/mtd/ofpart.c b/drivers/mtd/ofpart.c
> index 4800fecaf8cf..8a027a71e662 100644
> --- a/drivers/mtd/ofpart.c
> +++ b/drivers/mtd/ofpart.c
> @@ -26,7 +26,7 @@ static bool node_has_compatible(struct device_node *pp)
>  }
>  
>  static int parse_ofpart_partitions(struct mtd_info *master,
> -				   struct mtd_partition **pparts,
> +				   const struct mtd_partition **pparts,
>  				   struct mtd_part_parser_data *data)
>  {
>  	struct mtd_partition *parts;
> @@ -137,7 +137,7 @@ static struct mtd_part_parser ofpart_parser = {
>  };
>  
>  static int parse_ofoldpart_partitions(struct mtd_info *master,
> -				      struct mtd_partition **pparts,
> +				      const struct mtd_partition **pparts,
>  				      struct mtd_part_parser_data *data)
>  {
>  	struct mtd_partition *parts;
> diff --git a/drivers/mtd/redboot.c b/drivers/mtd/redboot.c
> index 11c3447eb8ff..7623ac5fc586 100644
> --- a/drivers/mtd/redboot.c
> +++ b/drivers/mtd/redboot.c
> @@ -57,7 +57,7 @@ static inline int redboot_checksum(struct fis_image_desc *img)
>  }
>  
>  static int parse_redboot_partitions(struct mtd_info *master,
> -				    struct mtd_partition **pparts,
> +				    const struct mtd_partition **pparts,
>  				    struct mtd_part_parser_data *data)
>  {
>  	int nrparts = 0;
> diff --git a/include/linux/mtd/partitions.h b/include/linux/mtd/partitions.h
> index d002d9b5d797..6185536daacc 100644
> --- a/include/linux/mtd/partitions.h
> +++ b/include/linux/mtd/partitions.h
> @@ -69,7 +69,7 @@ struct mtd_part_parser {
>  	struct list_head list;
>  	struct module *owner;
>  	const char *name;
> -	int (*parse_fn)(struct mtd_info *, struct mtd_partition **,
> +	int (*parse_fn)(struct mtd_info *, const struct mtd_partition **,
>  			struct mtd_part_parser_data *);
>  };
>  



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

Re: [PATCH v2 3/6] mtd: partitions: rename MTD parser get/put

[Boris Brezillon]

On Fri,  4 Dec 2015 15:25:15 -0800
Brian Norris <computersforpeace@gmail.com> wrote:

> We're going to reuse put_partition_parser(), so let's fix up the prefix
> naming a bit, to hopefully be more consistent. Also make convert to a
> true C function instead of a macro.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---
> v2: keep mtd_part_parser_put() in the *.c file instead of the header, since
>     we'll only use it in a wrapper now instead of exporting it directly

Don't know if you forgot my R-b tag or if you dropped it on purpose
because of this change, but it still applies here.

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

> 
>  drivers/mtd/mtdpart.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> index 1fa3ca95d9c1..c6fd4b24c822 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,7 +701,10 @@ 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)
> +static inline void mtd_part_parser_put(const struct mtd_part_parser *p)
> +{
> +	module_put(p->owner);
> +}
>  
>  int __register_mtd_parser(struct mtd_part_parser *p, struct module *owner)
>  {
> @@ -765,9 +768,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 +778,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

Re: [PATCH v2 4/6] mtd: partitions: remove kmemdup()

[Boris Brezillon]

On Fri,  4 Dec 2015 15:25:16 -0800
Brian Norris <computersforpeace@gmail.com> wrote:

> The use of kmemdup() complicates the error handling a bit. We don't
> actually need to allocate new memory, since this reference is treated as
> const, and it is copied into new memory by the partition registration
> code anyway. So remove it.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>

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

> ---
> New in v2
> 
>  drivers/mtd/mtdcore.c | 16 +++++++---------
>  drivers/mtd/mtdcore.h |  2 +-
>  drivers/mtd/mtdpart.c |  2 +-
>  3 files changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 62f83b050978..868ee52d5063 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -532,7 +532,7 @@ out_error:
>  }
>  
>  static int mtd_add_device_partitions(struct mtd_info *mtd,
> -				     struct mtd_partition *real_parts,
> +				     const struct mtd_partition *real_parts,
>  				     int nbparts)
>  {
>  	int ret;
> @@ -589,16 +589,12 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
>  			      int nr_parts)
>  {
>  	int ret;
> -	struct mtd_partition *real_parts = NULL;
> +	const struct mtd_partition *real_parts = NULL;
>  
>  	ret = parse_mtd_partitions(mtd, types, &real_parts, parser_data);
>  	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;
> +		real_parts = parts;
> +		ret = nr_parts;
>  	}
>  	/* Didn't come up with either parsed OR fallback partitions */
>  	if (ret < 0) {
> @@ -628,7 +624,9 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
>  	}
>  
>  out:
> -	kfree(real_parts);
> +	/* Cleanup any parsed partitions */
> +	if (real_parts != parts)
> +		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 7b0353399a10..537ec66f9cfd 100644
> --- a/drivers/mtd/mtdcore.h
> +++ b/drivers/mtd/mtdcore.h
> @@ -11,7 +11,7 @@ int del_mtd_device(struct mtd_info *mtd);
>  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,
> +			 const struct mtd_partition **pparts,
>  			 struct mtd_part_parser_data *data);
>  
>  int __init init_mtdchar(void);
> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> index c6fd4b24c822..86691a5c68b9 100644
> --- a/drivers/mtd/mtdpart.c
> +++ b/drivers/mtd/mtdpart.c
> @@ -757,7 +757,7 @@ static const char * const default_mtd_part_types[] = {
>   *   point to an array containing this number of &struct mtd_info objects.
>   */
>  int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
> -			 struct mtd_partition **pparts,
> +			 const struct mtd_partition **pparts,
>  			 struct mtd_part_parser_data *data)
>  {
>  	struct mtd_part_parser *parser;



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

Re: [PATCH v2 3/6] mtd: partitions: rename MTD parser get/put

[Brian Norris]

On Sat, Dec 05, 2015 at 01:00:02AM +0100, Boris Brezillon wrote:
> On Fri,  4 Dec 2015 15:25:15 -0800
> Brian Norris <computersforpeace@gmail.com> wrote:
> > We're going to reuse put_partition_parser(), so let's fix up the prefix
> > naming a bit, to hopefully be more consistent. Also make convert to a
> > true C function instead of a macro.
> > 
> > Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> > ---
> > v2: keep mtd_part_parser_put() in the *.c file instead of the header, since
> >     we'll only use it in a wrapper now instead of exporting it directly
> 
> Don't know if you forgot my R-b tag or if you dropped it on purpose
> because of this change, but it still applies here.

Was intentional, for that reason.

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

Thanks!

Re: [PATCH v2 5/6] mtd: partitions: pass around 'mtd_partitions' wrapper struct

[Boris Brezillon]

Hi Brian,

On Fri,  4 Dec 2015 15:25:17 -0800
Brian Norris <computersforpeace@gmail.com> wrote:

> For some of the core partitioning code, it helps to keep info about the
> parsed partition (and who parsed them) together in one place.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---
> New in v2
> 
>  drivers/mtd/mtdcore.c          | 33 +++++++++++++++++++--------------
>  drivers/mtd/mtdcore.h          |  5 ++++-
>  drivers/mtd/mtdpart.c          | 15 ++++++++-------
>  include/linux/mtd/partitions.h |  7 +++++++
>  4 files changed, 38 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 868ee52d5063..20b2b38247b6 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -532,9 +532,10 @@ out_error:
>  }
>  
>  static int mtd_add_device_partitions(struct mtd_info *mtd,
> -				     const struct mtd_partition *real_parts,
> -				     int nbparts)
> +				     struct mtd_partitions *parts)
>  {
> +	const struct mtd_partition *real_parts = parts->parts;
> +	int nbparts = parts->nr_parts;
>  	int ret;
>  
>  	if (nbparts == 0 || IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) {
> @@ -588,23 +589,27 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
>  			      const struct mtd_partition *parts,
>  			      int nr_parts)
>  {
> +	struct mtd_partitions parsed;
>  	int ret;
> -	const struct mtd_partition *real_parts = NULL;
>  
> -	ret = parse_mtd_partitions(mtd, types, &real_parts, parser_data);
> -	if (ret <= 0 && nr_parts && parts) {
> -		real_parts = parts;
> -		ret = nr_parts;
> -	}
> -	/* Didn't come up with either parsed OR fallback partitions */
> -	if (ret < 0) {
> +	memset(&parsed, 0, sizeof(parsed));
> +
> +	ret = parse_mtd_partitions(mtd, types, &parsed, parser_data);
> +	if ((ret < 0 || parsed.nr_parts == 0) && parts && nr_parts) {
> +		/* Fall back to driver-provided partitions */
> +		parsed = (struct mtd_partitions){
> +			.parts		= parts,
> +			.nr_parts	= nr_parts,
> +		};
> +	} else if (ret < 0) {
> +		/* Didn't come up with parsed OR fallback partitions */
>  		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;
> +		memset(&parsed, 0, sizeof(parsed));
>  	}
>  
> -	ret = mtd_add_device_partitions(mtd, real_parts, ret);
> +	ret = mtd_add_device_partitions(mtd, &parsed);
>  	if (ret)
>  		goto out;
>  
> @@ -625,8 +630,8 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
>  
>  out:
>  	/* Cleanup any parsed partitions */
> -	if (real_parts != parts)
> -		kfree(real_parts);
> +	if (parsed.parser)
> +		kfree(parsed.parts);
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(mtd_device_parse_register);

How about defining a new function to encourage mtd drivers to pass an
mtd_partitions structure instead of the parts + nr_parts arguments.
Note that I don't ask to update all call sites, but only to add a new
function and transform mtd_device_parse_register() into a wrapper.

int mtd_device_parse_and_register_parts(struct mtd_info *mtd,
					const char *const *types,
					const struct mtd_partitions *parts)
{
	struct mtd_partitions parsed = { };
	int ret;

	ret = parse_mtd_partitions(mtd, types, &parsed, parser_data);
	if (!ret)
		parts = &parsed;

	if (!parts || !parts->nr_parts) {
		/* Didn't come up with parsed OR fallback partitions */
  		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 = mtd_add_device_partitions(mtd, &parsed);
  	if (ret)
  		goto out;

[...]
out:
  	/* Cleanup any parsed partitions */
	if (parts->parser)
		kfree(parts->parts);
 	return ret;
}
EXPORT_SYMBOL_GPL(mtd_device_parse_and_register_parts);

int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
  			      const struct mtd_partition *parts,
  			      int nr_parts)
{
	struct mtd_partitions predef = {
		.parts = parts,
		.nr_parts = nr_parts,
	};

	return mtd_device_parse_and_register_parts(mtd, type, &predef);
}
EXPORT_SYMBOL_GPL(mtd_device_parse_and_register_parts);

> diff --git a/drivers/mtd/mtdcore.h b/drivers/mtd/mtdcore.h
> index 537ec66f9cfd..ce81cc2002f4 100644
> --- a/drivers/mtd/mtdcore.h
> +++ b/drivers/mtd/mtdcore.h
> @@ -10,8 +10,11 @@ int add_mtd_device(struct mtd_info *mtd);
>  int del_mtd_device(struct mtd_info *mtd);
>  int add_mtd_partitions(struct mtd_info *, const struct mtd_partition *, int);
>  int del_mtd_partitions(struct mtd_info *);
> +
> +struct mtd_partitions;
> +
>  int parse_mtd_partitions(struct mtd_info *master, const char * const *types,
> -			 const struct mtd_partition **pparts,
> +			 struct mtd_partitions *pparts,
>  			 struct mtd_part_parser_data *data);
>  
>  int __init init_mtdchar(void);
> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> index 86691a5c68b9..c5108e0efe88 100644
> --- a/drivers/mtd/mtdpart.c
> +++ b/drivers/mtd/mtdpart.c
> @@ -740,7 +740,7 @@ static const char * const default_mtd_part_types[] = {
>   * parse_mtd_partitions - parse MTD partitions
>   * @master: the master partition (describes whole MTD device)
>   * @types: names of partition parsers to try or %NULL
> - * @pparts: array of partitions found is returned here
> + * @pparts: info about partitions found is returned here
>   * @data: MTD partition parser-specific data
>   *
>   * This function tries to find partition on MTD device @master. It uses MTD
> @@ -752,12 +752,11 @@ static const char * const default_mtd_part_types[] = {
>   *
>   * This function may return:
>   * 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.
> + * o zero otherwise, and @pparts will describe the partitions, number of
> + *   partitions, and the parser which parsed them
>   */
>  int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
> -			 const struct mtd_partition **pparts,
> +			 struct mtd_partitions *pparts,
>  			 struct mtd_part_parser_data *data)
>  {
>  	struct mtd_part_parser *parser;
> @@ -775,14 +774,16 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
>  			 parser ? parser->name : NULL);
>  		if (!parser)
>  			continue;
> -		ret = (*parser->parse_fn)(master, pparts, data);
> +		ret = (*parser->parse_fn)(master, &pparts->parts, data);

Hm, you updated the ->parse_fn() prototype to take a const mtd_partition **
in patch 2, and it seemed pretty easy.
How about updating it again to take an mtd_partitions pointer here?

Best Regards,

Boris

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

Re: [PATCH v2 6/6] mtd: partitions: support a cleanup callback for parsers

[Boris Brezillon]

On Fri,  4 Dec 2015 15:25:18 -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().
> 
> Alongside this, define a default callback that we'll automatically use
> if the parser doesn't provide one, so we can still retain the old
> behavior.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---
> v2:
>  * put more common logic in mtd_part_parser_cleanup()
>  * provide default cleanup routine (i.e., kfree()) for parsers
> 
>  drivers/mtd/mtdcore.c          |  2 +-
>  drivers/mtd/mtdcore.h          |  2 ++
>  drivers/mtd/mtdpart.c          | 35 +++++++++++++++++++++++++++++++++--
>  include/linux/mtd/partitions.h |  1 +
>  4 files changed, 37 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 20b2b38247b6..466cb0da7c08 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -631,7 +631,7 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
>  out:
>  	/* Cleanup any parsed partitions */
>  	if (parsed.parser)
> -		kfree(parsed.parts);
> +		mtd_part_parser_cleanup(&parsed);

You can drop the if (parsed.parser) condition: it is already checked in
mtd_part_parser_cleanup().

>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(mtd_device_parse_register);
> diff --git a/drivers/mtd/mtdcore.h b/drivers/mtd/mtdcore.h
> index ce81cc2002f4..55fdb8e1fd2a 100644
> --- a/drivers/mtd/mtdcore.h
> +++ b/drivers/mtd/mtdcore.h
> @@ -17,6 +17,8 @@ int parse_mtd_partitions(struct mtd_info *master, const char * const *types,
>  			 struct mtd_partitions *pparts,
>  			 struct mtd_part_parser_data *data);
>  
> +void mtd_part_parser_cleanup(struct mtd_partitions *parts);
> +
>  int __init init_mtdchar(void);
>  void __exit cleanup_mtdchar(void);
>  
> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> index c5108e0efe88..7ab6b313add4 100644
> --- a/drivers/mtd/mtdpart.c
> +++ b/drivers/mtd/mtdpart.c
> @@ -706,10 +706,23 @@ static inline void mtd_part_parser_put(const struct mtd_part_parser *p)
>  	module_put(p->owner);
>  }
>  
> +/*
> + * Many partition parsers just expected the core to kfree() all their data in
> + * one chunk. Do that by default.
> + */
> +static void mtd_part_parser_cleanup_default(const struct mtd_partition *pparts,
> +					    int nr_parts)
> +{
> +	kfree(pparts);
> +}
> +
>  int __register_mtd_parser(struct mtd_part_parser *p, struct module *owner)
>  {
>  	p->owner = owner;
>  
> +	if (!p->cleanup)
> +		p->cleanup = &mtd_part_parser_cleanup_default;
> +
>  	spin_lock(&part_parser_lock);
>  	list_add(&p->list, &part_parsers);
>  	spin_unlock(&part_parser_lock);
> @@ -753,7 +766,9 @@ static const char * const default_mtd_part_types[] = {
>   * This function may return:
>   * o a negative error code in case of failure
>   * o zero otherwise, and @pparts will describe the partitions, number of
> - *   partitions, and the parser which parsed them
> + *   partitions, and the parser which parsed them. Caller must release
> + *   resources with mtd_part_parser_cleanup() when finished with the returned
> + *   data.
>   */
>  int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
>  			 struct mtd_partitions *pparts,
> @@ -777,7 +792,6 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
>  		ret = (*parser->parse_fn)(master, &pparts->parts, 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);
> @@ -785,6 +799,7 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
>  			pparts->parser = parser;
>  			return 0;
>  		}
> +		mtd_part_parser_put(parser);
>  		/*
>  		 * Stash the first error we see; only report it if no parser
>  		 * succeeds
> @@ -795,6 +810,22 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
>  	return err;
>  }
>  
> +void mtd_part_parser_cleanup(struct mtd_partitions *parts)
> +{
> +	const struct mtd_part_parser *parser;
> +
> +	if (!parts)
> +		return;
> +
> +	parser = parts->parser;
> +	if (parser) {
> +		if (parser->cleanup)
> +			parser->cleanup(parts->parts, parts->nr_parts);
> +
> +		mtd_part_parser_put(parser);
> +	}
> +}
> +
>  int mtd_is_partition(const struct mtd_info *mtd)
>  {
>  	struct mtd_part *part;
> diff --git a/include/linux/mtd/partitions.h b/include/linux/mtd/partitions.h
> index cceaf7bd1537..70736e1e6c8f 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 *, const struct mtd_partition **,
>  			struct mtd_part_parser_data *);
> +	void (*cleanup)(const struct mtd_partition *pparts, int nr_parts);

As said in my previous answer, I think it would be more consistent to
have the ->parse_fn() and ->cleanup() methods take an mtd_partitions
pointer.

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

Re: [PATCH v2 5/6] mtd: partitions: pass around 'mtd_partitions' wrapper struct

[Boris Brezillon]

On Sat, 5 Dec 2015 01:30:49 +0100
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> How about defining a new function to encourage mtd drivers to pass an
> mtd_partitions structure instead of the parts + nr_parts arguments.
> Note that I don't ask to update all call sites, but only to add a new
> function and transform mtd_device_parse_register() into a wrapper.
> 
> int mtd_device_parse_and_register_parts(struct mtd_info *mtd,
> 					const char *const *types,
> 					const struct mtd_partitions *parts)
> {
> 	struct mtd_partitions parsed = { };
> 	int ret;
> 
> 	ret = parse_mtd_partitions(mtd, types, &parsed, parser_data);
> 	if (!ret)

Should be

	if (!ret && parsed.nr_parts > 0)
> 		parts = &parsed;
> 
> 	if (!parts || !parts->nr_parts) {
> 		/* Didn't come up with parsed OR fallback partitions */
>   		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 = mtd_add_device_partitions(mtd, &parsed);
>   	if (ret)
>   		goto out;

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

Re: [PATCH v2 5/6] mtd: partitions: pass around 'mtd_partitions' wrapper struct

[Brian Norris]

Hi Boris,

On Sat, Dec 05, 2015 at 01:30:49AM +0100, Boris Brezillon wrote:
> On Fri,  4 Dec 2015 15:25:17 -0800
> Brian Norris <computersforpeace@gmail.com> wrote:


> How about defining a new function to encourage mtd drivers to pass an
> mtd_partitions structure instead of the parts + nr_parts arguments.

Hmm, I'm not sure about having drivers use 'mtd_partitions' yet. It's a
little awkward for them to have access to a 'parser' field there, since
they might think of it as an input argument. But I do like the wrapper
function, as it simplifies the core function a bit. I'll try that, and
if we decide it's good to have drivers also start using something more
like the non-wrapped version, then maybe we can export it later?

I'm also slightly hesitant, because I think there may be more things
we'd want to refactor on how drivers pass partition info to the MTD core
in the future. Ideally, they'd have to pass less, but for the bits we
can't kill, it might be good to stick all into a single struct. (For
one, why should the mtd_part_parser_data be separate too?) So in the
end, we might want a different struct for MTD drivers' use, and I'd like
not to touch all that right now, if that's OK with you.

(BTW, I have some RFC of_match_table stuff that I'm preparing to send. I
might just tack that onto the end of this series' v3.)

> Note that I don't ask to update all call sites, but only to add a new
> function and transform mtd_device_parse_register() into a wrapper.
> 
> int mtd_device_parse_and_register_parts(struct mtd_info *mtd,
> 					const char *const *types,
> 					const struct mtd_partitions *parts)

You've missed parser_data in the prototype?

> {
> 	struct mtd_partitions parsed = { };
> 	int ret;
> 
> 	ret = parse_mtd_partitions(mtd, types, &parsed, parser_data);
> 	if (!ret)
> 		parts = &parsed;
> 
> 	if (!parts || !parts->nr_parts) {
> 		/* Didn't come up with parsed OR fallback partitions */
>   		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 = mtd_add_device_partitions(mtd, &parsed);

s/&parsed/parts/

right?

Anyway, enough nitpicks, I can fixup the implementation for a real patch :)

>   	if (ret)
>   		goto out;
> 
> [...]
> out:
>   	/* Cleanup any parsed partitions */
> 	if (parts->parser)
> 		kfree(parts->parts);
>  	return ret;
> }
> EXPORT_SYMBOL_GPL(mtd_device_parse_and_register_parts);
> 
> int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
>   			      const struct mtd_partition *parts,
>   			      int nr_parts)
> {
> 	struct mtd_partitions predef = {
> 		.parts = parts,
> 		.nr_parts = nr_parts,
> 	};
> 
> 	return mtd_device_parse_and_register_parts(mtd, type, &predef);
> }
> EXPORT_SYMBOL_GPL(mtd_device_parse_and_register_parts);
> 

> > diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> > index 86691a5c68b9..c5108e0efe88 100644
> > --- a/drivers/mtd/mtdpart.c
> > +++ b/drivers/mtd/mtdpart.c

> > @@ -775,14 +774,16 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
> >  			 parser ? parser->name : NULL);
> >  		if (!parser)
> >  			continue;
> > -		ret = (*parser->parse_fn)(master, pparts, data);
> > +		ret = (*parser->parse_fn)(master, &pparts->parts, data);
> 
> Hm, you updated the ->parse_fn() prototype to take a const mtd_partition **
> in patch 2, and it seemed pretty easy.
> How about updating it again to take an mtd_partitions pointer here?

I can do that, I guess. Same issue about the 'parser' field; as long as
parser drivers don't think think they need to fill this out, I guess
that's OK.

Brian

Re: [PATCH v2 5/6] mtd: partitions: pass around 'mtd_partitions' wrapper struct

[Brian Norris]

Hi Boris,

On Fri, Dec 04, 2015 at 05:45:11PM -0800, Brian Norris wrote:
> On Sat, Dec 05, 2015 at 01:30:49AM +0100, Boris Brezillon wrote:
> > How about defining a new function to encourage mtd drivers to pass an
> > mtd_partitions structure instead of the parts + nr_parts arguments.

...

> > Hm, you updated the ->parse_fn() prototype to take a const mtd_partition **
> > in patch 2, and it seemed pretty easy.
> > How about updating it again to take an mtd_partitions pointer here?

OK, so I've hacked around at these two suggestions, and I'm not very
happy with the results so far. I mentioned some of my objection to the
first above already, and when I tried to do some kind of compromise, it
doesn't end up much cleaner IMO.

For the second suggestion, I don't really like the circular dependency,
where we have struct mtd_part_parser include pointers to struct
mtd_partitions which includes pointers to mtd_part_parser. This is
partly because of the re-use of the parts of the same struct as both an
input and an output, depending on the context. I don't think that's very
clean.

So, if there are good ways to extend some version of your suggestions,
perhaps they can be tackled after this patch set?

Brian

Re: [PATCH v2 5/6] mtd: partitions: pass around 'mtd_partitions' wrapper struct

[Boris Brezillon]

On Fri, 4 Dec 2015 20:18:35 -0800
Brian Norris <computersforpeace@gmail.com> wrote:

> Hi Boris,
> 
> On Fri, Dec 04, 2015 at 05:45:11PM -0800, Brian Norris wrote:
> > On Sat, Dec 05, 2015 at 01:30:49AM +0100, Boris Brezillon wrote:
> > > How about defining a new function to encourage mtd drivers to pass an
> > > mtd_partitions structure instead of the parts + nr_parts arguments.
> 
> ...
> 
> > > Hm, you updated the ->parse_fn() prototype to take a const mtd_partition **
> > > in patch 2, and it seemed pretty easy.
> > > How about updating it again to take an mtd_partitions pointer here?
> 
> OK, so I've hacked around at these two suggestions, and I'm not very
> happy with the results so far. I mentioned some of my objection to the
> first above already, and when I tried to do some kind of compromise, it
> doesn't end up much cleaner IMO.
> 
> For the second suggestion, I don't really like the circular dependency,
> where we have struct mtd_part_parser include pointers to struct
> mtd_partitions which includes pointers to mtd_part_parser. This is
> partly because of the re-use of the parts of the same struct as both an
> input and an output, depending on the context. I don't think that's very
> clean.
> 
> So, if there are good ways to extend some version of your suggestions,
> perhaps they can be tackled after this patch set?

Okay.

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

Re: [PATCH v2 5/6] mtd: partitions: pass around 'mtd_partitions' wrapper struct

[Boris Brezillon]

Hi Brian,

On Fri, 4 Dec 2015 17:45:11 -0800
Brian Norris <computersforpeace@gmail.com> wrote:

> Hi Boris,
> 
> On Sat, Dec 05, 2015 at 01:30:49AM +0100, Boris Brezillon wrote:
> > On Fri,  4 Dec 2015 15:25:17 -0800
> > Brian Norris <computersforpeace@gmail.com> wrote:
> 
> 
> > How about defining a new function to encourage mtd drivers to pass an
> > mtd_partitions structure instead of the parts + nr_parts arguments.
> 
> Hmm, I'm not sure about having drivers use 'mtd_partitions' yet. It's a
> little awkward for them to have access to a 'parser' field there, since
> they might think of it as an input argument. But I do like the wrapper
> function, as it simplifies the core function a bit. I'll try that, and
> if we decide it's good to have drivers also start using something more
> like the non-wrapped version, then maybe we can export it later?
> 
> I'm also slightly hesitant, because I think there may be more things
> we'd want to refactor on how drivers pass partition info to the MTD core
> in the future. Ideally, they'd have to pass less, but for the bits we
> can't kill, it might be good to stick all into a single struct. (For
> one, why should the mtd_part_parser_data be separate too?) So in the
> end, we might want a different struct for MTD drivers' use, and I'd like
> not to touch all that right now, if that's OK with you.

Fair enough. 

> 
> (BTW, I have some RFC of_match_table stuff that I'm preparing to send. I
> might just tack that onto the end of this series' v3.)
> 
> > Note that I don't ask to update all call sites, but only to add a new
> > function and transform mtd_device_parse_register() into a wrapper.
> > 
> > int mtd_device_parse_and_register_parts(struct mtd_info *mtd,
> > 					const char *const *types,
> > 					const struct mtd_partitions *parts)
> 
> You've missed parser_data in the prototype?
> 
> > {
> > 	struct mtd_partitions parsed = { };
> > 	int ret;
> > 
> > 	ret = parse_mtd_partitions(mtd, types, &parsed, parser_data);
> > 	if (!ret)
> > 		parts = &parsed;
> > 
> > 	if (!parts || !parts->nr_parts) {
> > 		/* Didn't come up with parsed OR fallback partitions */
> >   		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 = mtd_add_device_partitions(mtd, &parsed);
> 
> s/&parsed/parts/
> 
> right?

Right, but you know how much I like to propose code that does not even
compile ;-).

Best Regards,

Bori

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

[PATCH v3 6/6] mtd: partitions: support a cleanup callback for 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().

Alongside this, define a default callback that we'll automatically use
if the parser doesn't provide one, so we can still retain the old
behavior.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
v3:
 * drop redundant 'if (parsed.parser)' condition before cleanup

v2:
 * put more common logic in mtd_part_parser_cleanup()
 * provide default cleanup routine (i.e., kfree()) for parsers

 drivers/mtd/mtdcore.c          |  3 +--
 drivers/mtd/mtdcore.h          |  2 ++
 drivers/mtd/mtdpart.c          | 35 +++++++++++++++++++++++++++++++++--
 include/linux/mtd/partitions.h |  1 +
 4 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 20b2b38247b6..89d811e7b04a 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -630,8 +630,7 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
 
 out:
 	/* Cleanup any parsed partitions */
-	if (parsed.parser)
-		kfree(parsed.parts);
+	mtd_part_parser_cleanup(&parsed);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(mtd_device_parse_register);
diff --git a/drivers/mtd/mtdcore.h b/drivers/mtd/mtdcore.h
index ce81cc2002f4..55fdb8e1fd2a 100644
--- a/drivers/mtd/mtdcore.h
+++ b/drivers/mtd/mtdcore.h
@@ -17,6 +17,8 @@ int parse_mtd_partitions(struct mtd_info *master, const char * const *types,
 			 struct mtd_partitions *pparts,
 			 struct mtd_part_parser_data *data);
 
+void mtd_part_parser_cleanup(struct mtd_partitions *parts);
+
 int __init init_mtdchar(void);
 void __exit cleanup_mtdchar(void);
 
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index 53517d7653cb..10bf304027dd 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -709,10 +709,23 @@ static inline void mtd_part_parser_put(const struct mtd_part_parser *p)
 	module_put(p->owner);
 }
 
+/*
+ * Many partition parsers just expected the core to kfree() all their data in
+ * one chunk. Do that by default.
+ */
+static void mtd_part_parser_cleanup_default(const struct mtd_partition *pparts,
+					    int nr_parts)
+{
+	kfree(pparts);
+}
+
 int __register_mtd_parser(struct mtd_part_parser *p, struct module *owner)
 {
 	p->owner = owner;
 
+	if (!p->cleanup)
+		p->cleanup = &mtd_part_parser_cleanup_default;
+
 	spin_lock(&part_parser_lock);
 	list_add(&p->list, &part_parsers);
 	spin_unlock(&part_parser_lock);
@@ -756,7 +769,9 @@ static const char * const default_mtd_part_types[] = {
  * This function may return:
  * o a negative error code in case of failure
  * o zero otherwise, and @pparts will describe the partitions, number of
- *   partitions, and the parser which parsed them
+ *   partitions, and the parser which parsed them. Caller must release
+ *   resources with mtd_part_parser_cleanup() when finished with the returned
+ *   data.
  */
 int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
 			 struct mtd_partitions *pparts,
@@ -780,7 +795,6 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
 		ret = (*parser->parse_fn)(master, &pparts->parts, 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);
@@ -788,6 +802,7 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
 			pparts->parser = parser;
 			return 0;
 		}
+		mtd_part_parser_put(parser);
 		/*
 		 * Stash the first error we see; only report it if no parser
 		 * succeeds
@@ -798,6 +813,22 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
 	return err;
 }
 
+void mtd_part_parser_cleanup(struct mtd_partitions *parts)
+{
+	const struct mtd_part_parser *parser;
+
+	if (!parts)
+		return;
+
+	parser = parts->parser;
+	if (parser) {
+		if (parser->cleanup)
+			parser->cleanup(parts->parts, parts->nr_parts);
+
+		mtd_part_parser_put(parser);
+	}
+}
+
 int mtd_is_partition(const struct mtd_info *mtd)
 {
 	struct mtd_part *part;
diff --git a/include/linux/mtd/partitions.h b/include/linux/mtd/partitions.h
index cceaf7bd1537..70736e1e6c8f 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 *, const struct mtd_partition **,
 			struct mtd_part_parser_data *);
+	void (*cleanup)(const struct mtd_partition *pparts, int nr_parts);
 };
 
 /* Container for passing around a set of parsed partitions */
-- 
2.6.0.rc2.230.g3dd15c0

Re: [PATCH v2 0/6] mtd: partitions: support cleanup callback for parsers

[Brian Norris]

On Fri, Dec 04, 2015 at 03:25:12PM -0800, Brian Norris wrote:
> Hi,
> 
> Here's v2, which should address the comments made on v1.
> 
> 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 6) supports a cleanup() callback for parsers.
> 
> v1 -> v2:
>  * add "mtd_partitions" struct to hold info about the array of parsed partitions
>  * const-ify some arguments, which requires some small refactoring of the
>    existing partitions parsers
>  * remove the kmemdup() in mtd_device_parse_register()
>  * provide default cleanup routine for parsers that don't have one (all
>    parsers, ATM)
>  * put more common logic in mtd_part_parser_cleanup(), to avoid making the
>    caller worry about some of the reference counting
>  * (hopefully) less convoluted error handling in mtd_device_parse_register()
> 
> Brian
> 
> Brian Norris (6):
>   mtd: ofpart: assign return argument exactly once
>   mtd: partitions: make parsers return 'const' partition arrays
>   mtd: partitions: rename MTD parser get/put
>   mtd: partitions: remove kmemdup()
>   mtd: partitions: pass around 'mtd_partitions' wrapper struct

Pushed patch 1-5 to l2-mtd.git.

>   mtd: partitions: support a cleanup callback for parsers

Sent v3 of patch 6, with a small change.

Brian

> 
>  drivers/mtd/afs.c              |  2 +-
>  drivers/mtd/ar7part.c          |  2 +-
>  drivers/mtd/bcm47xxpart.c      |  2 +-
>  drivers/mtd/bcm63xxpart.c      |  2 +-
>  drivers/mtd/cmdlinepart.c      |  2 +-
>  drivers/mtd/mtdcore.c          | 39 +++++++++++++++-------------
>  drivers/mtd/mtdcore.h          |  7 ++++-
>  drivers/mtd/mtdpart.c          | 59 +++++++++++++++++++++++++++++++++---------
>  drivers/mtd/ofpart.c           | 39 +++++++++++++++-------------
>  drivers/mtd/redboot.c          |  2 +-
>  include/linux/mtd/partitions.h | 10 ++++++-
>  11 files changed, 110 insertions(+), 56 deletions(-)
> 
> -- 
> 2.6.0.rc2.230.g3dd15c0
> 

Re: [PATCH v3 6/6] mtd: partitions: support a cleanup callback for parsers

[Boris Brezillon]

On Wed, 9 Dec 2015 10:24:03 -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().
> 
> Alongside this, define a default callback that we'll automatically use
> if the parser doesn't provide one, so we can still retain the old
> behavior.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>

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

> ---
> v3:
>  * drop redundant 'if (parsed.parser)' condition before cleanup
> 
> v2:
>  * put more common logic in mtd_part_parser_cleanup()
>  * provide default cleanup routine (i.e., kfree()) for parsers
> 
>  drivers/mtd/mtdcore.c          |  3 +--
>  drivers/mtd/mtdcore.h          |  2 ++
>  drivers/mtd/mtdpart.c          | 35 +++++++++++++++++++++++++++++++++--
>  include/linux/mtd/partitions.h |  1 +
>  4 files changed, 37 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 20b2b38247b6..89d811e7b04a 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -630,8 +630,7 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
>  
>  out:
>  	/* Cleanup any parsed partitions */
> -	if (parsed.parser)
> -		kfree(parsed.parts);
> +	mtd_part_parser_cleanup(&parsed);
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(mtd_device_parse_register);
> diff --git a/drivers/mtd/mtdcore.h b/drivers/mtd/mtdcore.h
> index ce81cc2002f4..55fdb8e1fd2a 100644
> --- a/drivers/mtd/mtdcore.h
> +++ b/drivers/mtd/mtdcore.h
> @@ -17,6 +17,8 @@ int parse_mtd_partitions(struct mtd_info *master, const char * const *types,
>  			 struct mtd_partitions *pparts,
>  			 struct mtd_part_parser_data *data);
>  
> +void mtd_part_parser_cleanup(struct mtd_partitions *parts);
> +
>  int __init init_mtdchar(void);
>  void __exit cleanup_mtdchar(void);
>  
> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> index 53517d7653cb..10bf304027dd 100644
> --- a/drivers/mtd/mtdpart.c
> +++ b/drivers/mtd/mtdpart.c
> @@ -709,10 +709,23 @@ static inline void mtd_part_parser_put(const struct mtd_part_parser *p)
>  	module_put(p->owner);
>  }
>  
> +/*
> + * Many partition parsers just expected the core to kfree() all their data in
> + * one chunk. Do that by default.
> + */
> +static void mtd_part_parser_cleanup_default(const struct mtd_partition *pparts,
> +					    int nr_parts)
> +{
> +	kfree(pparts);
> +}
> +
>  int __register_mtd_parser(struct mtd_part_parser *p, struct module *owner)
>  {
>  	p->owner = owner;
>  
> +	if (!p->cleanup)
> +		p->cleanup = &mtd_part_parser_cleanup_default;
> +
>  	spin_lock(&part_parser_lock);
>  	list_add(&p->list, &part_parsers);
>  	spin_unlock(&part_parser_lock);
> @@ -756,7 +769,9 @@ static const char * const default_mtd_part_types[] = {
>   * This function may return:
>   * o a negative error code in case of failure
>   * o zero otherwise, and @pparts will describe the partitions, number of
> - *   partitions, and the parser which parsed them
> + *   partitions, and the parser which parsed them. Caller must release
> + *   resources with mtd_part_parser_cleanup() when finished with the returned
> + *   data.
>   */
>  int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
>  			 struct mtd_partitions *pparts,
> @@ -780,7 +795,6 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
>  		ret = (*parser->parse_fn)(master, &pparts->parts, 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);
> @@ -788,6 +802,7 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
>  			pparts->parser = parser;
>  			return 0;
>  		}
> +		mtd_part_parser_put(parser);
>  		/*
>  		 * Stash the first error we see; only report it if no parser
>  		 * succeeds
> @@ -798,6 +813,22 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
>  	return err;
>  }
>  
> +void mtd_part_parser_cleanup(struct mtd_partitions *parts)
> +{
> +	const struct mtd_part_parser *parser;
> +
> +	if (!parts)
> +		return;
> +
> +	parser = parts->parser;
> +	if (parser) {
> +		if (parser->cleanup)
> +			parser->cleanup(parts->parts, parts->nr_parts);
> +
> +		mtd_part_parser_put(parser);
> +	}
> +}
> +
>  int mtd_is_partition(const struct mtd_info *mtd)
>  {
>  	struct mtd_part *part;
> diff --git a/include/linux/mtd/partitions.h b/include/linux/mtd/partitions.h
> index cceaf7bd1537..70736e1e6c8f 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 *, const struct mtd_partition **,
>  			struct mtd_part_parser_data *);
> +	void (*cleanup)(const struct mtd_partition *pparts, int nr_parts);
>  };
>  
>  /* Container for passing around a set of parsed partitions */



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

Re: [PATCH v3 6/6] mtd: partitions: support a cleanup callback for parsers

[Brian Norris]

On Wed, Dec 09, 2015 at 10:46:50PM +0100, Boris Brezillon wrote:
> On Wed, 9 Dec 2015 10:24:03 -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().
> > 
> > Alongside this, define a default callback that we'll automatically use
> > if the parser doesn't provide one, so we can still retain the old
> > behavior.
> > 
> > Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> 
> Reviewed-by: Boris Brezillon <boris.brezillon@free-electrons.com>

Applied, thanks!