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@google.com
On Thu, Jun 9, 2011 at 5:33 PM, Carl-Daniel Hailfinger < c-d.hailfinger.devel.2006@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@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@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@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@gmx.net> wrote:
IIRC some of your error returns were inconsistent: return -1 vs. return
- 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@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@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@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@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@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@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@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.