[PATCH v2 6/6] powerpc/eeh: Refactor around eeh_probe_devices()
Sam Bobroff
sbobroff at linux.ibm.com
Wed Jun 19 15:53:13 AEST 2019
On Wed, Jun 05, 2019 at 03:49:15PM +1000, Oliver wrote:
> On Tue, May 7, 2019 at 2:30 PM Sam Bobroff <sbobroff at linux.ibm.com> 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 <sbobroff at linux.ibm.com>
> > ---
> > 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
> >
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/linuxppc-dev/attachments/20190619/8e855840/attachment-0001.sig>
More information about the Linuxppc-dev
mailing list