On Mon, Jun 13, 2011 at 5:13 PM, Carl-Daniel Hailfinger < [email protected]> 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 <[email protected]> > > > > 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 <[email protected]> > > Please go ahead and commit. > Thanks, committed in r1338. -- David Hendricks (dhendrix) Systems Software Engineer, Google Inc.
_______________________________________________ flashrom mailing list [email protected] http://www.flashrom.org/mailman/listinfo/flashrom
