Patchwork fix chip size limiting in atapromise

login
register
about
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

Carl-Daniel Hailfinger - 2016-01-17 00:57:25
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
Joseph Lehner - 2016-01-17 11:51:53
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)
>
Urja Rannikko - 2016-02-18 21:21:56
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>
Carl-Daniel Hailfinger - 2016-02-18 23:13:43
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)