[PATCH 00/10] IOCHK interface for I/O error handling/detecting

[PATCH 00/10] IOCHK interface for I/O error handling/detecting

[Hidetoshi Seto]

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.

[PATCH 01/10] IOCHK interface for I/O error handling/detecting

[Hidetoshi Seto]

[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).

Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>

---

  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
===================================================================
--- linux-2.6.11.11.orig/lib/iomap.c
+++ 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
===================================================================
--- linux-2.6.11.11.orig/include/asm-generic/iomap.h
+++ 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
===================================================================
--- linux-2.6.11.11.orig/drivers/pci/pci.c
+++ 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;
  }

[PATCH 02/10] IOCHK interface for I/O error handling/detecting

[Hidetoshi 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).

Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>

---

  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
===================================================================
--- linux-2.6.11.11.orig/arch/ia64/lib/Makefile
+++ 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
===================================================================
--- linux-2.6.11.11.orig/arch/ia64/Kconfig
+++ 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
===================================================================
--- /dev/null
+++ 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
===================================================================
--- linux-2.6.11.11.orig/include/asm-ia64/io.h
+++ 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>

  /*

[PATCH 03/10] IOCHK interface for I/O error handling/detecting

[Hidetoshi Seto]

[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).

Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>

---

  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

[PATCH 04/10] IOCHK interface for I/O error handling/detecting

[Hidetoshi 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).

Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>

---

  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
===================================================================
--- linux-2.6.11.11.orig/arch/ia64/lib/iomap_check.c
+++ 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
===================================================================
--- linux-2.6.11.11.orig/include/asm-ia64/io.h
+++ 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;

[PATCH 05/10] IOCHK interface for I/O error handling/detecting

[Hidetoshi Seto]

[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).

Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>

---

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

[PATCH 06/10] IOCHK interface for I/O error handling/detecting

[Hidetoshi Seto]

[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).

Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>

---

  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)

[PATCH 07/10] IOCHK interface for I/O error handling/detecting

[Hidetoshi Seto]

[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).

Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>

---

  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)
  {

[PATCH 08/10] IOCHK interface for I/O error handling/detecting

[Hidetoshi 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.

Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>

---

  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
  	 *

[PATCH 09/10] IOCHK interface for I/O error handling/detecting

[Hidetoshi Seto]

[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.

Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>

---

  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) {

[PATCH 10/10] IOCHK interface for I/O error handling/detecting

[Hidetoshi Seto]

[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.

Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>

---

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

Re: [PATCH 01/10] IOCHK interface for I/O error handling/detecting

[Greg KH]

On Thu, Jun 09, 2005 at 09:48:15PM +0900, Hidetoshi Seto wrote:
> --- linux-2.6.11.11.orig/lib/iomap.c
> +++ 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 */
> +}

A bit of a coding style difference between the two functions, yet they
do the same thing :)

> +
> +int iochk_read(iocookie *cookie)
> +{
> +	/* no-ops */
> +	return 0;
> +}

Why not just return the cookie?  Can this ever fail?

Shouldn't these go into a .h file and be made "static inline" so they
just compile away to nothing?

> +EXPORT_SYMBOL(iochk_clear);
> +EXPORT_SYMBOL(iochk_read);

EXPORT_SYMBOL_GPL() perhaps?

> +#endif /* HAVE_ARCH_IOMAP_CHECK */
> Index: linux-2.6.11.11/include/asm-generic/iomap.h
> ===================================================================
> --- linux-2.6.11.11.orig/include/asm-generic/iomap.h
> +++ 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

Why typedef this if it isn't specified?

thanks,

greg k-h

Re: [PATCH 03/10] IOCHK interface for I/O error handling/detecting

[Greg KH]

On Thu, Jun 09, 2005 at 09:51:57PM +0900, Hidetoshi Seto wrote:
> +	if( cookie->error || have_error(cookie->dev) )

This should be written as:
	if (cookie->error || have_error(cookie->dev))
instead (note the placement of spaces).

>  /* 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;

Hm, why not just make the thing be a "struct iocookie" in the first
place, then we don't have to mess with a typedef at all.  And then each
arch can define how the structure will look like in their private .c
files, ensuring that no user can ever try to touch the structure
themselves.

thanks,

greg k-h

Re: [PATCH 04/10] IOCHK interface for I/O error handling/detecting

[Greg KH]

On Thu, Jun 09, 2005 at 09:53:28PM +0900, Hidetoshi Seto wrote:
> +	/* there is no bridge */
> +	if (!dev->bus->self) return NULL;

Put the "return NULL;" on it's own line please.

thanks,

greg k-h

Re: [PATCH 00/10] IOCHK interface for I/O error handling/detecting

[Greg KH]

On Thu, Jun 09, 2005 at 09:39:11PM +0900, Hidetoshi Seto wrote:
> 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.

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

Re: [PATCH 00/10] IOCHK interface for I/O error handling/detecting

[Matthew Wilcox]

On Thu, Jun 09, 2005 at 09:39:11PM +0900, Hidetoshi Seto wrote:
> 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.

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().

> - 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.

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

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

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.

>     }
>     /* Critical section end. Did we have any trouble? */
>     if ( iochk_read(&cookie) ) return -1;
> 
>     /* OK, all system go. */
>     return 0;
> }

-- 
"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

Re: [PATCH 03/10] IOCHK interface for I/O error handling/detecting

[Matthew Wilcox]

On Thu, Jun 09, 2005 at 09:51:57PM +0900, Hidetoshi Seto wrote:
> +	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 somebody plugs a cardbus card into an ia64 machine, we BUG()?
Unacceptable.  Just return 0 if you don't know what to do with a
particular device.

-- 
"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

Re: [PATCH 00/10] IOCHK interface for I/O error handling/detecting

[Matthew Wilcox]

On Thu, Jun 09, 2005 at 09:39:11PM +0900, Hidetoshi Seto wrote:
> 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.

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

Re: [PATCH 07/10] IOCHK interface for I/O error handling/detecting

[David Mosberger]

Hidetoshi,

>>>>> On Thu, 09 Jun 2005 21:58:26 +0900, Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com> said:

  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

Re: [PATCH 00/10] IOCHK interface for I/O error handling/detecting

[Benjamin Herrenschmidt]

> 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().

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

> > - 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.
> 
> I'm not sure we need this.  Surely it can be deduced from the pci_dev or
> struct device?

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 ? :)

> > 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);
> 
> 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.
> 
> >     }
> >     /* Critical section end. Did we have any trouble? */
> >     if ( iochk_read(&cookie) ) return -1;
> > 
> >     /* OK, all system go. */
> >     return 0;
> > }
> 

Re: [PATCH 01/10] IOCHK interface for I/O error handling/detecting

[Hidetoshi Seto]

Hi Greg,

Thank you for giving me many useful advices!

Greg KH wrote:
> On Thu, Jun 09, 2005 at 09:48:15PM +0900, Hidetoshi Seto wrote:
> 
>>+void iochk_init(void) { ; }
>>+
>>+void iochk_clear(iocookie *cookie, struct pci_dev *dev)
>>+{
>>+	/* no-ops */
>>+}
> 
> A bit of a coding style difference between the two functions, yet they
> do the same thing :)

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

>>+int iochk_read(iocookie *cookie)
>>+{
>>+	/* no-ops */
>>+	return 0;
>>+}
> 
> Why not just return the cookie?  Can this ever fail?

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

> Shouldn't these go into a .h file and be made "static inline" so they
> just compile away to nothing?

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

>>+EXPORT_SYMBOL(iochk_clear);
>>+EXPORT_SYMBOL(iochk_read);
> 
> EXPORT_SYMBOL_GPL() perhaps?

Yea.

>>+#ifndef HAVE_ARCH_IOMAP_CHECK
>>+typedef unsigned long iocookie;
>>+#endif
> 
> Why typedef this if it isn't specified?

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

Thanks,
H.Seto

Re: [PATCH 07/10] IOCHK interface for I/O error handling/detecting

[Hidetoshi Seto]

Hi David,

David Mosberger wrote:
>>>>>>On Thu, 09 Jun 2005 21:58:26 +0900, Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com> said:
> 
>   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!

+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.

> 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.

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

Re: [PATCH 00/10] IOCHK interface for I/O error handling/detecting

[Hidetoshi Seto]

Matthew Wilcox wrote:
>>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.
> 
> 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().

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

>>- 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.
> 
> I'm not sure we need this.  Surely it can be deduced from the pci_dev or
> struct device?

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.

>>            *buf++ = ioread32(dev, ofs);
> 
> 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.

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

Thanks,
H.Seto

Re: [PATCH 03/10] IOCHK interface for I/O error handling/detecting

[Hidetoshi Seto]

Greg KH wrote:
>> /* 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;
> 
> Hm, why not just make the thing be a "struct iocookie" in the first
> place, then we don't have to mess with a typedef at all.  And then each
> arch can define how the structure will look like in their private .c
> files, ensuring that no user can ever try to touch the structure
> themselves.

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

Thanks,
H.Seto

Re: [PATCH 03/10] IOCHK interface for I/O error handling/detecting

[Hidetoshi Seto]

Matthew Wilcox wrote:
> On Thu, Jun 09, 2005 at 09:51:57PM +0900, Hidetoshi Seto wrote:
> 
>>+	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 somebody plugs a cardbus card into an ia64 machine, we BUG()?
> Unacceptable.  Just return 0 if you don't know what to do with a
> particular device.

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

Thanks,
H.Seto

Re: [PATCH 00/10] IOCHK interface for I/O error handling/detecting

[Hidetoshi Seto]

Hi Ben,

Benjamin Herrenschmidt wrote:
>>>- 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.
>>
>>I'm not sure we need this.  Surely it can be deduced from the pci_dev or
>>struct device?
> 
> 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 ? :)

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

Thanks,
H.Seto

Re: [PATCH 00/10] IOCHK interface for I/O error handling/detecting

[Hidetoshi Seto]

Matthew Wilcox wrote:
> On Thu, Jun 09, 2005 at 09:39:11PM +0900, Hidetoshi Seto wrote:
> 
>>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.
> 
> 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 ;-)

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

Re: [PATCH 07/10] IOCHK interface for I/O error handling/detecting

[David Mosberger]

>>>>> On Fri, 10 Jun 2005 19:29:58 +0900, Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com> said:

  Hidetoshi> Hi David,
  Hidetoshi> David Mosberger wrote:
  >>>>>>> On Thu, 09 Jun 2005 21:58:26 +0900, Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com> said:
  >> 
  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!

  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

Re: [PATCH 00/10] IOCHK interface for I/O error handling/detecting

[Benjamin Herrenschmidt]

> > 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.

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.
 
> > 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.

No, I strongly disagree.

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

> 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.

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.
 

Re: [PATCH 07/10] IOCHK interface for I/O error handling/detecting

[Hidetoshi Seto]

David Mosberger wrote:
>>>>>>On Fri, 10 Jun 2005 19:29:58 +0900, Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com> said:
> 
>   Hidetoshi> +#define ia64_poison_check(val)					\
>   Hidetoshi> +{	register unsigned long gr8 asm("r8");			\
>   Hidetoshi> +	asm volatile ("add %0=%1,r0" : "=r"(gr8) : "r"(val)); }
> 
>   >> 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!
> 
>   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.

Yes, thanks for your understanding.

> 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'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.

> 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.

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