Re: [PATCH V4 07/11] powerpc/powernv: Support EEH reset for VFs

On Fri, May 15, 2015 at 01:46:22PM +0800, Wei Yang wrote:

Could you change this function to something as below?

static bool pnv_eeh_wait_for_pending(struct pci_dn *pdn, int pos, u16 mask)

+{
+	int i;

	u32 status;
	int i;

You don't need the following "u32 status".

+
+	/* Wait for Transaction Pending bit clean */
+	for (i = 0; i < 4; i++) {
+		u32 status;
+		if (i)
+			msleep((1 << (i - 1)) * 100);
+
+		eeh_ops->read_config(pdn, pos, 2, &status);
+		if (!(status & mask))
+			return 1;
+	}
+
+	return 0;
+}
+
+static int pnv_eeh_do_flr(struct pci_dn *pdn, int option)
+{
+	u32 cap;
+	struct eeh_dev *edev = pdn_to_eeh_dev(pdn);
+

It's worthy to check if the device has PCIE cap though this function is
used by VFs who always have PCIE cap. However, it's still used for one
without PCIE cap.

+	eeh_ops->read_config(pdn, edev->pcie_cap + PCI_EXP_DEVCAP, 4, &cap);
+	if (!(cap & PCI_EXP_DEVCAP_FLR))
+		return -ENOTTY;
+
+	if (!pnv_pci_wait_for_pending(pdn, edev->pcie_cap + PCI_EXP_DEVSTA,
+			PCI_EXP_DEVSTA_TRPND))
+		pr_err("%04x:%02x:%02x:%01x timed out waiting for pending "
+		       "transaction; performing function level reset anyway\n",
+			edev->phb->global_number, pdn->busno,
+			PCI_SLOT(pdn->devfn), PCI_FUNC(pdn->devfn));

Please print the function name and simplify the log into following one.
Also, the connection symbol between device and function number would
be ".", not ":".

		pr_warn("%s: Pending transaction while issuing FLR to "
			"%04x:%02x:%02x.%01x",
			__func__, .....);

+
+	eeh_ops->read_config(pdn, edev->pcie_cap + PCI_EXP_DEVCTL, 4, &cap);
+	if (option == EEH_RESET_DEACTIVATE)
+		cap &= ~PCI_EXP_DEVCTL_BCR_FLR;
+	else
+		cap |= PCI_EXP_DEVCTL_BCR_FLR;
+	eeh_ops->write_config(pdn, edev->pcie_cap + PCI_EXP_DEVCTL, 4, cap);
+	msleep(100);

The hold and stablization delay has been standarized in EEH as below:

EEH_PE_RST_HOLD_TIME         - After asserting reset
EEH_PE_RST_SETTLE_TIME       - After deasserting reset

+	return 0;
+}
+
+static int pnv_eeh_do_af_flr(struct pci_dn *pdn, int option)
+{
+	u32 cap;
+	struct eeh_dev *edev = pdn_to_eeh_dev(pdn);
+
+	if (!edev->af_cap)
+		return -ENOTTY;
+
+	eeh_ops->read_config(pdn, edev->af_cap + PCI_AF_CAP, 1, &cap);
+	if (!(cap & PCI_AF_CAP_TP) || !(cap & PCI_AF_CAP_FLR))
+		return -ENOTTY;
+
+	/*
+	 * Wait for Transaction Pending bit to clear.  A word-aligned test
+	 * is used, so we use the conrol offset rather than status and shift
+	 * the test bit to match.
+	 */
+	if (!pnv_pci_wait_for_pending(pdn, edev->af_cap + PCI_AF_CTRL,
+				 PCI_AF_STATUS_TP << 8))
+		pr_err("%04x:%02x:%02x:%01x timed out waiting for pending "
+		    "transaction; performing AF function level reset anyway\n",
+			edev->phb->global_number, pdn->busno,
+			PCI_SLOT(pdn->devfn), PCI_FUNC(pdn->devfn));

Same as above.

+
+	if (option == EEH_RESET_DEACTIVATE)
+		eeh_ops->write_config(pdn, edev->af_cap + PCI_AF_CTRL, 1, 0);
+	else
+		eeh_ops->write_config(pdn, edev->af_cap + PCI_AF_CTRL, 1,
+				PCI_AF_CTRL_FLR);
+	msleep(100);

Same as above.

+	return 0;
+}
+
+static int pnv_eeh_reset_vf(struct pci_dn *pdn, int option)
+{
+	int rc;
+
+	might_sleep();

This can be removed safely. This function should be called from
"eehd" kernel thread. So we don't need the check.

+
+	rc = pnv_eeh_do_flr(pdn, option);
+	if (rc != -ENOTTY)
+		goto done;
+
+	rc = pnv_eeh_do_af_flr(pdn, option);
+	if (rc != -ENOTTY)
+		goto done;
+
+done:
+	return rc;
+}

You can avoid using unnecessary tag:

	rc = pnv_eeh_do_flr();
	if (!rc)
		return rc;

	rc = pnv_eeh_do_af_flr();
	return rc;

Thanks,
Gavin