[PATCH 0/5] powerpc: EEH fixes

[PATCH 0/5] powerpc: EEH fixes

[Gavin Shan]

The series of patches is to fix recursive EEH error during recovery
for VFIO-PCI except some cleanup. The recursive EEH error is caused
by MMIO access when the PE is under reset or the PCI device isn't
enabled yet. The solution is to freeze the PE until the PCI device
is enabled after PE reset.

Another issue is that one specific PE is frozen when being passed
through. It caused the device can't work properly in userland (e.g.
QEMU). The fix is to clear the frozen state when passing PE to
userland.

Gavin Shan (5):
  powerpc/eeh: Drop unused argument in eeh_check_failure()
  powerpc/eeh: Add eeh_pe_state sysfs entry
  powerpc/eeh: Freeze PE before PE reset
  powerpc/eeh: Reenable PCI devices after reset
  powerpc/eeh: Clear frozen state on passing device

 arch/powerpc/include/asm/eeh.h               |  30 +++----
 arch/powerpc/kernel/eeh.c                    | 116 +++++++++++++++++++++------
 arch/powerpc/kernel/eeh_sysfs.c              |  61 +++++++++++++-
 arch/powerpc/platforms/powernv/eeh-ioda.c    |  43 +++++++---
 arch/powerpc/platforms/pseries/eeh_pseries.c |   4 +-
 5 files changed, 203 insertions(+), 51 deletions(-)

-- 
1.8.3.2

[PATCH 1/5] powerpc/eeh: Drop unused argument in eeh_check_failure()

[Gavin Shan]

eeh_check_failure() is used to check frozen state of the PE which
owns the indicated I/O address. The argument "val" of the function
isn't used. The patch drops it and return the frozen state of the
PE as expected.

Cc: Vishal Mansur <vmansur@linux.vnet.ibm.com>
Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/eeh.h | 29 ++++++++++++++---------------
 arch/powerpc/kernel/eeh.c      | 15 ++++++---------
 2 files changed, 20 insertions(+), 24 deletions(-)

diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
index 9983c3d..adcddb1 100644
--- a/arch/powerpc/include/asm/eeh.h
+++ b/arch/powerpc/include/asm/eeh.h
@@ -269,8 +269,7 @@ void eeh_dev_phb_init_dynamic(struct pci_controller *phb);
 int eeh_init(void);
 int __init eeh_ops_register(struct eeh_ops *ops);
 int __exit eeh_ops_unregister(const char *name);
-unsigned long eeh_check_failure(const volatile void __iomem *token,
-				unsigned long val);
+int eeh_check_failure(const volatile void __iomem *token);
 int eeh_dev_check_failure(struct eeh_dev *edev);
 void eeh_addr_cache_build(void);
 void eeh_add_device_early(struct device_node *);
@@ -321,9 +320,9 @@ static inline void *eeh_dev_init(struct device_node *dn, void *data)
 
 static inline void eeh_dev_phb_init_dynamic(struct pci_controller *phb) { }
 
-static inline unsigned long eeh_check_failure(const volatile void __iomem *token, unsigned long val)
+static inline int eeh_check_failure(const volatile void __iomem *token)
 {
-	return val;
+	return 0;
 }
 
 #define eeh_dev_check_failure(x) (0)
@@ -354,7 +353,7 @@ static inline u8 eeh_readb(const volatile void __iomem *addr)
 {
 	u8 val = in_8(addr);
 	if (EEH_POSSIBLE_ERROR(val, u8))
-		return eeh_check_failure(addr, val);
+		eeh_check_failure(addr);
 	return val;
 }
 
@@ -362,7 +361,7 @@ static inline u16 eeh_readw(const volatile void __iomem *addr)
 {
 	u16 val = in_le16(addr);
 	if (EEH_POSSIBLE_ERROR(val, u16))
-		return eeh_check_failure(addr, val);
+		eeh_check_failure(addr);
 	return val;
 }
 
@@ -370,7 +369,7 @@ static inline u32 eeh_readl(const volatile void __iomem *addr)
 {
 	u32 val = in_le32(addr);
 	if (EEH_POSSIBLE_ERROR(val, u32))
-		return eeh_check_failure(addr, val);
+		eeh_check_failure(addr);
 	return val;
 }
 
@@ -378,7 +377,7 @@ static inline u64 eeh_readq(const volatile void __iomem *addr)
 {
 	u64 val = in_le64(addr);
 	if (EEH_POSSIBLE_ERROR(val, u64))
-		return eeh_check_failure(addr, val);
+		eeh_check_failure(addr);
 	return val;
 }
 
@@ -386,7 +385,7 @@ static inline u16 eeh_readw_be(const volatile void __iomem *addr)
 {
 	u16 val = in_be16(addr);
 	if (EEH_POSSIBLE_ERROR(val, u16))
-		return eeh_check_failure(addr, val);
+		eeh_check_failure(addr);
 	return val;
 }
 
@@ -394,7 +393,7 @@ static inline u32 eeh_readl_be(const volatile void __iomem *addr)
 {
 	u32 val = in_be32(addr);
 	if (EEH_POSSIBLE_ERROR(val, u32))
-		return eeh_check_failure(addr, val);
+		eeh_check_failure(addr);
 	return val;
 }
 
@@ -402,7 +401,7 @@ static inline u64 eeh_readq_be(const volatile void __iomem *addr)
 {
 	u64 val = in_be64(addr);
 	if (EEH_POSSIBLE_ERROR(val, u64))
-		return eeh_check_failure(addr, val);
+		eeh_check_failure(addr);
 	return val;
 }
 
@@ -416,7 +415,7 @@ static inline void eeh_memcpy_fromio(void *dest, const
 	 * were copied. Check all four bytes.
 	 */
 	if (n >= 4 && EEH_POSSIBLE_ERROR(*((u32 *)(dest + n - 4)), u32))
-		eeh_check_failure(src, *((u32 *)(dest + n - 4)));
+		eeh_check_failure(src);
 }
 
 /* in-string eeh macros */
@@ -425,7 +424,7 @@ static inline void eeh_readsb(const volatile void __iomem *addr, void * buf,
 {
 	_insb(addr, buf, ns);
 	if (EEH_POSSIBLE_ERROR((*(((u8*)buf)+ns-1)), u8))
-		eeh_check_failure(addr, *(u8*)buf);
+		eeh_check_failure(addr);
 }
 
 static inline void eeh_readsw(const volatile void __iomem *addr, void * buf,
@@ -433,7 +432,7 @@ static inline void eeh_readsw(const volatile void __iomem *addr, void * buf,
 {
 	_insw(addr, buf, ns);
 	if (EEH_POSSIBLE_ERROR((*(((u16*)buf)+ns-1)), u16))
-		eeh_check_failure(addr, *(u16*)buf);
+		eeh_check_failure(addr);
 }
 
 static inline void eeh_readsl(const volatile void __iomem *addr, void * buf,
@@ -441,7 +440,7 @@ static inline void eeh_readsl(const volatile void __iomem *addr, void * buf,
 {
 	_insl(addr, buf, nl);
 	if (EEH_POSSIBLE_ERROR((*(((u32*)buf)+nl-1)), u32))
-		eeh_check_failure(addr, *(u32*)buf);
+		eeh_check_failure(addr);
 }
 
 #endif /* CONFIG_PPC64 */
diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
index 59a64f8..17cf52ba 100644
--- a/arch/powerpc/kernel/eeh.c
+++ b/arch/powerpc/kernel/eeh.c
@@ -542,17 +542,16 @@ EXPORT_SYMBOL_GPL(eeh_dev_check_failure);
 
 /**
  * eeh_check_failure - Check if all 1's data is due to EEH slot freeze
- * @token: I/O token, should be address in the form 0xA....
- * @val: value, should be all 1's (XXX why do we need this arg??)
+ * @token: I/O address
  *
- * Check for an EEH failure at the given token address.  Call this
+ * Check for an EEH failure at the given I/O address. Call this
  * routine if the result of a read was all 0xff's and you want to
- * find out if this is due to an EEH slot freeze event.  This routine
+ * find out if this is due to an EEH slot freeze event. This routine
  * will query firmware for the EEH status.
  *
  * Note this routine is safe to call in an interrupt context.
  */
-unsigned long eeh_check_failure(const volatile void __iomem *token, unsigned long val)
+int eeh_check_failure(const volatile void __iomem *token)
 {
 	unsigned long addr;
 	struct eeh_dev *edev;
@@ -562,13 +561,11 @@ unsigned long eeh_check_failure(const volatile void __iomem *token, unsigned lon
 	edev = eeh_addr_cache_get_dev(addr);
 	if (!edev) {
 		eeh_stats.no_device++;
-		return val;
+		return 0;
 	}
 
-	eeh_dev_check_failure(edev);
-	return val;
+	return eeh_dev_check_failure(edev);
 }
-
 EXPORT_SYMBOL(eeh_check_failure);
 
 
-- 
1.8.3.2

[PATCH 2/5] powerpc/eeh: Add eeh_pe_state sysfs entry

[Gavin Shan]

The patch adds sysfs entry "eeh_pe_state". Reading on it returns
the PE's state while writing to it clears the frozen state. It's
used to check or clear the PE frozen state from userland for
debugging purpose.

The patch also replaces printk(KERN_WARNING ...) with pr_warn() in
eeh_sysfs.c

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/eeh_sysfs.c | 61 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 60 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/eeh_sysfs.c b/arch/powerpc/kernel/eeh_sysfs.c
index e2595ba..e69bcbb 100644
--- a/arch/powerpc/kernel/eeh_sysfs.c
+++ b/arch/powerpc/kernel/eeh_sysfs.c
@@ -54,6 +54,63 @@ EEH_SHOW_ATTR(eeh_mode,            mode,            "0x%x");
 EEH_SHOW_ATTR(eeh_config_addr,     config_addr,     "0x%x");
 EEH_SHOW_ATTR(eeh_pe_config_addr,  pe_config_addr,  "0x%x");
 
+static ssize_t eeh_pe_state_show(struct device *dev,
+				 struct device_attribute *attr, char *buf)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct eeh_dev *edev = pci_dev_to_eeh_dev(pdev);
+	int state;
+
+	if (!edev || !edev->pe)
+		return 0;
+
+	state = eeh_ops->get_state(edev->pe, NULL);
+	return sprintf(buf, "PHB#%d-PE#%d: 0x%08x 0x%08x\n",
+		       edev->pe->phb->global_number,
+		       edev->pe->addr, state, edev->pe->state);
+}
+
+static ssize_t eeh_pe_state_store(struct device *dev,
+				  struct device_attribute *attr,
+				  const char *buf, size_t count)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct eeh_dev *edev = pci_dev_to_eeh_dev(pdev);
+	int ret;
+
+	if (!edev || !edev->pe)
+		return 0;
+
+	/* Nothing to do if it's not frozen */
+	if (!(edev->pe->state & EEH_PE_ISOLATED))
+		return 0;
+
+	/* Enable MMIO */
+	ret = eeh_pci_enable(edev->pe, EEH_OPT_THAW_MMIO);
+	if (ret) {
+		pr_warn("%s: Failure %d enabling MMIO for PHB#%d-PE#%d\n",
+			__func__, ret, edev->pe->phb->global_number,
+			edev->pe->addr);
+		return 0;
+	}
+
+	/* Enable DMA */
+	ret = eeh_pci_enable(edev->pe, EEH_OPT_THAW_DMA);
+	if (ret) {
+		pr_warn("%s: Failure %d enabling DMA for PHB#%d-PE#%d\n",
+			__func__, ret, edev->pe->phb->global_number,
+			edev->pe->addr);
+		return 0;
+	}
+
+	/* Clear software state */
+	eeh_pe_state_clear(edev->pe, EEH_PE_ISOLATED);
+
+	return count;
+}
+
+static DEVICE_ATTR_RW(eeh_pe_state);
+
 void eeh_sysfs_add_device(struct pci_dev *pdev)
 {
 	struct eeh_dev *edev = pci_dev_to_eeh_dev(pdev);
@@ -68,9 +125,10 @@ void eeh_sysfs_add_device(struct pci_dev *pdev)
 	rc += device_create_file(&pdev->dev, &dev_attr_eeh_mode);
 	rc += device_create_file(&pdev->dev, &dev_attr_eeh_config_addr);
 	rc += device_create_file(&pdev->dev, &dev_attr_eeh_pe_config_addr);
+	rc += device_create_file(&pdev->dev, &dev_attr_eeh_pe_state);
 
 	if (rc)
-		printk(KERN_WARNING "EEH: Unable to create sysfs entries\n");
+		pr_warn("EEH: Unable to create sysfs entries\n");
 	else if (edev)
 		edev->mode |= EEH_DEV_SYSFS;
 }
@@ -92,6 +150,7 @@ void eeh_sysfs_remove_device(struct pci_dev *pdev)
 	device_remove_file(&pdev->dev, &dev_attr_eeh_mode);
 	device_remove_file(&pdev->dev, &dev_attr_eeh_config_addr);
 	device_remove_file(&pdev->dev, &dev_attr_eeh_pe_config_addr);
+	device_remove_file(&pdev->dev, &dev_attr_eeh_pe_state);
 
 	if (edev)
 		edev->mode &= ~EEH_DEV_SYSFS;
-- 
1.8.3.2

[PATCH 3/5] powerpc/eeh: Freeze PE before PE reset

[Gavin Shan]

The patch adds one more option (EEH_OPT_FREEZE_PE) to set_option()
method to proactively freeze PE, which will be issued before resetting
pass-throughed PE to drop MMIO access during reset because it's
always contributing to recursive EEH error.

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/eeh.h               |  1 +
 arch/powerpc/kernel/eeh.c                    |  7 +++++
 arch/powerpc/platforms/powernv/eeh-ioda.c    | 43 +++++++++++++++++++++-------
 arch/powerpc/platforms/pseries/eeh_pseries.c |  4 ++-
 4 files changed, 44 insertions(+), 11 deletions(-)

diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
index adcddb1..f98b1b5 100644
--- a/arch/powerpc/include/asm/eeh.h
+++ b/arch/powerpc/include/asm/eeh.h
@@ -167,6 +167,7 @@ enum {
 #define EEH_OPT_ENABLE		1	/* EEH enable	*/
 #define EEH_OPT_THAW_MMIO	2	/* MMIO enable	*/
 #define EEH_OPT_THAW_DMA	3	/* DMA enable	*/
+#define EEH_OPT_FREEZE_PE	4	/* Freeze PE	*/
 #define EEH_STATE_UNAVAILABLE	(1 << 0)	/* State unavailable	*/
 #define EEH_STATE_NOT_SUPPORT	(1 << 1)	/* EEH not supported	*/
 #define EEH_STATE_RESET_ACTIVE	(1 << 2)	/* Active reset		*/
diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
index 17cf52ba..898c75f 100644
--- a/arch/powerpc/kernel/eeh.c
+++ b/arch/powerpc/kernel/eeh.c
@@ -1382,6 +1382,13 @@ int eeh_pe_reset(struct eeh_pe *pe, int option)
 		break;
 	case EEH_RESET_HOT:
 	case EEH_RESET_FUNDAMENTAL:
+		/*
+		 * Proactively freeze the PE to drop all MMIO access
+		 * during reset, which should be banned as it's always
+		 * cause recursive EEH error.
+		 */
+		eeh_ops->set_option(pe, EEH_OPT_FREEZE_PE);
+
 		ret = eeh_ops->reset(pe, option);
 		break;
 	default:
diff --git a/arch/powerpc/platforms/powernv/eeh-ioda.c b/arch/powerpc/platforms/powernv/eeh-ioda.c
index c945bed..f3027b9 100644
--- a/arch/powerpc/platforms/powernv/eeh-ioda.c
+++ b/arch/powerpc/platforms/powernv/eeh-ioda.c
@@ -189,6 +189,7 @@ static int ioda_eeh_set_option(struct eeh_pe *pe, int option)
 {
 	struct pci_controller *hose = pe->phb;
 	struct pnv_phb *phb = hose->private_data;
+	bool freeze_pe = false;
 	int enable, ret = 0;
 	s64 rc;
 
@@ -212,6 +213,10 @@ static int ioda_eeh_set_option(struct eeh_pe *pe, int option)
 	case EEH_OPT_THAW_DMA:
 		enable = OPAL_EEH_ACTION_CLEAR_FREEZE_DMA;
 		break;
+	case EEH_OPT_FREEZE_PE:
+		freeze_pe = true;
+		enable = OPAL_EEH_ACTION_SET_FREEZE_ALL;
+		break;
 	default:
 		pr_warn("%s: Invalid option %d\n",
 			__func__, option);
@@ -219,17 +224,35 @@ static int ioda_eeh_set_option(struct eeh_pe *pe, int option)
 	}
 
 	/* If PHB supports compound PE, to handle it */
-	if (phb->unfreeze_pe) {
-		ret = phb->unfreeze_pe(phb, pe->addr, enable);
+	if (freeze_pe) {
+		if (phb->freeze_pe) {
+			phb->freeze_pe(phb, pe->addr);
+		} else {
+			rc = opal_pci_eeh_freeze_set(phb->opal_id,
+						     pe->addr,
+						     enable);
+			if (rc != OPAL_SUCCESS) {
+				pr_warn("%s: Failure %lld freezing "
+					"PHB#%x-PE#%x\n",
+					__func__, rc,
+					phb->hose->global_number, pe->addr);
+				ret = -EIO;
+			}
+		}
 	} else {
-		rc = opal_pci_eeh_freeze_clear(phb->opal_id,
-					       pe->addr,
-					       enable);
-		if (rc != OPAL_SUCCESS) {
-			pr_warn("%s: Failure %lld enable %d for PHB#%x-PE#%x\n",
-				__func__, rc, option, phb->hose->global_number,
-				pe->addr);
-			ret = -EIO;
+		if (phb->unfreeze_pe) {
+			ret = phb->unfreeze_pe(phb, pe->addr, enable);
+		} else {
+			rc = opal_pci_eeh_freeze_clear(phb->opal_id,
+						       pe->addr,
+						       enable);
+			if (rc != OPAL_SUCCESS) {
+				pr_warn("%s: Failure %lld enable %d "
+					"for PHB#%x-PE#%x\n",
+					__func__, rc, option,
+					phb->hose->global_number, pe->addr);
+				ret = -EIO;
+			}
 		}
 	}
 
diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/powerpc/platforms/pseries/eeh_pseries.c
index b080538..b645dc6 100644
--- a/arch/powerpc/platforms/pseries/eeh_pseries.c
+++ b/arch/powerpc/platforms/pseries/eeh_pseries.c
@@ -349,7 +349,9 @@ static int pseries_eeh_set_option(struct eeh_pe *pe, int option)
 		if (pe->addr)
 			config_addr = pe->addr;
 		break;
-
+	case EEH_OPT_FREEZE_PE:
+		/* Not support */
+		return 0;
 	default:
 		pr_err("%s: Invalid option %d\n",
 			__func__, option);
-- 
1.8.3.2

[PATCH 4/5] powerpc/eeh: Reenable PCI devices after reset

[Gavin Shan]

The PCI devices that have been passed through are enabled before
reset, we need restore to the enabled state after reset. Otherwise,
MMIO access might be issued to disabled devices after reset and
causes exceptional recursive EEH error.

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/eeh.c | 62 ++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 48 insertions(+), 14 deletions(-)

diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
index 898c75f..db2841c 100644
--- a/arch/powerpc/kernel/eeh.c
+++ b/arch/powerpc/kernel/eeh.c
@@ -1342,6 +1342,53 @@ int eeh_pe_get_state(struct eeh_pe *pe)
 }
 EXPORT_SYMBOL_GPL(eeh_pe_get_state);
 
+static int eeh_pe_reenable_devices(struct eeh_pe *pe)
+{
+	struct eeh_dev *edev, *tmp;
+	struct pci_dev *pdev;
+	int ret = 0;
+
+	/* Restore config space */
+	eeh_pe_restore_bars(pe);
+
+	/*
+	 * Reenable PCI devices as the devices passed
+	 * through are always enabled before the reset.
+	 */
+	eeh_pe_for_each_dev(pe, edev, tmp) {
+		pdev = eeh_dev_to_pci_dev(edev);
+		if (!pdev)
+			continue;
+
+		ret = pci_reenable_device(pdev);
+		if (ret) {
+			pr_warn("%s: Failure %d reenabling %s\n",
+				__func__, ret, pci_name(pdev));
+			return ret;
+		}
+	}
+
+	/* The PE is still in frozen state */
+	ret = eeh_ops->set_option(pe, EEH_OPT_THAW_MMIO);
+	if (ret) {
+		pr_warn("%s: Failure %d enabling MMIO for PHB#%x-PE#%x\n",
+			__func__, ret, pe->phb->global_number, pe->addr);
+		return ret;
+	}
+
+	ret = eeh_ops->set_option(pe, EEH_OPT_THAW_DMA);
+	if (ret) {
+		pr_warn("%s: Failure %d enabling DMA for PHB#%x-PE#%x\n",
+			__func__, ret, pe->phb->global_number, pe->addr);
+		return ret;
+	}
+
+	/* Clear software isolated state */
+	eeh_pe_state_clear(pe, EEH_PE_ISOLATED);
+
+	return ret;
+}
+
 /**
  * eeh_pe_reset - Issue PE reset according to specified type
  * @pe: EEH PE
@@ -1368,17 +1415,7 @@ int eeh_pe_reset(struct eeh_pe *pe, int option)
 		if (ret)
 			break;
 
-		/*
-		 * The PE is still in frozen state and we need to clear
-		 * that. It's good to clear frozen state after deassert
-		 * to avoid messy IO access during reset, which might
-		 * cause recursive frozen PE.
-		 */
-		ret = eeh_ops->set_option(pe, EEH_OPT_THAW_MMIO);
-		if (!ret)
-			ret = eeh_ops->set_option(pe, EEH_OPT_THAW_DMA);
-		if (!ret)
-			eeh_pe_state_clear(pe, EEH_PE_ISOLATED);
+		ret = eeh_pe_reenable_devices(pe);
 		break;
 	case EEH_RESET_HOT:
 	case EEH_RESET_FUNDAMENTAL:
@@ -1417,9 +1454,6 @@ int eeh_pe_configure(struct eeh_pe *pe)
 	if (!pe)
 		return -ENODEV;
 
-	/* Restore config space for the affected devices */
-	eeh_pe_restore_bars(pe);
-
 	return ret;
 }
 EXPORT_SYMBOL_GPL(eeh_pe_configure);
-- 
1.8.3.2

[PATCH 5/5] powerpc/eeh: Clear frozen state on passing device

[Gavin Shan]

When passing through device, its PE might have been put into frozen
state. One obvious example would be: the passed PE is forced to be
offline because of hitting maximal allowed EEH errors in userland.
In that case, the frozen state won't be cleared and then the PE is
returned back to host, which might not have chance detecting and
recovering from it.

The patch adds more check when passing through device and clear the
PE frozen state if necessary.

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/eeh.c | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
index db2841c..211175e 100644
--- a/arch/powerpc/kernel/eeh.c
+++ b/arch/powerpc/kernel/eeh.c
@@ -1150,6 +1150,8 @@ void eeh_remove_device(struct pci_dev *dev)
 int eeh_dev_open(struct pci_dev *pdev)
 {
 	struct eeh_dev *edev;
+	int flag = (EEH_STATE_MMIO_ACTIVE | EEH_STATE_DMA_ACTIVE);
+	int ret = -ENODEV;
 
 	mutex_lock(&eeh_dev_mutex);
 
@@ -1162,6 +1164,34 @@ int eeh_dev_open(struct pci_dev *pdev)
 	if (!edev || !edev->pe)
 		goto out;
 
+	/*
+	 * The PE might have been put into frozen state, but we
+	 * didn't detect that yet. The passed through PCI devices
+	 * in frozen PE won't work properly. Clear the frozen state
+	 * in advance.
+	 */
+	ret = eeh_ops->get_state(edev->pe, NULL);
+	if (ret > 0 && ret != EEH_STATE_NOT_SUPPORT &&
+	    (ret & flag) != flag) {
+		ret = eeh_ops->set_option(edev->pe, EEH_OPT_THAW_MMIO);
+		if (ret) {
+			pr_warn("%s: Failure %d enabling MMIO "
+				"for PHB#%x-PE#%x\n",
+				__func__, ret, edev->phb->global_number,
+				edev->pe->addr);
+			goto out;
+		}
+
+		ret = eeh_ops->set_option(edev->pe, EEH_OPT_THAW_DMA);
+		if (ret) {
+			pr_warn("%s: Failure %d enabling DMA "
+				"for PHB#%x-PE#%x\n",
+				__func__, ret, edev->phb->global_number,
+				edev->pe->addr);
+			goto out;
+		}
+	}
+
 	/* Increase PE's pass through count */
 	atomic_inc(&edev->pe->pass_dev_cnt);
 	mutex_unlock(&eeh_dev_mutex);
@@ -1169,7 +1199,7 @@ int eeh_dev_open(struct pci_dev *pdev)
 	return 0;
 out:
 	mutex_unlock(&eeh_dev_mutex);
-	return -ENODEV;
+	return ret;
 }
 EXPORT_SYMBOL_GPL(eeh_dev_open);
 
-- 
1.8.3.2

Re: [2/5] powerpc/eeh: Add eeh_pe_state sysfs entry

[Michael Ellerman]

On Sun, 2014-17-08 at 03:02:26 UTC, Gavin Shan wrote:
> The patch adds sysfs entry "eeh_pe_state". Reading on it returns
> the PE's state while writing to it clears the frozen state. It's
> used to check or clear the PE frozen state from userland for
> debugging purpose.
> 
> diff --git a/arch/powerpc/kernel/eeh_sysfs.c b/arch/powerpc/kernel/eeh_sysfs.c
> index e2595ba..e69bcbb 100644
> --- a/arch/powerpc/kernel/eeh_sysfs.c
> +++ b/arch/powerpc/kernel/eeh_sysfs.c
> @@ -54,6 +54,63 @@ EEH_SHOW_ATTR(eeh_mode,            mode,            "0x%x");
>  EEH_SHOW_ATTR(eeh_config_addr,     config_addr,     "0x%x");
>  EEH_SHOW_ATTR(eeh_pe_config_addr,  pe_config_addr,  "0x%x");
>  
> +static ssize_t eeh_pe_state_show(struct device *dev,
> +				 struct device_attribute *attr, char *buf)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	struct eeh_dev *edev = pci_dev_to_eeh_dev(pdev);
> +	int state;
> +
> +	if (!edev || !edev->pe)
> +		return 0;
> +
> +	state = eeh_ops->get_state(edev->pe, NULL);
> +	return sprintf(buf, "PHB#%d-PE#%d: 0x%08x 0x%08x\n",
> +		       edev->pe->phb->global_number,
> +		       edev->pe->addr, state, edev->pe->state);

Shouldn't this only display the state, ie not the number and addr etc.

And why are there two states, state and edev->pe->state ?

> +static ssize_t eeh_pe_state_store(struct device *dev,
> +				  struct device_attribute *attr,
> +				  const char *buf, size_t count)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	struct eeh_dev *edev = pci_dev_to_eeh_dev(pdev);
> +	int ret;
> +
> +	if (!edev || !edev->pe)
> +		return 0;

Shouldn't that be an error?

> +	/* Nothing to do if it's not frozen */
> +	if (!(edev->pe->state & EEH_PE_ISOLATED))
> +		return 0;
> +
> +	/* Enable MMIO */
> +	ret = eeh_pci_enable(edev->pe, EEH_OPT_THAW_MMIO);
> +	if (ret) {
> +		pr_warn("%s: Failure %d enabling MMIO for PHB#%d-PE#%d\n",
> +			__func__, ret, edev->pe->phb->global_number,
> +			edev->pe->addr);
> +		return 0;

Error ?

> +	}
> +
> +	/* Enable DMA */
> +	ret = eeh_pci_enable(edev->pe, EEH_OPT_THAW_DMA);
> +	if (ret) {
> +		pr_warn("%s: Failure %d enabling DMA for PHB#%d-PE#%d\n",
> +			__func__, ret, edev->pe->phb->global_number,
> +			edev->pe->addr);
> +		return 0;

Error?

And should it roll back, ie. unthaw MMIO?


cheers

Re: [2/5] powerpc/eeh: Add eeh_pe_state sysfs entry

[Gavin Shan]

On Thu, Sep 25, 2014 at 02:09:58PM +1000, Michael Ellerman wrote:
>On Sun, 2014-17-08 at 03:02:26 UTC, Gavin Shan wrote:
>> The patch adds sysfs entry "eeh_pe_state". Reading on it returns
>> the PE's state while writing to it clears the frozen state. It's
>> used to check or clear the PE frozen state from userland for
>> debugging purpose.
>> 
>> diff --git a/arch/powerpc/kernel/eeh_sysfs.c b/arch/powerpc/kernel/eeh_sysfs.c
>> index e2595ba..e69bcbb 100644
>> --- a/arch/powerpc/kernel/eeh_sysfs.c
>> +++ b/arch/powerpc/kernel/eeh_sysfs.c
>> @@ -54,6 +54,63 @@ EEH_SHOW_ATTR(eeh_mode,            mode,            "0x%x");
>>  EEH_SHOW_ATTR(eeh_config_addr,     config_addr,     "0x%x");
>>  EEH_SHOW_ATTR(eeh_pe_config_addr,  pe_config_addr,  "0x%x");
>>  
>> +static ssize_t eeh_pe_state_show(struct device *dev,
>> +				 struct device_attribute *attr, char *buf)
>> +{
>> +	struct pci_dev *pdev = to_pci_dev(dev);
>> +	struct eeh_dev *edev = pci_dev_to_eeh_dev(pdev);
>> +	int state;
>> +
>> +	if (!edev || !edev->pe)
>> +		return 0;
>> +
>> +	state = eeh_ops->get_state(edev->pe, NULL);
>> +	return sprintf(buf, "PHB#%d-PE#%d: 0x%08x 0x%08x\n",
>> +		       edev->pe->phb->global_number,
>> +		       edev->pe->addr, state, edev->pe->state);
>
>Shouldn't this only display the state, ie not the number and addr etc.
>

Yes, I'll remove PHB#%d-PE#%d in next revision. Another sysfs entry
gives the PE number: /sys/bus/pci/devices/xxxx:xx:xx.x/eeh_pe_config_addr

>And why are there two states, state and edev->pe->state ?
>

state is from hardware, edev->pe->state is software maintained state.

>> +static ssize_t eeh_pe_state_store(struct device *dev,
>> +				  struct device_attribute *attr,
>> +				  const char *buf, size_t count)
>> +{
>> +	struct pci_dev *pdev = to_pci_dev(dev);
>> +	struct eeh_dev *edev = pci_dev_to_eeh_dev(pdev);
>> +	int ret;
>> +
>> +	if (!edev || !edev->pe)
>> +		return 0;
>
>Shouldn't that be an error?
>
>> +	/* Nothing to do if it's not frozen */
>> +	if (!(edev->pe->state & EEH_PE_ISOLATED))
>> +		return 0;
>> +
>> +	/* Enable MMIO */
>> +	ret = eeh_pci_enable(edev->pe, EEH_OPT_THAW_MMIO);
>> +	if (ret) {
>> +		pr_warn("%s: Failure %d enabling MMIO for PHB#%d-PE#%d\n",
>> +			__func__, ret, edev->pe->phb->global_number,
>> +			edev->pe->addr);
>> +		return 0;
>
>Error ?
>
>> +	}
>> +
>> +	/* Enable DMA */
>> +	ret = eeh_pci_enable(edev->pe, EEH_OPT_THAW_DMA);
>> +	if (ret) {
>> +		pr_warn("%s: Failure %d enabling DMA for PHB#%d-PE#%d\n",
>> +			__func__, ret, edev->pe->phb->global_number,
>> +			edev->pe->addr);
>> +		return 0;
>
>Error?
>

Yes, I'll fix all "Error" cases.

>And should it roll back, ie. unthaw MMIO?
>

It's not necessary as it's only for debugging purpose. The main
purpose is to keep dumping the PE hardware/software state when
recovering one specific PE.

Thanks,
Gavin