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

[Carl-Daniel Hailfinger]

Am 03.05.2011 02:08 schrieb 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.
>    

Agreed.


> On Wed, Apr 27, 2011 at 7:56 AM, Carl-Daniel Hailfinger wrote:
>    
>> should get_io_perms automatically register
>> release_io_perms? This would kill a few useless shutdown functions.
>>      
> I agree that get_io_perms should register
> release_io_perms.
>    

The more I think about it, the more I wonder if we really should do 
that. If we decide to do it, a failed programmer_init() must be followed 
by a programmer_shutdown() call because there is no way to unregister a 
function (e.g. release_io_perms).
Can you leave out the release_io_perms automatic registration for now 
until we have discussed this and found a model which makes everyone happy?


> On Wed, Apr 27, 2011 at 7:56 AM, Carl-Daniel Hailfinger 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).

AFAICS that would lead to ordering problems for shutdown.


> Also, the structure looks a
> bit awkward without a shutdown member, even if it is superfluous.
>    

Right, that part is a bit awkward. That said, if we register the 
shutdown functions properly, .shutdown should never need to be defined.


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

Can you think of a technical reason where 
register_shutdown(somefunction) would not be sufficient? If not, I'd say 
we kill that structure member and reintroduce it only if we stumble over 
some case where it is needed.


> On Wed, Apr 27, 2011 at 7:56 AM, Carl-Daniel Hailfinger 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?
>    


Please do.

This is especially useful for libflashrom. My plan is to allow multiple 
sequences of
programmer_init(); do_stuff(); programmer_shutdown();
for different (or identical) programmers without quitting flashrom in 
between.
Assuming that init functions clean up after themselves, the only reason 
to refuse another init is a failed shutdown in the previous cycle.
(Side note: Programmer parameter parsing will need minor changes to work 
as intended once we support such a style of operation.)


> On Wed, Apr 27, 2011 at 7:56 AM, Carl-Daniel Hailfinger 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.
>    

Go ahead.

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/