fastboot: sparse: fix block addressing for don't care chunk type

PATCH Superseded Steve Rae 3 participants 10 messages Feb 3, 2016 – Feb 9, 2016

Share & tools

Steve Rae

author

2016-02-03 20:46 UTC #0 lore raw

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 <[email protected]>

---

 common/image-sparse.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

common/image-sparse.c +6 -4

index dffe844..f02aee4 100644
@@ -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");

Signature · 1 line

1.8.5

Steve Rae

author

2016-02-03 20:46 UTC #1 lore raw

PATCH 2/2 fastboot: sparse: remove unnecessary logging

remove logging of the 'skipped' blocks

Signed-off-by Steve Rae <[email protected]>

---

 common/image-sparse.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

common/image-sparse.c +2 -4

index f02aee4..594bf4e 100644
@@ -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,

Signature · 1 line

1.8.5

Maxime Ripard

2016-02-04 12:20 UTC #2 lore raw

PATCH 2/2 fastboot: sparse: remove unnecessary logging

Hi Steve,

Quoted 29 lines · Steve Rae

remove logging of the 'skipped' blocks

Signed-off-by Steve Rae <[email protected]>

---

 common/image-sparse.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

common/image-sparse.c +0 -2

index f02aee4..594bf4e 100644
@@ -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;
 		}

common/image-sparse.c int store_sparse_image(sparse_storage_t *storage, void *storage_priv, · line 377

Quoted 7 lines

common/image-sparse.c +2 -2

@@ -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

Signature · 10 lines

Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160204/08f4b982/attachment.sig>

Steve Rae

2016-02-04 18:51 UTC #3 lore raw

PATCH 2/2 fastboot: sparse: remove unnecessary logging

Hi Maxime,

Quoted 23 lines · Maxime Ripard

Hi Steve,
common/image-sparse.c int store_sparse_image(sparse_storage_t *storage,
Quoted 13 lines · Steve Rae
remove logging of the 'skipped' blocks
Signed-off-by Steve Rae <[email protected]>
---
 common/image-sparse.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
common/image-sparse.c +0 -0
index f02aee4..594bf4e 100644
@@ -275,7 +275,6 @@ int store_sparse_image(sparse_storage_t *storage,
void *storage_priv,
Quoted 6 lines
common/image-sparse.c +0 -1
      sparse_buffer_t *buffer;
      uint32_t start;
      uint32_t total_blocks = 0;
-     uint32_t skipped = 0;
      int i;
 

Quoted 8 lines

common/image-sparse.c int store_sparse_image(sparse_storage_t *storage,
Quoted 2 lines
common/image-sparse.c +0 -0
      debug("=== Storage ===\n");
@@ -334,7 +333,6 @@ int store_sparse_image(sparse_storage_t *storage,
void *storage_priv,
common/image-sparse.c int store_sparse_image(sparse_storage_t *storage, void *storage_priv, · line 338
Quoted 2 lines
common/image-sparse.c +0 -0
                                                            storage,
 
sparse_header);
Quoted 1 line
common/image-sparse.c +0 -0
                      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....)

Quoted 9 lines

- skipped += blkcnt;
common/image-sparse.c int store_sparse_image(sparse_storage_t *storage,
Quoted 4 lines
common/image-sparse.c +0 -0
                      continue;
              }
 
@@ -375,8 +373,8 @@ int store_sparse_image(sparse_storage_t *storage,
void *storage_priv,
Quoted 3 lines
common/image-sparse.c +0 -0
              sparse_put_data_buffer(buffer);
      }
 

Quoted 7 lines

common/image-sparse.c int store_sparse_image(sparse_storage_t *storage, void *storage_priv, · line 377
Quoted 4 lines
common/image-sparse.c +2 -2
-     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

Quoted 4 lines

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

Quoted 7 lines

Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Maxime Ripard

2016-02-08 08:19 UTC #4 lore raw

PATCH 2/2 fastboot: sparse: remove unnecessary logging

Hi Steve,

Quoted 44 lines · Steve Rae

Hi Maxime,
Quoted 23 lines · Maxime Ripard
Hi Steve,
common/image-sparse.c int store_sparse_image(sparse_storage_t *storage,
Quoted 13 lines · Steve Rae
remove logging of the 'skipped' blocks
Signed-off-by Steve Rae <[email protected]>
---
 common/image-sparse.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
common/image-sparse.c +0 -0
index f02aee4..594bf4e 100644
@@ -275,7 +275,6 @@ int store_sparse_image(sparse_storage_t *storage,
void *storage_priv,
Quoted 6 lines
common/image-sparse.c +0 -1
      sparse_buffer_t *buffer;
      uint32_t start;
      uint32_t total_blocks = 0;
-     uint32_t skipped = 0;
      int i;
 
Quoted 8 lines
common/image-sparse.c int store_sparse_image(sparse_storage_t *storage,
Quoted 2 lines
common/image-sparse.c +0 -0
      debug("=== Storage ===\n");
@@ -334,7 +333,6 @@ int store_sparse_image(sparse_storage_t *storage,
void *storage_priv,
common/image-sparse.c int store_sparse_image(sparse_storage_t *storage, void *storage_priv, · line 338
Quoted 2 lines
common/image-sparse.c +0 -0
                                                            storage,
 
sparse_header);
Quoted 1 line
common/image-sparse.c +0 -0
                      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);

Quoted 5 lines

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...

Quoted 4 lines

(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

Quoted 25 lines

Quoted 9 lines
- skipped += blkcnt;
common/image-sparse.c int store_sparse_image(sparse_storage_t *storage,
Quoted 4 lines
common/image-sparse.c +0 -0
                      continue;
              }
 
@@ -375,8 +373,8 @@ int store_sparse_image(sparse_storage_t *storage,
void *storage_priv,
Quoted 3 lines
common/image-sparse.c +0 -0
              sparse_put_data_buffer(buffer);
      }
 
Quoted 7 lines
common/image-sparse.c int store_sparse_image(sparse_storage_t *storage, void *storage_priv, · line 377
Quoted 4 lines
common/image-sparse.c +2 -2
-     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
Quoted 4 lines
Do we really want to treat the DONT_CARE chunks as if they were written?

Quoted 5 lines

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

Signature · 10 lines

Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160208/bf45240b/attachment.sig>

Steve Rae

2016-02-08 18:04 UTC #5 lore raw

PATCH 2/2 fastboot: sparse: remove unnecessary logging

Hi Maxime,

Quoted 67 lines · Maxime Ripard

Hi Steve,
Quoted 31 lines · Steve Rae
Hi Maxime,
Quoted 26 lines · Maxime Ripard
Hi Steve,
common/image-sparse.c int store_sparse_image(sparse_storage_t *storage,
Quoted 13 lines · Steve Rae
remove logging of the 'skipped' blocks
Signed-off-by Steve Rae <[email protected]>
---
 common/image-sparse.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
common/image-sparse.c +0 -0
index f02aee4..594bf4e 100644
@@ -275,7 +275,6 @@ int store_sparse_image(sparse_storage_t *storage,
void *storage_priv,
common/image-sparse.c int store_sparse_image(sparse_storage_t *storage,
Quoted 8 lines
common/image-sparse.c +0 -1
      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,
Quoted 7 lines
Quoted 5 lines
common/image-sparse.c int store_sparse_image(sparse_storage_t *storage, void *storage_priv, · line 338
Quoted 2 lines
common/image-sparse.c +0 -0
                                                            storage,
 
sparse_header);
Quoted 1 line
common/image-sparse.c +0 -0
                      total_blocks += blkcnt;
This change (in the first patch), updates the "total_blocks" value, so
that
Quoted 11 lines
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...
Quoted 4 lines
(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....)

Quoted 7 lines

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

Quoted 42 lines

Quoted 10 lines
Quoted 10 lines
- skipped += blkcnt;
common/image-sparse.c int store_sparse_image(sparse_storage_t *storage,
Quoted 4 lines
common/image-sparse.c +0 -0
                      continue;
              }
 
@@ -375,8 +373,8 @@ int store_sparse_image(sparse_storage_t *storage,
void *storage_priv,
Quoted 4 lines
common/image-sparse.c +0 -1
              sparse_put_data_buffer(buffer);
      }
 
-     debug("Wrote %d blocks, skipped %d, expected to write %d
blocks\n",
Quoted 15 lines
Quoted 6 lines
common/image-sparse.c int store_sparse_image(sparse_storage_t *storage, void *storage_priv, · line 377
Quoted 3 lines
common/image-sparse.c +2 -1
-           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
Quoted 4 lines
Do we really want to treat the DONT_CARE chunks as if they were written?
Quoted 5 lines
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

Maxime Ripard

2016-02-09 17:17 UTC #6 lore raw

PATCH 2/2 fastboot: sparse: remove unnecessary logging

Hi,

Quoted 82 lines · Steve Rae

Hi Maxime,
Quoted 67 lines · Maxime Ripard
Hi Steve,
Quoted 31 lines · Steve Rae
Hi Maxime,
Quoted 26 lines · Maxime Ripard
Hi Steve,
common/image-sparse.c int store_sparse_image(sparse_storage_t *storage,
Quoted 13 lines · Steve Rae
remove logging of the 'skipped' blocks
Signed-off-by Steve Rae <[email protected]>
---
 common/image-sparse.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
common/image-sparse.c +0 -0
index f02aee4..594bf4e 100644
@@ -275,7 +275,6 @@ int store_sparse_image(sparse_storage_t *storage,
void *storage_priv,
common/image-sparse.c int store_sparse_image(sparse_storage_t *storage,
Quoted 8 lines
common/image-sparse.c +0 -1
      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,
Quoted 7 lines
Quoted 5 lines
common/image-sparse.c int store_sparse_image(sparse_storage_t *storage, void *storage_priv, · line 338
Quoted 2 lines
common/image-sparse.c +0 -0
                                                            storage,
 
sparse_header);
Quoted 1 line
common/image-sparse.c +0 -0
                      total_blocks += blkcnt;
This change (in the first patch), updates the "total_blocks" value, so
that
Quoted 11 lines
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...
Quoted 4 lines
(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....)
Quoted 7 lines
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':

Review

Quoted 6 lines

(quoted diff) +5 -0

+#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

Signature · 10 lines

Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160209/960a99f3/attachment.sig>

Steve Rae

2016-02-09 17:58 UTC #7 lore raw

PATCH 2/2 fastboot: sparse: remove unnecessary logging

Quoted 107 lines · Maxime Ripard

Hi,
Quoted 21 lines · Steve Rae
Hi Maxime,
Quoted 16 lines · Maxime Ripard
Hi Steve,
Quoted 13 lines · Steve Rae
Hi Maxime,
Quoted 8 lines · Maxime Ripard
Hi Steve,
Quoted 5 lines · Steve Rae
remove logging of the 'skipped' blocks
Signed-off-by Steve Rae <[email protected]>
---
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
common/image-sparse.c int store_sparse_image(sparse_storage_t
Quoted 8 lines
 common/image-sparse.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
common/image-sparse.c +0 -0
index f02aee4..594bf4e 100644
@@ -275,7 +275,6 @@ int store_sparse_image(sparse_storage_t
*storage,
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
void *storage_priv,
common/image-sparse.c int store_sparse_image(sparse_storage_t
Quoted 8 lines
common/image-sparse.c +0 -1
      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,
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
void *storage_priv,
Quoted 1 line
storage,
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
sparse_header);
Quoted 1 line
                      total_blocks += blkcnt;
This change (in the first patch), updates the "total_blocks" value,
so
Quoted 4 lines
Quoted 4 lines
that
Quoted 3 lines
the "next" chunk has the proper "starting block" address
    (see these line 363...)
362 ret = storage->write(storage,
storage_priv,
Quoted 1 line
Quoted 1 line
Quoted 1 line
363 start +
total_blocks,
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
364 buffer_blk_cnt,
365 buffer->data);
Without this change, all the blocks written to the partition after
the
Quoted 1 line
Quoted 1 line
Quoted 1 line
CHUNK_TYPE_DONT_CARE blocks are corrupted (they are not in the
correct
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
location).
So, even though we are not actually writing any blocks to this
space, the
Quoted 8 lines
Quoted 8 lines
Quoted 1 line
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... :/
Quoted 5 lines
Quoted 5 lines
So, yeah, it's broken...
Quoted 3 lines
(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
Quoted 12 lines
Quoted 9 lines
Quoted 1 line
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':
Review
Quoted 6 lines
(quoted diff) +5 -0
+#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?

Quoted 3 lines

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

Quoted 5 lines

--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Maxime Ripard

2016-02-09 20:18 UTC #8 lore raw

PATCH 2/2 fastboot: sparse: remove unnecessary logging

Quoted 15 lines · Steve Rae

Quoted 15 lines
Review
Quoted 9 lines
So, to handle MMC versus NAND, I propose that we follow the same method used throughout 'fastboot':
(quoted diff) +5 -0
+#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?

Quoted 2 lines

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.

Signature · 10 lines

Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160209/2dc545c2/attachment.sig>

Steve Rae

2016-02-09 21:44 UTC #9 lore raw

PATCH 2/2 fastboot: sparse: remove unnecessary logging

Quoted 20 lines · Maxime Ripard

Quoted 1 line · Steve Rae
Quoted 1 line
Quoted 1 line
So, to handle MMC versus NAND, I propose that we follow the same
method
Quoted 14 lines
Quoted 14 lines
Review
Quoted 8 lines
used throughout 'fastboot':
(quoted diff) +5 -0
+#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?
Quoted 2 lines
Yes - it is broken now -- there is no "write function" in this CHUNK_TYPE_DONT_CARE logic....

Quoted 8 lines

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" !!!

Quoted 4 lines

Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com