Re: [PATCH V3 2/3] mtd: add core code reading DT specified part probes

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

Hi Rafal,

On Fri, 31 Mar 2017 13:40:15 +0200
Rafał Miłecki <zajec5@gmail.com> wrote:

> From: Rafał Miłecki <rafal@milecki.pl>
> 
> Handling (creating) partitions for flash devices requires using a proper
> driver that will read partition table (out of somewhere). We can't
> simply try all existing drivers one by one:
> 1) It would increase boot time
> 2) The order could be a problem
> 3) In some corner cases parsers could misinterpret some data as a table
> Due to this MTD subsystem allows drivers to specify a list of applicable
> part probes.
> 
> So far physmap_of was the only driver with support for linux,part-probe
> DT property. Other ones had list or probes hardcoded which wasn't making
> them really flexible. It prevented using common flash drivers on
> platforms that required some specific partition table access.
> 
> This commit adds support for mentioned DT property directly to the MTD
> core. It's a rewritten implementation of physmap_of's code and it makes
> original code obsolete. Thanks to calling it on device parse
> registration (as suggested by Boris) all drivers gain support for it for
> free.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
>  drivers/mtd/mtdcore.c | 36 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 66a9dedd1062..917def28c756 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -664,6 +664,32 @@ static void mtd_set_dev_defaults(struct mtd_info *mtd)
>  	}
>  }
>  
> +static const char * const *mtd_of_get_probes(struct device_node *np)

To be consistent with NAND DT helpers, can you put the of_get_ prefix
at the beginning: of_get_mtd_probes().
And get_probes() is a bit too generic IMHO, how about
of_get_mtd_part_probes() (or any other name clearly showing that this
is about partition parsers/probes).

> +{
> +	const char **res;
> +	int count;
> +
> +	count = of_property_count_strings(np, "linux,part-probe");
> +	if (count < 0)
> +		return NULL;
> +
> +	res = kzalloc((count + 1) * sizeof(*res), GFP_KERNEL);
> +	if (!res)
> +		return NULL;
> +
> +	count = of_property_read_string_array(np, "linux,part-probe", res,
> +					      count);
> +	if (count < 0)
> +		return NULL;
> +
> +	return res;
> +}
> +
> +static inline void mtd_of_free_probes(const char * const *probes)

Drop the inline, the compiler is smart enough to decide by itself.

> +{
> +	kfree(probes);
> +}

This is not really DT specific, and we might want to extract the list
of probes by other means in the future (cmdline, ACPI?).

How about declaring these 2 functions:

static char **mtd_alloc_part_type_table(int nentries)
{
	return kzalloc((nentries + 1) * sizeof(*res), GFP_KERNEL);
}

static void mtd_free_part_type_table(const char * const *table)
{
	kfree(table);
}

You can then use mtd_alloc_part_type_table() in
of_get_mtd_part_probes() to allocate your partition type table.

> +
>  /**
>   * mtd_device_parse_register - parse partitions and register an MTD device.
>   *
> @@ -698,14 +724,19 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
>  			      const struct mtd_partition *parts,
>  			      int nr_parts)
>  {
> +	const char * const *part_probe_types;
>  	struct mtd_partitions parsed;
>  	int ret;
>  
>  	mtd_set_dev_defaults(mtd);
>  
> +	part_probe_types = mtd_of_get_probes(mtd_get_of_node(mtd));
> +	if (!part_probe_types)
> +		part_probe_types = types;
> +

How about doing it the other way around:

	if (part_probe_types)
		types = part_probe_types;

>  	memset(&parsed, 0, sizeof(parsed));
>  
> -	ret = parse_mtd_partitions(mtd, types, &parsed, parser_data);
> +	ret = parse_mtd_partitions(mtd, part_probe_types, &parsed, parser_data);

this way you don't need this change

>  	if ((ret < 0 || parsed.nr_parts == 0) && parts && nr_parts) {
>  		/* Fall back to driver-provided partitions */
>  		parsed = (struct mtd_partitions){
> @@ -720,6 +751,9 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
>  		memset(&parsed, 0, sizeof(parsed));
>  	}
>  
> +	if (part_probe_types != types)
> +		mtd_of_free_probes(part_probe_types);

and here you simply have:

	mtd_of_free_probes(part_probe_types);


> +
>  	ret = mtd_add_device_partitions(mtd, &parsed);
>  	if (ret)
>  		goto out;