As always, thanks for the thorough review! Comments in-line, and a revised patch is attached.
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...
> 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.
> - 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.
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.
IIRC some of your error returns were inconsistent: return -1 vs. return
1. I can't find the offender right now, but please recheck that.
Nice catch, though I only found it once in (nicnatsemi).
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.
> 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.
> 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().
> 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.
> 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.
> 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.
> 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.