Re: [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  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;