* [U-Boot] [PATCH 1/2] fastboot: sparse: fix block addressing for don't care chunk type @ 2016-02-03 20:46 Steve Rae 2016-02-03 20:46 ` [U-Boot] [PATCH 2/2] fastboot: sparse: remove unnecessary logging Steve Rae 0 siblings, 1 reply; 10+ messages in thread From: Steve Rae @ 2016-02-03 20:46 UTC (permalink / raw) To: u-boot 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"); -- 1.8.5 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH 2/2] fastboot: sparse: remove unnecessary logging 2016-02-03 20:46 [U-Boot] [PATCH 1/2] fastboot: sparse: fix block addressing for don't care chunk type Steve Rae @ 2016-02-03 20:46 ` Steve Rae 2016-02-04 12:20 ` Maxime Ripard 0 siblings, 1 reply; 10+ messages in thread From: Steve Rae @ 2016-02-03 20:46 UTC (permalink / raw) To: u-boot 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, -- 1.8.5 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH 2/2] fastboot: sparse: remove unnecessary logging 2016-02-03 20:46 ` [U-Boot] [PATCH 2/2] fastboot: sparse: remove unnecessary logging Steve Rae @ 2016-02-04 12:20 ` Maxime Ripard 2016-02-04 18:51 ` Steve Rae 0 siblings, 1 reply; 10+ messages in thread From: Maxime Ripard @ 2016-02-04 12:20 UTC (permalink / raw) To: u-boot 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 -- 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> ^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH 2/2] fastboot: sparse: remove unnecessary logging 2016-02-04 12:20 ` Maxime Ripard @ 2016-02-04 18:51 ` Steve Rae 2016-02-08 8:19 ` Maxime Ripard 0 siblings, 1 reply; 10+ messages in thread From: Steve Rae @ 2016-02-04 18:51 UTC (permalink / raw) To: u-boot 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 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH 2/2] fastboot: sparse: remove unnecessary logging 2016-02-04 18:51 ` Steve Rae @ 2016-02-08 8:19 ` Maxime Ripard 2016-02-08 18:04 ` Steve Rae 0 siblings, 1 reply; 10+ messages in thread From: Maxime Ripard @ 2016-02-08 8:19 UTC (permalink / raw) To: u-boot 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 at 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 -- 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> ^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH 2/2] fastboot: sparse: remove unnecessary logging 2016-02-08 8:19 ` Maxime Ripard @ 2016-02-08 18:04 ` Steve Rae 2016-02-09 17:17 ` Maxime Ripard 0 siblings, 1 reply; 10+ messages in thread From: Steve Rae @ 2016-02-08 18:04 UTC (permalink / raw) To: u-boot 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 at 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 %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 > > -- > Maxime Ripard, Free Electrons > Embedded Linux, Kernel and Android engineering > http://free-electrons.com > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH 2/2] fastboot: sparse: remove unnecessary logging 2016-02-08 18:04 ` Steve Rae @ 2016-02-09 17:17 ` Maxime Ripard 2016-02-09 17:58 ` Steve Rae 0 siblings, 1 reply; 10+ messages in thread From: Maxime Ripard @ 2016-02-09 17:17 UTC (permalink / raw) To: u-boot 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 at 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 at 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 -- 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> ^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH 2/2] fastboot: sparse: remove unnecessary logging 2016-02-09 17:17 ` Maxime Ripard @ 2016-02-09 17:58 ` Steve Rae 2016-02-09 20:18 ` Maxime Ripard 0 siblings, 1 reply; 10+ messages in thread From: Steve Rae @ 2016-02-09 17:58 UTC (permalink / raw) To: u-boot 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 at 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 at 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 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH 2/2] fastboot: sparse: remove unnecessary logging 2016-02-09 17:58 ` Steve Rae @ 2016-02-09 20:18 ` Maxime Ripard 2016-02-09 21:44 ` Steve Rae 0 siblings, 1 reply; 10+ messages in thread From: Maxime Ripard @ 2016-02-09 20:18 UTC (permalink / raw) To: u-boot 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. -- 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> ^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH 2/2] fastboot: sparse: remove unnecessary logging 2016-02-09 20:18 ` Maxime Ripard @ 2016-02-09 21:44 ` Steve Rae 0 siblings, 0 replies; 10+ messages in thread From: Steve Rae @ 2016-02-09 21:44 UTC (permalink / raw) To: u-boot 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 > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-02-09 21:44 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-02-03 20:46 [U-Boot] [PATCH 1/2] fastboot: sparse: fix block addressing for don't care chunk type Steve Rae 2016-02-03 20:46 ` [U-Boot] [PATCH 2/2] fastboot: sparse: remove unnecessary logging Steve Rae 2016-02-04 12:20 ` Maxime Ripard 2016-02-04 18:51 ` Steve Rae 2016-02-08 8:19 ` Maxime Ripard 2016-02-08 18:04 ` Steve Rae 2016-02-09 17:17 ` Maxime Ripard 2016-02-09 17:58 ` Steve Rae 2016-02-09 20:18 ` Maxime Ripard 2016-02-09 21:44 ` Steve Rae
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.