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

David Hendricks dhendrix at google.com
Fri Jun 10 23:55:12 CEST 2011


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

New patch is:
Signed-off-by: David Hendricks <dhendrix at google.com>

On Thu, Jun 9, 2011 at 5:33 PM, Carl-Daniel Hailfinger <
c-d.hailfinger.devel.2006 at gmx.net> 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 <
c-d.hailfinger.devel.2006 at gmx.net> 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 <
c-d.hailfinger.devel.2006 at gmx.net> 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 <
c-d.hailfinger.devel.2006 at gmx.net> 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 <
c-d.hailfinger.devel.2006 at gmx.net> 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 <
c-d.hailfinger.devel.2006 at gmx.net> 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 <
c-d.hailfinger.devel.2006 at gmx.net> 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 <
c-d.hailfinger.devel.2006 at gmx.net> 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 <
c-d.hailfinger.devel.2006 at gmx.net> 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 <
c-d.hailfinger.devel.2006 at gmx.net> 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 <
c-d.hailfinger.devel.2006 at gmx.net> 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 <
c-d.hailfinger.devel.2006 at gmx.net> 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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.flashrom.org/pipermail/flashrom/attachments/20110610/03826ffe/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: call_programmer_shutdown_using_callbacks.patch
Type: text/x-patch
Size: 43942 bytes
Desc: not available
URL: <http://www.flashrom.org/pipermail/flashrom/attachments/20110610/03826ffe/attachment.patch>


More information about the flashrom mailing list