Message ID | 1454532362-31109-2-git-send-email-srae@broadcom.com |
---|---|
State | Superseded |
Delegated to: | Przemyslaw Marczak |
Headers | show |
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
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 >
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@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
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@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 >
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@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@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
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@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@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 >
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.
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 >
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,
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(-)