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

[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!