pflash: Fix erase command for unaligned start address

Message ID 20170913065946.11262-1-sjitindarsingh@gmail.com
State Accepted
Headers show
Series
  • pflash: Fix erase command for unaligned start address
Related show

Commit Message

Suraj Jitindar Singh Sept. 13, 2017, 6:59 a.m.
The erase_range() function handles erasing the flash for a given start
address and length, and can handle an unaligned start address and
length. However in the unaligned start address case we are incorrectly
calculating the remaining size which can lead to incomplete erases.

If we're going to update the remaining size based on what the start
address was then we probably want to do that before we overide the
origin start address. So rearrange the code so that this is indeed the
case.

Reported-by: Pridhiviraj Paidipeddi <ppaidipe@in.ibm.com>
Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
---
 external/pflash/pflash.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Cyril Bur Sept. 15, 2017, 1:21 a.m. | #1
On Wed, 2017-09-13 at 16:59 +1000, Suraj Jitindar Singh wrote:
> The erase_range() function handles erasing the flash for a given start
> address and length, and can handle an unaligned start address and
> length. However in the unaligned start address case we are incorrectly
> calculating the remaining size which can lead to incomplete erases.
> 
> If we're going to update the remaining size based on what the start
> address was then we probably want to do that before we overide the
> origin start address. So rearrange the code so that this is indeed the
> case.
> 
> Reported-by: Pridhiviraj Paidipeddi <ppaidipe@in.ibm.com>
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>

Good find.
Stewart: I know you usually do, but just to be sure, if you could
respond with the merged sha1 of this so that I'll remember to add a
testcase for this, I'm surprised this wasn't caught in testing.

Reviewed-by: Cyril Bur <cyril.bur@au1.ibm.com>

> ---
>  external/pflash/pflash.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/external/pflash/pflash.c b/external/pflash/pflash.c
> index bfc975f..5b1be3c 100644
> --- a/external/pflash/pflash.c
> +++ b/external/pflash/pflash.c
> @@ -331,9 +331,9 @@ static int erase_range(struct flash_details *flash,
>  			fprintf(stderr, "Failed to blocklevel_smart_erase(): %d\n", rc);
>  			return 1;
>  		}
> -		start += flash->erase_granule - (start & erase_mask);
>  		size -= flash->erase_granule - (start & erase_mask);
>  		done = flash->erase_granule - (start & erase_mask);
> +		start += flash->erase_granule - (start & erase_mask);
>  	}
>  	progress_tick(done);
>  	while (size & ~(erase_mask)) {
Stewart Smith Sept. 15, 2017, 8:18 a.m. | #2
Cyril Bur <cyrilbur@gmail.com> writes:
> On Wed, 2017-09-13 at 16:59 +1000, Suraj Jitindar Singh wrote:
>> The erase_range() function handles erasing the flash for a given start
>> address and length, and can handle an unaligned start address and
>> length. However in the unaligned start address case we are incorrectly
>> calculating the remaining size which can lead to incomplete erases.
>> 
>> If we're going to update the remaining size based on what the start
>> address was then we probably want to do that before we overide the
>> origin start address. So rearrange the code so that this is indeed the
>> case.
>> 
>> Reported-by: Pridhiviraj Paidipeddi <ppaidipe@in.ibm.com>
>> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
>
> Good find.
> Stewart: I know you usually do, but just to be sure, if you could
> respond with the merged sha1 of this so that I'll remember to add a
> testcase for this, I'm surprised this wasn't caught in testing.

Merged as of aefd3ffce6ed8f586290c0044446024f6e6fdcd5

Moar tests always welcomed. :)

Patch

diff --git a/external/pflash/pflash.c b/external/pflash/pflash.c
index bfc975f..5b1be3c 100644
--- a/external/pflash/pflash.c
+++ b/external/pflash/pflash.c
@@ -331,9 +331,9 @@  static int erase_range(struct flash_details *flash,
 			fprintf(stderr, "Failed to blocklevel_smart_erase(): %d\n", rc);
 			return 1;
 		}
-		start += flash->erase_granule - (start & erase_mask);
 		size -= flash->erase_granule - (start & erase_mask);
 		done = flash->erase_granule - (start & erase_mask);
+		start += flash->erase_granule - (start & erase_mask);
 	}
 	progress_tick(done);
 	while (size & ~(erase_mask)) {