From: Jeff Westfahl <[email protected]>
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 <[email protected]>
---
drivers/mtd/Kconfig | 24 ++++++++++++++++++++++++
drivers/mtd/cmdlinepart.c | 3 ++-
2 files changed, 26 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
index a03ad29..f7df08c 100644
--- a/drivers/mtd/Kconfig
+++ b/drivers/mtd/Kconfig
@@ -110,6 +110,30 @@ config MTD_CMDLINE_PARTS
If unsure, say 'N'.
+config MTD_RESERVE_END
+ int "Reserved space at the end of an all remaining space partition"
+ depends on MTD_CMDLINE_PARTS = "y"
+ 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.
+
config MTD_AFS_PARTS
tristate "ARM Firmware Suite partition parsing"
depends on ARM
diff --git a/drivers/mtd/cmdlinepart.c b/drivers/mtd/cmdlinepart.c
index c850300..9b83c1a 100644
--- 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;
if (offset + part->parts[i].size > master->size) {
printk(KERN_WARNING ERRP
--
2.3.7
> + 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 <[email protected]> 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 <[email protected]> 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