[PATCH] powerpc/pci: remove pci device on a bus in reverse order

[PATCH] powerpc/pci: remove pci device on a bus in reverse order

[Wei Yang]

As in commit ac205b7b (PCI: make sriov work with hotplug remove) indicates,
when removing pci devices on a bus which has VFs, we need to remove them in
the reverse order.

This patch applies this patten on the hotplug remove patch on powerpc arch.

Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/pci-hotplug.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/pci-hotplug.c b/arch/powerpc/kernel/pci-hotplug.c
index 5b78917..f0f00d9 100644
--- a/arch/powerpc/kernel/pci-hotplug.c
+++ b/arch/powerpc/kernel/pci-hotplug.c
@@ -50,7 +50,7 @@ void pcibios_remove_pci_devices(struct pci_bus *bus)
 
 	pr_debug("PCI: Removing devices on bus %04x:%02x\n",
 		 pci_domain_nr(bus),  bus->number);
-	list_for_each_entry_safe(dev, tmp, &bus->devices, bus_list) {
+	list_for_each_entry_safe_reverse(dev, tmp, &bus->devices, bus_list) {
 		pr_debug("   Removing %s...\n", pci_name(dev));
 		pci_stop_and_remove_bus_device(dev);
 	}
-- 
1.7.9.5

[PATCH] powerpc/pci: remove pci device on a bus in reverse order

[Wei Yang]

As in commit ac205b7b (PCI: make sriov work with hotplug remove) indicates,
when removing pci devices on a bus which has VFs, we need to remove them in
the reverse order.

This patch applies this pattern on the hotplug remove path on powerpc arch.

Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
Acked-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/pci-hotplug.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/pci-hotplug.c b/arch/powerpc/kernel/pci-hotplug.c
index 5b78917..f0f00d9 100644
--- a/arch/powerpc/kernel/pci-hotplug.c
+++ b/arch/powerpc/kernel/pci-hotplug.c
@@ -50,7 +50,7 @@ void pcibios_remove_pci_devices(struct pci_bus *bus)
 
 	pr_debug("PCI: Removing devices on bus %04x:%02x\n",
 		 pci_domain_nr(bus),  bus->number);
-	list_for_each_entry_safe(dev, tmp, &bus->devices, bus_list) {
+	list_for_each_entry_safe_reverse(dev, tmp, &bus->devices, bus_list) {
 		pr_debug("   Removing %s...\n", pci_name(dev));
 		pci_stop_and_remove_bus_device(dev);
 	}
-- 
1.7.9.5

Re: powerpc/pci: remove pci device on a bus in reverse order

[Michael Ellerman]

On Thu, 2014-04-12 at 03:24:37 UTC, Wei Yang wrote:
> As in commit ac205b7b (PCI: make sriov work with hotplug remove) indicates,
> when removing pci devices on a bus which has VFs, we need to remove them in
> the reverse order.
> 
> This patch applies this pattern on the hotplug remove path on powerpc arch.

So is this is a bug fix?

Where/how have you tested this?

cheers

Re: powerpc/pci: remove pci device on a bus in reverse order

[Wei Yang]

On Thu, Dec 04, 2014 at 04:40:35PM +1100, Michael Ellerman wrote:
>On Thu, 2014-04-12 at 03:24:37 UTC, Wei Yang wrote:
>> As in commit ac205b7b (PCI: make sriov work with hotplug remove) indicates,
>> when removing pci devices on a bus which has VFs, we need to remove them in
>> the reverse order.
>> 
>> This patch applies this pattern on the hotplug remove path on powerpc arch.
>
>So is this is a bug fix?

It hasn't trigger a bug yet. I found this issue during the code reading. When
VFs are enabled and try to remove a bus with VFs, it will face a problem. So I
port the change in commit ac205b7b here.

>
>Where/how have you tested this?

I have tested after change on Power8, the EEH hotplug path works fine for PFs
now. Will test this when EEH for VFs are ready.

Suggest me to keep it untill EEH for VFs are ready?

>
>cheers

-- 
Richard Yang
Help you, Help me

Re: powerpc/pci: remove pci device on a bus in reverse order

[Gavin Shan]

On Thu, Dec 04, 2014 at 03:21:18PM +0800, Wei Yang wrote:
>On Thu, Dec 04, 2014 at 04:40:35PM +1100, Michael Ellerman wrote:
>>On Thu, 2014-04-12 at 03:24:37 UTC, Wei Yang wrote:
>>> As in commit ac205b7b (PCI: make sriov work with hotplug remove) indicates,
>>> when removing pci devices on a bus which has VFs, we need to remove them in
>>> the reverse order.
>>> 
>>> This patch applies this pattern on the hotplug remove path on powerpc arch.
>>
>>So is this is a bug fix?
>
>It hasn't trigger a bug yet. I found this issue during the code reading. When
>VFs are enabled and try to remove a bus with VFs, it will face a problem. So I
>port the change in commit ac205b7b here.
>
>>
>>Where/how have you tested this?
>
>I have tested after change on Power8, the EEH hotplug path works fine for PFs
>now. Will test this when EEH for VFs are ready.
>
>Suggest me to keep it untill EEH for VFs are ready?
>

Please keep it and resend it (with typo fixed as I pointed) after SRIOV patchset
gets merged. If SRIOV isn't enabled, we don't need the code change.

By the way, it's something related to EEH for PFs. When PF and its VFs seat on
same PCI bus, we should remove VFs before putting PF offline in the reversed
order as you did in your code change. Otherwise, PF is put into offline and
its driver disables VFs. We try redoing the removal for VFs in hotplug path,
which would cause race condition. If VFs aren't existing, until your SRIOV
patchset is merged, we don't have this problem. Please correct me if I
understood things wrongly.

Thanks,
Gavin

Re: powerpc/pci: remove pci device on a bus in reverse order

[Wei Yang]

On Fri, Dec 05, 2014 at 09:48:13AM +1100, Gavin Shan wrote:
>On Thu, Dec 04, 2014 at 03:21:18PM +0800, Wei Yang wrote:
>>On Thu, Dec 04, 2014 at 04:40:35PM +1100, Michael Ellerman wrote:
>>>On Thu, 2014-04-12 at 03:24:37 UTC, Wei Yang wrote:
>>>> As in commit ac205b7b (PCI: make sriov work with hotplug remove) indicates,
>>>> when removing pci devices on a bus which has VFs, we need to remove them in
>>>> the reverse order.
>>>> 
>>>> This patch applies this pattern on the hotplug remove path on powerpc arch.
>>>
>>>So is this is a bug fix?
>>
>>It hasn't trigger a bug yet. I found this issue during the code reading. When
>>VFs are enabled and try to remove a bus with VFs, it will face a problem. So I
>>port the change in commit ac205b7b here.
>>
>>>
>>>Where/how have you tested this?
>>
>>I have tested after change on Power8, the EEH hotplug path works fine for PFs
>>now. Will test this when EEH for VFs are ready.
>>
>>Suggest me to keep it untill EEH for VFs are ready?
>>
>
>Please keep it and resend it (with typo fixed as I pointed) after SRIOV patchset
>gets merged. If SRIOV isn't enabled, we don't need the code change.
>
>By the way, it's something related to EEH for PFs. When PF and its VFs seat on
>same PCI bus, we should remove VFs before putting PF offline in the reversed
>order as you did in your code change. Otherwise, PF is put into offline and
>its driver disables VFs. We try redoing the removal for VFs in hotplug path,
>which would cause race condition. If VFs aren't existing, until your SRIOV
>patchset is merged, we don't have this problem. Please correct me if I
>understood things wrongly.
>

Current code is fine until VF is introduced. Yes, your understanding is
correct.

>Thanks,
>Gavin

-- 
Richard Yang
Help you, Help me

Re: [PATCH] powerpc/pci: remove pci device on a bus in reverse order

[Wei Yang]

Please ignore this one, I create an old version of the patch.

The new one is sent.

On Thu, Dec 04, 2014 at 11:04:32AM +0800, Wei Yang wrote:
>As in commit ac205b7b (PCI: make sriov work with hotplug remove) indicates,
>when removing pci devices on a bus which has VFs, we need to remove them in
>the reverse order.
>
>This patch applies this patten on the hotplug remove patch on powerpc arch.
>
>Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
>---
> arch/powerpc/kernel/pci-hotplug.c |    2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/arch/powerpc/kernel/pci-hotplug.c b/arch/powerpc/kernel/pci-hotplug.c
>index 5b78917..f0f00d9 100644
>--- a/arch/powerpc/kernel/pci-hotplug.c
>+++ b/arch/powerpc/kernel/pci-hotplug.c
>@@ -50,7 +50,7 @@ void pcibios_remove_pci_devices(struct pci_bus *bus)
>
> 	pr_debug("PCI: Removing devices on bus %04x:%02x\n",
> 		 pci_domain_nr(bus),  bus->number);
>-	list_for_each_entry_safe(dev, tmp, &bus->devices, bus_list) {
>+	list_for_each_entry_safe_reverse(dev, tmp, &bus->devices, bus_list) {
> 		pr_debug("   Removing %s...\n", pci_name(dev));
> 		pci_stop_and_remove_bus_device(dev);
> 	}
>-- 
>1.7.9.5

-- 
Richard Yang
Help you, Help me