[PATCH] register_shutdown for execution on programmer shutdown

[Carl-Daniel Hailfinger]

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.
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().
If in doubt, consult man atexit.

Usage example (try running the dummy flasher with this patch applied):
> --- dummyflasher.c	2010-02-12 02:35:18.000000000 +0100
> +++ dummyflasher.c	2010-02-12 03:52:59.000000000 +0100
> @@ -24,6 +24,18 @@
>  #include <sys/types.h>
>  #include "flash.h"
>
> +void foo(void)
> +{
> msg_pinfo("This is %s and was added first, will be executed second.\n",
>   __func__);
>
> +}
>
> +void bar(void)
> +{
> msg_pinfo("This is %s and was added second, will be executed first.\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);
> register_shutdown(bar);
> 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 895)
+++ flashrom-register_shutdown/flash.h	(Arbeitskopie)
@@ -100,6 +100,7 @@
 extern const struct programmer_entry programmer_table[];

int programmer_init(void);
+int register_shutdown(void (*function)(void));
 int programmer_shutdown(void);
 void *programmer_map_flash_region(const char *descr, unsigned long phys_addr,
    			  size_t len);
Index: flashrom-register_shutdown/flashrom.c
===================================================================
--- flashrom-register_shutdown/flashrom.c	(Revision 895)
+++ flashrom-register_shutdown/flashrom.c	(Arbeitskopie)
@@ -313,8 +313,28 @@
    return programmer_table[programmer].init();
 }

+#define SHUTDOWN_MAXFN 4
+static int shutdown_fn_count = 0;
+void (*shutdown_fn[SHUTDOWN_MAXFN]) (void);
+
+int register_shutdown(void (*function)(void))
+{
+	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++] = function;
+
+	return 0;
+}
+
 int programmer_shutdown(void)
 {
+	int i;
+
+	for (i = shutdown_fn_count - 1; i >= 0; i--)
+		shutdown_fn[i]();
    return programmer_table[programmer].shutdown();
 }

[PATCH] register_shutdown for execution on programmer shutdown

[Michael Karcher]

Am Freitag, den 12.02.2010, 03:59 +0100 schrieb Carl-Daniel Hailfinger:
> +#define SHUTDOWN_MAXFN 4
> +static int shutdown_fn_count = 0;
> +void (*shutdown_fn[SHUTDOWN_MAXFN]) (void);

style hint: Use a typedef for the function pointer type:
typedef void (*shutdown_proc)(void);
shutdown_proc shutdownfn[SHUTDOWN_MAXFN];
> +int register_shutdown(void (*function)(void))

And this gets "int register_shutdown(shutdown_proc function)
> +{
> 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++] = function;
> return 0;
>
> +}
>
> int programmer_shutdown(void)
>  {
> int i;
> for (i = shutdown_fn_count - 1; i >= 0; i--)
> shutdown_fn[i]();
> return programmer_table[programmer].shutdown();
>
> }

Except for the style hint, everything looks great, so
Acked-by: Michael Karcher

[PATCH] register_shutdown for execution on programmer shutdown

[Carl-Daniel Hailfinger]

On 13.02.2010 00:59, Michael Karcher wrote:
> Am Freitag, den 12.02.2010, 03:59 +0100 schrieb Carl-Daniel Hailfinger:
> > +#define SHUTDOWN_MAXFN 4
> > +static int shutdown_fn_count = 0;
> > +void (*shutdown_fn[SHUTDOWN_MAXFN]) (void);
>
> style hint: Use a typedef for the function pointer type:
> typedef void (*shutdown_proc)(void);

I thought about using a typedef, but given that most people expect
typedefs in the form
typedef type identifier;
whereas the suggested typedef has the form
typedef first-part-of-type identifier second-part-of-type;
a typedef may actually decrease readability for people to don't know
that such typedefs exist.
> shutdown_proc shutdownfn[SHUTDOWN_MAXFN];

About the name of the typedef: I think we should use the name
atexit_func or something like that to make it obvious we're basically
copying atexit.
> > +int register_shutdown(void (*function)(void))
>
> And this gets "int register_shutdown(shutdown_proc function)

While we're discussing typedefs, I'm not 100% sure we really want to
make this an atexit clone with all limitations. For example, some people
may want to undo any mainboard GPIO settings on shutdown, and for this,
allowing a void * parameter to functions registered with
register_shutdown would allow such functions to access a struct/whatever
with GPIO undo information.

However, if we use a struct, the typedef issue won't have a noticeable
impact anymore AFAICS.

struct shutdown_function_data {
    void (*func) (void *),
    void *data
} shutdown_fn[4];
int register_shutdown(void (*func)(void *), void *data);

What do you think?
> Except for the style hint, everything looks great, so
> Acked-by: Michael Karcher

Thanks for the review. I'll wait with a commit until you comment on my
review response.

Regards,
Carl-Daniel

[PATCH] register_shutdown for execution on programmer shutdown

[Michael Karcher]

Am Samstag, den 13.02.2010, 04:28 +0100 schrieb Carl-Daniel Hailfinger:
> On 13.02.2010 00:59, Michael Karcher wrote:
> > Am Freitag, den 12.02.2010, 03:59 +0100 schrieb Carl-Daniel Hailfinger:
> > > +#define SHUTDOWN_MAXFN 4
> > > +static int shutdown_fn_count = 0;
> > > +void (*shutdown_fn[SHUTDOWN_MAXFN]) (void);
> >
> > style hint: Use a typedef for the function pointer type:
> > typedef void (*shutdown_proc)(void);
>
> I thought about using a typedef, but given that most people expect
> typedefs in the form
> typedef type identifier;
> whereas the suggested typedef has the form
> typedef first-part-of-type identifier second-part-of-type;
> a typedef may actually decrease readability for people to don't know
> that such typedefs exist.

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.
> About the name of the typedef: I think we should use the name
> atexit_func or something like that to make it obvious we're basically
> copying atexit.

That's a nice idea, you can go for it.
> 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.
> struct shutdown_function_data {
>    void (*func) (void *),
>    void *data
> } shutdown_fn[4];
> int register_shutdown(void (*func)(void *), void *data);

Adding the void* parameter is a good idea.
> Thanks for the review. I'll wait with a commit until you comment on my
> review response.

If you add the void* parameter, I will Ack the patch soon.

Regards,
  Michael Karcher

[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();
 }

[PATCH] register_shutdown for execution on programmer shutdown

[Anders Juel Jensen]

On Friday 12 February 2010 03:59:10 Carl-Daniel Hailfinger wrote:
> 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.
> 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().
> If in doubt, consult man atexit.

If i read this right i am going to need a wakeup() function too.. otherwise i 
will be left with a system with no keyboard, mouse, power button and 
*automatic fan control*.. i damn nearly fried my CPU when I played with this 
:-/

Correct me if there is something about this i don't understand.. the below 
little program that can call flashrom between shutdown and wakeup, and then 
detection works.

#include <stdio.h>
#include <stdlib.h>
#include <sys/io.h>
#include <unistd.h>

void write_wait();
void read_wait();

int main()
{
    unsigned char ret = 0;

iopl (3);

outb_p (0xb4, 0x64);
    write_wait ();
    outb_p (0xff, 0x64);
    write_wait ();

while (! ret == 0xfa);
    {
    	read_wait ();
    	ret = inb_p (0x60);
    }

/* put nifty code that does magic trickery here.. */
        /* Like calling flashrom ;-)                                    */

outb_p (0xfb, 0x64);
    write_wait();
    outb_p (0xff, 0x64);
    write_wait ();

return 0;
}

void write_wait()
{
    int timer = 0;

while (inb_p(0x64) & 0x2)
    {
    	if (++timer == 40000)
    	{
    		printf ("KBC port 0x64 does not accept commands - \n");
    		printf ("We may in fact have been shafted..\n");
    		exit (-1);
    	}
    }
}

void read_wait()
{
    int timer = 0;

while (!inb_p(0x64) & 0x1)
    {
    	if (++timer == 40000)
    	{
    		printf ("KBC port 0x64 never became ready. Faulty Exit\n");
    		printf ("Your macine is in an unknown state.. great..\n");
    		exit (-1);
    	}
    }
}

[PATCH] register_shutdown for execution on programmer shutdown

[Michael Karcher]

Am Samstag, den 13.02.2010, 17:22 +0100 schrieb Anders Juel Jensen:
> > 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().
> > If in doubt, consult man atexit.
>
> If i read this right i am going to need a wakeup() function too.. otherwise i 
> will be left with a system with no keyboard, mouse, power button and 
> *automatic fan control*.. i damn nearly fried my CPU when I played with this 
> :-/

No, you don't read it correctly. What we call "shutdown" function is a
function that is used to shut down the flashing process, so it is a wake
up function for the EC that had to be shut down when we initialized the
flashing process.

You put the b4/ff/read sequence into the board_enable function, and
register a shutdown function that sends fb/ff.

Regards,
  Michael Karcher

[PATCH] register_shutdown for execution on programmer shutdown

[Carl-Daniel Hailfinger]

New patch, with comments from Michael addressed.

(short version of the changelog)
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.

Signed-off-by: Carl-Daniel Hailfinger

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

extern const struct programmer_entry programmer_table[];

+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 903)
+++ flashrom-register_shutdown/flashrom.c	(Arbeitskopie)
@@ -308,6 +308,35 @@
    {}, /* This entry corresponds to PROGRAMMER_INVALID. */
 };

+#define SHUTDOWN_MAXFN 4
+static int shutdown_fn_count = 0;
+struct shutdown_func_data {
+	void (*func) (void *data);
+	void *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_init(void)
 {
    return programmer_table[programmer].init();
@@ -315,6 +344,10 @@

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();
 }

[PATCH] register_shutdown for execution on programmer shutdown

[Michael Karcher]

Am Sonntag, den 14.02.2010, 02:09 +0100 schrieb Carl-Daniel Hailfinger:
> (short version of the changelog)
> 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.

Acked-by: Michael Karcher

Regards,
  Michael Karcher

[PATCH] register_shutdown for execution on programmer shutdown

[Carl-Daniel Hailfinger]

On 14.02.2010 02:15, Michael Karcher wrote:
> Am Sonntag, den 14.02.2010, 02:09 +0100 schrieb Carl-Daniel Hailfinger:
> > (short version of the changelog)
> > 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.
>
> Acked-by: Michael Karcher

Thanks for the reviews.
Committed in r904.

Regards,
Carl-Daniel

[PATCH] register_shutdown for execution on programmer shutdown

[Carl-Daniel Hailfinger]

On 13.02.2010 17:22, Anders Juel Jensen wrote:
> If i read this right i am going to need a wakeup() function too.. otherwise i 
> will be left with a system with no keyboard, mouse, power button and 
> *automatic fan control*.. i damn nearly fried my CPU when I played with this 
> :-/
>
> Correct me if there is something about this i don't understand.. the below 
> little program that can call flashrom between shutdown and wakeup, and then 
> detection works.

The idea is to move all this code into board_enable.c.

Suggested code for a pure flashrom solution (no wrapper program needed)
inside board_enable.c:

void restart_ec_on_anders_board(void *dontcare)
{
    outb_p (0xfb, 0x64);
    write_wait();
    outb_p (0xff, 0x64);
    write_wait();
}

static int board_enable_for_anders_board(const char *name)
{
    unsigned char ret = 0;

//do stuff. mostly what is in your board enable.

/* Now put the EC to sleep. You may want to factor this
     * out to a separate function.
     */
    OUTB(0xb4, 0x64);
    write_wait();
    OUTB(0xff, 0x64);
    write_wait();

while (ret != 0xfa) {
    	read_wait();
    	ret = INB(0x60);
    }

/* Make sure the EC will be woken up at the end.
     * This code will be executed unless flashrom exits
     * in between with some error (usually out of memory).
     */
    register_shutdown(restart_ec_on_anders_board, NULL);

return 0;
}

void write_wait()
{
    int timer = 0;

while (INB(0x64) & 0x2) {
    	if (++timer == 40000) {
    		msg_perr("KBC port 0x64 does not accept commands - \n");
    		msg_perr("We may in fact have been shafted..\n");
    		exit (-1);
    	}
    }
}

void read_wait()
{
    int timer = 0;

while (!(INB(0x64) & 0x1)) {
    	if (++timer == 40000) {
    		msg_perr("KBC port 0x64 never became ready. Faulty Exit\n");
    		msg_perr("Your machine is in an unknown state.. great..\n");
    		exit (-1);
    	}
    }
}

I took the liberty of adapting the coding style to flashrom standards.
Please make sure to pick appropriate function names (read_wait should
probably be called nameofec_read_wait) and make sure it actually compiles.

Please check that I didn't introduce any errors or logic inversions.

Regards,
Carl-Daniel

[PATCH] register_shutdown for execution on programmer shutdown

[Michael Karcher]

Am Sonntag, den 14.02.2010, 02:45 +0100 schrieb Carl-Daniel Hailfinger:
> I took the liberty of adapting the coding style to flashrom standards.
> Please make sure to pick appropriate function names (read_wait should
> probably be called nameofec_read_wait) and make sure it actually compiles.

It's the standard keyboard controller interface, so it should be
kbc_read_wait and kbc_read_write.

Regards,
  Michael Karcher