[PATCH] mtd: part: add generic parsing of linux,part-probe

[Hauke Mehrtens]

On 05/19/2015 12:49 AM, Brian Norris wrote:
> On Sun, May 17, 2015 at 07:35:12PM +0200, Hauke Mehrtens wrote:
>> This moves the linux,part-probe device tree parsing code from
>> physmap_of.c to mtdpart.c. Now all drivers can use this feature by just
>> providing a reference to they device tree node in struct
> 
> s/they/their/
> or
> s/they/the/
> ?
> 
>> mtd_part_parser_data.
>>
>> Signed-off-by: Hauke Mehrtens <hauke at hauke-m.de>
>> ---
>>  drivers/mtd/maps/physmap_of.c | 40 +---------------------------------------
>>  drivers/mtd/mtdpart.c         | 38 ++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 39 insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/mtd/maps/physmap_of.c b/drivers/mtd/maps/physmap_of.c
>> index 774b32f..fd3750f 100644
>> --- a/drivers/mtd/maps/physmap_of.c
>> +++ b/drivers/mtd/maps/physmap_of.c
>> @@ -112,45 +112,9 @@ static struct mtd_info *obsolete_probe(struct platform_device *dev,
>>  static const char * const part_probe_types_def[] = {
>>  	"cmdlinepart", "RedBoot", "ofpart", "ofoldpart", NULL };
>>  
>> -static const char * const *of_get_probes(struct device_node *dp)
>> -{
>> -	const char *cp;
>> -	int cplen;
>> -	unsigned int l;
>> -	unsigned int count;
>> -	const char **res;
>> -
>> -	cp = of_get_property(dp, "linux,part-probe", &cplen);
>> -	if (cp == NULL)
>> -		return part_probe_types_def;
>> -
>> -	count = 0;
>> -	for (l = 0; l != cplen; l++)
>> -		if (cp[l] == 0)
>> -			count++;
>> -
>> -	res = kzalloc((count + 1)*sizeof(*res), GFP_KERNEL);
>> -	count = 0;
>> -	while (cplen > 0) {
>> -		res[count] = cp;
>> -		l = strlen(cp) + 1;
>> -		cp += l;
>> -		cplen -= l;
>> -		count++;
>> -	}
>> -	return res;
>> -}
>> -
>> -static void of_free_probes(const char * const *probes)
>> -{
>> -	if (probes != part_probe_types_def)
>> -		kfree(probes);
>> -}
> 
> Huh. I didn't even notice this code existed. Like you, I've wondered if
> we could have a better way to suggest the partition parser types without
> out specifying them in each driver. That seems like the wrong level of
> abstraction.
> 
> Ideally, we'd just be able to have a standard set that satisfies all
> users, like how all loaded block device partitioners are all checked for
> each block device (check_part[] in block/partitions/check.c). The only
> problem I can think of would be if some parsers have subtleties such
> that they can't safely be applied to all MTDs, even with careful
> prioritization.

I do not think all the parsers are or will be completely safe when they
are trying to parse other data. Some random block could get mixed up
with some vendor header format and then you get wrong partition layout.

> But if that doesn't work, then I suppose a device-tree property like
> this could work.

Currently some flash drivers have the parser order hard coded and I do
not think that is a good idea, if we have device tree.

>> -
>>  static const struct of_device_id of_flash_match[];
>>  static int of_flash_probe(struct platform_device *dev)
>>  {
>> -	const char * const *part_probe_types;
>>  	const struct of_device_id *match;
>>  	struct device_node *dp = dev->dev.of_node;
>>  	struct resource res;
>> @@ -310,10 +274,8 @@ static int of_flash_probe(struct platform_device *dev)
>>  		goto err_out;
>>  
>>  	ppdata.of_node = dp;
>> -	part_probe_types = of_get_probes(dp);
>> -	mtd_device_parse_register(info->cmtd, part_probe_types, &ppdata,
>> +	mtd_device_parse_register(info->cmtd, part_probe_types_def, &ppdata,
>>  			NULL, 0);
>> -	of_free_probes(part_probe_types);
>>  
>>  	kfree(mtd_list);
>>  
>> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
>> index cafdb88..b3059d6 100644
>> --- a/drivers/mtd/mtdpart.c
>> +++ b/drivers/mtd/mtdpart.c
>> @@ -29,6 +29,7 @@
>>  #include <linux/kmod.h>
>>  #include <linux/mtd/mtd.h>
>>  #include <linux/mtd/partitions.h>
>> +#include <linux/of.h>
>>  #include <linux/err.h>
>>  #include <linux/kconfig.h>
>>  
>> @@ -718,6 +719,35 @@ void deregister_mtd_parser(struct mtd_part_parser *p)
>>  }
>>  EXPORT_SYMBOL_GPL(deregister_mtd_parser);
>>  
>> +static const char * const *of_get_probes(struct device_node *dp)
> 
> Maybe a quick comment at the top to note that if non-NULL, its return
> value must be kfree()'d by the caller? Could be helpful if we want to
> move the code elsewhere. (I'm almost tempted to suggest moving this to
> drivers/of/of_mtd.c, in fact.)

Yes I will add a comment.

>> +{
>> +	const char *cp;
>> +	int cplen;
>> +	unsigned int l;
>> +	unsigned int count;
>> +	const char **res;
>> +
>> +	cp = of_get_property(dp, "linux,part-probe", &cplen);
> 
> I see this is not property is not documented in the first place. If
> we're going to make it generically useful, we definitely need to
> document it, including what the supported partition probe names are.

Sorry I did not check, I just amused that it is documented because it
was already there, I will add.

> I'm also kind of ambivalent on this point, but I might as well ask: does
> the 'linux,...' naming make sense? We're not really supposed to be
> putting pure software information in the device tree, let alone info
> that's specific to Linux. I see how the partition parser could be (and
> should be, really) considered OS-agnostic, though, and might deserve a
> spot in the DT, at least if it's not safe to run arbitrary partition
> parsers on all MTDs.
> 
>> +	if (cp == NULL)
>> +		return NULL;
>> +
>> +	count = 0;
>> +	for (l = 0; l != cplen; l++)
>> +		if (cp[l] == 0)
>> +			count++;
>> +
>> +	res = kzalloc((count + 1)*sizeof(*res), GFP_KERNEL);
> 
> Might as well fix the spacing here while you're moving the code around.

Will fix that.

>> +	count = 0;
>> +	while (cplen > 0) {
>> +		res[count] = cp;
>> +		l = strlen(cp) + 1;
>> +		cp += l;
>> +		cplen -= l;
>> +		count++;
>> +	}
>> +	return res;
>> +}
>> +
>>  /*
>>   * Do not forget to update 'parse_mtd_partitions()' kerneldoc comment if you
>>   * are changing this array!
>> @@ -754,6 +784,13 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
>>  {
>>  	struct mtd_part_parser *parser;
>>  	int ret = 0;
>> +	const char *const *types_of = NULL;
>> +
>> +	if (data && data->of_node) {
>> +		types_of = of_get_probes(data->of_node);
>> +		if (types_of != NULL)
>> +			types = types_of;
>> +	}
> 
> I'm not sure I really like the semantics here. This means that the
> device tree can override all Linux defaults. So we can't, for instance,
> have the cmdline parser override all other selections (e.g., for
> debugging), or make sure that 'ofpart' is available for all MTDs.

parse_mtd_partitions() often gets called by the driver with a hard coded
list and I wanted to make it possible to overwrite this list through
device tree.

> 
>>  
>>  	if (!types)
>>  		types = default_mtd_part_types;
> 
> In fact, I don't really like this existing hunk either ^^^. It has the
> same effect, of killing things like cmdlinepart, which should be
> available on all MTDs.

I do not get this.
cmdlinepart is one of the default parsers, if you haven't specified any
you will get it. It could be that you do not want the cmdlinepart
parser, so I think it should be possible to deactivate it, like though
device tree when you do not specify it in the list of parsers.

> 
>> @@ -772,6 +809,7 @@ int parse_mtd_partitions(struct mtd_info *master, const char *const *types,
>>  			break;
>>  		}
>>  	}
>> +	kfree(types_of);
>>  	return ret;
>>  }
>>  
> 
> Brian
>