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


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