Re: [flashrom] [PATCH 2/2] ichspi: add support for Intel Hardware Sequencing

[Carl-Daniel Hailfinger]

Am 07.11.2011 23:51 schrieb Stefan Tauner:
> On Mon, 07 Nov 2011 00:15:53 +0100
> Carl-Daniel Hailfinger  wrote:
> > > Am 26.10.2011 15:35 schrieb Stefan Tauner:
> > > > > Based on the new opaque programmer framework this patch adds support
> > > > > for Intel Hardware Sequencing on ICH8 and its successors.
> > > > >
> > > > > By default (or when setting the ich_spi_mode option to auto)
> > > > > the module tries to use swseq and only activates hwseq if need be:
> > > > > if important opcodes are inaccessible due to lockdown
> > > > > if more than one flash chip is attached.
> > > > >
> > > > > The other options (swseq, hwseq) select the respective mode (if possible).
> > > > >
> > > > > A general description of Hardware Sequencing can be found in this blog entry:
> > > > > http://blogs.coreboot.org/blog/2011/06/11/gsoc-2011-flashrom-part-1/
> > > > >
> > > > > TODO: adding real documentation when we have a directory for it
> > > > >
> > > > > Signed-off-by: Stefan Tauner
> > >
> > > Acked-by: Carl-Daniel Hailfinger
>
> thanks, but ignored for now.

Hm, so if I ack this patch as well, will you commit one combined patch?
> > > with a few small comments.
>
> in contrast to those of course ;)
> > > > > flashrom.8 |   20 +++++
> > > > >  ichspi.c   |  268 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> > > > >  2 files changed, 283 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/flashrom.8 b/flashrom.8
> > > > > index a8f4660..66cde4f 100644
> > > > > --- a/flashrom.8
> > > > > +++ b/flashrom.8
> > > > > @@ -303,6 +303,26 @@ is the I/O port number (must be a multiple of 8). In the unlikely case
> > > > >  flashrom doesn't detect an active IT87 LPC<->SPI bridge, please send a bug
> > > > >  report so we can diagnose the problem.
> > > > >  .sp
> > > > > +If you have an Intel chipset with an ICH8 or later southbridge with SPI flash
> > > > > +attached, and if a valid descriptor was written to it (e.g. by the vendor), the
> > > > > +chipset provides an alternative way to access the flash chip(s) named
> > > > > +.BR "Hardware Sequencing" .
> > > > > +It is much simpler than the normal access method (called
> > > > > +.BR "Software Sequencing" "),"
> > > > > +but does not allow the software to choose the SPI commands to be sent.
> > > > > +You can use the
> > > > > +.sp
> > > > > +.B "  flashrom -p internal:ich_spi_mode=value"
> > > > > +.sp
> > > > > +syntax where value can be
> > > > > +.BR auto ", " swseq " or " hwseq .
> > > > > +By default
> > > > > +.RB "(or when setting " ich_spi_mode=auto )
> > > > > +the module tries to use swseq and only activates hwseq if need be (e.g. if
> > > > > +important opcodes are inaccessible due to lockdown; or if more than one flash
> > > > > +chip is attached). The other options (swseq, hwseq) select the respective mode
> > > > > +(if possible).
> > > > > +.sp
> > > > >  If you have an Intel chipset with an ICH6 or later southbridge and if you want
> > > > >  to set specific IDSEL values for a non-default flash chip or an embedded
> > > > >  controller (EC), you can use the
> > > > > diff --git a/ichspi.c b/ichspi.c
> > > > > index bc03554..1d5dd34 100644
> > > > > --- a/ichspi.c
> > > > > +++ b/ichspi.c
> > > > > @@ -575,6 +575,25 @@ static int program_opcodes(int ich_generation, OPCODES *op, int enable_undo)
> > > > >    return 0;
> > > > >  }
> > > > >
> > > > > +/* Returns true if the most important opcodes are accessible. */
> > >
> > > You assume that some erase opcode will be available if BYTE_PROGRAM is
> > > available. I think that assumption is reasonable, but it could be
> > > documented in this comment above the function.
>
> this was just a basis for discussion actually. sorry for not explaining
> that bit. there are more problems than just the missing check for *any*
> available erase opcode.
>
> i do not intend to fix this soon. OTOH it is not really a problem imho.
> if this trigger would enable hwseq the bios has probably not only
> locked down the opcodes, but also set up PR or FREG/FRAP protections,
> which we can not deal with correctly yet, not even with hwseq. so *not*
> selecting hwseq due to a too simple ich_check_opcodes method will not
> hurt us, because it would not have saved us from failure anyway.
>
> i have reduced the method to the absolute minimum (see patch) and
> added the following comment:
> FIXME: this should also check for
> at least one probing opcode (RDID (incl. AT25F variants?), REMS, RES?)
> at least one erasing opcode (lots.)
> at least one program opcode (BYTE_PROGRAM, AAI_WORD_PROGRAM, ...?)
> necessary preops? (EWSR, WREN, ...?)

Nice.
> > > > > […]
> > > > > arg = extract_programmer_param("ich_spi_mode");
> > > > > if (arg && !strcmp(arg, "hwseq")) {
> > > > > 	ich_spi_mode = ich_hwseq;
> > > > > 	msg_pspew("user selected hwseq\n");
> > > > > } else if (arg && !strcmp(arg, "swseq")) {
> > > > > 	ich_spi_mode = ich_swseq;
> > > > > 	msg_pspew("user selected swseq\n");
> > > > > } else if (arg && !strcmp(arg, "auto")) {
> > > > > 	msg_pspew("user selected auto\n");
> > > > > 	ich_spi_mode = ich_auto;
> > > > > } else if (arg && !strlen(arg))
> > > > > 	msg_pinfo("Missing argument for ich_spi_mode.\n");
> > > > > else if (arg)
> > > > > 	msg_pinfo("Unknown argument for ich_spi_mode: %s\n", arg);
> > >
> > > We should return an error all the way up to programmer init for both
> > > cases (and clean up everything). I know that this is a complicated code
> > > path, so if you decide not to fail programmer init here, please add a
> > > comment like the one below:
> > > /* FIXME: Return an error to programmer_init. */
>
> fixing this was not that easy, because the ich code in chipset_enable.c
> was not passing the fatal error further; see patch.

This code was indeed suboptimal. Thanks for fixing it!
> > > > > […]
> > > > > if (ich_spi_mode == ich_hwseq) {
> > > > > 	if (!desc_valid) {
> > > > > 		msg_perr("Hardware sequencing was requested "
> > > > > 			 "but the flash descriptor is not "
> > > > > 			 "valid. Aborting.\n");
> > > > > 		return 1;  
> > >
> > > Can you check that this indeed causes flashrom to return an error from
> > > programmer_init? Chipset init IIRC ignores most errors unless they are
> > > somehow declared to be fatal.
>
> done. and also if a bogus ich_gen is passed to ichspi_init at the
> beginning of the function.
>
> i am not sure if it was due to the changes to the previous patches, but
> ich_init_opcodes() were missing from the ich8+ code path (again).
>
> besides that i have also added null-pointer guards to find_opcode and
> find_preop, because i think it matches the other opcode methods better
> (curopcodes == NULL has some meaning and is actively used/checked in
> other functions).

Nice.
> ps: you like to abort when the user gets the command line wrong for
> safety. there is a case we do not handle in this manner. for example
> with this patch set one can do:
> flashrom -p internal:laptop=force_I_want_a_brick,ich_spi_mode
> it prints "Unhandled programmer parameters: ich_spi_mode" but
> continues, you may wanna look into this.

I have a fix handy, I just didn't want to push such a patch in before
hwseq because that would feel like jumping the review queue.
> > From 9d85001930afc54ca724d75dc80a7ca7e26c1b6d Mon Sep 17 00:00:00 2001
>
> From: Stefan Tauner 
> Date: Mon, 7 Nov 2011 21:06:36 +0100
> Subject: [PATCH] squash! ichspi: add support for Intel Hardware Sequencing
>
> Fix enable_flash_ich_dc_spi to pass ERROR_FATAL from ich_init_spi.
> The whole error handling looks a bit odd to me, so this patch does change very
> little. Also, it does not touch the tunnelcreek method, which should be refactored
> anyway.
>
> Add null-pointer guards to find_opcode and find_preop
> to matches the other opcode methods better:
> curopcodes == NULL has some meaning and is actively used/checked in other
> functions.
>
> Signed-off-by: Stefan Tauner

Acked-by: Carl-Daniel Hailfinger 
with comments.
> diff --git a/ichspi.c b/ichspi.c
> index 85456cd..3c3b7f1 100644
> --- a/ichspi.c
> +++ b/ichspi.c
> @@ -560,23 +570,29 @@ static int program_opcodes(OPCODES *op, int enable_undo)
>    return 0;
>  }
>
> -/* Returns true if the most important opcodes are accessible. */
> +/*
> Checks if all of the most important opcodes are accessible.
> Returns 0 if they are, or the first opcode to be found inaccessible.
> FIXME: this should also check for
> at least one probing opcode (RDID (incl. AT25F variants?), REMS, RES?)
> at least one erasing opcode (lots.)
> at least one program opcode (BYTE_PROGRAM, AAI_WORD_PROGRAM, ...?)
> necessary preops? (EWSR, WREN, ...?)
> */
>
> static int ich_check_opcodes()
>  {
>    uint8_t ops[] = {
>    	JEDEC_READ,
> JEDEC_BYTE_PROGRAM,
> JEDEC_RDSR,
> 0
> };
> int i = 0;
> while (ops[i] != 0) {
> msg_pspew("checking for opcode 0x%02x\n", ops[i]);
> if (find_opcode(curopcodes, ops[i]) == -1)
> 	return 0;
> 	return ops[i];

Can you return 1 (or -1 or some other nonzero value) instead? Returning
one missing opcode reduces redability and provides no gain (other
missing opcodes are ignored). Unless I'm mistaken, we'll see the
available opcodes anyway in the verbose log.
> i++;
>
> }
> return 1;
> return 0;
>
> }
>
> /*
> @@ -1705,9 +1729,9 @@ int ich_init_spi(struct pci_dev *dev, uint32_t base, void *rcrb,
>    	}
> if (ich_spi_mode == ich_auto && ichspi_lock &&
>     !ich_check_opcodes()) {
>     (i = ich_check_opcodes()) != 0) {

ich_check_opcodes()) {
> 	msg_pinfo("Enabling hardware sequencing because "
> 		  "some important opcodes are locked.\n");
> 		  "opcode 0x%02x is inaccessible.\n", i);

Please keep the old message or at least replace "opcode 0x%02x" with
"some important opcode".
> 	ich_spi_mode = ich_hwseq;
> }

Regards,
Carl-Daniel