[U-Boot] [PATCH 1/2] fastboot: sparse: fix block addressing for don't care chunk type
When 7bfc3b1 (sparse: Refactor chunk parsing function) was implemented, it dropped 9981945 (aboot: fix block addressing for don't care chunk type).
This re-implements the required fix for the "don't care chunk type"...
Signed-off-by: Steve Rae srae@broadcom.com ---
common/image-sparse.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/common/image-sparse.c b/common/image-sparse.c index dffe844..f02aee4 100644 --- a/common/image-sparse.c +++ b/common/image-sparse.c @@ -330,9 +330,11 @@ int store_sparse_image(sparse_storage_t *storage, void *storage_priv, * and go on parsing the rest of the chunks */ if (chunk_header->chunk_type == CHUNK_TYPE_DONT_CARE) { - skipped += sparse_block_size_to_storage(chunk_header->chunk_sz, - storage, - sparse_header); + blkcnt = sparse_block_size_to_storage(chunk_header->chunk_sz, + storage, + sparse_header); + total_blocks += blkcnt; + skipped += blkcnt; continue; }
@@ -380,7 +382,7 @@ int store_sparse_image(sparse_storage_t *storage, void *storage_priv, printf("........ wrote %d blocks to '%s'\n", total_blocks, storage->name);
- if ((total_blocks + skipped) != + if (total_blocks != sparse_block_size_to_storage(sparse_header->total_blks, storage, sparse_header)) { printf("sparse image write failure\n");
remove logging of the 'skipped' blocks
Signed-off-by: Steve Rae srae@broadcom.com ---
common/image-sparse.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/common/image-sparse.c b/common/image-sparse.c index f02aee4..594bf4e 100644 --- a/common/image-sparse.c +++ b/common/image-sparse.c @@ -275,7 +275,6 @@ int store_sparse_image(sparse_storage_t *storage, void *storage_priv, sparse_buffer_t *buffer; uint32_t start; uint32_t total_blocks = 0; - uint32_t skipped = 0; int i;
debug("=== Storage ===\n"); @@ -334,7 +333,6 @@ int store_sparse_image(sparse_storage_t *storage, void *storage_priv, storage, sparse_header); total_blocks += blkcnt; - skipped += blkcnt; continue; }
@@ -375,8 +373,8 @@ int store_sparse_image(sparse_storage_t *storage, void *storage_priv, sparse_put_data_buffer(buffer); }
- debug("Wrote %d blocks, skipped %d, expected to write %d blocks\n", - total_blocks, skipped, + debug("Wrote %d blocks, expected to write %d blocks\n", + total_blocks, sparse_block_size_to_storage(sparse_header->total_blks, storage, sparse_header)); printf("........ wrote %d blocks to '%s'\n", total_blocks,
Hi Steve,
On Wed, Feb 03, 2016 at 12:46:02PM -0800, Steve Rae wrote:
remove logging of the 'skipped' blocks
Signed-off-by: Steve Rae srae@broadcom.com
common/image-sparse.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/common/image-sparse.c b/common/image-sparse.c index f02aee4..594bf4e 100644 --- a/common/image-sparse.c +++ b/common/image-sparse.c @@ -275,7 +275,6 @@ int store_sparse_image(sparse_storage_t *storage, void *storage_priv, sparse_buffer_t *buffer; uint32_t start; uint32_t total_blocks = 0;
uint32_t skipped = 0; int i;
debug("=== Storage ===\n");
@@ -334,7 +333,6 @@ int store_sparse_image(sparse_storage_t *storage, void *storage_priv, storage, sparse_header); total_blocks += blkcnt;
skipped += blkcnt; continue;@@ -375,8 +373,8 @@ int store_sparse_image(sparse_storage_t *storage, void *storage_priv, sparse_put_data_buffer(buffer); }
- debug("Wrote %d blocks, skipped %d, expected to write %d blocks\n",
total_blocks, skipped,
- debug("Wrote %d blocks, expected to write %d blocks\n",
total_blocks,
What's the rationale between those two patches?
Do we really want to treat the DONT_CARE chunks as if they were written?
Maxime
Hi Maxime,
On Thu, Feb 4, 2016 at 4:20 AM, Maxime Ripard < maxime.ripard@free-electrons.com> wrote:
Hi Steve,
On Wed, Feb 03, 2016 at 12:46:02PM -0800, Steve Rae wrote:
remove logging of the 'skipped' blocks
Signed-off-by: Steve Rae srae@broadcom.com
common/image-sparse.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/common/image-sparse.c b/common/image-sparse.c index f02aee4..594bf4e 100644 --- a/common/image-sparse.c +++ b/common/image-sparse.c @@ -275,7 +275,6 @@ int store_sparse_image(sparse_storage_t *storage,
void *storage_priv,
sparse_buffer_t *buffer; uint32_t start; uint32_t total_blocks = 0;
uint32_t skipped = 0; int i; debug("=== Storage ===\n");@@ -334,7 +333,6 @@ int store_sparse_image(sparse_storage_t *storage,
void *storage_priv,
storage,sparse_header);
total_blocks += blkcnt;
This change (in the first patch), updates the "total_blocks" value, so that the "next" chunk has the proper "starting block" address (see these line 363...) 362 ret = storage->write(storage, storage_priv, 363 start + total_blocks, 364 buffer_blk_cnt, 365 buffer->data); Without this change, all the blocks written to the partition after the CHUNK_TYPE_DONT_CARE blocks are corrupted (they are not in the correct location). So, even though we are not actually writing any blocks to this space, the space must be maintained!
(Recently, I am now understanding that with NAND, there may be more complications; probably cannot just increment the "total_blocks" -- I suspect that it is required to actually determine if there are bad blocks in this space, and update the "total_blocks" value accordingly....)
skipped += blkcnt;continue; }@@ -375,8 +373,8 @@ int store_sparse_image(sparse_storage_t *storage,
void *storage_priv,
sparse_put_data_buffer(buffer); }
debug("Wrote %d blocks, skipped %d, expected to write %d blocks\n",total_blocks, skipped,
debug("Wrote %d blocks, expected to write %d blocks\n",total_blocks,What's the rationale between those two patches?
see inline comment above
Do we really want to treat the DONT_CARE chunks as if they were written?
I suspect that we do, and "sparse_header->total_blks" actually includes them in the count too... This "total_blocks" count is actually the number of blocks "processed" (which may or may not include actually writing to the partition). IMO - I think counting the "skipped blocks is unnecessary.
Thanks, Steve
Maxime
-- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com
Hi Steve,
On Thu, Feb 04, 2016 at 10:51:00AM -0800, Steve Rae wrote:
Hi Maxime,
On Thu, Feb 4, 2016 at 4:20 AM, Maxime Ripard < maxime.ripard@free-electrons.com> wrote:
Hi Steve,
On Wed, Feb 03, 2016 at 12:46:02PM -0800, Steve Rae wrote:
remove logging of the 'skipped' blocks
Signed-off-by: Steve Rae srae@broadcom.com
common/image-sparse.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/common/image-sparse.c b/common/image-sparse.c index f02aee4..594bf4e 100644 --- a/common/image-sparse.c +++ b/common/image-sparse.c @@ -275,7 +275,6 @@ int store_sparse_image(sparse_storage_t *storage,
void *storage_priv,
sparse_buffer_t *buffer; uint32_t start; uint32_t total_blocks = 0;
uint32_t skipped = 0; int i; debug("=== Storage ===\n");@@ -334,7 +333,6 @@ int store_sparse_image(sparse_storage_t *storage,
void *storage_priv,
storage,sparse_header);
total_blocks += blkcnt;This change (in the first patch), updates the "total_blocks" value, so that the "next" chunk has the proper "starting block" address (see these line 363...) 362 ret = storage->write(storage, storage_priv, 363 start + total_blocks, 364 buffer_blk_cnt, 365 buffer->data); Without this change, all the blocks written to the partition after the CHUNK_TYPE_DONT_CARE blocks are corrupted (they are not in the correct location). So, even though we are not actually writing any blocks to this space, the space must be maintained!
Ah, yeah, understood.
I'm guessing it was working in my case since I had no DONT_CARE chunks in the first sparse image sent, and then only DONT_CARE chunks for the space you already wrote, we got that covered by last_offset... :/
So, yeah, it's broken...
(Recently, I am now understanding that with NAND, there may be more complications; probably cannot just increment the "total_blocks" -- I suspect that it is required to actually determine if there are bad blocks in this space, and update the "total_blocks" value accordingly....)
Yes, if you try to write to a bad block on NAND, you're actually going to write to the next block, which will introduce some offset, or you'll going to write to a block that's already been written.
Maxime
skipped += blkcnt;continue; }@@ -375,8 +373,8 @@ int store_sparse_image(sparse_storage_t *storage,
void *storage_priv,
sparse_put_data_buffer(buffer); }
debug("Wrote %d blocks, skipped %d, expected to write %d blocks\n",total_blocks, skipped,
debug("Wrote %d blocks, expected to write %d blocks\n",total_blocks,What's the rationale between those two patches?
see inline comment above
Do we really want to treat the DONT_CARE chunks as if they were written?
I suspect that we do, and "sparse_header->total_blks" actually includes them in the count too... This "total_blocks" count is actually the number of blocks "processed" (which may or may not include actually writing to the partition). IMO - I think counting the "skipped blocks is unnecessary.
Ok, sounds good.
Thanks! Maxime
Hi Maxime,
On Mon, Feb 8, 2016 at 12:19 AM, Maxime Ripard < maxime.ripard@free-electrons.com> wrote:
Hi Steve,
On Thu, Feb 04, 2016 at 10:51:00AM -0800, Steve Rae wrote:
Hi Maxime,
On Thu, Feb 4, 2016 at 4:20 AM, Maxime Ripard < maxime.ripard@free-electrons.com> wrote:
Hi Steve,
On Wed, Feb 03, 2016 at 12:46:02PM -0800, Steve Rae wrote:
remove logging of the 'skipped' blocks
Signed-off-by: Steve Rae srae@broadcom.com
common/image-sparse.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/common/image-sparse.c b/common/image-sparse.c index f02aee4..594bf4e 100644 --- a/common/image-sparse.c +++ b/common/image-sparse.c @@ -275,7 +275,6 @@ int store_sparse_image(sparse_storage_t *storage,
void *storage_priv,
sparse_buffer_t *buffer; uint32_t start; uint32_t total_blocks = 0;
uint32_t skipped = 0; int i; debug("=== Storage ===\n");@@ -334,7 +333,6 @@ int store_sparse_image(sparse_storage_t *storage,
void *storage_priv,
storage,sparse_header);
total_blocks += blkcnt;This change (in the first patch), updates the "total_blocks" value, so
that
the "next" chunk has the proper "starting block" address (see these line 363...) 362 ret = storage->write(storage, storage_priv, 363 start + total_blocks, 364 buffer_blk_cnt, 365 buffer->data); Without this change, all the blocks written to the partition after the CHUNK_TYPE_DONT_CARE blocks are corrupted (they are not in the correct location). So, even though we are not actually writing any blocks to this space, the space must be maintained!
Ah, yeah, understood.
I'm guessing it was working in my case since I had no DONT_CARE chunks in the first sparse image sent, and then only DONT_CARE chunks for the space you already wrote, we got that covered by last_offset... :/
So, yeah, it's broken...
(Recently, I am now understanding that with NAND, there may be more complications; probably cannot just increment the "total_blocks" -- I suspect that it is required to actually determine if there are bad blocks in this space, and update the "total_blocks" value accordingly....)
Yes, if you try to write to a bad block on NAND, you're actually going to write to the next block, which will introduce some offset, or you'll going to write to a block that's already been written.
Maxime
So, to handle MMC versus NAND, I propose that we follow the same method used throughout 'fastboot':
+#ifdef CONFIG_FASTBOOT_FLASH_MMC_DEV total_blocks += blkcnt; +#endif +#ifdef CONFIG_FASTBOOT_FLASH_NAND_DEV + /* TBD */ +#endif
What do you think? I can submit a "v2" -- Could you create a patch for the the NAND part?
Thanks in advance, Steve
skipped += blkcnt;continue; }@@ -375,8 +373,8 @@ int store_sparse_image(sparse_storage_t *storage,
void *storage_priv,
sparse_put_data_buffer(buffer); }
debug("Wrote %d blocks, skipped %d, expected to write %dblocks\n",
total_blocks, skipped,
debug("Wrote %d blocks, expected to write %d blocks\n",total_blocks,What's the rationale between those two patches?
see inline comment above
Do we really want to treat the DONT_CARE chunks as if they were written?
I suspect that we do, and "sparse_header->total_blks" actually includes them in the count too... This "total_blocks" count is actually the number of blocks "processed" (which may or may not include actually writing to the partition). IMO - I think counting the "skipped blocks is unnecessary.
Ok, sounds good.
Thanks! Maxime
-- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com
Hi,
On Mon, Feb 08, 2016 at 10:04:03AM -0800, Steve Rae wrote:
Hi Maxime,
On Mon, Feb 8, 2016 at 12:19 AM, Maxime Ripard < maxime.ripard@free-electrons.com> wrote:
Hi Steve,
On Thu, Feb 04, 2016 at 10:51:00AM -0800, Steve Rae wrote:
Hi Maxime,
On Thu, Feb 4, 2016 at 4:20 AM, Maxime Ripard < maxime.ripard@free-electrons.com> wrote:
Hi Steve,
On Wed, Feb 03, 2016 at 12:46:02PM -0800, Steve Rae wrote:
remove logging of the 'skipped' blocks
Signed-off-by: Steve Rae srae@broadcom.com
common/image-sparse.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/common/image-sparse.c b/common/image-sparse.c index f02aee4..594bf4e 100644 --- a/common/image-sparse.c +++ b/common/image-sparse.c @@ -275,7 +275,6 @@ int store_sparse_image(sparse_storage_t *storage,
void *storage_priv,
sparse_buffer_t *buffer; uint32_t start; uint32_t total_blocks = 0;
uint32_t skipped = 0; int i; debug("=== Storage ===\n");@@ -334,7 +333,6 @@ int store_sparse_image(sparse_storage_t *storage,
void *storage_priv,
storage,sparse_header);
total_blocks += blkcnt;This change (in the first patch), updates the "total_blocks" value, so
that
the "next" chunk has the proper "starting block" address (see these line 363...) 362 ret = storage->write(storage, storage_priv, 363 start + total_blocks, 364 buffer_blk_cnt, 365 buffer->data); Without this change, all the blocks written to the partition after the CHUNK_TYPE_DONT_CARE blocks are corrupted (they are not in the correct location). So, even though we are not actually writing any blocks to this space, the space must be maintained!
Ah, yeah, understood.
I'm guessing it was working in my case since I had no DONT_CARE chunks in the first sparse image sent, and then only DONT_CARE chunks for the space you already wrote, we got that covered by last_offset... :/
So, yeah, it's broken...
(Recently, I am now understanding that with NAND, there may be more complications; probably cannot just increment the "total_blocks" -- I suspect that it is required to actually determine if there are bad blocks in this space, and update the "total_blocks" value accordingly....)
Yes, if you try to write to a bad block on NAND, you're actually going to write to the next block, which will introduce some offset, or you'll going to write to a block that's already been written.
Maxime
So, to handle MMC versus NAND, I propose that we follow the same method used throughout 'fastboot':
+#ifdef CONFIG_FASTBOOT_FLASH_MMC_DEV total_blocks += blkcnt; +#endif +#ifdef CONFIG_FASTBOOT_FLASH_NAND_DEV
/* TBD */+#endif
Eventually, we should support both. But is it even broken now? It was working just fine last time I tried. The write function is supposed to return the adjusted number of blocks that the write actually used (bad blocks included). Am I missing something?
Maxime
On Tue, Feb 9, 2016 at 9:17 AM, Maxime Ripard < maxime.ripard@free-electrons.com> wrote:
Hi,
On Mon, Feb 08, 2016 at 10:04:03AM -0800, Steve Rae wrote:
Hi Maxime,
On Mon, Feb 8, 2016 at 12:19 AM, Maxime Ripard < maxime.ripard@free-electrons.com> wrote:
Hi Steve,
On Thu, Feb 04, 2016 at 10:51:00AM -0800, Steve Rae wrote:
Hi Maxime,
On Thu, Feb 4, 2016 at 4:20 AM, Maxime Ripard < maxime.ripard@free-electrons.com> wrote:
Hi Steve,
On Wed, Feb 03, 2016 at 12:46:02PM -0800, Steve Rae wrote:
remove logging of the 'skipped' blocks
Signed-off-by: Steve Rae srae@broadcom.com
common/image-sparse.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/common/image-sparse.c b/common/image-sparse.c index f02aee4..594bf4e 100644 --- a/common/image-sparse.c +++ b/common/image-sparse.c @@ -275,7 +275,6 @@ int store_sparse_image(sparse_storage_t
*storage,
void *storage_priv,
sparse_buffer_t *buffer; uint32_t start; uint32_t total_blocks = 0;
uint32_t skipped = 0; int i; debug("=== Storage ===\n");@@ -334,7 +333,6 @@ int store_sparse_image(sparse_storage_t
*storage,
void *storage_priv,
storage,
sparse_header);
total_blocks += blkcnt;This change (in the first patch), updates the "total_blocks" value,
so
that
the "next" chunk has the proper "starting block" address (see these line 363...) 362 ret = storage->write(storage,
storage_priv,
363 start +
total_blocks,
364 buffer_blk_cnt, 365 buffer->data); Without this change, all the blocks written to the partition after
the
CHUNK_TYPE_DONT_CARE blocks are corrupted (they are not in the
correct
location). So, even though we are not actually writing any blocks to this
space, the
space must be maintained!
Ah, yeah, understood.
I'm guessing it was working in my case since I had no DONT_CARE chunks in the first sparse image sent, and then only DONT_CARE chunks for the space you already wrote, we got that covered by last_offset... :/
So, yeah, it's broken...
(Recently, I am now understanding that with NAND, there may be more complications; probably cannot just increment the "total_blocks" -- I suspect that it is required to actually determine if there are bad
blocks
in this space, and update the "total_blocks" value accordingly....)
Yes, if you try to write to a bad block on NAND, you're actually going to write to the next block, which will introduce some offset, or you'll going to write to a block that's already been written.
Maxime
So, to handle MMC versus NAND, I propose that we follow the same method used throughout 'fastboot':
+#ifdef CONFIG_FASTBOOT_FLASH_MMC_DEV total_blocks += blkcnt; +#endif +#ifdef CONFIG_FASTBOOT_FLASH_NAND_DEV
/* TBD */+#endif
Eventually, we should support both. But is it even broken now? It was working just fine last time I tried. The write function is supposed to return the adjusted number of blocks that the write actually used (bad blocks included). Am I missing something?
Maxime
Yes - it is broken now -- there is no "write function" in this CHUNK_TYPE_DONT_CARE
logic.... It is simply updating the total_blocks value, so that the start position for the "next" CHUNK is at the correct location.... Thanks, Steve
-- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com
On Tue, Feb 09, 2016 at 09:58:10AM -0800, Steve Rae wrote:
So, to handle MMC versus NAND, I propose that we follow the same method used throughout 'fastboot':
+#ifdef CONFIG_FASTBOOT_FLASH_MMC_DEV total_blocks += blkcnt; +#endif +#ifdef CONFIG_FASTBOOT_FLASH_NAND_DEV
/* TBD */+#endif
Eventually, we should support both. But is it even broken now? It was working just fine last time I tried. The write function is supposed to return the adjusted number of blocks that the write actually used (bad blocks included). Am I missing something?
Yes - it is broken now -- there is no "write function" in this CHUNK_TYPE_DONT_CARE logic....
Ah, yes, in the case where the block we skip is bad, and we should skip yet another block.
Jeffy also had an issue with the session_id that required to honour DONT_CARE to handle the case where you chain fastboot commands as part of one sessions. It should probably fix this issue as well.
On Tue, Feb 9, 2016 at 12:18 PM, Maxime Ripard < maxime.ripard@free-electrons.com> wrote:
On Tue, Feb 09, 2016 at 09:58:10AM -0800, Steve Rae wrote:
So, to handle MMC versus NAND, I propose that we follow the same
method
used throughout 'fastboot':
+#ifdef CONFIG_FASTBOOT_FLASH_MMC_DEV total_blocks += blkcnt; +#endif +#ifdef CONFIG_FASTBOOT_FLASH_NAND_DEV
/* TBD */+#endif
Eventually, we should support both. But is it even broken now? It was working just fine last time I tried. The write function is supposed to return the adjusted number of blocks that the write actually used (bad blocks included). Am I missing something?
Yes - it is broken now -- there is no "write function" in this CHUNK_TYPE_DONT_CARE logic....
Ah, yes, in the case where the block we skip is bad, and we should skip yet another block.
Jeffy also had an issue with the session_id that required to honour DONT_CARE to handle the case where you chain fastboot commands as part of one sessions. It should probably fix this issue as well.
yes -- I see this patch from jeffy (on Feb 2)
fastboot: sparse: fix chunk write offset calculation I haven't tested it, but it seems to ignore the "session_id" completely, and write each session, starting from "storage->start"... And since his "wrote_blocks" starts at 0 for each session, I don't think that is what we want.... Thanks, Steve PS. I have submitted a "v2" !!!
--
Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com
participants (3)
-
Maxime Ripard -
Steve Rae -
Steve Rae