| Submitter | Carl-Daniel Hailfinger |
|---|---|
| Date | 2016-01-17 00:57:25 |
| Message ID | <569AE6F5.8050904@gmx.net> |
| Download | mbox | patch |
| Permalink | /patch/4361/ |
| State | Accepted |
| Headers | show |
Comments
Just tested it, programmer is still working on my Ultra100! Regards, Joseph On 2016-01-17 01:57, Carl-Daniel Hailfinger wrote: > Atapromise chip size limiting has two bugs I didn't catch during review, > but only in the moment of commit. > > The current code is checking model_id to remember if a chip has already > been limited, but if flashchips.c contains two subsequent chips with > different vendor_id but identical model_id the adjustment will not be > done. Switch to checking the chip size instead. > > If a chip has multiple whole-chip erase functions, only one will be > modified. Fix that. > > Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> > > Index: flashrom-atapromise_fix_limit_chip/atapromise.c > =================================================================== > --- flashrom-atapromise_fix_limit_chip/atapromise.c (Revision 1916) > +++ flashrom-atapromise_fix_limit_chip/atapromise.c (Arbeitskopie) > @@ -79,38 +79,35 @@ > > static void atapromise_limit_chip(struct flashchip *chip) > { > - static uint32_t last_model_id = 0; > unsigned int i, size; > + unsigned int usable_erasers = 0; > > - if (chip->model_id == last_model_id) > - return; > - > size = chip->total_size * 1024; > - if (size > rom_size) { > - /* Undefine all block_erasers that don't operate on the whole chip, > - * and adjust the eraseblock size of the one that does. > - */ > - for (i = 0; i < NUM_ERASEFUNCTIONS; ++i) { > - if (chip->block_erasers[i].eraseblocks[0].size != size) { > - chip->block_erasers[i].eraseblocks[0].count = 0; > - chip->block_erasers[i].block_erase = NULL; > - } else { > - chip->block_erasers[i].eraseblocks[0].size = rom_size; > - break; > - } > - } > > - if (i != NUM_ERASEFUNCTIONS) { > - chip->total_size = rom_size / 1024; > - if (chip->page_size > rom_size) > - chip->page_size = rom_size; > + /* Chip is small enough or already limited. */ > + if (size <= rom_size) > + return; > + > + /* Undefine all block_erasers that don't operate on the whole chip, > + * and adjust the eraseblock size of those which do. > + */ > + for (i = 0; i < NUM_ERASEFUNCTIONS; ++i) { > + if (chip->block_erasers[i].eraseblocks[0].size != size) { > + chip->block_erasers[i].eraseblocks[0].count = 0; > + chip->block_erasers[i].block_erase = NULL; > } else { > - msg_pdbg("Failed to adjust size of chip \"%s\" (%d kB).\n", chip->name, > - chip->total_size); > + chip->block_erasers[i].eraseblocks[0].size = rom_size; > + usable_erasers++; > } > } > > - last_model_id = chip->model_id; > + if (usable_erasers) { > + chip->total_size = rom_size / 1024; > + if (chip->page_size > rom_size) > + chip->page_size = rom_size; > + } else { > + msg_pdbg("Failed to adjust size of chip \"%s\" (%d kB).\n", chip->name, chip->total_size); > + } > } > > int atapromise_init(void) >
Hi, Seems fine (i built it etc) except for one tiny nitpick noticed by git (or i think a default commit hook, but whatever) below. On Sun, Jan 17, 2016 at 2:57 AM, Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> wrote: > Atapromise chip size limiting has two bugs I didn't catch during review, > but only in the moment of commit. > > The current code is checking model_id to remember if a chip has already > been limited, but if flashchips.c contains two subsequent chips with > different vendor_id but identical model_id the adjustment will not be > done. Switch to checking the chip size instead. > > If a chip has multiple whole-chip erase functions, only one will be > modified. Fix that. > > Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> > > Index: flashrom-atapromise_fix_limit_chip/atapromise.c > =================================================================== > --- flashrom-atapromise_fix_limit_chip/atapromise.c (Revision 1916) > +++ flashrom-atapromise_fix_limit_chip/atapromise.c (Arbeitskopie) > @@ -79,38 +79,35 @@ > > static void atapromise_limit_chip(struct flashchip *chip) > { > - static uint32_t last_model_id = 0; > unsigned int i, size; > + unsigned int usable_erasers = 0; > > - if (chip->model_id == last_model_id) > - return; > - > size = chip->total_size * 1024; > - if (size > rom_size) { > - /* Undefine all block_erasers that don't operate on the whole chip, > - * and adjust the eraseblock size of the one that does. > - */ > - for (i = 0; i < NUM_ERASEFUNCTIONS; ++i) { > - if (chip->block_erasers[i].eraseblocks[0].size != size) { > - chip->block_erasers[i].eraseblocks[0].count = 0; > - chip->block_erasers[i].block_erase = NULL; > - } else { > - chip->block_erasers[i].eraseblocks[0].size = rom_size; > - break; > - } > - } > > - if (i != NUM_ERASEFUNCTIONS) { > - chip->total_size = rom_size / 1024; > - if (chip->page_size > rom_size) > - chip->page_size = rom_size; > + /* Chip is small enough or already limited. */ > + if (size <= rom_size) > + return; ^ That line has a mix of spaces (first) and a tab... rest of the function seems to be tabs, so fix with spaces=>tab seems appropriate. So with that fixed, this is: Acked-by: Urja Rannikko <urjaman@gmail.com>
On 17.01.2016 12:51, Joseph C. Lehner wrote: > Just tested it, programmer is still working on my Ultra100! > > Regards, > Joseph On 18.02.2016 22:21, Urja Rannikko wrote: > Seems fine (i built it etc) except for one tiny nitpick noticed by git > (or i think a default commit hook, but whatever) below. > > On Sun, Jan 17, 2016 at 2:57 AM, Carl-Daniel Hailfinger > <c-d.hailfinger.devel.2006@gmx.net> wrote: >> Atapromise chip size limiting has two bugs I didn't catch during review, >> but only in the moment of commit. >> >> The current code is checking model_id to remember if a chip has already >> been limited, but if flashchips.c contains two subsequent chips with >> different vendor_id but identical model_id the adjustment will not be >> done. Switch to checking the chip size instead. >> >> If a chip has multiple whole-chip erase functions, only one will be >> modified. Fix that. >> >> Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> >> >> Index: flashrom-atapromise_fix_limit_chip/atapromise.c >> =================================================================== >> --- flashrom-atapromise_fix_limit_chip/atapromise.c (Revision 1916) >> +++ flashrom-atapromise_fix_limit_chip/atapromise.c (Arbeitskopie) >> @@ -79,38 +79,35 @@ >> [...] >> - if (i != NUM_ERASEFUNCTIONS) { >> - chip->total_size = rom_size / 1024; >> - if (chip->page_size > rom_size) >> - chip->page_size = rom_size; >> + /* Chip is small enough or already limited. */ >> + if (size <= rom_size) >> + return; > ^ That line has a mix of spaces (first) and a tab... rest of the > function seems to be tabs, so fix with spaces=>tab seems appropriate. > > > So with that fixed, this is: > Acked-by: Urja Rannikko <urjaman@gmail.com> Thanks for the tests and the review! Fixed the whitespace and committed in r1930. Regards, Carl-Daniel
Patch
different vendor_id but identical model_id the adjustment will not be done. Switch to checking the chip size instead. If a chip has multiple whole-chip erase functions, only one will be modified. Fix that. Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> Index: flashrom-atapromise_fix_limit_chip/atapromise.c =================================================================== --- flashrom-atapromise_fix_limit_chip/atapromise.c (Revision 1916) +++ flashrom-atapromise_fix_limit_chip/atapromise.c (Arbeitskopie) @@ -79,38 +79,35 @@ static void atapromise_limit_chip(struct flashchip *chip) { - static uint32_t last_model_id = 0; unsigned int i, size; + unsigned int usable_erasers = 0; - if (chip->model_id == last_model_id) - return; - size = chip->total_size * 1024; - if (size > rom_size) { - /* Undefine all block_erasers that don't operate on the whole chip, - * and adjust the eraseblock size of the one that does. - */ - for (i = 0; i < NUM_ERASEFUNCTIONS; ++i) { - if (chip->block_erasers[i].eraseblocks[0].size != size) { - chip->block_erasers[i].eraseblocks[0].count = 0; - chip->block_erasers[i].block_erase = NULL; - } else { - chip->block_erasers[i].eraseblocks[0].size = rom_size; - break; - } - } - if (i != NUM_ERASEFUNCTIONS) { - chip->total_size = rom_size / 1024; - if (chip->page_size > rom_size) - chip->page_size = rom_size; + /* Chip is small enough or already limited. */ + if (size <= rom_size) + return; + + /* Undefine all block_erasers that don't operate on the whole chip, + * and adjust the eraseblock size of those which do. + */ + for (i = 0; i < NUM_ERASEFUNCTIONS; ++i) { + if (chip->block_erasers[i].eraseblocks[0].size != size) { + chip->block_erasers[i].eraseblocks[0].count = 0; + chip->block_erasers[i].block_erase = NULL; } else { - msg_pdbg("Failed to adjust size of chip \"%s\" (%d kB).\n", chip->name, - chip->total_size); + chip->block_erasers[i].eraseblocks[0].size = rom_size; + usable_erasers++; } } - last_model_id = chip->model_id; + if (usable_erasers) { + chip->total_size = rom_size / 1024; + if (chip->page_size > rom_size) + chip->page_size = rom_size; + } else { + msg_pdbg("Failed to adjust size of chip \"%s\" (%d kB).\n", chip->name, chip->total_size); + } } int atapromise_init(void)