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

[David Hendricks]

On Mon, Jun 13, 2011 at 5:13 PM, Carl-Daniel Hailfinger <
c-d.hailfinger.devel.2006 at gmx.net> wrote:

> Am 14.06.2011 00:45 schrieb David Hendricks:
> > Thanks again for the additional comments. I made the changes (comments
> > below) and re-compiled with the extra programmers enabled.
> >
> > New version of the patch attached.
> > Signed-off-by: David Hendricks <dhendrix at google.com>
> >
>
> Thanks, looks really good.
>
>
> > On Sat, Jun 11, 2011 at 5:09 AM, Carl-Daniel Hailfinger wrote:
> >
> >> nicintel_spi has the register_shutdown too early.
> >> nicintel will not unmap the nicintel_bar if mapping nicintel_control_bar
> >> fails (error case handling needs a second label which unmaps
> nicintel_bar).
> >>
> > Good catch -- I moved the register_shutdown call toward the bottom of the
> > function, but before the bitbang_spi_init call so that failure in
> > bitbang_spi_init will not prevent nicintel_spi_shutdown from being
> called.
> >
>
> Not exactly what I meant. I'll try to explain it better with a diff on
> top of your current diff. Apply that, and we're good to go:
>
> > --- nicintel.c~       2011-06-14 01:58:17.000000000 +0200
> > +++ nicintel.c        2011-06-14 01:59:57.000000000 +0200
> > @@ -77,7 +77,7 @@
> >       nicintel_control_bar = physmap("Intel NIC control/status reg",
> >                                      addr, NICINTEL_CONTROL_MEMMAP_SIZE);
> >       if (nicintel_control_bar == ERROR_PTR)
> > -             goto error_out;
> > +             goto error_out_unmap;
> >
> >       if (register_shutdown(nicintel_shutdown, NULL))
> >               return 1;
> > @@ -99,6 +99,8 @@
> >
> >       return 0;
> >
> > +error_out_unmap:
> > +     physunmap(nicintel_bar, NICINTEL_MEMMAP_SIZE);
> >  error_out:
> >       pci_cleanup(pacc);
> >       release_io_perms();
> >
>

D'oh, I should've read your comment more closely. I've added your changes.


>
>
> One comment about satasii.c:
> > Index: satasii.c
> > ===================================================================
> > --- satasii.c (revision 1327)
> > +++ satasii.c (working copy)
> > @@ -59,7 +69,8 @@
> >               reg_offset = 0x50;
> >       }
> >
> > -     sii_bar = physmap("SATA SIL registers", addr, 0x100) + reg_offset;
> > +     sii_bar = reg_offset +
> > +               physmap("SATA SIL registers", addr, SATASII_MEMMAP_SIZE);
> >
>
> Could you keep the old ordering? AFAIK the usual way to specify a
> location is base+offset, not the other way round.
>

Done.


>
>
> > Index: flashrom.c
> > ===================================================================
> > --- flashrom.c        (revision 1327)
> > +++ flashrom.c        (working copy)
> > @@ -543,13 +525,15 @@
> >
> >  int programmer_shutdown(void)
> >  {
> > +     int rc = 0;
> >
>
> int ret please.
>

Done.


>
>
> > +
> >       /* 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);
> >       }
> > -     return programmer_table[programmer].shutdown();
> > +     return rc;
> >  }
> >
> >  void *programmer_map_flash_region(const char *descr, unsigned long
> phys_addr,
> >
>
>
> With those minor changes, the patch is
> Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>
>
> Please go ahead and commit.
>

Thanks, committed in r1338.

-- 
David Hendricks (dhendrix)
Systems Software Engineer, Google Inc.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.flashrom.org/pipermail/flashrom/attachments/20110613/df606261/attachment.html>