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

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

[Hidetoshi Seto]

Hi all,

The followings are updated version of patches I've posted to
implement IOCHK interface for I/O error handling/detecting.

The abstraction of patches hasn't changed, so please refer
archives if you need, e.g.: http://lwn.net/Articles/139240/

Tony, how do you think about applying my patches to your tree?

Thanks,
H.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).

Changes from previous one for 2.6.11.11:
   - reform default "nop" functions in static inline style.
   - I don't mind using EXPORT_SYMBOL_GPL but keep them as
     before. Does anyone worry about this?

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

---

  drivers/pci/pci.c           |    2 ++
  include/asm-generic/iomap.h |   32 ++++++++++++++++++++++++++++++++
  lib/iomap.c                 |    6 ++++++
  3 files changed, 40 insertions(+)

Index: linux-2.6.13-rc1/lib/iomap.c
===================================================================
--- linux-2.6.13-rc1.orig/lib/iomap.c
+++ linux-2.6.13-rc1/lib/iomap.c
@@ -230,3 +230,9 @@ void pci_iounmap(struct pci_dev *dev, vo
  }
  EXPORT_SYMBOL(pci_iomap);
  EXPORT_SYMBOL(pci_iounmap);
+
+#ifndef HAVE_ARCH_IOMAP_CHECK
+/* Since generic funcs are inlined and defined in header, just export */
+EXPORT_SYMBOL(iochk_clear);
+EXPORT_SYMBOL(iochk_read);
+#endif
Index: linux-2.6.13-rc1/include/asm-generic/iomap.h
===================================================================
--- linux-2.6.13-rc1.orig/include/asm-generic/iomap.h
+++ linux-2.6.13-rc1/include/asm-generic/iomap.h
@@ -65,4 +65,36 @@ 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
+/* Dummy definition of default iocookie */
+typedef int iocookie;
+#endif
+
+/*
+ * 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.
+ */
+#ifdef HAVE_ARCH_IOMAP_CHECK
+extern void iochk_init(void);
+extern void iochk_clear(iocookie *cookie, struct pci_dev *dev);
+extern int iochk_read(iocookie *cookie);
+#else
+static inline void iochk_init(void) {}
+static inline void iochk_clear(iocookie *cookie, struct pci_dev *dev) {}
+static inline int iochk_read(iocookie *cookie) { return 0; }
+#endif
+
  #endif
Index: linux-2.6.13-rc1/drivers/pci/pci.c
===================================================================
--- linux-2.6.13-rc1.orig/drivers/pci/pci.c
+++ linux-2.6.13-rc1/drivers/pci/pci.c
@@ -767,6 +767,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 2.6.13-rc1 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).

Changes from previous one for 2.6.11.11:
   - simplify define of iocookie structure.

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       |   13 +++++++++++++
  4 files changed, 57 insertions(+)

Index: linux-2.6.13-rc1/arch/ia64/lib/Makefile
===================================================================
--- linux-2.6.13-rc1.orig/arch/ia64/lib/Makefile
+++ linux-2.6.13-rc1/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.13-rc1/arch/ia64/Kconfig
===================================================================
--- linux-2.6.13-rc1.orig/arch/ia64/Kconfig
+++ linux-2.6.13-rc1/arch/ia64/Kconfig
@@ -413,6 +413,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.13-rc1/arch/ia64/lib/iomap_check.c
===================================================================
--- /dev/null
+++ linux-2.6.13-rc1/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.13-rc1/include/asm-ia64/io.h
===================================================================
--- linux-2.6.13-rc1.orig/include/asm-ia64/io.h
+++ linux-2.6.13-rc1/include/asm-ia64/io.h
@@ -70,6 +70,19 @@ extern unsigned int num_io_spaces;
  #include <asm/machvec.h>
  #include <asm/page.h>
  #include <asm/system.h>
+
+#ifdef CONFIG_IOMAP_CHECK
+
+/* ia64 iocookie */
+typedef struct {
+	int dummy;
+} 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 2.6.13-rc1 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).

Changes from previous one for 2.6.11.11:
   - trivial coding style fix.

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

---

  arch/ia64/lib/iomap_check.c |   55 ++++++++++++++++++++++++++++++++++++++++++--
  include/asm-ia64/io.h       |    5 +++-
  2 files changed, 57 insertions(+), 3 deletions(-)

Index: linux-2.6.13-rc1/arch/ia64/lib/iomap_check.c
===================================================================
--- linux-2.6.13-rc1.orig/arch/ia64/lib/iomap_check.c
+++ linux-2.6.13-rc1/arch/ia64/lib/iomap_check.c
@@ -4,24 +4,75 @@
   */

  #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 */
+		return 0; /* FIX ME */
+	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.13-rc1/include/asm-ia64/io.h
===================================================================
--- linux-2.6.13-rc1.orig/include/asm-ia64/io.h
+++ linux-2.6.13-rc1/include/asm-ia64/io.h
@@ -72,10 +72,13 @@ extern unsigned int num_io_spaces;
  #include <asm/system.h>

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

  /* ia64 iocookie */
  typedef struct {
-	int dummy;
+	struct list_head	list;
+	struct pci_dev		*dev;	/* target device */
+	unsigned long		error;	/* error flag */
  } iocookie;

  /* Enable ia64 iochk - See arch/ia64/lib/iomap_check.c */

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

[Linas Vepstas]

Hi,

Sorry for the late response ... 

I'm reading the patch, and I'm wondering what about performance and
overhead.   Here's the code that concerns me:

On Wed, Jul 06, 2005 at 02:04:14PM +0900, Hidetoshi Seto was heard to remark:
> [This is 3 of 10 patches, "iochk-03-register.patch"]
> 
> - Implement ia64 version of basic codes:
>     iochk_clear, iochk_read, iochk_init, and iocookie
>
>  int iochk_read(iocookie *cookie)
>  {
> +	if (cookie->error || have_error(cookie->dev))
....
> +}
> +
> +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;
> +	}
> +
> +	if ( (status & PCI_STATUS_REC_TARGET_ABORT)
> +		|| (status & PCI_STATUS_REC_MASTER_ABORT)
> +		|| (status & PCI_STATUS_DETECTED_PARITY) )
> +		return 1;
> 
>  	return 0;
>  }


Are you assuming that a device driver will use an iochk_read() for
every DMA operation? for every MMIO to the card?  

For high performance devices, it seems to me that this will cause
a rather large performance burden, especially if its envisioned that
all architectures will do something similar.

My concern is that (at least on ppc64) the call pci_read_config_word()
requires a call into "firmware" aka "BIOS", which takes thousands upon
thousands of cpu cycles.  There are hundreds of cycles of gratuitous
crud just to get into the firmware, and then lord-knows-what the
firmware does while its in there; probably doing all sorts of crazy
math to compute bus addresses and other arcane things.  I would imagine
that most architectures, includig ia64, are similar.

Thus, one wouldn't want to perform an iochk_read() in this way unless
one was already pretty sure that an error had already occured ... 

Am I misunderstanding something?

--linas

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

[Benjamin Herrenschmidt]

> Are you assuming that a device driver will use an iochk_read() for
> every DMA operation? for every MMIO to the card?  
> 
> For high performance devices, it seems to me that this will cause
> a rather large performance burden, especially if its envisioned that
> all architectures will do something similar.
> 
> My concern is that (at least on ppc64) the call pci_read_config_word()
> requires a call into "firmware" aka "BIOS", which takes thousands upon
> thousands of cpu cycles.  There are hundreds of cycles of gratuitous
> crud just to get into the firmware, and then lord-knows-what the
> firmware does while its in there; probably doing all sorts of crazy
> math to compute bus addresses and other arcane things.  I would imagine
> that most architectures, includig ia64, are similar.
> 
> Thus, one wouldn't want to perform an iochk_read() in this way unless
> one was already pretty sure that an error had already occured ... 
> 
> Am I misunderstanding something?

I would expect pSeries not to use the "default" error checking (that
tests the status register) but rather use EEH.

Ben.

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

[Linas Vepstas]

Hi,

Yes, but ...

On Wed, Jul 13, 2005 at 10:18:57AM +1000, Benjamin Herrenschmidt was heard to remark:
> 
> > Are you assuming that a device driver will use an iochk_read() for
> > every DMA operation? for every MMIO to the card?  
> > 
> > For high performance devices, it seems to me that this will cause
> > a rather large performance burden, especially if its envisioned that
> > all architectures will do something similar.
> > 
> > My concern is that (at least on ppc64) the call pci_read_config_word()
> > requires a call into "firmware" aka "BIOS", which takes thousands upon
> > thousands of cpu cycles.  There are hundreds of cycles of gratuitous
> > crud just to get into the firmware, and then lord-knows-what the
> > firmware does while its in there; probably doing all sorts of crazy
> > math to compute bus addresses and other arcane things.  I would imagine
> > that most architectures, includig ia64, are similar.
> > 
> > Thus, one wouldn't want to perform an iochk_read() in this way unless
> > one was already pretty sure that an error had already occured ... 
> > 
> > Am I misunderstanding something?
> 
> I would expect pSeries not to use the "default" error checking (that
> tests the status register) but rather use EEH.


OK, it wasn't clear to me if every possible case of the "detected parity
error" bit being set on the pci adapter is converted into an EEH error.
I had the impression that the adapter can set the bit, but not signal a 
#PERR, adn thus have no EEH event.   I am investigating this now.

If a given device driver is expecting iochk_read() to catch this situation, 
then we'd be screwed.

--linas

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

[Hidetoshi Seto]

Linas Vepstas wrote:
> Thus, one wouldn't want to perform an iochk_read() in this way unless
> one was already pretty sure that an error had already occured ... 

If another kind of I/O error detecting system finds a error before
performing iochk_read(), it can prevents coming iochk_read() from
spending such crazy cycles in have_error() by marking cookie->error.

 >> int iochk_read(iocookie *cookie)
 >>  {
 >> +	if (cookie->error || have_error(cookie->dev))

Isn't it enough?

And as Ben said, it seems that ppc64 can have its own special iochk_*,
unless calling pci_read_config_word() ;-)

Thanks,
H.Seto

[PATCH 2.6.13-rc1 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).

Changes from previous one for 2.6.11.11:
   - trivial coding style fix.

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

---

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

Index: linux-2.6.13-rc1/arch/ia64/lib/iomap_check.c
===================================================================
--- linux-2.6.13-rc1.orig/arch/ia64/lib/iomap_check.c
+++ linux-2.6.13-rc1/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,21 @@ 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.13-rc1/include/asm-ia64/io.h
===================================================================
--- linux-2.6.13-rc1.orig/include/asm-ia64/io.h
+++ linux-2.6.13-rc1/include/asm-ia64/io.h
@@ -78,6 +78,7 @@ extern unsigned int num_io_spaces;
  typedef struct {
  	struct list_head	list;
  	struct pci_dev		*dev;	/* target device */
+	struct pci_dev		*host;	/* hosting bridge */
  	unsigned long		error;	/* error flag */
  } iocookie;

[PATCH 2.6.13-rc1 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).

Changes from previous one for 2.6.11.11:
   - (non)

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.13-rc1/arch/ia64/lib/iomap_check.c
===================================================================
--- linux-2.6.13-rc1.orig/arch/ia64/lib/iomap_check.c
+++ linux-2.6.13-rc1/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);

@@ -95,5 +103,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);

Re: [PATCH 2.6.13-rc1 05/10] IOCHK interface for I/O error handling/detecting

[Grant Grundler]

On Wed, Jul 06, 2005 at 02:11:42PM +0900, Hidetoshi Seto wrote:
> [This is 5 of 10 patches, "iochk-05-check_bridge.patch"]
...
>   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.

Sorry, I don't understand this last paragraph.
I don't see how it's more simple with two devices (vs three) if
we don't exactly know which device caused the error. I thought
one still needed to reset/restart both devices. Is that correct?

The devices operate asyncronously from the drivers.
Only the driver can tell us for sure if IO was in flight for a
particular device and decide that a device could NOT have generated
an error.


Otherwise, so far, the patches look fine to me.

thanks,
grant

[PATCH 2.6.13-rc1 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).

Changes from previous one for 2.6.11.11:
   - (non)

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.13-rc1/arch/ia64/kernel/mca.c
===================================================================
--- linux-2.6.13-rc1.orig/arch/ia64/kernel/mca.c
+++ linux-2.6.13-rc1/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.13-rc1/arch/ia64/lib/iomap_check.c
===================================================================
--- linux-2.6.13-rc1.orig/arch/ia64/lib/iomap_check.c
+++ linux-2.6.13-rc1/arch/ia64/lib/iomap_check.c
@@ -111,7 +111,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 2.6.13-rc1 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).

Changes from previous one for 2.6.11.11:
   - move barrier function macro into gcc_inirin.h.
   - could anyone write same barrier for intel compiler?
     Tony or David, could you help me?

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

---

  include/asm-ia64/gcc_intrin.h |   16 +++++++
  include/asm-ia64/io.h         |   96 ++++++++++++++++++++++++++++++++++++++++++
  2 files changed, 112 insertions(+)

Index: linux-2.6.13-rc1/include/asm-ia64/io.h
===================================================================
--- linux-2.6.13-rc1.orig/include/asm-ia64/io.h
+++ linux-2.6.13-rc1/include/asm-ia64/io.h
@@ -189,6 +189,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)
  {
@@ -197,6 +199,8 @@ ___ia64_inb (unsigned long port)

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

@@ -208,6 +212,8 @@ ___ia64_inw (unsigned long port)

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

@@ -219,9 +225,48 @@ ___ia64_inl (unsigned long port)

  	ret = *addr;
  	__ia64_mf_a();
+	ia64_mca_barrier(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)
  {
@@ -338,6 +383,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_mca_barrier(val);
+
+	return val;
+}
+
+static inline unsigned short
+___ia64_readw (const volatile void __iomem *addr)
+{
+	unsigned short val;
+
+	val = *(volatile unsigned short __force *)addr;
+	ia64_mca_barrier(val);
+
+	return val;
+}
+
+static inline unsigned int
+___ia64_readl (const volatile void __iomem *addr)
+{
+	unsigned int val;
+
+	val = *(volatile unsigned int __force *) addr;
+	ia64_mca_barrier(val);
+
+	return val;
+}
+
+static inline unsigned long
+___ia64_readq (const volatile void __iomem *addr)
+{
+	unsigned long val;
+
+	val = *(volatile unsigned long __force *) addr;
+	ia64_mca_barrier(val);
+
+	return val;
+}
+
+#else /* CONFIG_IOMAP_CHECK */
+
  static inline unsigned char
  ___ia64_readb (const volatile void __iomem *addr)
  {
@@ -362,6 +456,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)
  {
Index: linux-2.6.13-rc1/include/asm-ia64/gcc_intrin.h
===================================================================
--- linux-2.6.13-rc1.orig/include/asm-ia64/gcc_intrin.h
+++ linux-2.6.13-rc1/include/asm-ia64/gcc_intrin.h
@@ -598,4 +598,20 @@ do {								\
  		      :: "r"((x)) : "p6", "p7", "memory");	\
  } while (0)

+/*
+ * Some I/O bridges may poison the data read, instead of
+ * signaling a BERR. The consummation of poisoned data
+ * triggers a MCA, which tells us the polluted address.
+ * 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,0;;	// fake operation
+ */
+#define ia64_mca_barrier(val)					\
+({								\
+	register unsigned long gr8 asm("r8");			\
+        asm volatile ("add %0=%1,r0" : "=r"(gr8) : "r"(val)); 	\
+})
+
  #endif /* _ASM_IA64_GCC_INTRIN_H */

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

[david mosberger]

On 7/5/05, Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com> wrote:

>    - could anyone write same barrier for intel compiler?
>      Tony or David, could you help me?

I think it might be best to make ia64_mca_barrier() a proper
subroutine written in assembly code.  Yes, that costs some time, but
we're talking about wasting 1,000+ cycles just to consume the value
read via readX(), so the call-overhead is actually overlapped and
completely trivial.

  --david

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

[Hidetoshi Seto]

david mosberger wrote:
>>   - could anyone write same barrier for intel compiler?
>>     Tony or David, could you help me?
> 
> I think it might be best to make ia64_mca_barrier() a proper
> subroutine written in assembly code.  Yes, that costs some time, but
> we're talking about wasting 1,000+ cycles just to consume the value
> read via readX(), so the call-overhead is actually overlapped and
> completely trivial.

Yes, of course speed is worth, but in some situations it can happen
that the data integrity is better than the speed.
(sounds crazy but fact)

I'm not familiar with assembly code for intel compiler. So David,
could you write another macro of ia64_mca_barrier() or a proper
subroutine instead?

Thanks,
H.Seto

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

[Linas Vepstas]

On Wed, Jul 06, 2005 at 02:17:21PM +0900, Hidetoshi Seto was heard to remark:
> 
> Touching poisoned data become a MCA, so now it directly means

Several questions: 

Is MCA an exception or fault of some sort, so at some point, 
the kernel would catch a fault?

So when you say "Touching poisoned data become a MCA", you mean that
if the CPU attempts to read poisoned data through the pci-to-host
bridge, it will (at some point) catch an exception?

> +	ia64_mca_barrier(ret);

I assume that the point of this barrier is to make sure that the fault,
if any, is delivered before this routine returns?

--linas

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

[Hidetoshi Seto]

Linas Vepstas wrote:
> On Wed, Jul 06, 2005 at 02:17:21PM +0900, Hidetoshi Seto was heard to remark:
> 
>>Touching poisoned data become a MCA, so now it directly means
> 
> Several questions: 
> 
> Is MCA an exception or fault of some sort, so at some point, 
> the kernel would catch a fault?
> 
> So when you say "Touching poisoned data become a MCA", you mean that
> if the CPU attempts to read poisoned data through the pci-to-host
> bridge, it will (at some point) catch an exception?

Yes.
More specifically, transferring poisoned data doesn't cause MCA,
but loading it to CPU register cause MCA. At the end of load,
CPU checks the data and deliver MCA if it was poisoned.

>>+	ia64_mca_barrier(ret);
> 
> I assume that the point of this barrier is to make sure that the fault,
> if any, is delivered before this routine returns?

Yes, that's what I expecting.

Thanks,
H.Seto

[PATCH 2.6.13-rc1 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.

Changes from previous one for 2.6.11.11:
   - (non)

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

---

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

Index: linux-2.6.13-rc1/arch/ia64/lib/iomap_check.c
===================================================================
--- linux-2.6.13-rc1.orig/arch/ia64/lib/iomap_check.c
+++ linux-2.6.13-rc1/arch/ia64/lib/iomap_check.c
@@ -147,3 +147,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.13-rc1/arch/ia64/kernel/mca_drv.c
===================================================================
--- linux-2.6.13-rc1.orig/arch/ia64/kernel/mca_drv.c
+++ linux-2.6.13-rc1/arch/ia64/kernel/mca_drv.c
@@ -35,6 +35,12 @@

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

@@ -377,6 +383,79 @@ 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
@@ -399,6 +478,11 @@ 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
  	 *

Re: [PATCH 2.6.13-rc1 08/10] IOCHK interface for I/O error handling/detecting

[Linas Vepstas]

On Wed, Jul 06, 2005 at 02:18:53PM +0900, Hidetoshi Seto was heard to remark:
> +static int pci_error_recovery(peidx_table_t *peidx)

Minor comment:
Maybe a different name for this routine would be good;
this potentially conflicts with generic pci routines.

--linas

Re: [PATCH 2.6.13-rc1 08/10] IOCHK interface for I/O error handling/detecting

[Hidetoshi Seto]

Linas Vepstas wrote:
> On Wed, Jul 06, 2005 at 02:18:53PM +0900, Hidetoshi Seto was heard to remark:
> 
>>+static int pci_error_recovery(peidx_table_t *peidx)
> 
> Minor comment:
> Maybe a different name for this routine would be good;
> this potentially conflicts with generic pci routines.

Good point. I'll fix it.

Thanks,
H.Seto

[PATCH 2.6.13-rc1 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.

Changes from previous one for 2.6.11.11:
   - (non)

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.13-rc1/arch/ia64/lib/iomap_check.c
===================================================================
--- linux-2.6.13-rc1.orig/arch/ia64/lib/iomap_check.c
+++ linux-2.6.13-rc1/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)
  {
@@ -145,6 +146,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.13-rc1/arch/ia64/kernel/mca.c
===================================================================
--- linux-2.6.13-rc1.orig/arch/ia64/kernel/mca.c
+++ linux-2.6.13-rc1/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 2.6.13-rc1 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.

Changes from previous one for 2.6.11.11:
   - (non)

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.13-rc1/arch/ia64/lib/iomap_check.c
===================================================================
--- linux-2.6.13-rc1.orig/arch/ia64/lib/iomap_check.c
+++ linux-2.6.13-rc1/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;
  }
@@ -162,6 +162,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.13-rc1/include/asm-ia64/io.h
===================================================================
--- linux-2.6.13-rc1.orig/include/asm-ia64/io.h
+++ linux-2.6.13-rc1/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>

  /* ia64 iocookie */
  typedef struct {
@@ -82,6 +83,8 @@ typedef struct {
  	unsigned long		error;	/* error flag */
  } 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

@@ -196,10 +199,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_mca_barrier(ret);
+	read_unlock_irqrestore(&iochk_lock,flags);

  	return ret;
  }
@@ -209,10 +215,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_mca_barrier(ret);
+	read_unlock_irqrestore(&iochk_lock,flags);

  	return ret;
  }
@@ -222,10 +231,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_mca_barrier(ret);
+	read_unlock_irqrestore(&iochk_lock,flags);

  	return ret;
  }
@@ -390,9 +402,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_mca_barrier(val);
+	read_unlock_irqrestore(&iochk_lock,flags);

  	return val;
  }
@@ -401,9 +416,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_mca_barrier(val);
+	read_unlock_irqrestore(&iochk_lock,flags);

  	return val;
  }
@@ -412,9 +430,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_mca_barrier(val);
+	read_unlock_irqrestore(&iochk_lock,flags);

  	return val;
  }
@@ -423,9 +444,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_mca_barrier(val);
+	read_unlock_irqrestore(&iochk_lock,flags);

  	return val;
  }
Index: linux-2.6.13-rc1/arch/ia64/kernel/mca.c
===================================================================
--- linux-2.6.13-rc1.orig/arch/ia64/kernel/mca.c
+++ linux-2.6.13-rc1/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 2.6.13-rc1 01/10] IOCHK interface for I/O error handling/detecting

[YOSHIFUJI Hideaki / 吉藤英明]

In article <42CB63B2.6000505@jp.fujitsu.com> (at Wed, 06 Jul 2005 13:53:06 +0900), Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com> says:

> Index: linux-2.6.13-rc1/lib/iomap.c
> ===================================================================
> --- linux-2.6.13-rc1.orig/lib/iomap.c
> +++ linux-2.6.13-rc1/lib/iomap.c
> @@ -230,3 +230,9 @@ void pci_iounmap(struct pci_dev *dev, vo
>   }
>   EXPORT_SYMBOL(pci_iomap);
>   EXPORT_SYMBOL(pci_iounmap);
> +
> +#ifndef HAVE_ARCH_IOMAP_CHECK
> +/* Since generic funcs are inlined and defined in header, just export */
> +EXPORT_SYMBOL(iochk_clear);
> +EXPORT_SYMBOL(iochk_read);
> +#endif
> Index: linux-2.6.13-rc1/include/asm-generic/iomap.h
> ===================================================================
> --- linux-2.6.13-rc1.orig/include/asm-generic/iomap.h
> +++ linux-2.6.13-rc1/include/asm-generic/iomap.h
:
> + */
> +#ifdef HAVE_ARCH_IOMAP_CHECK
> +extern void iochk_init(void);
> +extern void iochk_clear(iocookie *cookie, struct pci_dev *dev);
> +extern int iochk_read(iocookie *cookie);
> +#else
> +static inline void iochk_init(void) {}
> +static inline void iochk_clear(iocookie *cookie, struct pci_dev *dev) {}
> +static inline int iochk_read(iocookie *cookie) { return 0; }
> +#endif
> +
>   #endif

It looks strange to me.
You cannot export "static inline" functions.
You can export iochk_{init,clear,read} only
if HAVE_ARCH_IOMAP_CHECK is defined.

--yoshfuji

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

[Hidetoshi Seto]

YOSHIFUJI Hideaki wrote:
>>Index: linux-2.6.13-rc1/lib/iomap.c
>>===================================================================
>>--- linux-2.6.13-rc1.orig/lib/iomap.c
>>+++ linux-2.6.13-rc1/lib/iomap.c
>>@@ -230,3 +230,9 @@ void pci_iounmap(struct pci_dev *dev, vo
>>  }
>>  EXPORT_SYMBOL(pci_iomap);
>>  EXPORT_SYMBOL(pci_iounmap);
>>+
>>+#ifndef HAVE_ARCH_IOMAP_CHECK
>>+/* Since generic funcs are inlined and defined in header, just export */
>>+EXPORT_SYMBOL(iochk_clear);
>>+EXPORT_SYMBOL(iochk_read);
>>+#endif
:
> It looks strange to me.
> You cannot export "static inline" functions.
> You can export iochk_{init,clear,read} only
> if HAVE_ARCH_IOMAP_CHECK is defined.

Oh yes, I had such strange feel too.
I'm not sure there was a compile error or not (it seems good),
but if you right(suppose so) I should remove them.

Fortunately dropping these export doesn't affect any of other
following patches in this series.
Please replace only 01 patch to following new one, and reuse
rest 02 to 10 patches if this is actually strange.

Thanks,
H.Seto

---

  drivers/pci/pci.c           |    2 ++
  include/asm-generic/iomap.h |   32 ++++++++++++++++++++++++++++++++
  2 files changed, 34 insertions(+)

Index: linux-2.6.13-rc1/include/asm-generic/iomap.h
===================================================================
--- linux-2.6.13-rc1.orig/include/asm-generic/iomap.h
+++ linux-2.6.13-rc1/include/asm-generic/iomap.h
@@ -65,4 +65,36 @@ 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
+/* Dummy definition of default iocookie */
+typedef int iocookie;
+#endif
+
+/*
+ * 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.
+ */
+#ifdef HAVE_ARCH_IOMAP_CHECK
+extern void iochk_init(void);
+extern void iochk_clear(iocookie *cookie, struct pci_dev *dev);
+extern int iochk_read(iocookie *cookie);
+#else
+static inline void iochk_init(void) {}
+static inline void iochk_clear(iocookie *cookie, struct pci_dev *dev) {}
+static inline int iochk_read(iocookie *cookie) { return 0; }
+#endif
+
  #endif
Index: linux-2.6.13-rc1/drivers/pci/pci.c
===================================================================
--- linux-2.6.13-rc1.orig/drivers/pci/pci.c
+++ linux-2.6.13-rc1/drivers/pci/pci.c
@@ -767,6 +767,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;
  }

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

[Greg KH]

On Wed, Jul 06, 2005 at 01:53:06PM +0900, Hidetoshi Seto wrote:
> Hi all,
> 
> The followings are updated version of patches I've posted to
> implement IOCHK interface for I/O error handling/detecting.
> 
> The abstraction of patches hasn't changed, so please refer
> archives if you need, e.g.: http://lwn.net/Articles/139240/

How about the issue of tying this into the other pci error reporting
infrastructure that is being worked on?

thanks,

greg k-h

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

[Benjamin Herrenschmidt]

On Thu, 2005-07-07 at 11:41 -0700, Greg KH wrote:
> On Wed, Jul 06, 2005 at 01:53:06PM +0900, Hidetoshi Seto wrote:
> > Hi all,
> > 
> > The followings are updated version of patches I've posted to
> > implement IOCHK interface for I/O error handling/detecting.
> > 
> > The abstraction of patches hasn't changed, so please refer
> > archives if you need, e.g.: http://lwn.net/Articles/139240/
> 
> How about the issue of tying this into the other pci error reporting
> infrastructure that is being worked on?

The other infrastructure is for asynchronous reporting and recovery. We
still need synchronous detection & reporting. So this is a bit
different.

However, it would be nice if Hidetoshi's work could be adapted a bit so
that 1) naming is a bit more consistent with the other stuff (pcierr_*
maybe) and 2) the error "token" is the same. The later is especially
important if we start adding ways to query the error token to know what
the error precisely was etc... There is no reason to have 2 different
ways of representing error details.

Ben

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

[Hidetoshi Seto]

Benjamin Herrenschmidt wrote:
> On Thu, 2005-07-07 at 11:41 -0700, Greg KH wrote:
>>How about the issue of tying this into the other pci error reporting
>>infrastructure that is being worked on?
> 
> The other infrastructure is for asynchronous reporting and recovery.
> We still need synchronous detection & reporting. So this is a bit
> different.

The interesting point is that it seems that both could use for reporting.

> However, it would be nice if Hidetoshi's work could be adapted a bit so
> that 1) naming is a bit more consistent with the other stuff (pcierr_*
> maybe) and 2) the error "token" is the same. The later is especially
> important if we start adding ways to query the error token to know what
> the error precisely was etc... There is no reason to have 2 different
> ways of representing error details.

The naming doesn't really matter. iochk_* is just my preference.
However it would be worth a try to move generic codes from iomap.*
(historical home) to pci.* and rename iochk_* to pcierr_*.

Well, I'd like to use this opportunity to sort out my thoughts...

Now iochk_read() returns a boolean value, whether there was a error
or not. Of course I agree that it would be more useful if it can return
the detail of the error.

A quick solution is return "token" instead of boolean value 1.
-extern int iochk_read(iocookie *cookie);
+extern token iochk_read(iocookie *cookie);

The token should be a pointer or a bitmask having proper flags, not 0.
So this still work:
   if(iochk_read(cookie)) return -EIO;
and now this will also work:
   if((token=iochk_read(cookie))!=0){
      switch(severity(token)) {
        ...
   }}

The error "token", tentatively named "pci_error_token" in document
you posted, is now temporarily defined in recent Linas's patch using
other alias:
  enum pci_channel_state {
	pci_channel_io_normal = 0, /* I/O channel is in normal state */
	pci_channel_io_frozen = 1, /* I/O to channel is blocked */
	pci_channel_io_perm_failure, /* pci card is dead */
  };
Of course this will be not enough in near future.

I have already agree with what few month ago you said:
> The token should be an opaque type with accessors. You could define a
> pci_error_get_severity(token) to return the severity. The idea is to
> define accessors which return an error when the data requested isn't
> present in the error info. The actual content of the token is to be
> defined. I was thinking about a type plus a union. I was hoping Seto
> could provide something here ...

For example:
  /* offsets */
  enum {
	pcierr_io_frozen = 0, 	/* I/O to channel is blocked */
	pcierr_io_perm_failure, /* pci card is dead */
	pcierr_severity_valid,  /* 1:valid */
	pcierr_severity,     	/* 0:non-fatal 1:fatal */
  };

  #define pcierr_perm_failure(token) \
	test_bit(pcierr_io_perm_failure, token)

  int pcierr_get_severity(token)
  {
	if(test_bit(pcierr_severity_valid, token)) {
	  return test_bit(pcierr_severity, token);
	}
	return -1;
  }

Objections?
Are there any better place to put a group of codes like above than
include/linux/pci.h?

By the way, if token says frozen but not perm_failure, is it mean
"recovery processing now"? Are there any special state which
synchronous detection have to report?

Thanks,
H.Seto