Patchwork Hack libpci not to exit(1) during init if PCI does not work

login
register
about
Submitter Carl-Daniel Hailfinger
Date 2011-07-06 22:34:55
Message ID <4E14E30F.5040809@gmx.net>
Download mbox | patch
Permalink /patch/3255/
State Bitrotted
Headers show

Comments

Carl-Daniel Hailfinger - 2011-07-06 22:34:55
New version: Should work for the internal programmer as well.

TODO: Add sane error handling to pcidev_init() and its callers.

libpci unhelpfully calls exit(1) instead of reporting an error if init
fails.
Fortunately, that part of libpci has not changed since the year 2000, so
we can use a really evil hack to avoid that.
We supply an alternative error printing function (which does not
exit(1)) and this function detects which error is happening and hacks
internal libpci structures in a way that avoids a later segfault due to
NULL pointer dereference.

We MUST absolutely make sure that pci_init() in libpci will not change
or this hack will explode spectacularly. Once we decide that this hack
is the way to go, notifying the libpci maintainer is essential.

To check if the hack works, run "flashrom -V" and respond to this mail
with the output.
If you don't have working PCI access, you should get a message near the
end which says that programmer initialization failed.
If you have working PCI access, the patch should not change anything.

Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net>
David Hendricks - 2011-07-06 23:55:21
Impressive!

On Wed, Jul 6, 2011 at 3:34 PM, Carl-Daniel Hailfinger <
c-d.hailfinger.devel.2006@gmx.net> wrote:

> -       /* Initialize PCI access for flash enables */

-       pacc = pci_alloc();     /* Get the pci_access structure */
> -       /* Set all options you want -- here we stick with the defaults */
> -       pci_init(pacc);         /* Initialize the PCI library */
> +       if (flashrom_pci_init())
> +               return 1;
>        pci_scan_bus(pacc);     /* We want to get the list of devices */
>

Change those last three lines to:
if (!flashrom_pci_init())
        pci_scan_bus(pacc);

and it works on systems w/o PCI (at least it did on ARM)

Here are my test results with this patch and the tegra2 patch:
http://paste.flashrom.org/view.php?id=683
Uwe Hermann - 2011-07-21 19:15:03
On Thu, Jul 07, 2011 at 12:34:55AM +0200, Carl-Daniel Hailfinger wrote:
> New version: Should work for the internal programmer as well.
> 
> TODO: Add sane error handling to pcidev_init() and its callers.

Yep, but that's for another patch.

 
> Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net>

Acked-by: Uwe Hermann <uwe@hermann-uwe.de>

I see no reason not to include this.
 

> +static struct {
> +  char *name;	/* name in all libpci versions */
> +  char *dummy1; /* Only present in some libpci versions */
> +  void *dummy2;
> +  void (*init1)(struct pci_access *); /* init in older versions */
> +  void (*init2)(struct pci_access *); /* init in newer versions */
> +  void *dummy3;
> +  void *dummy4;
> +  void *dummy5;
> +  void *dummy6;
> +  void *dummy7;
> +  void *dummy8;
> +  void *dummy9;
> +  void *dummy10;

Indentation should be one tab above too.


Attached are two logs, one from a normal board, one from a non-supported
laptop with force. I can make some more different logs if you like.


Uwe.
Stefan Tauner - 2012-09-05 08:49:44
On Thu, 07 Jul 2011 00:34:55 +0200
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> wrote:

> New version: Should work for the internal programmer as well.
> 
> TODO: Add sane error handling to pcidev_init() and its callers.
> 
> libpci unhelpfully calls exit(1) instead of reporting an error if init
> fails.
> Fortunately, that part of libpci has not changed since the year 2000, so
> we can use a really evil hack to avoid that.
> We supply an alternative error printing function (which does not
> exit(1)) and this function detects which error is happening and hacks
> internal libpci structures in a way that avoids a later segfault due to
> NULL pointer dereference.
> 
> We MUST absolutely make sure that pci_init() in libpci will not change
> or this hack will explode spectacularly. Once we decide that this hack
> is the way to go, notifying the libpci maintainer is essential.
> 

forgive me my naivety, but was upstream ever asked if they could
mitigate the problem (at least in the long term)? just exiting is of
course pretty much insane for any library... i would presume that they
understand that.

the git shows even less commits than in flashrom, so i think we would
have to do it on our own... but still better than the hack... in the
long term :)

Patch

Index: flashrom-libpci_hack_avoid_exit1_during_init/pcidev.c
===================================================================
--- flashrom-libpci_hack_avoid_exit1_during_init/pcidev.c	(Revision 1362)
+++ flashrom-libpci_hack_avoid_exit1_during_init/pcidev.c	(Arbeitskopie)
@@ -21,6 +21,8 @@ 
 
 #include <stdlib.h>
 #include <string.h>
+#include <stdio.h>
+#include <stdarg.h>
 #include "flash.h"
 #include "programmer.h"
 
@@ -188,6 +190,77 @@ 
 	return 0;
 }
 
+/* Ugliest hack ever in flashrom! Should work for all libpci versions released
+ * in the year 2000 or later. Checked to make sense until at least version
+ * 3.1.8-test1.
+ */
+static void fake_libpci_method_init(struct pci_access *foo)
+{
+	msg_pinfo("Working around another pcilib crash\n");
+	return;
+}
+
+static struct {
+  char *name;	/* name in all libpci versions */
+  char *dummy1; /* Only present in some libpci versions */
+  void *dummy2;
+  void (*init1)(struct pci_access *); /* init in older versions */
+  void (*init2)(struct pci_access *); /* init in newer versions */
+  void *dummy3;
+  void *dummy4;
+  void *dummy5;
+  void *dummy6;
+  void *dummy7;
+  void *dummy8;
+  void *dummy9;
+  void *dummy10;
+} fake_libpci_methods = {
+	.name = "None",
+	.init1 = &fake_libpci_method_init,
+	.init2 = &fake_libpci_method_init,
+};
+
+static int pci_init_failed = 0;
+
+static void my_libpci_generic_error(char *msg, ...)
+{
+	va_list ap;
+
+	msg_pinfo("pcilib: ");
+	va_start(ap, msg);
+	vprintf(msg, ap);
+	va_end(ap);
+	msg_pinfo("\n");
+	if (!strcmp(msg, "Cannot find any working access method.")) {
+		/* Workaround for crash, very ugly. */
+		msg_pinfo("Working around pcilib crash\n");
+		pacc->methods = (void *)&fake_libpci_methods;
+		pci_init_failed = 1;
+	} else {
+		/* Any error in libpci would result in exit(1) by default. */
+		exit(1);
+	}
+}
+
+/* Wrapper for libpci pci_init() which simply calls exit(1) on error. */
+int flashrom_pci_init(void)
+{
+	/* Allocate the pci_access structure. */
+	pacc = pci_alloc();     
+	/* Hook the error printing function which would exit(1). */
+	pacc->error = &my_libpci_generic_error;
+	/* Initialize the PCI library. */
+	pci_init(pacc);
+	/* pci_init returns void, so we have to get any error status from the
+	 * hooked pacc->error which updates pci_init_failed.
+	 */
+	if (pci_init_failed) {
+		pci_cleanup(pacc);
+		return 1;
+	}
+	return 0;
+}
+
 uintptr_t pcidev_init(int bar, const struct pcidev_status *devs)
 {
 	struct pci_dev *dev;
@@ -197,8 +270,9 @@ 
 	int found = 0;
 	uintptr_t addr = 0, curaddr = 0;
 
-	pacc = pci_alloc();     /* Get the pci_access structure */
-	pci_init(pacc);         /* Initialize the PCI library */
+	if (flashrom_pci_init())
+		return 1;
+
 	pci_scan_bus(pacc);     /* We want to get the list of devices */
 	pci_filter_init(pacc, &filter);
 
Index: flashrom-libpci_hack_avoid_exit1_during_init/internal.c
===================================================================
--- flashrom-libpci_hack_avoid_exit1_during_init/internal.c	(Revision 1362)
+++ flashrom-libpci_hack_avoid_exit1_during_init/internal.c	(Arbeitskopie)
@@ -192,10 +192,8 @@ 
 	 */
 	buses_supported = CHIP_BUSTYPE_NONSPI;
 
-	/* Initialize PCI access for flash enables */
-	pacc = pci_alloc();	/* Get the pci_access structure */
-	/* Set all options you want -- here we stick with the defaults */
-	pci_init(pacc);		/* Initialize the PCI library */
+	if (flashrom_pci_init())
+		return 1;
 	pci_scan_bus(pacc);	/* We want to get the list of devices */
 
 	if (processor_flash_enable()) {
Index: flashrom-libpci_hack_avoid_exit1_during_init/programmer.h
===================================================================
--- flashrom-libpci_hack_avoid_exit1_during_init/programmer.h	(Revision 1362)
+++ flashrom-libpci_hack_avoid_exit1_during_init/programmer.h	(Arbeitskopie)
@@ -225,6 +225,7 @@ 
 };
 uintptr_t pcidev_validate(struct pci_dev *dev, int bar, const struct pcidev_status *devs);
 uintptr_t pcidev_init(int bar, const struct pcidev_status *devs);
+int flashrom_pci_init(void);
 /* rpci_write_* are reversible writes. The original PCI config space register
  * contents will be restored on shutdown.
  */