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

[Brian Norris <computersforpeace@gmail.com>]

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