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

[Maxime Ripard]

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

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>