[U-Boot] [PATCH] Followup fixes on the mtdparts spread patchset
Consolidate some code in mtd_get_len_incl_bad(), and fix a condition where a valid partition could be reported as truncated if it has a good block at the end of the device (unlikely, since the BBT is usually there).
Fix mid-block declarations in net_part_size().
Signed-off-by: Scott Wood scottwood@freescale.com --- common/cmd_mtdparts.c | 5 +++-- drivers/mtd/mtdcore.c | 15 +++++---------- 2 files changed, 8 insertions(+), 12 deletions(-)
diff --git a/common/cmd_mtdparts.c b/common/cmd_mtdparts.c index 17865b7..5481c88 100644 --- a/common/cmd_mtdparts.c +++ b/common/cmd_mtdparts.c @@ -1228,15 +1228,16 @@ static int generate_mtdparts_save(char *buf, u32 buflen) */ static uint64_t net_part_size(struct mtd_info *mtd, struct part_info *part) { + uint64_t i, net_size = 0; + if (!mtd->block_isbad) return part->size;
- uint64_t i, net_size = 0; - for (i = 0; i < part->size; i += mtd->erasesize) { if (!mtd->block_isbad(mtd, part->offset + i)) net_size += mtd->erasesize; } + return net_size; } #endif diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c index 78f2a08..a195dda 100644 --- a/drivers/mtd/mtdcore.c +++ b/drivers/mtd/mtdcore.c @@ -162,11 +162,6 @@ void mtd_get_len_incl_bad(struct mtd_info *mtd, uint64_t offset, *truncated = 0; *len_incl_bad = 0;
- if (offset >= mtd->size) { - *truncated = 1; - return; - } - if (!mtd->block_isbad) { *len_incl_bad = length; return; @@ -176,6 +171,11 @@ void mtd_get_len_incl_bad(struct mtd_info *mtd, uint64_t offset, uint64_t block_len;
while (len_excl_bad < length) { + if (offset >= mtd->size) { + *truncated = 1; + return; + } + block_len = mtd->erasesize - (offset & (mtd->erasesize - 1));
if (!mtd->block_isbad(mtd, offset & ~(mtd->erasesize - 1))) @@ -183,11 +183,6 @@ void mtd_get_len_incl_bad(struct mtd_info *mtd, uint64_t offset,
*len_incl_bad += block_len; offset += block_len; - - if (offset >= mtd->size) { - *truncated = 1; - break; - } } } #endif /* defined(CONFIG_CMD_MTDPARTS_SPREAD) */
Hi Scott,
Thank you for picking up the 'mtdparts spread' patch series into u-boot-nand-flash next by applying the v6 series and doing this fixup on-top. It is greatly appreciated.
On Thu, Sep 9, 2010 at 4:50 PM, Scott Wood scottwood@freescale.com wrote:
Consolidate some code in mtd_get_len_incl_bad(), and fix a condition where a valid partition could be reported as truncated if it has a good block at the end of the device (unlikely, since the BBT is usually there).
Fix mid-block declarations in net_part_size().
Signed-off-by: Scott Wood scottwood@freescale.com
common/cmd_mtdparts.c | 5 +++-- drivers/mtd/mtdcore.c | 15 +++++---------- 2 files changed, 8 insertions(+), 12 deletions(-)
diff --git a/common/cmd_mtdparts.c b/common/cmd_mtdparts.c index 17865b7..5481c88 100644 --- a/common/cmd_mtdparts.c +++ b/common/cmd_mtdparts.c @@ -1228,15 +1228,16 @@ static int generate_mtdparts_save(char *buf, u32 buflen) */ static uint64_t net_part_size(struct mtd_info *mtd, struct part_info *part) {
- uint64_t i, net_size = 0;
if (!mtd->block_isbad) return part->size;
- uint64_t i, net_size = 0;
for (i = 0; i < part->size; i += mtd->erasesize) { if (!mtd->block_isbad(mtd, part->offset + i)) net_size += mtd->erasesize; }
return net_size; } #endif
It's fine by me but I'm curious: why move the declaration of the stack variables i and net_size to before the check of the block_isbad function pointer? Before this change, I think there would be no allocation of these variables on the stack if the block_isbad function pointer is null.
I double-checked and there were no warnings from 'make' 'MAKEALL' (I'm using (Sourcery G++ Lite 2009q1-203) 4.3.3 here) or checkpatch.pl (from 2.6.36-rc3) before this change. Just curious -- I am interested in learning so I can produce and submit patchsets that meet the standards of this list in the future.
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c index 78f2a08..a195dda 100644 --- a/drivers/mtd/mtdcore.c +++ b/drivers/mtd/mtdcore.c @@ -162,11 +162,6 @@ void mtd_get_len_incl_bad(struct mtd_info *mtd, uint64_t offset, *truncated = 0; *len_incl_bad = 0;
- if (offset >= mtd->size) {
- *truncated = 1;
- return;
- }
if (!mtd->block_isbad) { *len_incl_bad = length; return; @@ -176,6 +171,11 @@ void mtd_get_len_incl_bad(struct mtd_info *mtd, uint64_t offset, uint64_t block_len;
while (len_excl_bad < length) {
- if (offset >= mtd->size) {
- *truncated = 1;
- return;
- }
block_len = mtd->erasesize - (offset & (mtd->erasesize - 1));
if (!mtd->block_isbad(mtd, offset & ~(mtd->erasesize - 1))) @@ -183,11 +183,6 @@ void mtd_get_len_incl_bad(struct mtd_info *mtd, uint64_t offset,
*len_incl_bad += block_len; offset += block_len;
- if (offset >= mtd->size) {
- *truncated = 1;
- break;
- }
} } #endif /* defined(CONFIG_CMD_MTDPARTS_SPREAD) */
Awesome! Thank you for fixing that edge case.
Reviewed-by: Ben Gardiner bengardiner@nanometrics.ca
Best Regards, Ben Gardiner
--- Nanometrics Inc. http://www.nanometrics.ca
On Fri, 10 Sep 2010 10:35:04 -0400 Ben Gardiner bengardiner@nanometrics.ca wrote:
It's fine by me but I'm curious: why move the declaration of the stack variables i and net_size to before the check of the block_isbad function pointer?
U-Boot coding style prohibits mid-block declarations.
Before this change, I think there would be no allocation of these variables on the stack if the block_isbad function pointer is null.
The change should not make any difference in the generated code. The compiler is not going to delay its creation of the stack frame just because of where the variables are declared.
-Scott
On Fri, Sep 10, 2010 at 12:32 PM, Scott Wood scottwood@freescale.com wrote:
On Fri, 10 Sep 2010 10:35:04 -0400 Ben Gardiner bengardiner@nanometrics.ca wrote:
It's fine by me but I'm curious: why move the declaration of the stack variables i and net_size to before the check of the block_isbad function pointer?
U-Boot coding style prohibits mid-block declarations.
Before this change, I think there would be no allocation of these variables on the stack if the block_isbad function pointer is null.
The change should not make any difference in the generated code. The compiler is not going to delay its creation of the stack frame just because of where the variables are declared.
Ok. I understand now; thank you for clearing that up, Scott.
Best Regards, Ben Gardiner
--- Nanometrics Inc. http://www.nanometrics.ca
Dear Ben Gardiner,
In message AANLkTi=8oQgMy2+_tbLHgrYRoDP8MUBneB8jxjsFTtN6@mail.gmail.com you wrote:
It's fine by me but I'm curious: why move the declaration of the stack variables i and net_size to before the check of the block_isbad function pointer? Before this change, I think there would be no allocation of these variables on the stack if the block_isbad function pointer is null.
The optimizer is more clever than you assume.
Best regards,
Wolfgang Denk
Hi Wolfgang,
On Fri, Sep 10, 2010 at 1:32 PM, Wolfgang Denk wd@denx.de wrote:
Dear Ben Gardiner,
In message AANLkTi=8oQgMy2+_tbLHgrYRoDP8MUBneB8jxjsFTtN6@mail.gmail.com you wrote:
It's fine by me but I'm curious: why move the declaration of the stack variables i and net_size to before the check of the block_isbad function pointer? Before this change, I think there would be no allocation of these variables on the stack if the block_isbad function pointer is null.
The optimizer is more clever than you assume.
I see -- I'll start giving it the benefit of the doubt then :)
Best Regards, Ben Gardiner
--- Nanometrics Inc. http://www.nanometrics.ca
participants (3)
-
Ben Gardiner -
Scott Wood -
Wolfgang Denk