Re: [PATCH 13/13] powerpc/eeh: Refactor report functions

On Thu, May 03, 2018 at 11:27:12PM +1000, Michael Ellerman wrote:

Sam Bobroff [off-list ref] writes:

quoted

diff --git a/arch/powerpc/kernel/eeh_driver.c b/arch/powerpc/kernel/eeh_driver.c
index eb4feee81ff4..1c4336dcf9f5 100644
--- a/arch/powerpc/kernel/eeh_driver.c
+++ b/arch/powerpc/kernel/eeh_driver.c

@@ -54,6 +54,25 @@ static int eeh_result_priority(enum pci_ers_result result)
 	}
 };
 
+const char *pci_ers_result_name(enum pci_ers_result r)
+{
+	switch (r) {
+	case PCI_ERS_RESULT_NONE: return "none";
+	case PCI_ERS_RESULT_CAN_RECOVER: return "can recover";
+	case PCI_ERS_RESULT_NEED_RESET: return "need reset";
+	case PCI_ERS_RESULT_DISCONNECT: return "disconnect";
+	case PCI_ERS_RESULT_RECOVERED: return "recovered";
+	case PCI_ERS_RESULT_NO_AER_DRIVER: return "no AER driver";
+	default:
+		WARN_ONCE(1, "Unknown result type");
+		return "unknown";
+	}
+};
+
+#define eeh_infoline(EDEV, FMT, ...) \
+pr_info("EEH: PE#%x (PCI %s): " pr_fmt(FMT) "\n", EDEV->pe_config_addr, \
+((EDEV->pdev) ? dev_name(&EDEV->pdev->dev) : "NONE"), ##__VA_ARGS__)

Ooof, I guess.

It would be nicer as a function.

OK (I'd prefer to avoid macros too), but I'm not sure what kind of
function you mean. I initially tried to use a function, but the existing
pr_*() type macros seemed to be macros all the way down to printk, so there
didn't seem to be anywhere to hook into, and I ended up having to
open-code the varargs and allocate a buffer to print into.  Then it can
overflow etc. and it ended up seeming worse. Is there a better way I'm
missing here?

"infoline" annoys me for some reason, "eeh_info" ?

OK. How about eeh_edev_info(), to indicate that it acts on an 'edev'?

quoted

@@ -223,123 +242,118 @@ static void eeh_set_irq_state(struct eeh_pe *root, bool enable)
 				}
 }
 
+static void eeh_pe_report(const char *name, struct eeh_pe *root,
+			  enum pci_ers_result (*fn)(struct eeh_dev *,
+						    struct pci_driver *),
+			  enum pci_ers_result *result)
+{
+	struct eeh_pe *pe;
+	struct eeh_dev *edev, *tmp;
+	enum pci_ers_result new_result;
+
+	pr_info("EEH: Beginning: '%s'\n", name);
+	eeh_for_each_pe(root, pe) {
+		eeh_pe_for_each_dev(pe, edev, tmp) {

This should be in a separate function.

Oh, that's better. Will do!

quoted

+			device_lock(&edev->pdev->dev);
+			if (eeh_edev_actionable(edev)) {
+				struct pci_driver *driver;
+
+				driver = eeh_pcid_get(edev->pdev);
+
+				if (!driver)
+					eeh_infoline(edev, "no driver");
+				else if (!driver->err_handler)
+					eeh_infoline(edev,
+						     "driver not EEH aware");
+				else if (edev->mode & EEH_DEV_NO_HANDLER)
+					eeh_infoline(edev,
+						     "driver bound too late");
+				else {
+					new_result = fn(edev, driver);
+					eeh_infoline(edev,
+						"%s driver reports: '%s'",
+						driver->name,
+						pci_ers_result_name(new_result));
+					if (result)
+						*result = merge_result(*result,
+								       new_result);
+				}
+				if (driver)
+					eeh_pcid_put(edev->pdev);
+			} else {
+				eeh_infoline(edev, "not actionable (%d,%d,%d)",
+					     !!edev->pdev,
+					     !eeh_dev_removed(edev),
+					     !eeh_pe_passed(edev->pe));
+			}
+			device_unlock(&edev->pdev->dev);

^^^


cheers

Thanks!