Do not call EC programmer shutdown functions explicitly.

flashrom.c

 /*
  * This file is part of the flashrom project.
  *
  * Copyright (C) 2000 Silicon Integrated System Corporation
  * Copyright (C) 2004 Tyan Corp <yhlu@tyan.com>
  * Copyright (C) 2005-2008 coresystems GmbH
  * Copyright (C) 2008,2009 Carl-Daniel Hailfinger
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ skipping 106 matching lines @@
 unsigned long flashbase;
 
 /* Is writing allowed with this programmer? */
 int programmer_may_write;
 
 const struct programmer_entry programmer_table[] = {
 #if CONFIG_INTERNAL == 1
         {
                 .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,
                 .chip_readw             = internal_chip_readw,
                 .chip_readl             = internal_chip_readl,
                 .chip_readn             = internal_chip_readn,
                 .chip_writeb            = internal_chip_writeb,
                 .chip_writew            = internal_chip_writew,
                 .chip_writel            = internal_chip_writel,
                 .chip_writen            = fallback_chip_writen,
                 .delay                  = internal_delay,
         },
 #endif
 
 #if CONFIG_DUMMY == 1
         {
                 .name                   = "dummy",
                 .init                   = dummy_init,
                 .shutdown               = dummy_shutdown,

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

Those (below) shutdown functions are no longer called because we don't add it in
dummy_init(). See solution below.

                 .map_flash_region       = dummy_map,
                 .unmap_flash_region     = dummy_unmap,
                 .chip_readb             = dummy_chip_readb,
                 .chip_readw             = dummy_chip_readw,
                 .chip_readl             = dummy_chip_readl,
                 .chip_readn             = dummy_chip_readn,
                 .chip_writeb            = dummy_chip_writeb,
                 .chip_writew            = dummy_chip_writew,
                 .chip_writel            = dummy_chip_writel,
                 .chip_writen            = dummy_chip_writen,
@@ skipping 411 matching lines @@
 
         while (chip_restore_fn_count > 0) {
                 int i = --chip_restore_fn_count;
                 rc |= chip_restore_fn[i].func(chip_restore_fn[i].flash,
                                               chip_restore_fn[i].status);
         }
 
         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,
                                   size_t len)
 {
         return programmer_table[programmer].map_flash_region(descr,
                                                              phys_addr, len);
 }
 
 void programmer_unmap_flash_region(void *virt_addr, size_t len)
@@ skipping 1404 matching lines @@
                 if (ret && write_it)
                         emergency_help_message();
         }
 
 out:
         free(oldcontents);
         free(newcontents);
 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;
 }