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

[Carl-Daniel Hailfinger]

On 17.11.2008 14:28, Stefan Reinauer wrote:
> 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?

I tried factoring it out, but since we don't want to die() completely,
we have to return with an error code from this function. That pretty
much rules out factoring out the checks into a separate function. I
could move all checks one level down in the call graph and clutter up
only the end of each function.
What do you think?
> > +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)?

That's a bit difficult. There are some chips which have either the 0x60
or the 0xc7 opcode used for something else, so we need to keep this chip
specific.

Your point about the check for successful erase is definitely valid and
I'll post a followup patch to check this.
> Otherwise: Acked-by: Stefan Reinauer

Regards,
Carl-Daniel