Re: [flashrom] [PATCH] register_shutdown for execution on programmer shutdown

[Carl-Daniel Hailfinger]

On 13.02.2010 09:38, Michael Karcher wrote:
> I think if reading this form of typedef is difficult (which it is) for
> someone, reading the array declaration is even more difficult for that
> person.

Point taken.
> > However, if we use a struct, the typedef issue won't have a noticeable
> > impact anymore AFAICS.
>
> It does have: It makes the prototype of register_shutdown more readable.

Hm. We don't use typedefs for the functions in struct programmer and
struct flashchip either, so I somewhat expect people to be familiar with
this. That was one of the reasons why I avoided the typedef in the first
place. Anyway, the function now has a nice comment which should explain
how to use it and how to read the prototype.

Comments welcome.

Some programmers want to run certain functions during programmer
shutdown, but the function choice depends on the code path taken during
programmer init.
Rather than rebuilding the whole init logic in the shutdown function, it
is now possible to register functions for execution on programmer shutdown.
The behaviour is similar to atexit(), but the registered functions will
be run on programmer shutdown instead of on exit and the functions will
be called with a void * argument that is specified on registration.
Registered functions must have the prototype
void function(void *);
and will be executed in reverse registration order directly before
calling the programmer-specific shutdown() function. It is recommended
to have shutdown() only disable programmer/hardware access and leave all
code path sensitive shutdown to functions registered with
register_shutdown().
Consult man atexit to learn more about the motivation behind this.

Usage example (try running the dummy flasher with this patch applied):
> --- flashrom-register_shutdown/dummyflasher.c	(Revision 899)
> +++ flashrom-register_shutdown/dummyflasher.c	(Arbeitskopie)
> @@ -24,6 +24,18 @@
>  #include <sys/types.h>
>  #include "flash.h"
>
> +void foo(void *bla)
> +{
> msg_pinfo("This is %s and was added first, will be executed second. "
>   "The supplied pointer was %p.\n", __func__, bla);
>
> +}
>
> +void bar(void *bla)
> +{
> msg_pinfo("This is %s and was added second, will be executed first. "
>   "We don't care about the supplied pointer.\n", __func__);
>
> +}
>
> int dummy_init(void)
>  {
>    int i;
> @@ -61,6 +73,9 @@
>    if (buses_supported == CHIP_BUSTYPE_NONE)
>    	msg_pdbg("Support for all flash bus types disabled.\n");
>    free(programmer_param);
> register_shutdown(foo, (void *)0xdeadbeef);
> register_shutdown(bar, NULL);
> return 0;
>
> }

The most prominent use case is resetting the EC after flashing on
laptops. Anders, this should fit your requirements.

Note: There are quite a few code paths in flashrom which proceed to
terminate flashrom without any programmer shutdown. Those code paths
will not get the benefit of register_shutdown() and they should be
changed wherever possible.

Signed-off-by: Carl-Daniel Hailfinger

Index: flashrom-register_shutdown/flash.h
===================================================================
--- flashrom-register_shutdown/flash.h	(Revision 899)
+++ flashrom-register_shutdown/flash.h	(Arbeitskopie)
@@ -99,6 +99,13 @@

extern const struct programmer_entry programmer_table[];

+struct shutdown_func_data {
+	void (*func) (void *data);
+	void *data;
+};
+
+int register_shutdown(void (*function) (void *data), void *data);
+
 int programmer_init(void);
 int programmer_shutdown(void);
 void *programmer_map_flash_region(const char *descr, unsigned long phys_addr,
Index: flashrom-register_shutdown/flashrom.c
===================================================================
--- flashrom-register_shutdown/flashrom.c	(Revision 899)
+++ flashrom-register_shutdown/flashrom.c	(Arbeitskopie)
@@ -313,8 +313,38 @@
    return programmer_table[programmer].init();
 }

+#define SHUTDOWN_MAXFN 4
+static int shutdown_fn_count = 0;
+struct shutdown_func_data shutdown_fn[SHUTDOWN_MAXFN];
+
+/* Register a function to be executed on programmer shutdown.
+ * The advantage over atexit() is that you can supply a void pointer which will
+ * be used as parameter to the registered function upon programmer shutdown.
+ * This pointer can point to arbitrary data used by said function, e.g. undo
+ * information for GPIO settings etc. If unneeded, set data=NULL.
+ * Please note that the first (void *data) belongs to the function signature of
+ * the function passed as first parameter.
+ */
+int register_shutdown(void (*function) (void *data), void *data)
+{
+	if (shutdown_fn_count >= SHUTDOWN_MAXFN) {
+		msg_perr("Tried to register more than %n shutdown functions.\n",
+			 SHUTDOWN_MAXFN);
+		return 1;
+	}
+	shutdown_fn[shutdown_fn_count].func = function;
+	shutdown_fn[shutdown_fn_count].data = data;
+	shutdown_fn_count++;
+
+	return 0;
+}
+
 int programmer_shutdown(void)
 {
+	int i;
+
+	for (i = shutdown_fn_count - 1; i >= 0; i--)
+		shutdown_fn[i].func(shutdown_fn[i].data);
    return programmer_table[programmer].shutdown();
 }