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

[David Hendricks]

Thanks again for the additional comments. I made the changes (comments
below) and re-compiled with the extra programmers enabled.

New version of the patch attached.
Signed-off-by: David Hendricks <dhendrix at google.com>

On Sat, Jun 11, 2011 at 5:09 AM, Carl-Daniel Hailfinger <
c-d.hailfinger.devel.2006 at gmx.net> wrote:

> 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 at 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
>

eek--i forgot there are targets being changed that are not included in the
compilation by default. Basically I just forgot to update the
nicnatsemi_shutdown() function to take a void *. This was a change from the
second iteration, IIRC.

There was also a typo in dediprog.c that caused a compilation failure on my
machine (maybe only a warning on others).

On Sat, Jun 11, 2011 at 5:09 AM, Carl-Daniel Hailfinger <
c-d.hailfinger.devel.2006 at gmx.net> wrote:

> > 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.
>

Done. We now see that the changes to serprog.c are almost trivial. The
insertion of register_shutdown() is done after some command parsing. The
shutdown function is pretty simple; I don't think there's much to think
about in terms of ordering dependency.

On Sat, Jun 11, 2011 at 5:09 AM, Carl-Daniel Hailfinger <
c-d.hailfinger.devel.2006 at gmx.net> wrote:

> Other comments:
> drkaiser and satasii are missing physunmap in shutdown.
>

Fixed.

On Sat, Jun 11, 2011 at 5:09 AM, Carl-Daniel Hailfinger <
c-d.hailfinger.devel.2006 at gmx.net> wrote:

> 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).
>

Good catch -- I moved the register_shutdown call toward the bottom of the
function, but before the bitbang_spi_init call so that failure in
bitbang_spi_init will not prevent nicintel_spi_shutdown from being called.

-- 
David Hendricks (dhendrix)
Systems Software Engineer, Google Inc.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.flashrom.org/pipermail/flashrom/attachments/20110613/f9645ca6/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: call_programmer_shutdown_using_callbacks.patch
Type: text/x-patch
Size: 34379 bytes
Desc: not available
URL: <http://www.flashrom.org/pipermail/flashrom/attachments/20110613/f9645ca6/attachment.patch>