cfi_flash.c bug fix

this fix a possible bug which occur when the config file defines a flash which is actually smaller than the physical one. on top of that, if you want to force your flash to appear smaller you can define CONFIG_FORCE_FLASH_BANK_SIZE the a corresponding value.

Signed-off-by Eran Liberty <[email protected]>

Index: drivers/cfi_flash.c
===================================================================

Quoted 10 lines · eran.liberty at gmail.com

this fix a possible bug which occur when the config file defines a flash which is actually smaller than the physical one. on top of that, if you want to force your flash to appear smaller you can define CONFIG_FORCE_FLASH_BANK_SIZE the a corresponding value.

Signed-off-by Eran Liberty <[email protected]>

Index: drivers/cfi_flash.c
===================================================================

cfi_flash.c (.../branches/exsw6000) (revision 69) line 1289

Quoted 10 lines

cfi_flash.c (.../branches/exsw6000) (revision 69) renamed from cfi_flash.c (.../tags/trunk/20070620_2_merge_to_exsw6000) (revision 69) +3 -1

@@ -1281,7 +1281,9 @@
                        erase_region_count = (tmp & 0xffff) + 1;
                        debug ("erase_region_count = %d erase_region_size = %d\n",
                                erase_region_count, erase_region_size);
-                       for (j = 0; j < erase_region_count; j++) {
+                       for (j = 0;
+                            j < erase_region_count
+                            && sect_cnt < CFG_MAX_FLASH_SECT; j++) {
                                info->start[sect_cnt] = sector;
                                sector += (erase_region_size * size_ratio);
 

Nit: rather than adding a test in the for loop, can you modify erase_region_count before the loop to reflect the maximum size? I think it will result in cleaner code.

ie:
if (erase_region_count > CFG_MAX_FLASH_SECT - sect_cnt)

        erase_region_count = CFG_MAX_FLASH_SECT - sect_cnt;

(but double check that I haven't created an off-by-one error)

Cheers,
g.

--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely at secretlab.ca
(403) 399-0195

Quoted 17 lines · Grant Likely

cfi_flash.c (.../branches/exsw6000) (revision 69) line 1289

Quoted 13 lines · eran.liberty at gmail.com

cfi_flash.c (.../branches/exsw6000) (revision 69) renamed from cfi_flash.c (.../tags/trunk/20070620_2_merge_to_exsw6000) (revision 69) +3 -1

@@ -1281,7 +1281,9 @@
                        erase_region_count = (tmp & 0xffff) + 1;
                        debug ("erase_region_count = %d erase_region_size = %d\n",
                                erase_region_count, erase_region_size);
-                       for (j = 0; j < erase_region_count; j++) {
+                       for (j = 0;
+                            j < erase_region_count
+                            && sect_cnt < CFG_MAX_FLASH_SECT; j++) {
                                info->start[sect_cnt] = sector;
                                sector += (erase_region_size * size_ratio);
 

Nit: rather than adding a test in the for loop, can you modify erase_region_count before the loop to reflect the maximum size? I think it will result in cleaner code.

If I had not missed anything,
your suggestion tie together two variables which are not necessarily connected. True, your suggestion will prevent the j variable from letting the sect_cnt overflow in the , should be last, iteration. But i feel it is circumventive and less straight forward. The variable causing the spill over is sect_cnt, there for it should be the one that being tested against its own boundaries.

Quoted 19 lines · eran liberty

Quoted 17 lines · Grant Likely

cfi_flash.c (.../branches/exsw6000) (revision 69) line 1289

Quoted 13 lines · eran.liberty at gmail.com

cfi_flash.c (.../branches/exsw6000) (revision 69) renamed from cfi_flash.c (.../tags/trunk/20070620_2_merge_to_exsw6000) (revision 69) +3 -1

@@ -1281,7 +1281,9 @@
                        erase_region_count = (tmp & 0xffff) + 1;
                        debug ("erase_region_count = %d erase_region_size = %d\n",
                                erase_region_count, erase_region_size);
-                       for (j = 0; j < erase_region_count; j++) {
+                       for (j = 0;
+                            j < erase_region_count
+                            && sect_cnt < CFG_MAX_FLASH_SECT; j++) {
                                info->start[sect_cnt] = sector;
                                sector += (erase_region_size * size_ratio);
 

Nit: rather than adding a test in the for loop, can you modify erase_region_count before the loop to reflect the maximum size? I think it will result in cleaner code.

Quoted 7 lines

If I had not missed anything,
your suggestion tie together two variables which are not necessarily connected. True, your suggestion will prevent the j variable from letting the sect_cnt overflow in the , should be last, iteration. But i feel it is circumventive and less straight forward. The variable causing the spill over is sect_cnt, there for it should be the one that being tested against its own boundaries.

My 'nitpick' is about the for loop becoming longer than 80chars, and hence being wrapped to 3 lines which makes it harder to read. I'd prefer code that keeps the for(;;) statement short for readability.

g.

--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely at secretlab.ca
(403) 399-0195

Quoted 4 lines · Grant Likely

My 'nitpick' is about the for loop becoming longer than 80chars, and hence being wrapped to 3 lines which makes it harder to read. I'd prefer code that keeps the for(;;) statement short for readability.

Will taking the check inside the loop and breaking out, make us happy?

for (j = 0; j < erase_region_count; j++) {
   if (CFG_MAX_FLASH_SECT <= sect_cnt)
       break;
   ...
   ...
}