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