Re: [coreboot] [PATCH] flashrom: Check for failed SPI command execution

[Stefan Reinauer]

Carl-Daniel Hailfinger wrote:
> Check for failed SPI command execution in flashrom. Although SPI itself
> does not have a mechanism to signal command failure, the SPI host may be
> unable to send a given command over the wire due to security or hardware
> limitations. The current code ignores these mechanisms completely and
> simply assumes almost every command succeeds. Complain if SPI command
> execution fails.
>
> Since locked down Intel chipsets (like the one we had problems with
> earlier) only allow a small subset of commands, find the common subset
> of commands between the chipset and the ROM in the chip erase case. That
> is accomplished by the new spi_chip_erase_60_c7() which can be used for
> chips supporting both 0x60 and 0xc7 chip erase commands.
>
> Both parts of the patch address problems seen in the real world. The
> increased verbosity for the error case will help us diagnose and address
> problems better.
>
> Signed-off-by: Carl-Daniel Hailfinger
> while (spi_read_status_register() & JEDEC_RDSR_BIT_WIP)
>    	sleep(1);
>    return 0;
> @@ -290,19 +304,44 @@
>  int spi_chip_erase_c7(struct flashchip *flash)
>  {
>    const unsigned char cmd[JEDEC_CE_C7_OUTSIZE] = { JEDEC_CE_C7 };
> int result;
> spi_disable_blockprotect();
> spi_write_enable();
> result = spi_disable_blockprotect();
> if (result) {
> printf_debug("spi_disable_blockprotect failed\n");
> return result;
> }
> result = spi_write_enable();
> if (result) {
> printf_debug("spi_write_enable failed\n");
> return result;
> }

This code is duplicated in a couple of places. Is it worth factoring it out?
> +int spi_chip_erase_60_c7(struct flashchip *flash)
> +{
> int result;
> result = spi_chip_erase_60(flash);
> if (result) {
> printf_debug("spi_chip_erase_60 failed, trying c7\n");
> result = spi_chip_erase_c7(flash);
> }
> return result;
>
> +}

I don't particularly like this. Maybe we should have spi_chip_erase()
try all different erase functions in a row, and check whether the erase
was actually successfull (all bytes 0xff)?

Otherwise: Acked-by: Stefan Reinauer