Do not call EC programmer shutdown functions explicitly.

flashrom.c

Index: flashrom.c
diff --git a/flashrom.c b/flashrom.c
index a9fbbc1cb0e0dead111d25982d43269191f6e59c..58b66a9426297fcfd41c20e4f99cb3fb6d649b1a 100644
--- a/flashrom.c
+++ b/flashrom.c
@@ -124,7 +124,8 @@ const struct programmer_entry programmer_table[] = {
 	{
 		.name			= "internal",
 		.init			= internal_init,
-		.shutdown		= internal_shutdown,
+		/* called implicitly using shutdown callback */
+//		.shutdown		= internal_shutdown,

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

This line should be restored. See programmer_shutdown() comments.

 		.map_flash_region	= physmap,
 		.unmap_flash_region	= physunmap,
 		.chip_readb		= internal_chip_readb,
@@ -575,15 +576,17 @@ int chip_restore()
 	return rc;
 }
 
-int programmer_shutdown(void)
+int flashrom_shutdown(void)

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

We don't need to change to flashrom_shutdown. programmer_shutdown() is good. See
the last line of this function.

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

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

Reverted the re-naming for now.

 {
+	msg_gdbg("%s: carrying out %d shutdown callbacks\n",
+	         __func__, shutdown_fn_count);
 	/* Registering shutdown functions is no longer allowed. */

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

Could you add the following code to avoid flashrom_shutdown() re-entried?

  if (!may_register_shutdown) return 0;

Thanks.

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

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.

 	may_register_shutdown = 0;
 	while (shutdown_fn_count > 0) {
 		int i = --shutdown_fn_count;
 		shutdown_fn[i].func(shutdown_fn[i].data);
 	}
-	return programmer_table[programmer].shutdown();
+	return 0;

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

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.

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

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:
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.

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

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]

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

 }
 
 void *programmer_map_flash_region(const char *descr, unsigned long phys_addr,
@@ -2008,10 +2011,10 @@ out:
 out_nofree:
 	chip_restore();	/* must be done before programmer_shutdown() */
 	/*
-	 * programmer_shutdown() call is moved to cli_mfg() in chromium os
+	 * flashrom_shutdown() call is moved to cli_mfg() in chromium os
 	 * tree. This is because some operations, such as write protection,
 	 * requires programmer_shutdown() but does not call doit().
 	 */
-//	programmer_shutdown();
+//	flashrom_shutdown();
 	return ret;
 }