mtd: Introduce CONFIG_MTD_RESERVE_END

From: Jeff Westfahl <jeff.westfahl@ni.com>

Add a new config parameter CONFIG_MTD_RESERVE_END. This is used with
command line partition parsing to reserve space at the end of a
partition defined with a size of '-', which indicates it should use all
remaining space.

Signed-off-by: Jeff Westfahl <jeff.westfahl@ni.com>
---
 drivers/mtd/Kconfig       | 24 ++++++++++++++++++++++++
 drivers/mtd/cmdlinepart.c |  3 ++-
 2 files changed, 26 insertions(+), 1 deletion(-)

> +	  This can be useful if, for example, the BBT is stored at the end
> +	  of the flash, and you don't want those blocks counted as part of
> +	  the last MTD partition.

And why is that a problem? We don't add Kconfig options just because you
"don't want" something.

Brian

Hi Brian,

On 04/29, Brian Norris wrote:
> > +	  This can be useful if, for example, the BBT is stored at the end
> > +	  of the flash, and you don't want those blocks counted as part of
> > +	  the last MTD partition.
> 
> And why is that a problem? We don't add Kconfig options just because you
> "don't want" something.

It's not a "problem" per se, but it does cause the calculated/displayed number
of bad PEBs to be greater than it actually is.  In older kernel versions, this
used to cause a warning on mount that it could not reserve enough PEBs, but
this no longer occurs.

Without the patch and config setting:

UBI: attached mtd3 (name "root", size 942 MiB) to ubi1
UBI: PEB size: 131072 bytes (128 KiB), LEB size: 129024 bytes
UBI: min./max. I/O unit sizes: 2048/2048, sub-page size 512
UBI: VID header offset: 512 (aligned 512), data offset: 2048
UBI: good PEBs: 7539, bad PEBs: 4, corrupted PEBs: 0
UBI: user volume: 1, internal volumes: 1, max. volumes count: 128
UBI: max/mean erase counter: 549/79, WL threshold: 4096, image sequence number: 0
UBI: available PEBs: 4, total reserved PEBs: 7535, PEBs reserved for bad PEB handling: 156

With the patch and config setting:

UBI: attached mtd3 (name "root", size 942 MiB) to ubi1
UBI: PEB size: 131072 bytes (128 KiB), LEB size: 129024 bytes
UBI: min./max. I/O unit sizes: 2048/2048, sub-page size 512
UBI: VID header offset: 512 (aligned 512), data offset: 2048
UBI: good PEBs: 7539, bad PEBs: 0, corrupted PEBs: 0
UBI: user volume: 1, internal volumes: 1, max. volumes count: 128
UBI: max/mean erase counter: 549/79, WL threshold: 4096, image sequence number: 0
UBI: available PEBs: 0, total reserved PEBs: 7539, PEBs reserved for bad PEB handling: 160

The reason for doing this as a Kconfig option rather than with an additional
partition is that we use the same .itb boot image (and kernel arguments) for
a series of embedded controllers that have different NAND flash sizes, and we
use the '-' command line parameter to give the root partition all the available
space after the other partitions.

Thanks,
Ben

While you're discussing more substantial questions with Brian, I found
some nits.

On Wed, 2015-04-29 at 14:28 -0500, Ben Shelton wrote:
> --- a/drivers/mtd/Kconfig
> +++ b/drivers/mtd/Kconfig


> +config MTD_RESERVE_END
> +	int "Reserved space at the end of an all remaining space partition"
> +	depends on MTD_CMDLINE_PARTS = "y"

(The quotes are unneeded.)

MTD_CMDLINE_PARTS is tristate. Why does it need to be built-in for this
symbol?

> +	default 0
> +	---help---
> +	  Specify an amount of reserved space at the end of the last MTD
> +	  partition when the size is specified with '-' to denote all
> +	  remaining space.
> +
> +	  This can be useful if, for example, the BBT is stored at the end
> +	  of the flash, and you don't want those blocks counted as part of
> +	  the last MTD partition. This is less heavyweight than reserving
> +	  the BBT blocks with a separate MTD partition. The BBT marks its
> +	  own blocks as bad blocks, which prevents an MTD driver such as
> +	  UBI from getting an accurate count of the actual bad blocks in
> +	  the MTD partition that contains the BBT.
> +
> +	  The value is specified in bytes. As an example, a typical BBT
> +	  reserves four erase blocks, and a typical erase block size is
> +	  128kB. To reserve that much space at the end of the flash, the
> +	  value for this config option would be 524288.
> +
> +	  If unsure, use the default value of zero.

> --- a/drivers/mtd/cmdlinepart.c
> +++ b/drivers/mtd/cmdlinepart.c
> @@ -340,7 +340,8 @@ static int parse_cmdline_partitions(struct mtd_info *master,
>  			offset = part->parts[i].offset;
>  
>  		if (part->parts[i].size == SIZE_REMAINING)
> -			part->parts[i].size = master->size - offset;
> +			part->parts[i].size = master->size - offset -
> +					      CONFIG_MTD_RESERVE_END;

I haven't tested this. (Quite often that means: I'm wrong.) But if
MTD_CMDLINE_PARTS is set to "m" I think MTD_RESERVE_END will not be set.
In that case CPP will helpfully set CONFIG_MTD_RESERVE_END to zero
(which is what you want). But it should also trigger this warning:
    "CONFIG_MTD_RESERVE_END" is not defined [-Wundef]

>  
>  		if (offset + part->parts[i].size > master->size) {
>  			printk(KERN_WARNING ERRP


Paul Bolle

Hi Paul,

On 05/01, Paul Bolle wrote:
> While you're discussing more substantial questions with Brian, I found
> some nits.
> 
> On Wed, 2015-04-29 at 14:28 -0500, Ben Shelton wrote:
> > --- a/drivers/mtd/Kconfig
> > +++ b/drivers/mtd/Kconfig
> 
> 
> > +config MTD_RESERVE_END
> > +	int "Reserved space at the end of an all remaining space partition"
> > +	depends on MTD_CMDLINE_PARTS = "y"
> 
> (The quotes are unneeded.)
> 
> MTD_CMDLINE_PARTS is tristate. Why does it need to be built-in for this
> symbol?

Good catch -- MTD_RESERVE_END should apply whether MTD_CMDLINE_PARTS is
built-in or a module.  I'll fix that in the next version of the patch.

Thanks,
Ben

On Thu, Apr 30, 2015 at 7:36 PM, Ben Shelton <ben.shelton@ni.com> wrote:
> The reason for doing this as a Kconfig option rather than with an additional
> partition is that we use the same .itb boot image (and kernel arguments) for
> a series of embedded controllers that have different NAND flash sizes, and we
> use the '-' command line parameter to give the root partition all the available
> space after the other partitions.

Wouldn't it make more sense to make cmdlineparts to recognize if it is
run on a nand flash that has on-flash BBT enabled, and then reduce the
SIZE_REMAINING partition's size by the amount of nand_bbt_descr's
maxblocks * erase block size?

Currently your proposed solution would break if boards have differing
erase block sizes, or if some have NOR flash, which makes it an option
for a rather narrow use case IMHO.


Regards
Jonas

On Fri, May 01, 2015 at 01:57:13PM +0200, Jonas Gorski wrote:
> On Thu, Apr 30, 2015 at 7:36 PM, Ben Shelton <ben.shelton@ni.com> wrote:
> > The reason for doing this as a Kconfig option rather than with an additional
> > partition is that we use the same .itb boot image (and kernel arguments) for
> > a series of embedded controllers that have different NAND flash sizes, and we
> > use the '-' command line parameter to give the root partition all the available
> > space after the other partitions.
> 
> Wouldn't it make more sense to make cmdlineparts to recognize if it is
> run on a nand flash that has on-flash BBT enabled, and then reduce the
> SIZE_REMAINING partition's size by the amount of nand_bbt_descr's
> maxblocks * erase block size?

That's a possibility, but that seems like a bit too much hidden policy
to add on top of the cmdlineparts format, which is pretty precise.

A better improvement, if you're dead set on using cmdlineparts rather
than some other better parser, might be to support something like:

  mtdparts=name:-(main),bbt(1M)

So the "remaining space" partition will allow for following partitions.
IOW, you would need to handle this case, which is currently an error:

        /* test if more partitions are following */
        if (*s == ',') {
                if (size == SIZE_REMAINING) {
                        printk(KERN_ERR ERRP "no partitions allowed after a fill-up partition\n");
                        return ERR_PTR(-EINVAL);
                }
		...
	}

> Currently your proposed solution would break if boards have differing
> erase block sizes, or if some have NOR flash, which makes it an option
> for a rather narrow use case IMHO.

Yeah, consider this a NAK for the current patch.

Brian