On Thu, May 03, 2018 at 11:27:12PM +1000, Michael Ellerman wrote: > Sam Bobroff <[email protected]> writes: > > 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'?
> > @@ -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!
> > + 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!
signature.asc
Description: PGP signature
