Re: [PATCH 6/8] powerpc/eeh: Initialize EEH address cache earlier

[Sam Bobroff <sbobroff@linux.ibm.com>]

[-- Attachment #1: Type: text/plain, Size: 4606 bytes --]

On Thu, Apr 18, 2019 at 08:13:53PM +1000, Oliver O'Halloran wrote:
> On Wed, 2019-03-20 at 13:58 +1100, Sam Bobroff wrote:
> > The EEH address cache is currently initialized and populated by a
> > single function: eeh_addr_cache_build().  While the initial population
> > of the cache can only be done once resources are allocated,
> > initialization (just setting up a spinlock) could be done much
> > earlier.
> > 
> > So move the initialization step into a separate function and call it
> > from a core_initcall (rather than a subsys initcall).
> > 
> 
> > This will allow future work to make use of the cache during boot time
> > PCI scanning.
> 
> What's the idea there? Checking for overlaps in the BAR assignments?

The idea is just to be able to populate the cache earlier during boot
time, because that allows more code to be consolidated:

Before this set, the pcibios_add_device handlers were called during boot
but they couldn't populate the cache because it wasn't set up. Instead,
the handlers did nothing and a separate sweep was done after setting up
the cache. (This is annoying because when devices are added after boot,
the pcibios_add_device handlers are the only place where the cache can
be populated.)

> > Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com>
> > ---
> >  arch/powerpc/include/asm/eeh.h  |  3 +++
> >  arch/powerpc/kernel/eeh.c       |  2 ++
> >  arch/powerpc/kernel/eeh_cache.c | 13 +++++++++++--
> >  3 files changed, 16 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
> > index e217ccda55d0..791b9e6fcc45 100644
> > --- a/arch/powerpc/include/asm/eeh.h
> > +++ b/arch/powerpc/include/asm/eeh.h
> > @@ -295,6 +295,7 @@ 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 *);
> > @@ -362,6 +363,8 @@ static inline int eeh_check_failure(const volatile void __iomem *token)
> >  
> >  #define eeh_dev_check_failure(x) (0)
> >  
> > +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) { }
> > diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c
> > index 3dcff29cb9b3..7a406d58d2c0 100644
> > --- a/arch/powerpc/kernel/eeh.c
> > +++ b/arch/powerpc/kernel/eeh.c
> > @@ -1219,6 +1219,8 @@ static int eeh_init(void)
> >  	list_for_each_entry_safe(hose, tmp, &hose_list, list_node)
> >  		eeh_dev_phb_init_dynamic(hose);
> >  
> > +	eeh_addr_cache_init();
> > +
> >  	/* Initialize EEH event */
> >  	return eeh_event_init();
> >  }
> > diff --git a/arch/powerpc/kernel/eeh_cache.c b/arch/powerpc/kernel/eeh_cache.c
> > index 9c68f0837385..f93dd5cf6a39 100644
> > --- a/arch/powerpc/kernel/eeh_cache.c
> > +++ b/arch/powerpc/kernel/eeh_cache.c
> > @@ -267,6 +267,17 @@ void eeh_addr_cache_rmv_dev(struct pci_dev *dev)
> >  	spin_unlock_irqrestore(&pci_io_addr_cache_root.piar_lock, flags);
> >  }
> >  
> > +/**
> > + * eeh_addr_cache_init - Initialize a cache of I/O addresses
> > + *
> > + * Initialize a cache of pci i/o addresses.  This cache will be used to
> > + * find the pci device that corresponds to a given address.
> > + */
> > +void eeh_addr_cache_init(void)
> > +{
> > +	spin_lock_init(&pci_io_addr_cache_root.piar_lock);
> > +}
> 
> You could move this out of the pci_io_addr_cache structure and use
> DEFINE_SPINLOCK() too. We might even be able to get rid of the hand-
> rolled interval tree in eeh_cache.c in favour of the generic
> implementation (see mm/interval_tree.c). I'm pretty sure the generic
> one is a drop-in replacement, but I haven't had a chance to have a
> detailed look to see if there's any differences in behaviour.

Sounds good, I'll try to take a look at doing it in a future patch :-)

> > +
> >  /**
> >   * eeh_addr_cache_build - Build a cache of I/O addresses
> >   *
> > @@ -282,8 +293,6 @@ void eeh_addr_cache_build(void)
> >  	struct eeh_dev *edev;
> >  	struct pci_dev *dev = NULL;
> >  
> > -	spin_lock_init(&pci_io_addr_cache_root.piar_lock);
> > -
> >  	for_each_pci_dev(dev) {
> >  		pdn = pci_get_pdn_by_devfn(dev->bus, dev->devfn);
> >  		if (!pdn)
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]