[PATCH v2 0/6]

Also remove useless comment.

Signed-off-by Sam Bobroff <[email protected]>

Reviewed-by Alexey Kardashevskiy <[email protected]>

---
 arch/powerpc/kernel/eeh.c                    |  2 +-
 arch/powerpc/platforms/powernv/eeh-powernv.c | 14 ++++++++----
 arch/powerpc/platforms/pseries/eeh_pseries.c | 23 +++++++++++++++-----
 3 files changed, 28 insertions(+), 11 deletions(-)

arch/powerpc/kernel/eeh.c +1 -1

index 8d3c36a1f194..b14d89547895 100644
@@ -1291,7 +1291,7 @@ void eeh_add_device_late(struct pci_dev *dev)
 	pdn = pci_get_pdn_by_devfn(dev->bus, dev->devfn);
 	edev = pdn_to_eeh_dev(pdn);
 	if (edev->pdev == dev) {
-		pr_debug("EEH: Already referenced !\n");
+		pr_debug("EEH: Device %s already referenced!\n", pci_name(dev));
 		return;
 	}
 

arch/powerpc/platforms/powernv/eeh-powernv.c +0 -0

index 6fc1a463b796..0e374cdba961 100644

arch/powerpc/platforms/powernv/eeh-powernv.c +9 -0

 	eeh_add_device_early(pdn);
 	eeh_add_device_late(pdev);
 	eeh_sysfs_add_device(pdev);

@@ -397,6 +394,10 @@ static void *pnv_eeh_probe(struct pci_dn *pdn, void *data)
 	int ret;
 	int config_addr = (pdn->busno << 8) | (pdn->devfn);
 
+	pr_debug("%s: probing %04x:%02x:%02x.%01x\n",
+		__func__, hose->global_number, pdn->busno,
+		PCI_SLOT(pdn->devfn), PCI_FUNC(pdn->devfn));
+
 	/*
 	 * When probing the root bridge, which doesn't have any
 	 * subordinate PCI devices. We don't have OF node for

@@ -491,6 +492,11 @@ static void *pnv_eeh_probe(struct pci_dn *pdn, void *data)
 	/* Save memory bars */
 	eeh_save_bars(edev);
 
+	pr_debug("%s: EEH enabled on %02x:%02x.%01x PHB#%x-PE#%x\n",
+		__func__, pdn->busno, PCI_SLOT(pdn->devfn),
+		PCI_FUNC(pdn->devfn), edev->pe->phb->global_number,
+		edev->pe->addr);
+
 	return NULL;
 }
 

arch/powerpc/platforms/pseries/eeh_pseries.c +6 -0

index 7aa50258dd42..ae06878fbdea 100644
@@ -65,6 +65,8 @@ void pseries_pcibios_bus_add_device(struct pci_dev *pdev)
 	if (!pdev->is_virtfn)
 		return;
 
+	pr_debug("%s: EEH: Setting up device %s.\n", __func__, pci_name(pdev));
+
 	pdn->device_id  =  pdev->device;
 	pdn->vendor_id  =  pdev->vendor;
 	pdn->class_code =  pdev->class;

@@ -251,6 +253,10 @@ static void *pseries_eeh_probe(struct pci_dn *pdn, void *data)
 	int enable = 0;
 	int ret;
 
+	pr_debug("%s: probing %04x:%02x:%02x.%01x\n",
+		__func__, pdn->phb->global_number, pdn->busno,
+		PCI_SLOT(pdn->devfn), PCI_FUNC(pdn->devfn));
+
 	/* Retrieve OF node and eeh device */
 	edev = pdn_to_eeh_dev(pdn);
 	if (!edev || edev->pe)

arch/powerpc/platforms/pseries/eeh_pseries.c +0 -5

 		/* Retrieve PE address */
 		edev->pe_config_addr = eeh_ops->get_pe_addr(&pe);
 		pe.addr = edev->pe_config_addr;

@@ -310,11 +321,6 @@ static void *pseries_eeh_probe(struct pci_dn *pdn, void *data)
 		if (enable) {
 			eeh_add_flag(EEH_ENABLED);
 			eeh_add_to_parent_pe(edev);
-
-			pr_debug("%s: EEH enabled on %02x:%02x.%01x PHB#%x-PE#%x\n",
-				__func__, pdn->busno, PCI_SLOT(pdn->devfn),
-				PCI_FUNC(pdn->devfn), pe.phb->global_number,
-				pe.addr);
 		} else if (pdn->parent && pdn_to_eeh_dev(pdn->parent) &&
 			   (pdn_to_eeh_dev(pdn->parent))->pe) {
 			/* This device doesn't support EEH, but it may have an

arch/powerpc/platforms/pseries/eeh_pseries.c +0 -0

 	}
 
 	/* Save memory bars */

Quoted 31 lines · Sam Bobroff

Also remove useless comment.

Signed-off-by Sam Bobroff <[email protected]>

Reviewed-by Alexey Kardashevskiy <[email protected]>

---
 arch/powerpc/kernel/eeh.c                    |  2 +-
 arch/powerpc/platforms/powernv/eeh-powernv.c | 14 ++++++++----
 arch/powerpc/platforms/pseries/eeh_pseries.c | 23 +++++++++++++++-----
 3 files changed, 28 insertions(+), 11 deletions(-)

arch/powerpc/kernel/eeh.c +1 -1

index 8d3c36a1f194..b14d89547895 100644
@@ -1291,7 +1291,7 @@ void eeh_add_device_late(struct pci_dev *dev)
 	pdn = pci_get_pdn_by_devfn(dev->bus, dev->devfn);
 	edev = pdn_to_eeh_dev(pdn);
 	if (edev->pdev == dev) {
-		pr_debug("EEH: Already referenced !\n");
+		pr_debug("EEH: Device %s already referenced!\n", pci_name(dev));
 		return;
 	}
 

arch/powerpc/platforms/powernv/eeh-powernv.c +0 -0

index 6fc1a463b796..0e374cdba961 100644
@@ -50,10 +50,7 @@ void pnv_pcibios_bus_add_device(struct pci_dev *pdev)
 	if (!pdev->is_virtfn)
 		return;
 

arch/powerpc/platforms/powernv/eeh-powernv.c void pnv_pcibios_bus_add_device(struct pci_dev *pdev) · line 53

Quoted 5 lines

arch/powerpc/platforms/powernv/eeh-powernv.c +1 -4

-	/*
-	 * The following operations will fail if VF's sysfs files
-	 * aren't created or its resources aren't finalized.
-	 */
+	pr_debug("%s: EEH: Setting up device %s.\n", __func__, pci_name(pdev));

dev_dbg() seems more appropriate.

Quoted 52 lines

arch/powerpc/platforms/powernv/eeh-powernv.c +9 -0

 	eeh_add_device_early(pdn);
 	eeh_add_device_late(pdev);
 	eeh_sysfs_add_device(pdev);

@@ -397,6 +394,10 @@ static void *pnv_eeh_probe(struct pci_dn *pdn, void *data)
 	int ret;
 	int config_addr = (pdn->busno << 8) | (pdn->devfn);
 
+	pr_debug("%s: probing %04x:%02x:%02x.%01x\n",
+		__func__, hose->global_number, pdn->busno,
+		PCI_SLOT(pdn->devfn), PCI_FUNC(pdn->devfn));
+
 	/*
 	 * When probing the root bridge, which doesn't have any
 	 * subordinate PCI devices. We don't have OF node for

@@ -491,6 +492,11 @@ static void *pnv_eeh_probe(struct pci_dn *pdn, void *data)
 	/* Save memory bars */
 	eeh_save_bars(edev);
 
+	pr_debug("%s: EEH enabled on %02x:%02x.%01x PHB#%x-PE#%x\n",
+		__func__, pdn->busno, PCI_SLOT(pdn->devfn),
+		PCI_FUNC(pdn->devfn), edev->pe->phb->global_number,
+		edev->pe->addr);
+
 	return NULL;
 }
 

arch/powerpc/platforms/pseries/eeh_pseries.c +6 -0

index 7aa50258dd42..ae06878fbdea 100644
@@ -65,6 +65,8 @@ void pseries_pcibios_bus_add_device(struct pci_dev *pdev)
 	if (!pdev->is_virtfn)
 		return;
 
+	pr_debug("%s: EEH: Setting up device %s.\n", __func__, pci_name(pdev));
+
 	pdn->device_id  =  pdev->device;
 	pdn->vendor_id  =  pdev->vendor;
 	pdn->class_code =  pdev->class;

@@ -251,6 +253,10 @@ static void *pseries_eeh_probe(struct pci_dn *pdn, void *data)
 	int enable = 0;
 	int ret;
 
+	pr_debug("%s: probing %04x:%02x:%02x.%01x\n",
+		__func__, pdn->phb->global_number, pdn->busno,
+		PCI_SLOT(pdn->devfn), PCI_FUNC(pdn->devfn));
+
 	/* Retrieve OF node and eeh device */
 	edev = pdn_to_eeh_dev(pdn);
 	if (!edev || edev->pe)

@@ -294,7 +300,12 @@ static void *pseries_eeh_probe(struct pci_dn *pdn, void *data)
 

arch/powerpc/platforms/pseries/eeh_pseries.c static void *pseries_eeh_probe(struct pci_dn *pdn, void *data) · line 308

Quoted 9 lines

arch/powerpc/platforms/pseries/eeh_pseries.c +6 -1

 	/* Enable EEH on the device */
 	ret = eeh_ops->set_option(&pe, EEH_OPT_ENABLE);
-	if (!ret) {
+	if (ret) {
+		pr_debug("%s: EEH failed to enable on %02x:%02x.%01x PHB#%x-PE#%x (code %d)\n",
+			__func__, pdn->busno, PCI_SLOT(pdn->devfn),
+			PCI_FUNC(pdn->devfn), pe.phb->global_number,
+			pe.addr, ret);
+	} else {

edev!=NULL here so you could do dev_dbg(&edev->pdev->dev,...) and skip PCI_SLOT/PCI_FUNC. Or is (edev!=NULL && edev->pdev==NULL) possible (it could be, just asking)?

Quoted 17 lines

arch/powerpc/platforms/pseries/eeh_pseries.c +0 -5

 		/* Retrieve PE address */
 		edev->pe_config_addr = eeh_ops->get_pe_addr(&pe);
 		pe.addr = edev->pe_config_addr;

@@ -310,11 +321,6 @@ static void *pseries_eeh_probe(struct pci_dn *pdn, void *data)
 		if (enable) {
 			eeh_add_flag(EEH_ENABLED);
 			eeh_add_to_parent_pe(edev);
-
-			pr_debug("%s: EEH enabled on %02x:%02x.%01x PHB#%x-PE#%x\n",
-				__func__, pdn->busno, PCI_SLOT(pdn->devfn),
-				PCI_FUNC(pdn->devfn), pe.phb->global_number,
-				pe.addr);
 		} else if (pdn->parent && pdn_to_eeh_dev(pdn->parent) &&
 			   (pdn_to_eeh_dev(pdn->parent))->pe) {
 			/* This device doesn't support EEH, but it may have an

arch/powerpc/platforms/pseries/eeh_pseries.c static void *pseries_eeh_probe(struct pci_dn *pdn, void *data) · line 336

Quoted 8 lines

arch/powerpc/platforms/pseries/eeh_pseries.c +5 -0

@@ -323,6 +329,11 @@ static void *pseries_eeh_probe(struct pci_dn *pdn, void *data)
 			edev->pe_config_addr = pdn_to_eeh_dev(pdn->parent)->pe_config_addr;
 			eeh_add_to_parent_pe(edev);
 		}
+		pr_debug("%s: EEH %s on %02x:%02x.%01x PHB#%x-PE#%x (code %d)\n",
+			__func__, (enable ? "enabled" : "unsupported"),
+			pdn->busno, PCI_SLOT(pdn->devfn),
+			PCI_FUNC(pdn->devfn), pe.phb->global_number,
+			pe.addr, ret);

Same here. I understand though this one is a cut-n-paste :)

Quoted 4 lines

arch/powerpc/platforms/pseries/eeh_pseries.c +0 -0

 	}
 
 	/* Save memory bars */

Signature · 1 line

Alexey

Quoted 34 lines · Alexey Kardashevskiy

Quoted 31 lines · Sam Bobroff

Also remove useless comment.

Signed-off-by Sam Bobroff <[email protected]>

Reviewed-by Alexey Kardashevskiy <[email protected]>

---
 arch/powerpc/kernel/eeh.c                    |  2 +-
 arch/powerpc/platforms/powernv/eeh-powernv.c | 14 ++++++++----
 arch/powerpc/platforms/pseries/eeh_pseries.c | 23 +++++++++++++++-----
 3 files changed, 28 insertions(+), 11 deletions(-)

arch/powerpc/kernel/eeh.c +1 -1

index 8d3c36a1f194..b14d89547895 100644
@@ -1291,7 +1291,7 @@ void eeh_add_device_late(struct pci_dev *dev)
 	pdn = pci_get_pdn_by_devfn(dev->bus, dev->devfn);
 	edev = pdn_to_eeh_dev(pdn);
 	if (edev->pdev == dev) {
-		pr_debug("EEH: Already referenced !\n");
+		pr_debug("EEH: Device %s already referenced!\n", pci_name(dev));
 		return;
 	}
 

arch/powerpc/platforms/powernv/eeh-powernv.c +0 -0

index 6fc1a463b796..0e374cdba961 100644
@@ -50,10 +50,7 @@ void pnv_pcibios_bus_add_device(struct pci_dev *pdev)
 	if (!pdev->is_virtfn)
 		return;
 

Quoted 8 lines

arch/powerpc/platforms/powernv/eeh-powernv.c void pnv_pcibios_bus_add_device(struct pci_dev *pdev) · line 53

Quoted 5 lines

arch/powerpc/platforms/powernv/eeh-powernv.c +1 -4

-	/*
-	 * The following operations will fail if VF's sysfs files
-	 * aren't created or its resources aren't finalized.
-	 */
+	pr_debug("%s: EEH: Setting up device %s.\n", __func__, pci_name(pdev));

dev_dbg() seems more appropriate.

Oh! It does, or even pci_debug() :-)

I'll change it if I need to do another version, otherwise I'll clean it up later.

Quoted 52 lines

Quoted 52 lines

arch/powerpc/platforms/powernv/eeh-powernv.c +9 -0

 	eeh_add_device_early(pdn);
 	eeh_add_device_late(pdev);
 	eeh_sysfs_add_device(pdev);

@@ -397,6 +394,10 @@ static void *pnv_eeh_probe(struct pci_dn *pdn, void *data)
 	int ret;
 	int config_addr = (pdn->busno << 8) | (pdn->devfn);
 
+	pr_debug("%s: probing %04x:%02x:%02x.%01x\n",
+		__func__, hose->global_number, pdn->busno,
+		PCI_SLOT(pdn->devfn), PCI_FUNC(pdn->devfn));
+
 	/*
 	 * When probing the root bridge, which doesn't have any
 	 * subordinate PCI devices. We don't have OF node for

@@ -491,6 +492,11 @@ static void *pnv_eeh_probe(struct pci_dn *pdn, void *data)
 	/* Save memory bars */
 	eeh_save_bars(edev);
 
+	pr_debug("%s: EEH enabled on %02x:%02x.%01x PHB#%x-PE#%x\n",
+		__func__, pdn->busno, PCI_SLOT(pdn->devfn),
+		PCI_FUNC(pdn->devfn), edev->pe->phb->global_number,
+		edev->pe->addr);
+
 	return NULL;
 }
 

arch/powerpc/platforms/pseries/eeh_pseries.c +6 -0

index 7aa50258dd42..ae06878fbdea 100644
@@ -65,6 +65,8 @@ void pseries_pcibios_bus_add_device(struct pci_dev *pdev)
 	if (!pdev->is_virtfn)
 		return;
 
+	pr_debug("%s: EEH: Setting up device %s.\n", __func__, pci_name(pdev));
+
 	pdn->device_id  =  pdev->device;
 	pdn->vendor_id  =  pdev->vendor;
 	pdn->class_code =  pdev->class;

@@ -251,6 +253,10 @@ static void *pseries_eeh_probe(struct pci_dn *pdn, void *data)
 	int enable = 0;
 	int ret;
 
+	pr_debug("%s: probing %04x:%02x:%02x.%01x\n",
+		__func__, pdn->phb->global_number, pdn->busno,
+		PCI_SLOT(pdn->devfn), PCI_FUNC(pdn->devfn));
+
 	/* Retrieve OF node and eeh device */
 	edev = pdn_to_eeh_dev(pdn);
 	if (!edev || edev->pe)

@@ -294,7 +300,12 @@ static void *pseries_eeh_probe(struct pci_dn *pdn, void *data)
 

Quoted 14 lines

arch/powerpc/platforms/pseries/eeh_pseries.c static void *pseries_eeh_probe(struct pci_dn *pdn, void *data) · line 308

Quoted 9 lines

arch/powerpc/platforms/pseries/eeh_pseries.c +6 -1

 	/* Enable EEH on the device */
 	ret = eeh_ops->set_option(&pe, EEH_OPT_ENABLE);
-	if (!ret) {
+	if (ret) {
+		pr_debug("%s: EEH failed to enable on %02x:%02x.%01x PHB#%x-PE#%x (code %d)\n",
+			__func__, pdn->busno, PCI_SLOT(pdn->devfn),
+			PCI_FUNC(pdn->devfn), pe.phb->global_number,
+			pe.addr, ret);
+	} else {

edev!=NULL here so you could do dev_dbg(&edev->pdev->dev,...) and skip PCI_SLOT/PCI_FUNC. Or is (edev!=NULL && edev->pdev==NULL) possible (it could be, just asking)?

I can see that edev will be non-NULL here, but that pr_debug() pattern (using the PDN information to form the PCI address) is quite common across the EEH code, so I think rather than changing a couple of specific cases, I should do a separate cleanup patch and introduce something like pdn_debug(pdn, "...."). What do you think?

(I don't know exactly when edev->pdev can be NULL.)

Quoted 37 lines

Quoted 18 lines

arch/powerpc/platforms/pseries/eeh_pseries.c +0 -5

 		/* Retrieve PE address */
 		edev->pe_config_addr = eeh_ops->get_pe_addr(&pe);
 		pe.addr = edev->pe_config_addr;

@@ -310,11 +321,6 @@ static void *pseries_eeh_probe(struct pci_dn *pdn, void *data)
 		if (enable) {
 			eeh_add_flag(EEH_ENABLED);
 			eeh_add_to_parent_pe(edev);
-
-			pr_debug("%s: EEH enabled on %02x:%02x.%01x PHB#%x-PE#%x\n",
-				__func__, pdn->busno, PCI_SLOT(pdn->devfn),
-				PCI_FUNC(pdn->devfn), pe.phb->global_number,
-				pe.addr);
 		} else if (pdn->parent && pdn_to_eeh_dev(pdn->parent) &&
 			   (pdn_to_eeh_dev(pdn->parent))->pe) {
 			/* This device doesn't support EEH, but it may have an

arch/powerpc/platforms/pseries/eeh_pseries.c static void *pseries_eeh_probe(struct pci_dn *pdn, void *data) · line 336

Quoted 8 lines

arch/powerpc/platforms/pseries/eeh_pseries.c +5 -0

@@ -323,6 +329,11 @@ static void *pseries_eeh_probe(struct pci_dn *pdn, void *data)
 			edev->pe_config_addr = pdn_to_eeh_dev(pdn->parent)->pe_config_addr;
 			eeh_add_to_parent_pe(edev);
 		}
+		pr_debug("%s: EEH %s on %02x:%02x.%01x PHB#%x-PE#%x (code %d)\n",
+			__func__, (enable ? "enabled" : "unsupported"),
+			pdn->busno, PCI_SLOT(pdn->devfn),
+			PCI_FUNC(pdn->devfn), pe.phb->global_number,
+			pe.addr, ret);

Same here. I understand though this one is a cut-n-paste :)

Quoted 4 lines

arch/powerpc/platforms/pseries/eeh_pseries.c +0 -0

 	}
 
 	/* Save memory bars */

Signature · 1 line

Alexey

Quoted 8 lines · Sam Bobroff

Quoted 7 lines · Alexey Kardashevskiy

Quoted 5 lines · Sam Bobroff

Also remove useless comment.

Signed-off-by Sam Bobroff <[email protected]>

Reviewed-by Alexey Kardashevskiy <[email protected]>

---

*snip*

Quoted 6 lines

I can see that edev will be non-NULL here, but that pr_debug() pattern (using the PDN information to form the PCI address) is quite common across the EEH code, so I think rather than changing a couple of specific cases, I should do a separate cleanup patch and introduce something like pdn_debug(pdn, "...."). What do you think?

I'd switch them all to already existing dev_dbg/pci_debug rather than adding pdn_debug as imho it should not have been used in the first place really...

Quoted 1 line

(I don't know exactly when edev->pdev can be NULL.)

... and if you switch to dev_dbg/pci_debug, I think quite soon you'll know if it can or cannot be NULL :)

Quoted 18 lines · Alexey Kardashevskiy

Quoted 8 lines · Sam Bobroff

Quoted 7 lines · Alexey Kardashevskiy

Quoted 5 lines · Sam Bobroff

Also remove useless comment.

Signed-off-by Sam Bobroff <[email protected]>

Reviewed-by Alexey Kardashevskiy <[email protected]>

---

*snip*

Quoted 6 lines

I can see that edev will be non-NULL here, but that pr_debug() pattern (using the PDN information to form the PCI address) is quite common across the EEH code, so I think rather than changing a couple of specific cases, I should do a separate cleanup patch and introduce something like pdn_debug(pdn, "...."). What do you think?

Quoted 8 lines

I'd switch them all to already existing dev_dbg/pci_debug rather than adding pdn_debug as imho it should not have been used in the first place really...

Quoted 1 line

(I don't know exactly when edev->pdev can be NULL.)

... and if you switch to dev_dbg/pci_debug, I think quite soon you'll know if it can or cannot be NULL :)

As far as I can tell edev->pdev is NULL in two cases:

1. Before eeh_device_add_late() has been called on the pdev. The late part of the add maps the pdev to an edev and sets the pdev's edev pointer and vis a vis.
2. While recoverying EEH unaware devices. Unaware devices are destroyed and rescanned and the edev->pdev pointer is cleared by pcibios_device_release()

In most of these cases it should be safe to use the pci_*() functions rather than making a new one up for printing pdns. In the cases where we might not have a PCI dev i'd make a new set of prints that take an EEH dev rather than a pci_dn since i'd like pci_dn to die sooner rather than later.

Oliver

Quoted 34 lines · Oliver O'Halloran

Quoted 15 lines · Alexey Kardashevskiy

Quoted 7 lines · Sam Bobroff

Quoted 6 lines · Alexey Kardashevskiy

Quoted 5 lines · Sam Bobroff

Also remove useless comment.

Signed-off-by Sam Bobroff <[email protected]>

Reviewed-by Alexey Kardashevskiy <[email protected]>

---

*snip*

Quoted 5 lines

I can see that edev will be non-NULL here, but that pr_debug() pattern (using the PDN information to form the PCI address) is quite common across the EEH code, so I think rather than changing a couple of specific cases, I should do a separate cleanup patch and introduce something like pdn_debug(pdn, "...."). What do you think?

Quoted 8 lines

I'd switch them all to already existing dev_dbg/pci_debug rather than adding pdn_debug as imho it should not have been used in the first place really...

Quoted 1 line

(I don't know exactly when edev->pdev can be NULL.)

... and if you switch to dev_dbg/pci_debug, I think quite soon you'll know if it can or cannot be NULL :)

As far as I can tell edev->pdev is NULL in two cases:

1. Before eeh_device_add_late() has been called on the pdev. The late part of the add maps the pdev to an edev and sets the pdev's edev pointer and vis a vis.
2. While recoverying EEH unaware devices. Unaware devices are destroyed and rescanned and the edev->pdev pointer is cleared by pcibios_device_release()

Quoted 7 lines

In most of these cases it should be safe to use the pci_*() functions rather than making a new one up for printing pdns. In the cases where we might not have a PCI dev i'd make a new set of prints that take an EEH dev rather than a pci_dn since i'd like pci_dn to die sooner rather than later.

Oliver

I'll change the calls in {pnv,pseries}_pcibios_bus_add_device() and eeh_add_device_late() to use dev_dbg() and post a new version.

For {pnv,pseries}_eeh_probe() I'm not sure what we can do; there's no pci_dev available yet and while it would be nice to use the eeh_dev rather than the pdn, it doesn't seem to have the bus/device/fn information we need. Am I missing something there? (The code in the probe functions seems to get it from the pci_dn.)

If there isn't an easy way around this, would it therefore be reasonable to just leave them open-coded as they are?

Quoted 43 lines · Sam Bobroff

Quoted 34 lines · Oliver O'Halloran

Quoted 15 lines · Alexey Kardashevskiy

Quoted 7 lines · Sam Bobroff

Quoted 6 lines · Alexey Kardashevskiy

Quoted 5 lines · Sam Bobroff

Also remove useless comment.

Signed-off-by Sam Bobroff <[email protected]>

Reviewed-by Alexey Kardashevskiy <[email protected]>

---

*snip*

Quoted 5 lines

I can see that edev will be non-NULL here, but that pr_debug() pattern (using the PDN information to form the PCI address) is quite common across the EEH code, so I think rather than changing a couple of specific cases, I should do a separate cleanup patch and introduce something like pdn_debug(pdn, "...."). What do you think?

Quoted 8 lines

I'd switch them all to already existing dev_dbg/pci_debug rather than adding pdn_debug as imho it should not have been used in the first place really...

Quoted 1 line

(I don't know exactly when edev->pdev can be NULL.)

... and if you switch to dev_dbg/pci_debug, I think quite soon you'll know if it can or cannot be NULL :)

As far as I can tell edev->pdev is NULL in two cases:

1. Before eeh_device_add_late() has been called on the pdev. The late part of the add maps the pdev to an edev and sets the pdev's edev pointer and vis a vis.
2. While recoverying EEH unaware devices. Unaware devices are destroyed and rescanned and the edev->pdev pointer is cleared by pcibios_device_release()

Quoted 7 lines

In most of these cases it should be safe to use the pci_*() functions rather than making a new one up for printing pdns. In the cases where we might not have a PCI dev i'd make a new set of prints that take an EEH dev rather than a pci_dn since i'd like pci_dn to die sooner rather than later.

Oliver

Quoted 8 lines

I'll change the calls in {pnv,pseries}_pcibios_bus_add_device() and eeh_add_device_late() to use dev_dbg() and post a new version.

For {pnv,pseries}_eeh_probe() I'm not sure what we can do; there's no pci_dev available yet and while it would be nice to use the eeh_dev rather than the pdn, it doesn't seem to have the bus/device/fn information we need. Am I missing something there? (The code in the probe functions seems to get it from the pci_dn.)

We do have a pci_dev in the powernv case since pnv_eeh_probe() isn't called until the late probe happens (which is after the pci_dev has been created). I've got some patches to rework the probe path to make this a bit clearer, but they need a bit more work.

Quoted 1 line

Quoted 2 lines

If there isn't an easy way around this, would it therefore be reasonable to just leave them open-coded as they are?

I've had this patch floating around a while that should do the trick. The PCI_BUSNO macro is probably unnecessary since I'm sure there is something that does it in generic code, but I couldn't find it.

From 61ff8c23c4d13ff640fb2d069dcedacdf2ee22dd Mon Sep 17 00:00:00 2001
From: Oliver O'Halloran <[email protected]>
Date: Thu, 18 Apr 2019 18:25:13 +1000
Subject: [PATCH] powerpc/eeh: Add bdfn field to eeh_dev

Preperation for removing pci_dn from the powernv EEH code. The only thing we really use pci_dn for is to get the bdfn of the device for config space accesses, so adding that information to eeh_dev reduces the need to carry around the pci_dn.

Signed-off-by Oliver O'Halloran <[email protected]>

---
 arch/powerpc/include/asm/eeh.h     | 2 ++
 arch/powerpc/include/asm/ppc-pci.h | 2 ++
 arch/powerpc/kernel/eeh_dev.c      | 2 ++
 3 files changed, 6 insertions(+)

arch/powerpc/include/asm/eeh.h +2 -0

index 7fd476d..a208e02 100644
@@ -131,6 +131,8 @@ static inline bool eeh_pe_passed(struct eeh_pe *pe)
 struct eeh_dev {
 	int mode;			/* EEH mode			*/
 	int class_code;			/* Class code of the device	*/
+	int bdfn; 			/* bdfn of device (for cfg ops) */
+	struct pci_controller *controller;
 	int pe_config_addr;		/* PE config address		*/
 	u32 config_space[16];		/* Saved PCI config space	*/
 	int pcix_cap;			/* Saved PCIx capability	*/

arch/powerpc/include/asm/ppc-pci.h +2 -0

index cec2d64..72860de 100644
@@ -74,6 +74,8 @@ static inline const char *eeh_driver_name(struct pci_dev *pdev)
 
 #endif /* CONFIG_EEH */
 
+#define PCI_BUSNO(bdfn) ((bdfn >> 8) & 0xff)
+
 #else /* CONFIG_PCI */
 static inline void init_pci_config_tokens(void) { }
 #endif /* !CONFIG_PCI */

arch/powerpc/kernel/eeh_dev.c +2 -0

index c4317c4..7370185 100644
@@ -47,6 +47,8 @@ struct eeh_dev *eeh_dev_init(struct pci_dn *pdn)
 	/* Associate EEH device with OF node */
 	pdn->edev = edev;
 	edev->pdn = pdn;
+	edev->bdfn = (pdn->busno << 8) | pdn->devfn;
+	edev->controller = pdn->phb;
 
 	return edev;
 }

Signature · 1 line

2.9.5

Now that EEH support for all devices (on PowerNV and pSeries) is provided by the pcibios bus add device hooks, eeh_probe_devices() and eeh_addr_cache_build() are redundant and can be removed.

Move the EEH enabled message into it's own function so that it can be called from multiple places.

Note that previously on pSeries, useless EEH sysfs files were created for some devices that did not have EEH support and this change prevents them from being created.

Signed-off-by Sam Bobroff <[email protected]>

---
v2 - As it's so small, merged the enablement message patch into this one (where it's used).
- Reworked enablement messages.

 arch/powerpc/include/asm/eeh.h               |  7 ++---
 arch/powerpc/kernel/eeh.c                    | 27 ++++++-----------
 arch/powerpc/kernel/eeh_cache.c              | 32 --------------------
 arch/powerpc/platforms/powernv/eeh-powernv.c |  4 +--
 arch/powerpc/platforms/pseries/pci.c         |  3 +-
 5 files changed, 14 insertions(+), 59 deletions(-)

arch/powerpc/include/asm/eeh.h +2 -5

index 12baf1df134c..3994d45ae0d4 100644
@@ -283,13 +283,12 @@ struct pci_bus *eeh_pe_bus_get(struct eeh_pe *pe);
 
 struct eeh_dev *eeh_dev_init(struct pci_dn *pdn);
 void eeh_dev_phb_init_dynamic(struct pci_controller *phb);
-void eeh_probe_devices(void);
+void eeh_show_enabled(void);
 int __init eeh_ops_register(struct eeh_ops *ops);
 int __exit eeh_ops_unregister(const char *name);
 int eeh_check_failure(const volatile void __iomem *token);
 int eeh_dev_check_failure(struct eeh_dev *edev);
 void eeh_addr_cache_init(void);
-void eeh_addr_cache_build(void);
 void eeh_add_device_early(struct pci_dn *);
 void eeh_add_device_tree_early(struct pci_dn *);
 void eeh_add_device_late(struct pci_dev *);

@@ -333,7 +332,7 @@ static inline bool eeh_enabled(void)
         return false;
 }
 
-static inline void eeh_probe_devices(void) { }
+static inline void eeh_show_enabled(void) { }
 
 static inline void *eeh_dev_init(struct pci_dn *pdn, void *data)
 {

@@ -351,8 +350,6 @@ static inline int eeh_check_failure(const volatile void __iomem *token)
 
 static inline void eeh_addr_cache_init(void) { }
 
-static inline void eeh_addr_cache_build(void) { }
-
 static inline void eeh_add_device_early(struct pci_dn *pdn) { }
 
 static inline void eeh_add_device_tree_early(struct pci_dn *pdn) { }

arch/powerpc/kernel/eeh.c +10 -6

index 1ed80adb40a1..f905235f0307 100644
@@ -163,6 +163,16 @@ static int __init eeh_setup(char *str)
 }
 __setup("eeh=", eeh_setup);
 
+void eeh_show_enabled(void)
+{
+       if (eeh_has_flag(EEH_FORCE_DISABLED))
+               pr_info("EEH: Recovery disabled by kernel parameter.\n");
+       else if (eeh_has_flag(EEH_ENABLED))
+               pr_info("EEH: Capable adapter found: recovery enabled.\n");
+       else
+               pr_info("EEH: No capable adapters found: recovery disabled.\n");
+}
+
 /*
  * This routine captures assorted PCI configuration space data
  * for the indicated PCI device, and puts them into a buffer

@@ -1156,23 +1166,6 @@ static struct notifier_block eeh_reboot_nb = {
        .notifier_call = eeh_reboot_notifier,
 };
 
 
-void eeh_probe_devices(void)
-{
-       struct pci_controller *hose, *tmp;
-       struct pci_dn *pdn;
-
-       /* Enable EEH for all adapters */

arch/powerpc/kernel/eeh.c +0 -0

 /**
  * eeh_init - EEH initialization
  *

arch/powerpc/kernel/eeh_cache.c +0 -32

index f93dd5cf6a39..c40078d036af 100644
@@ -278,38 +278,6 @@ void eeh_addr_cache_init(void)
        spin_lock_init(&pci_io_addr_cache_root.piar_lock);
 }
 
-/**
- * eeh_addr_cache_build - Build a cache of I/O addresses
- *
- * Build a cache of pci i/o addresses.  This cache will be used to
- * find the pci device that corresponds to a given address.
- * This routine scans all pci busses to build the cache.
- * Must be run late in boot process, after the pci controllers
- * have been scanned for devices (after all device resources are known).
- */
-void eeh_addr_cache_build(void)
-{
-       struct pci_dn *pdn;
-       struct eeh_dev *edev;
-       struct pci_dev *dev = NULL;
-
-       for_each_pci_dev(dev) {
-               pdn = pci_get_pdn_by_devfn(dev->bus, dev->devfn);
-               if (!pdn)
-                       continue;
-
-               edev = pdn_to_eeh_dev(pdn);
-               if (!edev)
-                       continue;
-
-               dev->dev.archdata.edev = edev;
-               edev->pdev = dev;
-
-               eeh_addr_cache_insert_dev(dev);
-               eeh_sysfs_add_device(dev);
-       }
-}
-
 static int eeh_addr_cache_show(struct seq_file *s, void *v)
 {
        struct pci_io_addr_range *piar;

arch/powerpc/platforms/powernv/eeh-powernv.c +1 -3

index 90729d908a54..22a94f4b8586 100644
@@ -259,9 +259,7 @@ int pnv_eeh_post_init(void)
        struct pnv_phb *phb;
        int ret = 0;
 
-       /* Probe devices & build address cache */
-       eeh_probe_devices();
-       eeh_addr_cache_build();
+       eeh_show_enabled();
 
        /* Register OPAL event notifier */
        eeh_event_irq = opal_event_request(ilog2(OPAL_EVENT_PCI_ERROR));

arch/powerpc/platforms/pseries/pci.c +1 -3

index 37a77e57893e..d6a5f4f27507 100644
@@ -242,8 +242,7 @@ void __init pSeries_final_fixup(void)
 
        pSeries_request_regions();
 
-       eeh_probe_devices();
-       eeh_addr_cache_build();
+       eeh_show_enabled();
 
 #ifdef CONFIG_PCI_IOV
        ppc_md.pcibios_sriov_enable = pseries_pcibios_sriov_enable;
--

2.19.0.2.gcad72f5712

Quoted 93 lines · Sam Bobroff

Now that EEH support for all devices (on PowerNV and pSeries) is provided by the pcibios bus add device hooks, eeh_probe_devices() and eeh_addr_cache_build() are redundant and can be removed.

Move the EEH enabled message into it's own function so that it can be called from multiple places.

Note that previously on pSeries, useless EEH sysfs files were created for some devices that did not have EEH support and this change prevents them from being created.

Signed-off-by Sam Bobroff <[email protected]>

---
v2 - As it's so small, merged the enablement message patch into this one (where it's used).
- Reworked enablement messages.

 arch/powerpc/include/asm/eeh.h               |  7 ++---
 arch/powerpc/kernel/eeh.c                    | 27 ++++++-----------
 arch/powerpc/kernel/eeh_cache.c              | 32 --------------------
 arch/powerpc/platforms/powernv/eeh-powernv.c |  4 +--
 arch/powerpc/platforms/pseries/pci.c         |  3 +-
 5 files changed, 14 insertions(+), 59 deletions(-)

arch/powerpc/include/asm/eeh.h +2 -5

index 12baf1df134c..3994d45ae0d4 100644
@@ -283,13 +283,12 @@ struct pci_bus *eeh_pe_bus_get(struct eeh_pe *pe);
 
 struct eeh_dev *eeh_dev_init(struct pci_dn *pdn);
 void eeh_dev_phb_init_dynamic(struct pci_controller *phb);
-void eeh_probe_devices(void);
+void eeh_show_enabled(void);
 int __init eeh_ops_register(struct eeh_ops *ops);
 int __exit eeh_ops_unregister(const char *name);
 int eeh_check_failure(const volatile void __iomem *token);
 int eeh_dev_check_failure(struct eeh_dev *edev);
 void eeh_addr_cache_init(void);
-void eeh_addr_cache_build(void);
 void eeh_add_device_early(struct pci_dn *);
 void eeh_add_device_tree_early(struct pci_dn *);
 void eeh_add_device_late(struct pci_dev *);

@@ -333,7 +332,7 @@ static inline bool eeh_enabled(void)
         return false;
 }
 
-static inline void eeh_probe_devices(void) { }
+static inline void eeh_show_enabled(void) { }
 
 static inline void *eeh_dev_init(struct pci_dn *pdn, void *data)
 {

@@ -351,8 +350,6 @@ static inline int eeh_check_failure(const volatile void __iomem *token)
 
 static inline void eeh_addr_cache_init(void) { }
 
-static inline void eeh_addr_cache_build(void) { }
-
 static inline void eeh_add_device_early(struct pci_dn *pdn) { }
 
 static inline void eeh_add_device_tree_early(struct pci_dn *pdn) { }

arch/powerpc/kernel/eeh.c +10 -6

index 1ed80adb40a1..f905235f0307 100644
@@ -163,6 +163,16 @@ static int __init eeh_setup(char *str)
 }
 __setup("eeh=", eeh_setup);
 
+void eeh_show_enabled(void)
+{
+       if (eeh_has_flag(EEH_FORCE_DISABLED))
+               pr_info("EEH: Recovery disabled by kernel parameter.\n");
+       else if (eeh_has_flag(EEH_ENABLED))
+               pr_info("EEH: Capable adapter found: recovery enabled.\n");
+       else
+               pr_info("EEH: No capable adapters found: recovery disabled.\n");
+}
+
 /*
  * This routine captures assorted PCI configuration space data
  * for the indicated PCI device, and puts them into a buffer

@@ -1156,23 +1166,6 @@ static struct notifier_block eeh_reboot_nb = {
        .notifier_call = eeh_reboot_notifier,
 };
 
 
-void eeh_probe_devices(void)
-{
-       struct pci_controller *hose, *tmp;
-       struct pci_dn *pdn;
-
-       /* Enable EEH for all adapters */

arch/powerpc/kernel/eeh.c static struct notifier_block eeh_reboot_nb = { · line 1175

Quoted 10 lines

arch/powerpc/kernel/eeh.c +0 -10

-       list_for_each_entry_safe(hose, tmp, &hose_list, list_node) {
-               pdn = hose->pci_data;
-               traverse_pci_dn(pdn, eeh_ops->probe, NULL);
-       }
-       if (eeh_enabled())
-               pr_info("EEH: PCI Enhanced I/O Error Handling Enabled\n");
-       else
-               pr_info("EEH: No capable adapters found\n");
-
-}

The one concern I have about this is that PAPR requires us to enable EEH for the device before we do any config accesses. From PAPR:

R1–7.3.11.1–5. For the EEH option: If a device driver is going to enable EEH and the platform has not defaulted
to EEH enabled, then it must do so before it does any operations with its IOA, including any configuration
cycles or Load or Store operations.

So if we want to be strictly compatible we'd need to ensure the set-eeh-option RTAS call happens before we read the VDID in pci_scan_device(). The pseries eeh_probe() function does this currently, but if we defer it until the pcibios call happens we'll have done a pile of config accesses before then. Maybe it doesn't matter, but we'd need to do further testing under phyp or work out some other way to ensure it's done pre-probe.

Quoted 78 lines

arch/powerpc/kernel/eeh.c +0 -0

 /**
  * eeh_init - EEH initialization
  *

arch/powerpc/kernel/eeh_cache.c +0 -32

index f93dd5cf6a39..c40078d036af 100644
@@ -278,38 +278,6 @@ void eeh_addr_cache_init(void)
        spin_lock_init(&pci_io_addr_cache_root.piar_lock);
 }
 
-/**
- * eeh_addr_cache_build - Build a cache of I/O addresses
- *
- * Build a cache of pci i/o addresses.  This cache will be used to
- * find the pci device that corresponds to a given address.
- * This routine scans all pci busses to build the cache.
- * Must be run late in boot process, after the pci controllers
- * have been scanned for devices (after all device resources are known).
- */
-void eeh_addr_cache_build(void)
-{
-       struct pci_dn *pdn;
-       struct eeh_dev *edev;
-       struct pci_dev *dev = NULL;
-
-       for_each_pci_dev(dev) {
-               pdn = pci_get_pdn_by_devfn(dev->bus, dev->devfn);
-               if (!pdn)
-                       continue;
-
-               edev = pdn_to_eeh_dev(pdn);
-               if (!edev)
-                       continue;
-
-               dev->dev.archdata.edev = edev;
-               edev->pdev = dev;
-
-               eeh_addr_cache_insert_dev(dev);
-               eeh_sysfs_add_device(dev);
-       }
-}
-
 static int eeh_addr_cache_show(struct seq_file *s, void *v)
 {
        struct pci_io_addr_range *piar;

arch/powerpc/platforms/powernv/eeh-powernv.c +1 -3

index 90729d908a54..22a94f4b8586 100644
@@ -259,9 +259,7 @@ int pnv_eeh_post_init(void)
        struct pnv_phb *phb;
        int ret = 0;
 
-       /* Probe devices & build address cache */
-       eeh_probe_devices();
-       eeh_addr_cache_build();
+       eeh_show_enabled();
 
        /* Register OPAL event notifier */
        eeh_event_irq = opal_event_request(ilog2(OPAL_EVENT_PCI_ERROR));

arch/powerpc/platforms/pseries/pci.c +1 -3

index 37a77e57893e..d6a5f4f27507 100644
@@ -242,8 +242,7 @@ void __init pSeries_final_fixup(void)
 
        pSeries_request_regions();
 
-       eeh_probe_devices();
-       eeh_addr_cache_build();
+       eeh_show_enabled();
 
 #ifdef CONFIG_PCI_IOV
        ppc_md.pcibios_sriov_enable = pseries_pcibios_sriov_enable;
--

2.19.0.2.gcad72f5712