[PATCH] Undo all PCI writes on shutdown

[Carl-Daniel Hailfinger]

Undo all PCI config space writes on shutdown.
This means all chipset enables etc. will be undone on shutdown.
Any writes which are one-shot should use the permanent ppci_write_*
variants.

Extend the number of available register_shutdown slots to 32.

Signed-off-by: Carl-Daniel Hailfinger

Index: flashrom-pci_configspace_shutdown_restore/drkaiser.c
===================================================================
--- flashrom-pci_configspace_shutdown_restore/drkaiser.c	(Revision 1225)
+++ flashrom-pci_configspace_shutdown_restore/drkaiser.c	(Arbeitskopie)
@@ -61,8 +61,7 @@

int drkaiser_shutdown(void)
 {
-	/* Write protect the flash again. */
-	pci_write_word(pcidev_dev, PCI_MAGIC_DRKAISER_ADDR, 0);
+	/* Flash write is disabled automatically by PCI restore. */
    pci_cleanup(pacc);
    release_io_perms();
    return 0;
Index: flashrom-pci_configspace_shutdown_restore/pcidev.c
===================================================================
--- flashrom-pci_configspace_shutdown_restore/pcidev.c	(Revision 1225)
+++ flashrom-pci_configspace_shutdown_restore/pcidev.c	(Arbeitskopie)
@@ -21,6 +21,7 @@
 #include <stdlib.h>
 #include <string.h>
 #include "flash.h"
+#define NO_PCI_REDIRECT 1
 #include "programmer.h"

uint32_t io_base_addr;
@@ -90,7 +91,7 @@
    uint32_t addr = 0, curaddr = 0;

pacc = pci_alloc();     /* Get the pci_access structure */
-	pci_init(pacc);         /* Initialize the PCI library */
+	pci_init(pacc);	 /* Initialize the PCI library */
    pci_scan_bus(pacc);     /* We want to get the list of devices */
    pci_filter_init(pacc, &filter);

@@ -142,3 +143,85 @@
    	       (devs[i].status == NT) ? " (untested)" : "");
    }
 }
+
+enum pci_write_type {
+	pci_write_type_byte,
+	pci_write_type_word,
+	pci_write_type_long,
+};
+
+struct undo_pci_write_data {
+	struct pci_dev dev;
+	int pos;
+	enum pci_write_type type;
+	union {
+		uint8_t bytedata;
+		uint16_t worddata;
+		uint32_t longdata;
+	};
+};
+
+void undo_pci_write(void *p)
+{
+	struct undo_pci_write_data *data = p;
+	msg_pdbg("Restoring PCI config space for %02x:%02x:%01x pos 0x%02x\n",
+		 data->dev.bus, data->dev.dev, data->dev.func, data->pos);
+	switch (data->type) {
+	case pci_write_type_byte:
+		pci_write_byte(&data->dev, data->pos, data->bytedata);
+		break;
+	case pci_write_type_word:
+		pci_write_word(&data->dev, data->pos, data->worddata);
+		break;
+	case pci_write_type_long:
+		pci_write_long(&data->dev, data->pos, data->longdata);
+		break;
+	}
+	/* p was allocated in register_undo_pci_write. */
+	free(p);
+}
+
+#define register_undo_pci_write(a, b, c) 				\
+{									\
+	struct undo_pci_write_data *undo_pci_write_data;		\
+	undo_pci_write_data = malloc(sizeof(struct undo_pci_write_data)); \
+	undo_pci_write_data->dev = *a;					\
+	undo_pci_write_data->pos = b;					\
+	undo_pci_write_data->type = pci_write_type_##c;			\
+	undo_pci_write_data->c##data = pci_read_##c(dev, pos);		\
+	register_shutdown(undo_pci_write, undo_pci_write_data);		\
+}
+
+int rpci_write_byte(struct pci_dev *dev, int pos, uint8_t data)
+{
+	register_undo_pci_write(dev, pos, byte);
+	return pci_write_byte(dev, pos, data);
+}
+ 
+int rpci_write_word(struct pci_dev *dev, int pos, uint16_t data)
+{
+	register_undo_pci_write(dev, pos, word);
+	return pci_write_word(dev, pos, data);
+}
+ 
+int rpci_write_long(struct pci_dev *dev, int pos, uint32_t data)
+{
+	register_undo_pci_write(dev, pos, long);
+	return pci_write_long(dev, pos, data);
+}
+
+int ppci_write_byte(struct pci_dev *dev, int pos, u8 data)
+{
+	return pci_write_byte(dev, pos, data);
+}
+ 
+int ppci_write_word(struct pci_dev *dev, int pos, u16 data)
+{
+	return pci_write_word(dev, pos, data);
+}
+ 
+int ppci_write_long(struct pci_dev *dev, int pos, u32 data)
+{
+	return pci_write_long(dev, pos, data);
+}
+ 
Index: flashrom-pci_configspace_shutdown_restore/gfxnvidia.c
===================================================================
--- flashrom-pci_configspace_shutdown_restore/gfxnvidia.c	(Revision 1225)
+++ flashrom-pci_configspace_shutdown_restore/gfxnvidia.c	(Arbeitskopie)
@@ -89,13 +89,9 @@

int gfxnvidia_shutdown(void)
 {
-	uint32_t reg32;
-
-	/* Disallow access to flash interface (and re-enable screen). */
-	reg32 = pci_read_long(pcidev_dev, 0x50);
-	reg32 |= (1 << 0);
-	pci_write_long(pcidev_dev, 0x50, reg32);
-
+	/* Flash interface access is disabled (and screen enabled) automatically
+	 * by PCI restore.
+	 */
    pci_cleanup(pacc);
    release_io_perms();
    return 0;
Index: flashrom-pci_configspace_shutdown_restore/atahpt.c
===================================================================
--- flashrom-pci_configspace_shutdown_restore/atahpt.c	(Revision 1225)
+++ flashrom-pci_configspace_shutdown_restore/atahpt.c	(Arbeitskopie)
@@ -59,13 +59,7 @@

int atahpt_shutdown(void)
 {
-	uint32_t reg32;
-
-	/* Disable flash access again. */
-	reg32 = pci_read_long(pcidev_dev, REG_FLASH_ACCESS);
-	reg32 &= ~(1 << 24);
-	pci_write_long(pcidev_dev, REG_FLASH_ACCESS, reg32);
-
+	/* Flash access is disabled automatically by PCI restore. */
    pci_cleanup(pacc);
    release_io_perms();
    return 0;
Index: flashrom-pci_configspace_shutdown_restore/chipset_enable.c
===================================================================
--- flashrom-pci_configspace_shutdown_restore/chipset_enable.c	(Revision 1225)
+++ flashrom-pci_configspace_shutdown_restore/chipset_enable.c	(Arbeitskopie)
@@ -498,17 +498,6 @@
    return enable_flash_ich_dc_spi(dev, name, 10);
 }

-static void via_do_byte_merge(void * arg)
-{
-	struct pci_dev * dev = arg;
-	uint8_t val;
-
-	msg_pdbg("Re-enabling byte merging\n");
-	val = pci_read_byte(dev, 0x71);
-	val |= 0x40;
-	pci_write_byte(dev, 0x71, val);
-}
-
 static int via_no_byte_merge(struct pci_dev *dev, const char *name)
 {
    uint8_t val;
@@ -519,7 +508,6 @@
    	msg_pdbg("Disabling byte merging\n");
    	val &= ~0x40;
    	pci_write_byte(dev, 0x71, val);
-		register_shutdown(via_do_byte_merge, dev);
    }
    return NOT_DONE_YET;	/* need to find south bridge, too */
 }
Index: flashrom-pci_configspace_shutdown_restore/flashrom.c
===================================================================
--- flashrom-pci_configspace_shutdown_restore/flashrom.c	(Revision 1225)
+++ flashrom-pci_configspace_shutdown_restore/flashrom.c	(Arbeitskopie)
@@ -441,7 +441,7 @@
    {}, /* This entry corresponds to PROGRAMMER_INVALID. */
 };

-#define SHUTDOWN_MAXFN 4
+#define SHUTDOWN_MAXFN 32
 static int shutdown_fn_count = 0;
 struct shutdown_func_data {
    void (*func) (void *data);
Index: flashrom-pci_configspace_shutdown_restore/programmer.h
===================================================================
--- flashrom-pci_configspace_shutdown_restore/programmer.h	(Revision 1225)
+++ flashrom-pci_configspace_shutdown_restore/programmer.h	(Arbeitskopie)
@@ -212,7 +212,25 @@
 };
 uint32_t pcidev_validate(struct pci_dev *dev, uint32_t bar, const struct pcidev_status *devs);
 uint32_t pcidev_init(uint16_t vendor_id, uint32_t bar, const struct pcidev_status *devs);
+/* rpci_write_* are reversible writes. The original PCI config space register
+ * contents will be restored on shutdown.
+ */
+int rpci_write_byte(struct pci_dev *dev, int pos, u8 data);
+int rpci_write_word(struct pci_dev *dev, int pos, u16 data);
+int rpci_write_long(struct pci_dev *dev, int pos, u32 data);
+/* ppci_write_* are permanent writes, an alias for pci_write_*. */
+int ppci_write_byte(struct pci_dev *dev, int pos, u8 data);
+int ppci_write_word(struct pci_dev *dev, int pos, u16 data);
+int ppci_write_long(struct pci_dev *dev, int pos, u32 data);
+
+#if NO_PCI_REDIRECT
+/* Don't touch pci_write_* definitions. */
+#else
+#define pci_write_byte rpci_write_byte
+#define pci_write_word rpci_write_word
+#define pci_write_long rpci_write_long
 #endif
+#endif

/* print.c */
 #if CONFIG_NIC3COM+CONFIG_NICREALTEK+CONFIG_NICNATSEMI+CONFIG_GFXNVIDIA+CONFIG_DRKAISER+CONFIG_SATASII+CONFIG_ATAHPT+CONFIG_NICINTEL_SPI >= 1

[Michael Karcher]

Am Freitag, den 05.11.2010, 20:54 +0100 schrieb Carl-Daniel Hailfinger:
> Undo all PCI config space writes on shutdown.
> This means all chipset enables etc. will be undone on shutdown.
> Any writes which are one-shot should use the permanent ppci_write_*
> variants.

Great idea, I'm all for restoring PCI config on exiting flashrom and
have flash writes disabled after the program quits. I also understand
the motivation of the PCI function redirection to the auto-undo variant,
but I'm not quite sure that this is a good idea: It helps keeping this
patch short and simple, but it redefines what certain functions do that
many developers already know (as libpci is in wide use).

There might also be some writes that don't need to be undone or even
shouldn't be undone (e.g. think of "write 1 to clear" bits or other
magic actions triggered just by accessing PCI registers). While I hope
that flashrom doesn't need to touch any such register, the PCI writes
should be individually checked for whether it makes sense to undo them.
This review of all writes can then be documented by changing them to
rpci_write_foo calls.

I don't know whether it works, but you might want to add a "deprecated"
attribute to the pci_write_foo calls unless PCI_NO_REDIRECT is defined,
so you get a compiler warning (==error in flashrom) if you don't
explicitly specify "permanent" or "reversed".
> Extend the number of available register_shutdown slots to 32.

Did you count that this is enough?

Regards,
  Michael Karcher

[David Hendricks]

On Fri, Nov 5, 2010 at 12:54 PM, Carl-Daniel Hailfinger <
[e-mail]> wrote:
> Undo all PCI config space writes on shutdown.
> This means all chipset enables etc. will be undone on shutdown.
> Any writes which are one-shot should use the permanent ppci_write_*
> variants.

Nack. I like the idea of the rpci_* functions, but I think rpci_* should be
opt-in and pci_* functions should remain as they are. As Michael pointed
out, I don't think it's good to redefine this behavior, and will probably be
a source of errors in the future for those used to pci_* functions as
defined in libpci.

On Fri, Nov 5, 2010 at 12:54 PM, Carl-Daniel Hailfinger <
[e-mail]> wrote:
> Index: flashrom-pci_configspace_shutdown_restore/drkaiser.c
>
> --- flashrom-pci_configspace_shutdown_restore/drkaiser.c        (Revision
> 1225)
> +++ flashrom-pci_configspace_shutdown_restore/drkaiser.c
>  (Arbeitskopie)
> @@ -61,8 +61,7 @@
>
> int drkaiser_shutdown(void)
>  {
>   /* Write protect the flash again. */
>   pci_write_word(pcidev_dev, PCI_MAGIC_DRKAISER_ADDR, 0);
>   /* Flash write is disabled automatically by PCI restore. */

It's fine to nuke this function. However, I'd rather see corresponding
pci_write_word in drkaiser_init() changed to rpci_write_word().

On Fri, Nov 5, 2010 at 12:54 PM, Carl-Daniel Hailfinger <
[e-mail]> wrote:
> +#define NO_PCI_REDIRECT 1

Should this be "NO_PCI_RESTORE"? Maybe I misunderstand the context of this
particular change...

On Fri, Nov 5, 2010 at 12:54 PM, Carl-Daniel Hailfinger <
[e-mail]> wrote:
> +#define register_undo_pci_write(a, b, c)                               \
> +{                                                                      \
>   struct undo_pci_write_data *undo_pci_write_data;                \
>   undo_pci_write_data = malloc(sizeof(struct undo_pci_write_data)); \
>   undo_pci_write_data->dev = *a;                                  \
>   undo_pci_write_data->pos = b;                                   \
>   undo_pci_write_data->type = pci_write_type_##c;                 \
>   undo_pci_write_data->c##data = pci_read_##c(dev, pos);          \
>   register_shutdown(undo_pci_write, undo_pci_write_data);         \
>
> +}
>
> +int rpci_write_byte(struct pci_dev *dev, int pos, uint8_t data)
> +{
>   register_undo_pci_write(dev, pos, byte);
>   return pci_write_byte(dev, pos, data);
>
> +}
>
> +int rpci_write_word(struct pci_dev *dev, int pos, uint16_t data)
> +{
>   register_undo_pci_write(dev, pos, word);
>   return pci_write_word(dev, pos, data);
>
> +}
>
> +int rpci_write_long(struct pci_dev *dev, int pos, uint32_t data)
> +{
>   register_undo_pci_write(dev, pos, long);
>   return pci_write_long(dev, pos, data);
>
> +}
>
> +int ppci_write_byte(struct pci_dev *dev, int pos, u8 data)
> +{
>   return pci_write_byte(dev, pos, data);
>
> +}
>
> +int ppci_write_word(struct pci_dev *dev, int pos, u16 data)
> +{
>   return pci_write_word(dev, pos, data);
>
> +}
>
> +int ppci_write_long(struct pci_dev *dev, int pos, u32 data)
> +{
>   return pci_write_long(dev, pos, data);
>
> +}

Dumb question -- What exactly is "pos" in this context? It looks like
register offset, in which case "reg" or something would be a better name.

On Fri, Nov 5, 2010 at 12:54 PM, Carl-Daniel Hailfinger <
[e-mail]> wrote:
> Index: flashrom-pci_configspace_shutdown_restore/gfxnvidia.c
>
> --- flashrom-pci_configspace_shutdown_restore/gfxnvidia.c       (Revision
> 1225)
> +++ flashrom-pci_configspace_shutdown_restore/gfxnvidia.c
> (Arbeitskopie)
> @@ -89,13 +89,9 @@
>
> int gfxnvidia_shutdown(void)
>  {
>   uint32_t reg32;
>   /* Disallow access to flash interface (and re-enable screen). */
>   reg32 = pci_read_long(pcidev_dev, 0x50);
>   reg32 |= (1 << 0);
>   pci_write_long(pcidev_dev, 0x50, reg32);
>   /* Flash interface access is disabled (and screen enabled)
>
> automatically
>    * by PCI restore.
>    */
>  pci_cleanup(pacc);
>  release_io_perms();
>  return 0;
> Index: flashrom-pci_configspace_shutdown_restore/atahpt.c
>
> --- flashrom-pci_configspace_shutdown_restore/atahpt.c  (Revision 1225)
> +++ flashrom-pci_configspace_shutdown_restore/atahpt.c  (Arbeitskopie)
> @@ -59,13 +59,7 @@
>
> int atahpt_shutdown(void)
>  {
>   uint32_t reg32;
>   /* Disable flash access again. */
>   reg32 = pci_read_long(pcidev_dev, REG_FLASH_ACCESS);
>   reg32 &= ~(1 << 24);
>   pci_write_long(pcidev_dev, REG_FLASH_ACCESS, reg32);
>   /* Flash access is disabled automatically by PCI restore. */
>  pci_cleanup(pacc);
>  release_io_perms();
>  return 0;
> Index: flashrom-pci_configspace_shutdown_restore/chipset_enable.c
>
> --- flashrom-pci_configspace_shutdown_restore/chipset_enable.c  (Revision
> 1225)
> +++ flashrom-pci_configspace_shutdown_restore/chipset_enable.c
>  (Arbeitskopie)
> @@ -498,17 +498,6 @@
>        return enable_flash_ich_dc_spi(dev, name, 10);
>  }
>
> -static void via_do_byte_merge(void * arg)
> -{
>   struct pci_dev * dev = arg;
>   uint8_t val;
>   msg_pdbg("Re-enabling byte merging\n");
>   val = pci_read_byte(dev, 0x71);
>   val |= 0x40;
>   pci_write_byte(dev, 0x71, val);
>
> -}
>
> static int via_no_byte_merge(struct pci_dev *dev, const char *name)
>  {
>        uint8_t val;
> @@ -519,7 +508,6 @@
>                msg_pdbg("Disabling byte merging\n");
>                val &= ~0x40;
>                pci_write_byte(dev, 0x71, val);
>           register_shutdown(via_do_byte_merge, dev);
>  }
>  return NOT_DONE_YET;    /* need to find south bridge, too */
>
> }

Good stuff!

On Fri, Nov 5, 2010 at 12:54 PM, Carl-Daniel Hailfinger <
[e-mail]> wrote:
> Index: flashrom-pci_configspace_shutdown_restore/flashrom.c
> +#if NO_PCI_REDIRECT
> +/* Don't touch pci_write_* definitions. */
> +#else
> +#define pci_write_byte rpci_write_byte
> +#define pci_write_word rpci_write_word
> +#define pci_write_long rpci_write_long
>  #endif
> +#endif

This is really the only part of the patch that I dislike...

I think the added work of changing a couple pci_write_* calls to rpci_* in
the via, atahpt, drkaiser, and gfxnvidia functions is better than changing
the behavior of pci_write_* all together.

[Carl-Daniel Hailfinger]

Undo all PCI config space writes on shutdown.
This means all chipset enables etc. will be undone on shutdown.
Any writes which are one-shot should use the permanent ppci_write_*
variants.

Extend the number of available register_shutdown slots to 32.

Signed-off-by: Carl-Daniel Hailfinger

Index: flashrom-const_strlen/processor_enable.c
===================================================================
--- flashrom-const_strlen/processor_enable.c	(Revision 1225)
+++ flashrom-const_strlen/processor_enable.c	(Arbeitskopie)
@@ -56,13 +56,13 @@
    	while (*ptr && isspace((unsigned char)*ptr))
    		ptr++;
    	/* "cpu" part appears only with some Linux versions.  */
-		if (strncmp(ptr, "cpu", sizeof("cpu") - 1) == 0)
-			ptr += sizeof("cpu") - 1;
+		if (strncmp(ptr, "cpu", strlen("cpu")) == 0)
+			ptr += strlen("cpu");
    	while (*ptr && isspace((unsigned char)*ptr))
    		ptr++;
-		if (strncmp(ptr, "model", sizeof("model") - 1) != 0)
+		if (strncmp(ptr, "model", strlen("model")) != 0)
    		continue;
-		ptr += sizeof("model") - 1;
+		ptr += strlen("model");
    	while (*ptr && isspace((unsigned char)*ptr))
    		ptr++;
    	if (*ptr != ':')
@@ -72,9 +72,9 @@
    		ptr++;
    	fclose(cpuinfo);
    	return (strncmp(ptr, "ICT Loongson-2 V0.3",
-				sizeof("ICT Loongson-2 V0.3") - 1) == 0)
+				strlen("ICT Loongson-2 V0.3")) == 0)
    	    || (strncmp(ptr, "Godson2 V0.3  FPU V0.1",
-				sizeof("Godson2 V0.3  FPU V0.1") - 1) == 0);
+				strlen("Godson2 V0.3  FPU V0.1")) == 0);
    }
    fclose(cpuinfo);
    return 0;

[Carl-Daniel Hailfinger]

Sorry. It seems my patch generation script autocompleted the wrong filename.
Send the correct patch this time.

Undo all PCI config space writes on shutdown.
This means all chipset enables etc. will be undone on shutdown.
Any writes which are one-shot should use the permanent ppci_write_*
variants.

Extend the number of available register_shutdown slots to 32.

Signed-off-by: Carl-Daniel Hailfinger

Index: flashrom-pci_configspace_shutdown_restore/drkaiser.c
===================================================================
--- flashrom-pci_configspace_shutdown_restore/drkaiser.c	(Revision 1231)
+++ flashrom-pci_configspace_shutdown_restore/drkaiser.c	(Arbeitskopie)
@@ -47,7 +47,7 @@
    		   drkaiser_pcidev);

/* Write magic register to enable flash write. */
-	pci_write_word(pcidev_dev, PCI_MAGIC_DRKAISER_ADDR,
+	rpci_write_word(pcidev_dev, PCI_MAGIC_DRKAISER_ADDR,
    	       PCI_MAGIC_DRKAISER_VALUE);

/* Map 128KB flash memory window. */
@@ -61,8 +61,7 @@

int drkaiser_shutdown(void)
 {
-	/* Write protect the flash again. */
-	pci_write_word(pcidev_dev, PCI_MAGIC_DRKAISER_ADDR, 0);
+	/* Flash write is disabled automatically by PCI restore. */
    pci_cleanup(pacc);
    release_io_perms();
    return 0;
Index: flashrom-pci_configspace_shutdown_restore/pcidev.c
===================================================================
--- flashrom-pci_configspace_shutdown_restore/pcidev.c	(Revision 1231)
+++ flashrom-pci_configspace_shutdown_restore/pcidev.c	(Arbeitskopie)
@@ -21,6 +21,7 @@
 #include <stdlib.h>
 #include <string.h>
 #include "flash.h"
+#define NO_PCI_REDIRECT 1
 #include "programmer.h"

uint32_t io_base_addr;
@@ -142,3 +143,73 @@
    	       (devs[i].status == NT) ? " (untested)" : "");
    }
 }
+
+enum pci_write_type {
+	pci_write_type_byte,
+	pci_write_type_word,
+	pci_write_type_long,
+};
+
+struct undo_pci_write_data {
+	struct pci_dev dev;
+	int reg;
+	enum pci_write_type type;
+	union {
+		uint8_t bytedata;
+		uint16_t worddata;
+		uint32_t longdata;
+	};
+};
+
+void undo_pci_write(void *p)
+{
+	struct undo_pci_write_data *data = p;
+	msg_pdbg("Restoring PCI config space for %02x:%02x:%01x reg 0x%02x\n",
+		 data->dev.bus, data->dev.dev, data->dev.func, data->reg);
+	switch (data->type) {
+	case pci_write_type_byte:
+		pci_write_byte(&data->dev, data->reg, data->bytedata);
+		break;
+	case pci_write_type_word:
+		pci_write_word(&data->dev, data->reg, data->worddata);
+		break;
+	case pci_write_type_long:
+		pci_write_long(&data->dev, data->reg, data->longdata);
+		break;
+	}
+	/* p was allocated in register_undo_pci_write. */
+	free(p);
+}
+
+#define register_undo_pci_write(a, b, c) 				\
+{									\
+	struct undo_pci_write_data *undo_pci_write_data;		\
+	undo_pci_write_data = malloc(sizeof(struct undo_pci_write_data)); \
+	undo_pci_write_data->dev = *a;					\
+	undo_pci_write_data->reg = b;					\
+	undo_pci_write_data->type = pci_write_type_##c;			\
+	undo_pci_write_data->c##data = pci_read_##c(dev, reg);		\
+	register_shutdown(undo_pci_write, undo_pci_write_data);		\
+}
+
+#define register_undo_pci_write_byte(a, b) register_undo_pci_write(a, b, byte)
+#define register_undo_pci_write_word(a, b) register_undo_pci_write(a, b, word)
+#define register_undo_pci_write_long(a, b) register_undo_pci_write(a, b, long)
+
+int rpci_write_byte(struct pci_dev *dev, int reg, uint8_t data)
+{
+	register_undo_pci_write_byte(dev, reg);
+	return pci_write_byte(dev, reg, data);
+}
+ 
+int rpci_write_word(struct pci_dev *dev, int reg, uint16_t data)
+{
+	register_undo_pci_write_word(dev, reg);
+	return pci_write_word(dev, reg, data);
+}
+ 
+int rpci_write_long(struct pci_dev *dev, int reg, uint32_t data)
+{
+	register_undo_pci_write_long(dev, reg);
+	return pci_write_long(dev, reg, data);
+}
Index: flashrom-pci_configspace_shutdown_restore/gfxnvidia.c
===================================================================
--- flashrom-pci_configspace_shutdown_restore/gfxnvidia.c	(Revision 1231)
+++ flashrom-pci_configspace_shutdown_restore/gfxnvidia.c	(Arbeitskopie)
@@ -75,7 +75,7 @@
    /* Allow access to flash interface (will disable screen). */
    reg32 = pci_read_long(pcidev_dev, 0x50);
    reg32 &= ~(1 << 0);
-	pci_write_long(pcidev_dev, 0x50, reg32);
+	rpci_write_long(pcidev_dev, 0x50, reg32);

nvidia_bar = physmap("NVIDIA", io_base_addr, 16 * 1024 * 1024);

@@ -89,13 +89,9 @@

int gfxnvidia_shutdown(void)
 {
-	uint32_t reg32;
-
-	/* Disallow access to flash interface (and re-enable screen). */
-	reg32 = pci_read_long(pcidev_dev, 0x50);
-	reg32 |= (1 << 0);
-	pci_write_long(pcidev_dev, 0x50, reg32);
-
+	/* Flash interface access is disabled (and screen enabled) automatically
+	 * by PCI restore.
+	 */
    pci_cleanup(pacc);
    release_io_perms();
    return 0;
Index: flashrom-pci_configspace_shutdown_restore/atahpt.c
===================================================================
--- flashrom-pci_configspace_shutdown_restore/atahpt.c	(Revision 1231)
+++ flashrom-pci_configspace_shutdown_restore/atahpt.c	(Arbeitskopie)
@@ -50,7 +50,7 @@
    /* Enable flash access. */
    reg32 = pci_read_long(pcidev_dev, REG_FLASH_ACCESS);
    reg32 |= (1 << 24);
-	pci_write_long(pcidev_dev, REG_FLASH_ACCESS, reg32);
+	rpci_write_long(pcidev_dev, REG_FLASH_ACCESS, reg32);

buses_supported = CHIP_BUSTYPE_PARALLEL;

@@ -59,13 +59,7 @@

int atahpt_shutdown(void)
 {
-	uint32_t reg32;
-
-	/* Disable flash access again. */
-	reg32 = pci_read_long(pcidev_dev, REG_FLASH_ACCESS);
-	reg32 &= ~(1 << 24);
-	pci_write_long(pcidev_dev, REG_FLASH_ACCESS, reg32);
-
+	/* Flash access is disabled automatically by PCI restore. */
    pci_cleanup(pacc);
    release_io_perms();
    return 0;
Index: flashrom-pci_configspace_shutdown_restore/chipset_enable.c
===================================================================
--- flashrom-pci_configspace_shutdown_restore/chipset_enable.c	(Revision 1231)
+++ flashrom-pci_configspace_shutdown_restore/chipset_enable.c	(Arbeitskopie)
@@ -47,7 +47,7 @@
     */
    tmp = pci_read_byte(dev, 0x47);
    tmp |= 0x46;
-	pci_write_byte(dev, 0x47, tmp);
+	rpci_write_byte(dev, 0x47, tmp);

return 0;
 }
@@ -58,7 +58,7 @@

tmp = pci_read_byte(dev, 0xd0);
    tmp |= 0xf8;
-	pci_write_byte(dev, 0xd0, tmp);
+	rpci_write_byte(dev, 0xd0, tmp);

return 0;
 }
@@ -72,7 +72,7 @@
    new = pci_read_byte(dev, 0x40);
    new &= (~0x04); /* No idea why we clear bit 2. */
    new |= 0xb; /* 0x3 for some chipsets, bit 7 seems to be don't care. */
-	pci_write_byte(dev, 0x40, new);
+	rpci_write_byte(dev, 0x40, new);
    newer = pci_read_byte(dev, 0x40);
    if (newer != new) {
    	msg_pinfo("tried to set register 0x%x to 0x%x on %s failed (WARNING ONLY)\n", 0x40, new, name);
@@ -160,7 +160,7 @@
    new = pci_read_byte(sbdev, 0x45);
    new &= (~0x20);
    new |= 0x4;
-	pci_write_byte(sbdev, 0x45, new);
+	rpci_write_byte(sbdev, 0x45, new);
    newer = pci_read_byte(sbdev, 0x45);
    if (newer != new) {
    	msg_pinfo("tried to set register 0x%x to 0x%x on %s failed (WARNING ONLY)\n", 0x45, new, name);
@@ -186,7 +186,7 @@
    new = pci_read_byte(sbdev, 0x45);
    new &= (~0x80);
    new |= 0x40;
-	pci_write_byte(sbdev, 0x45, new);
+	rpci_write_byte(sbdev, 0x45, new);
    newer = pci_read_byte(sbdev, 0x45);
    if (newer != new) {
    	msg_pinfo("tried to set register 0x%x to 0x%x on %s failed (WARNING ONLY)\n", 0x45, new, name);
@@ -233,7 +233,7 @@
    if (new == old)
    	return 0;

-	pci_write_word(dev, xbcs, new);
+	rpci_write_word(dev, xbcs, new);

if (pci_read_word(dev, xbcs) != new) {
    	msg_pinfo("tried to set 0x%x to 0x%x on %s failed (WARNING ONLY)\n", xbcs, new, name);
@@ -269,7 +269,7 @@
    if (new == old)
    	return 0;

-	pci_write_byte(dev, bios_cntl, new);
+	rpci_write_byte(dev, bios_cntl, new);

if (pci_read_byte(dev, bios_cntl) != new) {
    	msg_pinfo("tried to set 0x%x to 0x%x on %s failed (WARNING ONLY)\n", bios_cntl, new, name);
@@ -306,8 +306,8 @@

/* FIXME: Need to undo this on shutdown. */
    	msg_pinfo("\nSetting IDSEL=0x%x for top 16 MB", fwh_conf);
-		pci_write_long(dev, 0xd0, fwh_conf);
-		pci_write_word(dev, 0xd4, fwh_conf);
+		rpci_write_long(dev, 0xd0, fwh_conf);
+		rpci_write_word(dev, 0xd4, fwh_conf);
    	/* FIXME: Decode settings are not changed. */
    } else if (idsel) {
    	msg_perr("Error: idsel= specified, but no number given.\n");
@@ -403,7 +403,7 @@
        new = old & ~1;

if (new != old)
-               pci_write_byte(dev, 0xd9, new);
+               rpci_write_byte(dev, 0xd9, new);

buses_supported = CHIP_BUSTYPE_FWH;
        return 0;
@@ -498,17 +498,6 @@
    return enable_flash_ich_dc_spi(dev, name, 10);
 }

-static void via_do_byte_merge(void * arg)
-{
-	struct pci_dev * dev = arg;
-	uint8_t val;
-
-	msg_pdbg("Re-enabling byte merging\n");
-	val = pci_read_byte(dev, 0x71);
-	val |= 0x40;
-	pci_write_byte(dev, 0x71, val);
-}
-
 static int via_no_byte_merge(struct pci_dev *dev, const char *name)
 {
    uint8_t val;
@@ -518,8 +507,7 @@
    {
    	msg_pdbg("Disabling byte merging\n");
    	val &= ~0x40;
-		pci_write_byte(dev, 0x71, val);
-		register_shutdown(via_do_byte_merge, dev);
+		rpci_write_byte(dev, 0x71, val);
    }
    return NOT_DONE_YET;	/* need to find south bridge, too */
 }
@@ -529,12 +517,12 @@
    uint8_t val;

/* enable ROM decode range (1MB) FFC00000 - FFFFFFFF */
-	pci_write_byte(dev, 0x41, 0x7f);
+	rpci_write_byte(dev, 0x41, 0x7f);

/* ROM write enable */
    val = pci_read_byte(dev, 0x40);
    val |= 0x10;
-	pci_write_byte(dev, 0x40, val);
+	rpci_write_byte(dev, 0x40, val);

if (pci_read_byte(dev, 0x40) != val) {
    	msg_pinfo("\nWARNING: Failed to enable flash write on "%s"\n",
@@ -546,7 +534,7 @@
        /* All memory cycles, not just ROM ones, go to LPC. */
        val = pci_read_byte(dev, 0x59);
        val &= ~0x80;
-	    pci_write_byte(dev, 0x59, val);
+	    rpci_write_byte(dev, 0x59, val);
    }

return 0;
@@ -580,12 +568,12 @@
    reg8 |= LOWER_ROM_ADDRESS_RANGE;
    reg8 |= UPPER_ROM_ADDRESS_RANGE;
    reg8 |= ROM_WRITE_ENABLE;
-	pci_write_byte(dev, ROM_AT_LOGIC_CONTROL_REG, reg8);
+	rpci_write_byte(dev, ROM_AT_LOGIC_CONTROL_REG, reg8);

/* Set positive decode on ROM. */
    reg8 = pci_read_byte(dev, DECODE_CONTROL_REG2);
    reg8 |= BIOS_ROM_POSITIVE_DECODE;
-	pci_write_byte(dev, DECODE_CONTROL_REG2, reg8);
+	rpci_write_byte(dev, DECODE_CONTROL_REG2, reg8);

reg8 = pci_read_byte(dev, CS5530_RESET_CONTROL_REG);
    if (reg8 & CS5530_ISA_MASTER) {
@@ -649,7 +637,7 @@
 {
    uint8_t new;

-	pci_write_byte(dev, 0x52, 0xee);
+	rpci_write_byte(dev, 0x52, 0xee);

new = pci_read_byte(dev, 0x52);

@@ -670,7 +658,7 @@
    old = pci_read_byte(dev, 0x43);
    new = old | 0xC0;
    if (new != old) {
-		pci_write_byte(dev, 0x43, new);
+		rpci_write_byte(dev, 0x43, new);
    	if (pci_read_byte(dev, 0x43) != new) {
    		msg_pinfo("tried to set 0x%x to 0x%x on %s failed (WARNING ONLY)\n", 0x43, new, name);
    	}
@@ -681,7 +669,7 @@
    new = old | 0x01;
    if (new == old)
    	return 0;
-	pci_write_byte(dev, 0x40, new);
+	rpci_write_byte(dev, 0x40, new);

if (pci_read_byte(dev, 0x40) != new) {
    	msg_pinfo("tried to set 0x%x to 0x%x on %s failed (WARNING ONLY)\n", 0x40, new, name);
@@ -709,7 +697,7 @@
    		(prot & 0xfffffc00),
    		(prot & 0xfffffc00) + ((prot & 0x3ff) << 8));
    	prot &= 0xfffffffc;
-		pci_write_byte(dev, reg, prot);
+		rpci_write_byte(dev, reg, prot);
    	prot = pci_read_long(dev, reg);
    	if (prot & 0x3)
    		msg_perr("SB600 %s%sunprotect failed from %u to %u\n",
@@ -765,11 +753,11 @@
 {
    uint8_t tmp;

-	pci_write_byte(dev, 0x92, 0);
+	rpci_write_byte(dev, 0x92, 0);

tmp = pci_read_byte(dev, 0x6d);
    tmp |= 0x01;
-	pci_write_byte(dev, 0x6d, tmp);
+	rpci_write_byte(dev, 0x6d, tmp);

return 0;
 }
@@ -781,7 +769,7 @@
    old = pci_read_byte(dev, 0x88);
    new = old | 0xc0;
    if (new != old) {
-		pci_write_byte(dev, 0x88, new);
+		rpci_write_byte(dev, 0x88, new);
    	if (pci_read_byte(dev, 0x88) != new) {
    		msg_pinfo("tried to set 0x%x to 0x%x on %s failed (WARNING ONLY)\n", 0x88, new, name);
    	}
@@ -791,7 +779,7 @@
    new = old | 0x01;
    if (new == old)
    	return 0;
-	pci_write_byte(dev, 0x6d, new);
+	rpci_write_byte(dev, 0x6d, new);

if (pci_read_byte(dev, 0x6d) != new) {
    	msg_pinfo("tried to set 0x%x to 0x%x on %s failed (WARNING ONLY)\n", 0x6d, new, name);
@@ -835,12 +823,12 @@
    /* Enable some SMBus stuff. */
    tmp = pci_read_byte(smbusdev, 0x79);
    tmp |= 0x01;
-	pci_write_byte(smbusdev, 0x79, tmp);
+	rpci_write_byte(smbusdev, 0x79, tmp);

/* Change southbridge. */
    tmp = pci_read_byte(dev, 0x48);
    tmp |= 0x21;
-	pci_write_byte(dev, 0x48, tmp);
+	rpci_write_byte(dev, 0x48, tmp);

/* Now become a bit silly. */
    tmp = INB(0xc6f);
@@ -862,19 +850,19 @@
    /* Set the 0-16 MB enable bits. */
    val = pci_read_byte(dev, 0x88);
    val |= 0xff;		/* 256K */
-	pci_write_byte(dev, 0x88, val);
+	rpci_write_byte(dev, 0x88, val);
    val = pci_read_byte(dev, 0x8c);
    val |= 0xff;		/* 1M */
-	pci_write_byte(dev, 0x8c, val);
+	rpci_write_byte(dev, 0x8c, val);
    wordval = pci_read_word(dev, 0x90);
    wordval |= 0x7fff;	/* 16M */
-	pci_write_word(dev, 0x90, wordval);
+	rpci_write_word(dev, 0x90, wordval);

old = pci_read_byte(dev, 0x6d);
    new = old | 0x01;
    if (new == old)
    	return 0;
-	pci_write_byte(dev, 0x6d, new);
+	rpci_write_byte(dev, 0x6d, new);

if (pci_read_byte(dev, 0x6d) != new) {
    	msg_pinfo("tried to set 0x%x to 0x%x on %s failed (WARNING ONLY)\n", 0x6d, new, name);
@@ -931,7 +919,7 @@
 #if 0
    val |= (1 << 6);
    val &= ~(1 << 5);
-	pci_write_byte(dev, 0x8a, val);
+	rpci_write_byte(dev, 0x8a, val);
 #endif

if (mcp6x_spi_init(want_spi)) {
@@ -954,11 +942,11 @@
    /* Set the 4MB enable bit. */
    val = pci_read_byte(dev, 0x41);
    val |= 0x0e;
-	pci_write_byte(dev, 0x41, val);
+	rpci_write_byte(dev, 0x41, val);

val = pci_read_byte(dev, 0x43);
    val |= (1 << 4);
-	pci_write_byte(dev, 0x43, val);
+	rpci_write_byte(dev, 0x43, val);

return 0;
 }
Index: flashrom-pci_configspace_shutdown_restore/flashrom.c
===================================================================
--- flashrom-pci_configspace_shutdown_restore/flashrom.c	(Revision 1231)
+++ flashrom-pci_configspace_shutdown_restore/flashrom.c	(Arbeitskopie)
@@ -442,7 +442,7 @@
    {}, /* This entry corresponds to PROGRAMMER_INVALID. */
 };

-#define SHUTDOWN_MAXFN 4
+#define SHUTDOWN_MAXFN 32
 static int shutdown_fn_count = 0;
 struct shutdown_func_data {
    void (*func) (void *data);
Index: flashrom-pci_configspace_shutdown_restore/programmer.h
===================================================================
--- flashrom-pci_configspace_shutdown_restore/programmer.h	(Revision 1231)
+++ flashrom-pci_configspace_shutdown_restore/programmer.h	(Arbeitskopie)
@@ -212,6 +212,12 @@
 };
 uint32_t pcidev_validate(struct pci_dev *dev, uint32_t bar, const struct pcidev_status *devs);
 uint32_t pcidev_init(uint16_t vendor_id, uint32_t bar, const struct pcidev_status *devs);
+/* rpci_write_* are reversible writes. The original PCI config space register
+ * contents will be restored on shutdown.
+ */
+int rpci_write_byte(struct pci_dev *dev, int reg, u8 data);
+int rpci_write_word(struct pci_dev *dev, int reg, u16 data);
+int rpci_write_long(struct pci_dev *dev, int reg, u32 data);
 #endif

/* print.c */

[Michael Karcher]

Am Mittwoch, den 10.11.2010, 05:14 +0100 schrieb Carl-Daniel Hailfinger:
> Sorry. It seems my patch generation script autocompleted the wrong filename.
> Send the correct patch this time.
>
> Undo all PCI config space writes on shutdown.
> This means all chipset enables etc. will be undone on shutdown.
> Any writes which are one-shot should use the permanent ppci_write_*
> variants.
>
> Extend the number of available register_shutdown slots to 32.
>
> Signed-off-by: Carl-Daniel Hailfinger

Acked-by: Michael Karcher
> +#define NO_PCI_REDIRECT 1

You do not use this define anywhere, you might want to remove it.

Regards,
  Michael Karcher

[Carl-Daniel Hailfinger]

On 10.11.2010 10:49, Michael Karcher wrote:
> Am Mittwoch, den 10.11.2010, 05:14 +0100 schrieb Carl-Daniel Hailfinger:
> > Sorry. It seems my patch generation script autocompleted the wrong filename.
> > Send the correct patch this time.
> >
> > Undo all PCI config space writes on shutdown.
> > This means all chipset enables etc. will be undone on shutdown.
> > Any writes which are one-shot should use the permanent ppci_write_*
> > variants.
> >
> > Extend the number of available register_shutdown slots to 32.
> >
> > Signed-off-by: Carl-Daniel Hailfinger
>
> Acked-by: Michael Karcher
> > +#define NO_PCI_REDIRECT 1
>
> You do not use this define anywhere, you might want to remove it.

Done. Thanks for your review!

Committed in r1232.

Regards,
Carl-Daniel