IOCHK interface for I/O error handling/detecting

Hi, long time no see :-D

This is a continuation of previous post quite a while ago:
"[PATCH/RFC] I/O-check interface for driver's error handling"

Reflecting every comments, I brushed up my patch for generic part. So today I'll post it again, and also post "ia64 part", which surely implements ia64-arch specific error checking. I think latter will be a sample of basic implement for other arch.

The patch is divided into 10 parts, as patch series:

	iochk-01-generic.patch
	iochk-02-ia64.patch
	iochk-03-register.patch
	iochk-04-register_bridge.patch
	iochk-05-check_bridge.patch
	iochk-06-mcanotify.patch
	iochk-07-poison.patch
	iochk-08-mcadrv.patch
	iochk-09-cpeh.patch
	iochk-10-rwlock.patch

Only "01" is for generic, and rest 9 parts are for ia64. Since parts from 02 to 05 are used to construct basic pci-based checking, they are not so arch-specific even they in /arch/ia64.

I think the generic part is almost completed. No matter if only "01" is accepted, because it will mean a good start for other arch where interested in I/O error recovery infrastructures. Of course, I'd appreciate it if all of them could be accepted.

I feel need of comments, especially against ia64 parts. Every comments are welcome.

Thanks,
H.Seto

-----

* ...followings are "abstract" copied from my previous post. If you know skip it:

-----

Currently, I/O error is not a leading cause of system failure. However, since Linux nowadays is making great progress on its scalability, and ever larger number of PCI devices are being connected to a single high-performance server, the risk of the I/O error is increasing day by day.

For example, PCI parity error is one of the most common errors in the hardware world. However, the major cause of parity error is not hardware's error but software's - low voltage, humidity, natural radiation... etc. Even though, some platforms are nervous to parity error enough to shutdown the system immediately on such error. So if device drivers can retry its transaction once results as an error, we can reduce the risk of I/O errors.

So I'd like to suggest new interfaces that enable drivers to check - detect error and retry their I/O transaction easily.

Previously I had post two prototypes to LKML:
1) readX_check() interface
Added new kin of basic readX(), which returns its result of I/O. But, it would not make sense that device driver have to check and react after each of I/Os.
2) clear/read_pci_errors() interface
Added new pair-interface to sandwich I/Os. It makes sense that device driver can adjust the number of checking I/Os and can react all of them at once. However, this was not generalized, so I thought that more expandable design would be required.

Today's patch is 3rd one - iochk_clear/read() interface.
- This also adds pair-interface, but not to sandwich only readX(). Depends on platform, starting with ioreadX(), inX(), writeX() if possible... and so on could be target of error checking.
- Additionally adds special token - abstract "iocookie" structure to control/identifies/manage I/Os, by passing it to OS. Actual type of "iocookie" could be arch-specific. Device drivers could use the iocookie structure without knowing its detail.

Expected usage(sample) is:
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
#include <linux/pci.h>
#include <asm/io.h>

int sample_read_with_iochk(struct pci_dev *dev, u32 *buf, int words)

{
     unsigned long ofs = pci_resource_start(dev, 0) + DATA_OFFSET;
     int i;

     /* Create magical cookie on the stack */
     iocookie cookie;

     /* Critical section start */
     iochk_clear(&dev, &cookie);
     {
         /* Get the whole packet of data */
         for (i = 0; i < words; i++)
             *buf++ = ioread32(dev, ofs);
     }
     /* Critical section end. Did we have any trouble? */
     if ( iochk_read(&cookie) ) return -1;

     /* OK, all system go. */
     return 0;
}

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

If arch doesn't(or cannot) have its io-checking strategy, these interfaces don't do anything, so driver maintainer can write their driver code with these interfaces for all arch, even where checking is not implemented.

[This is 1 of 10 patches, "iochk-01-generic.patch"]

- It defines:
a pair of function : iochk_clear and iochk_read a function for init : iochk_init
type of control var : iocookie
and describe "no-ops" as its "generic" action.

- HAVE_ARCH_IOMAP_CHECK allows us to change whole definition of these functions and type from generic one to specific one. See next patch (2 of 10).

---

 drivers/pci/pci.c           |    2 ++
 include/asm-generic/iomap.h |   16 ++++++++++++++++
 lib/iomap.c                 |   26 ++++++++++++++++++++++++++
 3 files changed, 44 insertions(+)

Index: linux-2.6.11.11/lib/iomap.c
===================================================================
@@ -210,3 +210,29 @@ void pci_iounmap(struct pci_dev *dev, vo
 }
 EXPORT_SYMBOL(pci_iomap);
 EXPORT_SYMBOL(pci_iounmap);
+
+/*
+ * Clear/Read iocookie to check IO error while using iomap.
+ *
+ * Note that default iochk_clear-read pair interfaces don't have
+ * any effective error check, but some high-reliable platforms
+ * would provide useful information to you.
+ * And note that some action may be limited (ex. irq-unsafe)
+ * between the pair depend on the facility of the platform.
+ */
+#ifndef HAVE_ARCH_IOMAP_CHECK
+void iochk_init(void) { ; }
+
+void iochk_clear(iocookie *cookie, struct pci_dev *dev)
+{
+	/* no-ops */
+}
+
+int iochk_read(iocookie *cookie)
+{
+	/* no-ops */
+	return 0;
+}
+EXPORT_SYMBOL(iochk_clear);
+EXPORT_SYMBOL(iochk_read);
+#endif /* HAVE_ARCH_IOMAP_CHECK */

Index: linux-2.6.11.11/include/asm-generic/iomap.h
===================================================================
@@ -60,4 +60,20 @@ struct pci_dev;
 extern void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long max);
 extern void pci_iounmap(struct pci_dev *dev, void __iomem *);
 
+/*
+ * IOMAP_CHECK provides additional interfaces for drivers to detect
+ * some IO errors, supports drivers having ability to recover errors.
+ *
+ * All works around iomap-check depends on the design of "iocookie"
+ * structure. Every architecture owning its iomap-check is free to
+ * define the actual design of iocookie to fit its special style.
+ */
+#ifndef HAVE_ARCH_IOMAP_CHECK
+typedef unsigned long iocookie;
+#endif
+
+extern void iochk_init(void);
+extern void iochk_clear(iocookie *cookie, struct pci_dev *dev);
+extern int  iochk_read(iocookie *cookie);
+
 #endif

Index: linux-2.6.11.11/drivers/pci/pci.c
===================================================================
@@ -782,6 +782,8 @@ static int __devinit pci_init(void)
 	while ((dev = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, dev)) != NULL) {
 		pci_fixup_device(pci_fixup_final, dev);
 	}
+
+	iochk_init();
 	return 0;
 }
 

Hi Greg,

Thank you for giving me many useful advices!

I intended to emphasize the pair. I'll unify them if not needed.

In this time, no one initializes the cookie, so I just ignored it.

I'm not used to inlining...
In case of generic definition above, absolutely it should be inlined. OK, I'll try.

Yea.

Because I stuck to have short name alias, and wanted to hide even whether it is struct or not.

Thanks,
H.Seto

[This is 2 of 10 patches, "iochk-02-ia64.patch"]

- Add "config IOMAP_CHECK" to change definitions from generic to specific.

- Defines ia64 version of:
iochk_clear, iochk_read, iochk_init, and iocookie
But they are no-ops yet. See next patch (3 of 10).

---

 arch/ia64/Kconfig           |   13 +++++++++++++
 arch/ia64/lib/Makefile      |    1 +
 arch/ia64/lib/iomap_check.c |   30 ++++++++++++++++++++++++++++++
 include/asm-ia64/io.h       |   11 +++++++++++
 4 files changed, 55 insertions(+)

Index: linux-2.6.11.11/arch/ia64/lib/Makefile
===================================================================
@@ -16,6 +16,7 @@ lib-$(CONFIG_MCKINLEY)	+= copy_page_mck.
 lib-$(CONFIG_PERFMON)	+= carta_random.o
 lib-$(CONFIG_MD_RAID5)	+= xor.o
 lib-$(CONFIG_HAVE_DEC_LOCK) += dec_and_lock.o
+lib-$(CONFIG_IOMAP_CHECK) += iomap_check.o
 
 AFLAGS___divdi3.o	=
 AFLAGS___udivdi3.o	= -DUNSIGNED

Index: linux-2.6.11.11/arch/ia64/Kconfig
===================================================================
@@ -381,6 +381,19 @@ config PCI_DOMAINS
 	bool
 	default PCI
 
+config IOMAP_CHECK
+	bool "Support iochk interfaces for IO error detection."
+	depends on PCI && EXPERIMENTAL
+	---help---
+	  Saying Y provides iochk infrastructure for "RAS-aware" drivers
+	  to detect and recover some IO errors, which strongly required by
+	  some of very-high-reliable systems.
+	  The implementation of this infrastructure is highly depend on arch,
+	  bus system, chipset and so on.
+	  Currentry, very few drivers on few arch actually implements this.
+
+	  If you don't know what to do here, say N.
+
 source "drivers/pci/Kconfig"
 
 source "drivers/pci/hotplug/Kconfig"

Index: linux-2.6.11.11/arch/ia64/lib/iomap_check.c
===================================================================
@@ -0,0 +1,30 @@
+/*
+ * File:    iomap_check.c
+ * Purpose: Implement the IA64 specific iomap recovery interfaces
+ */
+
+#include <linux/pci.h>
+
+void iochk_init(void);
+void iochk_clear(iocookie *cookie, struct pci_dev *dev);
+int  iochk_read(iocookie *cookie);
+
+void iochk_init(void)
+{
+	/* setup */
+}
+
+void iochk_clear(iocookie *cookie, struct pci_dev *dev)
+{
+	/* register device etc. */
+}
+
+int iochk_read(iocookie *cookie)
+{
+	/* check error etc. */
+
+	return 0;
+}
+
+EXPORT_SYMBOL(iochk_read);
+EXPORT_SYMBOL(iochk_clear);

Index: linux-2.6.11.11/include/asm-ia64/io.h
===================================================================
@@ -70,6 +70,17 @@ extern unsigned int num_io_spaces;
 #include <asm/machvec.h>
 #include <asm/page.h>
 #include <asm/system.h>
+
+#ifdef CONFIG_IOMAP_CHECK
+
+/* definition of ia64 iocookie */
+typedef unsigned long iocookie;
+
+/* enable ia64 iochk - See arch/ia64/lib/iomap_check.c */
+#define HAVE_ARCH_IOMAP_CHECK
+
+#endif /* CONFIG_IOMAP_CHECK  */
+
 #include <asm-generic/iomap.h>
 
 /*

[This is 3 of 10 patches, "iochk-03-register.patch"]

- Implement ia64 version of basic codes:
iochk_clear, iochk_read, iochk_init, and iocookie

The direction is:

- Have a "now in check" global list, "iochk_devices", for future use.

- Take a lock, "iochk_lock", to protect the global list.

- iochk_clear packs *dev into iocookie, and add it to the global list. After all prepared, clear error-flag in cookie to start io-critical-session.

- iochk_read checks error-flag and device's status register. After removing iocookie from list, return the result.

This is too simple. We need more codes... See next (4 of 10).

---

  arch/ia64/lib/iomap_check.c |   54 ++++++++++++++++++++++++++++++++++++++++++--
  include/asm-ia64/io.h       |    8 +++++-
  2 files changed, 59 insertions(+), 3 deletions(-)

Index: linux-2.6.11.11/arch/ia64/lib/iomap_check.c =================================--- linux-2.6.11.11.orig/arch/ia64/lib/iomap_check.c +++ linux-2.6.11.11/arch/ia64/lib/iomap_check.c
@@ -4,24 +4,74 @@
*/

#include <linux/pci.h>
+#include <linux/list.h>
+#include <linux/spinlock.h>

  void iochk_init(void);
  void iochk_clear(iocookie *cookie, struct pci_dev *dev);
  int  iochk_read(iocookie *cookie);

+struct list_head iochk_devices;
+DEFINE_SPINLOCK(iochk_lock); /* all works are excluded on this lock */
+
+static int have_error(struct pci_dev *dev);
+
void iochk_init(void)
{
/* setup */
+ INIT_LIST_HEAD(&iochk_devices);
}

void iochk_clear(iocookie *cookie, struct pci_dev *dev) {
- /* register device etc. */
+ unsigned long flag;
+
+ INIT_LIST_HEAD(&(cookie->list));
+
+ cookie->dev = dev;
+
+ spin_lock_irqsave(&iochk_lock, flag);
+ list_add(&cookie->list, &iochk_devices);
+ spin_unlock_irqrestore(&iochk_lock, flag);
+
+ cookie->error = 0;
}

int iochk_read(iocookie *cookie)
{
- /* check error etc. */
+ unsigned long flag;
+ int ret = 0;
+
+ spin_lock_irqsave(&iochk_lock, flag);
+ if( cookie->error || have_error(cookie->dev) )
+ ret = 1;
+ list_del(&cookie->list);
+ spin_unlock_irqrestore(&iochk_lock, flag);
+
+ return ret;
+}
+
+static int have_error(struct pci_dev *dev)
+{
+ u16 status;
+
+ /* check status */
+ switch (dev->hdr_type) {
+ case PCI_HEADER_TYPE_NORMAL: /* 0 */
+ pci_read_config_word(dev, PCI_STATUS, &status);
+ break;
+ case PCI_HEADER_TYPE_BRIDGE: /* 1 */
+ pci_read_config_word(dev, PCI_SEC_STATUS, &status);
+ break;
+ case PCI_HEADER_TYPE_CARDBUS: /* 2 */
+ default:
+ BUG();
+ }
+
+ if ( (status & PCI_STATUS_REC_TARGET_ABORT)

+		|| (status & PCI_STATUS_REC_MASTER_ABORT)
+		|| (status & PCI_STATUS_DETECTED_PARITY) )

+ return 1;

return 0;
}
Index: linux-2.6.11.11/include/asm-ia64/io.h =================================--- linux-2.6.11.11.orig/include/asm-ia64/io.h +++ linux-2.6.11.11/include/asm-ia64/io.h
@@ -72,9 +72,15 @@ extern unsigned int num_io_spaces;
#include <asm/system.h>

#ifdef CONFIG_IOMAP_CHECK
+#include <linux/list.h>

/* definition of ia64 iocookie */
-typedef unsigned long iocookie;
+struct __iocookie {
+ struct list_head list;
+ struct pci_dev *dev; /* targeting device */
+ unsigned long error; /* error flag */
+};
+typedef struct __iocookie iocookie;

  /* enable ia64 iochk - See arch/ia64/lib/iomap_check.c */
  #define HAVE_ARCH_IOMAP_CHECK

Aha.., maybe I understand it just now.
I don't know why, but I just stuck to typedef...

Thanks,
H.Seto

Sure, you are right. I'll fix it.

Thanks,
H.Seto

[This is 4 of 10 patches, "iochk-04-register_bridge.patch"]

- Since there could be a (PCI-)bus-error, some kind of error cannot detected on the device but on its hosting bridge. So, it is also required to check the bridge's register.

In other words, to check a bus-error correctly, we need to check both end of the bus, device and its host bridge.

OK, but often bridges are shared by multiple devices, right? So we need care to handle it... Yes, see next (5 of 10).

---

 arch/ia64/lib/iomap_check.c |   19 ++++++++++++++++++-
 include/asm-ia64/io.h       |    1 +
 2 files changed, 19 insertions(+), 1 deletion(-)

Index: linux-2.6.11.11/arch/ia64/lib/iomap_check.c
===================================================================
@@ -14,6 +14,7 @@ int  iochk_read(iocookie *cookie);
 struct list_head iochk_devices;
 DEFINE_SPINLOCK(iochk_lock);	/* all works are excluded on this lock */
 
+static struct pci_dev *search_host_bridge(struct pci_dev *dev);
 static int have_error(struct pci_dev *dev);
 
 void iochk_init(void)

@@ -29,6 +30,7 @@ void iochk_clear(iocookie *cookie, struc
 	INIT_LIST_HEAD(&(cookie->list));
 
 	cookie->dev = dev;
+	cookie->host = search_host_bridge(dev);
 
 	spin_lock_irqsave(&iochk_lock, flag);
 	list_add(&cookie->list, &iochk_devices);

@@ -43,7 +45,8 @@ int iochk_read(iocookie *cookie)
 	int ret = 0;
 
 	spin_lock_irqsave(&iochk_lock, flag);
-	if( cookie->error || have_error(cookie->dev) )
+	if( cookie->error || have_error(cookie->dev)
+		|| (cookie->host && have_error(cookie->host)) )
 		ret = 1;
 	list_del(&cookie->list);
 	spin_unlock_irqrestore(&iochk_lock, flag);

@@ -51,6 +54,20 @@ int iochk_read(iocookie *cookie)
 	return ret;
 }
 
+struct pci_dev *search_host_bridge(struct pci_dev *dev)
+{
+	struct pci_bus *pbus;
+
+	/* there is no bridge */
+	if (!dev->bus->self) return NULL;
+
+	/* find root bus bridge */
+	for (pbus = dev->bus; pbus->parent && pbus->parent->self;
+		pbus = pbus->parent);
+
+	return pbus->self;
+}
+
 static int have_error(struct pci_dev *dev)
 {
 	u16 status;

Index: linux-2.6.11.11/include/asm-ia64/io.h
===================================================================
@@ -78,6 +78,7 @@ extern unsigned int num_io_spaces;
 struct __iocookie {
 	struct list_head	list;
 	struct pci_dev		*dev;	/* targeting device */
+	struct pci_dev		*host;	/* hosting bridge */
 	unsigned long		error;	/* error flag */
 };
 typedef struct __iocookie iocookie;

[This is 5 of 10 patches, "iochk-05-check_bridge.patch"]

- Consider three devices, A, B, and C are placed under a same host bridge H. After A and B checked-in (=passed iochk_clear, doing some I/Os, not come to call iochk_read yet), now C is going to check-in, just entered iochk_clear, but C finds out that H indicates error.

It means that A or B hits a bus error, but there is no data which one actually hits the error. So, C should notify the error to both of A and B, and clear the H's status to start its own I/Os.

If there are only two devices, it become more simple. It is clear if one find a bridge error while another is check-in, the error is nothing except for another's.

Well, works concerning registers (devices and bridges) are almost shaped up. So, from next, I'll move to deep phase to implement more arch-specific codes... see next (6 of 10).

---

  arch/ia64/lib/iomap_check.c |   45 ++++++++++++++++++++++++++++++++++++++++++++
  1 files changed, 45 insertions(+)

Index: linux-2.6.11.11/arch/ia64/lib/iomap_check.c =================================--- linux-2.6.11.11.orig/arch/ia64/lib/iomap_check.c +++ linux-2.6.11.11/arch/ia64/lib/iomap_check.c
@@ -17,6 +17,9 @@ DEFINE_SPINLOCK(iochk_lock); /* all work
static struct pci_dev *search_host_bridge(struct pci_dev *dev); static int have_error(struct pci_dev *dev);

+void notify_bridge_error(struct pci_dev *bridge); +void clear_bridge_error(struct pci_dev *bridge); +
void iochk_init(void)
{
/* setup */
@@ -33,6 +36,11 @@ void iochk_clear(iocookie *cookie, struc
cookie->host = search_host_bridge(dev);

spin_lock_irqsave(&iochk_lock, flag);
+ if(cookie->host && have_error(cookie->host)) {
+ /* someone under my bridge causes error... */
+ notify_bridge_error(cookie->host);
+ clear_bridge_error(cookie->host);
+ }
list_add(&cookie->list, &iochk_devices); spin_unlock_irqrestore(&iochk_lock, flag);

@@ -93,5 +101,42 @@ static int have_error(struct pci_dev *de
return 0;
}

+void notify_bridge_error(struct pci_dev *bridge)
+{
+ iocookie *cookie;
+
+ if (list_empty(&iochk_devices))
+ return;
+
+ /* notify error to all transactions using this host bridge */
+ if (bridge) {
+ /* local notify, ex. Parity, Abort etc. */
+ list_for_each_entry(cookie, &iochk_devices, list) {
+ if (cookie->host = bridge)
+ cookie->error = 1;
+ }
+ }
+}
+
+void clear_bridge_error(struct pci_dev *bridge)
+{
+ u16 status = ( PCI_STATUS_REC_TARGET_ABORT
+ | PCI_STATUS_REC_MASTER_ABORT
+ | PCI_STATUS_DETECTED_PARITY );
+
+ /* clear bridge status */
+ switch (bridge->hdr_type) {
+ case PCI_HEADER_TYPE_NORMAL: /* 0 */
+ pci_write_config_word(bridge, PCI_STATUS, status);
+ break;
+ case PCI_HEADER_TYPE_BRIDGE: /* 1 */
+ pci_write_config_word(bridge, PCI_SEC_STATUS, status);
+ break;
+ case PCI_HEADER_TYPE_CARDBUS: /* 2 */
+ default:
+ BUG();
+ }
+}
+
EXPORT_SYMBOL(iochk_read);
EXPORT_SYMBOL(iochk_clear);

[This is 6 of 10 patches, "iochk-06-mcanotify.patch"]

- This is a headache:
When ia64 get a problem on hardware, OS could request SAL(System Abstraction Layer: ia64 firmware) to gather system status via calling SAL_GET_STATE_INFO procedure.

However (depend on implementation of SAL for its platform, hopefully), on the way of gathering, SAL also checks every host bridges and its status, and after that, resets the state...

So we should take care of this reset by SAL.

Handling MCA(Machine Check Abort) is one of a situation should we take care. Originally MCA is designed as a critical interruption, so when MCA comes, without OS's order, SAL gathers system status before OS gets its control. So since states of bridges are already reset on entrance of MCA, OS should notify "lost of state" to all "check-in" contexts, by marking its error flag, iocookie->error.

There would be better way if OS can know the bridge state from data which SAL gathered, but in the meanwhile, I just do simple way.

PCI-parity error is one of MCA causes, is it OK? Next, "data poisoning" helps us... see next (7 of 10).

---

  arch/ia64/kernel/mca.c      |   13 +++++++++++++
  arch/ia64/lib/iomap_check.c |    7 ++++++-
  2 files changed, 19 insertions(+), 1 deletion(-)

Index: linux-2.6.11.11/arch/ia64/kernel/mca.c =================================--- linux-2.6.11.11.orig/arch/ia64/kernel/mca.c +++ linux-2.6.11.11/arch/ia64/kernel/mca.c
@@ -77,6 +77,11 @@
#include <asm/irq.h>
#include <asm/hw_irq.h>

+#ifdef CONFIG_IOMAP_CHECK
+#include <linux/pci.h>
+extern void notify_bridge_error(struct pci_dev *bridge); +#endif
+
#if defined(IA64_MCA_DEBUG_INFO)

  # define IA64_MCA_DEBUG(fmt...)	printk(fmt)
  #else
@@ -893,6 +898,14 @@ ia64_mca_ucmc_handler(void)
  		sal_log_record_header_t *rh = IA64_LOG_CURR_BUFFER(SAL_INFO_TYPE_MCA);
  		rh->severity = sal_log_severity_corrected;
  		ia64_sal_clear_state_info(SAL_INFO_TYPE_MCA);
+
+#ifdef CONFIG_IOMAP_CHECK
+		/*
+		 * SAL already reads and clears error bits on bridge registers,
+		 * so we should have all running transactions to retry.
+		 */
+		notify_bridge_error(0);
+#endif
  	}
  	/*
  	 *  Wakeup all the processors which are spinning in the rendezvous
Index: linux-2.6.11.11/arch/ia64/lib/iomap_check.c
=================================--- linux-2.6.11.11.orig/arch/ia64/lib/iomap_check.c
+++ linux-2.6.11.11/arch/ia64/lib/iomap_check.c
@@ -109,7 +109,12 @@ void notify_bridge_error(struct pci_dev
  		return;

/* notify error to all transactions using this host bridge */
- if (bridge) {
+ if (!bridge) {
+ /* global notify, ex. MCA */
+ list_for_each_entry(cookie, &iochk_devices, list) {
+ cookie->error = 1;
+ }
+ } else {
/* local notify, ex. Parity, Abort etc. */ list_for_each_entry(cookie, &iochk_devices, list) {
if (cookie->host = bridge)

[This is 7 of 10 patches, "iochk-07-poison.patch"]

- When bus-error occur on write, write data is broken on the bus, so target device gets broken data.

There are 2 way for such device to take:
- send PERR(Parity Error) to host, expecting immediate panic.
- mark status register as error, expecting its driver to read it and decide to retry.

So it is not difficult for drivers to recover from error on write if it can take latter way, and if it don't worry about taking time to wait completion of write.

- When bus-error occur on read, read data is broken on the bus, so host bridge gets broken data.

There are 2 way for such bridge to take:
- send BERR(Bus Error) to host, expecting immediate panic.
- mark data as "poisoned" and throw it to destination, expecting panic if system touched it but cannot stop data pollution.

Former is traditional way, latter is modern way, called "data poisoning". The important difference is whether OS can get a chance to recover from the error. Usually, sending BERR doesn't tell us "where it comes", "who it orders", so we cannot do anything except panic. In the other hand, poisoned data will reach its destination and will cause a error on there again. Yes, destination is "where who lives".

Well, the idea is quite simple:
"driver checks read data, and recover if it was poisoned."

Checking all read at once (ex. take a memo of all read addresses touched after iochk_clear and check them all in iochk_read) does not make sense. Practical way is check each read, keep its result, and read it at end.

Touching poisoned data become a MCA, so now it directly means a system down. But since the MCA tells us "where it happens", we can recover it...? All right, let's see next (8 of 10).

---

  include/asm-ia64/io.h |  110 ++++++++++++++++++++++++++++++++++++++++++++++++++
  1 files changed, 110 insertions(+)

Index: linux-2.6.11.11/include/asm-ia64/io.h =================================--- linux-2.6.11.11.orig/include/asm-ia64/io.h +++ linux-2.6.11.11/include/asm-ia64/io.h
@@ -86,6 +86,20 @@ typedef struct __iocookie iocookie;
/* enable ia64 iochk - See arch/ia64/lib/iomap_check.c */ #define HAVE_ARCH_IOMAP_CHECK

+/*
+ * Some I/O bridges may poison the data read, instead of
+ * signaling a BERR. The consummation of poisoned data
+ * triggers a local, imprecise MCA.
+ * Note that the read operation by itself does not consume
+ * the bad data, you have to do something with it, e.g.:
+ *
+ * ld.8 r9=[r10];; // r10 = I/O address
+ * add.8 r8=r9,r9;; // fake operation
+ */
+#define ia64_poison_check(val) \
+{ register unsigned long gr8 asm("r8"); \
+ asm volatile ("add %0=%1,r0" : "=r"(gr8) : "r"(val)); }
+
#endif /* CONFIG_IOMAP_CHECK */

#include <asm-generic/iomap.h>
@@ -190,6 +204,8 @@ __ia64_mk_io_addr (unsigned long port)
* during optimization, which is why we use "volatile" pointers. */

+#ifdef CONFIG_IOMAP_CHECK
+
static inline unsigned int
___ia64_inb (unsigned long port)
{
@@ -198,6 +214,8 @@ ___ia64_inb (unsigned long port)

ret = *addr;
__ia64_mf_a();
+ ia64_poison_check(ret);
+
return ret;
}

@@ -209,6 +227,8 @@ ___ia64_inw (unsigned long port)

ret = *addr;
__ia64_mf_a();
+ ia64_poison_check(ret);
+
return ret;
}

@@ -220,9 +240,48 @@ ___ia64_inl (unsigned long port)

ret = *addr;
__ia64_mf_a();
+ ia64_poison_check(ret);
+
+ return ret;
+}
+
+#else /* CONFIG_IOMAP_CHECK */
+
+static inline unsigned int
+___ia64_inb (unsigned long port)
+{
+ volatile unsigned char *addr = __ia64_mk_io_addr(port);
+ unsigned char ret;
+
+ ret = *addr;
+ __ia64_mf_a();
return ret;
}

+static inline unsigned int
+___ia64_inw (unsigned long port)
+{
+ volatile unsigned short *addr = __ia64_mk_io_addr(port);
+ unsigned short ret;
+
+ ret = *addr;
+ __ia64_mf_a();
+ return ret;
+}
+
+static inline unsigned int
+___ia64_inl (unsigned long port)
+{
+ volatile unsigned int *addr = __ia64_mk_io_addr(port);
+ unsigned int ret;
+
+ ret = *addr;
+ __ia64_mf_a();
+ return ret;
+}
+
+#endif /* CONFIG_IOMAP_CHECK */
+
static inline void
___ia64_outb (unsigned char val, unsigned long port)
{
@@ -339,6 +398,55 @@ __outsl (unsigned long port, const void
* a good idea). Writes are ok though for all existing ia64 platforms (and
* hopefully it'll stay that way).
*/
+
+#ifdef CONFIG_IOMAP_CHECK
+
+static inline unsigned char
+___ia64_readb (const volatile void __iomem *addr)
+{
+ unsigned char val;
+
+ val = *(volatile unsigned char __force *)addr;
+ ia64_poison_check(val);
+
+ return val;
+}
+
+static inline unsigned short
+___ia64_readw (const volatile void __iomem *addr)
+{
+ unsigned short val;
+
+ val = *(volatile unsigned short __force *)addr;
+ ia64_poison_check(val);
+
+ return val;
+}
+
+static inline unsigned int
+___ia64_readl (const volatile void __iomem *addr)
+{
+ unsigned int val;
+
+ val = *(volatile unsigned int __force *) addr;
+ ia64_poison_check(val);
+
+ return val;
+}
+
+static inline unsigned long
+___ia64_readq (const volatile void __iomem *addr)
+{
+ unsigned long val;
+
+ val = *(volatile unsigned long __force *) addr;
+ ia64_poison_check(val);
+
+ return val;
+}
+
+#else /* CONFIG_IOMAP_CHECK */
+
static inline unsigned char
___ia64_readb (const volatile void __iomem *addr)
{
@@ -363,6 +471,8 @@ ___ia64_readq (const volatile void __iom
return *(volatile unsigned long __force *) addr;
}

+#endif /* CONFIG_IOMAP_CHECK */
+
static inline void
__writeb (unsigned char val, volatile void __iomem *addr) {

Hidetoshi,

Hidetoshi> +/*
Hidetoshi> + * Some I/O bridges may poison the data read, instead of Hidetoshi> + * signaling a BERR. The consummation of poisoned data Hidetoshi> + * triggers a local, imprecise MCA.
Hidetoshi> + * Note that the read operation by itself does not consume Hidetoshi> + * the bad data, you have to do something with it, e.g.:
Hidetoshi> + *
Hidetoshi> + * ld.8 r9=[r10];; // r10 = I/O address
Hidetoshi> + * add.8 r8=r9,r9;; // fake operation
Hidetoshi> + */
Hidetoshi> +#define ia64_poison_check(val) \
Hidetoshi> +{ register unsigned long gr8 asm("r8"); \
Hidetoshi> + asm volatile ("add %0=%1,r0" : "=r"(gr8) : "r"(val)); }
Hidetoshi> +
Hidetoshi> #endif /* CONFIG_IOMAP_CHECK */

I have only looked that this briefly and I didn't see off hand where you get the "r9=[r10]" sequence from --- I hope you're not relying on the compiler happening to generate this sequence!

More importantly: please avoid inline "asm" and use the intrinsics defined by gcc_intrin.h instead (if you need something new, we can add that), but I think ia64_getreg() will do much of what you want already.

Thanks,

	--david

Hi David,

+static inline unsigned char +___ia64_readb (const volatile void __iomem *addr) +{
+ unsigned char val;
+
+ val = *(volatile unsigned char __force *)addr;
+ ia64_poison_check(val);
+
+ return val;
+}

Assigning value from addr to variable val stands for "ld", is it right? What I want to do here is making sure that ld actually finishs loading data from memory or mmaped register or far place to general register, and make sure that the data is healthy enough to operate, not poisoned.

Umm, I think I need something like ia64_setreg(ANYWHERE_DUMMY_REG, val). How do you think?

Or don't you mind if I move the definition of ia64_poison_check above to gcc_intrin.h?

Thanks,
H.Seto

Hidetoshi> Hi David,

Hidetoshi> +/*
Hidetoshi> + * Some I/O bridges may poison the data read, instead of Hidetoshi> + * signaling a BERR. The consummation of poisoned data Hidetoshi> + * triggers a local, imprecise MCA.
Hidetoshi> + * Note that the read operation by itself does not consume Hidetoshi> + * the bad data, you have to do something with it, e.g.:
Hidetoshi> + *
Hidetoshi> + * ld.8 r9=[r10];; // r10 = I/O address
Hidetoshi> + * add.8 r8=r9,r9;; // fake operation
Hidetoshi> + */
Hidetoshi> +#define ia64_poison_check(val) \
Hidetoshi> +{ register unsigned long gr8 asm("r8"); \
Hidetoshi> + asm volatile ("add %0=%1,r0" : "=r"(gr8) : "r"(val)); }
Hidetoshi> +
Hidetoshi> #endif /* CONFIG_IOMAP_CHECK */

  Hidetoshi> +static inline unsigned char
  Hidetoshi> +___ia64_readb (const volatile void __iomem *addr)
  Hidetoshi> +{
  Hidetoshi> +    unsigned char val;
  Hidetoshi> +
  Hidetoshi> +    val = *(volatile unsigned char __force *)addr;
  Hidetoshi> +    ia64_poison_check(val);
  Hidetoshi> +
  Hidetoshi> +    return val;
  Hidetoshi> +}

Ah, I see now what you're trying to do. I think it's really a machine-check barrier that you want there.

I'm doubtful whether this is the right approach, though: your ia64_poison_check() will cause _every single_ readX() operation to stall the CPU for 1,000+ cycles. Why not define an explicit iochk_barrier() instead? Then you could do things like this:

	a = readb(X);
	b = readb(Y);
	c = readb(Z);
	iochk_barrier(a + b + c);

That is, if it's unimportant to know whether the read of X, Y, or Z caused the MCA, you can amortize the cost of iochk_barrier() over 3 reads.

I'd probably make iochk_barrier() an out-of-line no-op assembly routine. The cost of two branches compared to stalling for hundreds of cycles is rather trivial.

	--david

Yes, thanks for your understanding.

I'm also doubtful, I know it too costs... but I don't have any other better idea. As far as I can figure out, using iochk_barrier() style has difficulty like that:
- pain for driver maintainers.
They should be careful to make exact argument for barrier.
- arch-specific.
It will go against the spirit of iochk, "generic" interface.
- unenforceable.
You could forget it.
- it would be in form:

   {
      iocookie cookie;
      iochk_clear(cookie, dev);
      for(i=0;i<count;i++)
	 buf++ = readb(addr);
      iochk_barrier(***);
      iochk_read(cookie);

}
so it seems that iochk_barrier() is kind of something that should be include in iochk_read(), but it would be a difficult hack.

In case of ppc64, their eeh_readX() already have nervous error check, by comparing its result to -1 in every single operation. In fact, I didn't mind the cost so seriously because there is a precedent.

However, I think such cycle-saving would be possible in "string version", such as ioreadX_rep(). I'd like to postpone it as a future optimization.

Of course I agree to have such routine in proper header file, but it would not help us to save CPU cycles if we don't have any other idea... Or I'll just replace ia64_poison_check() to ia64_mca_barrier() or so.

Thanks,
H.Seto

[This is 8 of 10 patches, "iochk-08-mcadrv.patch"]

- Touching poisoned data become a MCA, so now it assumed as a fatal error, directly will be a system down. But since the MCA tells us a physical address - "where it happens", we can do some action to survive.

If the address is present in resource of "check-in" device, it is guaranteed that its driver will call iochk_read in the very near future, and that now the driver have a ability and responsibility of recovery from the error.

So if it was "check-in" address, what OS should do is mark "check-in" devices and just restart usual works. Soon the driver will notice the error and operate it properly.

Note:
We can identify a affected device, but because of SAL behavior (mentioned at 6 of 10), we need to mark all "check-in" devices. Fix in future, if possible.

---

  arch/ia64/kernel/mca_drv.c  |   85 ++++++++++++++++++++++++++++++++++++++++++++
  arch/ia64/lib/iomap_check.c |    1
  2 files changed, 86 insertions(+)

Index: linux-2.6.11.11/arch/ia64/lib/iomap_check.c =================================--- linux-2.6.11.11.orig/arch/ia64/lib/iomap_check.c +++ linux-2.6.11.11/arch/ia64/lib/iomap_check.c
@@ -145,3 +145,4 @@ void clear_bridge_error(struct pci_dev *

EXPORT_SYMBOL(iochk_read);
EXPORT_SYMBOL(iochk_clear);
+EXPORT_SYMBOL(iochk_devices); /* for MCA driver */
Index: linux-2.6.11.11/arch/ia64/kernel/mca_drv.c =================================--- linux-2.6.11.11.orig/arch/ia64/kernel/mca_drv.c +++ linux-2.6.11.11/arch/ia64/kernel/mca_drv.c
@@ -35,6 +35,13 @@

  #include "mca_drv.h"

+#ifdef CONFIG_IOMAP_CHECK
+#include <linux/pci.h>
+#include <asm/io.h>
+extern struct list_head iochk_devices;
+#endif
+
+
/* max size of SAL error record (default) */
static int sal_rec_max = 10000;

@@ -378,6 +385,80 @@ is_mca_global(peidx_table_t *peidx, pal_
return MCA_IS_GLOBAL;
}

+
+#ifdef CONFIG_IOMAP_CHECK
+
+/**
+ * get_target_identifier - get address of target_identifier
+ * @peidx: pointer of index of processor error section
+ *
+ * Return value:
+ * addr if valid / 0 if not valid
+ */
+static u64 get_target_identifier(peidx_table_t *peidx)
+{
+ sal_log_mod_error_info_t *smei;
+
+ smei = peidx_bus_check(peidx, 0);
+ if (smei->valid.target_identifier)
+ return (smei->target_identifier);
+ return 0;
+}
+
+/**
+ * offending_addr_in_check - Check if the addr is in checking resource.
+ * @addr: address offending this MCA
+ *
+ * Return value:
+ * 1 if in / 0 if out
+ */
+static int offending_addr_in_check(u64 addr)
+{
+ int i;
+ struct pci_dev *tdev;
+ iocookie *cookie;
+
+ if (list_empty(&iochk_devices))
+ return 0;
+
+ list_for_each_entry(cookie, &iochk_devices, list) {
+ tdev = cookie->dev;
+ for (i = 0; i < PCI_ROM_RESOURCE; i++) {
+ if (tdev->resource[i].start <= addr
+ && addr <= tdev->resource[i].end)
+ return 1;
+ if ((tdev->resource[i].flags
+ & (PCI_BASE_ADDRESS_SPACE|PCI_BASE_ADDRESS_MEM_TYPE_MASK))
+ = (PCI_BASE_ADDRESS_SPACE_MEMORY|PCI_BASE_ADDRESS_MEM_TYPE_64))
+ i++;
+ }
+ }
+ return 0;
+}
+
+/**
+ * pci_error_recovery - Check if MCA occur on transaction in iochk.
+ * @peidx: pointer of index of processor error section
+ *
+ * Return value:
+ * 1 if error could be cought in driver / 0 if not
+ */
+static int pci_error_recovery(peidx_table_t *peidx)
+{
+ u64 addr;
+
+ addr = get_target_identifier(peidx);
+ if(!addr) return 0;
+
+ if(offending_addr_in_check(addr))
+ return 1;
+
+ return 0;
+}
+
+#endif /* CONFIG_IOMAP_CHECK */
+
+
/**
* recover_from_read_error - Try to recover the errors which type are "read"s.
* @slidx: pointer of index of SAL error record
@@ -400,6 +481,10 @@ recover_from_read_error(slidx_table_t *s
if (!pbci->tv)
return 0;

+#ifdef CONFIG_IOMAP_CHECK
+ if ( pci_error_recovery(peidx) ) return 1;
+#endif
+
/*
* cpu read or memory-mapped io read
*

[This is 9 of 10 patches, "iochk-09-cpeh.patch"]

- SAL behavior doesn't affect only MCA. There are other chances to call SAL_GET_STATE_INFO, that's when CMC, CPE, and INIT is happen.

- CMC(Corrected Machine Check) is for non-fatal, processor local errors. Fortunately, calling SAL_GET_STATE_INFO for CMC only collect data from a processor issued it, without touching any bridge and its status. So, this is safe.

- CPE(Corrected Platform Error) is for non-fatal, platform related errors. Even it says corrected, but calling SAL procedure for CPE touchs every bridge on the platform, and "correct" bridge status that's bad for iochk works.

- INIT is a kind of system reset request, as far as I know. So restarting from INIT is out of design, also iochk after INIT is not required at this time.

In short, only MCA and CPE have the problem of SAL behavior. One of the difference from MCA is that SAL will not gather data before OS actually request it.

MCA:
1) SAL gathers data and keep it internally
2) OS gets control
3) if OS requests, SAL returns data gathered at beginning.
CPE:
1) OS gets control
2) OS request to SAL
3) SAL gathers data and return it to OS

Therefore, we can make CPE handler to care bridge states, to check states before calling SAL procedure.

---

  arch/ia64/kernel/mca.c      |   21 +++++++++++++++++++++
  arch/ia64/lib/iomap_check.c |   17 +++++++++++++++++
  2 files changed, 38 insertions(+)

Index: linux-2.6.11.11/arch/ia64/lib/iomap_check.c =================================--- linux-2.6.11.11.orig/arch/ia64/lib/iomap_check.c +++ linux-2.6.11.11/arch/ia64/lib/iomap_check.c
@@ -19,6 +19,7 @@ static int have_error(struct pci_dev *de

void notify_bridge_error(struct pci_dev *bridge); void clear_bridge_error(struct pci_dev *bridge);
+void save_bridge_error(void);

void iochk_init(void)
{
@@ -143,6 +144,22 @@ void clear_bridge_error(struct pci_dev *
}
}

+void save_bridge_error(void)
+{
+ iocookie *cookie;
+
+ if (list_empty(&iochk_devices))
+ return;
+
+ /* mark devices if its root bus bridge have errors */
+ list_for_each_entry(cookie, &iochk_devices, list) {
+ if (cookie->error)
+ continue;
+ if (have_error(cookie->host))
+ notify_bridge_error(cookie->host);
+ }
+}
+
EXPORT_SYMBOL(iochk_read);
EXPORT_SYMBOL(iochk_clear);
EXPORT_SYMBOL(iochk_devices); /* for MCA driver */
Index: linux-2.6.11.11/arch/ia64/kernel/mca.c =================================--- linux-2.6.11.11.orig/arch/ia64/kernel/mca.c +++ linux-2.6.11.11/arch/ia64/kernel/mca.c
@@ -80,6 +80,8 @@
#ifdef CONFIG_IOMAP_CHECK
#include <linux/pci.h>
extern void notify_bridge_error(struct pci_dev *bridge);
+extern void save_bridge_error(void);
+extern spinlock_t iochk_lock;
#endif

#if defined(IA64_MCA_DEBUG_INFO)
@@ -288,11 +290,30 @@ ia64_mca_cpe_int_handler (int cpe_irq, v
IA64_MCA_DEBUG("%s: received interrupt vector = %#x on CPU %d\n",
__FUNCTION__, cpe_irq, smp_processor_id());

+#ifndef CONFIG_IOMAP_CHECK
+
/* SAL spec states this should run w/ interrupts enabled */ local_irq_enable();

/* Get the CPE error record and log it */ ia64_mca_log_sal_error_record(SAL_INFO_TYPE_CPE);
+#else
+ /*
+ * Because SAL_GET_STATE_INFO for CPE might clear bridge states
+ * in process of gathering error information from the system,
+ * we should check the states before clearing it.
+ * While OS and SAL are handling bridge status, we have to protect
+ * the states from changing by any other I/Os running simultaneously,
+ * so this should be handled w/ lock and interrupts disabled.
+ */
+ spin_lock(&iochk_lock);
+ save_bridge_error();
+ ia64_mca_log_sal_error_record(SAL_INFO_TYPE_CPE);
+ spin_unlock(&iochk_lock);
+
+ /* Rests can go w/ interrupt enabled as usual */
+ local_irq_enable();
+#endif

  	spin_lock(&cpe_history_lock);
  	if (!cpe_poll_enabled && cpe_vector >= 0) {

[This is 10 of 10 patches, "iochk-10-rwlock.patch"]

- If a read access (i.g. readX/inX) cause a error while SAL gathers system data on other processor ,it could be happen a bridge error status is marked and vanished in a blink.

In case of MCA, thanks to rz_always flag, all MCA are handled as global, so all processor except one is paused during its handling.
But in case of CPE, as same as other interruption, it have to be handled beside of all other active processors.

Therefore, to avoid such status crash, exclusive control between read access and SAL_GET_STATE_INFO is required.

To realize this, I changed control lock from spin to rw. There would be better way, if so, this part should be replaced.

---

  arch/ia64/kernel/mca.c      |    6 +++---
  arch/ia64/lib/iomap_check.c |   11 ++++++-----
  include/asm-ia64/io.h       |   24 ++++++++++++++++++++++++
  3 files changed, 33 insertions(+), 8 deletions(-)

Index: linux-2.6.11.11/arch/ia64/lib/iomap_check.c =================================--- linux-2.6.11.11.orig/arch/ia64/lib/iomap_check.c +++ linux-2.6.11.11/arch/ia64/lib/iomap_check.c
@@ -12,7 +12,7 @@ void iochk_clear(iocookie *cookie, struc
int iochk_read(iocookie *cookie);

struct list_head iochk_devices;
-DEFINE_SPINLOCK(iochk_lock); /* all works are excluded on this lock */
+DEFINE_RWLOCK(iochk_lock); /* all works are excluded on this lock */

static struct pci_dev *search_host_bridge(struct pci_dev *dev); static int have_error(struct pci_dev *dev);
@@ -36,14 +36,14 @@ void iochk_clear(iocookie *cookie, struc
cookie->dev = dev;
cookie->host = search_host_bridge(dev);

- spin_lock_irqsave(&iochk_lock, flag);
+ write_lock_irqsave(&iochk_lock, flag);
if(cookie->host && have_error(cookie->host)) {
/* someone under my bridge causes error... */ notify_bridge_error(cookie->host); clear_bridge_error(cookie->host);
}
list_add(&cookie->list, &iochk_devices);
- spin_unlock_irqrestore(&iochk_lock, flag);
+ write_unlock_irqrestore(&iochk_lock, flag);

cookie->error = 0;
}
@@ -53,12 +53,12 @@ int iochk_read(iocookie *cookie)
unsigned long flag;
int ret = 0;

- spin_lock_irqsave(&iochk_lock, flag);
+ write_lock_irqsave(&iochk_lock, flag);
if( cookie->error || have_error(cookie->dev)
|| (cookie->host && have_error(cookie->host)) ) ret = 1;
list_del(&cookie->list);
- spin_unlock_irqrestore(&iochk_lock, flag);
+ write_unlock_irqrestore(&iochk_lock, flag);

return ret;
}
@@ -160,6 +160,7 @@ void save_bridge_error(void)
}
}

+EXPORT_SYMBOL(iochk_lock);
EXPORT_SYMBOL(iochk_read);
EXPORT_SYMBOL(iochk_clear);
EXPORT_SYMBOL(iochk_devices); /* for MCA driver */
Index: linux-2.6.11.11/include/asm-ia64/io.h =================================--- linux-2.6.11.11.orig/include/asm-ia64/io.h +++ linux-2.6.11.11/include/asm-ia64/io.h
@@ -73,6 +73,7 @@ extern unsigned int num_io_spaces;

#ifdef CONFIG_IOMAP_CHECK
#include <linux/list.h>
+#include <linux/spinlock.h>

/* definition of ia64 iocookie */
struct __iocookie {
@@ -83,6 +84,8 @@ struct __iocookie {
};
typedef struct __iocookie iocookie;

+extern rwlock_t iochk_lock; /* see arch/ia64/lib/iomap_check.c */ +
/* enable ia64 iochk - See arch/ia64/lib/iomap_check.c */ #define HAVE_ARCH_IOMAP_CHECK

@@ -211,10 +214,13 @@ ___ia64_inb (unsigned long port)
{
volatile unsigned char *addr = __ia64_mk_io_addr(port); unsigned char ret;
+ unsigned long flags;

+ read_lock_irqsave(&iochk_lock,flags);
ret = *addr;
__ia64_mf_a();
ia64_poison_check(ret);
+ read_unlock_irqrestore(&iochk_lock,flags);

return ret;
}
@@ -224,10 +230,13 @@ ___ia64_inw (unsigned long port)
{
volatile unsigned short *addr = __ia64_mk_io_addr(port); unsigned short ret;
+ unsigned long flags;

+ read_lock_irqsave(&iochk_lock,flags);
ret = *addr;
__ia64_mf_a();
ia64_poison_check(ret);
+ read_unlock_irqrestore(&iochk_lock,flags);

return ret;
}
@@ -237,10 +246,13 @@ ___ia64_inl (unsigned long port)
{
volatile unsigned int *addr = __ia64_mk_io_addr(port); unsigned int ret;
+ unsigned long flags;

+ read_lock_irqsave(&iochk_lock,flags);
ret = *addr;
__ia64_mf_a();
ia64_poison_check(ret);
+ read_unlock_irqrestore(&iochk_lock,flags);

return ret;
}
@@ -405,9 +417,12 @@ static inline unsigned char
___ia64_readb (const volatile void __iomem *addr) {
unsigned char val;
+ unsigned long flags;

+ read_lock_irqsave(&iochk_lock,flags);
val = *(volatile unsigned char __force *)addr; ia64_poison_check(val);
+ read_unlock_irqrestore(&iochk_lock,flags);

return val;
}
@@ -416,9 +431,12 @@ static inline unsigned short
___ia64_readw (const volatile void __iomem *addr) {
unsigned short val;
+ unsigned long flags;

+ read_lock_irqsave(&iochk_lock,flags);
val = *(volatile unsigned short __force *)addr; ia64_poison_check(val);
+ read_unlock_irqrestore(&iochk_lock,flags);

return val;
}
@@ -427,9 +445,12 @@ static inline unsigned int
___ia64_readl (const volatile void __iomem *addr) {
unsigned int val;
+ unsigned long flags;

+ read_lock_irqsave(&iochk_lock,flags);
val = *(volatile unsigned int __force *) addr; ia64_poison_check(val);
+ read_unlock_irqrestore(&iochk_lock,flags);

return val;
}
@@ -438,9 +459,12 @@ static inline unsigned long
___ia64_readq (const volatile void __iomem *addr) {
unsigned long val;
+ unsigned long flags;

+ read_lock_irqsave(&iochk_lock,flags);
val = *(volatile unsigned long __force *) addr; ia64_poison_check(val);
+ read_unlock_irqrestore(&iochk_lock,flags);

return val;
}
Index: linux-2.6.11.11/arch/ia64/kernel/mca.c =================================--- linux-2.6.11.11.orig/arch/ia64/kernel/mca.c +++ linux-2.6.11.11/arch/ia64/kernel/mca.c
@@ -81,7 +81,7 @@
#include <linux/pci.h>
extern void notify_bridge_error(struct pci_dev *bridge);
extern void save_bridge_error(void);
-extern spinlock_t iochk_lock;
+extern rwlock_t iochk_lock;
#endif

#if defined(IA64_MCA_DEBUG_INFO)
@@ -306,10 +306,10 @@ ia64_mca_cpe_int_handler (int cpe_irq, v
* the states from changing by any other I/Os running simultaneously,
* so this should be handled w/ lock and interrupts disabled. */
- spin_lock(&iochk_lock);
+ write_lock(&iochk_lock);
save_bridge_error(); ia64_mca_log_sal_error_record(SAL_INFO_TYPE_CPE);
- spin_unlock(&iochk_lock);
+ write_unlock(&iochk_lock);

/* Rests can go w/ interrupt enabled as usual */ local_irq_enable();

Overall, the idea and implementation looks very nice. I just had a few comments on the code style and how you implemented the .h and .c files. Good job.

thanks,

greg k-h

It makes sense to sandwich other kinds of device accesses. I don't think the previous clear/read_pci_errors() interface was intended *only* to sandwich readX().

I'm not sure we need this. Surely it can be deduced from the pci_dev or struct device?

You do know that ioread32() doesn't take a pci_dev, right? I hope you weren't counting on that for the rest of your implementation.

"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

On many platforms, only read() is guaranteed to reliably report errors though.

Might be useful to know more though, wether it was PIO or MMIO or other things. Also, I'd like to carry around the possible error details as can be returned by the firmware in some platforms.

In fact, Is there any reason this is not ioerr_cookie instead of iocookie ? :)

Hi Ben,

To be honest, No :)
Or is there any reason to limit use of this cookie only for errors?

Thanks,
H.Seto

At least there was _me_ who actually intended that... :-p Thank you for being so understanding.

Once I prepared a cookie per a device, added it into pci_dev. But one of our NIC driver folks pointed out that it was hard to handle because there could be many contexts/threads riding on one device at same time. So I reconsidered it and now come to "a cookie per a context" style.

Oops. It's just my typo. Please ignore it.

Thanks,
H.Seto

I think this is the wrong way to go about it. For PCI Express, we have a defined cross-architecture standard which tells us exactly how all future PCIe devices will behave in the face of errors. For PCI and PCI-X, we have a lot of legacy systems, each of which implements error checking and recovery in a somewhat eclectic way.

So, IMO, any implementation of PCI error recovery should start by implementing the PCI Express AER mechanisms and then each architecture can look at extending that scheme to fit their own legacy hardware systems. That way we have a clean implementation for the future rather than being tied to any one manufacturer or architecture's quirks.

Also, we can evaluate it based on looking at what the standard says, rather than all trying to wrap our brains around the idiosyncracies of a given platform ;-)

"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

All right, please take it a example of approach from legacy-side.

Already there are good working group, includes Linas, BenH, and Long. They are also implementing some PCI error recovery codes (currently setting home to ppc64), and I know their wonderful works are more PCI Express friendly than my mysterious ia64 works :-)

However, I also know that it doesn't mean my works were useless. Since there is a notable difference between their asynchronous error recovery and my synchronous error detecting, both could live in coexistence with each other.

How cooperate with is interesting coming agenda, I think.

Thanks,
H.Seto

We already defined something for recovery that everybody seem to be fine with and that isn't tied around PCIe specifics. I do not think PCIe is the ultimate panacea that will replace everything.

No, I strongly disagree.

Well, our recovery mecanism is intended to be an addition to the synchronous error detection. If you read carefully my document for example, I specify places where driver should still do synchronous detection, especially in some of the recovery phases themselves.

We have agreed a long time ago that a good mecanism for synchronous detection is to sandwitch IOs that way. The actual implementation may use AER, pSeries EEH mecanisms, PCI/PCI-X status errors bits (that need per-segment locks though) etc... depending on the architecture.

Since the actual error information can be very varied, the error "cookie" was suggested as an opaque way to carry that information and keep track of other platform specific things. We can then add specific accessors to extract useful infos (or dump as ASCII for some logging facility) the details of the error cookie.

Ben.