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.2...@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.2...@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.tau...@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.2...@gmx.net>
Acked-by: Stefan Tauner <stefan.tau...@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;
_______________________________________________
flashrom mailing list
flashrom@flashrom.org
https://www.flashrom.org/mailman/listinfo/flashrom
