Re: [PATCH v4 09/21] powerpc/powernv: Use PCI slot reset infrastructure

[Gavin Shan <gwshan@linux.vnet.ibm.com>]

On Sat, May 09, 2015 at 11:41:05PM +1000, Alexey Kardashevskiy wrote:
>On 05/01/2015 04:02 PM, Gavin Shan wrote:
>>For PowerNV platform, running on top of skiboot, all PE level reset
>>should be routed to firmware if the bridge of the PE primary bus has
>>device-node property "ibm,reset-by-firmware". Otherwise, the kernel
>>has to issue hot reset on PE's primary bus despite the requested reset
>>types, which is the behaviour before the firmware supports PCI slot
>>reset. So the changes don't depend on the PCI slot reset capability
>>exposed from the firmware.
>>
>>Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>>---
>>  arch/powerpc/include/asm/eeh.h               |   1 +
>>  arch/powerpc/include/asm/opal.h              |   4 +-
>>  arch/powerpc/platforms/powernv/eeh-powernv.c | 206 +++++++++++++--------------
>>  3 files changed, 102 insertions(+), 109 deletions(-)
>>
>>diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
>>index c5eb86f..2793d24 100644
>>--- a/arch/powerpc/include/asm/eeh.h
>>+++ b/arch/powerpc/include/asm/eeh.h
>>@@ -190,6 +190,7 @@ enum {
>>  #define EEH_RESET_DEACTIVATE	0	/* Deactivate the PE reset	*/
>>  #define EEH_RESET_HOT		1	/* Hot reset			*/
>>  #define EEH_RESET_FUNDAMENTAL	3	/* Fundamental reset		*/
>>+#define EEH_RESET_COMPLETE	4	/* PHB complete reset           */
>>  #define EEH_LOG_TEMP		1	/* EEH temporary error log	*/
>>  #define EEH_LOG_PERM		2	/* EEH permanent error log	*/
>>
>>diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
>>index 042af1a..6d467df 100644
>>--- a/arch/powerpc/include/asm/opal.h
>>+++ b/arch/powerpc/include/asm/opal.h
>>@@ -129,7 +129,7 @@ int64_t opal_pci_map_pe_dma_window(uint64_t phb_id, uint16_t pe_number, uint16_t
>>  int64_t opal_pci_map_pe_dma_window_real(uint64_t phb_id, uint16_t pe_number,
>>  					uint16_t dma_window_number, uint64_t pci_start_addr,
>>  					uint64_t pci_mem_size);
>>-int64_t opal_pci_reset(uint64_t phb_id, uint8_t reset_scope, uint8_t assert_state);
>>+int64_t opal_pci_reset(uint64_t id, uint8_t reset_scope, uint8_t assert_state);
>>
>>  int64_t opal_pci_get_hub_diag_data(uint64_t hub_id, void *diag_buffer,
>>  				   uint64_t diag_buffer_len);
>>@@ -145,7 +145,7 @@ int64_t opal_get_epow_status(__be64 *status);
>>  int64_t opal_set_system_attention_led(uint8_t led_action);
>>  int64_t opal_pci_next_error(uint64_t phb_id, __be64 *first_frozen_pe,
>>  			    __be16 *pci_error_type, __be16 *severity);
>>-int64_t opal_pci_poll(uint64_t phb_id);
>>+int64_t opal_pci_poll(uint64_t id, uint8_t *val);
>>  int64_t opal_return_cpu(void);
>>  int64_t opal_check_token(uint64_t token);
>>  int64_t opal_reinit_cpus(uint64_t flags);
>>diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c b/arch/powerpc/platforms/powernv/eeh-powernv.c
>>index ce738ab..3c01095 100644
>>--- a/arch/powerpc/platforms/powernv/eeh-powernv.c
>>+++ b/arch/powerpc/platforms/powernv/eeh-powernv.c
>>@@ -742,12 +742,12 @@ static int pnv_eeh_get_state(struct eeh_pe *pe, int *delay)
>>  	return ret;
>>  }
>>
>>-static s64 pnv_eeh_phb_poll(struct pnv_phb *phb)
>>+static s64 pnv_eeh_poll(uint64_t id)
>>  {
>>  	s64 rc = OPAL_HARDWARE;
>>
>>  	while (1) {
>>-		rc = opal_pci_poll(phb->opal_id);
>>+		rc = opal_pci_poll(id, NULL);
>>  		if (rc <= 0)
>>  			break;
>>
>>@@ -763,84 +763,38 @@ static s64 pnv_eeh_phb_poll(struct pnv_phb *phb)
>>  int pnv_eeh_phb_reset(struct pci_controller *hose, int option)
>>  {
>>  	struct pnv_phb *phb = hose->private_data;
>>+	uint8_t scope;
>>  	s64 rc = OPAL_HARDWARE;
>>
>>  	pr_debug("%s: Reset PHB#%x, option=%d\n",
>>  		 __func__, hose->global_number, option);
>>-
>>-	/* Issue PHB complete reset request */
>>-	if (option == EEH_RESET_FUNDAMENTAL ||
>>-	    option == EEH_RESET_HOT)
>>-		rc = opal_pci_reset(phb->opal_id,
>>-				    OPAL_RESET_PHB_COMPLETE,
>>-				    OPAL_ASSERT_RESET);
>>-	else if (option == EEH_RESET_DEACTIVATE)
>>-		rc = opal_pci_reset(phb->opal_id,
>>-				    OPAL_RESET_PHB_COMPLETE,
>>-				    OPAL_DEASSERT_RESET);
>>-	if (rc < 0)
>>-		goto out;
>>-
>>-	/*
>>-	 * Poll state of the PHB until the request is done
>>-	 * successfully. The PHB reset is usually PHB complete
>>-	 * reset followed by hot reset on root bus. So we also
>>-	 * need the PCI bus settlement delay.
>>-	 */
>>-	rc = pnv_eeh_phb_poll(phb);
>>-	if (option == EEH_RESET_DEACTIVATE) {
>>-		if (system_state < SYSTEM_RUNNING)
>>-			udelay(1000 * EEH_PE_RST_SETTLE_TIME);
>>-		else
>>-			msleep(EEH_PE_RST_SETTLE_TIME);
>
>
>These udelay() and msleep() are gone. How come they are not needed anymore?
>Worth commenting in the commit log or remove those in a separate patch.
>
>I just remember you mentioning some missing delays somewhere which caused
>NVIDIA device to issue EEH and I do not want those to disappear :)
>

Yeah, I think you're correct that it's not safe to remove this yet because
the old firmware (without corresponding PCI hotplug changes) doesn't have
the required delays from opal_pci_poll() yet. I'll add this in next revision.

>
>>+	switch (option) {
>>+	case EEH_RESET_HOT:
>>+		scope = OPAL_RESET_PCI_HOT;
>>+		break;
>>+	case EEH_RESET_FUNDAMENTAL:
>>+		scope = OPAL_RESET_PCI_FUNDAMENTAL;
>>+		break;
>>+	case EEH_RESET_COMPLETE:
>>+		scope = OPAL_RESET_PHB_COMPLETE;
>>+		break;
>>+	case EEH_RESET_DEACTIVATE:
>>+		return 0;
>>+	default:
>>+		pr_warn("%s: Unsupported option %d\n",
>>+			__func__, option);
>>+		return -EINVAL;
>>  	}
>>-out:
>>-	if (rc != OPAL_SUCCESS)
>>-		return -EIO;
>>
>>-	return 0;
>>-}
>>-
>>-static int pnv_eeh_root_reset(struct pci_controller *hose, int option)
>>-{
>>-	struct pnv_phb *phb = hose->private_data;
>>-	s64 rc = OPAL_HARDWARE;
>>+	/* Issue reset and poll until it's completed */
>>+	rc = opal_pci_reset(phb->opal_id, scope, OPAL_ASSERT_RESET);
>>+	if (rc > 0)
>>+		rc = pnv_eeh_poll(phb->opal_id);
>>
>>-	pr_debug("%s: Reset PHB#%x, option=%d\n",
>>-		 __func__, hose->global_number, option);
>>-
>>-	/*
>>-	 * During the reset deassert time, we needn't care
>>-	 * the reset scope because the firmware does nothing
>>-	 * for fundamental or hot reset during deassert phase.
>>-	 */
>>-	if (option == EEH_RESET_FUNDAMENTAL)
>>-		rc = opal_pci_reset(phb->opal_id,
>>-				    OPAL_RESET_PCI_FUNDAMENTAL,
>>-				    OPAL_ASSERT_RESET);
>>-	else if (option == EEH_RESET_HOT)
>>-		rc = opal_pci_reset(phb->opal_id,
>>-				    OPAL_RESET_PCI_HOT,
>>-				    OPAL_ASSERT_RESET);
>>-	else if (option == EEH_RESET_DEACTIVATE)
>>-		rc = opal_pci_reset(phb->opal_id,
>>-				    OPAL_RESET_PCI_HOT,
>>-				    OPAL_DEASSERT_RESET);
>>-	if (rc < 0)
>>-		goto out;
>>-
>>-	/* Poll state of the PHB until the request is done */
>>-	rc = pnv_eeh_phb_poll(phb);
>>-	if (option == EEH_RESET_DEACTIVATE)
>>-		msleep(EEH_PE_RST_SETTLE_TIME);
>>-out:
>>-	if (rc != OPAL_SUCCESS)
>>-		return -EIO;
>>-
>>-	return 0;
>>+	return (rc == OPAL_SUCCESS) ? 0 : -EIO;
>>  }
>>
>>-static int pnv_eeh_bridge_reset(struct pci_dev *dev, int option)
>>+static int __pnv_eeh_bridge_reset(struct pci_dev *dev, int option)
>>  {
>>  	struct pci_dn *pdn = pci_get_pdn_by_devfn(dev->bus, dev->devfn);
>>  	struct eeh_dev *edev = pdn_to_eeh_dev(pdn);
>>@@ -891,14 +845,57 @@ static int pnv_eeh_bridge_reset(struct pci_dev *dev, int option)
>>  	return 0;
>>  }
>>
>>+static int pnv_eeh_bridge_reset(struct pci_dev *dev, int option)
>>+{
>>+	struct pci_controller *hose;
>>+	struct pnv_phb *phb;
>>+	struct device_node *dn = dev ? pci_device_to_OF_node(dev) : NULL;
>>+	uint64_t id = (0x1ul << 60);
>>+	uint8_t scope;
>>+	s64 rc;
>
>
>int64_t for @rc?
>
>

Yes.

>>+
>>+	/*
>>+	 * If the firmware can't handle it, we will issue hot reset
>>+	 * on the secondary bus despite the requested reset type
>>+	 */
>>+	if (!dn || !of_get_property(dn, "ibm,reset-by-firmware", NULL))
>>+		return __pnv_eeh_bridge_reset(dev, option);
>>+
>>+	/* The firmware can handle the request */
>>+	switch (option) {
>>+	case EEH_RESET_HOT:
>>+		scope = OPAL_RESET_PCI_HOT;
>>+		break;
>>+	case EEH_RESET_FUNDAMENTAL:
>>+		scope = OPAL_RESET_PCI_FUNDAMENTAL;
>>+		break;
>>+	case EEH_RESET_DEACTIVATE:
>>+		return 0;
>>+	case EEH_RESET_COMPLETE:
>>+	default:
>>+		pr_warn("%s: Unsupported option %d on device %s\n",
>>+			__func__, option, pci_name(dev));
>>+		return -EINVAL;
>>+	}
>
>
>This is the same switch as earlier in this patch (slightly different order).
>Move it and opal_pci_reset() into a helper and call it pnv_opal_pci_reset()?
>
>

It sounds a good idea. I'll do accordingly.

>>+
>>+	hose = pci_bus_to_host(dev->bus);
>>+	phb = hose->private_data;
>
>Previously you would initialize @hose and @phb where you declared those but
>not here. If you did the same thing as before, the patch could have been
>smaller and easier to read.
>

Sure.

>>+	id |= (dev->bus->number << 24) | (dev->devfn << 16) | phb->opal_id;
>>+	rc = opal_pci_reset(id, scope, OPAL_ASSERT_RESET);
>>+	if (rc > 0)
>>+		rc = pnv_eeh_poll(id);
>>+
>>+	return (rc == OPAL_SUCCESS) ? 0 : -EIO;
>>+}
>>+
>>  void pnv_pci_reset_secondary_bus(struct pci_dev *dev)
>>  {
>>  	struct pci_controller *hose;
>>
>>  	if (pci_is_root_bus(dev->bus)) {
>>  		hose = pci_bus_to_host(dev->bus);
>>-		pnv_eeh_root_reset(hose, EEH_RESET_HOT);
>>-		pnv_eeh_root_reset(hose, EEH_RESET_DEACTIVATE);
>>+		pnv_eeh_phb_reset(hose, EEH_RESET_HOT);
>>+		pnv_eeh_phb_reset(hose, EEH_RESET_DEACTIVATE);
>>  	} else {
>>  		pnv_eeh_bridge_reset(dev, EEH_RESET_HOT);
>>  		pnv_eeh_bridge_reset(dev, EEH_RESET_DEACTIVATE);
>>@@ -920,8 +917,9 @@ void pnv_pci_reset_secondary_bus(struct pci_dev *dev)
>>  static int pnv_eeh_reset(struct eeh_pe *pe, int option)
>>  {
>>  	struct pci_controller *hose = pe->phb;
>>+	struct pnv_phb *phb;
>>  	struct pci_bus *bus;
>>-	int ret;
>>+	s64 rc;
>>
>>  	/*
>>  	 * For PHB reset, we always have complete reset. For those PEs whose
>>@@ -937,43 +935,37 @@ static int pnv_eeh_reset(struct eeh_pe *pe, int option)
>>  	 * reset. The side effect is that EEH core has to clear the frozen
>>  	 * state explicitly after BAR restore.
>>  	 */
>>-	if (pe->type & EEH_PE_PHB) {
>>-		ret = pnv_eeh_phb_reset(hose, option);
>>-	} else {
>>-		struct pnv_phb *phb;
>>-		s64 rc;
>>+	if (pe->type & EEH_PE_PHB)
>
>I would keep "{" in the line above ....
>
>>+		return pnv_eeh_phb_reset(hose, EEH_RESET_COMPLETE);
>
>...put "} else {" here...
>
>and the chunk below would become 1) very small 2) very trivial... And then
>you could make a trivial patch which would do scope removal but without
>functional changes. Or vice versa.
>

I intended to remove nested if(). If you really want me to change the code
according to your comments, I'll do. Otherwise, I prefer to keep it as
of being.

>>
>>-		/*
>>-		 * The frozen PE might be caused by PAPR error injection
>>-		 * registers, which are expected to be cleared after hitting
>>-		 * frozen PE as stated in the hardware spec. Unfortunately,
>>-		 * that's not true on P7IOC. So we have to clear it manually
>>-		 * to avoid recursive EEH errors during recovery.
>>-		 */
>>-		phb = hose->private_data;
>>-		if (phb->model == PNV_PHB_MODEL_P7IOC &&
>>-		    (option == EEH_RESET_HOT ||
>>-		    option == EEH_RESET_FUNDAMENTAL)) {
>>-			rc = opal_pci_reset(phb->opal_id,
>>-					    OPAL_RESET_PHB_ERROR,
>>-					    OPAL_ASSERT_RESET);
>>-			if (rc != OPAL_SUCCESS) {
>>-				pr_warn("%s: Failure %lld clearing "
>>-					"error injection registers\n",
>>-					__func__, rc);
>>-				return -EIO;
>>-			}
>>+	/*
>>+	 * The frozen PE might be caused by PAPR error injection
>>+	 * registers, which are expected to be cleared after hitting
>>+	 * frozen PE as stated in the hardware spec. Unfortunately,
>>+	 * that's not true on P7IOC. So we have to clear it manually
>>+	 * to avoid recursive EEH errors during recovery.
>>+	 */
>>+	phb = hose->private_data;
>>+	if (phb->model == PNV_PHB_MODEL_P7IOC &&
>>+	    (option == EEH_RESET_HOT ||
>>+	    option == EEH_RESET_FUNDAMENTAL)) {
>>+		rc = opal_pci_reset(phb->opal_id,
>>+				    OPAL_RESET_PHB_ERROR,
>>+				    OPAL_ASSERT_RESET);
>>+		if (rc != OPAL_SUCCESS) {
>>+			pr_warn("%s: Failure %lld clearing error "
>>+				"injection registers on PHB#%d\n",
>>+				__func__, rc, hose->global_number);
>>+			return -EIO;
>>  		}
>>-
>>-		bus = eeh_pe_bus_get(pe);
>>-		if (pci_is_root_bus(bus) ||
>>-			pci_is_root_bus(bus->parent))
>>-			ret = pnv_eeh_root_reset(hose, option);
>>-		else
>>-			ret = pnv_eeh_bridge_reset(bus->self, option);
>>  	}
>>
>>-	return ret;
>>+	/* Route the reset request to PHB or upstream bridge */
>>+	bus = eeh_pe_bus_get(pe);
>>+	if (pci_is_root_bus(bus))
>>+		return pnv_eeh_phb_reset(hose, option);
>>+
>>+	return pnv_eeh_bridge_reset(bus->self, option);
>>  }
>>
>>  /**
>>

Thanks,
Gavin