| Message ID | 1340437325-29282-5-git-send-email-yinghai@kernel.org |
|---|---|
| State | Rejected |
| Headers | show |
On Sat, Jun 23, 2012 at 1:42 AM, Yinghai Lu <yinghai@kernel.org> wrote: > When system support hotplug bridge with children hotplug slots, we need to make sure > that parent bridge get preallocated resource so later when device is plugged into > children slot, those children devices will get resource allocated. > > We do not meet this problem, because for pcie hotplug card, when acpiphp is used, > pci_scan_bridge will set that for us when detect hotplug bit in slot cap. > > Reported-and-tested-by: Jason Baron <jbaron@redhat.com> > Signed-off-by: Yinghai Lu <yinghai@kernel.org> > Acked-by: Jason Baron <jbaron@redhat.com> > --- > drivers/pci/hotplug/acpiphp_glue.c | 27 ++++++++++++++++++++++++++- > 1 files changed, 26 insertions(+), 1 deletions(-) > > diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c > index ad6fd66..0f2b72d 100644 > --- a/drivers/pci/hotplug/acpiphp_glue.c > +++ b/drivers/pci/hotplug/acpiphp_glue.c > @@ -783,6 +783,29 @@ static void acpiphp_set_acpi_region(struct acpiphp_slot *slot) > } > } > > +static void check_hotplug_bridge(struct acpiphp_slot *slot, struct pci_dev *dev) > +{ > + struct acpiphp_func *func; > + > + if (!dev->subordinate) > + return; > + > + /* quirk, or pcie could set it already */ > + if (dev->is_hotplug_bridge) > + return; > + > + if (PCI_SLOT(dev->devfn) != slot->device) > + return; > + > + list_for_each_entry(func, &slot->funcs, sibling) { > + if (PCI_FUNC(dev->devfn) == func->function) { > + /* check if this bridge has ejectable slots */ > + if ((detect_ejectable_slots(func->handle) > 0)) > + dev->is_hotplug_bridge = 1; > + break; > + } > + } > +} > /** > * enable_device - enable, configure a slot > * @slot: slot to be enabled > @@ -817,8 +840,10 @@ static int __ref enable_device(struct acpiphp_slot *slot) > if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE || > dev->hdr_type == PCI_HEADER_TYPE_CARDBUS) { > max = pci_scan_bridge(bus, dev, max, pass); > - if (pass && dev->subordinate) > + if (pass && dev->subordinate) { > + check_hotplug_bridge(slot, dev); I don't like this patch because it increases the differences between the hotplug drivers, rather than decreasing them. For PCI Express devices, we set dev->is_hotplug_bridge in the pci_setup_device() path (in set_pcie_hotplug_bridge()). I think it would make sense to try to expand that path to also handle SHPC and ACPI hotplug as well. ACPI is harder because it's not PCI-specified, so we'd need some sort of pcibios or other optional hook. I don't have a clear picture of how this works -- if I understand correctly, the situation is that we have a bridge managed by acpiphp. That part makes sense because the bridge is on the motherboard and can have a DSDT device. Now we plug something into the slot below the bridge. I *think* this patch handles the case where this new hot-added thing is also a bridge managed by acpiphp. But where does the ACPI device for this hot-added bridge come from? It's an arbitrary device the BIOS knows nothing about, so it can't be in the DSDT. > pci_bus_size_bridges(dev->subordinate); > + } > } > } > } > -- > 1.7.7 > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 11, 2012 at 09:50:19AM -0600, Bjorn Helgaas wrote: > On Sat, Jun 23, 2012 at 1:42 AM, Yinghai Lu <yinghai@kernel.org> wrote: > > When system support hotplug bridge with children hotplug slots, we need to make sure > > that parent bridge get preallocated resource so later when device is plugged into > > children slot, those children devices will get resource allocated. > > > > We do not meet this problem, because for pcie hotplug card, when acpiphp is used, > > pci_scan_bridge will set that for us when detect hotplug bit in slot cap. > > > > Reported-and-tested-by: Jason Baron <jbaron@redhat.com> > > Signed-off-by: Yinghai Lu <yinghai@kernel.org> > > Acked-by: Jason Baron <jbaron@redhat.com> > > --- > > drivers/pci/hotplug/acpiphp_glue.c | 27 ++++++++++++++++++++++++++- > > 1 files changed, 26 insertions(+), 1 deletions(-) > > > > diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c > > index ad6fd66..0f2b72d 100644 > > --- a/drivers/pci/hotplug/acpiphp_glue.c > > +++ b/drivers/pci/hotplug/acpiphp_glue.c > > @@ -783,6 +783,29 @@ static void acpiphp_set_acpi_region(struct acpiphp_slot *slot) > > } > > } > > > > +static void check_hotplug_bridge(struct acpiphp_slot *slot, struct pci_dev *dev) > > +{ > > + struct acpiphp_func *func; > > + > > + if (!dev->subordinate) > > + return; > > + > > + /* quirk, or pcie could set it already */ > > + if (dev->is_hotplug_bridge) > > + return; > > + > > + if (PCI_SLOT(dev->devfn) != slot->device) > > + return; > > + > > + list_for_each_entry(func, &slot->funcs, sibling) { > > + if (PCI_FUNC(dev->devfn) == func->function) { > > + /* check if this bridge has ejectable slots */ > > + if ((detect_ejectable_slots(func->handle) > 0)) > > + dev->is_hotplug_bridge = 1; > > + break; > > + } > > + } > > +} > > /** > > * enable_device - enable, configure a slot > > * @slot: slot to be enabled > > @@ -817,8 +840,10 @@ static int __ref enable_device(struct acpiphp_slot *slot) > > if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE || > > dev->hdr_type == PCI_HEADER_TYPE_CARDBUS) { > > max = pci_scan_bridge(bus, dev, max, pass); > > - if (pass && dev->subordinate) > > + if (pass && dev->subordinate) { > > + check_hotplug_bridge(slot, dev); > > I don't like this patch because it increases the differences between > the hotplug drivers, rather than decreasing them. > > For PCI Express devices, we set dev->is_hotplug_bridge in the > pci_setup_device() path (in set_pcie_hotplug_bridge()). I think it > would make sense to try to expand that path to also handle SHPC and > ACPI hotplug as well. ACPI is harder because it's not PCI-specified, > so we'd need some sort of pcibios or other optional hook. > > I don't have a clear picture of how this works -- if I understand > correctly, the situation is that we have a bridge managed by acpiphp. > That part makes sense because the bridge is on the motherboard and can > have a DSDT device. Now we plug something into the slot below the > bridge. I *think* this patch handles the case where this new > hot-added thing is also a bridge managed by acpiphp. But where does > the ACPI device for this hot-added bridge come from? It's an > arbitrary device the BIOS knows nothing about, so it can't be in the > DSDT. > So this came up while I was developing pci bridge hotplug for qemu. Currently, there is a top level host bus (with ACPI device definitions), where devices can be hot-plugged. What I've done is added a second level of hotplug pci busses (again with ACPI device definitions). Thus, we can now hotplug a bridge into the top-level bus and then devices behind it. Effectively increasing the hot-plug space from n -> n^2. Before the above pci patch, the devices behind the bridge would not configure their PCI BARs properly, since there were no i/o, mem resources assigned to the bridge. However, with the above patch in place things work as expected. Using the same code base I was able to do acpi hotplug on Windows 7, which correctly configured the both the bridge window and devices behind it on hot-plug. So currently, the above usage pattern works on Windows 7, but not on Linux. Thanks, -Jason -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 11, 2012 at 11:14 AM, Jason Baron <jbaron@redhat.com> wrote: > On Wed, Jul 11, 2012 at 09:50:19AM -0600, Bjorn Helgaas wrote: >> On Sat, Jun 23, 2012 at 1:42 AM, Yinghai Lu <yinghai@kernel.org> wrote: >> > When system support hotplug bridge with children hotplug slots, we need to make sure >> > that parent bridge get preallocated resource so later when device is plugged into >> > children slot, those children devices will get resource allocated. >> > >> > We do not meet this problem, because for pcie hotplug card, when acpiphp is used, >> > pci_scan_bridge will set that for us when detect hotplug bit in slot cap. >> > >> > Reported-and-tested-by: Jason Baron <jbaron@redhat.com> >> > Signed-off-by: Yinghai Lu <yinghai@kernel.org> >> > Acked-by: Jason Baron <jbaron@redhat.com> >> > --- >> > drivers/pci/hotplug/acpiphp_glue.c | 27 ++++++++++++++++++++++++++- >> > 1 files changed, 26 insertions(+), 1 deletions(-) >> > >> > diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c >> > index ad6fd66..0f2b72d 100644 >> > --- a/drivers/pci/hotplug/acpiphp_glue.c >> > +++ b/drivers/pci/hotplug/acpiphp_glue.c >> > @@ -783,6 +783,29 @@ static void acpiphp_set_acpi_region(struct acpiphp_slot *slot) >> > } >> > } >> > >> > +static void check_hotplug_bridge(struct acpiphp_slot *slot, struct pci_dev *dev) >> > +{ >> > + struct acpiphp_func *func; >> > + >> > + if (!dev->subordinate) >> > + return; >> > + >> > + /* quirk, or pcie could set it already */ >> > + if (dev->is_hotplug_bridge) >> > + return; >> > + >> > + if (PCI_SLOT(dev->devfn) != slot->device) >> > + return; >> > + >> > + list_for_each_entry(func, &slot->funcs, sibling) { >> > + if (PCI_FUNC(dev->devfn) == func->function) { >> > + /* check if this bridge has ejectable slots */ >> > + if ((detect_ejectable_slots(func->handle) > 0)) >> > + dev->is_hotplug_bridge = 1; >> > + break; >> > + } >> > + } >> > +} >> > /** >> > * enable_device - enable, configure a slot >> > * @slot: slot to be enabled >> > @@ -817,8 +840,10 @@ static int __ref enable_device(struct acpiphp_slot *slot) >> > if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE || >> > dev->hdr_type == PCI_HEADER_TYPE_CARDBUS) { >> > max = pci_scan_bridge(bus, dev, max, pass); >> > - if (pass && dev->subordinate) >> > + if (pass && dev->subordinate) { >> > + check_hotplug_bridge(slot, dev); >> >> I don't like this patch because it increases the differences between >> the hotplug drivers, rather than decreasing them. >> >> For PCI Express devices, we set dev->is_hotplug_bridge in the >> pci_setup_device() path (in set_pcie_hotplug_bridge()). I think it >> would make sense to try to expand that path to also handle SHPC and >> ACPI hotplug as well. ACPI is harder because it's not PCI-specified, >> so we'd need some sort of pcibios or other optional hook. >> >> I don't have a clear picture of how this works -- if I understand >> correctly, the situation is that we have a bridge managed by acpiphp. >> That part makes sense because the bridge is on the motherboard and can >> have a DSDT device. Now we plug something into the slot below the >> bridge. I *think* this patch handles the case where this new >> hot-added thing is also a bridge managed by acpiphp. But where does >> the ACPI device for this hot-added bridge come from? It's an >> arbitrary device the BIOS knows nothing about, so it can't be in the >> DSDT. >> > > So this came up while I was developing pci bridge hotplug for qemu. > Currently, there is a top level host bus (with ACPI device definitions), where > devices can be hot-plugged. What I've done is added a second level > of hotplug pci busses (again with ACPI device definitions). Thus, we can > now hotplug a bridge into the top-level bus and then devices behind it. > Effectively increasing the hot-plug space from n -> n^2. > > Before the above pci patch, the devices behind the bridge would not > configure their PCI BARs properly, since there were no i/o, mem resources > assigned to the bridge. However, with the above patch in place things > work as expected. > > Using the same code base I was able to do acpi hotplug on Windows 7, > which correctly configured the both the bridge window and devices behind > it on hot-plug. So currently, the above usage pattern works on Windows > 7, but not on Linux. Thanks, Jason. Do you have "lspci -v" output and the DSDT AML handy? I'd like to look in more detail at what we're missing. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 11, 2012 at 1:42 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: > On Wed, Jul 11, 2012 at 11:14 AM, Jason Baron <jbaron@redhat.com> wrote: >> Using the same code base I was able to do acpi hotplug on Windows 7, >> which correctly configured the both the bridge window and devices behind >> it on hot-plug. So currently, the above usage pattern works on Windows >> 7, but not on Linux. > > Thanks, Jason. Do you have "lspci -v" output and the DSDT AML handy? > I'd like to look in more detail at what we're missing. Ping? -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c index ad6fd66..0f2b72d 100644 --- a/drivers/pci/hotplug/acpiphp_glue.c +++ b/drivers/pci/hotplug/acpiphp_glue.c @@ -783,6 +783,29 @@ static void acpiphp_set_acpi_region(struct acpiphp_slot *slot) } } +static void check_hotplug_bridge(struct acpiphp_slot *slot, struct pci_dev *dev) +{ + struct acpiphp_func *func; + + if (!dev->subordinate) + return; + + /* quirk, or pcie could set it already */ + if (dev->is_hotplug_bridge) + return; + + if (PCI_SLOT(dev->devfn) != slot->device) + return; + + list_for_each_entry(func, &slot->funcs, sibling) { + if (PCI_FUNC(dev->devfn) == func->function) { + /* check if this bridge has ejectable slots */ + if ((detect_ejectable_slots(func->handle) > 0)) + dev->is_hotplug_bridge = 1; + break; + } + } +} /** * enable_device - enable, configure a slot * @slot: slot to be enabled @@ -817,8 +840,10 @@ static int __ref enable_device(struct acpiphp_slot *slot) if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE || dev->hdr_type == PCI_HEADER_TYPE_CARDBUS) { max = pci_scan_bridge(bus, dev, max, pass); - if (pass && dev->subordinate) + if (pass && dev->subordinate) { + check_hotplug_bridge(slot, dev); pci_bus_size_bridges(dev->subordinate); + } } } }