[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 at 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++;
>   	}