[PATCH] env: sf: single function env_sf_save()

[PATCH] env: sf: single function env_sf_save()

[Harry Waschkeit]

Instead of implementing redundant environments in two very similar
functions env_sf_save(), handle redundancy in one function, placing the
few differences in appropriate pre-compiler sections depending on config
option CONFIG_ENV_OFFSET_REDUND.

Additionally, several checkpatch complaints were addressed.

This patch is in preparation for adding support for env erase.

Signed-off-by: Harry Waschkeit <Harry.Waschkeit@men.de>
---
 env/sf.c | 132 ++++++++++++++++++-------------------------------------
 1 file changed, 43 insertions(+), 89 deletions(-)

diff --git a/env/sf.c b/env/sf.c
index 937778aa37..c60bd1deed 100644
--- a/env/sf.c
+++ b/env/sf.c
@@ -66,13 +66,16 @@ static int setup_flash_device(void)
 	return 0;
 }
 
-#if defined(CONFIG_ENV_OFFSET_REDUND)
 static int env_sf_save(void)
 {
 	env_t	env_new;
-	char	*saved_buffer = NULL, flag = ENV_REDUND_OBSOLETE;
+	char	*saved_buffer = NULL;
 	u32	saved_size, saved_offset, sector;
+	ulong	offset;
 	int	ret;
+#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
+	char	flag = ENV_REDUND_OBSOLETE;
+#endif
 
 	ret = setup_flash_device();
 	if (ret)
@@ -81,27 +84,33 @@ static int env_sf_save(void)
 	ret = env_export(&env_new);
 	if (ret)
 		return -EIO;
-	env_new.flags	= ENV_REDUND_ACTIVE;
 
-	if (gd->env_valid == ENV_VALID) {
-		env_new_offset = CONFIG_ENV_OFFSET_REDUND;
-		env_offset = CONFIG_ENV_OFFSET;
-	} else {
-		env_new_offset = CONFIG_ENV_OFFSET;
-		env_offset = CONFIG_ENV_OFFSET_REDUND;
-	}
+#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
+		env_new.flags	= ENV_REDUND_ACTIVE;
+
+		if (gd->env_valid == ENV_VALID) {
+			env_new_offset = CONFIG_ENV_OFFSET_REDUND;
+			env_offset = CONFIG_ENV_OFFSET;
+		} else {
+			env_new_offset = CONFIG_ENV_OFFSET;
+			env_offset = CONFIG_ENV_OFFSET_REDUND;
+		}
+		offset = env_new_offset;
+#else
+		offset = CONFIG_ENV_OFFSET;
+#endif
 
 	/* Is the sector larger than the env (i.e. embedded) */
 	if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
 		saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE;
-		saved_offset = env_new_offset + CONFIG_ENV_SIZE;
+		saved_offset = offset + CONFIG_ENV_SIZE;
 		saved_buffer = memalign(ARCH_DMA_MINALIGN, saved_size);
 		if (!saved_buffer) {
 			ret = -ENOMEM;
 			goto done;
 		}
-		ret = spi_flash_read(env_flash, saved_offset,
-					saved_size, saved_buffer);
+		ret = spi_flash_read(env_flash, saved_offset, saved_size,
+				     saved_buffer);
 		if (ret)
 			goto done;
 	}
@@ -109,35 +118,39 @@ static int env_sf_save(void)
 	sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
 
 	puts("Erasing SPI flash...");
-	ret = spi_flash_erase(env_flash, env_new_offset,
-				sector * CONFIG_ENV_SECT_SIZE);
+	ret = spi_flash_erase(env_flash, offset,
+			      sector * CONFIG_ENV_SECT_SIZE);
 	if (ret)
 		goto done;
 
 	puts("Writing to SPI flash...");
 
-	ret = spi_flash_write(env_flash, env_new_offset,
-		CONFIG_ENV_SIZE, &env_new);
+	ret = spi_flash_write(env_flash, offset,
+			      CONFIG_ENV_SIZE, &env_new);
 	if (ret)
 		goto done;
 
 	if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
-		ret = spi_flash_write(env_flash, saved_offset,
-					saved_size, saved_buffer);
+		ret = spi_flash_write(env_flash, saved_offset, saved_size,
+				      saved_buffer);
 		if (ret)
 			goto done;
 	}
 
-	ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags),
-				sizeof(env_new.flags), &flag);
-	if (ret)
-		goto done;
+#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
+		ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags),
+				      sizeof(env_new.flags), &flag);
+		if (ret)
+			goto done;
 
-	puts("done\n");
+		puts("done\n");
 
-	gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND;
+		gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND;
 
-	printf("Valid environment: %d\n", (int)gd->env_valid);
+		printf("Valid environment: %d\n", (int)gd->env_valid);
+#else
+		puts("done\n");
+#endif
 
  done:
 	if (saved_buffer)
@@ -146,6 +159,7 @@ static int env_sf_save(void)
 	return ret;
 }
 
+#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
 static int env_sf_load(void)
 {
 	int ret;
@@ -183,66 +197,6 @@ out:
 	return ret;
 }
 #else
-static int env_sf_save(void)
-{
-	u32	saved_size, saved_offset, sector;
-	char	*saved_buffer = NULL;
-	int	ret = 1;
-	env_t	env_new;
-
-	ret = setup_flash_device();
-	if (ret)
-		return ret;
-
-	/* Is the sector larger than the env (i.e. embedded) */
-	if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
-		saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE;
-		saved_offset = CONFIG_ENV_OFFSET + CONFIG_ENV_SIZE;
-		saved_buffer = malloc(saved_size);
-		if (!saved_buffer)
-			goto done;
-
-		ret = spi_flash_read(env_flash, saved_offset,
-			saved_size, saved_buffer);
-		if (ret)
-			goto done;
-	}
-
-	ret = env_export(&env_new);
-	if (ret)
-		goto done;
-
-	sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
-
-	puts("Erasing SPI flash...");
-	ret = spi_flash_erase(env_flash, CONFIG_ENV_OFFSET,
-		sector * CONFIG_ENV_SECT_SIZE);
-	if (ret)
-		goto done;
-
-	puts("Writing to SPI flash...");
-	ret = spi_flash_write(env_flash, CONFIG_ENV_OFFSET,
-		CONFIG_ENV_SIZE, &env_new);
-	if (ret)
-		goto done;
-
-	if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
-		ret = spi_flash_write(env_flash, saved_offset,
-			saved_size, saved_buffer);
-		if (ret)
-			goto done;
-	}
-
-	ret = 0;
-	puts("done\n");
-
- done:
-	if (saved_buffer)
-		free(saved_buffer);
-
-	return ret;
-}
-
 static int env_sf_load(void)
 {
 	int ret;
@@ -258,8 +212,8 @@ static int env_sf_load(void)
 	if (ret)
 		goto out;
 
-	ret = spi_flash_read(env_flash,
-		CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, buf);
+	ret = spi_flash_read(env_flash, CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE,
+			     buf);
 	if (ret) {
 		env_set_default("spi_flash_read() failed", 0);
 		goto err_read;
@@ -292,7 +246,7 @@ static int env_sf_init(void)
 	env_t *env_ptr = (env_t *)env_sf_get_env_addr();
 
 	if (crc32(0, env_ptr->data, ENV_SIZE) == env_ptr->crc) {
-		gd->env_addr	= (ulong)&(env_ptr->data);
+		gd->env_addr	= (ulong)&env_ptr->data;
 		gd->env_valid	= 1;
 	} else {
 		gd->env_addr = (ulong)&default_environment[0];
-- 
2.29.0

[PATCH] env: sf: single function env_sf_save()

[Stefan Roese]

Hi Harry,

On 28.01.21 08:21, Harry Waschkeit wrote:
> Instead of implementing redundant environments in two very similar
> functions env_sf_save(), handle redundancy in one function, placing the
> few differences in appropriate pre-compiler sections depending on config
> option CONFIG_ENV_OFFSET_REDUND.
> 
> Additionally, several checkpatch complaints were addressed.
> 
> This patch is in preparation for adding support for env erase.
> 
> Signed-off-by: Harry Waschkeit <Harry.Waschkeit@men.de>

I like this idea very much, thanks for working on this. One comment
below..

> ---
>   env/sf.c | 132 ++++++++++++++++++-------------------------------------
>   1 file changed, 43 insertions(+), 89 deletions(-)
> 
> diff --git a/env/sf.c b/env/sf.c
> index 937778aa37..c60bd1deed 100644
> --- a/env/sf.c
> +++ b/env/sf.c
> @@ -66,13 +66,16 @@ static int setup_flash_device(void)
>   	return 0;
>   }
>   
> -#if defined(CONFIG_ENV_OFFSET_REDUND)
>   static int env_sf_save(void)
>   {
>   	env_t	env_new;
> -	char	*saved_buffer = NULL, flag = ENV_REDUND_OBSOLETE;
> +	char	*saved_buffer = NULL;
>   	u32	saved_size, saved_offset, sector;
> +	ulong	offset;
>   	int	ret;
> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
> +	char	flag = ENV_REDUND_OBSOLETE;
> +#endif

No more #idef's if possible please. See below...

>   
>   	ret = setup_flash_device();
>   	if (ret)
> @@ -81,27 +84,33 @@ static int env_sf_save(void)
>   	ret = env_export(&env_new);
>   	if (ret)
>   		return -EIO;
> -	env_new.flags	= ENV_REDUND_ACTIVE;
>   
> -	if (gd->env_valid == ENV_VALID) {
> -		env_new_offset = CONFIG_ENV_OFFSET_REDUND;
> -		env_offset = CONFIG_ENV_OFFSET;
> -	} else {
> -		env_new_offset = CONFIG_ENV_OFFSET;
> -		env_offset = CONFIG_ENV_OFFSET_REDUND;
> -	}
> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
> +		env_new.flags	= ENV_REDUND_ACTIVE;
> +
> +		if (gd->env_valid == ENV_VALID) {
> +			env_new_offset = CONFIG_ENV_OFFSET_REDUND;
> +			env_offset = CONFIG_ENV_OFFSET;
> +		} else {
> +			env_new_offset = CONFIG_ENV_OFFSET;
> +			env_offset = CONFIG_ENV_OFFSET_REDUND;
> +		}
> +		offset = env_new_offset;
> +#else
> +		offset = CONFIG_ENV_OFFSET;
> +#endif

Can you please try to add this without adding #ifdef's but using
if (CONFIG_IS_ENABLE(SYS_REDUNDAND_ENVIRONMENT)) instead?

Thanks,
Stefan

>   
>   	/* Is the sector larger than the env (i.e. embedded) */
>   	if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>   		saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE;
> -		saved_offset = env_new_offset + CONFIG_ENV_SIZE;
> +		saved_offset = offset + CONFIG_ENV_SIZE;
>   		saved_buffer = memalign(ARCH_DMA_MINALIGN, saved_size);
>   		if (!saved_buffer) {
>   			ret = -ENOMEM;
>   			goto done;
>   		}
> -		ret = spi_flash_read(env_flash, saved_offset,
> -					saved_size, saved_buffer);
> +		ret = spi_flash_read(env_flash, saved_offset, saved_size,
> +				     saved_buffer);
>   		if (ret)
>   			goto done;
>   	}
> @@ -109,35 +118,39 @@ static int env_sf_save(void)
>   	sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
>   
>   	puts("Erasing SPI flash...");
> -	ret = spi_flash_erase(env_flash, env_new_offset,
> -				sector * CONFIG_ENV_SECT_SIZE);
> +	ret = spi_flash_erase(env_flash, offset,
> +			      sector * CONFIG_ENV_SECT_SIZE);
>   	if (ret)
>   		goto done;
>   
>   	puts("Writing to SPI flash...");
>   
> -	ret = spi_flash_write(env_flash, env_new_offset,
> -		CONFIG_ENV_SIZE, &env_new);
> +	ret = spi_flash_write(env_flash, offset,
> +			      CONFIG_ENV_SIZE, &env_new);
>   	if (ret)
>   		goto done;
>   
>   	if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
> -		ret = spi_flash_write(env_flash, saved_offset,
> -					saved_size, saved_buffer);
> +		ret = spi_flash_write(env_flash, saved_offset, saved_size,
> +				      saved_buffer);
>   		if (ret)
>   			goto done;
>   	}
>   
> -	ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags),
> -				sizeof(env_new.flags), &flag);
> -	if (ret)
> -		goto done;
> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
> +		ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags),
> +				      sizeof(env_new.flags), &flag);
> +		if (ret)
> +			goto done;
>   
> -	puts("done\n");
> +		puts("done\n");
>   
> -	gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND;
> +		gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND;
>   
> -	printf("Valid environment: %d\n", (int)gd->env_valid);
> +		printf("Valid environment: %d\n", (int)gd->env_valid);
> +#else
> +		puts("done\n");
> +#endif
>   
>    done:
>   	if (saved_buffer)
> @@ -146,6 +159,7 @@ static int env_sf_save(void)
>   	return ret;
>   }
>   
> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>   static int env_sf_load(void)
>   {
>   	int ret;
> @@ -183,66 +197,6 @@ out:
>   	return ret;
>   }
>   #else
> -static int env_sf_save(void)
> -{
> -	u32	saved_size, saved_offset, sector;
> -	char	*saved_buffer = NULL;
> -	int	ret = 1;
> -	env_t	env_new;
> -
> -	ret = setup_flash_device();
> -	if (ret)
> -		return ret;
> -
> -	/* Is the sector larger than the env (i.e. embedded) */
> -	if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
> -		saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE;
> -		saved_offset = CONFIG_ENV_OFFSET + CONFIG_ENV_SIZE;
> -		saved_buffer = malloc(saved_size);
> -		if (!saved_buffer)
> -			goto done;
> -
> -		ret = spi_flash_read(env_flash, saved_offset,
> -			saved_size, saved_buffer);
> -		if (ret)
> -			goto done;
> -	}
> -
> -	ret = env_export(&env_new);
> -	if (ret)
> -		goto done;
> -
> -	sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
> -
> -	puts("Erasing SPI flash...");
> -	ret = spi_flash_erase(env_flash, CONFIG_ENV_OFFSET,
> -		sector * CONFIG_ENV_SECT_SIZE);
> -	if (ret)
> -		goto done;
> -
> -	puts("Writing to SPI flash...");
> -	ret = spi_flash_write(env_flash, CONFIG_ENV_OFFSET,
> -		CONFIG_ENV_SIZE, &env_new);
> -	if (ret)
> -		goto done;
> -
> -	if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
> -		ret = spi_flash_write(env_flash, saved_offset,
> -			saved_size, saved_buffer);
> -		if (ret)
> -			goto done;
> -	}
> -
> -	ret = 0;
> -	puts("done\n");
> -
> - done:
> -	if (saved_buffer)
> -		free(saved_buffer);
> -
> -	return ret;
> -}
> -
>   static int env_sf_load(void)
>   {
>   	int ret;
> @@ -258,8 +212,8 @@ static int env_sf_load(void)
>   	if (ret)
>   		goto out;
>   
> -	ret = spi_flash_read(env_flash,
> -		CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, buf);
> +	ret = spi_flash_read(env_flash, CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE,
> +			     buf);
>   	if (ret) {
>   		env_set_default("spi_flash_read() failed", 0);
>   		goto err_read;
> @@ -292,7 +246,7 @@ static int env_sf_init(void)
>   	env_t *env_ptr = (env_t *)env_sf_get_env_addr();
>   
>   	if (crc32(0, env_ptr->data, ENV_SIZE) == env_ptr->crc) {
> -		gd->env_addr	= (ulong)&(env_ptr->data);
> +		gd->env_addr	= (ulong)&env_ptr->data;
>   		gd->env_valid	= 1;
>   	} else {
>   		gd->env_addr = (ulong)&default_environment[0];
> 


Viele Gr??e,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de

[PATCH] env: sf: single function env_sf_save()

[Harry Waschkeit]

?Hi Stefan,

thanks a lot for your prompt reply :-)

And sorry that I didn't manage to continue on that for such a long time ...


On 28.01.21 09:50, Stefan Roese wrote:
> Hi Harry,
> 
> On 28.01.21 08:21, Harry Waschkeit wrote:
>> Instead of implementing redundant environments in two very similar
>> functions env_sf_save(), handle redundancy in one function, placing the
>> few differences in appropriate pre-compiler sections depending on config
>> option CONFIG_ENV_OFFSET_REDUND.
>>
>> Additionally, several checkpatch complaints were addressed.
>>
>> This patch is in preparation for adding support for env erase.
>>
>> Signed-off-by: Harry Waschkeit <Harry.Waschkeit@men.de>
> 
> I like this idea very much, thanks for working on this. One comment
> below..
> 
>> ---
>> ? env/sf.c | 132 ++++++++++++++++++-------------------------------------
>> ? 1 file changed, 43 insertions(+), 89 deletions(-)
>>
>> diff --git a/env/sf.c b/env/sf.c
>> index 937778aa37..c60bd1deed 100644
>> --- a/env/sf.c
>> +++ b/env/sf.c
>> @@ -66,13 +66,16 @@ static int setup_flash_device(void)
>> ????? return 0;
>> ? }
>> -#if defined(CONFIG_ENV_OFFSET_REDUND)
>> ? static int env_sf_save(void)
>> ? {
>> ????? env_t??? env_new;
>> -??? char??? *saved_buffer = NULL, flag = ENV_REDUND_OBSOLETE;
>> +??? char??? *saved_buffer = NULL;
>> ????? u32??? saved_size, saved_offset, sector;
>> +??? ulong??? offset;
>> ????? int??? ret;
>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>> +??? char??? flag = ENV_REDUND_OBSOLETE;
>> +#endif
> 
> No more #idef's if possible please. See below...

As checkpatch.pl throws warnings for each such #if[def] occurrence, I already tried to get rid of them.
But I can't see how this shall be possible in a structure declaration.
I'm eager to learn how to do that if possible, though :-)

> 
>> ????? ret = setup_flash_device();
>> ????? if (ret)
>> @@ -81,27 +84,33 @@ static int env_sf_save(void)
>> ????? ret = env_export(&env_new);
>> ????? if (ret)
>> ????????? return -EIO;
>> -??? env_new.flags??? = ENV_REDUND_ACTIVE;
>> -??? if (gd->env_valid == ENV_VALID) {
>> -??????? env_new_offset = CONFIG_ENV_OFFSET_REDUND;
>> -??????? env_offset = CONFIG_ENV_OFFSET;
>> -??? } else {
>> -??????? env_new_offset = CONFIG_ENV_OFFSET;
>> -??????? env_offset = CONFIG_ENV_OFFSET_REDUND;
>> -??? }
>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>> +??????? env_new.flags??? = ENV_REDUND_ACTIVE;
>> +
>> +??????? if (gd->env_valid == ENV_VALID) {
>> +??????????? env_new_offset = CONFIG_ENV_OFFSET_REDUND;
>> +??????????? env_offset = CONFIG_ENV_OFFSET;
>> +??????? } else {
>> +??????????? env_new_offset = CONFIG_ENV_OFFSET;
>> +??????????? env_offset = CONFIG_ENV_OFFSET_REDUND;
>> +??????? }
>> +??????? offset = env_new_offset;
>> +#else
>> +??????? offset = CONFIG_ENV_OFFSET;
>> +#endif
> 
> Can you please try to add this without adding #ifdef's but using
> if (CONFIG_IS_ENABLE(SYS_REDUNDAND_ENVIRONMENT)) instead?

I tried that, too, but it was doomed to fail, because element flags in env_t structure is only defined if redundant environment is enabled.

Even though an "if (CONFIG_IS_ENABLED(...))" would statically evaluate to '0' without active redundancy in environments, the parser sees the syntax error of the non-existing structure element.

So, I don't see a chance for avoiding the #if construct here, too.
Let me know if I miss something :-)

Cheers,
Harry

> Thanks,
> Stefan
> 
>> ????? /* Is the sector larger than the env (i.e. embedded) */
>> ????? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>> ????????? saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE;
>> -??????? saved_offset = env_new_offset + CONFIG_ENV_SIZE;
>> +??????? saved_offset = offset + CONFIG_ENV_SIZE;
>> ????????? saved_buffer = memalign(ARCH_DMA_MINALIGN, saved_size);
>> ????????? if (!saved_buffer) {
>> ????????????? ret = -ENOMEM;
>> ????????????? goto done;
>> ????????? }
>> -??????? ret = spi_flash_read(env_flash, saved_offset,
>> -??????????????????? saved_size, saved_buffer);
>> +??????? ret = spi_flash_read(env_flash, saved_offset, saved_size,
>> +???????????????????? saved_buffer);
>> ????????? if (ret)
>> ????????????? goto done;
>> ????? }
>> @@ -109,35 +118,39 @@ static int env_sf_save(void)
>> ????? sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
>> ????? puts("Erasing SPI flash...");
>> -??? ret = spi_flash_erase(env_flash, env_new_offset,
>> -??????????????? sector * CONFIG_ENV_SECT_SIZE);
>> +??? ret = spi_flash_erase(env_flash, offset,
>> +????????????????? sector * CONFIG_ENV_SECT_SIZE);
>> ????? if (ret)
>> ????????? goto done;
>> ????? puts("Writing to SPI flash...");
>> -??? ret = spi_flash_write(env_flash, env_new_offset,
>> -??????? CONFIG_ENV_SIZE, &env_new);
>> +??? ret = spi_flash_write(env_flash, offset,
>> +????????????????? CONFIG_ENV_SIZE, &env_new);
>> ????? if (ret)
>> ????????? goto done;
>> ????? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>> -??????? ret = spi_flash_write(env_flash, saved_offset,
>> -??????????????????? saved_size, saved_buffer);
>> +??????? ret = spi_flash_write(env_flash, saved_offset, saved_size,
>> +????????????????????? saved_buffer);
>> ????????? if (ret)
>> ????????????? goto done;
>> ????? }
>> -??? ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags),
>> -??????????????? sizeof(env_new.flags), &flag);
>> -??? if (ret)
>> -??????? goto done;
>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>> +??????? ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags),
>> +????????????????????? sizeof(env_new.flags), &flag);
>> +??????? if (ret)
>> +??????????? goto done;
>> -??? puts("done\n");
>> +??????? puts("done\n");
>> -??? gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND;
>> +??????? gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND;
>> -??? printf("Valid environment: %d\n", (int)gd->env_valid);
>> +??????? printf("Valid environment: %d\n", (int)gd->env_valid);
>> +#else
>> +??????? puts("done\n");
>> +#endif
>> ?? done:
>> ????? if (saved_buffer)
>> @@ -146,6 +159,7 @@ static int env_sf_save(void)
>> ????? return ret;
>> ? }
>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>> ? static int env_sf_load(void)
>> ? {
>> ????? int ret;
>> @@ -183,66 +197,6 @@ out:
>> ????? return ret;
>> ? }
>> ? #else
>> -static int env_sf_save(void)
>> -{
>> -??? u32??? saved_size, saved_offset, sector;
>> -??? char??? *saved_buffer = NULL;
>> -??? int??? ret = 1;
>> -??? env_t??? env_new;
>> -
>> -??? ret = setup_flash_device();
>> -??? if (ret)
>> -??????? return ret;
>> -
>> -??? /* Is the sector larger than the env (i.e. embedded) */
>> -??? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>> -??????? saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE;
>> -??????? saved_offset = CONFIG_ENV_OFFSET + CONFIG_ENV_SIZE;
>> -??????? saved_buffer = malloc(saved_size);
>> -??????? if (!saved_buffer)
>> -??????????? goto done;
>> -
>> -??????? ret = spi_flash_read(env_flash, saved_offset,
>> -??????????? saved_size, saved_buffer);
>> -??????? if (ret)
>> -??????????? goto done;
>> -??? }
>> -
>> -??? ret = env_export(&env_new);
>> -??? if (ret)
>> -??????? goto done;
>> -
>> -??? sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
>> -
>> -??? puts("Erasing SPI flash...");
>> -??? ret = spi_flash_erase(env_flash, CONFIG_ENV_OFFSET,
>> -??????? sector * CONFIG_ENV_SECT_SIZE);
>> -??? if (ret)
>> -??????? goto done;
>> -
>> -??? puts("Writing to SPI flash...");
>> -??? ret = spi_flash_write(env_flash, CONFIG_ENV_OFFSET,
>> -??????? CONFIG_ENV_SIZE, &env_new);
>> -??? if (ret)
>> -??????? goto done;
>> -
>> -??? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>> -??????? ret = spi_flash_write(env_flash, saved_offset,
>> -??????????? saved_size, saved_buffer);
>> -??????? if (ret)
>> -??????????? goto done;
>> -??? }
>> -
>> -??? ret = 0;
>> -??? puts("done\n");
>> -
>> - done:
>> -??? if (saved_buffer)
>> -??????? free(saved_buffer);
>> -
>> -??? return ret;
>> -}
>> -
>> ? static int env_sf_load(void)
>> ? {
>> ????? int ret;
>> @@ -258,8 +212,8 @@ static int env_sf_load(void)
>> ????? if (ret)
>> ????????? goto out;
>> -??? ret = spi_flash_read(env_flash,
>> -??????? CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, buf);
>> +??? ret = spi_flash_read(env_flash, CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE,
>> +???????????????? buf);
>> ????? if (ret) {
>> ????????? env_set_default("spi_flash_read() failed", 0);
>> ????????? goto err_read;
>> @@ -292,7 +246,7 @@ static int env_sf_init(void)
>> ????? env_t *env_ptr = (env_t *)env_sf_get_env_addr();
>> ????? if (crc32(0, env_ptr->data, ENV_SIZE) == env_ptr->crc) {
>> -??????? gd->env_addr??? = (ulong)&(env_ptr->data);
>> +??????? gd->env_addr??? = (ulong)&env_ptr->data;
>> ????????? gd->env_valid??? = 1;
>> ????? } else {
>> ????????? gd->env_addr = (ulong)&default_environment[0];
>>
> 
> 
> Viele Gr??e,
> Stefan
> 


-- 
Harry Waschkeit - Software Engineer
duagon Germany GmbH
Neuwieder Stra?e 1-7 90411 Nuernberg
Geschaeftsfuehrer: Dr. Michael Goldbach - Matthias Kamolz - Kalina Scott - Handelsregister AG Nuernberg HRB 5540

[PATCH] env: sf: single function env_sf_save()

[Stefan Roese]

Hi Harry,

On 28.01.21 11:00, Harry Waschkeit wrote:
> ?Hi Stefan,
> 
> thanks a lot for your prompt reply :-)
> 
> And sorry that I didn't manage to continue on that for such a long time ...
> 
> 
> On 28.01.21 09:50, Stefan Roese wrote:
>> Hi Harry,
>>
>> On 28.01.21 08:21, Harry Waschkeit wrote:
>>> Instead of implementing redundant environments in two very similar
>>> functions env_sf_save(), handle redundancy in one function, placing the
>>> few differences in appropriate pre-compiler sections depending on config
>>> option CONFIG_ENV_OFFSET_REDUND.
>>>
>>> Additionally, several checkpatch complaints were addressed.
>>>
>>> This patch is in preparation for adding support for env erase.
>>>
>>> Signed-off-by: Harry Waschkeit <Harry.Waschkeit@men.de>
>>
>> I like this idea very much, thanks for working on this. One comment
>> below..
>>
>>> ---
>>> ? env/sf.c | 132 ++++++++++++++++++-------------------------------------
>>> ? 1 file changed, 43 insertions(+), 89 deletions(-)
>>>
>>> diff --git a/env/sf.c b/env/sf.c
>>> index 937778aa37..c60bd1deed 100644
>>> --- a/env/sf.c
>>> +++ b/env/sf.c
>>> @@ -66,13 +66,16 @@ static int setup_flash_device(void)
>>> ????? return 0;
>>> ? }
>>> -#if defined(CONFIG_ENV_OFFSET_REDUND)
>>> ? static int env_sf_save(void)
>>> ? {
>>> ????? env_t??? env_new;
>>> -??? char??? *saved_buffer = NULL, flag = ENV_REDUND_OBSOLETE;
>>> +??? char??? *saved_buffer = NULL;
>>> ????? u32??? saved_size, saved_offset, sector;
>>> +??? ulong??? offset;
>>> ????? int??? ret;
>>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>>> +??? char??? flag = ENV_REDUND_OBSOLETE;
>>> +#endif
>>
>> No more #idef's if possible please. See below...
> 
> As checkpatch.pl throws warnings for each such #if[def] occurrence, I 
> already tried to get rid of them.
> But I can't see how this shall be possible in a structure declaration.
> I'm eager to learn how to do that if possible, though :-)

Ah, right. It's probably not possible for this for a struct declaration.
But the variable above can be included always, right?

>>
>>> ????? ret = setup_flash_device();
>>> ????? if (ret)
>>> @@ -81,27 +84,33 @@ static int env_sf_save(void)
>>> ????? ret = env_export(&env_new);
>>> ????? if (ret)
>>> ????????? return -EIO;
>>> -??? env_new.flags??? = ENV_REDUND_ACTIVE;
>>> -??? if (gd->env_valid == ENV_VALID) {
>>> -??????? env_new_offset = CONFIG_ENV_OFFSET_REDUND;
>>> -??????? env_offset = CONFIG_ENV_OFFSET;
>>> -??? } else {
>>> -??????? env_new_offset = CONFIG_ENV_OFFSET;
>>> -??????? env_offset = CONFIG_ENV_OFFSET_REDUND;
>>> -??? }
>>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>>> +??????? env_new.flags??? = ENV_REDUND_ACTIVE;
>>> +
>>> +??????? if (gd->env_valid == ENV_VALID) {
>>> +??????????? env_new_offset = CONFIG_ENV_OFFSET_REDUND;
>>> +??????????? env_offset = CONFIG_ENV_OFFSET;
>>> +??????? } else {
>>> +??????????? env_new_offset = CONFIG_ENV_OFFSET;
>>> +??????????? env_offset = CONFIG_ENV_OFFSET_REDUND;
>>> +??????? }
>>> +??????? offset = env_new_offset;
>>> +#else
>>> +??????? offset = CONFIG_ENV_OFFSET;
>>> +#endif
>>
>> Can you please try to add this without adding #ifdef's but using
>> if (CONFIG_IS_ENABLE(SYS_REDUNDAND_ENVIRONMENT)) instead?
> 
> I tried that, too, but it was doomed to fail, because element flags in 
> env_t structure is only defined if redundant environment is enabled.

Too bad we didn't include "flag" for non-redundant env in the beginning.
Now its too late.

> Even though an "if (CONFIG_IS_ENABLED(...))" would statically evaluate 
> to '0' without active redundancy in environments, the parser sees the 
> syntax error of the non-existing structure element.
> 
> So, I don't see a chance for avoiding the #if construct here, too.
> Let me know if I miss something :-)

I agree its not easy or perhaps even impossible. One idea would be
to introduce a union in the struct with "flags" also defines in the
non-redundant case but not being used here. But this would result
in very non-intuitive code AFAICT. So better not go this way.

Perhaps somebody else has a quick idea on how to handle this without
introduncing #ifdef's here?

Thanks,
Stefan

> Cheers,
> Harry
> 
>> Thanks,
>> Stefan
>>
>>> ????? /* Is the sector larger than the env (i.e. embedded) */
>>> ????? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>> ????????? saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE;
>>> -??????? saved_offset = env_new_offset + CONFIG_ENV_SIZE;
>>> +??????? saved_offset = offset + CONFIG_ENV_SIZE;
>>> ????????? saved_buffer = memalign(ARCH_DMA_MINALIGN, saved_size);
>>> ????????? if (!saved_buffer) {
>>> ????????????? ret = -ENOMEM;
>>> ????????????? goto done;
>>> ????????? }
>>> -??????? ret = spi_flash_read(env_flash, saved_offset,
>>> -??????????????????? saved_size, saved_buffer);
>>> +??????? ret = spi_flash_read(env_flash, saved_offset, saved_size,
>>> +???????????????????? saved_buffer);
>>> ????????? if (ret)
>>> ????????????? goto done;
>>> ????? }
>>> @@ -109,35 +118,39 @@ static int env_sf_save(void)
>>> ????? sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
>>> ????? puts("Erasing SPI flash...");
>>> -??? ret = spi_flash_erase(env_flash, env_new_offset,
>>> -??????????????? sector * CONFIG_ENV_SECT_SIZE);
>>> +??? ret = spi_flash_erase(env_flash, offset,
>>> +????????????????? sector * CONFIG_ENV_SECT_SIZE);
>>> ????? if (ret)
>>> ????????? goto done;
>>> ????? puts("Writing to SPI flash...");
>>> -??? ret = spi_flash_write(env_flash, env_new_offset,
>>> -??????? CONFIG_ENV_SIZE, &env_new);
>>> +??? ret = spi_flash_write(env_flash, offset,
>>> +????????????????? CONFIG_ENV_SIZE, &env_new);
>>> ????? if (ret)
>>> ????????? goto done;
>>> ????? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>> -??????? ret = spi_flash_write(env_flash, saved_offset,
>>> -??????????????????? saved_size, saved_buffer);
>>> +??????? ret = spi_flash_write(env_flash, saved_offset, saved_size,
>>> +????????????????????? saved_buffer);
>>> ????????? if (ret)
>>> ????????????? goto done;
>>> ????? }
>>> -??? ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, 
>>> flags),
>>> -??????????????? sizeof(env_new.flags), &flag);
>>> -??? if (ret)
>>> -??????? goto done;
>>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>>> +??????? ret = spi_flash_write(env_flash, env_offset + 
>>> offsetof(env_t, flags),
>>> +????????????????????? sizeof(env_new.flags), &flag);
>>> +??????? if (ret)
>>> +??????????? goto done;
>>> -??? puts("done\n");
>>> +??????? puts("done\n");
>>> -??? gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : 
>>> ENV_REDUND;
>>> +??????? gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : 
>>> ENV_REDUND;
>>> -??? printf("Valid environment: %d\n", (int)gd->env_valid);
>>> +??????? printf("Valid environment: %d\n", (int)gd->env_valid);
>>> +#else
>>> +??????? puts("done\n");
>>> +#endif
>>> ?? done:
>>> ????? if (saved_buffer)
>>> @@ -146,6 +159,7 @@ static int env_sf_save(void)
>>> ????? return ret;
>>> ? }
>>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>>> ? static int env_sf_load(void)
>>> ? {
>>> ????? int ret;
>>> @@ -183,66 +197,6 @@ out:
>>> ????? return ret;
>>> ? }
>>> ? #else
>>> -static int env_sf_save(void)
>>> -{
>>> -??? u32??? saved_size, saved_offset, sector;
>>> -??? char??? *saved_buffer = NULL;
>>> -??? int??? ret = 1;
>>> -??? env_t??? env_new;
>>> -
>>> -??? ret = setup_flash_device();
>>> -??? if (ret)
>>> -??????? return ret;
>>> -
>>> -??? /* Is the sector larger than the env (i.e. embedded) */
>>> -??? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>> -??????? saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE;
>>> -??????? saved_offset = CONFIG_ENV_OFFSET + CONFIG_ENV_SIZE;
>>> -??????? saved_buffer = malloc(saved_size);
>>> -??????? if (!saved_buffer)
>>> -??????????? goto done;
>>> -
>>> -??????? ret = spi_flash_read(env_flash, saved_offset,
>>> -??????????? saved_size, saved_buffer);
>>> -??????? if (ret)
>>> -??????????? goto done;
>>> -??? }
>>> -
>>> -??? ret = env_export(&env_new);
>>> -??? if (ret)
>>> -??????? goto done;
>>> -
>>> -??? sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
>>> -
>>> -??? puts("Erasing SPI flash...");
>>> -??? ret = spi_flash_erase(env_flash, CONFIG_ENV_OFFSET,
>>> -??????? sector * CONFIG_ENV_SECT_SIZE);
>>> -??? if (ret)
>>> -??????? goto done;
>>> -
>>> -??? puts("Writing to SPI flash...");
>>> -??? ret = spi_flash_write(env_flash, CONFIG_ENV_OFFSET,
>>> -??????? CONFIG_ENV_SIZE, &env_new);
>>> -??? if (ret)
>>> -??????? goto done;
>>> -
>>> -??? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>> -??????? ret = spi_flash_write(env_flash, saved_offset,
>>> -??????????? saved_size, saved_buffer);
>>> -??????? if (ret)
>>> -??????????? goto done;
>>> -??? }
>>> -
>>> -??? ret = 0;
>>> -??? puts("done\n");
>>> -
>>> - done:
>>> -??? if (saved_buffer)
>>> -??????? free(saved_buffer);
>>> -
>>> -??? return ret;
>>> -}
>>> -
>>> ? static int env_sf_load(void)
>>> ? {
>>> ????? int ret;
>>> @@ -258,8 +212,8 @@ static int env_sf_load(void)
>>> ????? if (ret)
>>> ????????? goto out;
>>> -??? ret = spi_flash_read(env_flash,
>>> -??????? CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, buf);
>>> +??? ret = spi_flash_read(env_flash, CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE,
>>> +???????????????? buf);
>>> ????? if (ret) {
>>> ????????? env_set_default("spi_flash_read() failed", 0);
>>> ????????? goto err_read;
>>> @@ -292,7 +246,7 @@ static int env_sf_init(void)
>>> ????? env_t *env_ptr = (env_t *)env_sf_get_env_addr();
>>> ????? if (crc32(0, env_ptr->data, ENV_SIZE) == env_ptr->crc) {
>>> -??????? gd->env_addr??? = (ulong)&(env_ptr->data);
>>> +??????? gd->env_addr??? = (ulong)&env_ptr->data;
>>> ????????? gd->env_valid??? = 1;
>>> ????? } else {
>>> ????????? gd->env_addr = (ulong)&default_environment[0];
>>>
>>
>>
>> Viele Gr??e,
>> Stefan
>>
> 
> 


Viele Gr??e,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de

[PATCH] env: sf: single function env_sf_save()

[Harry Waschkeit]

On 28.01.21 11:11, Stefan Roese wrote:
> Hi Harry,
> 
> On 28.01.21 11:00, Harry Waschkeit wrote:
>> ?Hi Stefan,
>>
>> thanks a lot for your prompt reply :-)
>>
>> And sorry that I didn't manage to continue on that for such a long time ...
>>
>>
>> On 28.01.21 09:50, Stefan Roese wrote:
>>> Hi Harry,
>>>
>>> On 28.01.21 08:21, Harry Waschkeit wrote:
>>>> Instead of implementing redundant environments in two very similar
>>>> functions env_sf_save(), handle redundancy in one function, placing the
>>>> few differences in appropriate pre-compiler sections depending on config
>>>> option CONFIG_ENV_OFFSET_REDUND.
>>>>
>>>> Additionally, several checkpatch complaints were addressed.
>>>>
>>>> This patch is in preparation for adding support for env erase.
>>>>
>>>> Signed-off-by: Harry Waschkeit <Harry.Waschkeit@men.de>
>>>
>>> I like this idea very much, thanks for working on this. One comment
>>> below..
>>>
>>>> ---
>>>> ? env/sf.c | 132 ++++++++++++++++++-------------------------------------
>>>> ? 1 file changed, 43 insertions(+), 89 deletions(-)
>>>>
>>>> diff --git a/env/sf.c b/env/sf.c
>>>> index 937778aa37..c60bd1deed 100644
>>>> --- a/env/sf.c
>>>> +++ b/env/sf.c
>>>> @@ -66,13 +66,16 @@ static int setup_flash_device(void)
>>>> ????? return 0;
>>>> ? }
>>>> -#if defined(CONFIG_ENV_OFFSET_REDUND)
>>>> ? static int env_sf_save(void)
>>>> ? {
>>>> ????? env_t??? env_new;
>>>> -??? char??? *saved_buffer = NULL, flag = ENV_REDUND_OBSOLETE;
>>>> +??? char??? *saved_buffer = NULL;
>>>> ????? u32??? saved_size, saved_offset, sector;
>>>> +??? ulong??? offset;
>>>> ????? int??? ret;
>>>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>>>> +??? char??? flag = ENV_REDUND_OBSOLETE;
>>>> +#endif
>>>
>>> No more #idef's if possible please. See below...
>>
>> As checkpatch.pl throws warnings for each such #if[def] occurrence, I already tried to get rid of them.
>> But I can't see how this shall be possible in a structure declaration.
>> I'm eager to learn how to do that if possible, though :-)
> 
> Ah, right. It's probably not possible for this for a struct declaration.
> But the variable above can be included always, right?

Sure it could, but then we would have to live with a compiler warning about an unused variable I think.

>>>
>>>> ????? ret = setup_flash_device();
>>>> ????? if (ret)
>>>> @@ -81,27 +84,33 @@ static int env_sf_save(void)
>>>> ????? ret = env_export(&env_new);
>>>> ????? if (ret)
>>>> ????????? return -EIO;
>>>> -??? env_new.flags??? = ENV_REDUND_ACTIVE;
>>>> -??? if (gd->env_valid == ENV_VALID) {
>>>> -??????? env_new_offset = CONFIG_ENV_OFFSET_REDUND;
>>>> -??????? env_offset = CONFIG_ENV_OFFSET;
>>>> -??? } else {
>>>> -??????? env_new_offset = CONFIG_ENV_OFFSET;
>>>> -??????? env_offset = CONFIG_ENV_OFFSET_REDUND;
>>>> -??? }
>>>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>>>> +??????? env_new.flags??? = ENV_REDUND_ACTIVE;
>>>> +
>>>> +??????? if (gd->env_valid == ENV_VALID) {
>>>> +??????????? env_new_offset = CONFIG_ENV_OFFSET_REDUND;
>>>> +??????????? env_offset = CONFIG_ENV_OFFSET;
>>>> +??????? } else {
>>>> +??????????? env_new_offset = CONFIG_ENV_OFFSET;
>>>> +??????????? env_offset = CONFIG_ENV_OFFSET_REDUND;
>>>> +??????? }
>>>> +??????? offset = env_new_offset;
>>>> +#else
>>>> +??????? offset = CONFIG_ENV_OFFSET;
>>>> +#endif
>>>
>>> Can you please try to add this without adding #ifdef's but using
>>> if (CONFIG_IS_ENABLE(SYS_REDUNDAND_ENVIRONMENT)) instead?
>>
>> I tried that, too, but it was doomed to fail, because element flags in env_t structure is only defined if redundant environment is enabled.
> 
> Too bad we didn't include "flag" for non-redundant env in the beginning.
> Now its too late.

That would probably have been the best solution to avoid #if's, at least in that file.

But keep in mind: as a last consequence of this approach you would end up in structures bearing variables for all eventualities, however unlikely these are needed, blowing up the memory footprint for no reason.

In my opinion such things need to be thought through thoroughly :-)

>> Even though an "if (CONFIG_IS_ENABLED(...))" would statically evaluate to '0' without active redundancy in environments, the parser sees the syntax error of the non-existing structure element.
>>
>> So, I don't see a chance for avoiding the #if construct here, too.
>> Let me know if I miss something :-)
> 
> I agree its not easy or perhaps even impossible. One idea would be
> to introduce a union in the struct with "flags" also defines in the
> non-redundant case but not being used here. But this would result
> in very non-intuitive code AFAICT. So better not go this way.

That would feel very strange to me, too.

> Perhaps somebody else has a quick idea on how to handle this without
> introduncing #ifdef's here?

It would be possible to introduce a macro like env_set_flags(envp, flag) in env.h that only evaluates to something in case of active redundancy, sure.

But that's not even half of a solution, because also variables env_offset and env_new_offset which are set in that section are only defined if necessary, i.e. if CONFIG_SYS_REDUNDAND_ENVIRONMENT is set (btw also protected by #if while not possible otherwise).

The trade-off here is between code duplication - which I removed by combining two very similar separate functions for the two situations w/ and w/o redundancy in one - and in-line pre-compiler protected regions within one function.

While I fully agree that the latter should be avoided as far as possible, it is not avoidable here afaics.

And avoiding code duplication here outweighs the few pre-compiler occurrences in my eyes, but that's just my ? 0,02 :-)

Viele Gr??e,
Harry

> 
> Thanks,
> Stefan
> 
>> Cheers,
>> Harry
>>
>>> Thanks,
>>> Stefan
>>>
>>>> ????? /* Is the sector larger than the env (i.e. embedded) */
>>>> ????? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>>> ????????? saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE;
>>>> -??????? saved_offset = env_new_offset + CONFIG_ENV_SIZE;
>>>> +??????? saved_offset = offset + CONFIG_ENV_SIZE;
>>>> ????????? saved_buffer = memalign(ARCH_DMA_MINALIGN, saved_size);
>>>> ????????? if (!saved_buffer) {
>>>> ????????????? ret = -ENOMEM;
>>>> ????????????? goto done;
>>>> ????????? }
>>>> -??????? ret = spi_flash_read(env_flash, saved_offset,
>>>> -??????????????????? saved_size, saved_buffer);
>>>> +??????? ret = spi_flash_read(env_flash, saved_offset, saved_size,
>>>> +???????????????????? saved_buffer);
>>>> ????????? if (ret)
>>>> ????????????? goto done;
>>>> ????? }
>>>> @@ -109,35 +118,39 @@ static int env_sf_save(void)
>>>> ????? sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
>>>> ????? puts("Erasing SPI flash...");
>>>> -??? ret = spi_flash_erase(env_flash, env_new_offset,
>>>> -??????????????? sector * CONFIG_ENV_SECT_SIZE);
>>>> +??? ret = spi_flash_erase(env_flash, offset,
>>>> +????????????????? sector * CONFIG_ENV_SECT_SIZE);
>>>> ????? if (ret)
>>>> ????????? goto done;
>>>> ????? puts("Writing to SPI flash...");
>>>> -??? ret = spi_flash_write(env_flash, env_new_offset,
>>>> -??????? CONFIG_ENV_SIZE, &env_new);
>>>> +??? ret = spi_flash_write(env_flash, offset,
>>>> +????????????????? CONFIG_ENV_SIZE, &env_new);
>>>> ????? if (ret)
>>>> ????????? goto done;
>>>> ????? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>>> -??????? ret = spi_flash_write(env_flash, saved_offset,
>>>> -??????????????????? saved_size, saved_buffer);
>>>> +??????? ret = spi_flash_write(env_flash, saved_offset, saved_size,
>>>> +????????????????????? saved_buffer);
>>>> ????????? if (ret)
>>>> ????????????? goto done;
>>>> ????? }
>>>> -??? ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags),
>>>> -??????????????? sizeof(env_new.flags), &flag);
>>>> -??? if (ret)
>>>> -??????? goto done;
>>>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>>>> +??????? ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags),
>>>> +????????????????????? sizeof(env_new.flags), &flag);
>>>> +??????? if (ret)
>>>> +??????????? goto done;
>>>> -??? puts("done\n");
>>>> +??????? puts("done\n");
>>>> -??? gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND;
>>>> +??????? gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND;
>>>> -??? printf("Valid environment: %d\n", (int)gd->env_valid);
>>>> +??????? printf("Valid environment: %d\n", (int)gd->env_valid);
>>>> +#else
>>>> +??????? puts("done\n");
>>>> +#endif
>>>> ?? done:
>>>> ????? if (saved_buffer)
>>>> @@ -146,6 +159,7 @@ static int env_sf_save(void)
>>>> ????? return ret;
>>>> ? }
>>>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>>>> ? static int env_sf_load(void)
>>>> ? {
>>>> ????? int ret;
>>>> @@ -183,66 +197,6 @@ out:
>>>> ????? return ret;
>>>> ? }
>>>> ? #else
>>>> -static int env_sf_save(void)
>>>> -{
>>>> -??? u32??? saved_size, saved_offset, sector;
>>>> -??? char??? *saved_buffer = NULL;
>>>> -??? int??? ret = 1;
>>>> -??? env_t??? env_new;
>>>> -
>>>> -??? ret = setup_flash_device();
>>>> -??? if (ret)
>>>> -??????? return ret;
>>>> -
>>>> -??? /* Is the sector larger than the env (i.e. embedded) */
>>>> -??? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>>> -??????? saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE;
>>>> -??????? saved_offset = CONFIG_ENV_OFFSET + CONFIG_ENV_SIZE;
>>>> -??????? saved_buffer = malloc(saved_size);
>>>> -??????? if (!saved_buffer)
>>>> -??????????? goto done;
>>>> -
>>>> -??????? ret = spi_flash_read(env_flash, saved_offset,
>>>> -??????????? saved_size, saved_buffer);
>>>> -??????? if (ret)
>>>> -??????????? goto done;
>>>> -??? }
>>>> -
>>>> -??? ret = env_export(&env_new);
>>>> -??? if (ret)
>>>> -??????? goto done;
>>>> -
>>>> -??? sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
>>>> -
>>>> -??? puts("Erasing SPI flash...");
>>>> -??? ret = spi_flash_erase(env_flash, CONFIG_ENV_OFFSET,
>>>> -??????? sector * CONFIG_ENV_SECT_SIZE);
>>>> -??? if (ret)
>>>> -??????? goto done;
>>>> -
>>>> -??? puts("Writing to SPI flash...");
>>>> -??? ret = spi_flash_write(env_flash, CONFIG_ENV_OFFSET,
>>>> -??????? CONFIG_ENV_SIZE, &env_new);
>>>> -??? if (ret)
>>>> -??????? goto done;
>>>> -
>>>> -??? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>>> -??????? ret = spi_flash_write(env_flash, saved_offset,
>>>> -??????????? saved_size, saved_buffer);
>>>> -??????? if (ret)
>>>> -??????????? goto done;
>>>> -??? }
>>>> -
>>>> -??? ret = 0;
>>>> -??? puts("done\n");
>>>> -
>>>> - done:
>>>> -??? if (saved_buffer)
>>>> -??????? free(saved_buffer);
>>>> -
>>>> -??? return ret;
>>>> -}
>>>> -
>>>> ? static int env_sf_load(void)
>>>> ? {
>>>> ????? int ret;
>>>> @@ -258,8 +212,8 @@ static int env_sf_load(void)
>>>> ????? if (ret)
>>>> ????????? goto out;
>>>> -??? ret = spi_flash_read(env_flash,
>>>> -??????? CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, buf);
>>>> +??? ret = spi_flash_read(env_flash, CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE,
>>>> +???????????????? buf);
>>>> ????? if (ret) {
>>>> ????????? env_set_default("spi_flash_read() failed", 0);
>>>> ????????? goto err_read;
>>>> @@ -292,7 +246,7 @@ static int env_sf_init(void)
>>>> ????? env_t *env_ptr = (env_t *)env_sf_get_env_addr();
>>>> ????? if (crc32(0, env_ptr->data, ENV_SIZE) == env_ptr->crc) {
>>>> -??????? gd->env_addr??? = (ulong)&(env_ptr->data);
>>>> +??????? gd->env_addr??? = (ulong)&env_ptr->data;
>>>> ????????? gd->env_valid??? = 1;
>>>> ????? } else {
>>>> ????????? gd->env_addr = (ulong)&default_environment[0];
>>>>
>>>
>>>
>>> Viele Gr??e,
>>> Stefan
>>>
>>
>>
> 
> 
> Viele Gr??e,
> Stefan
> 


-- 
Harry Waschkeit - Software Engineer

[PATCH] env: sf: single function env_sf_save()

[Stefan Roese]

On 28.01.21 12:21, Harry Waschkeit wrote:

<snip>

>>> Even though an "if (CONFIG_IS_ENABLED(...))" would statically 
>>> evaluate to '0' without active redundancy in environments, the parser 
>>> sees the syntax error of the non-existing structure element.
>>>
>>> So, I don't see a chance for avoiding the #if construct here, too.
>>> Let me know if I miss something :-)
>>
>> I agree its not easy or perhaps even impossible. One idea would be
>> to introduce a union in the struct with "flags" also defines in the
>> non-redundant case but not being used here. But this would result
>> in very non-intuitive code AFAICT. So better not go this way.
> 
> That would feel very strange to me, too.
> 
>> Perhaps somebody else has a quick idea on how to handle this without
>> introduncing #ifdef's here?
> 
> It would be possible to introduce a macro like env_set_flags(envp, flag) 
> in env.h that only evaluates to something in case of active redundancy, 
> sure.
> 
> But that's not even half of a solution, because also variables 
> env_offset and env_new_offset which are set in that section are only 
> defined if necessary, i.e. if CONFIG_SYS_REDUNDAND_ENVIRONMENT is set 
> (btw also protected by #if while not possible otherwise).
> 
> The trade-off here is between code duplication - which I removed by 
> combining two very similar separate functions for the two situations w/ 
> and w/o redundancy in one - and in-line pre-compiler protected regions 
> within one function.
> 
> While I fully agree that the latter should be avoided as far as 
> possible, it is not avoidable here afaics.
> 
> And avoiding code duplication here outweighs the few pre-compiler 
> occurrences in my eyes, but that's just my ? 0,02 :-)

I also agree (now). If nobody else comes up with a better idea, then
please proceed with the current implementation to remove the code
duplication.

Thanks,
Stefan

> 
> Viele Gr??e,
> Harry
> 
>>
>> Thanks,
>> Stefan
>>
>>> Cheers,
>>> Harry
>>>
>>>> Thanks,
>>>> Stefan
>>>>
>>>>> ????? /* Is the sector larger than the env (i.e. embedded) */
>>>>> ????? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>>>> ????????? saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE;
>>>>> -??????? saved_offset = env_new_offset + CONFIG_ENV_SIZE;
>>>>> +??????? saved_offset = offset + CONFIG_ENV_SIZE;
>>>>> ????????? saved_buffer = memalign(ARCH_DMA_MINALIGN, saved_size);
>>>>> ????????? if (!saved_buffer) {
>>>>> ????????????? ret = -ENOMEM;
>>>>> ????????????? goto done;
>>>>> ????????? }
>>>>> -??????? ret = spi_flash_read(env_flash, saved_offset,
>>>>> -??????????????????? saved_size, saved_buffer);
>>>>> +??????? ret = spi_flash_read(env_flash, saved_offset, saved_size,
>>>>> +???????????????????? saved_buffer);
>>>>> ????????? if (ret)
>>>>> ????????????? goto done;
>>>>> ????? }
>>>>> @@ -109,35 +118,39 @@ static int env_sf_save(void)
>>>>> ????? sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
>>>>> ????? puts("Erasing SPI flash...");
>>>>> -??? ret = spi_flash_erase(env_flash, env_new_offset,
>>>>> -??????????????? sector * CONFIG_ENV_SECT_SIZE);
>>>>> +??? ret = spi_flash_erase(env_flash, offset,
>>>>> +????????????????? sector * CONFIG_ENV_SECT_SIZE);
>>>>> ????? if (ret)
>>>>> ????????? goto done;
>>>>> ????? puts("Writing to SPI flash...");
>>>>> -??? ret = spi_flash_write(env_flash, env_new_offset,
>>>>> -??????? CONFIG_ENV_SIZE, &env_new);
>>>>> +??? ret = spi_flash_write(env_flash, offset,
>>>>> +????????????????? CONFIG_ENV_SIZE, &env_new);
>>>>> ????? if (ret)
>>>>> ????????? goto done;
>>>>> ????? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>>>> -??????? ret = spi_flash_write(env_flash, saved_offset,
>>>>> -??????????????????? saved_size, saved_buffer);
>>>>> +??????? ret = spi_flash_write(env_flash, saved_offset, saved_size,
>>>>> +????????????????????? saved_buffer);
>>>>> ????????? if (ret)
>>>>> ????????????? goto done;
>>>>> ????? }
>>>>> -??? ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, 
>>>>> flags),
>>>>> -??????????????? sizeof(env_new.flags), &flag);
>>>>> -??? if (ret)
>>>>> -??????? goto done;
>>>>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>>>>> +??????? ret = spi_flash_write(env_flash, env_offset + 
>>>>> offsetof(env_t, flags),
>>>>> +????????????????????? sizeof(env_new.flags), &flag);
>>>>> +??????? if (ret)
>>>>> +??????????? goto done;
>>>>> -??? puts("done\n");
>>>>> +??????? puts("done\n");
>>>>> -??? gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : 
>>>>> ENV_REDUND;
>>>>> +??????? gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : 
>>>>> ENV_REDUND;
>>>>> -??? printf("Valid environment: %d\n", (int)gd->env_valid);
>>>>> +??????? printf("Valid environment: %d\n", (int)gd->env_valid);
>>>>> +#else
>>>>> +??????? puts("done\n");
>>>>> +#endif
>>>>> ?? done:
>>>>> ????? if (saved_buffer)
>>>>> @@ -146,6 +159,7 @@ static int env_sf_save(void)
>>>>> ????? return ret;
>>>>> ? }
>>>>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>>>>> ? static int env_sf_load(void)
>>>>> ? {
>>>>> ????? int ret;
>>>>> @@ -183,66 +197,6 @@ out:
>>>>> ????? return ret;
>>>>> ? }
>>>>> ? #else
>>>>> -static int env_sf_save(void)
>>>>> -{
>>>>> -??? u32??? saved_size, saved_offset, sector;
>>>>> -??? char??? *saved_buffer = NULL;
>>>>> -??? int??? ret = 1;
>>>>> -??? env_t??? env_new;
>>>>> -
>>>>> -??? ret = setup_flash_device();
>>>>> -??? if (ret)
>>>>> -??????? return ret;
>>>>> -
>>>>> -??? /* Is the sector larger than the env (i.e. embedded) */
>>>>> -??? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>>>> -??????? saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE;
>>>>> -??????? saved_offset = CONFIG_ENV_OFFSET + CONFIG_ENV_SIZE;
>>>>> -??????? saved_buffer = malloc(saved_size);
>>>>> -??????? if (!saved_buffer)
>>>>> -??????????? goto done;
>>>>> -
>>>>> -??????? ret = spi_flash_read(env_flash, saved_offset,
>>>>> -??????????? saved_size, saved_buffer);
>>>>> -??????? if (ret)
>>>>> -??????????? goto done;
>>>>> -??? }
>>>>> -
>>>>> -??? ret = env_export(&env_new);
>>>>> -??? if (ret)
>>>>> -??????? goto done;
>>>>> -
>>>>> -??? sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
>>>>> -
>>>>> -??? puts("Erasing SPI flash...");
>>>>> -??? ret = spi_flash_erase(env_flash, CONFIG_ENV_OFFSET,
>>>>> -??????? sector * CONFIG_ENV_SECT_SIZE);
>>>>> -??? if (ret)
>>>>> -??????? goto done;
>>>>> -
>>>>> -??? puts("Writing to SPI flash...");
>>>>> -??? ret = spi_flash_write(env_flash, CONFIG_ENV_OFFSET,
>>>>> -??????? CONFIG_ENV_SIZE, &env_new);
>>>>> -??? if (ret)
>>>>> -??????? goto done;
>>>>> -
>>>>> -??? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>>>> -??????? ret = spi_flash_write(env_flash, saved_offset,
>>>>> -??????????? saved_size, saved_buffer);
>>>>> -??????? if (ret)
>>>>> -??????????? goto done;
>>>>> -??? }
>>>>> -
>>>>> -??? ret = 0;
>>>>> -??? puts("done\n");
>>>>> -
>>>>> - done:
>>>>> -??? if (saved_buffer)
>>>>> -??????? free(saved_buffer);
>>>>> -
>>>>> -??? return ret;
>>>>> -}
>>>>> -
>>>>> ? static int env_sf_load(void)
>>>>> ? {
>>>>> ????? int ret;
>>>>> @@ -258,8 +212,8 @@ static int env_sf_load(void)
>>>>> ????? if (ret)
>>>>> ????????? goto out;
>>>>> -??? ret = spi_flash_read(env_flash,
>>>>> -??????? CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, buf);
>>>>> +??? ret = spi_flash_read(env_flash, CONFIG_ENV_OFFSET, 
>>>>> CONFIG_ENV_SIZE,
>>>>> +???????????????? buf);
>>>>> ????? if (ret) {
>>>>> ????????? env_set_default("spi_flash_read() failed", 0);
>>>>> ????????? goto err_read;
>>>>> @@ -292,7 +246,7 @@ static int env_sf_init(void)
>>>>> ????? env_t *env_ptr = (env_t *)env_sf_get_env_addr();
>>>>> ????? if (crc32(0, env_ptr->data, ENV_SIZE) == env_ptr->crc) {
>>>>> -??????? gd->env_addr??? = (ulong)&(env_ptr->data);
>>>>> +??????? gd->env_addr??? = (ulong)&env_ptr->data;
>>>>> ????????? gd->env_valid??? = 1;
>>>>> ????? } else {
>>>>> ????????? gd->env_addr = (ulong)&default_environment[0];
>>>>>
>>>>
>>>>
>>>> Viele Gr??e,
>>>> Stefan
>>>>
>>>
>>>
>>
>>
>> Viele Gr??e,
>> Stefan
>>
> 
> 


Viele Gr??e,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de

[PATCH] env: sf: single function env_sf_save()

[Harry Waschkeit]

Hi again Stefan,

On 29.01.21 08:16, Stefan Roese wrote:
> On 28.01.21 12:21, Harry Waschkeit wrote:
>
> <snip>
>
>>>> Even though an "if (CONFIG_IS_ENABLED(...))" would statically evaluate to '0' without active redundancy in environments, the parser sees the syntax error of the non-existing structure element.
>>>>
>>>> So, I don't see a chance for avoiding the #if construct here, too.
>>>> Let me know if I miss something :-)
>>>
>>> I agree its not easy or perhaps even impossible. One idea would be
>>> to introduce a union in the struct with "flags" also defines in the
>>> non-redundant case but not being used here. But this would result
>>> in very non-intuitive code AFAICT. So better not go this way.
>>
>> That would feel very strange to me, too.
>>
>>> Perhaps somebody else has a quick idea on how to handle this without
>>> introduncing #ifdef's here?
>>
>> It would be possible to introduce a macro like env_set_flags(envp, flag) in env.h that only evaluates to something in case of active redundancy, sure.
>>
>> But that's not even half of a solution, because also variables env_offset and env_new_offset which are set in that section are only defined if necessary, i.e. if CONFIG_SYS_REDUNDAND_ENVIRONMENT is set (btw also protected by #if while not possible otherwise).
>>
>> The trade-off here is between code duplication - which I removed by combining two very similar separate functions for the two situations w/ and w/o redundancy in one - and in-line pre-compiler protected regions within one function.
>>
>> While I fully agree that the latter should be avoided as far as possible, it is not avoidable here afaics.
>>
>> And avoiding code duplication here outweighs the few pre-compiler occurrences in my eyes, but that's just my ? 0,02 :-)
>
> I also agree (now). If nobody else comes up with a better idea, then
> please proceed with the current implementation to remove the code
> duplication.

sorry for the newbie question, I'm not familiar (yet) with the normal procedure and I don't want to keep that ball from rolling ;-/

As far as I understood you finally agreed on my patch, but you didn't give it a "reviewed-by" all the same; so do you still expect me to change my patch in some way or are you waiting for a "reviewed-by" from someone else to give that a go?

Why I am also asking: I have another small patch in he pipe that bases on the changed sources and that adds "env erase" support on top of that :-)

Thanks,
Harry

>
> Thanks,
> Stefan
>
>>
>> Viele Gr??e,
>> Harry
>>
>>>
>>> Thanks,
>>> Stefan
>>>
>>>> Cheers,
>>>> Harry
>>>>
>>>>> Thanks,
>>>>> Stefan
>>>>>
>>>>>> ????? /* Is the sector larger than the env (i.e. embedded) */
>>>>>> ????? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>>>>> ????????? saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE;
>>>>>> -??????? saved_offset = env_new_offset + CONFIG_ENV_SIZE;
>>>>>> +??????? saved_offset = offset + CONFIG_ENV_SIZE;
>>>>>> ????????? saved_buffer = memalign(ARCH_DMA_MINALIGN, saved_size);
>>>>>> ????????? if (!saved_buffer) {
>>>>>> ????????????? ret = -ENOMEM;
>>>>>> ????????????? goto done;
>>>>>> ????????? }
>>>>>> -??????? ret = spi_flash_read(env_flash, saved_offset,
>>>>>> -??????????????????? saved_size, saved_buffer);
>>>>>> +??????? ret = spi_flash_read(env_flash, saved_offset, saved_size,
>>>>>> +???????????????????? saved_buffer);
>>>>>> ????????? if (ret)
>>>>>> ????????????? goto done;
>>>>>> ????? }
>>>>>> @@ -109,35 +118,39 @@ static int env_sf_save(void)
>>>>>> ????? sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
>>>>>> ????? puts("Erasing SPI flash...");
>>>>>> -??? ret = spi_flash_erase(env_flash, env_new_offset,
>>>>>> -??????????????? sector * CONFIG_ENV_SECT_SIZE);
>>>>>> +??? ret = spi_flash_erase(env_flash, offset,
>>>>>> +????????????????? sector * CONFIG_ENV_SECT_SIZE);
>>>>>> ????? if (ret)
>>>>>> ????????? goto done;
>>>>>> ????? puts("Writing to SPI flash...");
>>>>>> -??? ret = spi_flash_write(env_flash, env_new_offset,
>>>>>> -??????? CONFIG_ENV_SIZE, &env_new);
>>>>>> +??? ret = spi_flash_write(env_flash, offset,
>>>>>> +????????????????? CONFIG_ENV_SIZE, &env_new);
>>>>>> ????? if (ret)
>>>>>> ????????? goto done;
>>>>>> ????? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>>>>> -??????? ret = spi_flash_write(env_flash, saved_offset,
>>>>>> -??????????????????? saved_size, saved_buffer);
>>>>>> +??????? ret = spi_flash_write(env_flash, saved_offset, saved_size,
>>>>>> +????????????????????? saved_buffer);
>>>>>> ????????? if (ret)
>>>>>> ????????????? goto done;
>>>>>> ????? }
>>>>>> -??? ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags),
>>>>>> -??????????????? sizeof(env_new.flags), &flag);
>>>>>> -??? if (ret)
>>>>>> -??????? goto done;
>>>>>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>>>>>> +??????? ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags),
>>>>>> +????????????????????? sizeof(env_new.flags), &flag);
>>>>>> +??????? if (ret)
>>>>>> +??????????? goto done;
>>>>>> -??? puts("done\n");
>>>>>> +??????? puts("done\n");
>>>>>> -??? gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND;
>>>>>> +??????? gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND;
>>>>>> -??? printf("Valid environment: %d\n", (int)gd->env_valid);
>>>>>> +??????? printf("Valid environment: %d\n", (int)gd->env_valid);
>>>>>> +#else
>>>>>> +??????? puts("done\n");
>>>>>> +#endif
>>>>>> ?? done:
>>>>>> ????? if (saved_buffer)
>>>>>> @@ -146,6 +159,7 @@ static int env_sf_save(void)
>>>>>> ????? return ret;
>>>>>> ? }
>>>>>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>>>>>> ? static int env_sf_load(void)
>>>>>> ? {
>>>>>> ????? int ret;
>>>>>> @@ -183,66 +197,6 @@ out:
>>>>>> ????? return ret;
>>>>>> ? }
>>>>>> ? #else
>>>>>> -static int env_sf_save(void)
>>>>>> -{
>>>>>> -??? u32??? saved_size, saved_offset, sector;
>>>>>> -??? char??? *saved_buffer = NULL;
>>>>>> -??? int??? ret = 1;
>>>>>> -??? env_t??? env_new;
>>>>>> -
>>>>>> -??? ret = setup_flash_device();
>>>>>> -??? if (ret)
>>>>>> -??????? return ret;
>>>>>> -
>>>>>> -??? /* Is the sector larger than the env (i.e. embedded) */
>>>>>> -??? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>>>>> -??????? saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE;
>>>>>> -??????? saved_offset = CONFIG_ENV_OFFSET + CONFIG_ENV_SIZE;
>>>>>> -??????? saved_buffer = malloc(saved_size);
>>>>>> -??????? if (!saved_buffer)
>>>>>> -??????????? goto done;
>>>>>> -
>>>>>> -??????? ret = spi_flash_read(env_flash, saved_offset,
>>>>>> -??????????? saved_size, saved_buffer);
>>>>>> -??????? if (ret)
>>>>>> -??????????? goto done;
>>>>>> -??? }
>>>>>> -
>>>>>> -??? ret = env_export(&env_new);
>>>>>> -??? if (ret)
>>>>>> -??????? goto done;
>>>>>> -
>>>>>> -??? sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
>>>>>> -
>>>>>> -??? puts("Erasing SPI flash...");
>>>>>> -??? ret = spi_flash_erase(env_flash, CONFIG_ENV_OFFSET,
>>>>>> -??????? sector * CONFIG_ENV_SECT_SIZE);
>>>>>> -??? if (ret)
>>>>>> -??????? goto done;
>>>>>> -
>>>>>> -??? puts("Writing to SPI flash...");
>>>>>> -??? ret = spi_flash_write(env_flash, CONFIG_ENV_OFFSET,
>>>>>> -??????? CONFIG_ENV_SIZE, &env_new);
>>>>>> -??? if (ret)
>>>>>> -??????? goto done;
>>>>>> -
>>>>>> -??? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>>>>> -??????? ret = spi_flash_write(env_flash, saved_offset,
>>>>>> -??????????? saved_size, saved_buffer);
>>>>>> -??????? if (ret)
>>>>>> -??????????? goto done;
>>>>>> -??? }
>>>>>> -
>>>>>> -??? ret = 0;
>>>>>> -??? puts("done\n");
>>>>>> -
>>>>>> - done:
>>>>>> -??? if (saved_buffer)
>>>>>> -??????? free(saved_buffer);
>>>>>> -
>>>>>> -??? return ret;
>>>>>> -}
>>>>>> -
>>>>>> ? static int env_sf_load(void)
>>>>>> ? {
>>>>>> ????? int ret;
>>>>>> @@ -258,8 +212,8 @@ static int env_sf_load(void)
>>>>>> ????? if (ret)
>>>>>> ????????? goto out;
>>>>>> -??? ret = spi_flash_read(env_flash,
>>>>>> -??????? CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, buf);
>>>>>> +??? ret = spi_flash_read(env_flash, CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE,
>>>>>> +???????????????? buf);
>>>>>> ????? if (ret) {
>>>>>> ????????? env_set_default("spi_flash_read() failed", 0);
>>>>>> ????????? goto err_read;
>>>>>> @@ -292,7 +246,7 @@ static int env_sf_init(void)
>>>>>> ????? env_t *env_ptr = (env_t *)env_sf_get_env_addr();
>>>>>> ????? if (crc32(0, env_ptr->data, ENV_SIZE) == env_ptr->crc) {
>>>>>> -??????? gd->env_addr??? = (ulong)&(env_ptr->data);
>>>>>> +??????? gd->env_addr??? = (ulong)&env_ptr->data;
>>>>>> ????????? gd->env_valid??? = 1;
>>>>>> ????? } else {
>>>>>> ????????? gd->env_addr = (ulong)&default_environment[0];
>>>>>>
>>>>>
>>>>>
>>>>> Viele Gr??e,
>>>>> Stefan
>>>>>
>>>>
>>>>
>>>
>>>
>>> Viele Gr??e,
>>> Stefan
>>>
>>
>>
>
>
> Viele Gr??e,
> Stefan
>
-- 
Harry Waschkeit - Software Engineer

[PATCH] env: sf: single function env_sf_save()

[Stefan Roese]

Hi Harry,

On 29.01.21 18:18, Harry Waschkeit wrote:
> Hi again Stefan,
> 
> On 29.01.21 08:16, Stefan Roese wrote:
>> On 28.01.21 12:21, Harry Waschkeit wrote:
>>
>> <snip>
>>
>>>>> Even though an "if (CONFIG_IS_ENABLED(...))" would statically 
>>>>> evaluate to '0' without active redundancy in environments, the 
>>>>> parser sees the syntax error of the non-existing structure element.
>>>>>
>>>>> So, I don't see a chance for avoiding the #if construct here, too.
>>>>> Let me know if I miss something :-)
>>>>
>>>> I agree its not easy or perhaps even impossible. One idea would be
>>>> to introduce a union in the struct with "flags" also defines in the
>>>> non-redundant case but not being used here. But this would result
>>>> in very non-intuitive code AFAICT. So better not go this way.
>>>
>>> That would feel very strange to me, too.
>>>
>>>> Perhaps somebody else has a quick idea on how to handle this without
>>>> introduncing #ifdef's here?
>>>
>>> It would be possible to introduce a macro like env_set_flags(envp, 
>>> flag) in env.h that only evaluates to something in case of active 
>>> redundancy, sure.
>>>
>>> But that's not even half of a solution, because also variables 
>>> env_offset and env_new_offset which are set in that section are only 
>>> defined if necessary, i.e. if CONFIG_SYS_REDUNDAND_ENVIRONMENT is set 
>>> (btw also protected by #if while not possible otherwise).
>>>
>>> The trade-off here is between code duplication - which I removed by 
>>> combining two very similar separate functions for the two situations 
>>> w/ and w/o redundancy in one - and in-line pre-compiler protected 
>>> regions within one function.
>>>
>>> While I fully agree that the latter should be avoided as far as 
>>> possible, it is not avoidable here afaics.
>>>
>>> And avoiding code duplication here outweighs the few pre-compiler 
>>> occurrences in my eyes, but that's just my ? 0,02 :-)
>>
>> I also agree (now). If nobody else comes up with a better idea, then
>> please proceed with the current implementation to remove the code
>> duplication.
> 
> sorry for the newbie question, I'm not familiar (yet) with the normal 
> procedure and I don't want to keep that ball from rolling ;-/

No Problem. Please double-check if not 100% sure. ;)

> As far as I understood you finally agreed on my patch, but you didn't 
> give it a "reviewed-by" all the same; so do you still expect me to 
> change my patch in some way or are you waiting for a "reviewed-by" from 
> someone else to give that a go?
> 
> Why I am also asking: I have another small patch in he pipe that bases 
> on the changed sources and that adds "env erase" support on top of that :-)

I didn't do a thorough review yet. Let me try to do this quickly...

Thanks,
Stefan

> Thanks,
> Harry
> 
>>
>> Thanks,
>> Stefan
>>
>>>
>>> Viele Gr??e,
>>> Harry
>>>
>>>>
>>>> Thanks,
>>>> Stefan
>>>>
>>>>> Cheers,
>>>>> Harry
>>>>>
>>>>>> Thanks,
>>>>>> Stefan
>>>>>>
>>>>>>> ????? /* Is the sector larger than the env (i.e. embedded) */
>>>>>>> ????? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>>>>>> ????????? saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE;
>>>>>>> -??????? saved_offset = env_new_offset + CONFIG_ENV_SIZE;
>>>>>>> +??????? saved_offset = offset + CONFIG_ENV_SIZE;
>>>>>>> ????????? saved_buffer = memalign(ARCH_DMA_MINALIGN, saved_size);
>>>>>>> ????????? if (!saved_buffer) {
>>>>>>> ????????????? ret = -ENOMEM;
>>>>>>> ????????????? goto done;
>>>>>>> ????????? }
>>>>>>> -??????? ret = spi_flash_read(env_flash, saved_offset,
>>>>>>> -??????????????????? saved_size, saved_buffer);
>>>>>>> +??????? ret = spi_flash_read(env_flash, saved_offset, saved_size,
>>>>>>> +???????????????????? saved_buffer);
>>>>>>> ????????? if (ret)
>>>>>>> ????????????? goto done;
>>>>>>> ????? }
>>>>>>> @@ -109,35 +118,39 @@ static int env_sf_save(void)
>>>>>>> ????? sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
>>>>>>> ????? puts("Erasing SPI flash...");
>>>>>>> -??? ret = spi_flash_erase(env_flash, env_new_offset,
>>>>>>> -??????????????? sector * CONFIG_ENV_SECT_SIZE);
>>>>>>> +??? ret = spi_flash_erase(env_flash, offset,
>>>>>>> +????????????????? sector * CONFIG_ENV_SECT_SIZE);
>>>>>>> ????? if (ret)
>>>>>>> ????????? goto done;
>>>>>>> ????? puts("Writing to SPI flash...");
>>>>>>> -??? ret = spi_flash_write(env_flash, env_new_offset,
>>>>>>> -??????? CONFIG_ENV_SIZE, &env_new);
>>>>>>> +??? ret = spi_flash_write(env_flash, offset,
>>>>>>> +????????????????? CONFIG_ENV_SIZE, &env_new);
>>>>>>> ????? if (ret)
>>>>>>> ????????? goto done;
>>>>>>> ????? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>>>>>> -??????? ret = spi_flash_write(env_flash, saved_offset,
>>>>>>> -??????????????????? saved_size, saved_buffer);
>>>>>>> +??????? ret = spi_flash_write(env_flash, saved_offset, saved_size,
>>>>>>> +????????????????????? saved_buffer);
>>>>>>> ????????? if (ret)
>>>>>>> ????????????? goto done;
>>>>>>> ????? }
>>>>>>> -??? ret = spi_flash_write(env_flash, env_offset + 
>>>>>>> offsetof(env_t, flags),
>>>>>>> -??????????????? sizeof(env_new.flags), &flag);
>>>>>>> -??? if (ret)
>>>>>>> -??????? goto done;
>>>>>>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>>>>>>> +??????? ret = spi_flash_write(env_flash, env_offset + 
>>>>>>> offsetof(env_t, flags),
>>>>>>> +????????????????????? sizeof(env_new.flags), &flag);
>>>>>>> +??????? if (ret)
>>>>>>> +??????????? goto done;
>>>>>>> -??? puts("done\n");
>>>>>>> +??????? puts("done\n");
>>>>>>> -??? gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : 
>>>>>>> ENV_REDUND;
>>>>>>> +??????? gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID 
>>>>>>> : ENV_REDUND;
>>>>>>> -??? printf("Valid environment: %d\n", (int)gd->env_valid);
>>>>>>> +??????? printf("Valid environment: %d\n", (int)gd->env_valid);
>>>>>>> +#else
>>>>>>> +??????? puts("done\n");
>>>>>>> +#endif
>>>>>>> ?? done:
>>>>>>> ????? if (saved_buffer)
>>>>>>> @@ -146,6 +159,7 @@ static int env_sf_save(void)
>>>>>>> ????? return ret;
>>>>>>> ? }
>>>>>>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>>>>>>> ? static int env_sf_load(void)
>>>>>>> ? {
>>>>>>> ????? int ret;
>>>>>>> @@ -183,66 +197,6 @@ out:
>>>>>>> ????? return ret;
>>>>>>> ? }
>>>>>>> ? #else
>>>>>>> -static int env_sf_save(void)
>>>>>>> -{
>>>>>>> -??? u32??? saved_size, saved_offset, sector;
>>>>>>> -??? char??? *saved_buffer = NULL;
>>>>>>> -??? int??? ret = 1;
>>>>>>> -??? env_t??? env_new;
>>>>>>> -
>>>>>>> -??? ret = setup_flash_device();
>>>>>>> -??? if (ret)
>>>>>>> -??????? return ret;
>>>>>>> -
>>>>>>> -??? /* Is the sector larger than the env (i.e. embedded) */
>>>>>>> -??? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>>>>>> -??????? saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE;
>>>>>>> -??????? saved_offset = CONFIG_ENV_OFFSET + CONFIG_ENV_SIZE;
>>>>>>> -??????? saved_buffer = malloc(saved_size);
>>>>>>> -??????? if (!saved_buffer)
>>>>>>> -??????????? goto done;
>>>>>>> -
>>>>>>> -??????? ret = spi_flash_read(env_flash, saved_offset,
>>>>>>> -??????????? saved_size, saved_buffer);
>>>>>>> -??????? if (ret)
>>>>>>> -??????????? goto done;
>>>>>>> -??? }
>>>>>>> -
>>>>>>> -??? ret = env_export(&env_new);
>>>>>>> -??? if (ret)
>>>>>>> -??????? goto done;
>>>>>>> -
>>>>>>> -??? sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
>>>>>>> -
>>>>>>> -??? puts("Erasing SPI flash...");
>>>>>>> -??? ret = spi_flash_erase(env_flash, CONFIG_ENV_OFFSET,
>>>>>>> -??????? sector * CONFIG_ENV_SECT_SIZE);
>>>>>>> -??? if (ret)
>>>>>>> -??????? goto done;
>>>>>>> -
>>>>>>> -??? puts("Writing to SPI flash...");
>>>>>>> -??? ret = spi_flash_write(env_flash, CONFIG_ENV_OFFSET,
>>>>>>> -??????? CONFIG_ENV_SIZE, &env_new);
>>>>>>> -??? if (ret)
>>>>>>> -??????? goto done;
>>>>>>> -
>>>>>>> -??? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>>>>>> -??????? ret = spi_flash_write(env_flash, saved_offset,
>>>>>>> -??????????? saved_size, saved_buffer);
>>>>>>> -??????? if (ret)
>>>>>>> -??????????? goto done;
>>>>>>> -??? }
>>>>>>> -
>>>>>>> -??? ret = 0;
>>>>>>> -??? puts("done\n");
>>>>>>> -
>>>>>>> - done:
>>>>>>> -??? if (saved_buffer)
>>>>>>> -??????? free(saved_buffer);
>>>>>>> -
>>>>>>> -??? return ret;
>>>>>>> -}
>>>>>>> -
>>>>>>> ? static int env_sf_load(void)
>>>>>>> ? {
>>>>>>> ????? int ret;
>>>>>>> @@ -258,8 +212,8 @@ static int env_sf_load(void)
>>>>>>> ????? if (ret)
>>>>>>> ????????? goto out;
>>>>>>> -??? ret = spi_flash_read(env_flash,
>>>>>>> -??????? CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, buf);
>>>>>>> +??? ret = spi_flash_read(env_flash, CONFIG_ENV_OFFSET, 
>>>>>>> CONFIG_ENV_SIZE,
>>>>>>> +???????????????? buf);
>>>>>>> ????? if (ret) {
>>>>>>> ????????? env_set_default("spi_flash_read() failed", 0);
>>>>>>> ????????? goto err_read;
>>>>>>> @@ -292,7 +246,7 @@ static int env_sf_init(void)
>>>>>>> ????? env_t *env_ptr = (env_t *)env_sf_get_env_addr();
>>>>>>> ????? if (crc32(0, env_ptr->data, ENV_SIZE) == env_ptr->crc) {
>>>>>>> -??????? gd->env_addr??? = (ulong)&(env_ptr->data);
>>>>>>> +??????? gd->env_addr??? = (ulong)&env_ptr->data;
>>>>>>> ????????? gd->env_valid??? = 1;
>>>>>>> ????? } else {
>>>>>>> ????????? gd->env_addr = (ulong)&default_environment[0];
>>>>>>>
>>>>>>
>>>>>>
>>>>>> Viele Gr??e,
>>>>>> Stefan
>>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>> Viele Gr??e,
>>>> Stefan
>>>>
>>>
>>>
>>
>>
>> Viele Gr??e,
>> Stefan
>>


Viele Gr??e,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de

[PATCH] env: sf: single function env_sf_save()

[Stefan Roese]

Hi Harry,

On 28.01.21 08:21, Harry Waschkeit wrote:
> Instead of implementing redundant environments in two very similar
> functions env_sf_save(), handle redundancy in one function, placing the
> few differences in appropriate pre-compiler sections depending on config
> option CONFIG_ENV_OFFSET_REDUND.
> 
> Additionally, several checkpatch complaints were addressed.
> 
> This patch is in preparation for adding support for env erase.
> 
> Signed-off-by: Harry Waschkeit <Harry.Waschkeit@men.de>
> ---
>   env/sf.c | 132 ++++++++++++++++++-------------------------------------
>   1 file changed, 43 insertions(+), 89 deletions(-)

Nice diffstat. ;)

> 
> diff --git a/env/sf.c b/env/sf.c
> index 937778aa37..c60bd1deed 100644
> --- a/env/sf.c
> +++ b/env/sf.c
> @@ -66,13 +66,16 @@ static int setup_flash_device(void)
>   	return 0;
>   }
>   
> -#if defined(CONFIG_ENV_OFFSET_REDUND)
>   static int env_sf_save(void)
>   {
>   	env_t	env_new;
> -	char	*saved_buffer = NULL, flag = ENV_REDUND_OBSOLETE;
> +	char	*saved_buffer = NULL;
>   	u32	saved_size, saved_offset, sector;
> +	ulong	offset;
>   	int	ret;
> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
> +	char	flag = ENV_REDUND_OBSOLETE;
> +#endif

I would prefer this to the #ifdef here:

	__maybe_unused char flag = ENV_REDUND_OBSOLETE;

Looks good aside from this:

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

>   
>   	ret = setup_flash_device();
>   	if (ret)
> @@ -81,27 +84,33 @@ static int env_sf_save(void)
>   	ret = env_export(&env_new);
>   	if (ret)
>   		return -EIO;
> -	env_new.flags	= ENV_REDUND_ACTIVE;
>   
> -	if (gd->env_valid == ENV_VALID) {
> -		env_new_offset = CONFIG_ENV_OFFSET_REDUND;
> -		env_offset = CONFIG_ENV_OFFSET;
> -	} else {
> -		env_new_offset = CONFIG_ENV_OFFSET;
> -		env_offset = CONFIG_ENV_OFFSET_REDUND;
> -	}
> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
> +		env_new.flags	= ENV_REDUND_ACTIVE;
> +
> +		if (gd->env_valid == ENV_VALID) {
> +			env_new_offset = CONFIG_ENV_OFFSET_REDUND;
> +			env_offset = CONFIG_ENV_OFFSET;
> +		} else {
> +			env_new_offset = CONFIG_ENV_OFFSET;
> +			env_offset = CONFIG_ENV_OFFSET_REDUND;
> +		}
> +		offset = env_new_offset;
> +#else
> +		offset = CONFIG_ENV_OFFSET;
> +#endif
>   
>   	/* Is the sector larger than the env (i.e. embedded) */
>   	if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>   		saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE;
> -		saved_offset = env_new_offset + CONFIG_ENV_SIZE;
> +		saved_offset = offset + CONFIG_ENV_SIZE;
>   		saved_buffer = memalign(ARCH_DMA_MINALIGN, saved_size);
>   		if (!saved_buffer) {
>   			ret = -ENOMEM;
>   			goto done;
>   		}
> -		ret = spi_flash_read(env_flash, saved_offset,
> -					saved_size, saved_buffer);
> +		ret = spi_flash_read(env_flash, saved_offset, saved_size,
> +				     saved_buffer);
>   		if (ret)
>   			goto done;
>   	}
> @@ -109,35 +118,39 @@ static int env_sf_save(void)
>   	sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
>   
>   	puts("Erasing SPI flash...");
> -	ret = spi_flash_erase(env_flash, env_new_offset,
> -				sector * CONFIG_ENV_SECT_SIZE);
> +	ret = spi_flash_erase(env_flash, offset,
> +			      sector * CONFIG_ENV_SECT_SIZE);
>   	if (ret)
>   		goto done;
>   
>   	puts("Writing to SPI flash...");
>   
> -	ret = spi_flash_write(env_flash, env_new_offset,
> -		CONFIG_ENV_SIZE, &env_new);
> +	ret = spi_flash_write(env_flash, offset,
> +			      CONFIG_ENV_SIZE, &env_new);
>   	if (ret)
>   		goto done;
>   
>   	if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
> -		ret = spi_flash_write(env_flash, saved_offset,
> -					saved_size, saved_buffer);
> +		ret = spi_flash_write(env_flash, saved_offset, saved_size,
> +				      saved_buffer);
>   		if (ret)
>   			goto done;
>   	}
>   
> -	ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags),
> -				sizeof(env_new.flags), &flag);
> -	if (ret)
> -		goto done;
> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
> +		ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags),
> +				      sizeof(env_new.flags), &flag);
> +		if (ret)
> +			goto done;
>   
> -	puts("done\n");
> +		puts("done\n");
>   
> -	gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND;
> +		gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND;
>   
> -	printf("Valid environment: %d\n", (int)gd->env_valid);
> +		printf("Valid environment: %d\n", (int)gd->env_valid);
> +#else
> +		puts("done\n");
> +#endif
>   
>    done:
>   	if (saved_buffer)
> @@ -146,6 +159,7 @@ static int env_sf_save(void)
>   	return ret;
>   }
>   
> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>   static int env_sf_load(void)
>   {
>   	int ret;
> @@ -183,66 +197,6 @@ out:
>   	return ret;
>   }
>   #else
> -static int env_sf_save(void)
> -{
> -	u32	saved_size, saved_offset, sector;
> -	char	*saved_buffer = NULL;
> -	int	ret = 1;
> -	env_t	env_new;
> -
> -	ret = setup_flash_device();
> -	if (ret)
> -		return ret;
> -
> -	/* Is the sector larger than the env (i.e. embedded) */
> -	if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
> -		saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE;
> -		saved_offset = CONFIG_ENV_OFFSET + CONFIG_ENV_SIZE;
> -		saved_buffer = malloc(saved_size);
> -		if (!saved_buffer)
> -			goto done;
> -
> -		ret = spi_flash_read(env_flash, saved_offset,
> -			saved_size, saved_buffer);
> -		if (ret)
> -			goto done;
> -	}
> -
> -	ret = env_export(&env_new);
> -	if (ret)
> -		goto done;
> -
> -	sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
> -
> -	puts("Erasing SPI flash...");
> -	ret = spi_flash_erase(env_flash, CONFIG_ENV_OFFSET,
> -		sector * CONFIG_ENV_SECT_SIZE);
> -	if (ret)
> -		goto done;
> -
> -	puts("Writing to SPI flash...");
> -	ret = spi_flash_write(env_flash, CONFIG_ENV_OFFSET,
> -		CONFIG_ENV_SIZE, &env_new);
> -	if (ret)
> -		goto done;
> -
> -	if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
> -		ret = spi_flash_write(env_flash, saved_offset,
> -			saved_size, saved_buffer);
> -		if (ret)
> -			goto done;
> -	}
> -
> -	ret = 0;
> -	puts("done\n");
> -
> - done:
> -	if (saved_buffer)
> -		free(saved_buffer);
> -
> -	return ret;
> -}
> -
>   static int env_sf_load(void)
>   {
>   	int ret;
> @@ -258,8 +212,8 @@ static int env_sf_load(void)
>   	if (ret)
>   		goto out;
>   
> -	ret = spi_flash_read(env_flash,
> -		CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, buf);
> +	ret = spi_flash_read(env_flash, CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE,
> +			     buf);
>   	if (ret) {
>   		env_set_default("spi_flash_read() failed", 0);
>   		goto err_read;
> @@ -292,7 +246,7 @@ static int env_sf_init(void)
>   	env_t *env_ptr = (env_t *)env_sf_get_env_addr();
>   
>   	if (crc32(0, env_ptr->data, ENV_SIZE) == env_ptr->crc) {
> -		gd->env_addr	= (ulong)&(env_ptr->data);
> +		gd->env_addr	= (ulong)&env_ptr->data;
>   		gd->env_valid	= 1;
>   	} else {
>   		gd->env_addr = (ulong)&default_environment[0];
> 


Viele Gr??e,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de

[PATCH] env: sf: single function env_sf_save()

[Harry Waschkeit]

?
On 30.01.21 10:07, Stefan Roese wrote:
> Hi Harry,

Hi Stefan,

> On 28.01.21 08:21, Harry Waschkeit wrote:
>> Instead of implementing redundant environments in two very similar
>> functions env_sf_save(), handle redundancy in one function, placing the
>> few differences in appropriate pre-compiler sections depending on config
>> option CONFIG_ENV_OFFSET_REDUND.
>>
>> Additionally, several checkpatch complaints were addressed.
>>
>> This patch is in preparation for adding support for env erase.
>>
>> Signed-off-by: Harry Waschkeit <Harry.Waschkeit@men.de>
>> ---
>> ? env/sf.c | 132 ++++++++++++++++++-------------------------------------
>> ? 1 file changed, 43 insertions(+), 89 deletions(-)
> 
> Nice diffstat. ;)
> 
>>
>> diff --git a/env/sf.c b/env/sf.c
>> index 937778aa37..c60bd1deed 100644
>> --- a/env/sf.c
>> +++ b/env/sf.c
>> @@ -66,13 +66,16 @@ static int setup_flash_device(void)
>> ????? return 0;
>> ? }
>> -#if defined(CONFIG_ENV_OFFSET_REDUND)
>> ? static int env_sf_save(void)
>> ? {
>> ????? env_t??? env_new;
>> -??? char??? *saved_buffer = NULL, flag = ENV_REDUND_OBSOLETE;
>> +??? char??? *saved_buffer = NULL;
>> ????? u32??? saved_size, saved_offset, sector;
>> +??? ulong??? offset;
>> ????? int??? ret;
>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>> +??? char??? flag = ENV_REDUND_OBSOLETE;
>> +#endif
> 
> I would prefer this to the #ifdef here:
> 
>  ????__maybe_unused char flag = ENV_REDUND_OBSOLETE;

Ok, will change that and send a v2.

> Looks good aside from this:
> 
> Reviewed-by: Stefan Roese <sr@denx.de>

Thanks for your review! :-)

Cheers,
Harry

> Thanks,
> Stefan
> 
>> ????? ret = setup_flash_device();
>> ????? if (ret)
>> @@ -81,27 +84,33 @@ static int env_sf_save(void)
>> ????? ret = env_export(&env_new);
>> ????? if (ret)
>> ????????? return -EIO;
>> -??? env_new.flags??? = ENV_REDUND_ACTIVE;
>> -??? if (gd->env_valid == ENV_VALID) {
>> -??????? env_new_offset = CONFIG_ENV_OFFSET_REDUND;
>> -??????? env_offset = CONFIG_ENV_OFFSET;
>> -??? } else {
>> -??????? env_new_offset = CONFIG_ENV_OFFSET;
>> -??????? env_offset = CONFIG_ENV_OFFSET_REDUND;
>> -??? }
>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>> +??????? env_new.flags??? = ENV_REDUND_ACTIVE;
>> +
>> +??????? if (gd->env_valid == ENV_VALID) {
>> +??????????? env_new_offset = CONFIG_ENV_OFFSET_REDUND;
>> +??????????? env_offset = CONFIG_ENV_OFFSET;
>> +??????? } else {
>> +??????????? env_new_offset = CONFIG_ENV_OFFSET;
>> +??????????? env_offset = CONFIG_ENV_OFFSET_REDUND;
>> +??????? }
>> +??????? offset = env_new_offset;
>> +#else
>> +??????? offset = CONFIG_ENV_OFFSET;
>> +#endif
>> ????? /* Is the sector larger than the env (i.e. embedded) */
>> ????? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>> ????????? saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE;
>> -??????? saved_offset = env_new_offset + CONFIG_ENV_SIZE;
>> +??????? saved_offset = offset + CONFIG_ENV_SIZE;
>> ????????? saved_buffer = memalign(ARCH_DMA_MINALIGN, saved_size);
>> ????????? if (!saved_buffer) {
>> ????????????? ret = -ENOMEM;
>> ????????????? goto done;
>> ????????? }
>> -??????? ret = spi_flash_read(env_flash, saved_offset,
>> -??????????????????? saved_size, saved_buffer);
>> +??????? ret = spi_flash_read(env_flash, saved_offset, saved_size,
>> +???????????????????? saved_buffer);
>> ????????? if (ret)
>> ????????????? goto done;
>> ????? }
>> @@ -109,35 +118,39 @@ static int env_sf_save(void)
>> ????? sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
>> ????? puts("Erasing SPI flash...");
>> -??? ret = spi_flash_erase(env_flash, env_new_offset,
>> -??????????????? sector * CONFIG_ENV_SECT_SIZE);
>> +??? ret = spi_flash_erase(env_flash, offset,
>> +????????????????? sector * CONFIG_ENV_SECT_SIZE);
>> ????? if (ret)
>> ????????? goto done;
>> ????? puts("Writing to SPI flash...");
>> -??? ret = spi_flash_write(env_flash, env_new_offset,
>> -??????? CONFIG_ENV_SIZE, &env_new);
>> +??? ret = spi_flash_write(env_flash, offset,
>> +????????????????? CONFIG_ENV_SIZE, &env_new);
>> ????? if (ret)
>> ????????? goto done;
>> ????? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>> -??????? ret = spi_flash_write(env_flash, saved_offset,
>> -??????????????????? saved_size, saved_buffer);
>> +??????? ret = spi_flash_write(env_flash, saved_offset, saved_size,
>> +????????????????????? saved_buffer);
>> ????????? if (ret)
>> ????????????? goto done;
>> ????? }
>> -??? ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags),
>> -??????????????? sizeof(env_new.flags), &flag);
>> -??? if (ret)
>> -??????? goto done;
>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>> +??????? ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags),
>> +????????????????????? sizeof(env_new.flags), &flag);
>> +??????? if (ret)
>> +??????????? goto done;
>> -??? puts("done\n");
>> +??????? puts("done\n");
>> -??? gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND;
>> +??????? gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : ENV_REDUND;
>> -??? printf("Valid environment: %d\n", (int)gd->env_valid);
>> +??????? printf("Valid environment: %d\n", (int)gd->env_valid);
>> +#else
>> +??????? puts("done\n");
>> +#endif
>> ?? done:
>> ????? if (saved_buffer)
>> @@ -146,6 +159,7 @@ static int env_sf_save(void)
>> ????? return ret;
>> ? }
>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>> ? static int env_sf_load(void)
>> ? {
>> ????? int ret;
>> @@ -183,66 +197,6 @@ out:
>> ????? return ret;
>> ? }
>> ? #else
>> -static int env_sf_save(void)
>> -{
>> -??? u32??? saved_size, saved_offset, sector;
>> -??? char??? *saved_buffer = NULL;
>> -??? int??? ret = 1;
>> -??? env_t??? env_new;
>> -
>> -??? ret = setup_flash_device();
>> -??? if (ret)
>> -??????? return ret;
>> -
>> -??? /* Is the sector larger than the env (i.e. embedded) */
>> -??? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>> -??????? saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE;
>> -??????? saved_offset = CONFIG_ENV_OFFSET + CONFIG_ENV_SIZE;
>> -??????? saved_buffer = malloc(saved_size);
>> -??????? if (!saved_buffer)
>> -??????????? goto done;
>> -
>> -??????? ret = spi_flash_read(env_flash, saved_offset,
>> -??????????? saved_size, saved_buffer);
>> -??????? if (ret)
>> -??????????? goto done;
>> -??? }
>> -
>> -??? ret = env_export(&env_new);
>> -??? if (ret)
>> -??????? goto done;
>> -
>> -??? sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
>> -
>> -??? puts("Erasing SPI flash...");
>> -??? ret = spi_flash_erase(env_flash, CONFIG_ENV_OFFSET,
>> -??????? sector * CONFIG_ENV_SECT_SIZE);
>> -??? if (ret)
>> -??????? goto done;
>> -
>> -??? puts("Writing to SPI flash...");
>> -??? ret = spi_flash_write(env_flash, CONFIG_ENV_OFFSET,
>> -??????? CONFIG_ENV_SIZE, &env_new);
>> -??? if (ret)
>> -??????? goto done;
>> -
>> -??? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>> -??????? ret = spi_flash_write(env_flash, saved_offset,
>> -??????????? saved_size, saved_buffer);
>> -??????? if (ret)
>> -??????????? goto done;
>> -??? }
>> -
>> -??? ret = 0;
>> -??? puts("done\n");
>> -
>> - done:
>> -??? if (saved_buffer)
>> -??????? free(saved_buffer);
>> -
>> -??? return ret;
>> -}
>> -
>> ? static int env_sf_load(void)
>> ? {
>> ????? int ret;
>> @@ -258,8 +212,8 @@ static int env_sf_load(void)
>> ????? if (ret)
>> ????????? goto out;
>> -??? ret = spi_flash_read(env_flash,
>> -??????? CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, buf);
>> +??? ret = spi_flash_read(env_flash, CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE,
>> +???????????????? buf);
>> ????? if (ret) {
>> ????????? env_set_default("spi_flash_read() failed", 0);
>> ????????? goto err_read;
>> @@ -292,7 +246,7 @@ static int env_sf_init(void)
>> ????? env_t *env_ptr = (env_t *)env_sf_get_env_addr();
>> ????? if (crc32(0, env_ptr->data, ENV_SIZE) == env_ptr->crc) {
>> -??????? gd->env_addr??? = (ulong)&(env_ptr->data);
>> +??????? gd->env_addr??? = (ulong)&env_ptr->data;
>> ????????? gd->env_valid??? = 1;
>> ????? } else {
>> ????????? gd->env_addr = (ulong)&default_environment[0];
>>
> 
> 
> Viele Gr??e,
> Stefan
> 


-- 
Harry Waschkeit - Software Engineer
duagon Germany GmbH
Neuwieder Stra?e 1-7 90411 Nuernberg
Geschaeftsfuehrer: Dr. Michael Goldbach - Matthias Kamolz - Kalina Scott - Handelsregister AG Nuernberg HRB 5540