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