Re: [PATCH 5/6] powerpc/eeh: Make early EEH init pseries specific

["Oliver O'Halloran" <oohall@gmail.com>]

On Fri, Feb 7, 2020 at 2:35 PM Oliver O'Halloran <oohall@gmail.com> wrote:
>
> On Fri, Feb 7, 2020 at 1:24 PM Sam Bobroff <sbobroff@linux.ibm.com> wrote:
> >
> > On Mon, Feb 03, 2020 at 07:35:20PM +1100, Oliver O'Halloran wrote:
> > > The eeh_ops->probe() function is called from two different contexts:
> > >
> > > 1. On pseries, where set set EEH_PROBE_MODE_DEVTREE, it's called in
> > "set set" -> "we set"
> > >    eeh_add_device_early() which is supposed to run before we create
> > >    a pci_dev.
> > >
> > > 2. On PowerNV, where we set EEH_PROBE_MODE_DEV, it's called in
> > >    eeh_device_add_late() which is supposed to run *after* the
> > >    pci_dev is created.
> > >
> > > The "early" probe is required because PAPR requires that we perform an RTAS
> > > call to enable EEH support on a device before we start interacting with it
> > > via config space or MMIO. This requirement doesn't exist on PowerNV and
> > > shoehorning two completely separate initialisation paths into a common
> > > interface just results in a convoluted code everywhere.
> > >
> > > Additionally the early probe requires the probe function to take an pci_dn
> > > rather than a pci_dev argument. We'd like to make pci_dn a pseries specific
> > > data structure since there's no real requirement for them on PowerNV. To
> > > help both goals move the early probe into the pseries containment zone
> > > so the platform depedence is more explicit.
> > >
> > I had a look around near your comment:
> > > +                     // XXX: uh, do we have the rescan lock held here?
> > And we definitely don't have the lock when it gets called via the module
> > init path (as rpaphp is loaded) -- I tried it and there was no deadlock.
> > I don't think we have the lock in other situations but I haven't
> > unravelled it all enough yet to tell, either.
>
> The other hotplug drivers seem to be taking the lock manually in their
> enable_slot() callback. So I guess we need to be doing it there too.
> I'll fix it in another patch since this one is a bit big.

On closer inspection I think we'll need to have a deeper look at this.
This function isn't used for operations on the hotplug slot. Instead
it's used for DLPAR operations including adding / removing whole PHBs.
There doesn't appear to be any code in the DLPAR add / remove paths
which takes the PCI rescan / remove lock so I think we'll need to have
a careful look at what's going on there. Great stuff...