[PATCH] Rewrite and fix corner case in sb600spi

[Carl-Daniel Hailfinger]

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

Note: I prefer ! instead of == 0, but Stefan Tauner prefers == 0. A
matter of taste.

Signed-off-by: Carl-Daniel Hailfinger

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,22 @@
 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.
+		 * Only Yangtze supports the second half of indices; no 66 MHz before SB8xx. */
+		if ((strcasecmp(spispeed, "reserved") == 0) ||
+		    (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;

[Carl-Daniel Hailfinger]

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

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;

[Stefan Tauner]

On Thu, 10 Mar 2016 13:34:27 +0100
Carl-Daniel Hailfinger  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
> 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

Thanks!

[Carl-Daniel Hailfinger]

On 10.03.2016 22:07, Stefan Tauner wrote:
> On Thu, 10 Mar 2016 13:34:27 +0100
> Carl-Daniel Hailfinger  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
> > 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
>
> 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 
Acked-by: Stefan Tauner

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;

[Carl-Daniel Hailfinger]

On 12.03.2016 20:46, Carl-Daniel Hailfinger wrote:
> 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 
> Acked-by: Stefan Tauner

Committed in r1949, but I forgot to include the subject as first line of
the commit message.

Regards,
Carl-Daniel