Re: [flashrom] [PATCH] Error out when chipset/board doesn't decode full ROM

[Carl-Daniel Hailfinger]

On 28.10.2009 03:59, Uwe Hermann wrote:
> Most of it looks good, but there are some changeѕ required.
> The checks have to be done later in the code, after the chip has been
> detected, so that we only check the sizes for the chip we actually
> find, not for all the chips we probe for. I moved the checks and
> simplified them a bit (I hope).

This breaks on Ron's setup where the chip is too big to be detected.
That's why I wanted to get the warning during probe time.
> Index: flashrom.c
>
> --- flashrom.c	(revision 753)
> +++ flashrom.c	(working copy)
> @@ -449,6 +452,25 @@
>           flash->vendor, flash->name, flash->total_size,
>           flashbuses_to_text(flash->bustype), base);
> if ((buses_common & CHIP_BUSTYPE_PARALLEL) &&
>     (max_rom_decode.parallel < flash->total_size * 1024))
> tmpsize = max_rom_decode.parallel;
> if ((buses_common & CHIP_BUSTYPE_LPC) &&
>    (max_rom_decode.lpc < flash->total_size * 1024))
> tmpsize = max_rom_decode.lpc;
> if ((buses_common & CHIP_BUSTYPE_FWH) &&
>    (max_rom_decode.fwh < flash->total_size * 1024))
> tmpsize = max_rom_decode.fwh;
> if ((buses_common & CHIP_BUSTYPE_SPI) &&
>    (max_rom_decode.spi < flash->total_size * 1024))
> tmpsize = max_rom_decode.spi;
> if (tmpsize != 0xffffffff) {
> printf("Chipset/board/programmer only supports "
>        "a max. size of %u KB. ABORTING.\n", tmpsize / 1024);
> programmer_shutdown();
> exit(1);
> }

I had this code in an earlier version, but decided to change it because
the chipset and the chip may have more than one bus in common (e.g.
SB600 and Pm49* can both speak LPC+FWH) and on SB600/SB7x0/SB8x0 there
are different limits for LPC and FWH. The only way to tell the user
about the exact circumstances is to spew error messages per bus. My
patch was missing the bus-specific error messages, so I can't blame you
for refactoring.

Would it be OK for you if I moved the size checking to a separate
function size_supported(buses_common, size, loglevel) which can be
called during probe (with printf_debug level) and before any
read/write/erase action (with printf_error level)? That way, we avoid
cluttering the main loop, get free checking during probe, and can abort
before any action happens.
> Use printf instead of printf_debug so that users will see the error
> without having to use -V.

OK. On a related note, we really need printf_warn/printf_notice/...
> Abort if the chip is bigger than the supported ROM decode size.
> Continuing will result in a non-bootable system in almost all cases.
> Adding a --force option later is fine with me, but I think the
> default should be to abort.

Yes.
> Attached is an updated patch with the above changes. I tested it on the
> Shuttle AK38N (all operations), output looks like this:
>
> $ flashrom v0.9.1-r753
>   No coreboot table found.
>   Found chipset "VIA VT8235", enabling flash write... OK.
>   This chipset supports the following protocols: Non-SPI.
>   Disabling flash write protection for board "Shuttle AK38N"... OK.
>   Calibrating delay loop... OK.
>   Found chip "SST SST39SF040" (512 KB, Parallel) at physical address 0xfff80000.
>   Chipset/board/programmer only supports a max. size of 256 KB. ABORTING.
>
> I also tested that with a 256KB chip all operations still work fine.
>
> Additionally I tested a 512KB chip on ASUS P4B266 (FWH) with a (simulated)
> max. decode size of 256KB and 512KB to ensure that non-parallel chips also
> work OK with the code.
>
> The updated patch is
>
> Acked-by: Uwe Hermann
>
> Feel free to commit.

Thanks. I hope to clarify one or two things in an additional round, then
commit.
> Index: chipset_enable.c
>
> --- chipset_enable.c	(revision 753)
> +++ chipset_enable.c	(working copy)
> @@ -536,6 +582,9 @@
>    reg8 |= BIOS_ROM_POSITIVE_DECODE;
>    pci_write_byte(dev, DECODE_CONTROL_REG2, reg8);
> /* No idea which buses this limit applies to. */
> //max_rom_decode.lpc = 16 * 1024 * 1024;
> return 0;
>
> }

This limit was originally added by you, but I commented out because I
was not sure if this was a LPC limit or a parallel limit or a FWH limit.
Since you are the original author, can you shed some light on this?
> > @@ -977,6 +1004,8 @@
> >    size = flash->total_size * 1024;
> >    buf = (uint8_t *) calloc(size, sizeof(char));
> > // FIXME: Check for decode size again?
> > if (erase_it) {
> >   if (flash->tested & TEST_BAD_ERASE) {
> >   	fprintf(stderr, "Erase is not working on this chip. ");
>
> Not sure about this one. Why would an additional check be needed?
> According to my tests there's no need for additional checks.

The idea behind this was to have a warning during probe (which does fail
for some chips) and abort before the first real read/write/erase action.
That way, a user can find out why probe might not have worked, and will
be stopped before he/she gets incorrect results.

What do you think? If you want me to post an updated patch for easier
review, just say so.

Regards,
Carl-Daniel