[U-Boot] [PATCH 2/2] fastboot: sparse: remove unnecessary logging

[Steve Rae]

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