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

[Carl-Daniel Hailfinger]

Am 10.06.2011 23:55 schrieb David Hendricks:
> As always, thanks for the thorough review! Comments in-line, and a revised
> patch is attached.
>
> New patch is:
> Signed-off-by: David Hendricks

Thanks, looks good. I found one compile issue:
nicnatsemi.c:61:24: error: incompatible pointer types passing 'int
(void)' to parameter of type 'int (*)(void *)'
        if (register_shutdown(nicnatsemi_shutdown, NULL))
                              ^~~~~~~~~~~~~~~~~~~
In file included from nicnatsemi.c:24:
./flash.h:43:29: note: passing argument to parameter 'function' here
int register_shutdown(int (*function) (void *data), void *data);
                            ^
1 error generated.
make: *** [nicnatsemi.o] Fehler 1

The compilation test was run like this:
make CONFIG_ATAHPT=yes CONFIG_NICNATSEMI=yes CONFIG_DEDIPROG=yes
CONFIG_PRINT_WIKI=yes
> On Thu, Jun 9, 2011 at 5:33 PM, Carl-Daniel Hailfinger 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...

I'd say we handle this in a followup patch.
> On Thu, Jun 9, 2011 at 5:33 PM, Carl-Daniel Hailfinger 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.

Indeed.
> On Thu, Jun 9, 2011 at 5:33 PM, Carl-Daniel Hailfinger 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.

Good idea.
> On Thu, Jun 9, 2011 at 5:33 PM, Carl-Daniel Hailfinger 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.

Hm yes.
> 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().

Looks good.
> On Thu, Jun 9, 2011 at 5:33 PM, Carl-Daniel Hailfinger <
> [e-mail]> 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.

Good catch.
> On Thu, Jun 9, 2011 at 5:33 PM, Carl-Daniel Hailfinger wrote:
> > > Index: flashrom.c
> > >
> > > --- flashrom.c        (revision 1299)
> > > +++ flashrom.c        (working copy)
> > > @@ -543,13 +525,15 @@
> > >
> > > int programmer_shutdown(void)
> >
> > 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.

Agreed.

Other comments:
drkaiser and satasii are missing physunmap in shutdown.
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).

This looks pretty good, I think the next iteration will get a straight ack.

Regards,
Carl-Daniel