Do not call EC programmer shutdown functions explicitly.

Do not call EC programmer shutdown functions explicitly.

Use shutdown callbacks instead.

This CL is intended to help with the ordering problem with reverse MMIO writes. With this approach, the EC shutdown function is treated the same as other shutdown callbacks.

EC code will only register a shutdown callback if initialization is successful. The internal programmer shutdown routine will not call EC shutdown functions explicitly. This has the nice side-effect of cleaning up unneded calls to EC shutdown routines since the EC init routines will add shutdown callbacks as needed.

R=yjlou@chromium.org,reinauer@chromium.org
BUG=chromium-os:9586
TEST=tested manually by reading EC ROM on alex, mario, and zgb

The following verbose output is from before and after applying this patch and the reverse MMIO patch.

ZGB before:
done.SUCCESS
it85xx_shutdown():347
it85xx_exit_scratch_rom():219 was called ...
wpce775x_shutdown(): firmware not changed
wpce775x_shutdown: in_flash_update_mode: 1

ZGB after:
done.SUCCESS
wpce775x_shutdown(): firmware not changed
wpce775x_shutdown: in_flash_update_mode: 1
Restoring MMIO space at 0x777d6410


Mario before:
Reading flash... done.SUCCESS
it85xx_shutdown():347
it85xx_exit_scratch_rom():219 was called ...
it85xx_exit_scratch_rom():248 * SUCCESS.
powerd start/running, process 11020

Mario after:
Reading flash... done.SUCCESS
it85xx_shutdown():351
it85xx_exit_scratch_rom():221 was called ...
it85xx_exit_scratch_rom():250 * SUCCESS.
powerd start/running, process 11187
Restoring MMIO space at 0x7789e410


Alex before:
SUCCESS
it85xx_shutdown():347
it85xx_exit_scratch_rom():219 was called ...
mec1308_exit_passthru_mode: result: 0xaa (exited passthru mode)

Alex after:
SUCCESS
mec1308_exit_passthru_mode: result: 0xaa (exited passthru mode)
Restoring MMIO space at 0x7775d410

I also tested by explicitly setting and examining the BBS register using iotools before and after a flashrom trial, e.g.:

localhost chronos # RCBA_ADDR=$((($(iotools pci_read32 0x00 0x1f 0x00 0xf0) >> 14) << 14))
localhost chronos # GCS_ADDR=$((${RCBA_ADDR} + 0x3410))
localhost chronos # iotools mmio_read32 ${GCS_ADDR}
0x00400460
localhost chronos # ./flashrom -p internal:bus=lpc -r /tmp/foo.bin
flashrom v0.9.3 git@gitrw.chromium.org/flashrom : 2f88b7a : Apr 27 2011 04:21:03 UTC on Linux 2.6.32.26+drm33.12 (i686), built with libpci 3.1.4, GCC 4.4.3, little endian
sh: dmidecode: not found
dmidecode execution unsucessfull - continuing without DMI info
Reading flash... SUCCESS
localhost chronos # iotools mmio_read32 ${GCS_ADDR}
0x00400460

In this case, the register value gets set to 0x00400c60 by flashrom, reads the EC ROM, and then sets the register value back to 0x00400460.

[Louis, 2011-04-26 10:06:06]

Good job!!! I love your idea that put SPI-programmer-specified shutdown
functions into shutdown callback mechanism. Bravo.

Since the SPI-programmer-specified shutdown functions have been moved to
shutdown callbacks, I think programmer[].shutdown is safe to be called.
Otherwise, non-internal programmer will not be shutdown properly. Note that the
beauty of shutdown callback would make SPI-programmer-shutdown function is
called before restoring MMIO registers. Please see my comment inline.

Please also test BIOS read and write, especially the --wp-range function because
I worry the chip_restore() would restore the BIOS status bits, instead of EC's.

A minor question: where is the message going? "%s: carrying out %d shutdown
callbacks"

http://codereview.chromium.org/6897014/diff/6002/flashrom.c
File flashrom.c (right):

http://codereview.chromium.org/6897014/diff/6002/flashrom.c#newcode128
flashrom.c:128: //		.shutdown		= internal_shutdown,
This line should be restored. See programmer_shutdown() comments.

http://codereview.chromium.org/6897014/diff/6002/flashrom.c#newcode147
flashrom.c:147: .shutdown		= dummy_shutdown,
Those (below) shutdown functions are no longer called because we don't add it in
dummy_init(). See solution below.

http://codereview.chromium.org/6897014/diff/6002/flashrom.c#newcode579
flashrom.c:579: int flashrom_shutdown(void)
We don't need to change to flashrom_shutdown. programmer_shutdown() is good. See
the last line of this function.

http://codereview.chromium.org/6897014/diff/6002/flashrom.c#newcode583
flashrom.c:583: /* Registering shutdown functions is no longer allowed. */
Could you add the following code to avoid flashrom_shutdown() re-entried?

  if (!may_register_shutdown) return 0;

Thanks.

http://codereview.chromium.org/6897014/diff/6002/flashrom.c#newcode589
flashrom.c:589: return 0;
We still need programmer_table[programmer].shutdown(). Otherwise other
programmer's shutdown functions are not called.

Actually internal_shutdown() is okay to be called here since we removed all SPI
programmer specific code in shutdown callbacks, and they will be invoked before
MMIO registers are restored.

http://codereview.chromium.org/6897014/diff/6002/internal.c
File internal.c (right):

http://codereview.chromium.org/6897014/diff/6002/internal.c#newcode241
internal.c:241: register_shutdown(internal_shutdown, NULL);
remove this if we restore the shutdown function in the programmer_table[] in
flashrom.c

http://codereview.chromium.org/6897014/diff/6002/mec1308.c
File mec1308.c (right):

http://codereview.chromium.org/6897014/diff/6002/mec1308.c#newcode375
mec1308.c:375: msg_pdbg("%s(): successfully initialized mec1308\n", __func__);
Swap this message to next line?  To sync the same behavior as wpce775.

http://codereview.chromium.org/6897014/diff/6002/programmer.h
File programmer.h (right):

http://codereview.chromium.org/6897014/diff/6002/programmer.h#newcode112
programmer.h:112: int flashrom_shutdown(void);
hm... I am not sure if renaming is a good idea since this may conflict to
upstream if one day upstream improves the shutdown mecahnism. I'd like to
suggest to keep the name. Also see my comment in programmer_shutdown()
implementation.

[dhendrix, 2011-04-27 02:08:06]

http://codereview.chromium.org/6897014/diff/6002/flashrom.c
File flashrom.c (right):

http://codereview.chromium.org/6897014/diff/6002/flashrom.c#newcode579
flashrom.c:579: int flashrom_shutdown(void)
On 2011/04/26 10:06:06, Louis wrote:
> We don't need to change to flashrom_shutdown. programmer_shutdown() is good.
See
> the last line of this function.

Agreed. That was a bit ambitious of me :-)

Reverted the re-naming for now.

http://codereview.chromium.org/6897014/diff/6002/flashrom.c#newcode583
flashrom.c:583: /* Registering shutdown functions is no longer allowed. */
On 2011/04/26 10:06:06, Louis wrote:
> Could you add the following code to avoid flashrom_shutdown() re-entried?
> 
>   if (!may_register_shutdown) return 0;
> 
> Thanks.

Good idea, however may_register_shutdown is set by this function explicitly.
I've added "programmer_shutdown_done" as a static variable to keep track of
this.

http://codereview.chromium.org/6897014/diff/6002/flashrom.c#newcode589
flashrom.c:589: return 0;
On 2011/04/26 10:06:06, Louis wrote:
> We still need programmer_table[programmer].shutdown(). Otherwise other
> programmer's shutdown functions are not called.

Hmm, the reason I removed programmer_table[programmer].shutdown() is because it
causes the shutdown function for the programmers we care about to be called
twice. This can cause undesirable behavior, such as double free() of certain
buffers.

I think there are two options here:
1. Add code in the *_shutdown functions for internal programmer and ECs to check
if *_shutdown() has already been called, and return if so. This is sort of a
hack IMHO.

2. Complete this CL and migrate all programmers to use shutdown callbacks so
that we no longer need this.

I think I prefer option 2, but am open to the other option. LMK if you have a
strong preference.

On 2011/04/26 10:06:06, Louis wrote:
> Actually internal_shutdown() is okay to be called here since we removed all
SPI
> programmer specific code in shutdown callbacks, and they will be invoked
before
> MMIO registers are restored.

Yes. The code flow for the devices we care about should look like:
1. Internal programmer init called, registers a shutdown callback.
2. Chipset initialization done, including setting BBS field on x86 southbridge.
3. EC init called, registers EC shutdown callback

And then the reverse is done when programmer_shutdown() is called.

http://codereview.chromium.org/6897014/diff/6002/internal.c
File internal.c (right):

http://codereview.chromium.org/6897014/diff/6002/internal.c#newcode241
internal.c:241: register_shutdown(internal_shutdown, NULL);
On 2011/04/26 10:06:06, Louis wrote:
> remove this if we restore the shutdown function in the programmer_table[] in
> flashrom.c

The idea is to no longer rely on programmer_table[].shutdown, since it can only
apply to a single programmer.

http://codereview.chromium.org/6897014/diff/6002/mec1308.c
File mec1308.c (right):

http://codereview.chromium.org/6897014/diff/6002/mec1308.c#newcode375
mec1308.c:375: msg_pdbg("%s(): successfully initialized mec1308\n", __func__);
On 2011/04/26 10:06:06, Louis wrote:
> Swap this message to next line?  To sync the same behavior as wpce775.

On second thought, I think we should do these earlier so that we ensure the
shutdown callback is registered _before_ entering passthru mode. LMK what you
think.

http://codereview.chromium.org/6897014/diff/6002/programmer.h
File programmer.h (right):

http://codereview.chromium.org/6897014/diff/6002/programmer.h#newcode112
programmer.h:112: int flashrom_shutdown(void);
On 2011/04/26 10:06:06, Louis wrote:
> hm... I am not sure if renaming is a good idea since this may conflict to
> upstream if one day upstream improves the shutdown mecahnism. I'd like to
> suggest to keep the name. Also see my comment in programmer_shutdown()
> implementation.

agreed -- reverted back to original naming for now.

[Louis, 2011-04-27 02:25:33]

Please see my comment inline. Actually we can do it more simply and match our
needs (if it runs as what I think).

http://codereview.chromium.org/6897014/diff/6002/flashrom.c
File flashrom.c (right):

http://codereview.chromium.org/6897014/diff/6002/flashrom.c#newcode589
flashrom.c:589: return 0;
Sorry that I didn't say my comment very clearly. My suggestion is:

  1. remove line 241 in internal.c
     register_shutdown(internal_shutdown, NULL);
  2. restore line 589 in this file
     return programmer_table[programmer].shutdown();

In this way, the progrmamer's shutdown function would be called only once and it
also follows the code flow you described (perfect!). Bonus is we don't need to
migrate all programmers for callback. This makes your CL simpler and more
elegant.

[dhendrix, 2011-04-27 20:07:29]

Thanks for the feedback! I've made the changes, done further testing, and
updated the CL description. PTAL.

http://codereview.chromium.org/6897014/diff/6002/flashrom.c
File flashrom.c (right):

http://codereview.chromium.org/6897014/diff/6002/flashrom.c#newcode589
flashrom.c:589: return 0;
On 2011/04/27 02:25:33, Louis wrote:
> Sorry that I didn't say my comment very clearly. My suggestion is:
> 
>   1. remove line 241 in internal.c
>      register_shutdown(internal_shutdown, NULL);
>   2. restore line 589 in this file
>      return programmer_table[programmer].shutdown();
> 
> In this way, the progrmamer's shutdown function would be called only once and
it
> also follows the code flow you described (perfect!). Bonus is we don't need to
> migrate all programmers for callback. This makes your CL simpler and more
> elegant.

Ah! That makes more sense. Thanks for the detailed explanation.

[Stefan Reinauer, 2011-04-27 20:34:05]

http://codereview.chromium.org/6897014/diff/13001/internal.c
File internal.c (right):

http://codereview.chromium.org/6897014/diff/13001/internal.c#newcode374
internal.c:374: #if 0
delete this?

http://codereview.chromium.org/6897014/diff/13001/mec1308.c
File mec1308.c (right):

http://codereview.chromium.org/6897014/diff/13001/mec1308.c#newcode45
mec1308.c:45: void mec1308_shutdown(void *);
Please,

1. no function prototypes in .c files
2. no nameless parameters in function prototypes.

Since this function is explicitly never called outside of this file (except
through the function pointer) it should be made static and the function
prototype should be deleted.

http://codereview.chromium.org/6897014/diff/13001/mec1308.c#newcode383
mec1308.c:383: void mec1308_shutdown(void *data)
make static and move upstairs.

http://codereview.chromium.org/6897014/diff/13001/wpce775x.c
File wpce775x.c (right):

http://codereview.chromium.org/6897014/diff/13001/wpce775x.c#newcode55
wpce775x.c:55: void wpce775x_shutdown(void *);
delete this.

http://codereview.chromium.org/6897014/diff/13001/wpce775x.c#newcode609
wpce775x.c:609: void wpce775x_shutdown(void *data)
static and move before register

[dhendrix, 2011-04-27 21:15:05]

Thanks for the style comments, Stefan. I also applied your suggested fixes to
it85spi.c.

http://codereview.chromium.org/6897014/diff/13001/internal.c
File internal.c (right):

http://codereview.chromium.org/6897014/diff/13001/internal.c#newcode374
internal.c:374: #if 0
On 2011/04/27 20:34:05, Stefan Reinauer wrote:
> delete this?

Done.

http://codereview.chromium.org/6897014/diff/13001/mec1308.c
File mec1308.c (right):

http://codereview.chromium.org/6897014/diff/13001/mec1308.c#newcode45
mec1308.c:45: void mec1308_shutdown(void *);
On 2011/04/27 20:34:05, Stefan Reinauer wrote:
> Please,
> 
> 1. no function prototypes in .c files
> 2. no nameless parameters in function prototypes.
> 
> Since this function is explicitly never called outside of this file (except
> through the function pointer) it should be made static and the function
> prototype should be deleted.

Yeah, that's ugly. I just did it to reduce diffs earlier.

http://codereview.chromium.org/6897014/diff/13001/mec1308.c#newcode383
mec1308.c:383: void mec1308_shutdown(void *data)
On 2011/04/27 20:34:05, Stefan Reinauer wrote:
> make static and move upstairs.

Done.

http://codereview.chromium.org/6897014/diff/13001/wpce775x.c
File wpce775x.c (right):

http://codereview.chromium.org/6897014/diff/13001/wpce775x.c#newcode55
wpce775x.c:55: void wpce775x_shutdown(void *);
On 2011/04/27 20:34:05, Stefan Reinauer wrote:
> delete this.

Done.

http://codereview.chromium.org/6897014/diff/13001/wpce775x.c#newcode609
wpce775x.c:609: void wpce775x_shutdown(void *data)
On 2011/04/27 20:34:05, Stefan Reinauer wrote:
> static and move before register

Done.

[Stefan Reinauer, 2011-04-27 21:21:04]

LGTM

[dhendrix, 2011-04-27 21:32:48]

On 2011/04/27 21:21:04, Stefan Reinauer wrote:
> LGTM

I think I addressed Louis' comments, so I'll go ahead and push this CL so that
he can push the reverse MMIO patch.