* [PATCH 0/2] mtd: simplify mtd_device_parse_register code @ 2018-01-12 14:40 Rafał Miłecki 2018-01-12 14:40 ` [PATCH 1/2] mtd: move code adding master MTD out of mtd_add_device_partitions Rafał Miłecki ` (2 more replies) 0 siblings, 3 replies; 19+ messages in thread From: Rafał Miłecki @ 2018-01-12 14:40 UTC (permalink / raw) To: Brian Norris, David Woodhouse, Boris Brezillon, Marek Vasut, Richard Weinberger, Cyrille Pitchen Cc: linux-mtd, Jonas Gorski, Rafał Miłecki From: Rafał Miłecki <rafal@milecki.pl> Even though I have some experience with MTD subsystem every time I was starting working on it I had to do some quick research on the code flow. I think there is some place for cleanup and optimization that would simplify overall code and allow new developers to start working on it more easily. In these patches I meant to slightly simplify mtd_device_parse_register function. I think we could reduce amount of add/register/del functions and this is the first step for that. Ideally I think mtdcore.c shouldn't handle parsing but just use a single function from mtdpart.c. Patch 1/2 modifies code to make sure parsing code is not mixed with MTD master code. This should allow moving parsing parts to the mtdpart.c in the future. Patch 2/2 simplifies code and makes sure parsing is handled at 1 place only. Even though this cleanup isn't complete I think it already has some value and could be accepted (if there won't be objections) on its own. It's also quite small which should allow reviewing it easily. Rafał Miłecki (2): mtd: move code adding master MTD out of mtd_add_device_partitions mtd: get rid of the mtd_add_device_partitions function drivers/mtd/mtdcore.c | 45 +++++++++++++++------------------------------ 1 file changed, 15 insertions(+), 30 deletions(-) -- 2.11.0 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 1/2] mtd: move code adding master MTD out of mtd_add_device_partitions 2018-01-12 14:40 [PATCH 0/2] mtd: simplify mtd_device_parse_register code Rafał Miłecki @ 2018-01-12 14:40 ` Rafał Miłecki 2018-01-12 15:01 ` Boris Brezillon 2018-01-12 14:40 ` [PATCH 2/2] mtd: get rid of the mtd_add_device_partitions function Rafał Miłecki 2018-01-15 13:22 ` [PATCH V2 0/2] mtd: simplify mtd_device_parse_register code Rafał Miłecki 2 siblings, 1 reply; 19+ messages in thread From: Rafał Miłecki @ 2018-01-12 14:40 UTC (permalink / raw) To: Brian Norris, David Woodhouse, Boris Brezillon, Marek Vasut, Richard Weinberger, Cyrille Pitchen Cc: linux-mtd, Jonas Gorski, Rafał Miłecki From: Rafał Miłecki <rafal@milecki.pl> This change is a small cleanup of mtd_device_parse_register. When using MTD_PARTITIONED_MASTER it makes sure a master MTD is registered before dealing with partitions. There are 2 advantages of this: 1) Not mixing code handling master MTD with code handling partitions 2) Possibility of reusing mtd_parse_part in the future to avoid some code duplication. This commit doesn't change any behavior except from a slightly different failure code path. The new code may need to call del_mtd_device when something goes wrong. Signed-off-by: Rafał Miłecki <rafal@milecki.pl> --- drivers/mtd/mtdcore.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c index f80e911b8843..f6460862e2ad 100644 --- a/drivers/mtd/mtdcore.c +++ b/drivers/mtd/mtdcore.c @@ -641,20 +641,12 @@ static int mtd_add_device_partitions(struct mtd_info *mtd, { const struct mtd_partition *real_parts = parts->parts; int nbparts = parts->nr_parts; - int ret; - if (nbparts == 0 || IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) { - ret = add_mtd_device(mtd); - if (ret) - return ret; - } + if (nbparts == 0 && !IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) + return add_mtd_device(mtd); - if (nbparts > 0) { - ret = add_mtd_partitions(mtd, real_parts, nbparts); - if (ret && IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) - del_mtd_device(mtd); - return ret; - } + if (nbparts > 0) + return add_mtd_partitions(mtd, real_parts, nbparts); return 0; } @@ -714,6 +706,12 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types, mtd_set_dev_defaults(mtd); + if (IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) { + ret = add_mtd_device(mtd); + if (ret) + return ret; + } + memset(&parsed, 0, sizeof(parsed)); ret = parse_mtd_partitions(mtd, types, &parsed, parser_data); @@ -753,6 +751,8 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types, out: /* Cleanup any parsed partitions */ mtd_part_parser_cleanup(&parsed); + if (ret && IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) + del_mtd_device(mtd); return ret; } EXPORT_SYMBOL_GPL(mtd_device_parse_register); -- 2.11.0 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] mtd: move code adding master MTD out of mtd_add_device_partitions 2018-01-12 14:40 ` [PATCH 1/2] mtd: move code adding master MTD out of mtd_add_device_partitions Rafał Miłecki @ 2018-01-12 15:01 ` Boris Brezillon 2018-01-15 10:58 ` Rafał Miłecki 0 siblings, 1 reply; 19+ messages in thread From: Boris Brezillon @ 2018-01-12 15:01 UTC (permalink / raw) To: Rafał Miłecki Cc: Brian Norris, David Woodhouse, Marek Vasut, Richard Weinberger, Cyrille Pitchen, linux-mtd, Jonas Gorski, Rafał Miłecki On Fri, 12 Jan 2018 15:40:33 +0100 Rafał Miłecki <zajec5@gmail.com> wrote: > From: Rafał Miłecki <rafal@milecki.pl> > > This change is a small cleanup of mtd_device_parse_register. When using > MTD_PARTITIONED_MASTER it makes sure a master MTD is registered before > dealing with partitions. There are 2 advantages of this: > 1) Not mixing code handling master MTD with code handling partitions > 2) Possibility of reusing mtd_parse_part in the future to avoid some > code duplication. > > This commit doesn't change any behavior except from a slightly different > failure code path. The new code may need to call del_mtd_device when > something goes wrong. > > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > --- > drivers/mtd/mtdcore.c | 24 ++++++++++++------------ > 1 file changed, 12 insertions(+), 12 deletions(-) > > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c > index f80e911b8843..f6460862e2ad 100644 > --- a/drivers/mtd/mtdcore.c > +++ b/drivers/mtd/mtdcore.c > @@ -641,20 +641,12 @@ static int mtd_add_device_partitions(struct mtd_info *mtd, > { > const struct mtd_partition *real_parts = parts->parts; > int nbparts = parts->nr_parts; > - int ret; > > - if (nbparts == 0 || IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) { > - ret = add_mtd_device(mtd); > - if (ret) > - return ret; > - } > + if (nbparts == 0 && !IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) > + return add_mtd_device(mtd); Why not moving this part in mtd_device_parse_register() as well. And I'd prefer something like: if (!nbparts && !device_is_registered(&mtd->dev)) ... > > - if (nbparts > 0) { > - ret = add_mtd_partitions(mtd, real_parts, nbparts); > - if (ret && IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) > - del_mtd_device(mtd); > - return ret; > - } > + if (nbparts > 0) > + return add_mtd_partitions(mtd, real_parts, nbparts); > > return 0; > } > @@ -714,6 +706,12 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types, > > mtd_set_dev_defaults(mtd); > > + if (IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) { > + ret = add_mtd_device(mtd); > + if (ret) > + return ret; > + } > + > memset(&parsed, 0, sizeof(parsed)); > > ret = parse_mtd_partitions(mtd, types, &parsed, parser_data); > @@ -753,6 +751,8 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types, > out: > /* Cleanup any parsed partitions */ > mtd_part_parser_cleanup(&parsed); > + if (ret && IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) if (ret && device_is_registered(&mtd->dev)) > + del_mtd_device(mtd); Blank line here, please. > return ret; > } > EXPORT_SYMBOL_GPL(mtd_device_parse_register); ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] mtd: move code adding master MTD out of mtd_add_device_partitions 2018-01-12 15:01 ` Boris Brezillon @ 2018-01-15 10:58 ` Rafał Miłecki 0 siblings, 0 replies; 19+ messages in thread From: Rafał Miłecki @ 2018-01-15 10:58 UTC (permalink / raw) To: Boris Brezillon Cc: Brian Norris, David Woodhouse, Marek Vasut, Richard Weinberger, Cyrille Pitchen, linux-mtd, Jonas Gorski, Rafał Miłecki On 12 January 2018 at 16:01, Boris Brezillon <boris.brezillon@free-electrons.com> wrote: >> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c >> index f80e911b8843..f6460862e2ad 100644 >> --- a/drivers/mtd/mtdcore.c >> +++ b/drivers/mtd/mtdcore.c >> @@ -641,20 +641,12 @@ static int mtd_add_device_partitions(struct mtd_info *mtd, >> { >> const struct mtd_partition *real_parts = parts->parts; >> int nbparts = parts->nr_parts; >> - int ret; >> >> - if (nbparts == 0 || IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) { >> - ret = add_mtd_device(mtd); >> - if (ret) >> - return ret; >> - } >> + if (nbparts == 0 && !IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) >> + return add_mtd_device(mtd); > > Why not moving this part in mtd_device_parse_register() as well. I move that in 2/2. I prefer to handle these changes in 2 steps as it should be quite easier to review it that way. > And I'd prefer something like: > > if (!nbparts && !device_is_registered(&mtd->dev)) > ... Nice idea with that device_is_registered. ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 2/2] mtd: get rid of the mtd_add_device_partitions function 2018-01-12 14:40 [PATCH 0/2] mtd: simplify mtd_device_parse_register code Rafał Miłecki 2018-01-12 14:40 ` [PATCH 1/2] mtd: move code adding master MTD out of mtd_add_device_partitions Rafał Miłecki @ 2018-01-12 14:40 ` Rafał Miłecki 2018-01-12 15:11 ` Boris Brezillon 2018-01-15 13:22 ` [PATCH V2 0/2] mtd: simplify mtd_device_parse_register code Rafał Miłecki 2 siblings, 1 reply; 19+ messages in thread From: Rafał Miłecki @ 2018-01-12 14:40 UTC (permalink / raw) To: Brian Norris, David Woodhouse, Boris Brezillon, Marek Vasut, Richard Weinberger, Cyrille Pitchen Cc: linux-mtd, Jonas Gorski, Rafał Miłecki From: Rafał Miłecki <rafal@milecki.pl> This simplifies code a bit by: 1) Avoiding an extra (tiny) function 2) Checking for amount of parsed (found) partitions just once 3) Avoiding clearing/filling struct mtd_partitions manually With this commit a proper functions are called directly from the mtd_device_parse_register. It doesn't need to use minor tricks like memsetting struct to 0 to trigger an expected mtd_add_device_partitions behavior. Signed-off-by: Rafał Miłecki <rafal@milecki.pl> --- drivers/mtd/mtdcore.c | 29 +++++++---------------------- 1 file changed, 7 insertions(+), 22 deletions(-) diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c index f6460862e2ad..0a414750bc8b 100644 --- a/drivers/mtd/mtdcore.c +++ b/drivers/mtd/mtdcore.c @@ -636,21 +636,6 @@ int del_mtd_device(struct mtd_info *mtd) return ret; } -static int mtd_add_device_partitions(struct mtd_info *mtd, - struct mtd_partitions *parts) -{ - const struct mtd_partition *real_parts = parts->parts; - int nbparts = parts->nr_parts; - - if (nbparts == 0 && !IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) - return add_mtd_device(mtd); - - if (nbparts > 0) - return add_mtd_partitions(mtd, real_parts, nbparts); - - return 0; -} - /* * Set a few defaults based on the parent devices, if not provided by the * driver @@ -717,19 +702,19 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types, 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, - }; + ret = add_mtd_partitions(mtd, 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 */ - memset(&parsed, 0, sizeof(parsed)); + if (!IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) + ret = add_mtd_device(mtd); + else + ret = 0; + } else { + ret = add_mtd_partitions(mtd, parsed.parts, parsed.nr_parts); } - - ret = mtd_add_device_partitions(mtd, &parsed); if (ret) goto out; -- 2.11.0 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] mtd: get rid of the mtd_add_device_partitions function 2018-01-12 14:40 ` [PATCH 2/2] mtd: get rid of the mtd_add_device_partitions function Rafał Miłecki @ 2018-01-12 15:11 ` Boris Brezillon 2018-01-15 12:56 ` Rafał Miłecki 0 siblings, 1 reply; 19+ messages in thread From: Boris Brezillon @ 2018-01-12 15:11 UTC (permalink / raw) To: Rafał Miłecki Cc: Brian Norris, David Woodhouse, Marek Vasut, Richard Weinberger, Cyrille Pitchen, linux-mtd, Jonas Gorski, Rafał Miłecki On Fri, 12 Jan 2018 15:40:34 +0100 Rafał Miłecki <zajec5@gmail.com> wrote: > From: Rafał Miłecki <rafal@milecki.pl> > > This simplifies code a bit by: > 1) Avoiding an extra (tiny) function > 2) Checking for amount of parsed (found) partitions just once > 3) Avoiding clearing/filling struct mtd_partitions manually > > With this commit a proper functions are called directly from the > mtd_device_parse_register. It doesn't need to use minor tricks like > memsetting struct to 0 to trigger an expected mtd_add_device_partitions > behavior. > > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > --- > drivers/mtd/mtdcore.c | 29 +++++++---------------------- > 1 file changed, 7 insertions(+), 22 deletions(-) > > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c > index f6460862e2ad..0a414750bc8b 100644 > --- a/drivers/mtd/mtdcore.c > +++ b/drivers/mtd/mtdcore.c > @@ -636,21 +636,6 @@ int del_mtd_device(struct mtd_info *mtd) > return ret; > } > > -static int mtd_add_device_partitions(struct mtd_info *mtd, > - struct mtd_partitions *parts) > -{ > - const struct mtd_partition *real_parts = parts->parts; > - int nbparts = parts->nr_parts; > - > - if (nbparts == 0 && !IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) > - return add_mtd_device(mtd); > - > - if (nbparts > 0) > - return add_mtd_partitions(mtd, real_parts, nbparts); > - > - return 0; > -} > - > /* > * Set a few defaults based on the parent devices, if not provided by the > * driver > @@ -717,19 +702,19 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types, > 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, > - }; > + ret = add_mtd_partitions(mtd, 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 */ > - memset(&parsed, 0, sizeof(parsed)); > + if (!IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) > + ret = add_mtd_device(mtd); > + else > + ret = 0; > + } else { > + ret = add_mtd_partitions(mtd, parsed.parts, parsed.nr_parts); > } How about: ret = parse_mtd_partitions(mtd, types, &parsed, parser_data); if (!ret && parsed.nr_parts) { parts = parsed.parts; nr_parts = parsed.nr_parts; } if (nr_parts) ret = add_mtd_partitions(mtd, parts, nr_parts); else if (!device_is_registered(&mtd->dev)) ret = add_mtd_device(mtd); else ret = 0; ... > - > - ret = mtd_add_device_partitions(mtd, &parsed); > if (ret) > goto out; > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] mtd: get rid of the mtd_add_device_partitions function 2018-01-12 15:11 ` Boris Brezillon @ 2018-01-15 12:56 ` Rafał Miłecki 0 siblings, 0 replies; 19+ messages in thread From: Rafał Miłecki @ 2018-01-15 12:56 UTC (permalink / raw) To: Boris Brezillon Cc: Brian Norris, David Woodhouse, Marek Vasut, Richard Weinberger, Cyrille Pitchen, linux-mtd, Jonas Gorski, Rafał Miłecki On 12 January 2018 at 16:11, Boris Brezillon <boris.brezillon@free-electrons.com> wrote: >> @@ -717,19 +702,19 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types, >> 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, >> - }; >> + ret = add_mtd_partitions(mtd, 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 */ >> - memset(&parsed, 0, sizeof(parsed)); >> + if (!IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) >> + ret = add_mtd_device(mtd); >> + else >> + ret = 0; >> + } else { >> + ret = add_mtd_partitions(mtd, parsed.parts, parsed.nr_parts); >> } > > How about: > > ret = parse_mtd_partitions(mtd, types, &parsed, parser_data); > if (!ret && parsed.nr_parts) { > parts = parsed.parts; > nr_parts = parsed.nr_parts; > } > > if (nr_parts) > ret = add_mtd_partitions(mtd, parts, nr_parts); > else if (!device_is_registered(&mtd->dev)) > ret = add_mtd_device(mtd); > else > ret = 0; I spent a moment writing possible cases on a paper and your solution should keep the same logic. Looks good! ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH V2 0/2] mtd: simplify mtd_device_parse_register code 2018-01-12 14:40 [PATCH 0/2] mtd: simplify mtd_device_parse_register code Rafał Miłecki 2018-01-12 14:40 ` [PATCH 1/2] mtd: move code adding master MTD out of mtd_add_device_partitions Rafał Miłecki 2018-01-12 14:40 ` [PATCH 2/2] mtd: get rid of the mtd_add_device_partitions function Rafał Miłecki @ 2018-01-15 13:22 ` Rafał Miłecki 2018-01-15 13:22 ` [PATCH V2 1/2] mtd: move code adding master MTD out of mtd_add_device_partitions Rafał Miłecki ` (2 more replies) 2 siblings, 3 replies; 19+ messages in thread From: Rafał Miłecki @ 2018-01-15 13:22 UTC (permalink / raw) To: Brian Norris, David Woodhouse, Boris Brezillon, Marek Vasut, Richard Weinberger, Cyrille Pitchen Cc: linux-mtd, Jonas Gorski, Rafał Miłecki From: Rafał Miłecki <rafal@milecki.pl> Even though I have some experience with MTD subsystem every time I was starting working on it I had to do some quick research on the code flow. I think there is some place for cleanup and optimization that would simplify overall code and allow new developers to start working on it more easily. In these patches I meant to slightly simplify mtd_device_parse_register function. I think we could reduce amount of add/register/del functions and this is the first step for that. Ideally I think mtdcore.c shouldn't handle parsing but just use a single function from mtdpart.c. Patch 1/2 modifies code to make sure parsing code is not mixed with MTD master code. This should allow moving parsing parts to the mtdpart.c in the future. Patch 2/2 simplifies code and makes sure parsing is handled at 1 place only. Even though this cleanup isn't complete I think it already has some value and could be accepted (if there won't be objections) on its own. It's also quite small which should allow reviewing it easily. Rafał Miłecki (2): mtd: move code adding master MTD out of mtd_add_device_partitions mtd: get rid of the mtd_add_device_partitions function drivers/mtd/mtdcore.c | 56 +++++++++++++++++---------------------------------- 1 file changed, 19 insertions(+), 37 deletions(-) -- 2.11.0 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH V2 1/2] mtd: move code adding master MTD out of mtd_add_device_partitions 2018-01-15 13:22 ` [PATCH V2 0/2] mtd: simplify mtd_device_parse_register code Rafał Miłecki @ 2018-01-15 13:22 ` Rafał Miłecki 2018-01-15 13:36 ` Boris Brezillon 2018-01-15 13:22 ` [PATCH V2 2/2] mtd: get rid of the mtd_add_device_partitions function Rafał Miłecki 2018-01-15 14:35 ` [PATCH V3 0/2] mtd: simplify mtd_device_parse_register code Rafał Miłecki 2 siblings, 1 reply; 19+ messages in thread From: Rafał Miłecki @ 2018-01-15 13:22 UTC (permalink / raw) To: Brian Norris, David Woodhouse, Boris Brezillon, Marek Vasut, Richard Weinberger, Cyrille Pitchen Cc: linux-mtd, Jonas Gorski, Rafał Miłecki From: Rafał Miłecki <rafal@milecki.pl> This change is a small cleanup of mtd_device_parse_register. When using MTD_PARTITIONED_MASTER it makes sure a master MTD is registered before dealing with partitions. There are 2 advantages of this: 1) Not mixing code handling master MTD with code handling partitions 2) Possibility of reusing mtd_parse_part in the future to avoid some code duplication. This commit doesn't change any behavior except from a slightly different failure code path. The new code may need to call del_mtd_device when something goes wrong. Signed-off-by: Rafał Miłecki <rafal@milecki.pl> --- V2: Use device_is_registered (thanks Boris) Add an empty line before "return ret;" --- drivers/mtd/mtdcore.c | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c index f80e911b8843..dd9ba5b7b68e 100644 --- a/drivers/mtd/mtdcore.c +++ b/drivers/mtd/mtdcore.c @@ -641,20 +641,12 @@ static int mtd_add_device_partitions(struct mtd_info *mtd, { const struct mtd_partition *real_parts = parts->parts; int nbparts = parts->nr_parts; - int ret; - if (nbparts == 0 || IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) { - ret = add_mtd_device(mtd); - if (ret) - return ret; - } + if (!nbparts && !device_is_registered(&mtd->dev)) + return add_mtd_device(mtd); - if (nbparts > 0) { - ret = add_mtd_partitions(mtd, real_parts, nbparts); - if (ret && IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) - del_mtd_device(mtd); - return ret; - } + if (nbparts > 0) + return add_mtd_partitions(mtd, real_parts, nbparts); return 0; } @@ -714,6 +706,12 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types, mtd_set_dev_defaults(mtd); + if (IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) { + ret = add_mtd_device(mtd); + if (ret) + return ret; + } + memset(&parsed, 0, sizeof(parsed)); ret = parse_mtd_partitions(mtd, types, &parsed, parser_data); @@ -753,6 +751,9 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types, out: /* Cleanup any parsed partitions */ mtd_part_parser_cleanup(&parsed); + if (ret && IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) + del_mtd_device(mtd); + return ret; } EXPORT_SYMBOL_GPL(mtd_device_parse_register); -- 2.11.0 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH V2 1/2] mtd: move code adding master MTD out of mtd_add_device_partitions 2018-01-15 13:22 ` [PATCH V2 1/2] mtd: move code adding master MTD out of mtd_add_device_partitions Rafał Miłecki @ 2018-01-15 13:36 ` Boris Brezillon 0 siblings, 0 replies; 19+ messages in thread From: Boris Brezillon @ 2018-01-15 13:36 UTC (permalink / raw) To: Rafał Miłecki Cc: Brian Norris, David Woodhouse, Marek Vasut, Richard Weinberger, Cyrille Pitchen, linux-mtd, Jonas Gorski, Rafał Miłecki On Mon, 15 Jan 2018 14:22:22 +0100 Rafał Miłecki <zajec5@gmail.com> wrote: > From: Rafał Miłecki <rafal@milecki.pl> > > This change is a small cleanup of mtd_device_parse_register. When using > MTD_PARTITIONED_MASTER it makes sure a master MTD is registered before > dealing with partitions. There are 2 advantages of this: > 1) Not mixing code handling master MTD with code handling partitions > 2) Possibility of reusing mtd_parse_part in the future to avoid some > code duplication. > > This commit doesn't change any behavior except from a slightly different > failure code path. The new code may need to call del_mtd_device when > something goes wrong. > > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > --- > V2: Use device_is_registered (thanks Boris) > Add an empty line before "return ret;" > --- > drivers/mtd/mtdcore.c | 25 +++++++++++++------------ > 1 file changed, 13 insertions(+), 12 deletions(-) > > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c > index f80e911b8843..dd9ba5b7b68e 100644 > --- a/drivers/mtd/mtdcore.c > +++ b/drivers/mtd/mtdcore.c > @@ -641,20 +641,12 @@ static int mtd_add_device_partitions(struct mtd_info *mtd, > { > const struct mtd_partition *real_parts = parts->parts; > int nbparts = parts->nr_parts; > - int ret; > > - if (nbparts == 0 || IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) { > - ret = add_mtd_device(mtd); > - if (ret) > - return ret; > - } > + if (!nbparts && !device_is_registered(&mtd->dev)) > + return add_mtd_device(mtd); > > - if (nbparts > 0) { > - ret = add_mtd_partitions(mtd, real_parts, nbparts); > - if (ret && IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) > - del_mtd_device(mtd); > - return ret; > - } > + if (nbparts > 0) > + return add_mtd_partitions(mtd, real_parts, nbparts); > > return 0; > } > @@ -714,6 +706,12 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types, > > mtd_set_dev_defaults(mtd); > > + if (IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) { > + ret = add_mtd_device(mtd); > + if (ret) > + return ret; > + } > + > memset(&parsed, 0, sizeof(parsed)); > > ret = parse_mtd_partitions(mtd, types, &parsed, parser_data); > @@ -753,6 +751,9 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types, > out: > /* Cleanup any parsed partitions */ > mtd_part_parser_cleanup(&parsed); > + if (ret && IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) Why not using device_is_registered(&mtd->dev) here as well? > + del_mtd_device(mtd); > + > return ret; > } > EXPORT_SYMBOL_GPL(mtd_device_parse_register); ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH V2 2/2] mtd: get rid of the mtd_add_device_partitions function 2018-01-15 13:22 ` [PATCH V2 0/2] mtd: simplify mtd_device_parse_register code Rafał Miłecki 2018-01-15 13:22 ` [PATCH V2 1/2] mtd: move code adding master MTD out of mtd_add_device_partitions Rafał Miłecki @ 2018-01-15 13:22 ` Rafał Miłecki 2018-01-15 14:35 ` [PATCH V3 0/2] mtd: simplify mtd_device_parse_register code Rafał Miłecki 2 siblings, 0 replies; 19+ messages in thread From: Rafał Miłecki @ 2018-01-15 13:22 UTC (permalink / raw) To: Brian Norris, David Woodhouse, Boris Brezillon, Marek Vasut, Richard Weinberger, Cyrille Pitchen Cc: linux-mtd, Jonas Gorski, Rafał Miłecki From: Rafał Miłecki <rafal@milecki.pl> This simplifies code a bit by: 1) Avoiding an extra (tiny) function 2) Checking for amount of parsed (found) partitions just once 3) Avoiding clearing/filling struct mtd_partitions manually With this commit a proper functions are called directly from the mtd_device_parse_register. It doesn't need to use minor tricks like memsetting struct to 0 to trigger an expected mtd_add_device_partitions behavior. Signed-off-by: Rafał Miłecki <rafal@milecki.pl> --- V2: Simplify conditions in the mtd_device_parse_register (thanks Boris) --- drivers/mtd/mtdcore.c | 41 +++++++++++------------------------------ 1 file changed, 11 insertions(+), 30 deletions(-) diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c index dd9ba5b7b68e..e8fa05941f64 100644 --- a/drivers/mtd/mtdcore.c +++ b/drivers/mtd/mtdcore.c @@ -636,21 +636,6 @@ int del_mtd_device(struct mtd_info *mtd) return ret; } -static int mtd_add_device_partitions(struct mtd_info *mtd, - struct mtd_partitions *parts) -{ - const struct mtd_partition *real_parts = parts->parts; - int nbparts = parts->nr_parts; - - if (!nbparts && !device_is_registered(&mtd->dev)) - return add_mtd_device(mtd); - - if (nbparts > 0) - return add_mtd_partitions(mtd, real_parts, nbparts); - - return 0; -} - /* * Set a few defaults based on the parent devices, if not provided by the * driver @@ -712,24 +697,20 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types, return ret; } - memset(&parsed, 0, sizeof(parsed)); - + /* Prefer parsed partitions over driver-provided fallback */ 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 */ - memset(&parsed, 0, sizeof(parsed)); + if (!ret && parsed.nr_parts) { + parts = parsed.parts; + nr_parts = parsed.nr_parts; } - ret = mtd_add_device_partitions(mtd, &parsed); + if (nr_parts) + ret = add_mtd_partitions(mtd, parts, nr_parts); + else if (!device_is_registered(&mtd->dev)) + ret = add_mtd_device(mtd); + else + ret = 0; + if (ret) goto out; -- 2.11.0 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH V3 0/2] mtd: simplify mtd_device_parse_register code 2018-01-15 13:22 ` [PATCH V2 0/2] mtd: simplify mtd_device_parse_register code Rafał Miłecki 2018-01-15 13:22 ` [PATCH V2 1/2] mtd: move code adding master MTD out of mtd_add_device_partitions Rafał Miłecki 2018-01-15 13:22 ` [PATCH V2 2/2] mtd: get rid of the mtd_add_device_partitions function Rafał Miłecki @ 2018-01-15 14:35 ` Rafał Miłecki 2018-01-15 14:35 ` [PATCH V3 1/2] mtd: move code adding master MTD out of mtd_add_device_partitions Rafał Miłecki ` (2 more replies) 2 siblings, 3 replies; 19+ messages in thread From: Rafał Miłecki @ 2018-01-15 14:35 UTC (permalink / raw) To: Brian Norris, David Woodhouse, Boris Brezillon, Marek Vasut, Richard Weinberger, Cyrille Pitchen Cc: linux-mtd, Jonas Gorski, Rafał Miłecki From: Rafał Miłecki <rafal@milecki.pl> Even though I have some experience with MTD subsystem every time I was starting working on it I had to do some quick research on the code flow. I think there is some place for cleanup and optimization that would simplify overall code and allow new developers to start working on it more easily. In these patches I meant to slightly simplify mtd_device_parse_register function. I think we could reduce amount of add/register/del functions and this is the first step for that. Ideally I think mtdcore.c shouldn't handle parsing but just use a single function from mtdpart.c. Patch 1/2 modifies code to make sure parsing code is not mixed with MTD master code. This should allow moving parsing parts to the mtdpart.c in the future. Patch 2/2 simplifies code and makes sure parsing is handled at 1 place only. Even though this cleanup isn't complete I think it already has some value and could be accepted (if there won't be objections) on its own. It's also quite small which should allow reviewing it easily. Rafał Miłecki (2): mtd: move code adding master MTD out of mtd_add_device_partitions mtd: get rid of the mtd_add_device_partitions function drivers/mtd/mtdcore.c | 56 +++++++++++++++++---------------------------------- 1 file changed, 19 insertions(+), 37 deletions(-) -- 2.11.0 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH V3 1/2] mtd: move code adding master MTD out of mtd_add_device_partitions 2018-01-15 14:35 ` [PATCH V3 0/2] mtd: simplify mtd_device_parse_register code Rafał Miłecki @ 2018-01-15 14:35 ` Rafał Miłecki 2018-01-15 14:35 ` [PATCH V3 2/2] mtd: get rid of the mtd_add_device_partitions function Rafał Miłecki 2018-01-15 22:45 ` [PATCH V4 0/2] mtd: simplify mtd_device_parse_register code Rafał Miłecki 2 siblings, 0 replies; 19+ messages in thread From: Rafał Miłecki @ 2018-01-15 14:35 UTC (permalink / raw) To: Brian Norris, David Woodhouse, Boris Brezillon, Marek Vasut, Richard Weinberger, Cyrille Pitchen Cc: linux-mtd, Jonas Gorski, Rafał Miłecki From: Rafał Miłecki <rafal@milecki.pl> This change is a small cleanup of mtd_device_parse_register. When using MTD_PARTITIONED_MASTER it makes sure a master MTD is registered before dealing with partitions. There are 2 advantages of this: 1) Not mixing code handling master MTD with code handling partitions 2) Possibility of reusing mtd_parse_part in the future to avoid some code duplication. This commit doesn't change any behavior except from a slightly different failure code path. The new code may need to call del_mtd_device when something goes wrong. Signed-off-by: Rafał Miłecki <rafal@milecki.pl> --- V2: Use device_is_registered (thanks Boris) Add an empty line before "return ret;" V3: Use device_is_registered also in a out code path --- drivers/mtd/mtdcore.c | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c index f80e911b8843..3304199cca07 100644 --- a/drivers/mtd/mtdcore.c +++ b/drivers/mtd/mtdcore.c @@ -641,20 +641,12 @@ static int mtd_add_device_partitions(struct mtd_info *mtd, { const struct mtd_partition *real_parts = parts->parts; int nbparts = parts->nr_parts; - int ret; - if (nbparts == 0 || IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) { - ret = add_mtd_device(mtd); - if (ret) - return ret; - } + if (!nbparts && !device_is_registered(&mtd->dev)) + return add_mtd_device(mtd); - if (nbparts > 0) { - ret = add_mtd_partitions(mtd, real_parts, nbparts); - if (ret && IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) - del_mtd_device(mtd); - return ret; - } + if (nbparts > 0) + return add_mtd_partitions(mtd, real_parts, nbparts); return 0; } @@ -714,6 +706,12 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types, mtd_set_dev_defaults(mtd); + if (IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) { + ret = add_mtd_device(mtd); + if (ret) + return ret; + } + memset(&parsed, 0, sizeof(parsed)); ret = parse_mtd_partitions(mtd, types, &parsed, parser_data); @@ -753,6 +751,9 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types, out: /* Cleanup any parsed partitions */ mtd_part_parser_cleanup(&parsed); + if (ret && device_is_registered(&mtd->dev)) + del_mtd_device(mtd); + return ret; } EXPORT_SYMBOL_GPL(mtd_device_parse_register); -- 2.11.0 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH V3 2/2] mtd: get rid of the mtd_add_device_partitions function 2018-01-15 14:35 ` [PATCH V3 0/2] mtd: simplify mtd_device_parse_register code Rafał Miłecki 2018-01-15 14:35 ` [PATCH V3 1/2] mtd: move code adding master MTD out of mtd_add_device_partitions Rafał Miłecki @ 2018-01-15 14:35 ` Rafał Miłecki 2018-01-15 22:45 ` [PATCH V4 0/2] mtd: simplify mtd_device_parse_register code Rafał Miłecki 2 siblings, 0 replies; 19+ messages in thread From: Rafał Miłecki @ 2018-01-15 14:35 UTC (permalink / raw) To: Brian Norris, David Woodhouse, Boris Brezillon, Marek Vasut, Richard Weinberger, Cyrille Pitchen Cc: linux-mtd, Jonas Gorski, Rafał Miłecki From: Rafał Miłecki <rafal@milecki.pl> This simplifies code a bit by: 1) Avoiding an extra (tiny) function 2) Checking for amount of parsed (found) partitions just once 3) Avoiding clearing/filling struct mtd_partitions manually With this commit a proper functions are called directly from the mtd_device_parse_register. It doesn't need to use minor tricks like memsetting struct to 0 to trigger an expected mtd_add_device_partitions behavior. Signed-off-by: Rafał Miłecki <rafal@milecki.pl> --- V2: Simplify conditions in the mtd_device_parse_register (thanks Boris) --- drivers/mtd/mtdcore.c | 41 +++++++++++------------------------------ 1 file changed, 11 insertions(+), 30 deletions(-) diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c index 3304199cca07..9baf93219873 100644 --- a/drivers/mtd/mtdcore.c +++ b/drivers/mtd/mtdcore.c @@ -636,21 +636,6 @@ int del_mtd_device(struct mtd_info *mtd) return ret; } -static int mtd_add_device_partitions(struct mtd_info *mtd, - struct mtd_partitions *parts) -{ - const struct mtd_partition *real_parts = parts->parts; - int nbparts = parts->nr_parts; - - if (!nbparts && !device_is_registered(&mtd->dev)) - return add_mtd_device(mtd); - - if (nbparts > 0) - return add_mtd_partitions(mtd, real_parts, nbparts); - - return 0; -} - /* * Set a few defaults based on the parent devices, if not provided by the * driver @@ -712,24 +697,20 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types, return ret; } - memset(&parsed, 0, sizeof(parsed)); - + /* Prefer parsed partitions over driver-provided fallback */ 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 */ - memset(&parsed, 0, sizeof(parsed)); + if (!ret && parsed.nr_parts) { + parts = parsed.parts; + nr_parts = parsed.nr_parts; } - ret = mtd_add_device_partitions(mtd, &parsed); + if (nr_parts) + ret = add_mtd_partitions(mtd, parts, nr_parts); + else if (!device_is_registered(&mtd->dev)) + ret = add_mtd_device(mtd); + else + ret = 0; + if (ret) goto out; -- 2.11.0 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH V4 0/2] mtd: simplify mtd_device_parse_register code 2018-01-15 14:35 ` [PATCH V3 0/2] mtd: simplify mtd_device_parse_register code Rafał Miłecki 2018-01-15 14:35 ` [PATCH V3 1/2] mtd: move code adding master MTD out of mtd_add_device_partitions Rafał Miłecki 2018-01-15 14:35 ` [PATCH V3 2/2] mtd: get rid of the mtd_add_device_partitions function Rafał Miłecki @ 2018-01-15 22:45 ` Rafał Miłecki 2018-01-15 22:45 ` [PATCH V4 1/2] mtd: move code adding master MTD out of mtd_add_device_partitions Rafał Miłecki ` (2 more replies) 2 siblings, 3 replies; 19+ messages in thread From: Rafał Miłecki @ 2018-01-15 22:45 UTC (permalink / raw) To: Brian Norris, David Woodhouse, Boris Brezillon, Marek Vasut, Richard Weinberger, Cyrille Pitchen Cc: linux-mtd, Jonas Gorski, Rafał Miłecki From: Rafał Miłecki <rafal@milecki.pl> Even though I have some experience with MTD subsystem every time I was starting working on it I had to do some quick research on the code flow. I think there is some place for cleanup and optimization that would simplify overall code and allow new developers to start working on it more easily. In these patches I meant to slightly simplify mtd_device_parse_register function. I think we could reduce amount of add/register/del functions and this is the first step for that. Ideally I think mtdcore.c shouldn't handle parsing but just use a single function from mtdpart.c. Patch 1/2 modifies code to make sure parsing code is not mixed with MTD master code. This should allow moving parsing parts to the mtdpart.c in the future. Patch 2/2 simplifies code and makes sure parsing is handled at 1 place only. Even though this cleanup isn't complete I think it already has some value and could be accepted (if there won't be objections) on its own. It's also quite small which should allow reviewing it easily. Rafał Miłecki (2): mtd: move code adding master MTD out of mtd_add_device_partitions mtd: get rid of the mtd_add_device_partitions function drivers/mtd/mtdcore.c | 58 ++++++++++++++++++--------------------------------- 1 file changed, 20 insertions(+), 38 deletions(-) -- 2.11.0 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH V4 1/2] mtd: move code adding master MTD out of mtd_add_device_partitions 2018-01-15 22:45 ` [PATCH V4 0/2] mtd: simplify mtd_device_parse_register code Rafał Miłecki @ 2018-01-15 22:45 ` Rafał Miłecki 2018-01-15 22:45 ` [PATCH V4 2/2] mtd: get rid of the mtd_add_device_partitions() Rafał Miłecki [not found] ` <20180116154542.9767-1-zajec5@gmail.com> 2 siblings, 0 replies; 19+ messages in thread From: Rafał Miłecki @ 2018-01-15 22:45 UTC (permalink / raw) To: Brian Norris, David Woodhouse, Boris Brezillon, Marek Vasut, Richard Weinberger, Cyrille Pitchen Cc: linux-mtd, Jonas Gorski, Rafał Miłecki From: Rafał Miłecki <rafal@milecki.pl> This change is a small cleanup of mtd_device_parse_register. When using MTD_PARTITIONED_MASTER it makes sure a master MTD is registered before dealing with partitions. There are 2 advantages of this: 1) Not mixing code handling master MTD with code handling partitions 2) Possibility of reusing mtd_parse_part in the future to avoid some code duplication. This commit doesn't change any behavior except from a slightly different failure code path. The new code may need to call del_mtd_device when something goes wrong. Signed-off-by: Rafał Miłecki <rafal@milecki.pl> --- V2: Use device_is_registered (thanks Boris) Add an empty line before "return ret;" V3: Use device_is_registered also in a out code path --- drivers/mtd/mtdcore.c | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c index f80e911b8843..3304199cca07 100644 --- a/drivers/mtd/mtdcore.c +++ b/drivers/mtd/mtdcore.c @@ -641,20 +641,12 @@ static int mtd_add_device_partitions(struct mtd_info *mtd, { const struct mtd_partition *real_parts = parts->parts; int nbparts = parts->nr_parts; - int ret; - if (nbparts == 0 || IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) { - ret = add_mtd_device(mtd); - if (ret) - return ret; - } + if (!nbparts && !device_is_registered(&mtd->dev)) + return add_mtd_device(mtd); - if (nbparts > 0) { - ret = add_mtd_partitions(mtd, real_parts, nbparts); - if (ret && IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) - del_mtd_device(mtd); - return ret; - } + if (nbparts > 0) + return add_mtd_partitions(mtd, real_parts, nbparts); return 0; } @@ -714,6 +706,12 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types, mtd_set_dev_defaults(mtd); + if (IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) { + ret = add_mtd_device(mtd); + if (ret) + return ret; + } + memset(&parsed, 0, sizeof(parsed)); ret = parse_mtd_partitions(mtd, types, &parsed, parser_data); @@ -753,6 +751,9 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types, out: /* Cleanup any parsed partitions */ mtd_part_parser_cleanup(&parsed); + if (ret && device_is_registered(&mtd->dev)) + del_mtd_device(mtd); + return ret; } EXPORT_SYMBOL_GPL(mtd_device_parse_register); -- 2.11.0 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH V4 2/2] mtd: get rid of the mtd_add_device_partitions() 2018-01-15 22:45 ` [PATCH V4 0/2] mtd: simplify mtd_device_parse_register code Rafał Miłecki 2018-01-15 22:45 ` [PATCH V4 1/2] mtd: move code adding master MTD out of mtd_add_device_partitions Rafał Miłecki @ 2018-01-15 22:45 ` Rafał Miłecki [not found] ` <20180116154542.9767-1-zajec5@gmail.com> 2 siblings, 0 replies; 19+ messages in thread From: Rafał Miłecki @ 2018-01-15 22:45 UTC (permalink / raw) To: Brian Norris, David Woodhouse, Boris Brezillon, Marek Vasut, Richard Weinberger, Cyrille Pitchen Cc: linux-mtd, Jonas Gorski, Rafał Miłecki From: Rafał Miłecki <rafal@milecki.pl> This simplifies code a bit by: 1) Avoiding an extra (tiny) function 2) Checking for amount of parsed (found) partitions just once 3) Avoiding clearing/filling struct mtd_partitions manually With this commit proper functions are called directly from the mtd_device_parse_register(). It doesn't need to use minor tricks like memsetting struct to 0 to trigger an expected mtd_add_device_partitions() behavior. Signed-off-by: Rafał Miłecki <rafal@milecki.pl> --- V2: Simplify conditions in the mtd_device_parse_register (thanks Boris) V4: Zero-initialize parsed variable. Some parsers return 0 without setting nr_parts. Update commit message: drop "a" from "a functions" & use () --- drivers/mtd/mtdcore.c | 43 ++++++++++++------------------------------- 1 file changed, 12 insertions(+), 31 deletions(-) diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c index 3304199cca07..4eebbc1cc967 100644 --- a/drivers/mtd/mtdcore.c +++ b/drivers/mtd/mtdcore.c @@ -636,21 +636,6 @@ int del_mtd_device(struct mtd_info *mtd) return ret; } -static int mtd_add_device_partitions(struct mtd_info *mtd, - struct mtd_partitions *parts) -{ - const struct mtd_partition *real_parts = parts->parts; - int nbparts = parts->nr_parts; - - if (!nbparts && !device_is_registered(&mtd->dev)) - return add_mtd_device(mtd); - - if (nbparts > 0) - return add_mtd_partitions(mtd, real_parts, nbparts); - - return 0; -} - /* * Set a few defaults based on the parent devices, if not provided by the * driver @@ -701,7 +686,7 @@ 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; + struct mtd_partitions parsed = { }; int ret; mtd_set_dev_defaults(mtd); @@ -712,24 +697,20 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types, return ret; } - memset(&parsed, 0, sizeof(parsed)); - + /* Prefer parsed partitions over driver-provided fallback */ 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 */ - memset(&parsed, 0, sizeof(parsed)); + if (!ret && parsed.nr_parts) { + parts = parsed.parts; + nr_parts = parsed.nr_parts; } - ret = mtd_add_device_partitions(mtd, &parsed); + if (nr_parts) + ret = add_mtd_partitions(mtd, parts, nr_parts); + else if (!device_is_registered(&mtd->dev)) + ret = add_mtd_device(mtd); + else + ret = 0; + if (ret) goto out; -- 2.11.0 ^ permalink raw reply related [flat|nested] 19+ messages in thread
[parent not found: <20180116154542.9767-1-zajec5@gmail.com>]
* Re: [PATCH V5 0/2] mtd: simplify mtd_device_parse_register() code [not found] ` <20180116154542.9767-1-zajec5@gmail.com> @ 2018-02-17 8:38 ` Boris Brezillon 2018-02-18 7:50 ` Rafał Miłecki 0 siblings, 1 reply; 19+ messages in thread From: Boris Brezillon @ 2018-02-17 8:38 UTC (permalink / raw) To: Rafał Miłecki Cc: Brian Norris, David Woodhouse, Boris Brezillon, Marek Vasut, Richard Weinberger, Cyrille Pitchen, linux-mtd, Jonas Gorski, Rafał Miłecki On Tue, 16 Jan 2018 16:45:40 +0100 Rafał Miłecki <zajec5@gmail.com> wrote: > From: Rafał Miłecki <rafal@milecki.pl> > > Even though I have some experience with MTD subsystem every time I was > starting working on it I had to do some quick research on the code flow. > I think there is some place for cleanup and optimization that would > simplify overall code and allow new developers to start working on it > more easily. > > In these patches I meant to slightly simplify mtd_device_parse_register > function. I think we could reduce amount of add/register/del functions > and this is the first step for that. > > Ideally I think mtdcore.c shouldn't handle parsing but just use a single > function from mtdpart.c. > > Patch 1/2 modifies code to make sure parsing code is not mixed with MTD > master code. This should allow moving parsing parts to the mtdpart.c in > the future. > > Patch 2/2 simplifies code and makes sure parsing is handled at 1 place > only. > > Even though this cleanup isn't complete I think it already has some > value and could be accepted (if there won't be objections) on its own. > It's also quite small which should allow reviewing it easily. > > Rafał Miłecki (2): > mtd: move code adding master MTD out of mtd_add_device_partitions() > mtd: get rid of the mtd_add_device_partitions() Applied (even if I keep thinking those 2 commits should have been merged in a single one :P) Thanks, Boris > > drivers/mtd/mtdcore.c | 58 ++++++++++++++++++--------------------------------- > 1 file changed, 20 insertions(+), 38 deletions(-) > -- Boris Brezillon, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering http://bootlin.com ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH V5 0/2] mtd: simplify mtd_device_parse_register() code 2018-02-17 8:38 ` [PATCH V5 0/2] mtd: simplify mtd_device_parse_register() code Boris Brezillon @ 2018-02-18 7:50 ` Rafał Miłecki 0 siblings, 0 replies; 19+ messages in thread From: Rafał Miłecki @ 2018-02-18 7:50 UTC (permalink / raw) To: Boris Brezillon Cc: Rafał Miłecki, Brian Norris, David Woodhouse, Boris Brezillon, Marek Vasut, Richard Weinberger, Cyrille Pitchen, linux-mtd, Jonas Gorski On 2018-02-17 09:38, Boris Brezillon wrote: > On Tue, 16 Jan 2018 16:45:40 +0100 > Rafał Miłecki <zajec5@gmail.com> wrote: > >> From: Rafał Miłecki <rafal@milecki.pl> >> >> Even though I have some experience with MTD subsystem every time I was >> starting working on it I had to do some quick research on the code >> flow. >> I think there is some place for cleanup and optimization that would >> simplify overall code and allow new developers to start working on it >> more easily. >> >> In these patches I meant to slightly simplify >> mtd_device_parse_register >> function. I think we could reduce amount of add/register/del functions >> and this is the first step for that. >> >> Ideally I think mtdcore.c shouldn't handle parsing but just use a >> single >> function from mtdpart.c. >> >> Patch 1/2 modifies code to make sure parsing code is not mixed with >> MTD >> master code. This should allow moving parsing parts to the mtdpart.c >> in >> the future. >> >> Patch 2/2 simplifies code and makes sure parsing is handled at 1 place >> only. >> >> Even though this cleanup isn't complete I think it already has some >> value and could be accepted (if there won't be objections) on its own. >> It's also quite small which should allow reviewing it easily. >> >> Rafał Miłecki (2): >> mtd: move code adding master MTD out of mtd_add_device_partitions() >> mtd: get rid of the mtd_add_device_partitions() > > Applied (even if I keep thinking those 2 commits should have been > merged in a single one :P) Thanks, I appreciate your help on this. ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2018-02-18 7:50 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-01-12 14:40 [PATCH 0/2] mtd: simplify mtd_device_parse_register code Rafał Miłecki 2018-01-12 14:40 ` [PATCH 1/2] mtd: move code adding master MTD out of mtd_add_device_partitions Rafał Miłecki 2018-01-12 15:01 ` Boris Brezillon 2018-01-15 10:58 ` Rafał Miłecki 2018-01-12 14:40 ` [PATCH 2/2] mtd: get rid of the mtd_add_device_partitions function Rafał Miłecki 2018-01-12 15:11 ` Boris Brezillon 2018-01-15 12:56 ` Rafał Miłecki 2018-01-15 13:22 ` [PATCH V2 0/2] mtd: simplify mtd_device_parse_register code Rafał Miłecki 2018-01-15 13:22 ` [PATCH V2 1/2] mtd: move code adding master MTD out of mtd_add_device_partitions Rafał Miłecki 2018-01-15 13:36 ` Boris Brezillon 2018-01-15 13:22 ` [PATCH V2 2/2] mtd: get rid of the mtd_add_device_partitions function Rafał Miłecki 2018-01-15 14:35 ` [PATCH V3 0/2] mtd: simplify mtd_device_parse_register code Rafał Miłecki 2018-01-15 14:35 ` [PATCH V3 1/2] mtd: move code adding master MTD out of mtd_add_device_partitions Rafał Miłecki 2018-01-15 14:35 ` [PATCH V3 2/2] mtd: get rid of the mtd_add_device_partitions function Rafał Miłecki 2018-01-15 22:45 ` [PATCH V4 0/2] mtd: simplify mtd_device_parse_register code Rafał Miłecki 2018-01-15 22:45 ` [PATCH V4 1/2] mtd: move code adding master MTD out of mtd_add_device_partitions Rafał Miłecki 2018-01-15 22:45 ` [PATCH V4 2/2] mtd: get rid of the mtd_add_device_partitions() Rafał Miłecki [not found] ` <20180116154542.9767-1-zajec5@gmail.com> 2018-02-17 8:38 ` [PATCH V5 0/2] mtd: simplify mtd_device_parse_register() code Boris Brezillon 2018-02-18 7:50 ` Rafał Miłecki
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.