[U-Boot] [PATCH v2] tools/env: fix environment alignment tests for block devices

[U-Boot] [PATCH v2] tools/env: fix environment alignment tests for block devices

[Max Krummenacher]

commit 183923d3e412500bdc597d1745e2fb6f7f679ec7 enforces that the
environment must start at an erase block boundary.

For block devices the sample fw_env.config does not mandate a erase block size
for block devices. A missing setting defaults to the full env size.

Depending on the environment location the alignment check now errors out for
perfectly legal settings.

Fix this by defaulting to the standard blocksize of 0x200 for environments
stored in a block device.
That keeps the fw_env.config files for block devices working even with that
new check.

Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>

---

Changes in v2:
- move default value handling from parse_config(), get_config() to
  check_device_config(), so that !defined(CONFIG_FILE) is covered also.
- use DIV_ROUND_UP instead of doing this manually
- move the check after the setting of default values in check_device_config().
- use 0 as the marker for default values to be used.

 tools/env/fw_env.c | 60 ++++++++++++++++++++++++++++++------------------------
 1 file changed, 33 insertions(+), 27 deletions(-)

diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
index 3dc0d53..862a0b1 100644
--- a/tools/env/fw_env.c
+++ b/tools/env/fw_env.c
@@ -1291,18 +1291,6 @@ 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,
@@ -1335,9 +1323,15 @@ static int check_device_config(int dev)
 			goto err;
 		}
 		DEVTYPE(dev) = mtdinfo.type;
+		if (DEVESIZE(dev) == 0)
+			/* Assume the erase size is the same as the env-size */
+			DEVESIZE(dev) = ENVSIZE(dev);
 	} else {
 		uint64_t size;
 		DEVTYPE(dev) = MTD_ABSENT;
+		if (DEVESIZE(dev) == 0)
+			/* Assume the erase size to be 512 bytes */
+			DEVESIZE(dev) = 0x200;
 
 		/*
 		 * Check for negative offsets, treat it as backwards offset
@@ -1359,6 +1353,22 @@ static int check_device_config(int dev)
 		}
 	}
 
+	if (ENVSECTORS(dev) == 0)
+		/* Assume enough sectors to cover the environment */
+		ENVSECTORS(dev) = DIV_ROUND_UP(ENVSIZE(dev), DEVESIZE(dev));
+
+	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;
+	}
+
 err:
 	close(fd);
 	return rc;
@@ -1382,10 +1392,10 @@ static int parse_config(struct env_opts *opts)
 	DEVNAME (0) = DEVICE1_NAME;
 	DEVOFFSET (0) = DEVICE1_OFFSET;
 	ENVSIZE (0) = ENV1_SIZE;
-	/* Default values are: erase-size=env-size */
-	DEVESIZE (0) = ENVSIZE (0);
-	/* #sectors=env-size/erase-size (rounded up) */
-	ENVSECTORS (0) = (ENVSIZE(0) + DEVESIZE(0) - 1) / DEVESIZE(0);
+
+	/* Set defaults for DEVESIZE, ENVSECTORS later once we
+	 * know DEVTYPE
+	 */
 #ifdef DEVICE1_ESIZE
 	DEVESIZE (0) = DEVICE1_ESIZE;
 #endif
@@ -1397,10 +1407,10 @@ static int parse_config(struct env_opts *opts)
 	DEVNAME (1) = DEVICE2_NAME;
 	DEVOFFSET (1) = DEVICE2_OFFSET;
 	ENVSIZE (1) = ENV2_SIZE;
-	/* Default values are: erase-size=env-size */
-	DEVESIZE (1) = ENVSIZE (1);
-	/* #sectors=env-size/erase-size (rounded up) */
-	ENVSECTORS (1) = (ENVSIZE(1) + DEVESIZE(1) - 1) / DEVESIZE(1);
+
+	/* Set defaults for DEVESIZE, ENVSECTORS later once we
+	 * know DEVTYPE
+	 */
 #ifdef DEVICE2_ESIZE
 	DEVESIZE (1) = DEVICE2_ESIZE;
 #endif
@@ -1466,13 +1476,9 @@ static int get_config (char *fname)
 
 		DEVNAME(i) = devname;
 
-		if (rc < 4)
-			/* Assume the erase size is the same as the env-size */
-			DEVESIZE(i) = ENVSIZE(i);
-
-		if (rc < 5)
-			/* Assume enough env sectors to cover the environment */
-			ENVSECTORS (i) = (ENVSIZE(i) + DEVESIZE(i) - 1) / DEVESIZE(i);
+		/* Set defaults for DEVESIZE, ENVSECTORS later once we
+		 * know DEVTYPE
+		 */
 
 		i++;
 	}
-- 
2.5.5

[U-Boot] [PATCH v2] tools/env: fix environment alignment tests for block devices

[Max Krummenacher]

Hi

Any news on this?
The env utility is currently broken for block devices.
Alternatively we could also revert commit
183923d3e412500bdc597d1745e2fb6f7f679ec7.

Regards
Max

[U-Boot] [PATCH v2] tools/env: fix environment alignment tests for block devices

[Andreas Fenkart]

Hi Max,

LGTM, see one nit below, can fixed later

On 11/19/2016 01:58 PM, Max Krummenacher wrote:
> commit 183923d3e412500bdc597d1745e2fb6f7f679ec7 enforces that the
> environment must start at an erase block boundary.
>
> For block devices the sample fw_env.config does not mandate a erase block size
> for block devices. A missing setting defaults to the full env size.
>
> Depending on the environment location the alignment check now errors out for
> perfectly legal settings.
>
> Fix this by defaulting to the standard blocksize of 0x200 for environments
> stored in a block device.
> That keeps the fw_env.config files for block devices working even with that
> new check.
>
> Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
>
> ---
>
> Changes in v2:
> - move default value handling from parse_config(), get_config() to
>    check_device_config(), so that !defined(CONFIG_FILE) is covered also.
> - use DIV_ROUND_UP instead of doing this manually
> - move the check after the setting of default values in check_device_config().
> - use 0 as the marker for default values to be used.
>
>   tools/env/fw_env.c | 60 ++++++++++++++++++++++++++++++------------------------
>   1 file changed, 33 insertions(+), 27 deletions(-)
>
> diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
> index 3dc0d53..862a0b1 100644
> --- a/tools/env/fw_env.c
> +++ b/tools/env/fw_env.c
> @@ -1291,18 +1291,6 @@ 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,
> @@ -1335,9 +1323,15 @@ static int check_device_config(int dev)
>   			goto err;
>   		}
>   		DEVTYPE(dev) = mtdinfo.type;
> +		if (DEVESIZE(dev) == 0)
> +			/* Assume the erase size is the same as the env-size */
> +			DEVESIZE(dev) = ENVSIZE(dev);
Since we already checked for character devices, we could use the 
mtdinfo.erasesize. I guess we can relay on mtdinfo on new kernels, and 
if not there is still the fallback to specify it in fw_env.config.

>   	} else {
>   		uint64_t size;
>   		DEVTYPE(dev) = MTD_ABSENT;
> +		if (DEVESIZE(dev) == 0)
> +			/* Assume the erase size to be 512 bytes */
> +			DEVESIZE(dev) = 0x200;
It doesn't even matter. In case of MTD_ABSENT, erasize is never used 
itself, only the tuple (DEVESIZE * ENVSECTORS) matters.

>   
>   		/*
>   		 * Check for negative offsets, treat it as backwards offset
> @@ -1359,6 +1353,22 @@ static int check_device_config(int dev)
>   		}
>   	}
>   
> +	if (ENVSECTORS(dev) == 0)
> +		/* Assume enough sectors to cover the environment */
> +		ENVSECTORS(dev) = DIV_ROUND_UP(ENVSIZE(dev), DEVESIZE(dev));
> +
> +	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;
> +	}
> +
>   err:
>   	close(fd);
>   	return rc;
> @@ -1382,10 +1392,10 @@ static int parse_config(struct env_opts *opts)
>   	DEVNAME (0) = DEVICE1_NAME;
>   	DEVOFFSET (0) = DEVICE1_OFFSET;
>   	ENVSIZE (0) = ENV1_SIZE;
> -	/* Default values are: erase-size=env-size */
> -	DEVESIZE (0) = ENVSIZE (0);
> -	/* #sectors=env-size/erase-size (rounded up) */
> -	ENVSECTORS (0) = (ENVSIZE(0) + DEVESIZE(0) - 1) / DEVESIZE(0);
> +
> +	/* Set defaults for DEVESIZE, ENVSECTORS later once we
> +	 * know DEVTYPE
> +	 */
>   #ifdef DEVICE1_ESIZE
>   	DEVESIZE (0) = DEVICE1_ESIZE;
>   #endif
> @@ -1397,10 +1407,10 @@ static int parse_config(struct env_opts *opts)
>   	DEVNAME (1) = DEVICE2_NAME;
>   	DEVOFFSET (1) = DEVICE2_OFFSET;
>   	ENVSIZE (1) = ENV2_SIZE;
> -	/* Default values are: erase-size=env-size */
> -	DEVESIZE (1) = ENVSIZE (1);
> -	/* #sectors=env-size/erase-size (rounded up) */
> -	ENVSECTORS (1) = (ENVSIZE(1) + DEVESIZE(1) - 1) / DEVESIZE(1);
> +
> +	/* Set defaults for DEVESIZE, ENVSECTORS later once we
> +	 * know DEVTYPE
> +	 */
>   #ifdef DEVICE2_ESIZE
>   	DEVESIZE (1) = DEVICE2_ESIZE;
>   #endif
> @@ -1466,13 +1476,9 @@ static int get_config (char *fname)
>   
>   		DEVNAME(i) = devname;
>   
> -		if (rc < 4)
> -			/* Assume the erase size is the same as the env-size */
> -			DEVESIZE(i) = ENVSIZE(i);
> -
> -		if (rc < 5)
> -			/* Assume enough env sectors to cover the environment */
> -			ENVSECTORS (i) = (ENVSIZE(i) + DEVESIZE(i) - 1) / DEVESIZE(i);
> +		/* Set defaults for DEVESIZE, ENVSECTORS later once we
> +		 * know DEVTYPE
> +		 */
>   
>   		i++;
>   	}

[U-Boot] [PATCH v2] tools/env: fix environment alignment tests for block devices

[Max Krummenacher]

Hi Andreas

2016-11-28 10:47 GMT+01:00 Andreas Fenkart <andreas.fenkart@digitalstrom.com>:
> Hi Max,
>
> LGTM, see one nit below, can fixed later
>
>
> On 11/19/2016 01:58 PM, Max Krummenacher wrote:
>>
>> commit 183923d3e412500bdc597d1745e2fb6f7f679ec7 enforces that the
>> environment must start at an erase block boundary.
>>
>> For block devices the sample fw_env.config does not mandate a erase block
>> size
>> for block devices. A missing setting defaults to the full env size.
>>
>>   tools/env/fw_env.c | 60
...
>>                 }
>>                 DEVTYPE(dev) = mtdinfo.type;
>> +               if (DEVESIZE(dev) == 0)
>> +                       /* Assume the erase size is the same as the
>> env-size */
>> +                       DEVESIZE(dev) = ENVSIZE(dev);
>
> Since we already checked for character devices, we could use the
> mtdinfo.erasesize. I guess we can relay on mtdinfo on new kernels, and if
> not there is still the fallback to specify it in fw_env.config.

Agreed, however since that changes functionallity I think this must go
into a follow up patch.
Also, since the config file skeleton mandates a erase size I don't see
a pressing need.

>
>>         } else {
>>                 uint64_t size;
>>                 DEVTYPE(dev) = MTD_ABSENT;
>> +               if (DEVESIZE(dev) == 0)
>> +                       /* Assume the erase size to be 512 bytes */
>> +                       DEVESIZE(dev) = 0x200;
>
> It doesn't even matter. In case of MTD_ABSENT, erasize is never used itself,
> only the tuple (DEVESIZE * ENVSECTORS) matters.

That is not true. with the test for DEVOFFSET being aligned to
DEVESIZE the previous used default broke env with certain legal config
files.
Other than this new test I agree.

Max

[U-Boot] [U-Boot, v2] tools/env: fix environment alignment tests for block devices

[Tom Rini]

On Sat, Nov 19, 2016 at 01:58:56PM +0100, Max Krummenacher wrote:

> commit 183923d3e412500bdc597d1745e2fb6f7f679ec7 enforces that the
> environment must start at an erase block boundary.
> 
> For block devices the sample fw_env.config does not mandate a erase block size
> for block devices. A missing setting defaults to the full env size.
> 
> Depending on the environment location the alignment check now errors out for
> perfectly legal settings.
> 
> Fix this by defaulting to the standard blocksize of 0x200 for environments
> stored in a block device.
> That keeps the fw_env.config files for block devices working even with that
> new check.
> 
> Signed-off-by: Max Krummenacher <max.krummenacher@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/20161128/8d8fca51/attachment.sig>