Patchwork Rewrite and fix corner case in sb600spi

login
register
about
Submitter Carl-Daniel Hailfinger
Date 2016-03-10 12:34:27
Message ID <56E169D3.6060707@gmx.net>
Download mbox | patch
Permalink /patch/4418/
State Superseded
Headers show

Comments

Carl-Daniel Hailfinger - 2016-03-10 12:34:27
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@gmx.net>
Stefan Tauner - 2016-03-10 21:07:42
On Thu, 10 Mar 2016 13:34:27 +0100
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@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@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.

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

> +		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@alumni.tuwien.ac.at>

Thanks!

Patch

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