Re: [flashrom] [Patch] + RFC: Use shutdown callback mechanism to shutdown programmers

[Carl-Daniel Hailfinger]

Am 17.05.2011 01:00 schrieb David Hendricks:
> Finally got a chance to revise this. There are a lot of small, tedious
> changes to all the programmer .c files and some interesting changes to
> flashrom.c. Otherwise it's a pretty simple patch.
>
> To recap from earlier discussion, this version does the following:
> Registers a shutdown callback during initialization for each programmer.
>
> See notes below.
> Kills the .shutdown function pointer from programmer_entry struct
> Updates all programmer shutdown functions to return an int and take a void
>
> *data arg. Also, make them static.

So far, so good.
> programmer_shutdown() checks the return code of all shutdown callbacks.

I'm not 100% sure about this one. If we plan to make libflashrom viable
in cases where various programmers are initialized more than once (of
course with programmer shutdown in between), this is essential to refuse
further programmer init calls once an init/shutdown screwed up. OTOH, we
have no libflashrom user for now.
> A few minor questions:
> If programmer_init() fails when called in cli_classic(), should we call
>
> programmer_shutdown()? As Carl-Daniel noted earlier, this could potentially
> be used to do stuff like release_io_perms(), or perhaps free resources
> allocated by an init routine that fails.

I think so, yes. Especially for libflashrom.
> nicintel_spi -- Looks like flash write enable/disable should be done using
>
> rpci?

No, rpci_ would clobber other bits. The comment in nicintel_spi
hopefully explains that. However, you have a valid point. We should
store the original state of the write enable register somewhere instead
of forcing it off on shutdown.
> drkaiser - shutdown function missing a physunmap?

Nice find! Same for gfxnvidia.

I wonder if this applies to the various internal chipset functions as
well... and if we should eventually move towards implicit unmap
registration. If we decide to do that implicit registration, we should
create a separate patch for it to keep this patch reviewable.
> Additional notes:
> Most programmers have a simple init routines with no failure condition, so
>
> placement of register_shutdown() doesn't really matter.

Well, it should happen after the relevant variables are initialized.
> serialport_shutdown non-static since it's used by buspirate.

OK.
> There were cases where the programmer's init routine would do something
>
> less simple, like open a file or map physical memory, and the shutdown
> callback would undo it. In those cases, placement took a little more
> thought. Someone more familiar with those should double-check that this
> patch does the right thing for buspirate, serprog, and satamv.

To be honest, I don't know serprog well enough to say something useful
about it. And the massive code move in serprog makes review a bit hard.
satamv looks good.
I have to trust you with it85, I don't have any insight into what the EC
expects there.
buspirate looks good.

IIRC some of your error returns were inconsistent: return -1 vs. return
1. I can't find the offender right now, but please recheck that.
> I did a quick test using an NM10/ICH7 platform and callbacks were shown in
> verbose mode output as expected.

Thanks for the patch, it is really appreciated.
> Signed-off-by: David Hendricks

A general comment first... I expect some more code changes in programmer
init (other pending patches for programmer init), and I wonder if it
would make sense to keep the shutdown functions in the code where they
are, and just add forward declarations. It would definitely make the
patch easier to review, but it might also help with code history (svn
blame). OTOH forward declarations are ugly...
> Index: ogp_spi.c
>
> --- ogp_spi.c	(revision 1299)
> +++ ogp_spi.c	(working copy)
> @@ -93,6 +93,15 @@
>    .release_bus = ogp_release_spibus,
>  };
>
> +static int ogp_spi_shutdown(void *data)
> +{
> physunmap(ogp_spibar, 4096);
> pci_cleanup(pacc);
> release_io_perms();
> return 0;
>
> +}
>
> int ogp_spi_init(void)
>  {
>    char *type;
> @@ -120,6 +129,9 @@
>
> get_io_perms();
> if (register_shutdown(ogp_spi_shutdown, NULL))

Move it after the physmap call or you'll unmap an uninitialized address.
> return 1;
>
> io_base_addr = pcidev_init(PCI_BASE_ADDRESS_0, ogp_spi);
>
> ogp_spibar = physmap("OGP registers", io_base_addr, 4096);
>
> @@ -130,12 +142,3 @@
>
> return 0;
>  }
>
> -int ogp_spi_shutdown(void)
> -{
> physunmap(ogp_spibar, 4096);
> pci_cleanup(pacc);
> release_io_perms();
> return 0;
>
> -}
> Index: gfxnvidia.c
> ===================================================================
> --- gfxnvidia.c	(revision 1299)
> +++ gfxnvidia.c	(working copy)
> @@ -60,12 +60,26 @@
>    {},
>  };
>
> +static int gfxnvidia_shutdown(void *data)
> +{
> /* Flash interface access is disabled (and screen enabled) automatically
> * by PCI restore.
> */
> pci_cleanup(pacc);
> release_io_perms();
> return 0;
>
> +}
>
> int gfxnvidia_init(void)
>  {
>    uint32_t reg32;
>
> get_io_perms();
> /* must be done before rpci calls */
> if (register_shutdown(gfxnvidia_shutdown, NULL))

If you change shutdown to include unmap, you'll have to move the
register_shutdown after physmap, and that's unfortunately after rpci...
boom! Can you reorder this a bit to call physmap before using rpci? That
would make sense anyway because we don't want to keep the screen
disabled in case physmap fails.
> return 1;
>
> io_base_addr = pcidev_init(PCI_BASE_ADDRESS_0, gfx_nvidia);
>
> io_base_addr += 0x300000;
>
> @@ -86,16 +100,6 @@
>    return 0;
>  }
>
> -int gfxnvidia_shutdown(void)
> -{
> /* Flash interface access is disabled (and screen enabled) automatically
> * by PCI restore.
> */
> pci_cleanup(pacc);
> release_io_perms();
> return 0;
>
> -}
>
> void gfxnvidia_chip_writeb(uint8_t val, chipaddr addr)
>  {
>    pci_mmio_writeb(val, nvidia_bar + (addr & GFXNVIDIA_MEMMAP_MASK));
> Index: dummyflasher.c
> ===================================================================
> --- dummyflasher.c	(revision 1299)
> +++ dummyflasher.c	(working copy)
> @@ -73,6 +73,23 @@
>    .read = default_spi_read,
>    .write_256 = dummy_spi_write_256,
>  };
>
> +static int dummy_shutdown(void *data)
> +{
> msg_pspew("%s\n", __func__);
>
> +#if EMULATE_CHIP
> if (emu_chip != EMULATE_NONE) {
> if (emu_persistent_image) {
> 	msg_pdbg("Writing %s\n", emu_persistent_image);
> 	write_buf_to_file(flashchip_contents, emu_chip_size,
> 			  emu_persistent_image);
> }
> free(flashchip_contents);
> }
>
> +#endif
> return 0;
>
> +}
>
> int dummy_init(void)
>  {
>    char *bustext = NULL;
> @@ -180,6 +197,11 @@
>    	msg_perr("Out of memory!\n");
>    	return 1;
>    }
> /* shutdown will free the flashchip contents */
> if (register_shutdown(dummy_shutdown, NULL))

A bit earlier we already have a return 0 for the non-emulation case.
This means register_shutdown has to happen there as well, or we replace
the various return 0 with goto out (create an out: label at the end of
the init function) and add register_shutdown to the end of the init
function.
> return 1;
> msg_pdbg("Filling fake flash chip with 0xff, size %i\n", emu_chip_size);
>  memset(flashchip_contents, 0xff, emu_chip_size);
>
> @@ -204,22 +226,6 @@
>    return 0;
>  }
>
> -int dummy_shutdown(void)
> -{
> msg_pspew("%s\n", __func__);
>
> -#if EMULATE_CHIP
> if (emu_chip != EMULATE_NONE) {
> if (emu_persistent_image) {
> 	msg_pdbg("Writing %s\n", emu_persistent_image);
> 	write_buf_to_file(flashchip_contents, emu_chip_size,
> 			  emu_persistent_image);
> }
> free(flashchip_contents);
> }
>
> -#endif
> return 0;
>
> -}
>
> void *dummy_map(const char *descr, unsigned long phys_addr, size_t len)
>  {
>    msg_pspew("%s: Mapping %s, 0x%lx bytes at 0x%08lx\n",
> Index: nic3com.c
> ===================================================================
> --- nic3com.c	(revision 1299)
> +++ nic3com.c	(working copy)
> @@ -55,6 +55,21 @@
>    {},
>  };
>
> +static int nic3com_shutdown(void *data)
> +{
> /* 3COM 3C90xB cards need a special fixup. */
> if (id == 0x9055 || id == 0x9001 || id == 0x9004 || id == 0x9005
>    || id == 0x9006 || id == 0x900a || id == 0x905a || id == 0x9058) {
> /* Select register window 3 and restore the receiver status. */
> OUTW(SELECT_REG_WINDOW + 3, io_base_addr + INT_STATUS);
> OUTL(internal_conf, io_base_addr + INTERNAL_CONFIG);
> }
> pci_cleanup(pacc);
> release_io_perms();
> return 0;
>
> +}
>
> int nic3com_init(void)
>  {
>    get_io_perms();
> @@ -84,24 +99,9 @@
>    buses_supported = CHIP_BUSTYPE_PARALLEL;
>    max_rom_decode.parallel = 128 * 1024;
> return 0;
> return register_shutdown(nic3com_shutdown, NULL);

I like the following idiom (which you used everywhere else) better:
if (register_shutdown(...))
    return 1;
return 0;
> }
>
> -int nic3com_shutdown(void)
> -{
> /* 3COM 3C90xB cards need a special fixup. */
> if (id == 0x9055 || id == 0x9001 || id == 0x9004 || id == 0x9005
>    || id == 0x9006 || id == 0x900a || id == 0x905a || id == 0x9058) {
> /* Select register window 3 and restore the receiver status. */
> OUTW(SELECT_REG_WINDOW + 3, io_base_addr + INT_STATUS);
> OUTL(internal_conf, io_base_addr + INTERNAL_CONFIG);
> }
> pci_cleanup(pacc);
> release_io_perms();
> return 0;
>
> -}
>
> void nic3com_chip_writeb(uint8_t val, chipaddr addr)
>  {
>    OUTL((uint32_t)addr, io_base_addr + BIOS_ROM_ADDR);
> Index: nicintel.c
> ===================================================================
> --- nicintel.c	(revision 1299)
> +++ nicintel.c	(working copy)
> @@ -41,6 +41,14 @@
>
> #define CSR_FCR 0x0c
>
> +static int nicintel_shutdown(void *data)
> +{
> physunmap(nicintel_bar, NICINTEL_MEMMAP_SIZE);

physunmap(nicintel_control_bar) is missing.
> pci_cleanup(pacc);
> release_io_perms();
> return 0;
>
> +}
>
> int nicintel_init(void)
>  {
>    uintptr_t addr;
> @@ -60,6 +68,9 @@
>    if (nicintel_bar == ERROR_PTR)
>    	goto error_out;
> if (register_shutdown(nicintel_shutdown, NULL))

Move this after the next physmap.
> return 1;
> /* FIXME: Using pcidev_dev _will_ cause pretty explosions in the future. */
>  addr = pcidev_validate(pcidev_dev, PCI_BASE_ADDRESS_0, nics_intel);
>  /* FIXME: This is not an aligned mapping. Use 4k? */
>
> @@ -90,14 +101,6 @@
>    return 1;
>  }
>
> -int nicintel_shutdown(void)
> -{
> physunmap(nicintel_bar, NICINTEL_MEMMAP_SIZE);
> pci_cleanup(pacc);
> release_io_perms();
> return 0;
>
> -}
>
> void nicintel_chip_writeb(uint8_t val, chipaddr addr)
>  {
>    pci_mmio_writeb(val, nicintel_bar + (addr & NICINTEL_MEMMAP_MASK));
> Index: flashrom.c
> ===================================================================
> --- flashrom.c	(revision 1299)
> +++ flashrom.c	(working copy)
> @@ -543,13 +525,15 @@
>
> int programmer_shutdown(void)
>  {
> int rc = 0;
> /* Registering shutdown functions is no longer allowed. */
>  may_register_shutdown = 0;
>  while (shutdown_fn_count > 0) {
>   int i = --shutdown_fn_count;
> shutdown_fn[i].func(shutdown_fn[i].data);
> rc |= shutdown_fn[i].func(shutdown_fn[i].data);

That's a big question. Should we proceed even if one shutdown function
failed, or should we stop? Both options make sense, and it's all about
which one will screw the user less. Error cases suck if you're dealing
with hardware.
> }
> return programmer_table[programmer].shutdown();
> return rc;
>
> }
>
> void *programmer_map_flash_region(const char *descr, unsigned long phys_addr,

Regards,
Carl-Daniel