On Wed, Jun 05, 2019 at 03:49:15PM +1000, Oliver wrote: > On Tue, May 7, 2019 at 2:30 PM Sam Bobroff <[email protected]> wrote: > > > > Now that EEH support for all devices (on PowerNV and pSeries) is > > provided by the pcibios bus add device hooks, eeh_probe_devices() and > > eeh_addr_cache_build() are redundant and can be removed. > > > > Move the EEH enabled message into it's own function so that it can be > > called from multiple places. > > > > Note that previously on pSeries, useless EEH sysfs files were created > > for some devices that did not have EEH support and this change > > prevents them from being created. > > > > Signed-off-by: Sam Bobroff <[email protected]> > > --- > > v2 - As it's so small, merged the enablement message patch into this one > > (where it's used). > > - Reworked enablement messages. > > > > arch/powerpc/include/asm/eeh.h | 7 ++--- > > arch/powerpc/kernel/eeh.c | 27 ++++++----------- > > arch/powerpc/kernel/eeh_cache.c | 32 -------------------- > > arch/powerpc/platforms/powernv/eeh-powernv.c | 4 +-- > > arch/powerpc/platforms/pseries/pci.c | 3 +- > > 5 files changed, 14 insertions(+), 59 deletions(-) > > > > diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h > > index 12baf1df134c..3994d45ae0d4 100644 > > --- a/arch/powerpc/include/asm/eeh.h > > +++ b/arch/powerpc/include/asm/eeh.h > > @@ -283,13 +283,12 @@ struct pci_bus *eeh_pe_bus_get(struct eeh_pe *pe); > > > > struct eeh_dev *eeh_dev_init(struct pci_dn *pdn); > > void eeh_dev_phb_init_dynamic(struct pci_controller *phb); > > -void eeh_probe_devices(void); > > +void eeh_show_enabled(void); > > int __init eeh_ops_register(struct eeh_ops *ops); > > int __exit eeh_ops_unregister(const char *name); > > int eeh_check_failure(const volatile void __iomem *token); > > int eeh_dev_check_failure(struct eeh_dev *edev); > > void eeh_addr_cache_init(void); > > -void eeh_addr_cache_build(void); > > void eeh_add_device_early(struct pci_dn *); > > void eeh_add_device_tree_early(struct pci_dn *); > > void eeh_add_device_late(struct pci_dev *); > > @@ -333,7 +332,7 @@ static inline bool eeh_enabled(void) > > return false; > > } > > > > -static inline void eeh_probe_devices(void) { } > > +static inline void eeh_show_enabled(void) { } > > > > static inline void *eeh_dev_init(struct pci_dn *pdn, void *data) > > { > > @@ -351,8 +350,6 @@ static inline int eeh_check_failure(const volatile void > > __iomem *token) > > > > static inline void eeh_addr_cache_init(void) { } > > > > -static inline void eeh_addr_cache_build(void) { } > > - > > static inline void eeh_add_device_early(struct pci_dn *pdn) { } > > > > static inline void eeh_add_device_tree_early(struct pci_dn *pdn) { } > > diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c > > index 1ed80adb40a1..f905235f0307 100644 > > --- a/arch/powerpc/kernel/eeh.c > > +++ b/arch/powerpc/kernel/eeh.c > > @@ -163,6 +163,16 @@ static int __init eeh_setup(char *str) > > } > > __setup("eeh=", eeh_setup); > > > > +void eeh_show_enabled(void) > > +{ > > + if (eeh_has_flag(EEH_FORCE_DISABLED)) > > + pr_info("EEH: Recovery disabled by kernel parameter.\n"); > > + else if (eeh_has_flag(EEH_ENABLED)) > > + pr_info("EEH: Capable adapter found: recovery enabled.\n"); > > + else > > + pr_info("EEH: No capable adapters found: recovery > > disabled.\n"); > > +} > > + > > /* > > * This routine captures assorted PCI configuration space data > > * for the indicated PCI device, and puts them into a buffer > > @@ -1156,23 +1166,6 @@ static struct notifier_block eeh_reboot_nb = { > > .notifier_call = eeh_reboot_notifier, > > }; > > > > > -void eeh_probe_devices(void) > > -{ > > - struct pci_controller *hose, *tmp; > > - struct pci_dn *pdn; > > - > > - /* Enable EEH for all adapters */ > > - list_for_each_entry_safe(hose, tmp, &hose_list, list_node) { > > - pdn = hose->pci_data; > > - traverse_pci_dn(pdn, eeh_ops->probe, NULL); > > - } > > - if (eeh_enabled()) > > - pr_info("EEH: PCI Enhanced I/O Error Handling Enabled\n"); > > - else > > - pr_info("EEH: No capable adapters found\n"); > > - > > -} > > The one concern I have about this is that PAPR requires us to enable > EEH for the device before we do any config accesses. From PAPR: > > R1–7.3.11.1–5. For the EEH option: If a device driver is going to > enable EEH and the platform has not defaulted > to EEH enabled, then it must do so before it does any operations with > its IOA, including any configuration > cycles or Load or Store operations. > > So if we want to be strictly compatible we'd need to ensure the > set-eeh-option RTAS call happens before we read the VDID in > pci_scan_device(). The pseries eeh_probe() function does this > currently, but if we defer it until the pcibios call happens we'll > have done a pile of config accesses before then. Maybe it doesn't > matter, but we'd need to do further testing under phyp or work out > some other way to ensure it's done pre-probe.
Hmm! I had not looked at this specifically, but I don't think I've made
it any worse. The reordering is all underneath pcibios_init(), and it
changes things from this...
for each PHB: pcibios_scan_phb() and then pci_bus_add_devices().
pcibios_resource_survey() [calls
ppc_md.pcibios_fixup()->eeh_probe_devices()]
... to this:
for each PHB: pcibios_scan_phb()
pcibios_resource_survey()
for each PHB: pci_bus_add_devices()
ppc_md.pcibios_fixup()->eeh_probe_devices()
So either way, the EEH probe (and therefore setup) happens last. Moving
the probe into pseries_pcibios_bus_add_device() actually moves it a
little earlier than before.
Just for kicks, I instrumented rtas_pci_read_config() and it shows
that there are indeed several accesses before the probe function is
called. Here's the first:
pcibios_init() ->
pcibios_scan_phb() ->
__of_scan_bus() ->
of_scan_pci_dev() ->
of_create_pci_dev() ->
set_pcie_port_type() ->
pci_find_capability()
Most of that is PowerPC specific, and there's already an EEH hack in
of_scan_pci_dev(), so I tried a quick hack to probe there and it does
seem at least boot OK. And it's before any accesses! :-)
What do you think? (Although, I think I'd prefer to leave this as follow
up work.)
> > /**
> > * eeh_init - EEH initialization
> > *
> > diff --git a/arch/powerpc/kernel/eeh_cache.c
> > b/arch/powerpc/kernel/eeh_cache.c
> > index f93dd5cf6a39..c40078d036af 100644
> > --- a/arch/powerpc/kernel/eeh_cache.c
> > +++ b/arch/powerpc/kernel/eeh_cache.c
> > @@ -278,38 +278,6 @@ void eeh_addr_cache_init(void)
> > spin_lock_init(&pci_io_addr_cache_root.piar_lock);
> > }
> >
> > -/**
> > - * eeh_addr_cache_build - Build a cache of I/O addresses
> > - *
> > - * Build a cache of pci i/o addresses. This cache will be used to
> > - * find the pci device that corresponds to a given address.
> > - * This routine scans all pci busses to build the cache.
> > - * Must be run late in boot process, after the pci controllers
> > - * have been scanned for devices (after all device resources are known).
> > - */
> > -void eeh_addr_cache_build(void)
> > -{
> > - struct pci_dn *pdn;
> > - struct eeh_dev *edev;
> > - struct pci_dev *dev = NULL;
> > -
> > - for_each_pci_dev(dev) {
> > - pdn = pci_get_pdn_by_devfn(dev->bus, dev->devfn);
> > - if (!pdn)
> > - continue;
> > -
> > - edev = pdn_to_eeh_dev(pdn);
> > - if (!edev)
> > - continue;
> > -
> > - dev->dev.archdata.edev = edev;
> > - edev->pdev = dev;
> > -
> > - eeh_addr_cache_insert_dev(dev);
> > - eeh_sysfs_add_device(dev);
> > - }
> > -}
> > -
> > static int eeh_addr_cache_show(struct seq_file *s, void *v)
> > {
> > struct pci_io_addr_range *piar;
> > diff --git a/arch/powerpc/platforms/powernv/eeh-powernv.c
> > b/arch/powerpc/platforms/powernv/eeh-powernv.c
> > index 90729d908a54..22a94f4b8586 100644
> > --- a/arch/powerpc/platforms/powernv/eeh-powernv.c
> > +++ b/arch/powerpc/platforms/powernv/eeh-powernv.c
> > @@ -259,9 +259,7 @@ int pnv_eeh_post_init(void)
> > struct pnv_phb *phb;
> > int ret = 0;
> >
> > - /* Probe devices & build address cache */
> > - eeh_probe_devices();
> > - eeh_addr_cache_build();
> > + eeh_show_enabled();
> >
> > /* Register OPAL event notifier */
> > eeh_event_irq = opal_event_request(ilog2(OPAL_EVENT_PCI_ERROR));
> > diff --git a/arch/powerpc/platforms/pseries/pci.c
> > b/arch/powerpc/platforms/pseries/pci.c
> > index 37a77e57893e..d6a5f4f27507 100644
> > --- a/arch/powerpc/platforms/pseries/pci.c
> > +++ b/arch/powerpc/platforms/pseries/pci.c
> > @@ -242,8 +242,7 @@ void __init pSeries_final_fixup(void)
> >
> > pSeries_request_regions();
> >
> > - eeh_probe_devices();
> > - eeh_addr_cache_build();
> > + eeh_show_enabled();
> >
> > #ifdef CONFIG_PCI_IOV
> > ppc_md.pcibios_sriov_enable = pseries_pcibios_sriov_enable;
> > --
> > 2.19.0.2.gcad72f5712
> >
>
signature.asc
Description: PGP signature
