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

[Carl-Daniel Hailfinger]

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

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();

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.
> 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.
> /* 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

Please go ahead and commit.

Regards,
Carl-Daniel

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

[David Hendricks]

On Mon, Jun 13, 2011 at 5:13 PM, Carl-Daniel Hailfinger <
[e-mail]> 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
>
> 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
>
> Please go ahead and commit.

Thanks, committed in r1338.