| Message ID | 1449271518-118900-6-git-send-email-computersforpeace@gmail.com |
|---|---|
| State | Accepted |
| Commit | 07fd2f871c5e3dfb8ff5eb9c4b44fdb4cf1aeff5 |
| Headers | show |
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
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;
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
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
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.
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
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)
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(-)