[PATCH v6 0/2] PCI: acpiphp: remove multifunction devices

[PATCH v6 0/2] PCI: acpiphp: remove multifunction devices

[Bjorn Helgaas]

Here's my proposal for fixing this problem.

I split out the pci_dev_put() for function 0 because it looks to me
like that is actually a separate issue -- it looks like we leaked
that reference even for single-function devices.

I restructured the iteration over bus->devices to make it read a bit
more naturally, and also to remove it from inside the slot->funcs
loop.  It didn't make sense to me to have a nested loop there.

The cleanup_p2p_bridge() stuff is mostly just fiddling with notify
handlers and other ACPI-specific stuff.  I don't think there's
actually a dependency there on the PCI device removal, so I think
it's safe to do that separately.

Does this make sense to you?  If so, can you test it and make sure
it actually works?

Thanks,
  Bjorn

---

Amos Kong (2):
      PCI: acpiphp: fix function 0 leak when disabling a slot
      PCI: acpiphp: remove all functions in slot, even without ACPI _EJx


 drivers/pci/hotplug/acpiphp_glue.c |   40 +++++++++++++++++++++++++++---------
 1 files changed, 30 insertions(+), 10 deletions(-)

[Qemu-devel] [PATCH v6 0/2] PCI: acpiphp: remove multifunction devices

[Bjorn Helgaas]

Here's my proposal for fixing this problem.

I split out the pci_dev_put() for function 0 because it looks to me
like that is actually a separate issue -- it looks like we leaked
that reference even for single-function devices.

I restructured the iteration over bus->devices to make it read a bit
more naturally, and also to remove it from inside the slot->funcs
loop.  It didn't make sense to me to have a nested loop there.

The cleanup_p2p_bridge() stuff is mostly just fiddling with notify
handlers and other ACPI-specific stuff.  I don't think there's
actually a dependency there on the PCI device removal, so I think
it's safe to do that separately.

Does this make sense to you?  If so, can you test it and make sure
it actually works?

Thanks,
  Bjorn

---

Amos Kong (2):
      PCI: acpiphp: fix function 0 leak when disabling a slot
      PCI: acpiphp: remove all functions in slot, even without ACPI _EJx


 drivers/pci/hotplug/acpiphp_glue.c |   40 +++++++++++++++++++++++++++---------
 1 files changed, 30 insertions(+), 10 deletions(-)

[PATCH v6 1/2] PCI: acpiphp: fix function 0 leak when disabling a slot

[Bjorn Helgaas]

From: Amos Kong <kongjianjun@gmail.com>

Previously, we acquired two references to function 0, but only released
one.

[bhelgaas: split this out from "remove all functions" fix]
Signed-off-by: Amos Kong <kongjianjun@gmail.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/hotplug/acpiphp_glue.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 62d0ae4..c8f9991 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -893,6 +893,7 @@ static int disable_device(struct acpiphp_slot *slot)
 	pdev = pci_get_slot(bus, PCI_DEVFN(slot->device, 0));
 	if (!pdev)
 		goto err_exit;
+	pci_dev_put(pdev);
 
 	list_for_each_entry(func, &slot->funcs, sibling) {
 		if (func->bridge) {

[Qemu-devel] [PATCH v6 1/2] PCI: acpiphp: fix function 0 leak when disabling a slot

[Bjorn Helgaas]

From: Amos Kong <kongjianjun@gmail.com>

Previously, we acquired two references to function 0, but only released
one.

[bhelgaas: split this out from "remove all functions" fix]
Signed-off-by: Amos Kong <kongjianjun@gmail.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/hotplug/acpiphp_glue.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 62d0ae4..c8f9991 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -893,6 +893,7 @@ static int disable_device(struct acpiphp_slot *slot)
 	pdev = pci_get_slot(bus, PCI_DEVFN(slot->device, 0));
 	if (!pdev)
 		goto err_exit;
+	pci_dev_put(pdev);
 
 	list_for_each_entry(func, &slot->funcs, sibling) {
 		if (func->bridge) {

[PATCH v6 2/2] PCI: acpiphp: remove all functions in slot, even without ACPI _EJx

[Bjorn Helgaas]

From: Amos Kong <kongjianjun@gmail.com>

When we add a device with acpiphp, we enumerate all functions in the
slot with pci_scan_slot(), regardless of whether they have associated
ACPI methods such as _EJ0.

When removing the device, we previously removed only the functions
with those ACPI methods.  This patch makes the remove symmetric with the
add: we remove all functions in the slot, whether they have associated
ACPI methods or not.

With qemu-kvm and SeaBIOS, we can build a multi-function device where
only function 0 has _EJ0 and _ADR (see bugzilla below).  Removing and
re-adding that device works correctly with Windows guests.  This patch
makes it also work in Linux guests.

[bhelgaas: restructure loop iteration, pull out of slot->funcs loop]
Reference: https://bugzilla.kernel.org/show_bug.cgi?id=43219
Signed-off-by: Amos Kong <kongjianjun@gmail.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/hotplug/acpiphp_glue.c |   39 +++++++++++++++++++++++++++---------
 1 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index c8f9991..9aff4f0 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -878,6 +878,21 @@ static void disable_bridges(struct pci_bus *bus)
 	}
 }
 
+/* return first device in slot, acquiring a reference on it */
+static struct pci_dev *dev_in_slot(struct acpiphp_slot *slot)
+{
+	struct pci_bus *bus = slot->bridge->pci_bus;
+	struct pci_dev *dev;
+
+	down_read(&pci_bus_sem);
+	list_for_each_entry(dev, &bus->devices, bus_list)
+		if (PCI_SLOT(dev->devfn) == slot->device)
+			return pci_dev_get(dev);
+	up_read(&pci_bus_sem);
+
+	return NULL;
+}
+
 /**
  * disable_device - disable a slot
  * @slot: ACPI PHP slot
@@ -902,18 +917,22 @@ static int disable_device(struct acpiphp_slot *slot)
 						(u32)1, NULL, NULL);
 			func->bridge = NULL;
 		}
+	}
 
-		pdev = pci_get_slot(slot->bridge->pci_bus,
-				    PCI_DEVFN(slot->device, func->function));
-		if (pdev) {
-			pci_stop_bus_device(pdev);
-			if (pdev->subordinate) {
-				disable_bridges(pdev->subordinate);
-				pci_disable_device(pdev);
-			}
-			__pci_remove_bus_device(pdev);
-			pci_dev_put(pdev);
+	/*
+	 * enable_device() enumerates all functions in this device via
+	 * pci_scan_slot(), whether they have associated ACPI hotplug
+	 * methods (_EJ0, etc.) or not.  Therefore, we remove all functions
+	 * here.
+	 */
+	while ((pdev = dev_in_slot(slot))) {
+		pci_stop_bus_device(pdev);
+		if (pdev->subordinate) {
+			disable_bridges(pdev->subordinate);
+			pci_disable_device(pdev);
 		}
+		__pci_remove_bus_device(pdev);
+		pci_dev_put(pdev);
 	}
 
 	list_for_each_entry(func, &slot->funcs, sibling) {

[Qemu-devel] [PATCH v6 2/2] PCI: acpiphp: remove all functions in slot, even without ACPI _EJx

[Bjorn Helgaas]

From: Amos Kong <kongjianjun@gmail.com>

When we add a device with acpiphp, we enumerate all functions in the
slot with pci_scan_slot(), regardless of whether they have associated
ACPI methods such as _EJ0.

When removing the device, we previously removed only the functions
with those ACPI methods.  This patch makes the remove symmetric with the
add: we remove all functions in the slot, whether they have associated
ACPI methods or not.

With qemu-kvm and SeaBIOS, we can build a multi-function device where
only function 0 has _EJ0 and _ADR (see bugzilla below).  Removing and
re-adding that device works correctly with Windows guests.  This patch
makes it also work in Linux guests.

[bhelgaas: restructure loop iteration, pull out of slot->funcs loop]
Reference: https://bugzilla.kernel.org/show_bug.cgi?id=43219
Signed-off-by: Amos Kong <kongjianjun@gmail.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/hotplug/acpiphp_glue.c |   39 +++++++++++++++++++++++++++---------
 1 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index c8f9991..9aff4f0 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -878,6 +878,21 @@ static void disable_bridges(struct pci_bus *bus)
 	}
 }
 
+/* return first device in slot, acquiring a reference on it */
+static struct pci_dev *dev_in_slot(struct acpiphp_slot *slot)
+{
+	struct pci_bus *bus = slot->bridge->pci_bus;
+	struct pci_dev *dev;
+
+	down_read(&pci_bus_sem);
+	list_for_each_entry(dev, &bus->devices, bus_list)
+		if (PCI_SLOT(dev->devfn) == slot->device)
+			return pci_dev_get(dev);
+	up_read(&pci_bus_sem);
+
+	return NULL;
+}
+
 /**
  * disable_device - disable a slot
  * @slot: ACPI PHP slot
@@ -902,18 +917,22 @@ static int disable_device(struct acpiphp_slot *slot)
 						(u32)1, NULL, NULL);
 			func->bridge = NULL;
 		}
+	}
 
-		pdev = pci_get_slot(slot->bridge->pci_bus,
-				    PCI_DEVFN(slot->device, func->function));
-		if (pdev) {
-			pci_stop_bus_device(pdev);
-			if (pdev->subordinate) {
-				disable_bridges(pdev->subordinate);
-				pci_disable_device(pdev);
-			}
-			__pci_remove_bus_device(pdev);
-			pci_dev_put(pdev);
+	/*
+	 * enable_device() enumerates all functions in this device via
+	 * pci_scan_slot(), whether they have associated ACPI hotplug
+	 * methods (_EJ0, etc.) or not.  Therefore, we remove all functions
+	 * here.
+	 */
+	while ((pdev = dev_in_slot(slot))) {
+		pci_stop_bus_device(pdev);
+		if (pdev->subordinate) {
+			disable_bridges(pdev->subordinate);
+			pci_disable_device(pdev);
 		}
+		__pci_remove_bus_device(pdev);
+		pci_dev_put(pdev);
 	}
 
 	list_for_each_entry(func, &slot->funcs, sibling) {

Re: [PATCH v6 2/2] PCI: acpiphp: remove all functions in slot, even without ACPI _EJx

[Yinghai Lu]

On Tue, May 22, 2012 at 9:35 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> From: Amos Kong <kongjianjun@gmail.com>
>
> When we add a device with acpiphp, we enumerate all functions in the
> slot with pci_scan_slot(), regardless of whether they have associated
> ACPI methods such as _EJ0.
>
> When removing the device, we previously removed only the functions
> with those ACPI methods.  This patch makes the remove symmetric with the
> add: we remove all functions in the slot, whether they have associated
> ACPI methods or not.
>
> With qemu-kvm and SeaBIOS, we can build a multi-function device where
> only function 0 has _EJ0 and _ADR (see bugzilla below).  Removing and
> re-adding that device works correctly with Windows guests.  This patch
> makes it also work in Linux guests.
>
> [bhelgaas: restructure loop iteration, pull out of slot->funcs loop]
> Reference: https://bugzilla.kernel.org/show_bug.cgi?id=43219
> Signed-off-by: Amos Kong <kongjianjun@gmail.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/hotplug/acpiphp_glue.c |   39 +++++++++++++++++++++++++++---------
>  1 files changed, 29 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> index c8f9991..9aff4f0 100644
> --- a/drivers/pci/hotplug/acpiphp_glue.c
> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> @@ -878,6 +878,21 @@ static void disable_bridges(struct pci_bus *bus)
>        }
>  }
>
> +/* return first device in slot, acquiring a reference on it */
> +static struct pci_dev *dev_in_slot(struct acpiphp_slot *slot)
> +{
> +       struct pci_bus *bus = slot->bridge->pci_bus;
> +       struct pci_dev *dev;
> +
> +       down_read(&pci_bus_sem);
> +       list_for_each_entry(dev, &bus->devices, bus_list)
> +               if (PCI_SLOT(dev->devfn) == slot->device)
> +                       return pci_dev_get(dev);
> +       up_read(&pci_bus_sem);

Do you need to keep that pci_bus_sem operation pair on return path?

> +
> +       return NULL;
> +}
> +
>  /**
>  * disable_device - disable a slot
>  * @slot: ACPI PHP slot
> @@ -902,18 +917,22 @@ static int disable_device(struct acpiphp_slot *slot)
>                                                (u32)1, NULL, NULL);
>                        func->bridge = NULL;
>                }
> +       }
>
> -               pdev = pci_get_slot(slot->bridge->pci_bus,
> -                                   PCI_DEVFN(slot->device, func->function));
> -               if (pdev) {
> -                       pci_stop_bus_device(pdev);
> -                       if (pdev->subordinate) {
> -                               disable_bridges(pdev->subordinate);
> -                               pci_disable_device(pdev);
> -                       }
> -                       __pci_remove_bus_device(pdev);
> -                       pci_dev_put(pdev);
> +       /*
> +        * enable_device() enumerates all functions in this device via
> +        * pci_scan_slot(), whether they have associated ACPI hotplug
> +        * methods (_EJ0, etc.) or not.  Therefore, we remove all functions
> +        * here.
> +        */
> +       while ((pdev = dev_in_slot(slot))) {
> +               pci_stop_bus_device(pdev);
> +               if (pdev->subordinate) {
> +                       disable_bridges(pdev->subordinate);
> +                       pci_disable_device(pdev);
>                }
> +               __pci_remove_bus_device(pdev);
> +               pci_dev_put(pdev);
>        }
>
>        list_for_each_entry(func, &slot->funcs, sibling) {
>
> --
> 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

Re: [Qemu-devel] [PATCH v6 2/2] PCI: acpiphp: remove all functions in slot, even without ACPI _EJx

[Yinghai Lu]

On Tue, May 22, 2012 at 9:35 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> From: Amos Kong <kongjianjun@gmail.com>
>
> When we add a device with acpiphp, we enumerate all functions in the
> slot with pci_scan_slot(), regardless of whether they have associated
> ACPI methods such as _EJ0.
>
> When removing the device, we previously removed only the functions
> with those ACPI methods.  This patch makes the remove symmetric with the
> add: we remove all functions in the slot, whether they have associated
> ACPI methods or not.
>
> With qemu-kvm and SeaBIOS, we can build a multi-function device where
> only function 0 has _EJ0 and _ADR (see bugzilla below).  Removing and
> re-adding that device works correctly with Windows guests.  This patch
> makes it also work in Linux guests.
>
> [bhelgaas: restructure loop iteration, pull out of slot->funcs loop]
> Reference: https://bugzilla.kernel.org/show_bug.cgi?id=43219
> Signed-off-by: Amos Kong <kongjianjun@gmail.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/hotplug/acpiphp_glue.c |   39 +++++++++++++++++++++++++++---------
>  1 files changed, 29 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> index c8f9991..9aff4f0 100644
> --- a/drivers/pci/hotplug/acpiphp_glue.c
> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> @@ -878,6 +878,21 @@ static void disable_bridges(struct pci_bus *bus)
>        }
>  }
>
> +/* return first device in slot, acquiring a reference on it */
> +static struct pci_dev *dev_in_slot(struct acpiphp_slot *slot)
> +{
> +       struct pci_bus *bus = slot->bridge->pci_bus;
> +       struct pci_dev *dev;
> +
> +       down_read(&pci_bus_sem);
> +       list_for_each_entry(dev, &bus->devices, bus_list)
> +               if (PCI_SLOT(dev->devfn) == slot->device)
> +                       return pci_dev_get(dev);
> +       up_read(&pci_bus_sem);

Do you need to keep that pci_bus_sem operation pair on return path?

> +
> +       return NULL;
> +}
> +
>  /**
>  * disable_device - disable a slot
>  * @slot: ACPI PHP slot
> @@ -902,18 +917,22 @@ static int disable_device(struct acpiphp_slot *slot)
>                                                (u32)1, NULL, NULL);
>                        func->bridge = NULL;
>                }
> +       }
>
> -               pdev = pci_get_slot(slot->bridge->pci_bus,
> -                                   PCI_DEVFN(slot->device, func->function));
> -               if (pdev) {
> -                       pci_stop_bus_device(pdev);
> -                       if (pdev->subordinate) {
> -                               disable_bridges(pdev->subordinate);
> -                               pci_disable_device(pdev);
> -                       }
> -                       __pci_remove_bus_device(pdev);
> -                       pci_dev_put(pdev);
> +       /*
> +        * enable_device() enumerates all functions in this device via
> +        * pci_scan_slot(), whether they have associated ACPI hotplug
> +        * methods (_EJ0, etc.) or not.  Therefore, we remove all functions
> +        * here.
> +        */
> +       while ((pdev = dev_in_slot(slot))) {
> +               pci_stop_bus_device(pdev);
> +               if (pdev->subordinate) {
> +                       disable_bridges(pdev->subordinate);
> +                       pci_disable_device(pdev);
>                }
> +               __pci_remove_bus_device(pdev);
> +               pci_dev_put(pdev);
>        }
>
>        list_for_each_entry(func, &slot->funcs, sibling) {
>
> --
> 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 v7 2/2] PCI: acpiphp: remove all functions in slot, even without ACPI _EJx

[Amos Kong]

[-- Attachment #1: Type: text/plain, Size: 31 bytes --]

Attached the v7,  test passed.

[-- Attachment #2: 0002-PCI-acpiphp-remove-all-functions-in-slot-even-withou.patch --]
[-- Type: text/x-patch, Size: 3040 bytes --]

From bec7ec71dbeb92dc830719be7c11f87786830863 Mon Sep 17 00:00:00 2001
From: Amos Kong <kongjianjun@gmail.com>
Date: Tue, 22 May 2012 18:35:11 +0000
Subject: [PATCH 2/2] PCI: acpiphp: remove all functions in slot, even without ACPI _EJx

When we add a device with acpiphp, we enumerate all functions in the
slot with pci_scan_slot(), regardless of whether they have associated
ACPI methods such as _EJ0.

When removing the device, we previously removed only the functions
with those ACPI methods.  This patch makes the remove symmetric with the
add: we remove all functions in the slot, whether they have associated
ACPI methods or not.

With qemu-kvm and SeaBIOS, we can build a multi-function device where
only function 0 has _EJ0 and _ADR (see bugzilla below).  Removing and
re-adding that device works correctly with Windows guests.  This patch
makes it also work in Linux guests.

[bhelgaas: restructure loop iteration, pull out of slot->funcs loop]
Reference: https://bugzilla.kernel.org/show_bug.cgi?id=43219
Signed-off-by: Amos Kong <kongjianjun@gmail.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

---
v7: keep that pci_bus_sem operation pair on return path
---
 drivers/pci/hotplug/acpiphp_glue.c |   40 +++++++++++++++++++++++++++---------
 1 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 0d87136..c94a12f 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -878,6 +878,22 @@ static void disable_bridges(struct pci_bus *bus)
 	}
 }
 
+/* return first device in slot, acquiring a reference on it */
+static struct pci_dev *dev_in_slot(struct acpiphp_slot *slot)
+{
+	struct pci_bus *bus = slot->bridge->pci_bus;
+	struct pci_dev *dev;
+	int ret = NULL;
+
+	down_read(&pci_bus_sem);
+	list_for_each_entry(dev, &bus->devices, bus_list)
+		if (PCI_SLOT(dev->devfn) == slot->device)
+			ret = pci_dev_get(dev);
+	up_read(&pci_bus_sem);
+
+	return ret;
+}
+
 /**
  * disable_device - disable a slot
  * @slot: ACPI PHP slot
@@ -902,18 +918,22 @@ static int disable_device(struct acpiphp_slot *slot)
 						(u32)1, NULL, NULL);
 			func->bridge = NULL;
 		}
+	}
 
-		pdev = pci_get_slot(slot->bridge->pci_bus,
-				    PCI_DEVFN(slot->device, func->function));
-		if (pdev) {
-			pci_stop_bus_device(pdev);
-			if (pdev->subordinate) {
-				disable_bridges(pdev->subordinate);
-				pci_disable_device(pdev);
-			}
-			__pci_remove_bus_device(pdev);
-			pci_dev_put(pdev);
+	/*
+	 * enable_device() enumerates all functions in this device via
+	 * pci_scan_slot(), whether they have associated ACPI hotplug
+	 * methods (_EJ0, etc.) or not.  Therefore, we remove all functions
+	 * here.
+	 */
+	while ((pdev = dev_in_slot(slot))) {
+		pci_stop_bus_device(pdev);
+		if (pdev->subordinate) {
+			disable_bridges(pdev->subordinate);
+			pci_disable_device(pdev);
 		}
+		__pci_remove_bus_device(pdev);
+		pci_dev_put(pdev);
 	}
 
 	list_for_each_entry(func, &slot->funcs, sibling) {
-- 
1.7.1

[Qemu-devel] [PATCH v7 2/2] PCI: acpiphp: remove all functions in slot, even without ACPI _EJx

[Amos Kong]

[-- Attachment #1: Type: text/plain, Size: 31 bytes --]

Attached the v7,  test passed.

[-- Attachment #2: 0002-PCI-acpiphp-remove-all-functions-in-slot-even-withou.patch --]
[-- Type: text/x-patch, Size: 3040 bytes --]

From bec7ec71dbeb92dc830719be7c11f87786830863 Mon Sep 17 00:00:00 2001
From: Amos Kong <kongjianjun@gmail.com>
Date: Tue, 22 May 2012 18:35:11 +0000
Subject: [PATCH 2/2] PCI: acpiphp: remove all functions in slot, even without ACPI _EJx

When we add a device with acpiphp, we enumerate all functions in the
slot with pci_scan_slot(), regardless of whether they have associated
ACPI methods such as _EJ0.

When removing the device, we previously removed only the functions
with those ACPI methods.  This patch makes the remove symmetric with the
add: we remove all functions in the slot, whether they have associated
ACPI methods or not.

With qemu-kvm and SeaBIOS, we can build a multi-function device where
only function 0 has _EJ0 and _ADR (see bugzilla below).  Removing and
re-adding that device works correctly with Windows guests.  This patch
makes it also work in Linux guests.

[bhelgaas: restructure loop iteration, pull out of slot->funcs loop]
Reference: https://bugzilla.kernel.org/show_bug.cgi?id=43219
Signed-off-by: Amos Kong <kongjianjun@gmail.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

---
v7: keep that pci_bus_sem operation pair on return path
---
 drivers/pci/hotplug/acpiphp_glue.c |   40 +++++++++++++++++++++++++++---------
 1 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 0d87136..c94a12f 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -878,6 +878,22 @@ static void disable_bridges(struct pci_bus *bus)
 	}
 }
 
+/* return first device in slot, acquiring a reference on it */
+static struct pci_dev *dev_in_slot(struct acpiphp_slot *slot)
+{
+	struct pci_bus *bus = slot->bridge->pci_bus;
+	struct pci_dev *dev;
+	int ret = NULL;
+
+	down_read(&pci_bus_sem);
+	list_for_each_entry(dev, &bus->devices, bus_list)
+		if (PCI_SLOT(dev->devfn) == slot->device)
+			ret = pci_dev_get(dev);
+	up_read(&pci_bus_sem);
+
+	return ret;
+}
+
 /**
  * disable_device - disable a slot
  * @slot: ACPI PHP slot
@@ -902,18 +918,22 @@ static int disable_device(struct acpiphp_slot *slot)
 						(u32)1, NULL, NULL);
 			func->bridge = NULL;
 		}
+	}
 
-		pdev = pci_get_slot(slot->bridge->pci_bus,
-				    PCI_DEVFN(slot->device, func->function));
-		if (pdev) {
-			pci_stop_bus_device(pdev);
-			if (pdev->subordinate) {
-				disable_bridges(pdev->subordinate);
-				pci_disable_device(pdev);
-			}
-			__pci_remove_bus_device(pdev);
-			pci_dev_put(pdev);
+	/*
+	 * enable_device() enumerates all functions in this device via
+	 * pci_scan_slot(), whether they have associated ACPI hotplug
+	 * methods (_EJ0, etc.) or not.  Therefore, we remove all functions
+	 * here.
+	 */
+	while ((pdev = dev_in_slot(slot))) {
+		pci_stop_bus_device(pdev);
+		if (pdev->subordinate) {
+			disable_bridges(pdev->subordinate);
+			pci_disable_device(pdev);
 		}
+		__pci_remove_bus_device(pdev);
+		pci_dev_put(pdev);
 	}
 
 	list_for_each_entry(func, &slot->funcs, sibling) {
-- 
1.7.1

Re: [PATCH v7 2/2] PCI: acpiphp: remove all functions in slot, even without ACPI _EJx

[Yinghai Lu]

On Tue, May 22, 2012 at 10:15 PM, Amos Kong <kongjianjun@gmail.com> wrote:
> Attached the v7,  test passed.

would be better to have break...

+static struct pci_dev *dev_in_slot(struct acpiphp_slot *slot)
+{
+	struct pci_bus *bus = slot->bridge->pci_bus;
+	struct pci_dev *dev;
+	int ret = NULL;
+
+	down_read(&pci_bus_sem);
+	list_for_each_entry(dev, &bus->devices, bus_list)
+		if (PCI_SLOT(dev->devfn) == slot->device)
+			ret = pci_dev_get(dev);
+	up_read(&pci_bus_sem);
+
+	return ret;
+}

===>


+static struct pci_dev *dev_in_slot(struct acpiphp_slot *slot)
+{
+	struct pci_bus *bus = slot->bridge->pci_bus;
+	struct pci_dev *dev;
+	int ret = NULL;
+
+	down_read(&pci_bus_sem);
+	list_for_each_entry(dev, &bus->devices, bus_list)
+		if (PCI_SLOT(dev->devfn) == slot->device) {
+			ret = pci_dev_get(dev);
+                      break;
+              }
+	up_read(&pci_bus_sem);
+
+	return ret;
+}

Re: [Qemu-devel] [PATCH v7 2/2] PCI: acpiphp: remove all functions in slot, even without ACPI _EJx

[Yinghai Lu]

On Tue, May 22, 2012 at 10:15 PM, Amos Kong <kongjianjun@gmail.com> wrote:
> Attached the v7,  test passed.

would be better to have break...

+static struct pci_dev *dev_in_slot(struct acpiphp_slot *slot)
+{
+	struct pci_bus *bus = slot->bridge->pci_bus;
+	struct pci_dev *dev;
+	int ret = NULL;
+
+	down_read(&pci_bus_sem);
+	list_for_each_entry(dev, &bus->devices, bus_list)
+		if (PCI_SLOT(dev->devfn) == slot->device)
+			ret = pci_dev_get(dev);
+	up_read(&pci_bus_sem);
+
+	return ret;
+}

===>


+static struct pci_dev *dev_in_slot(struct acpiphp_slot *slot)
+{
+	struct pci_bus *bus = slot->bridge->pci_bus;
+	struct pci_dev *dev;
+	int ret = NULL;
+
+	down_read(&pci_bus_sem);
+	list_for_each_entry(dev, &bus->devices, bus_list)
+		if (PCI_SLOT(dev->devfn) == slot->device) {
+			ret = pci_dev_get(dev);
+                      break;
+              }
+	up_read(&pci_bus_sem);
+
+	return ret;
+}

[PATCH v8 2/2] PCI: acpiphp: remove all functions in slot, even without ACPI _EJx

[Amos Kong]

[-- Attachment #1: Type: text/plain, Size: 1325 bytes --]

On Wed, May 23, 2012 at 1:29 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Tue, May 22, 2012 at 10:15 PM, Amos Kong <kongjianjun@gmail.com> wrote:
>> Attached the v7,  test passed.
>
> would be better to have break...

Yeah. Otherwise, it will delete from the last function.
Attached v8, test passed.


> +static struct pci_dev *dev_in_slot(struct acpiphp_slot *slot)
> +{
> +       struct pci_bus *bus = slot->bridge->pci_bus;
> +       struct pci_dev *dev;
> +       int ret = NULL;
> +
> +       down_read(&pci_bus_sem);
> +       list_for_each_entry(dev, &bus->devices, bus_list)
> +               if (PCI_SLOT(dev->devfn) == slot->device)
> +                       ret = pci_dev_get(dev);
> +       up_read(&pci_bus_sem);
> +
> +       return ret;
> +}
>
> ===>
>
>
> +static struct pci_dev *dev_in_slot(struct acpiphp_slot *slot)
> +{
> +       struct pci_bus *bus = slot->bridge->pci_bus;
> +       struct pci_dev *dev;
> +       int ret = NULL;
> +
> +       down_read(&pci_bus_sem);
> +       list_for_each_entry(dev, &bus->devices, bus_list)
> +               if (PCI_SLOT(dev->devfn) == slot->device) {
> +                       ret = pci_dev_get(dev);
> +                      break;
> +              }
> +       up_read(&pci_bus_sem);
> +
> +       return ret;
> +}

[-- Attachment #2: 0002-PCI-acpiphp-remove-all-functions-in-slot-even-withou.patch --]
[-- Type: text/x-patch, Size: 3123 bytes --]

From 32c42e815669837e114acbd14715eb60bc741bbe Mon Sep 17 00:00:00 2001
From: Amos Kong <kongjianjun@gmail.com>
Date: Tue, 22 May 2012 18:35:11 +0000
Subject: [PATCH V8 2/2] PCI: acpiphp: remove all functions in slot, even without ACPI _EJx

When we add a device with acpiphp, we enumerate all functions in the
slot with pci_scan_slot(), regardless of whether they have associated
ACPI methods such as _EJ0.

When removing the device, we previously removed only the functions
with those ACPI methods.  This patch makes the remove symmetric with the
add: we remove all functions in the slot, whether they have associated
ACPI methods or not.

With qemu-kvm and SeaBIOS, we can build a multi-function device where
only function 0 has _EJ0 and _ADR (see bugzilla below).  Removing and
re-adding that device works correctly with Windows guests.  This patch
makes it also work in Linux guests.

[bhelgaas: restructure loop iteration, pull out of slot->funcs loop]
Reference: https://bugzilla.kernel.org/show_bug.cgi?id=43219
Signed-off-by: Amos Kong <kongjianjun@gmail.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

---
v7: keep that pci_bus_sem operation pair on return path
v8: break when the first available device is found
---
 drivers/pci/hotplug/acpiphp_glue.c |   42 +++++++++++++++++++++++++++--------
 1 files changed, 32 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 0d87136..e37e73c 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -878,6 +878,24 @@ static void disable_bridges(struct pci_bus *bus)
 	}
 }
 
+/* return first device in slot, acquiring a reference on it */
+static struct pci_dev *dev_in_slot(struct acpiphp_slot *slot)
+{
+	struct pci_bus *bus = slot->bridge->pci_bus;
+	struct pci_dev *dev;
+	struct pci_dev *ret = NULL;
+
+	down_read(&pci_bus_sem);
+	list_for_each_entry(dev, &bus->devices, bus_list)
+		if (PCI_SLOT(dev->devfn) == slot->device) {
+			ret = pci_dev_get(dev);
+			break;
+		}
+	up_read(&pci_bus_sem);
+
+	return ret;
+}
+
 /**
  * disable_device - disable a slot
  * @slot: ACPI PHP slot
@@ -902,18 +920,22 @@ static int disable_device(struct acpiphp_slot *slot)
 						(u32)1, NULL, NULL);
 			func->bridge = NULL;
 		}
+	}
 
-		pdev = pci_get_slot(slot->bridge->pci_bus,
-				    PCI_DEVFN(slot->device, func->function));
-		if (pdev) {
-			pci_stop_bus_device(pdev);
-			if (pdev->subordinate) {
-				disable_bridges(pdev->subordinate);
-				pci_disable_device(pdev);
-			}
-			__pci_remove_bus_device(pdev);
-			pci_dev_put(pdev);
+	/*
+	 * enable_device() enumerates all functions in this device via
+	 * pci_scan_slot(), whether they have associated ACPI hotplug
+	 * methods (_EJ0, etc.) or not.  Therefore, we remove all functions
+	 * here.
+	 */
+	while ((pdev = dev_in_slot(slot))) {
+		pci_stop_bus_device(pdev);
+		if (pdev->subordinate) {
+			disable_bridges(pdev->subordinate);
+			pci_disable_device(pdev);
 		}
+		__pci_remove_bus_device(pdev);
+		pci_dev_put(pdev);
 	}
 
 	list_for_each_entry(func, &slot->funcs, sibling) {
-- 
1.7.1

[Qemu-devel] [PATCH v8 2/2] PCI: acpiphp: remove all functions in slot, even without ACPI _EJx

[Amos Kong]

[-- Attachment #1: Type: text/plain, Size: 1325 bytes --]

On Wed, May 23, 2012 at 1:29 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Tue, May 22, 2012 at 10:15 PM, Amos Kong <kongjianjun@gmail.com> wrote:
>> Attached the v7,  test passed.
>
> would be better to have break...

Yeah. Otherwise, it will delete from the last function.
Attached v8, test passed.


> +static struct pci_dev *dev_in_slot(struct acpiphp_slot *slot)
> +{
> +       struct pci_bus *bus = slot->bridge->pci_bus;
> +       struct pci_dev *dev;
> +       int ret = NULL;
> +
> +       down_read(&pci_bus_sem);
> +       list_for_each_entry(dev, &bus->devices, bus_list)
> +               if (PCI_SLOT(dev->devfn) == slot->device)
> +                       ret = pci_dev_get(dev);
> +       up_read(&pci_bus_sem);
> +
> +       return ret;
> +}
>
> ===>
>
>
> +static struct pci_dev *dev_in_slot(struct acpiphp_slot *slot)
> +{
> +       struct pci_bus *bus = slot->bridge->pci_bus;
> +       struct pci_dev *dev;
> +       int ret = NULL;
> +
> +       down_read(&pci_bus_sem);
> +       list_for_each_entry(dev, &bus->devices, bus_list)
> +               if (PCI_SLOT(dev->devfn) == slot->device) {
> +                       ret = pci_dev_get(dev);
> +                      break;
> +              }
> +       up_read(&pci_bus_sem);
> +
> +       return ret;
> +}

[-- Attachment #2: 0002-PCI-acpiphp-remove-all-functions-in-slot-even-withou.patch --]
[-- Type: text/x-patch, Size: 3123 bytes --]

From 32c42e815669837e114acbd14715eb60bc741bbe Mon Sep 17 00:00:00 2001
From: Amos Kong <kongjianjun@gmail.com>
Date: Tue, 22 May 2012 18:35:11 +0000
Subject: [PATCH V8 2/2] PCI: acpiphp: remove all functions in slot, even without ACPI _EJx

When we add a device with acpiphp, we enumerate all functions in the
slot with pci_scan_slot(), regardless of whether they have associated
ACPI methods such as _EJ0.

When removing the device, we previously removed only the functions
with those ACPI methods.  This patch makes the remove symmetric with the
add: we remove all functions in the slot, whether they have associated
ACPI methods or not.

With qemu-kvm and SeaBIOS, we can build a multi-function device where
only function 0 has _EJ0 and _ADR (see bugzilla below).  Removing and
re-adding that device works correctly with Windows guests.  This patch
makes it also work in Linux guests.

[bhelgaas: restructure loop iteration, pull out of slot->funcs loop]
Reference: https://bugzilla.kernel.org/show_bug.cgi?id=43219
Signed-off-by: Amos Kong <kongjianjun@gmail.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

---
v7: keep that pci_bus_sem operation pair on return path
v8: break when the first available device is found
---
 drivers/pci/hotplug/acpiphp_glue.c |   42 +++++++++++++++++++++++++++--------
 1 files changed, 32 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 0d87136..e37e73c 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -878,6 +878,24 @@ static void disable_bridges(struct pci_bus *bus)
 	}
 }
 
+/* return first device in slot, acquiring a reference on it */
+static struct pci_dev *dev_in_slot(struct acpiphp_slot *slot)
+{
+	struct pci_bus *bus = slot->bridge->pci_bus;
+	struct pci_dev *dev;
+	struct pci_dev *ret = NULL;
+
+	down_read(&pci_bus_sem);
+	list_for_each_entry(dev, &bus->devices, bus_list)
+		if (PCI_SLOT(dev->devfn) == slot->device) {
+			ret = pci_dev_get(dev);
+			break;
+		}
+	up_read(&pci_bus_sem);
+
+	return ret;
+}
+
 /**
  * disable_device - disable a slot
  * @slot: ACPI PHP slot
@@ -902,18 +920,22 @@ static int disable_device(struct acpiphp_slot *slot)
 						(u32)1, NULL, NULL);
 			func->bridge = NULL;
 		}
+	}
 
-		pdev = pci_get_slot(slot->bridge->pci_bus,
-				    PCI_DEVFN(slot->device, func->function));
-		if (pdev) {
-			pci_stop_bus_device(pdev);
-			if (pdev->subordinate) {
-				disable_bridges(pdev->subordinate);
-				pci_disable_device(pdev);
-			}
-			__pci_remove_bus_device(pdev);
-			pci_dev_put(pdev);
+	/*
+	 * enable_device() enumerates all functions in this device via
+	 * pci_scan_slot(), whether they have associated ACPI hotplug
+	 * methods (_EJ0, etc.) or not.  Therefore, we remove all functions
+	 * here.
+	 */
+	while ((pdev = dev_in_slot(slot))) {
+		pci_stop_bus_device(pdev);
+		if (pdev->subordinate) {
+			disable_bridges(pdev->subordinate);
+			pci_disable_device(pdev);
 		}
+		__pci_remove_bus_device(pdev);
+		pci_dev_put(pdev);
 	}
 
 	list_for_each_entry(func, &slot->funcs, sibling) {
-- 
1.7.1

Re: [PATCH v8 2/2] PCI: acpiphp: remove all functions in slot, even without ACPI _EJx

[Bjorn Helgaas]

On Wed, May 23, 2012 at 1:03 AM, Amos Kong <kongjianjun@gmail.com> wrote:
> On Wed, May 23, 2012 at 1:29 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> On Tue, May 22, 2012 at 10:15 PM, Amos Kong <kongjianjun@gmail.com> wrote:
>>> Attached the v7,  test passed.
>>
>> would be better to have break...
>
> Yeah. Otherwise, it will delete from the last function.
> Attached v8, test passed.
>
>
>> +static struct pci_dev *dev_in_slot(struct acpiphp_slot *slot)
>> +{
>> +       struct pci_bus *bus = slot->bridge->pci_bus;
>> +       struct pci_dev *dev;
>> +       int ret = NULL;
>> +
>> +       down_read(&pci_bus_sem);
>> +       list_for_each_entry(dev, &bus->devices, bus_list)
>> +               if (PCI_SLOT(dev->devfn) == slot->device)
>> +                       ret = pci_dev_get(dev);
>> +       up_read(&pci_bus_sem);
>> +
>> +       return ret;
>> +}
>>
>> ===>
>>
>>
>> +static struct pci_dev *dev_in_slot(struct acpiphp_slot *slot)
>> +{
>> +       struct pci_bus *bus = slot->bridge->pci_bus;
>> +       struct pci_dev *dev;
>> +       int ret = NULL;
>> +
>> +       down_read(&pci_bus_sem);
>> +       list_for_each_entry(dev, &bus->devices, bus_list)
>> +               if (PCI_SLOT(dev->devfn) == slot->device) {
>> +                       ret = pci_dev_get(dev);
>> +                      break;
>> +              }
>> +       up_read(&pci_bus_sem);
>> +
>> +       return ret;
>> +}

I applied the v6 1/2 and this v8 patch to my "for-3.6" branch.

Thanks for fixing this, Amos!  It's another step towards rationalizing
all the hotplug drivers we have.

Bjorn

Re: [Qemu-devel] [PATCH v8 2/2] PCI: acpiphp: remove all functions in slot, even without ACPI _EJx

[Bjorn Helgaas]

On Wed, May 23, 2012 at 1:03 AM, Amos Kong <kongjianjun@gmail.com> wrote:
> On Wed, May 23, 2012 at 1:29 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> On Tue, May 22, 2012 at 10:15 PM, Amos Kong <kongjianjun@gmail.com> wrote:
>>> Attached the v7,  test passed.
>>
>> would be better to have break...
>
> Yeah. Otherwise, it will delete from the last function.
> Attached v8, test passed.
>
>
>> +static struct pci_dev *dev_in_slot(struct acpiphp_slot *slot)
>> +{
>> +       struct pci_bus *bus = slot->bridge->pci_bus;
>> +       struct pci_dev *dev;
>> +       int ret = NULL;
>> +
>> +       down_read(&pci_bus_sem);
>> +       list_for_each_entry(dev, &bus->devices, bus_list)
>> +               if (PCI_SLOT(dev->devfn) == slot->device)
>> +                       ret = pci_dev_get(dev);
>> +       up_read(&pci_bus_sem);
>> +
>> +       return ret;
>> +}
>>
>> ===>
>>
>>
>> +static struct pci_dev *dev_in_slot(struct acpiphp_slot *slot)
>> +{
>> +       struct pci_bus *bus = slot->bridge->pci_bus;
>> +       struct pci_dev *dev;
>> +       int ret = NULL;
>> +
>> +       down_read(&pci_bus_sem);
>> +       list_for_each_entry(dev, &bus->devices, bus_list)
>> +               if (PCI_SLOT(dev->devfn) == slot->device) {
>> +                       ret = pci_dev_get(dev);
>> +                      break;
>> +              }
>> +       up_read(&pci_bus_sem);
>> +
>> +       return ret;
>> +}

I applied the v6 1/2 and this v8 patch to my "for-3.6" branch.

Thanks for fixing this, Amos!  It's another step towards rationalizing
all the hotplug drivers we have.

Bjorn