Subject: Re: [PATCH] powerpc/eeh: Drop delay in pcibios_set_pcie_reset_state()

From: Gavin Shan <[email-masked]>
Date: Wed, 11 Feb 2015 10:05:52 +1100

On Fri, Jan 23, 2015 at 03:01:42PM +1100, Gavin Shan wrote:
>pcibios_set_pcie_reset_state(), implemented based on platform's EEH reset
>backends, helps to reset PCI adapters. IPR driver might call this function
>in timer handler and it's hard to make reset assertion and settlement delays
>with msleep() in the atomic context. The issue was caused by commit 26833a5
>("powerpc/eeh: Make the delay for PE reset unified").
>
>The patch drops the reset assertion and settlement delays in the API and
>caller will have to take corresponding delays. With the patch applied, the
>reset assertion and settlement delays related to EEH reset become:
>
>  * Reset for EEH recovery: inband
>  * Reset for Guest PE recovery: inband
>  * pcibios_set_pcie_reset_state(): outband
>
>Cc: <[email-masked]> # v3.15+
>Signed-off-by: Gavin Shan <[email-masked]>
>Tested-by: Wen Xiong <[email-masked]>

Ben and Michael, please ignore it: When rethinking the issue, I
believe the driver need change to use non-atomic context to do
reset. This function is still called by pci_reset_function() or
pci_try_reset_function(), on which VFIO PCI driver depends to do
reset and we need the delay for the case.

Thanks,
Gavin

>---
> arch/powerpc/include/asm/eeh.h               |  2 +-
> arch/powerpc/kernel/eeh.c                    | 16 +++++++--------
> arch/powerpc/platforms/powernv/eeh-ioda.c    | 29 ++++++++++++++++------------
> arch/powerpc/platforms/powernv/eeh-powernv.c |  4 ++--
> arch/powerpc/platforms/powernv/pci.h         |  2 +-
> arch/powerpc/platforms/pseries/eeh_pseries.c |  6 +++++-
> 6 files changed, 34 insertions(+), 25 deletions(-)
>
>diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
>index 55abfd0..0bc7d3f 100644
>--- a/arch/powerpc/include/asm/eeh.h
>+++ b/arch/powerpc/include/asm/eeh.h
>@@ -205,7 +205,7 @@ struct eeh_ops {
> 	int (*set_option)(struct eeh_pe *pe, int option);
> 	int (*get_pe_addr)(struct eeh_pe *pe);
> 	int (*get_state)(struct eeh_pe *pe, int *state);
>-	int (*reset)(struct eeh_pe *pe, int option);
>+	int (*reset)(struct eeh_pe *pe, int option, bool delay);
> 	int (*wait_state)(struct eeh_pe *pe, int max_wait);
> 	int (*get_log)(struct eeh_pe *pe, int severity, char *drv_log, unsigned long len);
> 	int (*configure_bridge)(struct eeh_pe *pe);
>diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
>index 3b2252e..b0352b5c 100644
>--- a/arch/powerpc/kernel/eeh.c
>+++ b/arch/powerpc/kernel/eeh.c
>@@ -688,16 +688,16 @@ int pcibios_set_pcie_reset_state(struct pci_dev *dev, enum pcie_reset_state stat
>
> 	switch (state) {
> 	case pcie_deassert_reset:
>-		eeh_ops->reset(pe, EEH_RESET_DEACTIVATE);
>+		eeh_ops->reset(pe, EEH_RESET_DEACTIVATE, false);
> 		eeh_pe_state_clear(pe, EEH_PE_CFG_BLOCKED);
> 		break;
> 	case pcie_hot_reset:
> 		eeh_pe_state_mark(pe, EEH_PE_CFG_BLOCKED);
>-		eeh_ops->reset(pe, EEH_RESET_HOT);
>+		eeh_ops->reset(pe, EEH_RESET_HOT, false);
> 		break;
> 	case pcie_warm_reset:
> 		eeh_pe_state_mark(pe, EEH_PE_CFG_BLOCKED);
>-		eeh_ops->reset(pe, EEH_RESET_FUNDAMENTAL);
>+		eeh_ops->reset(pe, EEH_RESET_FUNDAMENTAL, false);
> 		break;
> 	default:
> 		eeh_pe_state_clear(pe, EEH_PE_CFG_BLOCKED);
>@@ -749,11 +749,11 @@ static void eeh_reset_pe_once(struct eeh_pe *pe)
> 	eeh_pe_dev_traverse(pe, eeh_set_dev_freset, &freset);
>
> 	if (freset)
>-		eeh_ops->reset(pe, EEH_RESET_FUNDAMENTAL);
>+		eeh_ops->reset(pe, EEH_RESET_FUNDAMENTAL, true);
> 	else
>-		eeh_ops->reset(pe, EEH_RESET_HOT);
>+		eeh_ops->reset(pe, EEH_RESET_HOT, true);
>
>-	eeh_ops->reset(pe, EEH_RESET_DEACTIVATE);
>+	eeh_ops->reset(pe, EEH_RESET_DEACTIVATE, true);
> }
>
> /**
>@@ -1547,7 +1547,7 @@ int eeh_pe_reset(struct eeh_pe *pe, int option)
>
> 	switch (option) {
> 	case EEH_RESET_DEACTIVATE:
>-		ret = eeh_ops->reset(pe, option);
>+		ret = eeh_ops->reset(pe, option, true);
> 		eeh_pe_state_clear(pe, EEH_PE_CFG_BLOCKED);
> 		if (ret)
> 			break;
>@@ -1564,7 +1564,7 @@ int eeh_pe_reset(struct eeh_pe *pe, int option)
> 		eeh_ops->set_option(pe, EEH_OPT_FREEZE_PE);
>
> 		eeh_pe_state_mark(pe, EEH_PE_CFG_BLOCKED);
>-		ret = eeh_ops->reset(pe, option);
>+		ret = eeh_ops->reset(pe, option, true);
> 		break;
> 	default:
> 		pr_debug("%s: Unsupported option %d\n",
>diff --git a/arch/powerpc/platforms/powernv/eeh-ioda.c b/arch/powerpc/platforms/powernv/eeh-ioda.c
>index 2809c98..1a53cda 100644
>--- a/arch/powerpc/platforms/powernv/eeh-ioda.c
>+++ b/arch/powerpc/platforms/powernv/eeh-ioda.c
>@@ -548,7 +548,8 @@ out:
> 	return 0;
> }
>
>-static int ioda_eeh_root_reset(struct pci_controller *hose, int option)
>+static int ioda_eeh_root_reset(struct pci_controller *hose,
>+			       int option, bool delay)
> {
> 	struct pnv_phb *phb = hose->private_data;
> 	s64 rc = OPAL_SUCCESS;
>@@ -578,7 +579,7 @@ static int ioda_eeh_root_reset(struct pci_controller *hose, int option)
>
> 	/* Poll state of the PHB until the request is done */
> 	rc = ioda_eeh_phb_poll(phb);
>-	if (option == EEH_RESET_DEACTIVATE)
>+	if (delay && option == EEH_RESET_DEACTIVATE)
> 		msleep(EEH_PE_RST_SETTLE_TIME);
> out:
> 	if (rc != OPAL_SUCCESS)
>@@ -587,7 +588,7 @@ out:
> 	return 0;
> }
>
>-static int ioda_eeh_bridge_reset(struct pci_dev *dev, int option)
>+static int ioda_eeh_bridge_reset(struct pci_dev *dev, int option, bool delay)
>
> {
> 	struct device_node *dn = pci_device_to_OF_node(dev);
>@@ -614,14 +615,18 @@ static int ioda_eeh_bridge_reset(struct pci_dev *dev, int option)
> 		eeh_ops->read_config(dn, PCI_BRIDGE_CONTROL, 2, &ctrl);
> 		ctrl |= PCI_BRIDGE_CTL_BUS_RESET;
> 		eeh_ops->write_config(dn, PCI_BRIDGE_CONTROL, 2, ctrl);
>-		msleep(EEH_PE_RST_HOLD_TIME);
>+
>+		if (delay)
>+			msleep(EEH_PE_RST_HOLD_TIME);
>
> 		break;
> 	case EEH_RESET_DEACTIVATE:
> 		eeh_ops->read_config(dn, PCI_BRIDGE_CONTROL, 2, &ctrl);
> 		ctrl &= ~PCI_BRIDGE_CTL_BUS_RESET;
> 		eeh_ops->write_config(dn, PCI_BRIDGE_CONTROL, 2, ctrl);
>-		msleep(EEH_PE_RST_SETTLE_TIME);
>+
>+		if (delay)
>+			msleep(EEH_PE_RST_SETTLE_TIME);
>
> 		/* Continue reporting linkDown event */
> 		if (aer) {
>@@ -644,11 +649,11 @@ void pnv_pci_reset_secondary_bus(struct pci_dev *dev)
>
> 	if (pci_is_root_bus(dev->bus)) {
> 		hose = pci_bus_to_host(dev->bus);
>-		ioda_eeh_root_reset(hose, EEH_RESET_HOT);
>-		ioda_eeh_root_reset(hose, EEH_RESET_DEACTIVATE);
>+		ioda_eeh_root_reset(hose, EEH_RESET_HOT, true);
>+		ioda_eeh_root_reset(hose, EEH_RESET_DEACTIVATE, true);
> 	} else {
>-		ioda_eeh_bridge_reset(dev, EEH_RESET_HOT);
>-		ioda_eeh_bridge_reset(dev, EEH_RESET_DEACTIVATE);
>+		ioda_eeh_bridge_reset(dev, EEH_RESET_HOT, true);
>+		ioda_eeh_bridge_reset(dev, EEH_RESET_DEACTIVATE, true);
> 	}
> }
>
>@@ -664,7 +669,7 @@ void pnv_pci_reset_secondary_bus(struct pci_dev *dev)
>  * through FLR. For now, we don't have OPAL APIs to do HARD
>  * reset yet, so all reset would be SOFT (HOT) reset.
>  */
>-static int ioda_eeh_reset(struct eeh_pe *pe, int option)
>+static int ioda_eeh_reset(struct eeh_pe *pe, int option, bool delay)
> {
> 	struct pci_controller *hose = pe->phb;
> 	struct pci_bus *bus;
>@@ -715,9 +720,9 @@ static int ioda_eeh_reset(struct eeh_pe *pe, int option)
> 		bus = eeh_pe_bus_get(pe);
> 		if (pci_is_root_bus(bus) ||
> 		    pci_is_root_bus(bus->parent))
>-			ret = ioda_eeh_root_reset(hose, option);
>+			ret = ioda_eeh_root_reset(hose, option, delay);
> 		else
>-			ret = ioda_eeh_bridge_reset(bus->self, option);
>+			ret = ioda_eeh_bridge_reset(bus->self, option, delay);
> 	}
>
> 	return ret;
>diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c b/arch/powerpc/platforms/powernv/eeh-powernv.c
>index e261869..c89a7c4 100644
>--- a/arch/powerpc/platforms/powernv/eeh-powernv.c
>+++ b/arch/powerpc/platforms/powernv/eeh-powernv.c
>@@ -298,14 +298,14 @@ static int powernv_eeh_get_state(struct eeh_pe *pe, int *delay)
>  *
>  * Reset the specified PE
>  */
>-static int powernv_eeh_reset(struct eeh_pe *pe, int option)
>+static int powernv_eeh_reset(struct eeh_pe *pe, int option, bool delay)
> {
> 	struct pci_controller *hose = pe->phb;
> 	struct pnv_phb *phb = hose->private_data;
> 	int ret = -EEXIST;
>
> 	if (phb->eeh_ops && phb->eeh_ops->reset)
>-		ret = phb->eeh_ops->reset(pe, option);
>+		ret = phb->eeh_ops->reset(pe, option, delay);
>
> 	return ret;
> }
>diff --git a/arch/powerpc/platforms/powernv/pci.h b/arch/powerpc/platforms/powernv/pci.h
>index 6c02ff8..a998f2e 100644
>--- a/arch/powerpc/platforms/powernv/pci.h
>+++ b/arch/powerpc/platforms/powernv/pci.h
>@@ -81,7 +81,7 @@ struct pnv_eeh_ops {
> 	int (*post_init)(struct pci_controller *hose);
> 	int (*set_option)(struct eeh_pe *pe, int option);
> 	int (*get_state)(struct eeh_pe *pe);
>-	int (*reset)(struct eeh_pe *pe, int option);
>+	int (*reset)(struct eeh_pe *pe, int option, bool delay);
> 	int (*get_log)(struct eeh_pe *pe, int severity,
> 		       char *drv_log, unsigned long len);
> 	int (*configure_bridge)(struct eeh_pe *pe);
>diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/powerpc/platforms/pseries/eeh_pseries.c
>index a6c7e19..0c8d550 100644
>--- a/arch/powerpc/platforms/pseries/eeh_pseries.c
>+++ b/arch/powerpc/platforms/pseries/eeh_pseries.c
>@@ -501,7 +501,7 @@ static int pseries_eeh_get_state(struct eeh_pe *pe, int *state)
>  *
>  * Reset the specified PE
>  */
>-static int pseries_eeh_reset(struct eeh_pe *pe, int option)
>+static int pseries_eeh_reset(struct eeh_pe *pe, int option, bool delay)
> {
> 	int config_addr;
> 	int ret;
>@@ -525,6 +525,9 @@ static int pseries_eeh_reset(struct eeh_pe *pe, int option)
> 				BUID_LO(pe->phb->buid), option);
> 	}
>
>+	if (!delay)
>+		goto out;
>+
> 	/* We need reset hold or settlement delay */
> 	if (option == EEH_RESET_FUNDAMENTAL ||
> 	    option == EEH_RESET_HOT)
>@@ -532,6 +535,7 @@ static int pseries_eeh_reset(struct eeh_pe *pe, int option)
> 	else
> 		msleep(EEH_PE_RST_SETTLE_TIME);
>
>+out:
> 	return ret;
> }
>
>-- 
>1.8.3.2
>