[flashrom] [PATCH] Rewrite and fix corner case in sb600spi

[Carl-Daniel Hailfinger]

On 10.03.2016 22:07, Stefan Tauner wrote:
> On Thu, 10 Mar 2016 13:34:27 +0100
> Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:
>
>> New version, fix corner case found by Stefan Tauner during review.
>>
>> Specifying spispeed=reserved as programmer parameter resulted in
>> selecting the default SPI speed instead of aborting. Rewrite the logic
>> to be more readable.
>>
>> Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>
>>
>> Index: flashrom-sb600_spi_speedselection_cleanup/sb600spi.c
>> ===================================================================
>> --- flashrom-sb600_spi_speedselection_cleanup/sb600spi.c	(Revision 1948)
>> +++ flashrom-sb600_spi_speedselection_cleanup/sb600spi.c	(Arbeitskopie)
>> @@ -387,24 +387,24 @@
>>  static int handle_speed(struct pci_dev *dev)
>>  {
>>  	uint32_t tmp;
>> -	int8_t spispeed_idx = 3; /* Default to 16.5 MHz */
>> +	uint8_t spispeed_idx = 3; /* Default to 16.5 MHz */
>>  
>>  	char *spispeed = extract_programmer_param("spispeed");
>>  	if (spispeed != NULL) {
>> -		if (strcasecmp(spispeed, "reserved") != 0) {
>> -			int i;
>> -			for (i = 0; i < ARRAY_SIZE(spispeeds); i++) {
>> -				if (strcasecmp(spispeeds[i].name, spispeed) == 0) {
>> -					spispeed_idx = i;
>> -					break;
>> -				}
>> +		int i;
> This could (and always should have been AFAICS) unsigned.

Indeed. Thanks.

 
>> +		for (i = 0; i < ARRAY_SIZE(spispeeds); i++) {
>> +			if (strcasecmp(spispeeds[i].name, spispeed) == 0) {
>> +				spispeed_idx = i;
>> +				break;
>>  			}
>> -			/* Only Yangtze supports the second half of indices; no 66 MHz before SB8xx. */
>> -			if ((amd_gen < CHIPSET_YANGTZE && spispeed_idx > 3) ||
>> -			    (amd_gen < CHIPSET_SB89XX && spispeed_idx == 0))
>> -				spispeed_idx = -1;
>>  		}
>> -		if (spispeed_idx < 0) {
>> +		/* "reserved" is not a valid speed.
>> +		 * Error out on speeds not present in the spispeeds array.
>> +		 * Only Yangtze supports the second half of indices; no 66 MHz before SB8xx. */
> Breaking that last sentence into two lines would make them perfectly
> align with the code. On the other hand we could probably move each
> comment to the end of the respective line. Only the Yangtze comment
> needs to be shortened.

Good observation. I split the last line.


>> +		if ((strcasecmp(spispeed, "reserved") == 0) ||
>> +		    (i == ARRAY_SIZE(spispeeds)) ||
>> +		    (amd_gen < CHIPSET_YANGTZE && spispeed_idx > 3) ||
>> +		    (amd_gen < CHIPSET_SB89XX && spispeed_idx == 0)) {
>>  			msg_perr("Error: Invalid spispeed value: '%s'.\n", spispeed);
>>  			free(spispeed);
>>  			return 1;
>>
> Works fine with the code about as well as unsigned i. Tested on my
> IMB-A180-H with Yangtze.
>
> With or w/o the changes above, this is
> Acked-by: Stefan Tauner <stefan.tauner at alumni.tuwien.ac.at>
>
> Thanks!

Thanks for the review. This is the version I'm going to commit:

Specifying spispeed=reserved as programmer parameter resulted in
selecting the default SPI speed instead of aborting. Rewrite the logic
to be more readable.

Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>
Acked-by: Stefan Tauner <stefan.tauner at alumni.tuwien.ac.at>

Index: flashrom-sb600_spi_speedselection_cleanup/sb600spi.c
===================================================================
--- flashrom-sb600_spi_speedselection_cleanup/sb600spi.c	(Revision 1948)
+++ flashrom-sb600_spi_speedselection_cleanup/sb600spi.c	(Arbeitskopie)
@@ -387,24 +387,25 @@
 static int handle_speed(struct pci_dev *dev)
 {
 	uint32_t tmp;
-	int8_t spispeed_idx = 3; /* Default to 16.5 MHz */
+	uint8_t spispeed_idx = 3; /* Default to 16.5 MHz */
 
 	char *spispeed = extract_programmer_param("spispeed");
 	if (spispeed != NULL) {
-		if (strcasecmp(spispeed, "reserved") != 0) {
-			int i;
-			for (i = 0; i < ARRAY_SIZE(spispeeds); i++) {
-				if (strcasecmp(spispeeds[i].name, spispeed) == 0) {
-					spispeed_idx = i;
-					break;
-				}
+		unsigned int i;
+		for (i = 0; i < ARRAY_SIZE(spispeeds); i++) {
+			if (strcasecmp(spispeeds[i].name, spispeed) == 0) {
+				spispeed_idx = i;
+				break;
 			}
-			/* Only Yangtze supports the second half of indices; no 66 MHz before SB8xx. */
-			if ((amd_gen < CHIPSET_YANGTZE && spispeed_idx > 3) ||
-			    (amd_gen < CHIPSET_SB89XX && spispeed_idx == 0))
-				spispeed_idx = -1;
 		}
-		if (spispeed_idx < 0) {
+		/* "reserved" is not a valid speed.
+		 * Error out on speeds not present in the spispeeds array.
+		 * Only Yangtze supports the second half of indices.
+		 * No 66 MHz before SB8xx. */
+		if ((strcasecmp(spispeed, "reserved") == 0) ||
+		    (i == ARRAY_SIZE(spispeeds)) ||
+		    (amd_gen < CHIPSET_YANGTZE && spispeed_idx > 3) ||
+		    (amd_gen < CHIPSET_SB89XX && spispeed_idx == 0)) {
 			msg_perr("Error: Invalid spispeed value: '%s'.\n", spispeed);
 			free(spispeed);
 			return 1;