<div class="gmail_quote">As always, thanks for the thorough review! Comments in-line, and a revised patch is attached.</div><div class="gmail_quote"><br></div><div class="gmail_quote">New patch is:</div><div class="gmail_quote">
Signed-off-by: David Hendricks <<a href="mailto:dhendrix@google.com">dhendrix@google.com</a>></div><div class="gmail_quote"><br></div><div class="gmail_quote">On Thu, Jun 9, 2011 at 5:33 PM, Carl-Daniel Hailfinger <span dir="ltr"><<a href="mailto:c-d.hailfinger.devel.2006@gmx.net">c-d.hailfinger.devel.2006@gmx.net</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><div class="im">> - programmer_shutdown() checks the return code of all shutdown callbacks.<br>
><br>
<br>
</div>I'm not 100% sure about this one. If we plan to make libflashrom viable<br>
in cases where various programmers are initialized more than once (of<br>
course with programmer shutdown in between), this is essential to refuse<br>
further programmer init calls once an init/shutdown screwed up. OTOH, we<br>
have no libflashrom user for now.<br></blockquote><div><br></div><div>That sounds more like a job for higher-level logic. Currently, none of the callers of programmer_shutdown() bother to check its return code.</div><div>
<br></div><div>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...</div><div><br></div><div><meta http-equiv="content-type" content="text/html; charset=utf-8">On Thu, Jun 9, 2011 at 5:33 PM, Carl-Daniel Hailfinger <span dir="ltr"><<a href="mailto:c-d.hailfinger.devel.2006@gmx.net">c-d.hailfinger.devel.2006@gmx.net</a>></span> wrote:</div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><div class="im">
> A few minor questions:<br>
> - If programmer_init() fails when called in cli_classic(), should we call<br>
> programmer_shutdown()? As Carl-Daniel noted earlier, this could potentially<br>
> be used to do stuff like release_io_perms(), or perhaps free resources<br>
> allocated by an init routine that fails.<br>
><br>
<br>
</div>I think so, yes. Especially for libflashrom.<br></blockquote><div><br></div><div>Done.</div><div><br></div><meta http-equiv="content-type" content="text/html; charset=utf-8"><div>On Thu, Jun 9, 2011 at 5:33 PM, Carl-Daniel Hailfinger <span dir="ltr"><<a href="mailto:c-d.hailfinger.devel.2006@gmx.net">c-d.hailfinger.devel.2006@gmx.net</a>></span> wrote: </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div class="im">> - drkaiser - shutdown function missing a physunmap?</div><div class="im">
><br>
<br>
</div>Nice find! Same for gfxnvidia.<br>
<br>
I wonder if this applies to the various internal chipset functions as<br>
well... and if we should eventually move towards implicit unmap<br>
registration. If we decide to do that implicit registration, we should<br>
create a separate patch for it to keep this patch reviewable.<br></blockquote><div><br></div><div>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.</div>
<div><br></div><div><meta http-equiv="content-type" content="text/html; charset=utf-8">On Thu, Jun 9, 2011 at 5:33 PM, Carl-Daniel Hailfinger <span dir="ltr"><<a href="mailto:c-d.hailfinger.devel.2006@gmx.net">c-d.hailfinger.devel.2006@gmx.net</a>></span> wrote: </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div class="im">To be honest, I don't know serprog well enough to say something useful</div>
about it. And the massive code move in serprog makes review a bit hard.<br></blockquote><div><br></div><div>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.</div>
<meta http-equiv="content-type" content="text/html; charset=utf-8"><div><br></div><meta http-equiv="content-type" content="text/html; charset=utf-8"><div>On Thu, Jun 9, 2011 at 5:33 PM, Carl-Daniel Hailfinger <span dir="ltr"><<a href="mailto:c-d.hailfinger.devel.2006@gmx.net">c-d.hailfinger.devel.2006@gmx.net</a>></span> wrote: </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">IIRC some of your error returns were inconsistent: return -1 vs. return<br>
1. I can't find the offender right now, but please recheck that.<br></blockquote><div><br></div><div>Nice catch, though I only found it once in (nicnatsemi).</div><div><br></div><meta http-equiv="content-type" content="text/html; charset=utf-8"><div>
On Thu, Jun 9, 2011 at 5:33 PM, Carl-Daniel Hailfinger <span dir="ltr"><<a href="mailto:c-d.hailfinger.devel.2006@gmx.net">c-d.hailfinger.devel.2006@gmx.net</a>></span> wrote: </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div class="im">A general comment first... I expect some more code changes in programmer</div>
init (other pending patches for programmer init), and I wonder if it<br>
would make sense to keep the shutdown functions in the code where they<br>
are, and just add forward declarations. It would definitely make the<br>
patch easier to review, but it might also help with code history (svn<br>
blame). OTOH forward declarations are ugly...<br></blockquote><div><br></div><div>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.</div>
<div><br></div><meta http-equiv="content-type" content="text/html; charset=utf-8"><div>On Thu, Jun 9, 2011 at 5:33 PM, Carl-Daniel Hailfinger <span dir="ltr"><<a href="mailto:c-d.hailfinger.devel.2006@gmx.net">c-d.hailfinger.devel.2006@gmx.net</a>></span> wrote: </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">> int ogp_spi_init(void)<br>
> {<br>
> char *type;<br>
> @@ -120,6 +129,9 @@<br>
><br>
> get_io_perms();<br>
><br>
> + if (register_shutdown(ogp_spi_shutdown, NULL))<br>
><br>
<br>
Move it after the physmap call or you'll unmap an uninitialized address.<br></blockquote><div><br></div><div>Done.</div><div> </div><div><meta http-equiv="content-type" content="text/html; charset=utf-8">On Thu, Jun 9, 2011 at 5:33 PM, Carl-Daniel Hailfinger <span dir="ltr"><<a href="mailto:c-d.hailfinger.devel.2006@gmx.net">c-d.hailfinger.devel.2006@gmx.net</a>></span> wrote: </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">> Index: gfxnvidia.c<br>
> ===================================================================<br>
> --- gfxnvidia.c (revision 1299)<br>
> +++ gfxnvidia.c (working copy)<br>
> @@ -60,12 +60,26 @@<br>
> {},<br>
> };<br>
><br>
> +static int gfxnvidia_shutdown(void *data)<br>
> +{<br>
> + /* Flash interface access is disabled (and screen enabled) automatically<br>
> + * by PCI restore.<br>
> + */<br>
> + pci_cleanup(pacc);<br>
> + release_io_perms();<br>
> + return 0;<br>
> +}<br>
> +<br>
> int gfxnvidia_init(void)<br>
> {<br>
> uint32_t reg32;<br>
><br>
> get_io_perms();<br>
><br>
> + /* must be done before rpci calls */<br>
> + if (register_shutdown(gfxnvidia_shutdown, NULL))<br>
><br>
<br>
If you change shutdown to include unmap, you'll have to move the<br>
register_shutdown after physmap, and that's unfortunately after rpci...<br>
boom! Can you reorder this a bit to call physmap before using rpci? That<br>
would make sense anyway because we don't want to keep the screen<br>
disabled in case physmap fails.<br></blockquote><div><br></div><div>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().</div><div>
</div><div><meta http-equiv="content-type" content="text/html; charset=utf-8">On Thu, Jun 9, 2011 at 5:33 PM, Carl-Daniel Hailfinger <span dir="ltr"><<a href="mailto:c-d.hailfinger.devel.2006@gmx.net">c-d.hailfinger.devel.2006@gmx.net</a>></span> wrote: </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">> int dummy_init(void)<br>
> {<br>
> char *bustext = NULL;<br>
> @@ -180,6 +197,11 @@<br>
> msg_perr("Out of memory!\n");<br>
> return 1;<br>
> }<br>
> +<br>
> + /* shutdown will free the flashchip contents */<br>
> + if (register_shutdown(dummy_shutdown, NULL))<br>
><br>
<br>
A bit earlier we already have a return 0 for the non-emulation case.<br>
This means register_shutdown has to happen there as well, or we replace<br>
the various return 0 with goto out (create an out: label at the end of<br>
the init function) and add register_shutdown to the end of the init<br>
function.<br></blockquote><div><br></div><div>Fixed. I also added free(flashchip_contents) in case shutdown callback registration fails.</div><div><br></div><div><meta http-equiv="content-type" content="text/html; charset=utf-8">On Thu, Jun 9, 2011 at 5:33 PM, Carl-Daniel Hailfinger <span dir="ltr"><<a href="mailto:c-d.hailfinger.devel.2006@gmx.net">c-d.hailfinger.devel.2006@gmx.net</a>></span> wrote:</div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">> int nic3com_init(void)<br>
> {<br>
> get_io_perms();<br>
> @@ -84,24 +99,9 @@<br>
> buses_supported = CHIP_BUSTYPE_PARALLEL;<br>
> max_rom_decode.parallel = 128 * 1024;<br>
><br>
> - return 0;<br>
> + return register_shutdown(nic3com_shutdown, NULL);<br>
><br>
<br>
I like the following idiom (which you used everywhere else) better:<br>
if (register_shutdown(...))<br>
return 1;<br>
return 0;<br></blockquote><div><br></div><div>Fixed. It was only intended as a shortcut to save 2 lines since we're ending the function in this case.</div><div><br></div><div><meta http-equiv="content-type" content="text/html; charset=utf-8">On Thu, Jun 9, 2011 at 5:33 PM, Carl-Daniel Hailfinger <span dir="ltr"><<a href="mailto:c-d.hailfinger.devel.2006@gmx.net">c-d.hailfinger.devel.2006@gmx.net</a>></span> wrote:</div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
> Index: nicintel.c<br>
> ===================================================================<br>
> --- nicintel.c (revision 1299)<br>
> +++ nicintel.c (working copy)<br>
> @@ -41,6 +41,14 @@<br>
><br>
> #define CSR_FCR 0x0c<br>
><br>
> +static int nicintel_shutdown(void *data)<br>
> +{<br>
> + physunmap(nicintel_bar, NICINTEL_MEMMAP_SIZE);<br>
><br>
<br>
physunmap(nicintel_control_bar) is missing.<br></blockquote><div><br></div><div>I added a fix for that, and moved register_shutdown() below the physmap call in the init function as per your other comment.</div><div><br></div>
<div><meta http-equiv="content-type" content="text/html; charset=utf-8">On Thu, Jun 9, 2011 at 5:33 PM, Carl-Daniel Hailfinger <span dir="ltr"><<a href="mailto:c-d.hailfinger.devel.2006@gmx.net">c-d.hailfinger.devel.2006@gmx.net</a>></span> wrote:</div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">> Index: flashrom.c<br>
> ===================================================================<br>
> --- flashrom.c (revision 1299)<br>
> +++ flashrom.c (working copy)<br>
> @@ -543,13 +525,15 @@<br>
><br>
> int programmer_shutdown(void)<br>
> {<br>
> + int rc = 0;<br>
<div class="im">> +<br>
> /* Registering shutdown functions is no longer allowed. */<br>
> may_register_shutdown = 0;<br>
</div>> while (shutdown_fn_count > 0) {<br>
<div class="im">> int i = --shutdown_fn_count;<br>
> - shutdown_fn[i].func(shutdown_fn[i].data);<br>
</div>> + rc |= shutdown_fn[i].func(shutdown_fn[i].data);<br>
><br>
<br>
That's a big question. Should we proceed even if one shutdown function<br>
failed, or should we stop? Both options make sense, and it's all about<br>
which one will screw the user less. Error cases suck if you're dealing<br>
with hardware.</blockquote><div><br></div><div>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.</div>
</div><br>-- <br>David Hendricks (dhendrix)<br>Systems Software Engineer, Google Inc.<br>