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

[David Hendricks]

Thanks for the comments! I tend to agree with them all. FWIW, we've been
testing the basic ideas here in the Chromium OS version and it seems to work
pretty well. So at least I think we're on the right path.

A few details to address before making broader changes (and potentially
making a huge mess of things)....

On Wed, Apr 27, 2011 at 7:56 AM, Carl-Daniel Hailfinger <
c-d.hailfinger.devel.2006 at gmx.net> wrote:

> Am 22.04.2011 06:31 schrieb David Hendricks:
>
>> Index: internal.c
>> ===================================================================
>> --- internal.c  (revision 1288)
>> +++ internal.c  (working copy)
>> @@ -164,6 +164,7 @@
>>        }
>>        free(arg);
>>
>> +       register_shutdown(internal_shutdown, NULL);
>>        get_io_perms();
>>
>>
>
> Wrong order. We want to register the shutdown function only after we got IO
> permissions. That said, should get_io_perms automatically register
> release_io_perms? This would kill a few useless shutdown functions.
>

Fixed the ordering, and I agree that get_io_perms should register
release_io_perms.

On Wed, Apr 27, 2011 at 7:56 AM, Carl-Daniel Hailfinger <
c-d.hailfinger.devel.2006 at gmx.net> wrote:

> It would be great if you could forward-port this patch to the current IT85*
>> SPI code.
>
>
Done. Also fixed the prototype thing Stefan pointed out.

On Wed, Apr 27, 2011 at 7:56 AM, Carl-Daniel Hailfinger <
c-d.hailfinger.devel.2006 at gmx.net> wrote:

> Index: flashrom.c
>> ===================================================================
>> --- flashrom.c  (revision 1288)
>> +++ flashrom.c  (working copy)
>> @@ -126,7 +126,8 @@
>>        {
>>                .name                   = "internal",
>>                .init                   = internal_init,
>> -               .shutdown               = internal_shutdown,
>> +               /* called implicitly using shutdown callback */
>> +//             .shutdown               = internal_shutdown,
>>                .map_flash_region       = physmap,
>>                .unmap_flash_region     = physunmap,
>>                .chip_readb             = internal_chip_readb,
>>
>>
>
> The same for all other programmers, please.
> And kill the .shutdown struct member everywhere (even in the header file)
> while you're at it.
>

After giving this some thought, I think getting rid of .shutdown struct
members might be premature. Perhaps they can be useful for something like
register_shutdown(programmer.shutdown, data). Also, the structure looks a
bit awkward without a shutdown member, even if it is superfluous.

If you still feel we should get rid of the .shutdown member, that's fine
too. Otherwise we can just change the structure member so that it matches
the function signature that register_shutdown() expects.

On Wed, Apr 27, 2011 at 7:56 AM, Carl-Daniel Hailfinger <
c-d.hailfinger.devel.2006 at gmx.net> wrote:

> @@ -543,7 +544,7 @@
>>        return ret;
>>  }
>>
>> -int programmer_shutdown(void)
>> +int flashrom_shutdown(void)
>>  {
>>        /* Registering shutdown functions is no longer allowed. */
>>        may_register_shutdown = 0;
>> @@ -551,7 +552,7 @@
>>                int i = --shutdown_fn_count;
>>                shutdown_fn[i].func(shutdown_fn[i].data);
>>
>>
>
> We should care about the result of the shutdown function, even if it is
> only for printing diagnostic messages.
>

In that case, the shutdown callbacks should return an int. Right now they
return void. Should we go ahead and update the return type for shutdown
functions?

On Wed, Apr 27, 2011 at 7:56 AM, Carl-Daniel Hailfinger <
c-d.hailfinger.devel.2006 at gmx.net> wrote:

>        }
>> -       return programmer_table[programmer].shutdown();
>> +       return 0;
>>
>>
>
> And return the error code of the failing shutdown function (if one failed),
> zero (if none failed) or some arbitrary value if more than one shutdown
> function failed.
>

That would be much more useful, but requires the above change.

On Wed, Apr 27, 2011 at 7:56 AM, Carl-Daniel Hailfinger <
c-d.hailfinger.devel.2006 at gmx.net> wrote:

> Index: programmer.h
>> ===================================================================
>> --- programmer.h        (revision 1288)
>> +++ programmer.h        (working copy)
>> @@ -111,7 +111,7 @@
>>  extern const struct programmer_entry programmer_table[];
>>
>>  int programmer_init(char *param);
>> -int programmer_shutdown(void);
>> +int flashrom_shutdown(void);
>>
>>
>
>
> Given that this is still a programmer shutdown (as opposed to a flash chip
> shutdown which may be necessary in the future), I'd like to keep the name.
>

Agreed.

-- 
David Hendricks (dhendrix)
Systems Software Engineer, Google Inc.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.flashrom.org/pipermail/flashrom/attachments/20110502/ddaf0484/attachment.html>