[PATCH] pci: clean all funcs when hot-removing multifunc device

[PATCH] pci: clean all funcs when hot-removing multifunc device

[Amos Kong]

CC: kvm@vger.kernel.org

----- Original Message -----

'slot->funcs' is initialized in acpiphp_glue.c:register_slot() before
hotpluging device, and only one entry(func 0) is added to it,
no new entry will be added to the list when hotpluging devices to the slot.
When we release the whole device, there is only one entry in the list,
this causes func1~7 could not be released.
I try to add entries for all hotpluged device in enable_device(), but
it doesn't work, because 'slot->funcs' is used in many place which we only
need to process func 0. This patch just try to clean all funcs in
disable_device().

drivers/pci/hotplug/acpiphp_glue.c:
static int disable_device(struct acpiphp_slot *slot) {
        list_for_each_entry(func, &slot->funcs, sibling) {
                pdev = pci_get_slot(slot->bridge->pci_bus,
                       PCI_DEVFN(slot->device, func->function));
                ..clean code.. // those code can only be executed one time(func 0)
                pci_remove_bus_device(pdev);
---
pci_bus_add_device() is called for each func device in acpiphp_glue.c:enable_device().
pci_remove_bus_device(pdev) is only called for func 0 in acpiphp_glue.c:disable_device().

Boot up a KVM guest, hotplug a multifunc device(8 funcs), we can find it in the guest.
@ ls /dev/vd*
   vda vdb vdc vde vdf vdg vdh
@ lspci
   00:06.0 SCSI storage controller: Red Hat, Inc Virtio block device
   ...
   00:06.7 SCSI storage controller: Red Hat, Inc Virtio block device

But func 1~7 still exist in guest after hot-removing the multifunc
device through qemu monitor.
@ lspci (00:06.0 disappeared)
   00:06.1 SCSI storage controller: Red Hat, Inc Virtio block device (rev ff)
   ...
   00:06.7 SCSI storage controller: Red Hat, Inc Virtio block device (rev ff)
                                                                     ^^^^^^^^
@ ls /dev/vd*
   vdb vdc vde vdf vdg vdh
@ mkfs /dev/vdb
   INFO: task mkfs.ext2:1784 blocked for more than 120 seconds. (task hung)

Hotpluging multifunc of WinXp is fine.

Signed-off-by: Amos Kong <akong@redhat.com>
---
 drivers/pci/hotplug/acpiphp_glue.c |   27 ++++++++++++++++++---------
 1 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index a70fa89..3b86d1a 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -880,6 +880,8 @@ static int disable_device(struct acpiphp_slot *slot)
 {
         struct acpiphp_func *func;
         struct pci_dev *pdev;
+        struct pci_bus *bus = slot->bridge->pci_bus;
+        int i, num = 1;
 
         /* is this slot already disabled? */
         if (!(slot->flags & SLOT_ENABLED))
@@ -893,16 +895,23 @@ static int disable_device(struct acpiphp_slot *slot)
                         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);
+                pdev = pci_scan_single_device(bus,
+                                        PCI_DEVFN(slot->device, 0));
+                if (!pdev)
+                        goto err_exit;
+                if (pdev->multifunction == 1)
+                        num = 8;
+                for (i=0; i<num; i++) {
+                        pdev = pci_get_slot(bus, PCI_DEVFN(slot->device, i));
+                        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);
                         }
-                        pci_remove_bus_device(pdev);
-                        pci_dev_put(pdev);
                 }
         }
 
-- 
1.7.6.1

Re: [PATCH] pci: clean all funcs when hot-removing multifunc device

[Bjorn Helgaas]

On Tue, Sep 13, 2011 at 10:55 PM, Amos Kong <akong@redhat.com> wrote:
> 'slot->funcs' is initialized in acpiphp_glue.c:register_slot() before
> hotpluging device, and only one entry(func 0) is added to it,
> no new entry will be added to the list when hotpluging devices to the slot.
> When we release the whole device, there is only one entry in the list,
> this causes func1~7 could not be released.
> I try to add entries for all hotpluged device in enable_device(), but
> it doesn't work, because 'slot->funcs' is used in many place which we only
> need to process func 0. This patch just try to clean all funcs in
> disable_device().
...
> Hotpluging multifunc of WinXp is fine.

I'm going to ignore this patch for now.  Please consider these
questions, then repost it if you still want it:

I assume you mean that Linux and WinXP are both running on top of the
same SeaBIOS, and hot-remove of a multifunction device works in WinXP
and fails in Linux.  That sounds like Linux is broken, and we should
fix it.  We might want to make a SeaBIOS change for other reasons, but
it'd still be good to fix Linux in case there are other similar
BIOSes.

Why do we need pci_scan_single_device()?  The device should have been
scanned already when it was added, and I would think that should have
set pdev->multifunction.

Your patch needs spaces around the operators in the "for" loop.

In the changelog, it would be nice to have the URL of a bugzilla where
the dmesg and DSDT are attached.

Bjorn

> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  drivers/pci/hotplug/acpiphp_glue.c |   27 ++++++++++++++++++---------
>  1 files changed, 18 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> index a70fa89..3b86d1a 100644
> --- a/drivers/pci/hotplug/acpiphp_glue.c
> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> @@ -880,6 +880,8 @@ static int disable_device(struct acpiphp_slot *slot)
>  {
>        struct acpiphp_func *func;
>        struct pci_dev *pdev;
> +       struct pci_bus *bus = slot->bridge->pci_bus;
> +       int i, num = 1;
>
>        /* is this slot already disabled? */
>        if (!(slot->flags & SLOT_ENABLED))
> @@ -893,16 +895,23 @@ static int disable_device(struct acpiphp_slot *slot)
>                        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);
> +               pdev = pci_scan_single_device(bus,
> +                                       PCI_DEVFN(slot->device, 0));
> +               if (!pdev)
> +                       goto err_exit;
> +               if (pdev->multifunction == 1)
> +                       num = 8;
> +                for (i=0; i<num; i++) {
> +                       pdev = pci_get_slot(bus, PCI_DEVFN(slot->device, i));
> +                       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);
>                        }
> -                       pci_remove_bus_device(pdev);
> -                       pci_dev_put(pdev);
>                }
>        }
>
> --
> 1.7.6.1
>
> --
> 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: [PATCH] pci: clean all funcs when hot-removing multifunc device

[Kenji Kaneshige]

(2011/09/16 4:03), Bjorn Helgaas wrote:
> On Tue, Sep 13, 2011 at 10:55 PM, Amos Kong<akong@redhat.com>  wrote:
>> 'slot->funcs' is initialized in acpiphp_glue.c:register_slot() before
>> hotpluging device, and only one entry(func 0) is added to it,
>> no new entry will be added to the list when hotpluging devices to the slot.
>> When we release the whole device, there is only one entry in the list,
>> this causes func1~7 could not be released.
>> I try to add entries for all hotpluged device in enable_device(), but
>> it doesn't work, because 'slot->funcs' is used in many place which we only
>> need to process func 0. This patch just try to clean all funcs in
>> disable_device().
> ...
>> Hotpluging multifunc of WinXp is fine.
>
> I'm going to ignore this patch for now.  Please consider these
> questions, then repost it if you still want it:
>
> I assume you mean that Linux and WinXP are both running on top of the
> same SeaBIOS, and hot-remove of a multifunction device works in WinXP
> and fails in Linux.  That sounds like Linux is broken, and we should
> fix it.  We might want to make a SeaBIOS change for other reasons, but
> it'd still be good to fix Linux in case there are other similar
> BIOSes.

No objection about fixing Linux.

>
> Why do we need pci_scan_single_device()?  The device should have been
> scanned already when it was added, and I would think that should have
> set pdev->multifunction.

It should be pci_get_slot() instead. Note that it needs
corresponding pci_dev_put().

Regards,
Kenji Kaneshige



>
> Your patch needs spaces around the operators in the "for" loop.
>
> In the changelog, it would be nice to have the URL of a bugzilla where
> the dmesg and DSDT are attached.
>
> Bjorn
>
>> Signed-off-by: Amos Kong<akong@redhat.com>
>> ---
>>   drivers/pci/hotplug/acpiphp_glue.c |   27 ++++++++++++++++++---------
>>   1 files changed, 18 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
>> index a70fa89..3b86d1a 100644
>> --- a/drivers/pci/hotplug/acpiphp_glue.c
>> +++ b/drivers/pci/hotplug/acpiphp_glue.c
>> @@ -880,6 +880,8 @@ static int disable_device(struct acpiphp_slot *slot)
>>   {
>>         struct acpiphp_func *func;
>>         struct pci_dev *pdev;
>> +       struct pci_bus *bus = slot->bridge->pci_bus;
>> +       int i, num = 1;
>>
>>         /* is this slot already disabled? */
>>         if (!(slot->flags&  SLOT_ENABLED))
>> @@ -893,16 +895,23 @@ static int disable_device(struct acpiphp_slot *slot)
>>                         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);
>> +               pdev = pci_scan_single_device(bus,
>> +                                       PCI_DEVFN(slot->device, 0));
>> +               if (!pdev)
>> +                       goto err_exit;
>> +               if (pdev->multifunction == 1)
>> +                       num = 8;
>> +                for (i=0; i<num; i++) {
>> +                       pdev = pci_get_slot(bus, PCI_DEVFN(slot->device, i));
>> +                       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);
>>                         }
>> -                       pci_remove_bus_device(pdev);
>> -                       pci_dev_put(pdev);
>>                 }
>>         }
>>
>> --
>> 1.7.6.1
>>
>> --
>> 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 v2] pci: clean all funcs when hot-removing multifunc device

[Amos Kong]

RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=737720#c8
(dmesg and DSDT were attached in bz)

Boot up a Linux VM with 8 pci block devices which
are the 8 functions in one pci slot.
| # qemu-kvm ...
| -drive file=images/u0,if=none,id=drv0,format=qcow2,cache=none \
| -device virtio-blk-pci,drive=drv0,id=v0,multifunction=on,addr=0x03.0 \
| ....
| -drive file=images/u7,if=none,id=drv7,format=qcow2,cache=none \
| -device virtio-blk-pci,drive=drv7,id=v7,multifunction=on,addr=0x03.7 \

Check devices in guest.
| vm)# ls /dev/vd*
|    vda vdb vdc vde vdf vdg vdh
| vm)# lspci |grep block
| 00:03.0 SCSI storage controller: Red Hat, Inc Virtio block device
|    ...
| 00:03.7 SCSI storage controller: Red Hat, Inc Virtio block device
|

Func1~7 still exist in guest after hot-removing the whole slot
by qemu monitor cmd.
| vm)# lspci |grep block    (00:03.0 disappeared)
| 00:03.1 SCSI storage controller: Red Hat, Inc Virtio block device (rev ff)
|    ...
| 00:03.7 SCSI storage controller: Red Hat, Inc Virtio block device (rev ff)
| vm)# ls /dev/vd*          (vda disappeared)
|    vdb vdc vde vdf vdg vdh
| vm)# mkfs /dev/vdb
|    INFO: task mkfs.ext2:1784 blocked for more than 120 seconds.

We process pci slot as a whole device, in seabios only defines one
device for a slot in ACPI DSDT table, then there is only one entry
(for func#0) would be added into 'slot->funcs' list.
When we release the whole slot, only the entry in 'slot->funcs' will
be cleaned, so func#1~7 could not be cleaned from system.

| drivers/pci/hotplug/acpiphp_glue.c:
| static int disable_device(struct acpiphp_slot *slot) {
| 	list_for_each_entry(func, &slot->funcs, sibling) {
| 		pdev = pci_get_slot(slot->bridge->pci_bus,
| 		       PCI_DEVFN(slot->device, func->function));
| 		..clean code.. // those code is only executed 1 time(for func#0)
|                 __pci_remove_bus_device(pdev);
|                 pci_dev_put(pdev);

I try to add entries for all hotpluged device in enable_device(), but
it doesn't work, because 'slot->funcs' is used in many place which we
only need to process func#0. This patch just try to clean all funcs in
disable_device().

Hotpluging multifunc of guests(WinXp/Win7) is fine.

---
v1 thread: http://marc.info/?t=131597601700003&r=1&w=2

Changes from v1:
- rebase patch to latest linux.git
- remove unnecessary multiplefunction check
- rename 'i' to meaningful 'fn'
- fix coding style

Signed-off-by: Amos Kong <akong@redhat.com>
---
 drivers/pci/hotplug/acpiphp_glue.c |   20 +++++++++++---------
 1 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 806c44f..42f9bb9 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -887,6 +887,7 @@ static int disable_device(struct acpiphp_slot *slot)
 	struct acpiphp_func *func;
 	struct pci_dev *pdev;
 	struct pci_bus *bus = slot->bridge->pci_bus;
+	int fn;
 
 	/* The slot will be enabled when func 0 is added, so check
 	   func 0 before disable the slot. */
@@ -902,16 +903,17 @@ static int disable_device(struct acpiphp_slot *slot)
 			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);
+		for (fn = 0; fn < 8; fn++) {
+			pdev = pci_get_slot(bus, PCI_DEVFN(slot->device, fn));
+			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);
 			}
-			__pci_remove_bus_device(pdev);
-			pci_dev_put(pdev);
 		}
 	}
 

[Qemu-devel] [PATCH v2] pci: clean all funcs when hot-removing multifunc device

[Amos Kong]

RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=737720#c8
(dmesg and DSDT were attached in bz)

Boot up a Linux VM with 8 pci block devices which
are the 8 functions in one pci slot.
| # qemu-kvm ...
| -drive file=images/u0,if=none,id=drv0,format=qcow2,cache=none \
| -device virtio-blk-pci,drive=drv0,id=v0,multifunction=on,addr=0x03.0 \
| ....
| -drive file=images/u7,if=none,id=drv7,format=qcow2,cache=none \
| -device virtio-blk-pci,drive=drv7,id=v7,multifunction=on,addr=0x03.7 \

Check devices in guest.
| vm)# ls /dev/vd*
|    vda vdb vdc vde vdf vdg vdh
| vm)# lspci |grep block
| 00:03.0 SCSI storage controller: Red Hat, Inc Virtio block device
|    ...
| 00:03.7 SCSI storage controller: Red Hat, Inc Virtio block device
|

Func1~7 still exist in guest after hot-removing the whole slot
by qemu monitor cmd.
| vm)# lspci |grep block    (00:03.0 disappeared)
| 00:03.1 SCSI storage controller: Red Hat, Inc Virtio block device (rev ff)
|    ...
| 00:03.7 SCSI storage controller: Red Hat, Inc Virtio block device (rev ff)
| vm)# ls /dev/vd*          (vda disappeared)
|    vdb vdc vde vdf vdg vdh
| vm)# mkfs /dev/vdb
|    INFO: task mkfs.ext2:1784 blocked for more than 120 seconds.

We process pci slot as a whole device, in seabios only defines one
device for a slot in ACPI DSDT table, then there is only one entry
(for func#0) would be added into 'slot->funcs' list.
When we release the whole slot, only the entry in 'slot->funcs' will
be cleaned, so func#1~7 could not be cleaned from system.

| drivers/pci/hotplug/acpiphp_glue.c:
| static int disable_device(struct acpiphp_slot *slot) {
| 	list_for_each_entry(func, &slot->funcs, sibling) {
| 		pdev = pci_get_slot(slot->bridge->pci_bus,
| 		       PCI_DEVFN(slot->device, func->function));
| 		..clean code.. // those code is only executed 1 time(for func#0)
|                 __pci_remove_bus_device(pdev);
|                 pci_dev_put(pdev);

I try to add entries for all hotpluged device in enable_device(), but
it doesn't work, because 'slot->funcs' is used in many place which we
only need to process func#0. This patch just try to clean all funcs in
disable_device().

Hotpluging multifunc of guests(WinXp/Win7) is fine.

---
v1 thread: http://marc.info/?t=131597601700003&r=1&w=2

Changes from v1:
- rebase patch to latest linux.git
- remove unnecessary multiplefunction check
- rename 'i' to meaningful 'fn'
- fix coding style

Signed-off-by: Amos Kong <akong@redhat.com>
---
 drivers/pci/hotplug/acpiphp_glue.c |   20 +++++++++++---------
 1 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 806c44f..42f9bb9 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -887,6 +887,7 @@ static int disable_device(struct acpiphp_slot *slot)
 	struct acpiphp_func *func;
 	struct pci_dev *pdev;
 	struct pci_bus *bus = slot->bridge->pci_bus;
+	int fn;
 
 	/* The slot will be enabled when func 0 is added, so check
 	   func 0 before disable the slot. */
@@ -902,16 +903,17 @@ static int disable_device(struct acpiphp_slot *slot)
 			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);
+		for (fn = 0; fn < 8; fn++) {
+			pdev = pci_get_slot(bus, PCI_DEVFN(slot->device, fn));
+			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);
 			}
-			__pci_remove_bus_device(pdev);
-			pci_dev_put(pdev);
 		}
 	}
 

[PATCH v3] pci: clean all funcs when hot-removing multifunc device

[Amos Kong]

Hotplug CallTrace:
int acpiphp_enable_slot(struct acpiphp_slot *slot)
    \_enable_device(slot);
       \_pci_bus_add_devices(bus);
            # un-added new devs(all funcs in slot) will be added
            list_for_each_entry(dev, &bus->devices, bus_list) {
                if (dev->is_added)
                        continue;
                pci_bus_add_device(dev);
                device_add(&dev->dev);
                dev->is_added = 1;

'dev->is_added' is used to trace if pci dev is added to bus, all funcs in
same slot would be added to bus in enable_device(slot). So we need to clean
all funcs of same slot in disable_device(slot).


But hot-remove exists bug: https://bugzilla.kernel.org/show_bug.cgi?id=43219
(dmesg and DSDT were attached in bz), detail:

. Boot up a Linux VM with 8 pci block devices which are the 8
functions in one pci slot.
| # qemu-kvm ...
| -drive file=images/u0,if=none,id=drv0,format=qcow2,cache=none \
| -device virtio-blk-pci,drive=drv0,id=v0,multifunction=on,addr=0x03.0 \
| ....
| -drive file=images/u7,if=none,id=drv7,format=qcow2,cache=none \
| -device virtio-blk-pci,drive=drv7,id=v7,multifunction=on,addr=0x03.7 \

. Check devices in guest.
| vm)# ls /dev/vd*
|    vda vdb vdc vde vdf vdg vdh
| vm)# lspci |grep block
| 00:03.0 SCSI storage controller: Red Hat, Inc Virtio block device
|    ...
| 00:03.7 SCSI storage controller: Red Hat, Inc Virtio block device
|

. Func1~7 still exist in guest after hot-removing the whole slot
by qemu monitor cmd.
| vm)# lspci |grep block    (00:03.0 disappeared)
| 00:03.1 SCSI storage controller: Red Hat, Inc Virtio block device (rev ff)
|    ...
| 00:03.7 SCSI storage controller: Red Hat, Inc Virtio block device (rev ff)
| vm)# ls /dev/vd*          (vda disappeared)
|    vdb vdc vde vdf vdg vdh
| vm)# mkfs /dev/vdb
|    INFO: task mkfs.ext2:1784 blocked for more than 120 seconds.

We process pci slot as a whole device in pciphp spec, seabios only
defines one device for a slot in ACPI DSDT table.
In acpiphp_glue.c:register_slot(), only one entry is added (for func#0)
into 'slot->funcs' list. When we release the whole slot, only
the entry in 'slot->funcs' will be cleaned, so func#1~7 could
not be cleaned from system.

| drivers/pci/hotplug/acpiphp_glue.c:
| static int disable_device(struct acpiphp_slot *slot) {
| 	list_for_each_entry(func, &slot->funcs, sibling) {
| 		pdev = pci_get_slot(slot->bridge->pci_bus,
| 		       PCI_DEVFN(slot->device, func->function));
| 		..clean code.. // those code is only executed 1 time(for func#0)
|                 __pci_remove_bus_device(pdev);
|                 pci_dev_put(pdev);


Hotpluging multifunc of guests(WinXp/Win7) is fine.

---
v1 thread: http://marc.info/?t=131597601700003&r=1&w=2

Changes from v1:
- rebase patch to latest linux.git
- remove unnecessary multiplefunction check
- rename 'i' to meaningful 'fn'
- fix coding style

Changes from v2:
- update detail reason(calltrace) to commitlog
- remove hardcode 8, find funcs in pci devlist

Signed-off-by: Amos Kong <akong@redhat.com>
---
 drivers/pci/hotplug/acpiphp_glue.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 806c44f..a7442d9 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -885,7 +885,7 @@ static void disable_bridges(struct pci_bus *bus)
 static int disable_device(struct acpiphp_slot *slot)
 {
 	struct acpiphp_func *func;
-	struct pci_dev *pdev;
+	struct pci_dev *pdev, *tmp;
 	struct pci_bus *bus = slot->bridge->pci_bus;
 
 	/* The slot will be enabled when func 0 is added, so check
@@ -902,9 +902,10 @@ static int disable_device(struct acpiphp_slot *slot)
 			func->bridge = NULL;
 		}
 
-		pdev = pci_get_slot(slot->bridge->pci_bus,
-				    PCI_DEVFN(slot->device, func->function));
-		if (pdev) {
+		list_for_each_entry_safe(pdev, tmp, &bus->devices, bus_list) {
+			if (PCI_SLOT(pdev->devfn) != slot->device)
+				continue;
+
 			pci_stop_bus_device(pdev);
 			if (pdev->subordinate) {
 				disable_bridges(pdev->subordinate);

[Qemu-devel] [PATCH v3] pci: clean all funcs when hot-removing multifunc device

[Amos Kong]

Hotplug CallTrace:
int acpiphp_enable_slot(struct acpiphp_slot *slot)
    \_enable_device(slot);
       \_pci_bus_add_devices(bus);
            # un-added new devs(all funcs in slot) will be added
            list_for_each_entry(dev, &bus->devices, bus_list) {
                if (dev->is_added)
                        continue;
                pci_bus_add_device(dev);
                device_add(&dev->dev);
                dev->is_added = 1;

'dev->is_added' is used to trace if pci dev is added to bus, all funcs in
same slot would be added to bus in enable_device(slot). So we need to clean
all funcs of same slot in disable_device(slot).


But hot-remove exists bug: https://bugzilla.kernel.org/show_bug.cgi?id=43219
(dmesg and DSDT were attached in bz), detail:

. Boot up a Linux VM with 8 pci block devices which are the 8
functions in one pci slot.
| # qemu-kvm ...
| -drive file=images/u0,if=none,id=drv0,format=qcow2,cache=none \
| -device virtio-blk-pci,drive=drv0,id=v0,multifunction=on,addr=0x03.0 \
| ....
| -drive file=images/u7,if=none,id=drv7,format=qcow2,cache=none \
| -device virtio-blk-pci,drive=drv7,id=v7,multifunction=on,addr=0x03.7 \

. Check devices in guest.
| vm)# ls /dev/vd*
|    vda vdb vdc vde vdf vdg vdh
| vm)# lspci |grep block
| 00:03.0 SCSI storage controller: Red Hat, Inc Virtio block device
|    ...
| 00:03.7 SCSI storage controller: Red Hat, Inc Virtio block device
|

. Func1~7 still exist in guest after hot-removing the whole slot
by qemu monitor cmd.
| vm)# lspci |grep block    (00:03.0 disappeared)
| 00:03.1 SCSI storage controller: Red Hat, Inc Virtio block device (rev ff)
|    ...
| 00:03.7 SCSI storage controller: Red Hat, Inc Virtio block device (rev ff)
| vm)# ls /dev/vd*          (vda disappeared)
|    vdb vdc vde vdf vdg vdh
| vm)# mkfs /dev/vdb
|    INFO: task mkfs.ext2:1784 blocked for more than 120 seconds.

We process pci slot as a whole device in pciphp spec, seabios only
defines one device for a slot in ACPI DSDT table.
In acpiphp_glue.c:register_slot(), only one entry is added (for func#0)
into 'slot->funcs' list. When we release the whole slot, only
the entry in 'slot->funcs' will be cleaned, so func#1~7 could
not be cleaned from system.

| drivers/pci/hotplug/acpiphp_glue.c:
| static int disable_device(struct acpiphp_slot *slot) {
| 	list_for_each_entry(func, &slot->funcs, sibling) {
| 		pdev = pci_get_slot(slot->bridge->pci_bus,
| 		       PCI_DEVFN(slot->device, func->function));
| 		..clean code.. // those code is only executed 1 time(for func#0)
|                 __pci_remove_bus_device(pdev);
|                 pci_dev_put(pdev);


Hotpluging multifunc of guests(WinXp/Win7) is fine.

---
v1 thread: http://marc.info/?t=131597601700003&r=1&w=2

Changes from v1:
- rebase patch to latest linux.git
- remove unnecessary multiplefunction check
- rename 'i' to meaningful 'fn'
- fix coding style

Changes from v2:
- update detail reason(calltrace) to commitlog
- remove hardcode 8, find funcs in pci devlist

Signed-off-by: Amos Kong <akong@redhat.com>
---
 drivers/pci/hotplug/acpiphp_glue.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 806c44f..a7442d9 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -885,7 +885,7 @@ static void disable_bridges(struct pci_bus *bus)
 static int disable_device(struct acpiphp_slot *slot)
 {
 	struct acpiphp_func *func;
-	struct pci_dev *pdev;
+	struct pci_dev *pdev, *tmp;
 	struct pci_bus *bus = slot->bridge->pci_bus;
 
 	/* The slot will be enabled when func 0 is added, so check
@@ -902,9 +902,10 @@ static int disable_device(struct acpiphp_slot *slot)
 			func->bridge = NULL;
 		}
 
-		pdev = pci_get_slot(slot->bridge->pci_bus,
-				    PCI_DEVFN(slot->device, func->function));
-		if (pdev) {
+		list_for_each_entry_safe(pdev, tmp, &bus->devices, bus_list) {
+			if (PCI_SLOT(pdev->devfn) != slot->device)
+				continue;
+
 			pci_stop_bus_device(pdev);
 			if (pdev->subordinate) {
 				disable_bridges(pdev->subordinate);

Re: [PATCH v3] pci: clean all funcs when hot-removing multifunc device

[Jiang Liu]

On 05/10/2012 11:44 PM, Amos Kong wrote:

> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> index 806c44f..a7442d9 100644
> --- a/drivers/pci/hotplug/acpiphp_glue.c
> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> @@ -885,7 +885,7 @@ static void disable_bridges(struct pci_bus *bus)
>  static int disable_device(struct acpiphp_slot *slot)
>  {
>  	struct acpiphp_func *func;
> -	struct pci_dev *pdev;
> +	struct pci_dev *pdev, *tmp;
>  	struct pci_bus *bus = slot->bridge->pci_bus;
>  
>  	/* The slot will be enabled when func 0 is added, so check
> @@ -902,9 +902,10 @@ static int disable_device(struct acpiphp_slot *slot)
>  			func->bridge = NULL;
>  		}
>  
> -		pdev = pci_get_slot(slot->bridge->pci_bus,
> -				    PCI_DEVFN(slot->device, func->function));
> -		if (pdev) {
> +		list_for_each_entry_safe(pdev, tmp, &bus->devices, bus_list) {
> +			if (PCI_SLOT(pdev->devfn) != slot->device)
> +				continue;
> +
The pci_bus_sem lock should be acquired when walking the bus->devices list.
Otherwise it may cause invalid memory access if another thread is modifying
the bus->devices list concurrently.

BTW, what's the relationship with "[PATCH v3] hotplug: add device per func
in ACPI DSDT tables"? Seems they are both solving the same issue.

>  			pci_stop_bus_device(pdev);
>  			if (pdev->subordinate) {
>  				disable_bridges(pdev->subordinate);
> 
> --
> 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 v3] pci: clean all funcs when hot-removing multifunc device

[Jiang Liu]

On 05/10/2012 11:44 PM, Amos Kong wrote:

> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> index 806c44f..a7442d9 100644
> --- a/drivers/pci/hotplug/acpiphp_glue.c
> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> @@ -885,7 +885,7 @@ static void disable_bridges(struct pci_bus *bus)
>  static int disable_device(struct acpiphp_slot *slot)
>  {
>  	struct acpiphp_func *func;
> -	struct pci_dev *pdev;
> +	struct pci_dev *pdev, *tmp;
>  	struct pci_bus *bus = slot->bridge->pci_bus;
>  
>  	/* The slot will be enabled when func 0 is added, so check
> @@ -902,9 +902,10 @@ static int disable_device(struct acpiphp_slot *slot)
>  			func->bridge = NULL;
>  		}
>  
> -		pdev = pci_get_slot(slot->bridge->pci_bus,
> -				    PCI_DEVFN(slot->device, func->function));
> -		if (pdev) {
> +		list_for_each_entry_safe(pdev, tmp, &bus->devices, bus_list) {
> +			if (PCI_SLOT(pdev->devfn) != slot->device)
> +				continue;
> +
The pci_bus_sem lock should be acquired when walking the bus->devices list.
Otherwise it may cause invalid memory access if another thread is modifying
the bus->devices list concurrently.

BTW, what's the relationship with "[PATCH v3] hotplug: add device per func
in ACPI DSDT tables"? Seems they are both solving the same issue.

>  			pci_stop_bus_device(pdev);
>  			if (pdev->subordinate) {
>  				disable_bridges(pdev->subordinate);
> 
> --
> 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: [PATCH v3] pci: clean all funcs when hot-removing multifunc device

[Michael S. Tsirkin]

On Fri, May 11, 2012 at 01:09:13AM +0800, Jiang Liu wrote:
> On 05/10/2012 11:44 PM, Amos Kong wrote:
> 
> > diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> > index 806c44f..a7442d9 100644
> > --- a/drivers/pci/hotplug/acpiphp_glue.c
> > +++ b/drivers/pci/hotplug/acpiphp_glue.c
> > @@ -885,7 +885,7 @@ static void disable_bridges(struct pci_bus *bus)
> >  static int disable_device(struct acpiphp_slot *slot)
> >  {
> >  	struct acpiphp_func *func;
> > -	struct pci_dev *pdev;
> > +	struct pci_dev *pdev, *tmp;
> >  	struct pci_bus *bus = slot->bridge->pci_bus;
> >  
> >  	/* The slot will be enabled when func 0 is added, so check
> > @@ -902,9 +902,10 @@ static int disable_device(struct acpiphp_slot *slot)
> >  			func->bridge = NULL;
> >  		}
> >  
> > -		pdev = pci_get_slot(slot->bridge->pci_bus,
> > -				    PCI_DEVFN(slot->device, func->function));
> > -		if (pdev) {
> > +		list_for_each_entry_safe(pdev, tmp, &bus->devices, bus_list) {
> > +			if (PCI_SLOT(pdev->devfn) != slot->device)
> > +				continue;
> > +
> The pci_bus_sem lock should be acquired when walking the bus->devices list.
> Otherwise it may cause invalid memory access if another thread is modifying
> the bus->devices list concurrently.
> 
> BTW, what's the relationship with "[PATCH v3] hotplug: add device per func
> in ACPI DSDT tables"? Seems they are both solving the same issue.

That's a bios patch. It's needed if you want broken linux to work.  This
makes linux behave properly on the original bios.

> >  			pci_stop_bus_device(pdev);
> >  			if (pdev->subordinate) {
> >  				disable_bridges(pdev->subordinate);
> > 
> > --
> > 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 v3] pci: clean all funcs when hot-removing multifunc device

[Michael S. Tsirkin]

On Fri, May 11, 2012 at 01:09:13AM +0800, Jiang Liu wrote:
> On 05/10/2012 11:44 PM, Amos Kong wrote:
> 
> > diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> > index 806c44f..a7442d9 100644
> > --- a/drivers/pci/hotplug/acpiphp_glue.c
> > +++ b/drivers/pci/hotplug/acpiphp_glue.c
> > @@ -885,7 +885,7 @@ static void disable_bridges(struct pci_bus *bus)
> >  static int disable_device(struct acpiphp_slot *slot)
> >  {
> >  	struct acpiphp_func *func;
> > -	struct pci_dev *pdev;
> > +	struct pci_dev *pdev, *tmp;
> >  	struct pci_bus *bus = slot->bridge->pci_bus;
> >  
> >  	/* The slot will be enabled when func 0 is added, so check
> > @@ -902,9 +902,10 @@ static int disable_device(struct acpiphp_slot *slot)
> >  			func->bridge = NULL;
> >  		}
> >  
> > -		pdev = pci_get_slot(slot->bridge->pci_bus,
> > -				    PCI_DEVFN(slot->device, func->function));
> > -		if (pdev) {
> > +		list_for_each_entry_safe(pdev, tmp, &bus->devices, bus_list) {
> > +			if (PCI_SLOT(pdev->devfn) != slot->device)
> > +				continue;
> > +
> The pci_bus_sem lock should be acquired when walking the bus->devices list.
> Otherwise it may cause invalid memory access if another thread is modifying
> the bus->devices list concurrently.
> 
> BTW, what's the relationship with "[PATCH v3] hotplug: add device per func
> in ACPI DSDT tables"? Seems they are both solving the same issue.

That's a bios patch. It's needed if you want broken linux to work.  This
makes linux behave properly on the original bios.

> >  			pci_stop_bus_device(pdev);
> >  			if (pdev->subordinate) {
> >  				disable_bridges(pdev->subordinate);
> > 
> > --
> > 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 v3] pci: clean all funcs when hot-removing multifunc device

[Amos Kong]

On 05/11/2012 02:55 AM, Michael S. Tsirkin wrote:
> On Fri, May 11, 2012 at 01:09:13AM +0800, Jiang Liu wrote:
>> On 05/10/2012 11:44 PM, Amos Kong wrote:
>>
>>> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
>>> index 806c44f..a7442d9 100644
>>> --- a/drivers/pci/hotplug/acpiphp_glue.c
>>> +++ b/drivers/pci/hotplug/acpiphp_glue.c
>>> @@ -885,7 +885,7 @@ static void disable_bridges(struct pci_bus *bus)
>>>  static int disable_device(struct acpiphp_slot *slot)
>>>  {
>>>  	struct acpiphp_func *func;
>>> -	struct pci_dev *pdev;
>>> +	struct pci_dev *pdev, *tmp;
>>>  	struct pci_bus *bus = slot->bridge->pci_bus;
>>>  
>>>  	/* The slot will be enabled when func 0 is added, so check
>>> @@ -902,9 +902,10 @@ static int disable_device(struct acpiphp_slot *slot)
>>>  			func->bridge = NULL;
>>>  		}
>>>  
>>> -		pdev = pci_get_slot(slot->bridge->pci_bus,
>>> -				    PCI_DEVFN(slot->device, func->function));
>>> -		if (pdev) {
>>> +		list_for_each_entry_safe(pdev, tmp, &bus->devices, bus_list) {
>>> +			if (PCI_SLOT(pdev->devfn) != slot->device)
>>> +				continue;
>>> +
>> The pci_bus_sem lock should be acquired when walking the bus->devices list.
>> Otherwise it may cause invalid memory access if another thread is modifying
>> the bus->devices list concurrently.


>> BTW, what's the relationship with "[PATCH v3] hotplug: add device per func
>> in ACPI DSDT tables"? Seems they are both solving the same issue.

Two work need to be done when we disable a slot, cleaning
configuration(in OS) and power off slot.

Currently the second part(power off) works fine, all funcs disappear
from "(qemu)#info block" after hot-remove slot.
The only problem is func 1~7 are not unconfigured, so I NAKed seabios
patch, and try to fix this problem in pci driver.

(btw, winxp & win7 hotplug works currently)

/**
 * acpiphp_disable_slot - power off slot
 * @slot: ACPI PHP slot
 */
int acpiphp_disable_slot(struct acpiphp_slot *slot)
{
        mutex_lock(&slot->crit_sect);

        /* unconfigure all functions */
        retval = disable_device(slot);

        /* power off all functions */
        retval = power_off_slot(slot);
        ....
}

> That's a bios patch. It's needed if you want broken linux to work.  This
> makes linux behave properly on the original bios.
> 
>>>  			pci_stop_bus_device(pdev);
>>>  			if (pdev->subordinate) {
>>>  				disable_bridges(pdev->subordinate);



-- 
			Amos.

Re: [Qemu-devel] [PATCH v3] pci: clean all funcs when hot-removing multifunc device

[Amos Kong]

On 05/11/2012 02:55 AM, Michael S. Tsirkin wrote:
> On Fri, May 11, 2012 at 01:09:13AM +0800, Jiang Liu wrote:
>> On 05/10/2012 11:44 PM, Amos Kong wrote:
>>
>>> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
>>> index 806c44f..a7442d9 100644
>>> --- a/drivers/pci/hotplug/acpiphp_glue.c
>>> +++ b/drivers/pci/hotplug/acpiphp_glue.c
>>> @@ -885,7 +885,7 @@ static void disable_bridges(struct pci_bus *bus)
>>>  static int disable_device(struct acpiphp_slot *slot)
>>>  {
>>>  	struct acpiphp_func *func;
>>> -	struct pci_dev *pdev;
>>> +	struct pci_dev *pdev, *tmp;
>>>  	struct pci_bus *bus = slot->bridge->pci_bus;
>>>  
>>>  	/* The slot will be enabled when func 0 is added, so check
>>> @@ -902,9 +902,10 @@ static int disable_device(struct acpiphp_slot *slot)
>>>  			func->bridge = NULL;
>>>  		}
>>>  
>>> -		pdev = pci_get_slot(slot->bridge->pci_bus,
>>> -				    PCI_DEVFN(slot->device, func->function));
>>> -		if (pdev) {
>>> +		list_for_each_entry_safe(pdev, tmp, &bus->devices, bus_list) {
>>> +			if (PCI_SLOT(pdev->devfn) != slot->device)
>>> +				continue;
>>> +
>> The pci_bus_sem lock should be acquired when walking the bus->devices list.
>> Otherwise it may cause invalid memory access if another thread is modifying
>> the bus->devices list concurrently.


>> BTW, what's the relationship with "[PATCH v3] hotplug: add device per func
>> in ACPI DSDT tables"? Seems they are both solving the same issue.

Two work need to be done when we disable a slot, cleaning
configuration(in OS) and power off slot.

Currently the second part(power off) works fine, all funcs disappear
from "(qemu)#info block" after hot-remove slot.
The only problem is func 1~7 are not unconfigured, so I NAKed seabios
patch, and try to fix this problem in pci driver.

(btw, winxp & win7 hotplug works currently)

/**
 * acpiphp_disable_slot - power off slot
 * @slot: ACPI PHP slot
 */
int acpiphp_disable_slot(struct acpiphp_slot *slot)
{
        mutex_lock(&slot->crit_sect);

        /* unconfigure all functions */
        retval = disable_device(slot);

        /* power off all functions */
        retval = power_off_slot(slot);
        ....
}

> That's a bios patch. It's needed if you want broken linux to work.  This
> makes linux behave properly on the original bios.
> 
>>>  			pci_stop_bus_device(pdev);
>>>  			if (pdev->subordinate) {
>>>  				disable_bridges(pdev->subordinate);



-- 
			Amos.

Re: [Qemu-devel] [PATCH v3] pci: clean all funcs when hot-removing multifunc device

[Amos Kong]

On 05/11/2012 07:54 AM, Amos Kong wrote:
> On 05/11/2012 02:55 AM, Michael S. Tsirkin wrote:
>> On Fri, May 11, 2012 at 01:09:13AM +0800, Jiang Liu wrote:
>>> On 05/10/2012 11:44 PM, Amos Kong wrote:
>>>
>>>> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
>>>> index 806c44f..a7442d9 100644
>>>> --- a/drivers/pci/hotplug/acpiphp_glue.c
>>>> +++ b/drivers/pci/hotplug/acpiphp_glue.c
>>>> @@ -885,7 +885,7 @@ static void disable_bridges(struct pci_bus *bus)
>>>>  static int disable_device(struct acpiphp_slot *slot)
>>>>  {
>>>>  	struct acpiphp_func *func;
>>>> -	struct pci_dev *pdev;
>>>> +	struct pci_dev *pdev, *tmp;
>>>>  	struct pci_bus *bus = slot->bridge->pci_bus;
>>>>  
>>>>  	/* The slot will be enabled when func 0 is added, so check
>>>> @@ -902,9 +902,10 @@ static int disable_device(struct acpiphp_slot *slot)
>>>>  			func->bridge = NULL;
>>>>  		}
>>>>  
>>>> -		pdev = pci_get_slot(slot->bridge->pci_bus,
>>>> -				    PCI_DEVFN(slot->device, func->function));
>>>> -		if (pdev) {
>>>> +		list_for_each_entry_safe(pdev, tmp, &bus->devices, bus_list) {
>>>> +			if (PCI_SLOT(pdev->devfn) != slot->device)
>>>> +				continue;
>>>> +
>>> The pci_bus_sem lock should be acquired when walking the bus->devices list.
>>> Otherwise it may cause invalid memory access if another thread is modifying
>>> the bus->devices list concurrently.

pci_bus_sem lock is only request for writing &bus->devices list, right ?
and this protection already exists in pci_destory_dev().


static int disable_device(struct acpiphp_slot *slot)
    \_ list_for_each_entry_safe(pdev, tmp, &bus->devices, bus_list) {
        \_  __pci_remove_bus_device(pdev);
               \_ pci_destroy_dev(dev);


static void pci_destroy_dev(struct pci_dev *dev)
{
        /* Remove the device from the device lists, and prevent any further
         * list accesses from this device */
        down_write(&pci_bus_sem);
        list_del(&dev->bus_list);
        dev->bus_list.next = dev->bus_list.prev = NULL;
        up_write(&pci_bus_sem);
        ...
}


-- 
			Amos.

Re: [Qemu-devel] [PATCH v3] pci: clean all funcs when hot-removing multifunc device

[Amos Kong]

On 05/11/2012 07:54 AM, Amos Kong wrote:
> On 05/11/2012 02:55 AM, Michael S. Tsirkin wrote:
>> On Fri, May 11, 2012 at 01:09:13AM +0800, Jiang Liu wrote:
>>> On 05/10/2012 11:44 PM, Amos Kong wrote:
>>>
>>>> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
>>>> index 806c44f..a7442d9 100644
>>>> --- a/drivers/pci/hotplug/acpiphp_glue.c
>>>> +++ b/drivers/pci/hotplug/acpiphp_glue.c
>>>> @@ -885,7 +885,7 @@ static void disable_bridges(struct pci_bus *bus)
>>>>  static int disable_device(struct acpiphp_slot *slot)
>>>>  {
>>>>  	struct acpiphp_func *func;
>>>> -	struct pci_dev *pdev;
>>>> +	struct pci_dev *pdev, *tmp;
>>>>  	struct pci_bus *bus = slot->bridge->pci_bus;
>>>>  
>>>>  	/* The slot will be enabled when func 0 is added, so check
>>>> @@ -902,9 +902,10 @@ static int disable_device(struct acpiphp_slot *slot)
>>>>  			func->bridge = NULL;
>>>>  		}
>>>>  
>>>> -		pdev = pci_get_slot(slot->bridge->pci_bus,
>>>> -				    PCI_DEVFN(slot->device, func->function));
>>>> -		if (pdev) {
>>>> +		list_for_each_entry_safe(pdev, tmp, &bus->devices, bus_list) {
>>>> +			if (PCI_SLOT(pdev->devfn) != slot->device)
>>>> +				continue;
>>>> +
>>> The pci_bus_sem lock should be acquired when walking the bus->devices list.
>>> Otherwise it may cause invalid memory access if another thread is modifying
>>> the bus->devices list concurrently.

pci_bus_sem lock is only request for writing &bus->devices list, right ?
and this protection already exists in pci_destory_dev().


static int disable_device(struct acpiphp_slot *slot)
    \_ list_for_each_entry_safe(pdev, tmp, &bus->devices, bus_list) {
        \_  __pci_remove_bus_device(pdev);
               \_ pci_destroy_dev(dev);


static void pci_destroy_dev(struct pci_dev *dev)
{
        /* Remove the device from the device lists, and prevent any further
         * list accesses from this device */
        down_write(&pci_bus_sem);
        list_del(&dev->bus_list);
        dev->bus_list.next = dev->bus_list.prev = NULL;
        up_write(&pci_bus_sem);
        ...
}


-- 
			Amos.

Re: [Qemu-devel] [PATCH v3] pci: clean all funcs when hot-removing multifunc device

[Jiang Liu]

On 05/11/2012 08:24 AM, Amos Kong wrote:
> On 05/11/2012 07:54 AM, Amos Kong wrote:
>> On 05/11/2012 02:55 AM, Michael S. Tsirkin wrote:
>>> On Fri, May 11, 2012 at 01:09:13AM +0800, Jiang Liu wrote:
>>>> On 05/10/2012 11:44 PM, Amos Kong wrote:
>>>>
>>>>> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
>>>>> index 806c44f..a7442d9 100644
>>>>> --- a/drivers/pci/hotplug/acpiphp_glue.c
>>>>> +++ b/drivers/pci/hotplug/acpiphp_glue.c
>>>>> @@ -885,7 +885,7 @@ static void disable_bridges(struct pci_bus *bus)
>>>>>  static int disable_device(struct acpiphp_slot *slot)
>>>>>  {
>>>>>  	struct acpiphp_func *func;
>>>>> -	struct pci_dev *pdev;
>>>>> +	struct pci_dev *pdev, *tmp;
>>>>>  	struct pci_bus *bus = slot->bridge->pci_bus;
>>>>>  
>>>>>  	/* The slot will be enabled when func 0 is added, so check
>>>>> @@ -902,9 +902,10 @@ static int disable_device(struct acpiphp_slot *slot)
>>>>>  			func->bridge = NULL;
>>>>>  		}
>>>>>  
>>>>> -		pdev = pci_get_slot(slot->bridge->pci_bus,
>>>>> -				    PCI_DEVFN(slot->device, func->function));
>>>>> -		if (pdev) {
>>>>> +		list_for_each_entry_safe(pdev, tmp, &bus->devices, bus_list) {
>>>>> +			if (PCI_SLOT(pdev->devfn) != slot->device)
>>>>> +				continue;
>>>>> +
>>>> The pci_bus_sem lock should be acquired when walking the bus->devices list.
>>>> Otherwise it may cause invalid memory access if another thread is modifying
>>>> the bus->devices list concurrently.
> 
> pci_bus_sem lock is only request for writing &bus->devices list, right ?
> and this protection already exists in pci_destory_dev().
That's for writer. For reader to walk the pci_bus->devices list, you also need
to acquire the reader lock by down_read(&pci_bus_sem). Please refer to 
pci_get_slot() for example. This especially import for native OS because there
may be multiple PCI slots/devices on the bus.

Re: [Qemu-devel] [PATCH v3] pci: clean all funcs when hot-removing multifunc device

[Bjorn Helgaas]

On Fri, May 11, 2012 at 8:00 AM, Jiang Liu <liuj97@gmail.com> wrote:
> On 05/11/2012 08:24 AM, Amos Kong wrote:
>> On 05/11/2012 07:54 AM, Amos Kong wrote:
>>> On 05/11/2012 02:55 AM, Michael S. Tsirkin wrote:
>>>> On Fri, May 11, 2012 at 01:09:13AM +0800, Jiang Liu wrote:
>>>>> On 05/10/2012 11:44 PM, Amos Kong wrote:
>>>>>
>>>>>> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
>>>>>> index 806c44f..a7442d9 100644
>>>>>> --- a/drivers/pci/hotplug/acpiphp_glue.c
>>>>>> +++ b/drivers/pci/hotplug/acpiphp_glue.c
>>>>>> @@ -885,7 +885,7 @@ static void disable_bridges(struct pci_bus *bus)
>>>>>>  static int disable_device(struct acpiphp_slot *slot)
>>>>>>  {
>>>>>>   struct acpiphp_func *func;
>>>>>> - struct pci_dev *pdev;
>>>>>> + struct pci_dev *pdev, *tmp;
>>>>>>   struct pci_bus *bus = slot->bridge->pci_bus;
>>>>>>
>>>>>>   /* The slot will be enabled when func 0 is added, so check
>>>>>> @@ -902,9 +902,10 @@ static int disable_device(struct acpiphp_slot *slot)
>>>>>>                   func->bridge = NULL;
>>>>>>           }
>>>>>>
>>>>>> -         pdev = pci_get_slot(slot->bridge->pci_bus,
>>>>>> -                             PCI_DEVFN(slot->device, func->function));
>>>>>> -         if (pdev) {
>>>>>> +         list_for_each_entry_safe(pdev, tmp, &bus->devices, bus_list) {
>>>>>> +                 if (PCI_SLOT(pdev->devfn) != slot->device)
>>>>>> +                         continue;

I think the concept is good: in enable_device(), we use
pci_scan_slot(), which scans all possible functions in the slot.  So
in disable_device() we should do something symmetric to remove all the
functions.

>>>>>> +
>>>>> The pci_bus_sem lock should be acquired when walking the bus->devices list.
>>>>> Otherwise it may cause invalid memory access if another thread is modifying
>>>>> the bus->devices list concurrently.
>>
>> pci_bus_sem lock is only request for writing &bus->devices list, right ?
>> and this protection already exists in pci_destory_dev().
> That's for writer. For reader to walk the pci_bus->devices list, you also need
> to acquire the reader lock by down_read(&pci_bus_sem). Please refer to
> pci_get_slot() for example. This especially import for native OS because there
> may be multiple PCI slots/devices on the bus.

There is a lot of existing code that walks bus->devices without
holding pci_bus_sem, but most of it is boot-time code that is arguably
safe (though I think things like pcibios_fixup_bus() are poorly
designed and don't fit well in the hotplug-enabled world).

In this case, I do think we need to protect against updates while
we're walking bus->devices.  It's probably not trivial because
__pci_remove_bus_device() calls pci_destroy_dev(), where we do the
down_write(), so simply wrapping the whole thing with down_read() will
cause a deadlock.

Kenji-san, Yinghai, do you have any input?

Bjorn

Re: [Qemu-devel] [PATCH v3] pci: clean all funcs when hot-removing multifunc device

[Bjorn Helgaas]

On Fri, May 11, 2012 at 8:00 AM, Jiang Liu <liuj97@gmail.com> wrote:
> On 05/11/2012 08:24 AM, Amos Kong wrote:
>> On 05/11/2012 07:54 AM, Amos Kong wrote:
>>> On 05/11/2012 02:55 AM, Michael S. Tsirkin wrote:
>>>> On Fri, May 11, 2012 at 01:09:13AM +0800, Jiang Liu wrote:
>>>>> On 05/10/2012 11:44 PM, Amos Kong wrote:
>>>>>
>>>>>> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
>>>>>> index 806c44f..a7442d9 100644
>>>>>> --- a/drivers/pci/hotplug/acpiphp_glue.c
>>>>>> +++ b/drivers/pci/hotplug/acpiphp_glue.c
>>>>>> @@ -885,7 +885,7 @@ static void disable_bridges(struct pci_bus *bus)
>>>>>>  static int disable_device(struct acpiphp_slot *slot)
>>>>>>  {
>>>>>>   struct acpiphp_func *func;
>>>>>> - struct pci_dev *pdev;
>>>>>> + struct pci_dev *pdev, *tmp;
>>>>>>   struct pci_bus *bus = slot->bridge->pci_bus;
>>>>>>
>>>>>>   /* The slot will be enabled when func 0 is added, so check
>>>>>> @@ -902,9 +902,10 @@ static int disable_device(struct acpiphp_slot *slot)
>>>>>>                   func->bridge = NULL;
>>>>>>           }
>>>>>>
>>>>>> -         pdev = pci_get_slot(slot->bridge->pci_bus,
>>>>>> -                             PCI_DEVFN(slot->device, func->function));
>>>>>> -         if (pdev) {
>>>>>> +         list_for_each_entry_safe(pdev, tmp, &bus->devices, bus_list) {
>>>>>> +                 if (PCI_SLOT(pdev->devfn) != slot->device)
>>>>>> +                         continue;

I think the concept is good: in enable_device(), we use
pci_scan_slot(), which scans all possible functions in the slot.  So
in disable_device() we should do something symmetric to remove all the
functions.

>>>>>> +
>>>>> The pci_bus_sem lock should be acquired when walking the bus->devices list.
>>>>> Otherwise it may cause invalid memory access if another thread is modifying
>>>>> the bus->devices list concurrently.
>>
>> pci_bus_sem lock is only request for writing &bus->devices list, right ?
>> and this protection already exists in pci_destory_dev().
> That's for writer. For reader to walk the pci_bus->devices list, you also need
> to acquire the reader lock by down_read(&pci_bus_sem). Please refer to
> pci_get_slot() for example. This especially import for native OS because there
> may be multiple PCI slots/devices on the bus.

There is a lot of existing code that walks bus->devices without
holding pci_bus_sem, but most of it is boot-time code that is arguably
safe (though I think things like pcibios_fixup_bus() are poorly
designed and don't fit well in the hotplug-enabled world).

In this case, I do think we need to protect against updates while
we're walking bus->devices.  It's probably not trivial because
__pci_remove_bus_device() calls pci_destroy_dev(), where we do the
down_write(), so simply wrapping the whole thing with down_read() will
cause a deadlock.

Kenji-san, Yinghai, do you have any input?

Bjorn

[PATCH v4] pci: clean all funcs when hot-removing multifunc device

[kongjianjun]

From: Amos Kong <kongjianjun@gmail.com>

Hotplug CallTrace:
int acpiphp_enable_slot(struct acpiphp_slot *slot)
    \_enable_device(slot);
       \_pci_bus_add_devices(bus);
            # un-added new devs(all funcs in slot) will be added
            list_for_each_entry(dev, &bus->devices, bus_list) {
                if (dev->is_added)
                        continue;
                pci_bus_add_device(dev);
                device_add(&dev->dev);
                dev->is_added = 1;

'dev->is_added' is used to trace if pci dev is added to bus, all funcs in
same slot would be added to bus in enable_device(slot). So we need to clean
all funcs of same slot in disable_device(slot).

But hot-remove exists bug: https://bugzilla.kernel.org/show_bug.cgi?id=43219
(dmesg and DSDT were attached in bz), detail:

. Boot up a Linux VM with 8 pci block devices which are the 8
functions in one pci slot.
| # qemu-kvm ...
| -drive file=images/u0,if=none,id=drv0,format=qcow2,cache=none \
| -device virtio-blk-pci,drive=drv0,id=v0,multifunction=on,addr=0x03.0 \
| ....
| -drive file=images/u7,if=none,id=drv7,format=qcow2,cache=none \
| -device virtio-blk-pci,drive=drv7,id=v7,multifunction=on,addr=0x03.7 \

. Check devices in guest.
| vm)# ls /dev/vd*
|    vda vdb vdc vde vdf vdg vdh
| vm)# lspci |grep block
| 00:03.0 SCSI storage controller: Red Hat, Inc Virtio block device
|    ...
| 00:03.7 SCSI storage controller: Red Hat, Inc Virtio block device
|

. Func1~7 still exist in guest after hot-removing the whole slot
by qemu monitor cmd.
| vm)# lspci |grep block    (00:03.0 disappeared)
| 00:03.1 SCSI storage controller: Red Hat, Inc Virtio block device (rev ff)
|    ...
| 00:03.7 SCSI storage controller: Red Hat, Inc Virtio block device (rev ff)
| vm)# ls /dev/vd*          (vda disappeared)
|    vdb vdc vde vdf vdg vdh
| vm)# mkfs /dev/vdb
|    INFO: task mkfs.ext2:1784 blocked for more than 120 seconds.

We process pci slot as a whole device in pciphp spec, seabios only
defines one device for a slot in ACPI DSDT table.
In acpiphp_glue.c:register_slot(), only one entry is added (for func#0)
into 'slot->funcs' list. When we release the whole slot, only
the entry in 'slot->funcs' will be cleaned, so func#1~7 could
not be cleaned from system.

| drivers/pci/hotplug/acpiphp_glue.c:
| static int disable_device(struct acpiphp_slot *slot) {
| 	list_for_each_entry(func, &slot->funcs, sibling) {
| 		pdev = pci_get_slot(slot->bridge->pci_bus,
| 		       PCI_DEVFN(slot->device, func->function));
| 		..clean code.. // those code is only executed 1 time(for func#0)
|                 __pci_remove_bus_device(pdev);
|                 pci_dev_put(pdev);

Hotpluging multifunc of guests(WinXp/Win7) is fine.

---
v1 thread: http://marc.info/?t=131597601700003&r=1&w=2

Changes from v1:
- rebase patch to latest linux.git
- remove unnecessary multiplefunction check
- rename 'i' to meaningful 'fn'
- fix coding style

Changes from v2:
- update detail reason(calltrace) to commitlog
- remove hardcode 8, find funcs in pci devlist

Changes from v3:
- use pci_bus_sem lock when walking bus->devices list

Signed-off-by: Amos Kong <kongjianjun@gmail.com>
---
 drivers/pci/hotplug/acpiphp_glue.c |   18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 806c44f..7109506 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) {
@@ -902,9 +903,20 @@ static int disable_device(struct acpiphp_slot *slot)
 			func->bridge = NULL;
 		}
 
-		pdev = pci_get_slot(slot->bridge->pci_bus,
-				    PCI_DEVFN(slot->device, func->function));
-		if (pdev) {
+		while (1) {
+			/* pci_bus_sem is used to protect bus->devices list,
+			   it may cause invalid memory access if threads
+			   modify bus->devices list concurrently. */
+			down_read(&pci_bus_sem);
+			list_for_each_entry(pdev, &bus->devices, bus_list)
+				if (PCI_SLOT(pdev->devfn) == slot->device) {
+					pci_dev_get(pdev);
+					break;
+				}
+			up_read(&pci_bus_sem);
+
+			if (PCI_SLOT(pdev->devfn) != slot->device)
+				break;
 			pci_stop_bus_device(pdev);
 			if (pdev->subordinate) {
 				disable_bridges(pdev->subordinate);
-- 
1.7.9.5

[Qemu-devel] [PATCH v4] pci: clean all funcs when hot-removing multifunc device

[kongjianjun]

From: Amos Kong <kongjianjun@gmail.com>

Hotplug CallTrace:
int acpiphp_enable_slot(struct acpiphp_slot *slot)
    \_enable_device(slot);
       \_pci_bus_add_devices(bus);
            # un-added new devs(all funcs in slot) will be added
            list_for_each_entry(dev, &bus->devices, bus_list) {
                if (dev->is_added)
                        continue;
                pci_bus_add_device(dev);
                device_add(&dev->dev);
                dev->is_added = 1;

'dev->is_added' is used to trace if pci dev is added to bus, all funcs in
same slot would be added to bus in enable_device(slot). So we need to clean
all funcs of same slot in disable_device(slot).

But hot-remove exists bug: https://bugzilla.kernel.org/show_bug.cgi?id=43219
(dmesg and DSDT were attached in bz), detail:

. Boot up a Linux VM with 8 pci block devices which are the 8
functions in one pci slot.
| # qemu-kvm ...
| -drive file=images/u0,if=none,id=drv0,format=qcow2,cache=none \
| -device virtio-blk-pci,drive=drv0,id=v0,multifunction=on,addr=0x03.0 \
| ....
| -drive file=images/u7,if=none,id=drv7,format=qcow2,cache=none \
| -device virtio-blk-pci,drive=drv7,id=v7,multifunction=on,addr=0x03.7 \

. Check devices in guest.
| vm)# ls /dev/vd*
|    vda vdb vdc vde vdf vdg vdh
| vm)# lspci |grep block
| 00:03.0 SCSI storage controller: Red Hat, Inc Virtio block device
|    ...
| 00:03.7 SCSI storage controller: Red Hat, Inc Virtio block device
|

. Func1~7 still exist in guest after hot-removing the whole slot
by qemu monitor cmd.
| vm)# lspci |grep block    (00:03.0 disappeared)
| 00:03.1 SCSI storage controller: Red Hat, Inc Virtio block device (rev ff)
|    ...
| 00:03.7 SCSI storage controller: Red Hat, Inc Virtio block device (rev ff)
| vm)# ls /dev/vd*          (vda disappeared)
|    vdb vdc vde vdf vdg vdh
| vm)# mkfs /dev/vdb
|    INFO: task mkfs.ext2:1784 blocked for more than 120 seconds.

We process pci slot as a whole device in pciphp spec, seabios only
defines one device for a slot in ACPI DSDT table.
In acpiphp_glue.c:register_slot(), only one entry is added (for func#0)
into 'slot->funcs' list. When we release the whole slot, only
the entry in 'slot->funcs' will be cleaned, so func#1~7 could
not be cleaned from system.

| drivers/pci/hotplug/acpiphp_glue.c:
| static int disable_device(struct acpiphp_slot *slot) {
| 	list_for_each_entry(func, &slot->funcs, sibling) {
| 		pdev = pci_get_slot(slot->bridge->pci_bus,
| 		       PCI_DEVFN(slot->device, func->function));
| 		..clean code.. // those code is only executed 1 time(for func#0)
|                 __pci_remove_bus_device(pdev);
|                 pci_dev_put(pdev);

Hotpluging multifunc of guests(WinXp/Win7) is fine.

---
v1 thread: http://marc.info/?t=131597601700003&r=1&w=2

Changes from v1:
- rebase patch to latest linux.git
- remove unnecessary multiplefunction check
- rename 'i' to meaningful 'fn'
- fix coding style

Changes from v2:
- update detail reason(calltrace) to commitlog
- remove hardcode 8, find funcs in pci devlist

Changes from v3:
- use pci_bus_sem lock when walking bus->devices list

Signed-off-by: Amos Kong <kongjianjun@gmail.com>
---
 drivers/pci/hotplug/acpiphp_glue.c |   18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 806c44f..7109506 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) {
@@ -902,9 +903,20 @@ static int disable_device(struct acpiphp_slot *slot)
 			func->bridge = NULL;
 		}
 
-		pdev = pci_get_slot(slot->bridge->pci_bus,
-				    PCI_DEVFN(slot->device, func->function));
-		if (pdev) {
+		while (1) {
+			/* pci_bus_sem is used to protect bus->devices list,
+			   it may cause invalid memory access if threads
+			   modify bus->devices list concurrently. */
+			down_read(&pci_bus_sem);
+			list_for_each_entry(pdev, &bus->devices, bus_list)
+				if (PCI_SLOT(pdev->devfn) == slot->device) {
+					pci_dev_get(pdev);
+					break;
+				}
+			up_read(&pci_bus_sem);
+
+			if (PCI_SLOT(pdev->devfn) != slot->device)
+				break;
 			pci_stop_bus_device(pdev);
 			if (pdev->subordinate) {
 				disable_bridges(pdev->subordinate);
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH v3] pci: clean all funcs when hot-removing multifunc device

[Amos Kong]

On Wed, May 16, 2012 at 11:26 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Fri, May 11, 2012 at 8:00 AM, Jiang Liu <liuj97@gmail.com> wrote:
>> On 05/11/2012 08:24 AM, Amos Kong wrote:
>>> On 05/11/2012 07:54 AM, Amos Kong wrote:
>>>> On 05/11/2012 02:55 AM, Michael S. Tsirkin wrote:
>>>>> On Fri, May 11, 2012 at 01:09:13AM +0800, Jiang Liu wrote:
>>>>>> On 05/10/2012 11:44 PM, Amos Kong wrote:
>>>>>>
>>>>>>> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
>>>>>>> index 806c44f..a7442d9 100644
>>>>>>> --- a/drivers/pci/hotplug/acpiphp_glue.c
>>>>>>> +++ b/drivers/pci/hotplug/acpiphp_glue.c
>>>>>>> @@ -885,7 +885,7 @@ static void disable_bridges(struct pci_bus *bus)
>>>>>>>  static int disable_device(struct acpiphp_slot *slot)
>>>>>>>  {
>>>>>>>   struct acpiphp_func *func;
>>>>>>> - struct pci_dev *pdev;
>>>>>>> + struct pci_dev *pdev, *tmp;
>>>>>>>   struct pci_bus *bus = slot->bridge->pci_bus;
>>>>>>>
>>>>>>>   /* The slot will be enabled when func 0 is added, so check
>>>>>>> @@ -902,9 +902,10 @@ static int disable_device(struct acpiphp_slot *slot)
>>>>>>>                   func->bridge = NULL;
>>>>>>>           }
>>>>>>>
>>>>>>> -         pdev = pci_get_slot(slot->bridge->pci_bus,
>>>>>>> -                             PCI_DEVFN(slot->device, func->function));
>>>>>>> -         if (pdev) {
>>>>>>> +         list_for_each_entry_safe(pdev, tmp, &bus->devices, bus_list) {
>>>>>>> +                 if (PCI_SLOT(pdev->devfn) != slot->device)
>>>>>>> +                         continue;
>
> I think the concept is good: in enable_device(), we use
> pci_scan_slot(), which scans all possible functions in the slot.  So
> in disable_device() we should do something symmetric to remove all the
> functions.

Right!

>>>>>>> +
>>>>>> The pci_bus_sem lock should be acquired when walking the bus->devices list.
>>>>>> Otherwise it may cause invalid memory access if another thread is modifying
>>>>>> the bus->devices list concurrently.
>>>
>>> pci_bus_sem lock is only request for writing &bus->devices list, right ?
>>> and this protection already exists in pci_destory_dev().
>> That's for writer. For reader to walk the pci_bus->devices list, you also need
>> to acquire the reader lock by down_read(&pci_bus_sem). Please refer to
>> pci_get_slot() for example. This especially import for native OS because there
>> may be multiple PCI slots/devices on the bus.
>
> There is a lot of existing code that walks bus->devices without
> holding pci_bus_sem, but most of it is boot-time code that is arguably
> safe (though I think things like pcibios_fixup_bus() are poorly
> designed and don't fit well in the hotplug-enabled world).

disable_remove() is not boot-time code, we might hot-remove devices
when system is running.

> In this case, I do think we need to protect against updates while
> we're walking bus->devices.  It's probably not trivial because
> __pci_remove_bus_device() calls pci_destroy_dev(), where we do the
> down_write(), so simply wrapping the whole thing with down_read() will
> cause a deadlock.

I posted a V4 to add pci_bus_sem protection , please help to review.
Thanks for Jiang Liu's guide.

> Kenji-san, Yinghai, do you have any input?
>
> Bjorn

Re: [Qemu-devel] [PATCH v3] pci: clean all funcs when hot-removing multifunc device

[Amos Kong]

On Wed, May 16, 2012 at 11:26 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Fri, May 11, 2012 at 8:00 AM, Jiang Liu <liuj97@gmail.com> wrote:
>> On 05/11/2012 08:24 AM, Amos Kong wrote:
>>> On 05/11/2012 07:54 AM, Amos Kong wrote:
>>>> On 05/11/2012 02:55 AM, Michael S. Tsirkin wrote:
>>>>> On Fri, May 11, 2012 at 01:09:13AM +0800, Jiang Liu wrote:
>>>>>> On 05/10/2012 11:44 PM, Amos Kong wrote:
>>>>>>
>>>>>>> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
>>>>>>> index 806c44f..a7442d9 100644
>>>>>>> --- a/drivers/pci/hotplug/acpiphp_glue.c
>>>>>>> +++ b/drivers/pci/hotplug/acpiphp_glue.c
>>>>>>> @@ -885,7 +885,7 @@ static void disable_bridges(struct pci_bus *bus)
>>>>>>>  static int disable_device(struct acpiphp_slot *slot)
>>>>>>>  {
>>>>>>>   struct acpiphp_func *func;
>>>>>>> - struct pci_dev *pdev;
>>>>>>> + struct pci_dev *pdev, *tmp;
>>>>>>>   struct pci_bus *bus = slot->bridge->pci_bus;
>>>>>>>
>>>>>>>   /* The slot will be enabled when func 0 is added, so check
>>>>>>> @@ -902,9 +902,10 @@ static int disable_device(struct acpiphp_slot *slot)
>>>>>>>                   func->bridge = NULL;
>>>>>>>           }
>>>>>>>
>>>>>>> -         pdev = pci_get_slot(slot->bridge->pci_bus,
>>>>>>> -                             PCI_DEVFN(slot->device, func->function));
>>>>>>> -         if (pdev) {
>>>>>>> +         list_for_each_entry_safe(pdev, tmp, &bus->devices, bus_list) {
>>>>>>> +                 if (PCI_SLOT(pdev->devfn) != slot->device)
>>>>>>> +                         continue;
>
> I think the concept is good: in enable_device(), we use
> pci_scan_slot(), which scans all possible functions in the slot.  So
> in disable_device() we should do something symmetric to remove all the
> functions.

Right!

>>>>>>> +
>>>>>> The pci_bus_sem lock should be acquired when walking the bus->devices list.
>>>>>> Otherwise it may cause invalid memory access if another thread is modifying
>>>>>> the bus->devices list concurrently.
>>>
>>> pci_bus_sem lock is only request for writing &bus->devices list, right ?
>>> and this protection already exists in pci_destory_dev().
>> That's for writer. For reader to walk the pci_bus->devices list, you also need
>> to acquire the reader lock by down_read(&pci_bus_sem). Please refer to
>> pci_get_slot() for example. This especially import for native OS because there
>> may be multiple PCI slots/devices on the bus.
>
> There is a lot of existing code that walks bus->devices without
> holding pci_bus_sem, but most of it is boot-time code that is arguably
> safe (though I think things like pcibios_fixup_bus() are poorly
> designed and don't fit well in the hotplug-enabled world).

disable_remove() is not boot-time code, we might hot-remove devices
when system is running.

> In this case, I do think we need to protect against updates while
> we're walking bus->devices.  It's probably not trivial because
> __pci_remove_bus_device() calls pci_destroy_dev(), where we do the
> down_write(), so simply wrapping the whole thing with down_read() will
> cause a deadlock.

I posted a V4 to add pci_bus_sem protection , please help to review.
Thanks for Jiang Liu's guide.

> Kenji-san, Yinghai, do you have any input?
>
> Bjorn

[PATCH v5] pci: clean all funcs when hot-removing multifunc device

[kongjianjun]

From: Amos Kong <kongjianjun@gmail.com>

Hotplug CallTrace:
int acpiphp_enable_slot(struct acpiphp_slot *slot)
    \_enable_device(slot);
       \_pci_bus_add_devices(bus);
            # un-added new devs(all funcs in slot) will be added
            list_for_each_entry(dev, &bus->devices, bus_list) {
                if (dev->is_added)
                        continue;
                pci_bus_add_device(dev);
                device_add(&dev->dev);
                dev->is_added = 1;

'dev->is_added' is used to trace if pci dev is added to bus, all funcs in
same slot would be added to bus in enable_device(slot). So we need to clean
all funcs of same slot in disable_device(slot).

But hot-remove exists bug: https://bugzilla.kernel.org/show_bug.cgi?id=43219
(dmesg and DSDT were attached in bz), detail:

. Boot up a Linux VM with 8 pci block devices which are the 8
functions in one pci slot.
| # qemu-kvm ...
| -drive file=images/u0,if=none,id=drv0,format=qcow2,cache=none \
| -device virtio-blk-pci,drive=drv0,id=v0,multifunction=on,addr=0x03.0 \
| ....
| -drive file=images/u7,if=none,id=drv7,format=qcow2,cache=none \
| -device virtio-blk-pci,drive=drv7,id=v7,multifunction=on,addr=0x03.7 \

. Check devices in guest.
| vm)# ls /dev/vd*
|    vda vdb vdc vde vdf vdg vdh
| vm)# lspci |grep block
| 00:03.0 SCSI storage controller: Red Hat, Inc Virtio block device
|    ...
| 00:03.7 SCSI storage controller: Red Hat, Inc Virtio block device
|

. Func1~7 still exist in guest after hot-removing the whole slot
by qemu monitor cmd.
| vm)# lspci |grep block    (00:03.0 disappeared)
| 00:03.1 SCSI storage controller: Red Hat, Inc Virtio block device (rev ff)
|    ...
| 00:03.7 SCSI storage controller: Red Hat, Inc Virtio block device (rev ff)
| vm)# ls /dev/vd*          (vda disappeared)
|    vdb vdc vde vdf vdg vdh
| vm)# mkfs /dev/vdb
|    INFO: task mkfs.ext2:1784 blocked for more than 120 seconds.

We process pci slot as a whole device in pciphp spec, seabios only
defines one device for a slot in ACPI DSDT table.
In acpiphp_glue.c:register_slot(), only one entry is added (for func#0)
into 'slot->funcs' list. When we release the whole slot, only
the entry in 'slot->funcs' will be cleaned, so func#1~7 could
not be cleaned from system.

| drivers/pci/hotplug/acpiphp_glue.c:
| static int disable_device(struct acpiphp_slot *slot) {
| 	list_for_each_entry(func, &slot->funcs, sibling) {
| 		pdev = pci_get_slot(slot->bridge->pci_bus,
| 		       PCI_DEVFN(slot->device, func->function));
| 		..clean code.. // those code is only executed 1 time(for func#0)
|                 __pci_remove_bus_device(pdev);
|                 pci_dev_put(pdev);

Hotpluging multifunc of guests(WinXp/Win7) is fine.

Changes from v1:
- rebase patch to latest linux.git
- remove unnecessary multiplefunction check
- rename 'i' to meaningful 'fn'
- fix coding style

Changes from v2:
- update detail reason(calltrace) to commitlog
- remove hardcode 8, find funcs in pci devlist

Changes from v3:
- use pci_bus_sem lock when walking bus->devices list

Changes from V4:
- check null point of pdev

Signed-off-by: Amos Kong <kongjianjun@gmail.com>
---
 drivers/pci/hotplug/acpiphp_glue.c |   18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 806c44f..ba46724 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) {
@@ -902,9 +903,20 @@ static int disable_device(struct acpiphp_slot *slot)
 			func->bridge = NULL;
 		}
 
-		pdev = pci_get_slot(slot->bridge->pci_bus,
-				    PCI_DEVFN(slot->device, func->function));
-		if (pdev) {
+		while (1) {
+			/* pci_bus_sem is used to protect bus->devices list,
+			   it may cause invalid memory access if threads
+			   modify bus->devices list concurrently. */
+			down_read(&pci_bus_sem);
+			list_for_each_entry(pdev, &bus->devices, bus_list)
+				if (PCI_SLOT(pdev->devfn) == slot->device) {
+					pci_dev_get(pdev);
+					break;
+				}
+			up_read(&pci_bus_sem);
+
+			if (!pdev || PCI_SLOT(pdev->devfn) != slot->device)
+				break;
 			pci_stop_bus_device(pdev);
 			if (pdev->subordinate) {
 				disable_bridges(pdev->subordinate);
-- 
1.7.9.5

[Qemu-devel] [PATCH v5] pci: clean all funcs when hot-removing multifunc device

[kongjianjun]

From: Amos Kong <kongjianjun@gmail.com>

Hotplug CallTrace:
int acpiphp_enable_slot(struct acpiphp_slot *slot)
    \_enable_device(slot);
       \_pci_bus_add_devices(bus);
            # un-added new devs(all funcs in slot) will be added
            list_for_each_entry(dev, &bus->devices, bus_list) {
                if (dev->is_added)
                        continue;
                pci_bus_add_device(dev);
                device_add(&dev->dev);
                dev->is_added = 1;

'dev->is_added' is used to trace if pci dev is added to bus, all funcs in
same slot would be added to bus in enable_device(slot). So we need to clean
all funcs of same slot in disable_device(slot).

But hot-remove exists bug: https://bugzilla.kernel.org/show_bug.cgi?id=43219
(dmesg and DSDT were attached in bz), detail:

. Boot up a Linux VM with 8 pci block devices which are the 8
functions in one pci slot.
| # qemu-kvm ...
| -drive file=images/u0,if=none,id=drv0,format=qcow2,cache=none \
| -device virtio-blk-pci,drive=drv0,id=v0,multifunction=on,addr=0x03.0 \
| ....
| -drive file=images/u7,if=none,id=drv7,format=qcow2,cache=none \
| -device virtio-blk-pci,drive=drv7,id=v7,multifunction=on,addr=0x03.7 \

. Check devices in guest.
| vm)# ls /dev/vd*
|    vda vdb vdc vde vdf vdg vdh
| vm)# lspci |grep block
| 00:03.0 SCSI storage controller: Red Hat, Inc Virtio block device
|    ...
| 00:03.7 SCSI storage controller: Red Hat, Inc Virtio block device
|

. Func1~7 still exist in guest after hot-removing the whole slot
by qemu monitor cmd.
| vm)# lspci |grep block    (00:03.0 disappeared)
| 00:03.1 SCSI storage controller: Red Hat, Inc Virtio block device (rev ff)
|    ...
| 00:03.7 SCSI storage controller: Red Hat, Inc Virtio block device (rev ff)
| vm)# ls /dev/vd*          (vda disappeared)
|    vdb vdc vde vdf vdg vdh
| vm)# mkfs /dev/vdb
|    INFO: task mkfs.ext2:1784 blocked for more than 120 seconds.

We process pci slot as a whole device in pciphp spec, seabios only
defines one device for a slot in ACPI DSDT table.
In acpiphp_glue.c:register_slot(), only one entry is added (for func#0)
into 'slot->funcs' list. When we release the whole slot, only
the entry in 'slot->funcs' will be cleaned, so func#1~7 could
not be cleaned from system.

| drivers/pci/hotplug/acpiphp_glue.c:
| static int disable_device(struct acpiphp_slot *slot) {
| 	list_for_each_entry(func, &slot->funcs, sibling) {
| 		pdev = pci_get_slot(slot->bridge->pci_bus,
| 		       PCI_DEVFN(slot->device, func->function));
| 		..clean code.. // those code is only executed 1 time(for func#0)
|                 __pci_remove_bus_device(pdev);
|                 pci_dev_put(pdev);

Hotpluging multifunc of guests(WinXp/Win7) is fine.

Changes from v1:
- rebase patch to latest linux.git
- remove unnecessary multiplefunction check
- rename 'i' to meaningful 'fn'
- fix coding style

Changes from v2:
- update detail reason(calltrace) to commitlog
- remove hardcode 8, find funcs in pci devlist

Changes from v3:
- use pci_bus_sem lock when walking bus->devices list

Changes from V4:
- check null point of pdev

Signed-off-by: Amos Kong <kongjianjun@gmail.com>
---
 drivers/pci/hotplug/acpiphp_glue.c |   18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 806c44f..ba46724 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) {
@@ -902,9 +903,20 @@ static int disable_device(struct acpiphp_slot *slot)
 			func->bridge = NULL;
 		}
 
-		pdev = pci_get_slot(slot->bridge->pci_bus,
-				    PCI_DEVFN(slot->device, func->function));
-		if (pdev) {
+		while (1) {
+			/* pci_bus_sem is used to protect bus->devices list,
+			   it may cause invalid memory access if threads
+			   modify bus->devices list concurrently. */
+			down_read(&pci_bus_sem);
+			list_for_each_entry(pdev, &bus->devices, bus_list)
+				if (PCI_SLOT(pdev->devfn) == slot->device) {
+					pci_dev_get(pdev);
+					break;
+				}
+			up_read(&pci_bus_sem);
+
+			if (!pdev || PCI_SLOT(pdev->devfn) != slot->device)
+				break;
 			pci_stop_bus_device(pdev);
 			if (pdev->subordinate) {
 				disable_bridges(pdev->subordinate);
-- 
1.7.9.5