[PATCH] fix chip size limiting in atapromise

[Carl-Daniel Hailfinger]

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

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)

[Joseph C. Lehner]

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

[Urja Rannikko]

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