[PATCH 0/2] mtd: simplify mtd_device_parse_register code

[PATCH 0/2] mtd: simplify mtd_device_parse_register code

[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

[PATCH 1/2] mtd: move code adding master MTD out of mtd_add_device_partitions

[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

[PATCH 2/2] mtd: get rid of the mtd_add_device_partitions function

[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

Re: [PATCH 1/2] mtd: move code adding master MTD out of mtd_add_device_partitions

[Boris Brezillon]

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);

Re: [PATCH 2/2] mtd: get rid of the mtd_add_device_partitions function

[Boris Brezillon]

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;
>  

Re: [PATCH 1/2] mtd: move code adding master MTD out of mtd_add_device_partitions

[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.

Re: [PATCH 2/2] mtd: get rid of the mtd_add_device_partitions function

[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!

[PATCH V2 0/2] mtd: simplify mtd_device_parse_register code

[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

[PATCH V2 1/2] mtd: move code adding master MTD out of mtd_add_device_partitions

[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

[PATCH V2 2/2] mtd: get rid of the mtd_add_device_partitions function

[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

Re: [PATCH V2 1/2] mtd: move code adding master MTD out of mtd_add_device_partitions

[Boris Brezillon]

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);

[PATCH V3 0/2] mtd: simplify mtd_device_parse_register code

[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

[PATCH V3 1/2] mtd: move code adding master MTD out of mtd_add_device_partitions

[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

[PATCH V3 2/2] mtd: get rid of the mtd_add_device_partitions function

[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

[PATCH V4 0/2] mtd: simplify mtd_device_parse_register code

[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

[PATCH V4 1/2] mtd: move code adding master MTD out of mtd_add_device_partitions

[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

[PATCH V4 2/2] mtd: get rid of the mtd_add_device_partitions()

[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

Re: [PATCH V5 0/2] mtd: simplify mtd_device_parse_register() code

[Boris Brezillon]

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

Re: [PATCH V5 0/2] mtd: simplify mtd_device_parse_register() code

[Rafał Miłecki]

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.