diff mbox series

fw_setenv: Unbreak fw_setenv caused by buggy MEMISLOCKED use

Message ID 20211208143311.32178-1-joakim.tjernlund@infinera.com
State Accepted
Commit 08cf1a5e6995697bcf2405ae50826634bc43d78a
Delegated to: Tom Rini
Headers show
Series fw_setenv: Unbreak fw_setenv caused by buggy MEMISLOCKED use | expand

Commit Message

Joakim Tjernlund Dec. 8, 2021, 2:33 p.m. UTC 
Commit "fw_setenv: lock the flash only if it was locked before"
checks for Locked status with uninitialized erase data.
Address by moving the test for MEMISLOCKED.

Fixes: 8a726b852502 ("fw_setenv: lock the flash only if it was locked before")
Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
---
 tools/env/fw_env.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

Comments

Joakim Tjernlund Dec. 13, 2021, 5:22 p.m. UTC | #1 
+Joe Hershberger

 Jocke

On Wed, 2021-12-08 at 15:33 +0100, Joakim Tjernlund wrote:
> Commit "fw_setenv: lock the flash only if it was locked before"
> checks for Locked status with uninitialized erase data.
> Address by moving the test for MEMISLOCKED.
> 
> Fixes: 8a726b852502 ("fw_setenv: lock the flash only if it was locked before")
> Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> ---
>  tools/env/fw_env.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
> index e39c39e23a..3da75be783 100644
> --- a/tools/env/fw_env.c
> +++ b/tools/env/fw_env.c
> @@ -1083,12 +1083,6 @@ static int flash_write_buf(int dev, int fd, void *buf, size_t count)
>  	}
>  
>  	erase.length = erasesize;
> -	if (DEVTYPE(dev) != MTD_ABSENT) {
> -		was_locked = ioctl(fd, MEMISLOCKED, &erase);
> -		/* treat any errors as unlocked flash */
> -		if (was_locked < 0)
> -			was_locked = 0;
> -	}
>  
>  	/* This only runs once on NOR flash and SPI-dataflash */
>  	while (processed < write_total) {
> @@ -1108,6 +1102,10 @@ static int flash_write_buf(int dev, int fd, void *buf, size_t count)
>  
>  		if (DEVTYPE(dev) != MTD_ABSENT) {
>  			erase.start = blockstart;
> +			was_locked = ioctl(fd, MEMISLOCKED, &erase);
> +			/* treat any errors as unlocked flash */
> +			if (was_locked < 0)
> +					was_locked = 0;
>  			if (was_locked)
>  				ioctl(fd, MEMUNLOCK, &erase);
>  			/* These do not need an explicit erase cycle */
> @@ -1163,7 +1161,6 @@ static int flash_flag_obsolete(int dev, int fd, off_t offset)
>  	char tmp = ENV_REDUND_OBSOLETE;
>  	int was_locked;	/* flash lock flag */
>  
> -	was_locked = ioctl(fd, MEMISLOCKED, &erase);
>  	erase.start = DEVOFFSET(dev);
>  	erase.length = DEVESIZE(dev);
>  	/* This relies on the fact, that ENV_REDUND_OBSOLETE == 0 */
> @@ -1173,6 +1170,10 @@ static int flash_flag_obsolete(int dev, int fd, off_t offset)
>  			DEVNAME(dev));
>  		return rc;
>  	}
> +	was_locked = ioctl(fd, MEMISLOCKED, &erase);
> +	/* treat any errors as unlocked flash */
> +	if (was_locked < 0)
> +		was_locked = 0;
>  	if (was_locked)
>  		ioctl(fd, MEMUNLOCK, &erase);
>  	rc = write(fd, &tmp, sizeof(tmp));
Joakim Tjernlund Dec. 18, 2021, 6:23 p.m. UTC | #2 
Ping?
Maybe just revert commit 8a726b852502 ("fw_setenv: lock the flash only if it was locked before") ?
Ivan Mikhaylov Dec. 19, 2021, 2:20 p.m. UTC | #3 
On Sat, 2021-12-18 at 18:23 +0000, Joakim Tjernlund wrote:
> Ping?
> Maybe just revert commit 8a726b852502 ("fw_setenv: lock the flash
> only if it was locked before") ?
> 
> ________________________________________
> From: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
> Sent: 13 December 2021 18:22
> To: u-boot@lists.denx.de; joe.hershberger@ni.com; fr0st61te@gmail.com
> Subject: Re: [PATCH] fw_setenv: Unbreak fw_setenv caused by buggy
> MEMISLOCKED use
> 
> +Joe Hershberger
> 
>  Jocke
> 
> On Wed, 2021-12-08 at 15:33 +0100, Joakim Tjernlund wrote:
> > Commit "fw_setenv: lock the flash only if it was locked before"
> > checks for Locked status with uninitialized erase data.
> > Address by moving the test for MEMISLOCKED.
> > 
> > Fixes: 8a726b852502 ("fw_setenv: lock the flash only if it was
> > locked before")
> > Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>

Joakim, can you provide more detailed description about what you want
to fix or revert, please? In which case you see problems with
8a726b852502?

Thanks.
Joakim Tjernlund Dec. 19, 2021, 3:28 p.m. UTC | #4 
On Sun, 2021-12-19 at 14:20 +0000, Ivan Mikhaylov wrote:
> On Sat, 2021-12-18 at 18:23 +0000, Joakim Tjernlund wrote:
> > Ping?
> > Maybe just revert commit 8a726b852502 ("fw_setenv: lock the flash
> > only if it was locked before") ?
> > 
> > ________________________________________
> > From: Joakim Tjernlund <Joakim.Tjernlund@infinera.com>
> > Sent: 13 December 2021 18:22
> > To: u-boot@lists.denx.de; joe.hershberger@ni.com; fr0st61te@gmail.com
> > Subject: Re: [PATCH] fw_setenv: Unbreak fw_setenv caused by buggy
> > MEMISLOCKED use
> > 
> > +Joe Hershberger
> > 
> >  Jocke
> > 
> > On Wed, 2021-12-08 at 15:33 +0100, Joakim Tjernlund wrote:
> > > Commit "fw_setenv: lock the flash only if it was locked before"
> > > checks for Locked status with uninitialized erase data.
> > > Address by moving the test for MEMISLOCKED.
> > > 
> > > Fixes: 8a726b852502 ("fw_setenv: lock the flash only if it was
> > > locked before")
> > > Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> 
> Joakim, can you provide more detailed description about what you want
> to fix or revert, please? In which case you see problems with
> 8a726b852502?
> 
> Thanks.
> 

We see it when we set a variable to the same value it already had, then we get a locking error
from fw_setenv.
If you just look 1 min at the code you will see that 8a726b852502 sends garbage to MEMISLOCKED

 Jocke
Tom Rini Dec. 20, 2021, 4:16 p.m. UTC | #5 
On Wed, Dec 08, 2021 at 03:33:11PM +0100, Joakim Tjernlund wrote:

> Commit "fw_setenv: lock the flash only if it was locked before"
> checks for Locked status with uninitialized erase data.
> Address by moving the test for MEMISLOCKED.
> 
> Fixes: 8a726b852502 ("fw_setenv: lock the flash only if it was locked before")
> Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>

Applied to u-boot/master, thanks!
diff mbox series

Patch

diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
index e39c39e23a..3da75be783 100644
--- a/tools/env/fw_env.c
+++ b/tools/env/fw_env.c
@@ -1083,12 +1083,6 @@  static int flash_write_buf(int dev, int fd, void *buf, size_t count)
 	}
 
 	erase.length = erasesize;
-	if (DEVTYPE(dev) != MTD_ABSENT) {
-		was_locked = ioctl(fd, MEMISLOCKED, &erase);
-		/* treat any errors as unlocked flash */
-		if (was_locked < 0)
-			was_locked = 0;
-	}
 
 	/* This only runs once on NOR flash and SPI-dataflash */
 	while (processed < write_total) {
@@ -1108,6 +1102,10 @@  static int flash_write_buf(int dev, int fd, void *buf, size_t count)
 
 		if (DEVTYPE(dev) != MTD_ABSENT) {
 			erase.start = blockstart;
+			was_locked = ioctl(fd, MEMISLOCKED, &erase);
+			/* treat any errors as unlocked flash */
+			if (was_locked < 0)
+					was_locked = 0;
 			if (was_locked)
 				ioctl(fd, MEMUNLOCK, &erase);
 			/* These do not need an explicit erase cycle */
@@ -1163,7 +1161,6 @@  static int flash_flag_obsolete(int dev, int fd, off_t offset)
 	char tmp = ENV_REDUND_OBSOLETE;
 	int was_locked;	/* flash lock flag */
 
-	was_locked = ioctl(fd, MEMISLOCKED, &erase);
 	erase.start = DEVOFFSET(dev);
 	erase.length = DEVESIZE(dev);
 	/* This relies on the fact, that ENV_REDUND_OBSOLETE == 0 */
@@ -1173,6 +1170,10 @@  static int flash_flag_obsolete(int dev, int fd, off_t offset)
 			DEVNAME(dev));
 		return rc;
 	}
+	was_locked = ioctl(fd, MEMISLOCKED, &erase);
+	/* treat any errors as unlocked flash */
+	if (was_locked < 0)
+		was_locked = 0;
 	if (was_locked)
 		ioctl(fd, MEMUNLOCK, &erase);
 	rc = write(fd, &tmp, sizeof(tmp));