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

[Boris Brezillon <boris.brezillon@free-electrons.com>]

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