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

On Fri, Feb 7, 2020 at 2:35 PM Oliver O'Halloran [off-list ref] wrote:

On Fri, Feb 7, 2020 at 1:24 PM Sam Bobroff [off-list ref] wrote:

quoted

On Mon, Feb 03, 2020 at 07:35:20PM +1100, Oliver O'Halloran wrote:

quoted

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"

quoted

   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:

quoted

+                     // 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...