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

Sam Bobroff [off-list ref] writes:

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

quoted

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?

Something like (not tested):

void eeh_edev_info(const struct eeh_info *info, const char *fmt, ...)
{
	struct va_format vaf;
	va_list args;

	va_start(args, fmt);

	vaf.fmt = fmt;
	vaf.va = &args;

        printk(KERN_INFO "EEH: PE#%x (PCI %s): %pV\n",
        	info->pe_config_addr,
                info->pdev ? dev_name(&info->pdev->dev) : "none",
                &vaf);

	va_end(args);
}

quoted

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

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

Sounds good.

cheers