[PATCH] cmd: sf: Support unaligned flash updates with 'sf update'

[PATCH] cmd: sf: Support unaligned flash updates with 'sf update'

[Frieder Schrempf]

From: Frieder Schrempf <frieder.schrempf@kontron.de>

Currently 'sf update' supports only offsets that are aligned to the
erase block size of the serial flash. Unaligned offsets result in
something like:

=> sf update ${kernel_addr_r} 0x400 ${filesize}
device 0 offset 0x400, size 0x11f758
SPI flash failed in erase step

In order to support unaligned updates, we simply read the first full
block and check only the requested part of the block for changes. If
necessary, the block is erased, the first (unchanged) part of the block
is written back together with the second part of the block (updated data).

Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
 cmd/sf.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/cmd/sf.c b/cmd/sf.c
index eac27ed2d7..c54b4b10bb 100644
--- a/cmd/sf.c
+++ b/cmd/sf.c
@@ -171,30 +171,32 @@ static int do_spi_flash_probe(int argc, char *const argv[])
 static const char *spi_flash_update_block(struct spi_flash *flash, u32 offset,
 		size_t len, const char *buf, char *cmp_buf, size_t *skipped)
 {
+	u32 offset_aligned = ALIGN_DOWN(offset, flash->sector_size);
+	u32 sector_offset = offset - offset_aligned;
 	char *ptr = (char *)buf;
 
 	debug("offset=%#x, sector_size=%#x, len=%#zx\n",
 	      offset, flash->sector_size, len);
 	/* Read the entire sector so to allow for rewriting */
-	if (spi_flash_read(flash, offset, flash->sector_size, cmp_buf))
+	if (spi_flash_read(flash, offset_aligned, flash->sector_size, cmp_buf))
 		return "read";
 	/* Compare only what is meaningful (len) */
-	if (memcmp(cmp_buf, buf, len) == 0) {
+	if (memcmp(cmp_buf + sector_offset, buf, len) == 0) {
 		debug("Skip region %x size %zx: no change\n",
 		      offset, len);
 		*skipped += len;
 		return NULL;
 	}
 	/* Erase the entire sector */
-	if (spi_flash_erase(flash, offset, flash->sector_size))
+	if (spi_flash_erase(flash, offset_aligned, flash->sector_size))
 		return "erase";
 	/* If it's a partial sector, copy the data into the temp-buffer */
 	if (len != flash->sector_size) {
-		memcpy(cmp_buf, buf, len);
+		memcpy(cmp_buf + sector_offset, buf, len);
 		ptr = cmp_buf;
 	}
 	/* Write one complete sector */
-	if (spi_flash_write(flash, offset, flash->sector_size, ptr))
+	if (spi_flash_write(flash, offset_aligned, flash->sector_size, ptr))
 		return "write";
 
 	return NULL;
@@ -230,7 +232,10 @@ static int spi_flash_update(struct spi_flash *flash, u32 offset,
 		ulong last_update = get_timer(0);
 
 		for (; buf < end && !err_oper; buf += todo, offset += todo) {
-			todo = min_t(size_t, end - buf, flash->sector_size);
+			if (offset % flash->sector_size)
+				todo = flash->sector_size - (offset % flash->sector_size);
+			else
+				todo = min_t(size_t, end - buf, flash->sector_size);
 			if (get_timer(last_update) > 100) {
 				printf("   \rUpdating, %zu%% %lu B/s",
 				       100 - (end - buf) / scale,
-- 
2.33.0

Re: [PATCH] cmd: sf: Support unaligned flash updates with 'sf update'

[Michael Walle]

Am 2021-09-30 18:19, schrieb Frieder Schrempf:
> In order to support unaligned updates, we simply read the first full
> block and check only the requested part of the block for changes. If
> necessary, the block is erased, the first (unchanged) part of the block
> is written back together with the second part of the block (updated 
> data).

I'm not sure what I should think of this. You might loose that unchanged
part if there is an power loss in the middle, even worse, you likely 
don't
have this part anymore because it isn't part of the data you want to 
write
to the flash.

Maybe add an parameter for allow (unsafe) unaligned updates?

Now you might argue, that with "sf erase, sf write" you can do the same
harm, to which i reply: but then it is intentional ;) Because "sf erase"
just works on sector boundaries (which isn't really checked in the 
command,
i just realized, but at least my driver returns EINVAL) and then the
user has to include the "unchanged part" for the arguments on the
commandline.

-michael

Re: [PATCH] cmd: sf: Support unaligned flash updates with 'sf update'

[Frieder Schrempf]

On 30.09.21 18:35, Michael Walle wrote:
> Am 2021-09-30 18:19, schrieb Frieder Schrempf:
>> In order to support unaligned updates, we simply read the first full
>> block and check only the requested part of the block for changes. If
>> necessary, the block is erased, the first (unchanged) part of the block
>> is written back together with the second part of the block (updated
>> data).
> 
> I'm not sure what I should think of this. You might loose that unchanged
> part if there is an power loss in the middle, even worse, you likely don't
> have this part anymore because it isn't part of the data you want to write
> to the flash.
> 
> Maybe add an parameter for allow (unsafe) unaligned updates?
> 
> Now you might argue, that with "sf erase, sf write" you can do the same
> harm, to which i reply: but then it is intentional ;) Because "sf erase"
> just works on sector boundaries (which isn't really checked in the command,
> i just realized, but at least my driver returns EINVAL) and then the
> user has to include the "unchanged part" for the arguments on the
> commandline.

True, but "sf update" already is "unsafe" even without supporting
unaligned start offsets. The code already handles partial sector writes
for the last sector in the same way (read, erase, write), which is also
prone to data loss in case of power loss.

So this patch in fact just adds support for partial sector updates for
the first sector. It is slightly more probable to loose data, but it
doesn't introduce new "unsafe" behavior.

Maybe we could cover this by adding a warning to the documentation, or
even printing a warning?

Re: [PATCH] cmd: sf: Support unaligned flash updates with 'sf update'

[Michael Walle]

Am 2021-09-30 19:17, schrieb Frieder Schrempf:
> On 30.09.21 18:35, Michael Walle wrote:
>> Am 2021-09-30 18:19, schrieb Frieder Schrempf:
>>> In order to support unaligned updates, we simply read the first full
>>> block and check only the requested part of the block for changes. If
>>> necessary, the block is erased, the first (unchanged) part of the 
>>> block
>>> is written back together with the second part of the block (updated
>>> data).
>> 
>> I'm not sure what I should think of this. You might loose that 
>> unchanged
>> part if there is an power loss in the middle, even worse, you likely 
>> don't
>> have this part anymore because it isn't part of the data you want to 
>> write
>> to the flash.
>> 
>> Maybe add an parameter for allow (unsafe) unaligned updates?
>> 
>> Now you might argue, that with "sf erase, sf write" you can do the 
>> same
>> harm, to which i reply: but then it is intentional ;) Because "sf 
>> erase"
>> just works on sector boundaries (which isn't really checked in the 
>> command,
>> i just realized, but at least my driver returns EINVAL) and then the
>> user has to include the "unchanged part" for the arguments on the
>> commandline.
> 
> True, but "sf update" already is "unsafe" even without supporting
> unaligned start offsets. The code already handles partial sector writes
> for the last sector in the same way (read, erase, write), which is also
> prone to data loss in case of power loss.

Ah, I missed that. Yes, in this case, we don't loose anything. Agreed.

> So this patch in fact just adds support for partial sector updates for
> the first sector. It is slightly more probable to loose data, but it
> doesn't introduce new "unsafe" behavior.
> 
> Maybe we could cover this by adding a warning to the documentation, or
> even printing a warning?

Heh, although I was using "sf update" all the time, I wasn't aware of
the read - "partly modify" - write cycle. Maybe it's just me being
over cautious.

Printing a warning might scare users, though. I'd prefer to fix the
online help, where only "erase and write" is mentioned.

-michael

Re: [PATCH] cmd: sf: Support unaligned flash updates with 'sf update'

[Frieder Schrempf]

On 30.09.21 23:08, Michael Walle wrote:
> Am 2021-09-30 19:17, schrieb Frieder Schrempf:
>> On 30.09.21 18:35, Michael Walle wrote:
>>> Am 2021-09-30 18:19, schrieb Frieder Schrempf:
>>>> In order to support unaligned updates, we simply read the first full
>>>> block and check only the requested part of the block for changes. If
>>>> necessary, the block is erased, the first (unchanged) part of the block
>>>> is written back together with the second part of the block (updated
>>>> data).
>>>
>>> I'm not sure what I should think of this. You might loose that unchanged
>>> part if there is an power loss in the middle, even worse, you likely
>>> don't
>>> have this part anymore because it isn't part of the data you want to
>>> write
>>> to the flash.
>>>
>>> Maybe add an parameter for allow (unsafe) unaligned updates?
>>>
>>> Now you might argue, that with "sf erase, sf write" you can do the same
>>> harm, to which i reply: but then it is intentional ;) Because "sf erase"
>>> just works on sector boundaries (which isn't really checked in the
>>> command,
>>> i just realized, but at least my driver returns EINVAL) and then the
>>> user has to include the "unchanged part" for the arguments on the
>>> commandline.
>>
>> True, but "sf update" already is "unsafe" even without supporting
>> unaligned start offsets. The code already handles partial sector writes
>> for the last sector in the same way (read, erase, write), which is also
>> prone to data loss in case of power loss.
> 
> Ah, I missed that. Yes, in this case, we don't loose anything. Agreed.
> 
>> So this patch in fact just adds support for partial sector updates for
>> the first sector. It is slightly more probable to loose data, but it
>> doesn't introduce new "unsafe" behavior.
>>
>> Maybe we could cover this by adding a warning to the documentation, or
>> even printing a warning?
> 
> Heh, although I was using "sf update" all the time, I wasn't aware of
> the read - "partly modify" - write cycle. Maybe it's just me being
> over cautious.
> 
> Printing a warning might scare users, though. I'd prefer to fix the
> online help, where only "erase and write" is mentioned.

Which document are you referring to? I don't really see the "sf" command
documented anywhere.

Re: [PATCH] cmd: sf: Support unaligned flash updates with 'sf update'

[Michael Walle]

Am 2021-10-04 12:31, schrieb Frieder Schrempf:
> On 30.09.21 23:08, Michael Walle wrote:

>> Printing a warning might scare users, though. I'd prefer to fix the
>> online help, where only "erase and write" is mentioned.
> 
> Which document are you referring to? I don't really see the "sf" 
> command
> documented anywhere.

the "sf update" online help:

         "sf update addr offset|partition len    - erase and write `len' 
bytes from memory\n"
         "                                         at `addr' to flash at 
`offset'\n"
         "                                         or to start of mtd 
`partition'\n"

-michael

Re: [PATCH] cmd: sf: Support unaligned flash updates with 'sf update'

[Michael Walle]

Am 2021-09-30 18:19, schrieb Frieder Schrempf:
> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> 
> Currently 'sf update' supports only offsets that are aligned to the
> erase block size of the serial flash. Unaligned offsets result in
> something like:
> 
> => sf update ${kernel_addr_r} 0x400 ${filesize}
> device 0 offset 0x400, size 0x11f758
> SPI flash failed in erase step
> 
> In order to support unaligned updates, we simply read the first full
> block and check only the requested part of the block for changes. If
> necessary, the block is erased, the first (unchanged) part of the block
> is written back together with the second part of the block (updated 
> data).
> 
> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> ---
>  cmd/sf.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/cmd/sf.c b/cmd/sf.c
> index eac27ed2d7..c54b4b10bb 100644
> --- a/cmd/sf.c
> +++ b/cmd/sf.c
> @@ -171,30 +171,32 @@ static int do_spi_flash_probe(int argc, char
> *const argv[])
>  static const char *spi_flash_update_block(struct spi_flash *flash, u32 
> offset,
>  		size_t len, const char *buf, char *cmp_buf, size_t *skipped)
>  {
> +	u32 offset_aligned = ALIGN_DOWN(offset, flash->sector_size);
> +	u32 sector_offset = offset - offset_aligned;
>  	char *ptr = (char *)buf;
> 
>  	debug("offset=%#x, sector_size=%#x, len=%#zx\n",
>  	      offset, flash->sector_size, len);
>  	/* Read the entire sector so to allow for rewriting */
> -	if (spi_flash_read(flash, offset, flash->sector_size, cmp_buf))
> +	if (spi_flash_read(flash, offset_aligned, flash->sector_size, 
> cmp_buf))
>  		return "read";
>  	/* Compare only what is meaningful (len) */
> -	if (memcmp(cmp_buf, buf, len) == 0) {
> +	if (memcmp(cmp_buf + sector_offset, buf, len) == 0) {
>  		debug("Skip region %x size %zx: no change\n",
>  		      offset, len);
>  		*skipped += len;
>  		return NULL;
>  	}
>  	/* Erase the entire sector */
> -	if (spi_flash_erase(flash, offset, flash->sector_size))
> +	if (spi_flash_erase(flash, offset_aligned, flash->sector_size))
>  		return "erase";
>  	/* If it's a partial sector, copy the data into the temp-buffer */
>  	if (len != flash->sector_size) {
> -		memcpy(cmp_buf, buf, len);
> +		memcpy(cmp_buf + sector_offset, buf, len);
>  		ptr = cmp_buf;
>  	}
>  	/* Write one complete sector */
> -	if (spi_flash_write(flash, offset, flash->sector_size, ptr))
> +	if (spi_flash_write(flash, offset_aligned, flash->sector_size, ptr))
>  		return "write";
> 
>  	return NULL;
> @@ -230,7 +232,10 @@ static int spi_flash_update(struct spi_flash
> *flash, u32 offset,
>  		ulong last_update = get_timer(0);
> 
>  		for (; buf < end && !err_oper; buf += todo, offset += todo) {
> -			todo = min_t(size_t, end - buf, flash->sector_size);
> +			if (offset % flash->sector_size)

do_div() to avoid linking errors on some archs, I guess.

> +				todo = flash->sector_size - (offset % flash->sector_size);

This is missing the end-buf calculation, no? I.e. if you update just one
sector at an offset and the data you write is smaller than "sector_size 
- offset".

> +			else
> +				todo = min_t(size_t, end - buf, flash->sector_size);
>  			if (get_timer(last_update) > 100) {
>  				printf("   \rUpdating, %zu%% %lu B/s",
>  				       100 - (end - buf) / scale,

-michael

Re: [PATCH] cmd: sf: Support unaligned flash updates with 'sf update'

[Frieder Schrempf]

On 30.09.21 23:21, Michael Walle wrote:
> Am 2021-09-30 18:19, schrieb Frieder Schrempf:
>> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>>
>> Currently 'sf update' supports only offsets that are aligned to the
>> erase block size of the serial flash. Unaligned offsets result in
>> something like:
>>
>> => sf update ${kernel_addr_r} 0x400 ${filesize}
>> device 0 offset 0x400, size 0x11f758
>> SPI flash failed in erase step
>>
>> In order to support unaligned updates, we simply read the first full
>> block and check only the requested part of the block for changes. If
>> necessary, the block is erased, the first (unchanged) part of the block
>> is written back together with the second part of the block (updated
>> data).
>>
>> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
>> ---
>>  cmd/sf.c | 17 +++++++++++------
>>  1 file changed, 11 insertions(+), 6 deletions(-)
>>
>> diff --git a/cmd/sf.c b/cmd/sf.c
>> index eac27ed2d7..c54b4b10bb 100644
>> --- a/cmd/sf.c
>> +++ b/cmd/sf.c
>> @@ -171,30 +171,32 @@ static int do_spi_flash_probe(int argc, char
>> *const argv[])
>>  static const char *spi_flash_update_block(struct spi_flash *flash,
>> u32 offset,
>>          size_t len, const char *buf, char *cmp_buf, size_t *skipped)
>>  {
>> +    u32 offset_aligned = ALIGN_DOWN(offset, flash->sector_size);
>> +    u32 sector_offset = offset - offset_aligned;
>>      char *ptr = (char *)buf;
>>
>>      debug("offset=%#x, sector_size=%#x, len=%#zx\n",
>>            offset, flash->sector_size, len);
>>      /* Read the entire sector so to allow for rewriting */
>> -    if (spi_flash_read(flash, offset, flash->sector_size, cmp_buf))
>> +    if (spi_flash_read(flash, offset_aligned, flash->sector_size,
>> cmp_buf))
>>          return "read";
>>      /* Compare only what is meaningful (len) */
>> -    if (memcmp(cmp_buf, buf, len) == 0) {
>> +    if (memcmp(cmp_buf + sector_offset, buf, len) == 0) {
>>          debug("Skip region %x size %zx: no change\n",
>>                offset, len);
>>          *skipped += len;
>>          return NULL;
>>      }
>>      /* Erase the entire sector */
>> -    if (spi_flash_erase(flash, offset, flash->sector_size))
>> +    if (spi_flash_erase(flash, offset_aligned, flash->sector_size))
>>          return "erase";
>>      /* If it's a partial sector, copy the data into the temp-buffer */
>>      if (len != flash->sector_size) {
>> -        memcpy(cmp_buf, buf, len);
>> +        memcpy(cmp_buf + sector_offset, buf, len);
>>          ptr = cmp_buf;
>>      }
>>      /* Write one complete sector */
>> -    if (spi_flash_write(flash, offset, flash->sector_size, ptr))
>> +    if (spi_flash_write(flash, offset_aligned, flash->sector_size, ptr))
>>          return "write";
>>
>>      return NULL;
>> @@ -230,7 +232,10 @@ static int spi_flash_update(struct spi_flash
>> *flash, u32 offset,
>>          ulong last_update = get_timer(0);
>>
>>          for (; buf < end && !err_oper; buf += todo, offset += todo) {
>> -            todo = min_t(size_t, end - buf, flash->sector_size);
>> +            if (offset % flash->sector_size)
> 
> do_div() to avoid linking errors on some archs, I guess.

Ok, will fix it.

> 
>> +                todo = flash->sector_size - (offset %
>> flash->sector_size);
> 
> This is missing the end-buf calculation, no? I.e. if you update just one
> sector at an offset and the data you write is smaller than "sector_size
> - offset".

Indeed, thanks for catching this.

> 
>> +            else
>> +                todo = min_t(size_t, end - buf, flash->sector_size);
>>              if (get_timer(last_update) > 100) {
>>                  printf("   \rUpdating, %zu%% %lu B/s",
>>                         100 - (end - buf) / scale,
> 
> -michael

Re: [PATCH] cmd: sf: Support unaligned flash updates with 'sf update'

[Simon Glass]

Hi Frieder,

On Thu, 30 Sept 2021 at 10:20, Frieder Schrempf <frieder@fris.de> wrote:
>
> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>
> Currently 'sf update' supports only offsets that are aligned to the
> erase block size of the serial flash. Unaligned offsets result in
> something like:
>
> => sf update ${kernel_addr_r} 0x400 ${filesize}
> device 0 offset 0x400, size 0x11f758
> SPI flash failed in erase step

<insert motivation for patch here>

>
> In order to support unaligned updates, we simply read the first full
> block and check only the requested part of the block for changes. If
> necessary, the block is erased, the first (unchanged) part of the block
> is written back together with the second part of the block (updated data).
>
> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> ---
>  cmd/sf.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
>

Regards,
Simon