Patchwork [4/5] PCI, acpiphp: add is_hotplug_bridge detection

login
register
mail settings
Submitter Yinghai Lu
Date June 23, 2012, 7:42 a.m.
Message ID <1340437325-29282-5-git-send-email-yinghai@kernel.org>
Download mbox | patch
Permalink /patch/166734/
State Rejected
Headers show

Comments

Yinghai Lu - June 23, 2012, 7:42 a.m.
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(-)
Bjorn Helgaas - July 11, 2012, 3:50 p.m.
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
Jason Baron - July 11, 2012, 5:14 p.m.
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
Bjorn Helgaas - July 11, 2012, 7:42 p.m.
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
Bjorn Helgaas - July 16, 2012, 4:05 p.m.
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

Patch

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);
+				}
 			}
 		}
 	}