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

[David Hendricks]

Hi everyone,
Lately we've been looking into reverse
PCI<http://flashrom.org/trac/flashrom/changeset/1232>and
MMIO <http://patchwork.coreboot.org/patch/2331/> writes, along with the
shutdown callback mechanism.

The main usage case for Flashrom makes this a pretty simple matter, since
most users only worry about a single programmer being used. However, when
external devices are used which depend on internal programmer settings,
things can get messy.

Consider the case of a laptop with an x86 southbridge and an EC -- To
communicate with the EC thru the LPC/FWH interface, some register settings
in the southbridge might need to be changed. The code flow should be:
1. Set up SB, use rpci_* and rmmio_* to register reverse PCI and MMIO
callbacks wherever necessary.
2. Set up EC.
3. Run code.
4. Call EC shutdown routine.
5. Reverse SB PCI/MMIO writes.

However, the current code calls the programmer shutdown routine in step 4 *
after* doing all shutdown callbacks in step 5. This causes southbridge
registers to be reverted before the EC's shutdown routine is called,
potentially making the EC unreachable.

The attached patch addresses this by using programmer shutdown functions the
same as any other shutdown callback. The internal_shutdown function is
registered early in the internal_init() function so that newly discovered
external programmers (ie SuperIO/EC LPC -> SPI bridges) can be discovered
and have their shutdown callbacks added in the correct order.

If this seems useful, we should make necessary changes to other programmers
as well before committing this patch.

Signed-off-by: David Hendricks <[e-mail]>

-- 
David Hendricks (dhendrix)
Systems Software Engineer, Google Inc.

[David Hendricks]

Second try, with the patch actually attached this time.

Signed-off-by: David Hendricks <[e-mail]>

On Thu, Apr 21, 2011 at 9:28 PM, David Hendricks <[e-mail]>wrote:
> Hi everyone,
> Lately we've been looking into reverse PCI<http://flashrom.org/trac/flashrom/changeset/1232>and
> MMIO <http://patchwork.coreboot.org/patch/2331/> writes, along with the
> shutdown callback mechanism.
>
> The main usage case for Flashrom makes this a pretty simple matter, since
> most users only worry about a single programmer being used. However, when
> external devices are used which depend on internal programmer settings,
> things can get messy.
>
> Consider the case of a laptop with an x86 southbridge and an EC -- To
> communicate with the EC thru the LPC/FWH interface, some register settings
> in the southbridge might need to be changed. The code flow should be:
> 1. Set up SB, use rpci_* and rmmio_* to register reverse PCI and MMIO
> callbacks wherever necessary.
> 2. Set up EC.
> 3. Run code.
> 4. Call EC shutdown routine.
> 5. Reverse SB PCI/MMIO writes.
>
> However, the current code calls the programmer shutdown routine in step 4
> *after* doing all shutdown callbacks in step 5. This causes southbridge
> registers to be reverted before the EC's shutdown routine is called,
> potentially making the EC unreachable.
>
> The attached patch addresses this by using programmer shutdown functions
> the same as any other shutdown callback. The internal_shutdown function is
> registered early in the internal_init() function so that newly discovered
> external programmers (ie SuperIO/EC LPC -> SPI bridges) can be discovered
> and have their shutdown callbacks added in the correct order.
>
> If this seems useful, we should make necessary changes to other programmers
> as well before committing this patch.
>
> Signed-off-by: David Hendricks <[e-mail]>

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

[Carl-Daniel Hailfinger]

Am 22.04.2011 06:31 schrieb David Hendricks:
> Index: internal.c
> ===================================================================
> --- internal.c	(revision 1288)
> +++ internal.c	(working copy)
> @@ -164,6 +164,7 @@
>   }
>   free(arg);
>
> +	register_shutdown(internal_shutdown, NULL);
>   get_io_perms();

Wrong order. We want to register the shutdown function only after we got 
IO permissions. That said, should get_io_perms automatically register 
release_io_perms? This would kill a few useless shutdown functions.
> /* Initialize PCI access for flash enables */
> @@ -264,11 +265,9 @@
>   #endif
>   }
>
> -int internal_shutdown(void)
> +void internal_shutdown(void *data)
>   {
>   release_io_perms();
> -
> -	return 0;
>   }
>   #endif
>
> Index: it85spi.c
> ===================================================================
> --- it85spi.c	(revision 1288)
> +++ it85spi.c	(working copy)
> @@ -80,6 +80,8 @@
>   #define INDIRECT_WRITE(base, value) OUTB(value, (base) + 4)
>   #endif  /* LPC_IO */
>
> +void it85xx_shutdown(void *);
> +
>   #ifdef LPC_IO
>   unsigned int shm_io_base;
>   #endif
> @@ -308,6 +310,7 @@
>   /* Set this as spi controller. */
>   spi_controller = SPI_CONTROLLER_IT85XX;
>
> +	register_shutdown(it85xx_shutdown, NULL);
>   return 0;
>   }
>
> @@ -349,11 +352,10 @@
>   return ret;
>   }
>
> -int it85xx_shutdown(void)
> +void it85xx_shutdown(void *data)
>   {
>   msg_pdbg("%s():%d\n", __func__, __LINE__);
>   it85xx_exit_scratch_rom();
> -	return 0;
>   }
>
> /* According to ITE 8502 document, the procedure to follow mode is following:

It would be great if you could forward-port this patch to the current 
IT85* SPI code.
> Index: flashrom.c
> ===================================================================
> --- flashrom.c	(revision 1288)
> +++ flashrom.c	(working copy)
> @@ -126,7 +126,8 @@
>   {
>       .name			= "internal",
>       .init			= internal_init,
> -		.shutdown		= internal_shutdown,
> +		/* called implicitly using shutdown callback */
> +//		.shutdown		= internal_shutdown,
>       .map_flash_region	= physmap,
>       .unmap_flash_region	= physunmap,
>       .chip_readb		= internal_chip_readb,

The same for all other programmers, please.
And kill the .shutdown struct member everywhere (even in the header 
file) while you're at it.
> @@ -543,7 +544,7 @@
>   return ret;
>   }
>
> -int programmer_shutdown(void)
> +int flashrom_shutdown(void)
>   {
>   /* Registering shutdown functions is no longer allowed. */
>   may_register_shutdown = 0;
> @@ -551,7 +552,7 @@
>       int i = --shutdown_fn_count;
>       shutdown_fn[i].func(shutdown_fn[i].data);

We should care about the result of the shutdown function, even if it is 
only for printing diagnostic messages.
> }
> -	return programmer_table[programmer].shutdown();
> +	return 0;

And return the error code of the failing shutdown function (if one 
failed), zero (if none failed) or some arbitrary value if more than one 
shutdown function failed.
> }
>
> void *programmer_map_flash_region(const char *descr, unsigned long phys_addr,
> @@ -1939,6 +1940,6 @@
>   free(oldcontents);
>   free(newcontents);
>   out_nofree:
> -	programmer_shutdown();
> +	flashrom_shutdown();
>   return ret;
>   }
> Index: programmer.h
> ===================================================================
> --- programmer.h	(revision 1288)
> +++ programmer.h	(working copy)
> @@ -111,7 +111,7 @@
>   extern const struct programmer_entry programmer_table[];
>
> int programmer_init(char *param);
> -int programmer_shutdown(void);
> +int flashrom_shutdown(void);

Given that this is still a programmer shutdown (as opposed to a flash 
chip shutdown which may be necessary in the future), I'd like to keep 
the name.

Overall, I like this patch. If you send a variant which converts all 
programmers and addresses the comments, I'll ack.

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/

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

[Stefan Reinauer]

On Wed, Apr 27, 2011 at 7:56 AM, Carl-Daniel Hailfinger
<[e-mail]> wrote:
> Am 22.04.2011 06:31 schrieb David Hendricks:
> > Index: internal.c
> > ===================================================================
> > --- internal.c  (revision 1288)
> > +++ internal.c  (working copy)
> > @@ -164,6 +164,7 @@
> >        }
> >        free(arg);
> >
> > +       register_shutdown(internal_shutdown, NULL);
> >        get_io_perms();
>
> Wrong order. We want to register the shutdown function only after we got IO permissions. That said, should get_io_perms automatically register release_io_perms? This would kill a few useless shutdown functions.

I like this idea of everybody cleaning up after themselfes.
> Index: it85spi.c
> ===================================================================
> --- it85spi.c   (revision 1288)
> +++ it85spi.c   (working copy)
> @@ -80,6 +80,8 @@
>  #define INDIRECT_WRITE(base, value) OUTB(value, (base) + 4)
>  #endif  /* LPC_IO */
>
> +void it85xx_shutdown(void *);
> +

This should go away, rather make the function static instead.
>  #ifdef LPC_IO
>  unsigned int shm_io_base;
>  #endif
> @@ -308,6 +310,7 @@
>        /* Set this as spi controller. */
>        spi_controller = SPI_CONTROLLER_IT85XX;
>
> +       register_shutdown(it85xx_shutdown, NULL);
>        return 0;
>  }
>
> @@ -349,11 +352,10 @@
>        return ret;
>  }
>
> -int it85xx_shutdown(void)
> +void it85xx_shutdown(void *data)
>  {
>        msg_pdbg("%s():%d\n", __func__, __LINE__);
>        it85xx_exit_scratch_rom();
> -       return 0;
>  }

And move it above the register_... function.

--
Stefan Reinauer
Google Inc.

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

[David Hendricks]

Thanks for the comments! I tend to agree with them all. FWIW, we've been
testing the basic ideas here in the Chromium OS version and it seems to work
pretty well. So at least I think we're on the right path.

A few details to address before making broader changes (and potentially
making a huge mess of things)....

On Wed, Apr 27, 2011 at 7:56 AM, Carl-Daniel Hailfinger <
[e-mail]> wrote:
> Am 22.04.2011 06:31 schrieb David Hendricks:
> > Index: internal.c
> > ===================================================================
> > --- internal.c  (revision 1288)
> > +++ internal.c  (working copy)
> > @@ -164,6 +164,7 @@
> >        }
> >        free(arg);
> >
> > +       register_shutdown(internal_shutdown, NULL);
> >        get_io_perms();
>
> Wrong order. We want to register the shutdown function only after we got IO
> permissions. That said, should get_io_perms automatically register
> release_io_perms? This would kill a few useless shutdown functions.

Fixed the ordering, and I agree that get_io_perms should register
release_io_perms.

On Wed, Apr 27, 2011 at 7:56 AM, Carl-Daniel Hailfinger <
[e-mail]> wrote:
> It would be great if you could forward-port this patch to the current IT85*
> > SPI code.

Done. Also fixed the prototype thing Stefan pointed out.

On Wed, Apr 27, 2011 at 7:56 AM, Carl-Daniel Hailfinger <
[e-mail]> wrote:
> Index: flashrom.c
> > ===================================================================
> > --- flashrom.c  (revision 1288)
> > +++ flashrom.c  (working copy)
> > @@ -126,7 +126,8 @@
> >        {
> >                .name                   = "internal",
> >                .init                   = internal_init,
> > -               .shutdown               = internal_shutdown,
> > +               /* called implicitly using shutdown callback */
> > +//             .shutdown               = internal_shutdown,
> >                .map_flash_region       = physmap,
> >                .unmap_flash_region     = physunmap,
> >                .chip_readb             = internal_chip_readb,
>
> The same for all other programmers, please.
> And kill the .shutdown struct member everywhere (even in the header file)
> while you're at it.

After giving this some thought, I think getting rid of .shutdown struct
members might be premature. Perhaps they can be useful for something like
register_shutdown(programmer.shutdown, data). Also, the structure looks a
bit awkward without a shutdown member, even if it is superfluous.

If you still feel we should get rid of the .shutdown member, that's fine
too. Otherwise we can just change the structure member so that it matches
the function signature that register_shutdown() expects.

On Wed, Apr 27, 2011 at 7:56 AM, Carl-Daniel Hailfinger <
[e-mail]> wrote:
> @@ -543,7 +544,7 @@
> > return ret;
> >  }
> >
> > -int programmer_shutdown(void)
> > +int flashrom_shutdown(void)
> >  {
> >        /* Registering shutdown functions is no longer allowed. */
> >        may_register_shutdown = 0;
> > @@ -551,7 +552,7 @@
> >                int i = --shutdown_fn_count;
> >                shutdown_fn[i].func(shutdown_fn[i].data);
>
> We should care about the result of the shutdown function, even if it is
> only for printing diagnostic messages.

In that case, the shutdown callbacks should return an int. Right now they
return void. Should we go ahead and update the return type for shutdown
functions?

On Wed, Apr 27, 2011 at 7:56 AM, Carl-Daniel Hailfinger <
[e-mail]> wrote:
> }
> > -       return programmer_table[programmer].shutdown();
> > +       return 0;
>
> And return the error code of the failing shutdown function (if one failed),
> zero (if none failed) or some arbitrary value if more than one shutdown
> function failed.

That would be much more useful, but requires the above change.

On Wed, Apr 27, 2011 at 7:56 AM, Carl-Daniel Hailfinger <
[e-mail]> wrote:
> Index: programmer.h
> > ===================================================================
> > --- programmer.h        (revision 1288)
> > +++ programmer.h        (working copy)
> > @@ -111,7 +111,7 @@
> >  extern const struct programmer_entry programmer_table[];
> >
> > int programmer_init(char *param);
> > -int programmer_shutdown(void);
> > +int flashrom_shutdown(void);
>
> Given that this is still a programmer shutdown (as opposed to a flash chip
> shutdown which may be necessary in the future), I'd like to keep the name.

Agreed.

-- 
David Hendricks (dhendrix)
Systems Software Engineer, Google Inc.

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

[Carl-Daniel Hailfinger]

Am 03.05.2011 02:08 schrieb David Hendricks:
> Thanks for the comments! I tend to agree with them all. FWIW, we've been
> testing the basic ideas here in the Chromium OS version and it seems to work
> pretty well. So at least I think we're on the right path.

Agreed.
> On Wed, Apr 27, 2011 at 7:56 AM, Carl-Daniel Hailfinger wrote:
> > should get_io_perms automatically register
> > release_io_perms? This would kill a few useless shutdown functions.
>
> I agree that get_io_perms should register
> release_io_perms.

The more I think about it, the more I wonder if we really should do 
that. If we decide to do it, a failed programmer_init() must be followed 
by a programmer_shutdown() call because there is no way to unregister a 
function (e.g. release_io_perms).
Can you leave out the release_io_perms automatic registration for now 
until we have discussed this and found a model which makes everyone happy?
> On Wed, Apr 27, 2011 at 7:56 AM, Carl-Daniel Hailfinger wrote:
> > Index: flashrom.c
> > > ===================================================================
> > > --- flashrom.c  (revision 1288)
> > > +++ flashrom.c  (working copy)
> > > @@ -126,7 +126,8 @@
> > >         {
> > >                 .name                   = "internal",
> > >                 .init                   = internal_init,
> > > -               .shutdown               = internal_shutdown,
> > > +               /* called implicitly using shutdown callback */
> > > +//             .shutdown               = internal_shutdown,
> > >                 .map_flash_region       = physmap,
> > >                 .unmap_flash_region     = physunmap,
> > >                 .chip_readb             = internal_chip_readb,
> >
> > The same for all other programmers, please.
> > And kill the .shutdown struct member everywhere (even in the header file)
> > while you're at it.
>
> After giving this some thought, I think getting rid of .shutdown struct
> members might be premature. Perhaps they can be useful for something like
> register_shutdown(programmer.shutdown, data).

AFAICS that would lead to ordering problems for shutdown.
> Also, the structure looks a
> bit awkward without a shutdown member, even if it is superfluous.

Right, that part is a bit awkward. That said, if we register the 
shutdown functions properly, .shutdown should never need to be defined.
> If you still feel we should get rid of the .shutdown member, that's fine
> too. Otherwise we can just change the structure member so that it matches
> the function signature that register_shutdown() expects.

Can you think of a technical reason where 
register_shutdown(somefunction) would not be sufficient? If not, I'd say 
we kill that structure member and reintroduce it only if we stumble over 
some case where it is needed.
> On Wed, Apr 27, 2011 at 7:56 AM, Carl-Daniel Hailfinger wrote:
> > @@ -543,7 +544,7 @@
> > > return ret;
> > >   }
> > >
> > > -int programmer_shutdown(void)
> > > +int flashrom_shutdown(void)
> > >   {
> > >         /* Registering shutdown functions is no longer allowed. */
> > >         may_register_shutdown = 0;
> > > @@ -551,7 +552,7 @@
> > >                 int i = --shutdown_fn_count;
> > >                 shutdown_fn[i].func(shutdown_fn[i].data);
> >
> > We should care about the result of the shutdown function, even if it is
> > only for printing diagnostic messages.
>
> In that case, the shutdown callbacks should return an int. Right now they
> return void. Should we go ahead and update the return type for shutdown
> functions?

Please do.

This is especially useful for libflashrom. My plan is to allow multiple 
sequences of
programmer_init(); do_stuff(); programmer_shutdown();
for different (or identical) programmers without quitting flashrom in 
between.
Assuming that init functions clean up after themselves, the only reason 
to refuse another init is a failed shutdown in the previous cycle.
(Side note: Programmer parameter parsing will need minor changes to work 
as intended once we support such a style of operation.)
> On Wed, Apr 27, 2011 at 7:56 AM, Carl-Daniel Hailfinger wrote
> > > -       return programmer_table[programmer].shutdown();
> > > +       return 0;
> >
> > And return the error code of the failing shutdown function (if one failed),
> > zero (if none failed) or some arbitrary value if more than one shutdown
> > function failed.
>
> That would be much more useful, but requires the above change.

Go ahead.

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/

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

[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.
- programmer_shutdown() checks the return code of all shutdown callbacks.

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.
- nicintel_spi -- Looks like flash write enable/disable should be done using
rpci?
- drkaiser - shutdown function missing a physunmap?

Additional notes:
- Most programmers have a simple init routines with no failure condition, so
placement of register_shutdown() doesn't really matter.
- serialport_shutdown non-static since it's used by buspirate.
- 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.

I did a quick test using an NM10/ICH7 platform and callbacks were shown in
verbose mode output as expected.
Signed-off-by: David Hendricks <[e-mail]>

-- 
David Hendricks (dhendrix)
Systems Software Engineer, Google Inc.

[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 <[e-mail]>

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

-- 
http://www.hailfinger.org/

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

[David Hendricks]

As always, thanks for the thorough review! Comments in-line, and a revised
patch is attached.

New patch is:
Signed-off-by: David Hendricks <[e-mail]>

On Thu, Jun 9, 2011 at 5:33 PM, Carl-Daniel Hailfinger <
[e-mail]> wrote:
> > - 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.

That sounds more like a job for higher-level logic. Currently, none of the
callers of programmer_shutdown() bother to check its return code.

Also, the callers of register_shutdown() check return code, which will fail
if programmer_shutdown() is used beforehand since may_register_shutdown will
be set to 0. Hmmmm...

On Thu, Jun 9, 2011 at 5:33 PM, Carl-Daniel Hailfinger <
[e-mail]> wrote:
> > 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.

Done.

On Thu, Jun 9, 2011 at 5:33 PM, Carl-Daniel Hailfinger <
[e-mail]> wrote:
> > - 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.

I didn't notice other obvious cases where this applied. Implicit unmapping
(or maybe rphysmap()?) sounds like it might be good in the long-run. That,
in conjunction with rpci_* and rmmio_* functions, could certainly help to
make shutdown functions less prone to error w.r.t. ordering.

On Thu, Jun 9, 2011 at 5:33 PM, Carl-Daniel Hailfinger <
[e-mail]> wrote:
> 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.

Yeah, that one was quite a bit more painful than the others. Maybe I re-do
serprog using a forward declaration for serprog_shutdown() and leave the
rest as is? We can remove the forward declaration and move the code in a
follow-up patch to reduce diffs in this one.

On Thu, Jun 9, 2011 at 5:33 PM, Carl-Daniel Hailfinger <
[e-mail]> wrote:
> 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.

Nice catch, though I only found it once in (nicnatsemi).

On Thu, Jun 9, 2011 at 5:33 PM, Carl-Daniel Hailfinger <
[e-mail]> wrote:
> 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...

Someone (Stefan?) requested that I move the code in an earlier revision. I
think the only real pain here is from serprog due to the large amount of
code that shifted by a few lines.

On Thu, Jun 9, 2011 at 5:33 PM, Carl-Daniel Hailfinger <
[e-mail]> wrote:
> > 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.

Done.

On Thu, Jun 9, 2011 at 5:33 PM, Carl-Daniel Hailfinger <
[e-mail]> wrote:
> > 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.

Fixed. Give it one more glance to make sure I put the physunmap() in a
sensible place as well. I assumed it should be done before pci_cleanup().

On Thu, Jun 9, 2011 at 5:33 PM, Carl-Daniel Hailfinger <
[e-mail]> wrote:
> > 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.

Fixed. I also added free(flashchip_contents) in case shutdown callback
registration fails.

On Thu, Jun 9, 2011 at 5:33 PM, Carl-Daniel Hailfinger <
[e-mail]> wrote:
> > 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;

Fixed. It was only intended as a shortcut to save 2 lines since we're ending
the function in this case.

On Thu, Jun 9, 2011 at 5:33 PM, Carl-Daniel Hailfinger <
[e-mail]> wrote:
> > 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.

I added a fix for that, and moved register_shutdown() below the physmap call
in the init function as per your other comment.

On Thu, Jun 9, 2011 at 5:33 PM, Carl-Daniel Hailfinger <
[e-mail]> wrote:
> > 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.

Good point, but I don't think we should try to solve it in this patch. The
way it's coded now at least preserves the current behavior which doesn't
check return code at all. We can address the issue later when we have more
concrete use cases.

-- 
David Hendricks (dhendrix)
Systems Software Engineer, Google Inc.