New subject: [Patch] + RFC: Use shutdown callback mechanism to shutdown programmers

11 Jun 2011

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 dhendrix@google.com
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 <
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.
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
-- 
http://www.hailfinger.org/



    

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

Index: flashrom.c