[U-Boot] [RFC: v2] tools/env: ensure environment starts at erase block boundary

[U-Boot] [RFC: v2] tools/env: ensure environment starts at erase block boundary

[Andreas Fenkart]

56086921 added support for unaligned environments access.
U-boot itself does not support this:
- env_nand.c fails when using an unaligned offset. It produces an
  error in nand_erase_opts{drivers/mtd/nand/nand_util.c}
- in env_sf/env_flash the unused space at the end is preserved, but
  not in the beginning. block alignment is assumed
- env_sata/env_mmc aligns offset/length to the block size of the
  underlying device. data is silently redirected to the beginning of
  a block

There is seems no use case for unaligned environment. If there is
some useful data at the beginning of the the block (e.g. end of u-boot)
that would be very unsafe. If the redundant environments are hosted by
the same erase block then that invalidates the idea of double buffering.
It might be that unaligned access was allowed in the past, and that
people with legacy u-boot are trapped. But at the time of 56086921
it wasn't supported and due to reasons above I guess it was never
introduced.
I prefer to remove that (unused) feature in favor of simplicity

Signed-off-by: Andreas Fenkart <andreas.fenkart@digitalstrom.com>
---
 tools/env/fw_env.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
index 6b0dcaa..d2b167d 100644
--- a/tools/env/fw_env.c
+++ b/tools/env/fw_env.c
@@ -1294,6 +1294,18 @@ static int check_device_config(int dev)
 	struct stat st;
 	int fd, rc = 0;
 
+	if (DEVOFFSET(dev) % DEVESIZE(dev) != 0) {
+		fprintf(stderr, "Environment does not start on erase block boundary\n");
+		errno = EINVAL;
+		return -1;
+	}
+
+	if (ENVSIZE(dev) > ENVSECTORS(dev) * DEVESIZE(dev)) {
+		fprintf(stderr, "Environment does not fit into available sectors\n");
+		errno = EINVAL;
+		return -1;
+	}
+
 	fd = open(DEVNAME(dev), O_RDONLY);
 	if (fd < 0) {
 		fprintf(stderr,
-- 
2.8.1

[U-Boot] [RFC: v2] tools/env: ensure environment starts at erase block boundary

[Stefan Agner]

On 2016-08-11 12:39, Andreas Fenkart wrote:
> 56086921 added support for unaligned environments access.
> U-boot itself does not support this:
> - env_nand.c fails when using an unaligned offset. It produces an
>   error in nand_erase_opts{drivers/mtd/nand/nand_util.c}
> - in env_sf/env_flash the unused space at the end is preserved, but
>   not in the beginning. block alignment is assumed
> - env_sata/env_mmc aligns offset/length to the block size of the
>   underlying device. data is silently redirected to the beginning of
>   a block
> 
> There is seems no use case for unaligned environment. If there is
> some useful data at the beginning of the the block (e.g. end of u-boot)
> that would be very unsafe. If the redundant environments are hosted by
> the same erase block then that invalidates the idea of double buffering.
> It might be that unaligned access was allowed in the past, and that
> people with legacy u-boot are trapped. But at the time of 56086921
> it wasn't supported and due to reasons above I guess it was never
> introduced.
> I prefer to remove that (unused) feature in favor of simplicity

I also don't see any value supporting unaligned environment, so FWIW:

Acked-by: Stefan Agner <stefan.agner@toradex.com>

Small nit below:

> 
> Signed-off-by: Andreas Fenkart <andreas.fenkart@digitalstrom.com>
> ---
>  tools/env/fw_env.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
> index 6b0dcaa..d2b167d 100644
> --- a/tools/env/fw_env.c
> +++ b/tools/env/fw_env.c
> @@ -1294,6 +1294,18 @@ static int check_device_config(int dev)
>  	struct stat st;
>  	int fd, rc = 0;
>  
> +	if (DEVOFFSET(dev) % DEVESIZE(dev) != 0) {
> +		fprintf(stderr, "Environment does not start on erase block boundary\n");

Erase block is sometwhat confusing in the MMC context, maybe
parenthesize "erase"?

--
Stefan

> +		errno = EINVAL;
> +		return -1;
> +	}
> +
> +	if (ENVSIZE(dev) > ENVSECTORS(dev) * DEVESIZE(dev)) {
> +		fprintf(stderr, "Environment does not fit into available sectors\n");
> +		errno = EINVAL;
> +		return -1;
> +	}
> +
>  	fd = open(DEVNAME(dev), O_RDONLY);
>  	if (fd < 0) {
>  		fprintf(stderr,

[U-Boot] [U-Boot, RFC:, v2] tools/env: ensure environment starts at erase block boundary

[Tom Rini]

On Thu, Aug 11, 2016 at 09:39:17PM +0200, Andreas Fenkart wrote:

> 56086921 added support for unaligned environments access.
> U-boot itself does not support this:
> - env_nand.c fails when using an unaligned offset. It produces an
>   error in nand_erase_opts{drivers/mtd/nand/nand_util.c}
> - in env_sf/env_flash the unused space at the end is preserved, but
>   not in the beginning. block alignment is assumed
> - env_sata/env_mmc aligns offset/length to the block size of the
>   underlying device. data is silently redirected to the beginning of
>   a block
> 
> There is seems no use case for unaligned environment. If there is
> some useful data at the beginning of the the block (e.g. end of u-boot)
> that would be very unsafe. If the redundant environments are hosted by
> the same erase block then that invalidates the idea of double buffering.
> It might be that unaligned access was allowed in the past, and that
> people with legacy u-boot are trapped. But at the time of 56086921
> it wasn't supported and due to reasons above I guess it was never
> introduced.
> I prefer to remove that (unused) feature in favor of simplicity
> 
> Signed-off-by: Andreas Fenkart <andreas.fenkart@digitalstrom.com>
> Acked-by: Stefan Agner <stefan.agner@toradex.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160815/5d9cae0e/attachment.sig>