* Paul Mackerras <paulus@xxxxxxxxx>:
> Andrew Morton writes:
>
> > On Wed, 28 May 2008 17:04:34 -0600
> > Alex Chiang <achiang@xxxxxx> wrote:
> >
> > > >
> > > > I assume these patches will destroy Alex's frequently-destroyed
> > > > pci-hotplug-introduce-pci_slot.patch. Ho hum.
> > >
> > > I took a quick scan, and yeah, they'll break my patch series, but
> > > I should be able to adapt. Ho hum indeed.
> > >
> > > More importantly though, I can't seem to get any help on the
> > > pseries front. Benjamin Herrenschmidt is too busy, and I haven't
> > > heard a single peep from the IBM fellow (Mike Mason).
> >
> > Ben, Paul: can you please help out here?
>
> I think Ben's going to take a look at it, but in the meantime, perhaps
> it would help if Alex can tell us how he thinks we can handle
> hot-plugging an entire PCI host bridge (which we can do on pSeries),
> or having slots appear and disappear (which happens when the
> hypervisor assigns slots to our partition or takes them away from
> us). AIUI, the problem was that Alex's code seemed to assume those
> things didn't/couldn't happen.
I spent some time digging through the pseries hotplug stuff, and
I think we're in better shape than the above comment indicates.
The main change I introduce, as far as pseries is concerned, is
the new pci_hp_register() interface, as it now takes a pci_bus
and slotno as additional arguments. pci_hp_deregister() does not
change. So let's just talk about the add path.
Based on my reading, everything has to go through dlpar_add_slot(),
where we choose between PHB, VIO, and/or plain old PCI slots.
In the cases of PHB and slots, both code paths look like this:
dlpar_add_slot()
dlpar_add_pci_slot()
dlpar_pci_add_bus()
rpaphp_add_slot()
rpaphp_enable_slot()
rpaphp_register_slot()
pci_hp_register()
So by the time we get to pci_hp_register(), we must have a pci_bus for
the hotplug slot we're registering. The only thing we're missing for the
new pci_hp_register() interface is a slotno.
Badari originally ran into his oops because I was blindly trying to
deref slot->dn->child. Looking in rpaphp_enable_slot(), we discover why
that was wrong:
/* if there's an adapter in the slot, go add the pci devices */
if (state == PRESENT) {
/* non-empty slot has to have child */
if (!slot->dn->child) {
So I was trying to register a non-populated slot, which did *not* have a
->child, and we got the NULL deref. So the fix for this should be pretty
easy, and the patch is below (which replaces the earlier fixup patch
that I pointed Andrew at).
Basically, for populated slots, we can get the PCI_SLOT() of the
->child, and for unpopulated slots, we just use 0 as a placeholder.
For what it's worth, the remove path for both PHBs and PCI slots looks
[mostly] like this:
dlpar_remove_slot()
dlpar_remove_pci_slot()
rpaphp_deregister_slot()
pci_hp_deregister()
This is not so different from what any other arch might do to hotplug a
slot, and so I don't expect my design to encounter any fundamental
issues with pseries. [of course, that doesn't mean the implementation is
completely bug-free :-/]
So from here, I'm thinking...
1) get Paul/Ben to sanity-check my thoughts above, and review
the patch below (which is meant to apply on top of the series
currently in -mm)
2) fold patch below into my 3/4 patch to help with future
bisectability
3) refresh entire series and send to Jesse for queue into
linux-next and aiming for 2.6.27 merge window
Thanks.
/ac
diff --git a/drivers/pci/hotplug/rpaphp_slot.c b/drivers/pci/hotplug/rpaphp_slot.c
index 5508a6f..fba34ed 100644
--- a/drivers/pci/hotplug/rpaphp_slot.c
+++ b/drivers/pci/hotplug/rpaphp_slot.c
@@ -121,6 +121,7 @@ int rpaphp_register_slot(struct slot *slot)
{
struct hotplug_slot *php_slot = slot->hotplug_slot;
int retval;
+ int slotno;
dbg("%s registering slot:path[%s] index[%x], name[%s] pdomain[%x] type[%d]\n",
__func__, slot->dn->full_name, slot->index, slot->name,
@@ -132,8 +133,11 @@ int rpaphp_register_slot(struct slot *slot)
return -EAGAIN;
}
- retval = pci_hp_register(php_slot, slot->bus,
- PCI_SLOT(PCI_DN(slot->dn->child)->devfn));
+ if (slot->dn->child)
+ slotno = PCI_SLOT(PCI_DN(slot->dn->child)->devfn);
+ else
+ slotno = 0;
+ retval = pci_hp_register(php_slot, slot->bus, slotno);
if (retval) {
err("pci_hp_register failed with error %d\n", retval);
return retval;
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
