PCI: Separate stop and remove devices in pciehp

After commit dc087f2f6a2925e81831f3016b9cbb6e470e7423 (PCI: Simplify IOV implementation and fix reference count races) VF need to be removed via virtfn_remove to make sure ref to PF is put back.

Some driver (like ixgbe) does not call pci_disable_sriov() if sriov is enabled via /sys/.../sriov_numvfs setting. ixgbe does allow driver for PF get detached, but still have VFs around.

But how about PF get removed via /sys or pciehp finally?

During hot-remove, VF will still hold one ref to PF and it prevent PF to be removed.
That make the next hot-add fails, as old PF dev struct is still around.

We need to add pci_disable_sriov() calling during stop PF .

Need this one for v3.11

Signed-off-by Yinghai Lu <[email protected]>

Cc Jiang Liu <[email protected]>

Cc Alexander Duyck <[email protected]>

Cc Donald Dutile <[email protected]>

Cc Greg Rose <[email protected]>

---
 drivers/pci/remove.c |    2 ++
 1 file changed, 2 insertions(+)

Index: linux-2.6/drivers/pci/remove.c
===================================================================

drivers/pci/remove.c +2 -0

@@ -25,6 +25,8 @@ static void pci_stop_dev(struct pci_dev
                pci_proc_detach_device(dev);
                pci_remove_sysfs_dev_files(dev);
                device_del(&dev->dev);
+               /* remove VF, if PF driver skip that */
+               pci_disable_sriov(dev);
                dev->is_added = 0;
        }
 

Quoted 9 lines · Yinghai Lu

After commit dc087f2f6a2925e81831f3016b9cbb6e470e7423 (PCI: Simplify IOV implementation and fix reference count races) VF need to be removed via virtfn_remove to make sure ref to PF is put back.

Some driver (like ixgbe) does not call pci_disable_sriov() if sriov is enabled via /sys/.../sriov_numvfs setting. ixgbe does allow driver for PF get detached, but still have VFs around.

Is this something ixgbe should be doing differently?

I'm not 100% sold on the idea of the VFs staying active after the driver releases the PF. It seems asymmetric because I think the driver has to claim the PF to *enable* the VFs, but we don't disable them when releasing the PF.

What's the use case for detaching the PF driver while the VFs are active?

Quoted 9 lines

But how about PF get removed via /sys or pciehp finally?

During hot-remove, VF will still hold one ref to PF and it prevent PF to be removed.
That make the next hot-add fails, as old PF dev struct is still around.

We need to add pci_disable_sriov() calling during stop PF .

Need this one for v3.11

Don had a concern that there might be a regression here ... I'm a bit confused on the details, but you guys need to come to agreement that this doesn't make things worse.

Quoted 23 lines

Signed-off-by Yinghai Lu<[email protected]>

Cc Jiang Liu<[email protected]>

Cc Alexander Duyck<[email protected]>

Cc Donald Dutile<[email protected]>

Cc Greg Rose<[email protected]>

---
  drivers/pci/remove.c |    2 ++
  1 file changed, 2 insertions(+)

Index: linux-2.6/drivers/pci/remove.c
===================================================================

drivers/pci/remove.c +2 -0

@@ -25,6 +25,8 @@ static void pci_stop_dev(struct pci_dev
                 pci_proc_detach_device(dev);
                 pci_remove_sysfs_dev_files(dev);
                 device_del(&dev->dev);
+               /* remove VF, if PF driver skip that */
+               pci_disable_sriov(dev);
                 dev->is_added = 0;
         }
 

Quoted 13 lines · Bjorn Helgaas

Quoted 9 lines · Yinghai Lu

After commit dc087f2f6a2925e81831f3016b9cbb6e470e7423 (PCI: Simplify IOV implementation and fix reference count races) VF need to be removed via virtfn_remove to make sure ref to PF is put back.

Some driver (like ixgbe) does not call pci_disable_sriov() if sriov is enabled via /sys/.../sriov_numvfs setting. ixgbe does allow driver for PF get detached, but still have VFs around.

Is this something ixgbe should be doing differently?

Quoted 8 lines

I'm not 100% sold on the idea of the VFs staying active after the driver releases the PF. It seems asymmetric because I think the driver has to claim the PF to *enable* the VFs, but we don't disable them when releasing the PF.

What's the use case for detaching the PF driver while the VFs are active?

VF's assigned to (KVM) guest (via device-assignment). Virtually, it's as if the enet cable is unplugged to the VF in the guest --
the device is still there (the PF wasn't unplugged, just the driver de-configured).

Pre-sysfs-based configuration, the std way to configure the VFs into a system was to unload the PF driver, and reload it with a vf-enabling parameter
(like max_vfs=<n> in the case of ixgbe, igb).
Now, if someone unloaded the PF driver in the host, the unplanned removal of the PF enabled the VF to crash the host (maybe AlexD can provide the details how that occurred).
So, the solution was to 'pause' the VF operation and let packets drop in the guest, and re-enable the VF operation if the PF driver was re-configured.

So, as I stated in previous postings, this patch is acceptable if it doesn't cause a guest crash when a VF is assigned to a KVM guest. If you tested this case, then please state as such in the posting. If not, then can AlexD test this case ?

- Don

Quoted 37 lines

Quoted 9 lines

But how about PF get removed via /sys or pciehp finally?

During hot-remove, VF will still hold one ref to PF and it prevent PF to be removed.
That make the next hot-add fails, as old PF dev struct is still around.

We need to add pci_disable_sriov() calling during stop PF .

Need this one for v3.11

Don had a concern that there might be a regression here ... I'm a bit confused on the details, but you guys need to come to agreement that this doesn't make things worse.

Quoted 23 lines

Signed-off-by Yinghai Lu<[email protected]>

Cc Jiang Liu<[email protected]>

Cc Alexander Duyck<[email protected]>

Cc Donald Dutile<[email protected]>

Cc Greg Rose<[email protected]>

---
  drivers/pci/remove.c |    2 ++
  1 file changed, 2 insertions(+)

Index: linux-2.6/drivers/pci/remove.c
===================================================================

drivers/pci/remove.c +2 -0

@@ -25,6 +25,8 @@ static void pci_stop_dev(struct pci_dev
                 pci_proc_detach_device(dev);
                 pci_remove_sysfs_dev_files(dev);
                 device_del(&dev->dev);
+               /* remove VF, if PF driver skip that */
+               pci_disable_sriov(dev);
                 dev->is_added = 0;
         }
 

Quoted 39 lines · Don Dutile

Quoted 13 lines · Bjorn Helgaas

Quoted 9 lines · Yinghai Lu

After commit dc087f2f6a2925e81831f3016b9cbb6e470e7423 (PCI: Simplify IOV implementation and fix reference count races) VF need to be removed via virtfn_remove to make sure ref to PF is put back.

Some driver (like ixgbe) does not call pci_disable_sriov() if sriov is enabled via /sys/.../sriov_numvfs setting. ixgbe does allow driver for PF get detached, but still have VFs around.

Is this something ixgbe should be doing differently?

Quoted 8 lines

I'm not 100% sold on the idea of the VFs staying active after the driver releases the PF. It seems asymmetric because I think the driver has to claim the PF to *enable* the VFs, but we don't disable them when releasing the PF.

What's the use case for detaching the PF driver while the VFs are active?

VF's assigned to (KVM) guest (via device-assignment). Virtually, it's as if the enet cable is unplugged to the VF in the guest --
the device is still there (the PF wasn't unplugged, just the driver de-configured).

Pre-sysfs-based configuration, the std way to configure the VFs into a system was to unload the PF driver, and reload it with a vf-enabling parameter
(like max_vfs=<n> in the case of ixgbe, igb).
Now, if someone unloaded the PF driver in the host, the unplanned removal of the PF enabled the VF to crash the host (maybe AlexD can provide the details how that occurred).
So, the solution was to 'pause' the VF operation and let packets drop in the guest, and re-enable the VF operation if the PF driver was re-configured.

Quoted 6 lines

So, as I stated in previous postings, this patch is acceptable if it doesn't cause a guest crash when a VF is assigned to a KVM guest. If you tested this case, then please state as such in the posting. If not, then can AlexD test this case ?

- Don

I actually haven't done much with direct assignment to guests. I believe it was Greg who did that work to fix this issue.

I'm adding Jeff Kirsher to the CC. Perhaps he can pull this patch into a copy of the net tree and submit it to our validation team for testing to see if they end up being able to reproduce the kernel panic issue that was originally addressed by allowing the VFs to be persistent.

Thanks,

Alex

Quoted 38 lines

Quoted 37 lines

Quoted 9 lines

But how about PF get removed via /sys or pciehp finally?

During hot-remove, VF will still hold one ref to PF and it prevent PF to be removed.
That make the next hot-add fails, as old PF dev struct is still around.

We need to add pci_disable_sriov() calling during stop PF .

Need this one for v3.11

Don had a concern that there might be a regression here ... I'm a bit confused on the details, but you guys need to come to agreement that this doesn't make things worse.

Quoted 23 lines

Signed-off-by Yinghai Lu<[email protected]>

Cc Jiang Liu<[email protected]>

Cc Alexander Duyck<[email protected]>

Cc Donald Dutile<[email protected]>

Cc Greg Rose<[email protected]>

---
   drivers/pci/remove.c |    2 ++
   1 file changed, 2 insertions(+)

Index: linux-2.6/drivers/pci/remove.c
===================================================================

drivers/pci/remove.c +2 -0

@@ -25,6 +25,8 @@ static void pci_stop_dev(struct pci_dev
                  pci_proc_detach_device(dev);
                  pci_remove_sysfs_dev_files(dev);
                  device_del(&dev->dev);
+               /* remove VF, if PF driver skip that */
+               pci_disable_sriov(dev);
                  dev->is_added = 0;
          }
 

Quoted 50 lines · Alexander Duyck

Quoted 39 lines · Don Dutile

Quoted 13 lines · Bjorn Helgaas

Quoted 9 lines · Yinghai Lu

After commit dc087f2f6a2925e81831f3016b9cbb6e470e7423 (PCI: Simplify IOV implementation and fix reference count races) VF need to be removed via virtfn_remove to make sure ref to PF is put back.

Some driver (like ixgbe) does not call pci_disable_sriov() if sriov is enabled via /sys/.../sriov_numvfs setting. ixgbe does allow driver for PF get detached, but still have VFs around.

Is this something ixgbe should be doing differently?

Quoted 8 lines

I'm not 100% sold on the idea of the VFs staying active after the driver releases the PF. It seems asymmetric because I think the driver has to claim the PF to *enable* the VFs, but we don't disable them when releasing the PF.

What's the use case for detaching the PF driver while the VFs are active?

VF's assigned to (KVM) guest (via device-assignment). Virtually, it's as if the enet cable is unplugged to the VF in the guest --
the device is still there (the PF wasn't unplugged, just the driver de-configured).

Pre-sysfs-based configuration, the std way to configure the VFs into a system was to unload the PF driver, and reload it with a vf-enabling parameter
(like max_vfs=<n> in the case of ixgbe, igb).
Now, if someone unloaded the PF driver in the host, the unplanned removal of the PF enabled the VF to crash the host (maybe AlexD can provide the details how that occurred).
So, the solution was to 'pause' the VF operation and let packets drop in the guest, and re-enable the VF operation if the PF driver was re-configured.

Quoted 6 lines

So, as I stated in previous postings, this patch is acceptable if it doesn't cause a guest crash when a VF is assigned to a KVM guest. If you tested this case, then please state as such in the posting. If not, then can AlexD test this case ?

- Don

I actually haven't done much with direct assignment to guests. I believe it was Greg who did that work to fix this issue.

Quoted 5 lines

I'm adding Jeff Kirsher to the CC. Perhaps he can pull this patch into a copy of the net tree and submit it to our validation team for testing to see if they end up being able to reproduce the kernel panic issue that was originally addressed by allowing the VFs to be persistent.

I'd appreciate hearing Jeff's test results....

Quoted 44 lines

Thanks,

Alex

Quoted 38 lines

Quoted 37 lines

Quoted 9 lines

But how about PF get removed via /sys or pciehp finally?

During hot-remove, VF will still hold one ref to PF and it prevent PF to be removed.
That make the next hot-add fails, as old PF dev struct is still around.

We need to add pci_disable_sriov() calling during stop PF .

Need this one for v3.11

Don had a concern that there might be a regression here ... I'm a bit confused on the details, but you guys need to come to agreement that this doesn't make things worse.

Quoted 23 lines

Signed-off-by Yinghai Lu<[email protected]>

Cc Jiang Liu<[email protected]>

Cc Alexander Duyck<[email protected]>

Cc Donald Dutile<[email protected]>

Cc Greg Rose<[email protected]>

---
   drivers/pci/remove.c |    2 ++
   1 file changed, 2 insertions(+)

Index: linux-2.6/drivers/pci/remove.c
===================================================================

drivers/pci/remove.c +2 -0

@@ -25,6 +25,8 @@ static void pci_stop_dev(struct pci_dev
                  pci_proc_detach_device(dev);
                  pci_remove_sysfs_dev_files(dev);
                  device_del(&dev->dev);
+               /* remove VF, if PF driver skip that */
+               pci_disable_sriov(dev);
                  dev->is_added = 0;
          }
 

Can I apply this patch as a standalone? or will I need the entire 3 patch series?

Quoted 2 lines · Jeff Kirsher

Can I apply this patch as a standalone? or will I need the entire 3 patch series?

would be better if you can test those three on top of pci/for-linus branch.
http://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=for-linus

Quoted 7 lines · Yinghai Lu

Quoted 2 lines · Jeff Kirsher

Can I apply this patch as a standalone? or will I need the entire 3 patch series?

would be better if you can test those three on top of pci/for-linus branch.
http://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=for-linus

I will have to do that, because I was only able to get patch 3 of the series to apply to David Miller's net tree.

Quoted 7 lines · Bjorn Helgaas

Quoted 5 lines · Yinghai Lu

After commit dc087f2f6a2925e81831f3016b9cbb6e470e7423 (PCI: Simplify IOV implementation and fix reference count races) VF need to be removed via virtfn_remove to make sure ref to PF is put back.

Quoted 12 lines

Quoted 4 lines

Some driver (like ixgbe) does not call pci_disable_sriov() if sriov is enabled via /sys/.../sriov_numvfs setting. ixgbe does allow driver for PF get detached, but still have VFs around.

Is this something ixgbe should be doing differently?

I'm not 100% sold on the idea of the VFs staying active after the driver releases the PF. It seems asymmetric because I think the driver has to claim the PF to *enable* the VFs, but we don't disable them when releasing the PF.

Quoted 2 lines

What's the use case for detaching the PF driver while the VFs are active?

VF's assigned to (KVM) guest (via device-assignment). Virtually, it's as if the enet cable is unplugged to the VF in the guest -- the device is still there (the PF wasn't unplugged, just the driver de-configured).

Pre-sysfs-based configuration, the std way to configure the VFs into a system was to unload the PF driver, and reload it with a vf-enabling parameter
(like max_vfs=<n> in the case of ixgbe, igb).

Now, if someone unloaded the PF driver in the host, the unplanned removal of the PF enabled the VF to crash the host (maybe AlexD can provide the details how that occurred).
So, the solution was to 'pause' the VF operation and let packets drop in the guest, and re-enable the VF operation if the PF driver was re-configured.

Quoted 8 lines · Don Dutile

Quoted 7 lines · Bjorn Helgaas

Quoted 5 lines · Yinghai Lu

After commit dc087f2f6a2925e81831f3016b9cbb6e470e7423 (PCI: Simplify IOV implementation and fix reference count races) VF need to be removed via virtfn_remove to make sure ref to PF is put back.

OK, I'm lost here. I think this is the scenario where Yinghai saw a regression (please correct me if not):

   00:03.0 Root port to [bus 02]
   02:00.0 SR-IOV NIC (PF in slot 2)
   02:00.1 VF (for example)

   # echo -n 0>  /sys/bus/pci/slots/2/power
   02:00.0 PF is powered off
   02:00.0 PF pci_dev is released, but VF 02:00.1 pci_dev still exists

and holds a reference to the PF pci_dev, so the 02:00.0 pci_dev is not actually deallocated

   # echo -n 1>  /sys/bus/pci/slots/2/power
   pciehp 0000:00:03.0:pcie04: Device 0000:02:00.0 already exists at
0000:02:00, cannot hot-add

Prior to dc087f2f6 ("Simplify IOV implementation and fix reference count races"), this scenario (powering the slot off then back on) apparently worked, and hot-adding 02:00.0 worked fine.

But what about the VF pci_devs? Prior to dc087f2f6, I assume they still existed even after we removed power from the PF. But obviously the hardware VFs are disabled when we power the slot back up. It doesn't make sense to me to have pci_devs for these VFs that are no longer enabled, so maybe I'm missing something here.

Quoted 14 lines

Quoted 14 lines

Quoted 4 lines

Some driver (like ixgbe) does not call pci_disable_sriov() if sriov is enabled via /sys/.../sriov_numvfs setting. ixgbe does allow driver for PF get detached, but still have VFs around.

Is this something ixgbe should be doing differently?

I'm not 100% sold on the idea of the VFs staying active after the driver releases the PF. It seems asymmetric because I think the driver has to claim the PF to *enable* the VFs, but we don't disable them when releasing the PF.

What's the use case for detaching the PF driver while the VFs are active?

Quoted 4 lines

VF's assigned to (KVM) guest (via device-assignment). Virtually, it's as if the enet cable is unplugged to the VF in the guest -- the device is still there (the PF wasn't unplugged, just the driver de-configured).

OK, but why is it necessary to detach and reattach the PF driver at all? Are you trying to update the PF driver or something? Change the number of VFs?

Quoted 4 lines

Pre-sysfs-based configuration, the std way to configure the VFs into a system was to unload the PF driver, and reload it with a vf-enabling parameter
(like max_vfs=<n> in the case of ixgbe, igb).

Yes. But I assume the first time the PF was loaded, there were no VFs enabled. So disabling VFs at unload wouldn't cause any problem there.
Then you reload the driver with "max_vfs=<n>". The driver enables
VFs. Is there any reason to unload the PF driver again?

Quoted 6 lines

Now, if someone unloaded the PF driver in the host, the unplanned removal of the PF enabled the VF to crash the host (maybe AlexD can provide the details how that occurred).
So, the solution was to 'pause' the VF operation and let packets drop in the guest, and re-enable the VF operation if the PF driver was re-configured.

I don't get this. Why should we tolerate "unplanned removal of the PF"? If you yank the card, I would *expect* anything using it to crash.

Bjorn

Quoted 65 lines · Bjorn Helgaas

Quoted 11 lines · Don Dutile

Quoted 9 lines · Bjorn Helgaas

Quoted 6 lines · Yinghai Lu

After commit dc087f2f6a2925e81831f3016b9cbb6e470e7423 (PCI: Simplify IOV implementation and fix reference count races) VF need to be removed via virtfn_remove to make sure ref to PF is put back.

OK, I'm lost here. I think this is the scenario where Yinghai saw a regression (please correct me if not):

   00:03.0 Root port to [bus 02]
   02:00.0 SR-IOV NIC (PF in slot 2)
   02:00.1 VF (for example)

   # echo -n 0>  /sys/bus/pci/slots/2/power
   02:00.0 PF is powered off
   02:00.0 PF pci_dev is released, but VF 02:00.1 pci_dev still exists

and holds a reference to the PF pci_dev, so the 02:00.0 pci_dev is not actually deallocated

   # echo -n 1>  /sys/bus/pci/slots/2/power
   pciehp 0000:00:03.0:pcie04: Device 0000:02:00.0 already exists at
0000:02:00, cannot hot-add

Prior to dc087f2f6 ("Simplify IOV implementation and fix reference count races"), this scenario (powering the slot off then back on) apparently worked, and hot-adding 02:00.0 worked fine.

But what about the VF pci_devs? Prior to dc087f2f6, I assume they still existed even after we removed power from the PF. But obviously the hardware VFs are disabled when we power the slot back up. It doesn't make sense to me to have pci_devs for these VFs that are no longer enabled, so maybe I'm missing something here.

Quoted 21 lines

Quoted 8 lines

Quoted 4 lines

Some driver (like ixgbe) does not call pci_disable_sriov() if sriov is enabled via /sys/.../sriov_numvfs setting. ixgbe does allow driver for PF get detached, but still have VFs around.

Is this something ixgbe should be doing differently?

Quoted 8 lines

I'm not 100% sold on the idea of the VFs staying active after the driver releases the PF. It seems asymmetric because I think the driver has to claim the PF to *enable* the VFs, but we don't disable them when releasing the PF.

What's the use case for detaching the PF driver while the VFs are active?

VF's assigned to (KVM) guest (via device-assignment). Virtually, it's as if the enet cable is unplugged to the VF in the guest --
the device is still there (the PF wasn't unplugged, just the driver de-configured).

Quoted 4 lines

OK, but why is it necessary to detach and reattach the PF driver at all? Are you trying to update the PF driver or something? Change the number of VFs?

One case is to update the PF driver; the other is 'operator error' :-/ Want to not crash the guest w/VFs, and allow operator to 'mend their error', i.e., re-load the PF driver back.

Quoted 32 lines · Bjorn Helgaas

Quoted 9 lines · Don Dutile

Quoted 8 lines · Bjorn Helgaas

Quoted 5 lines · Yinghai Lu

After commit dc087f2f6a2925e81831f3016b9cbb6e470e7423 (PCI: Simplify IOV implementation and fix reference count races) VF need to be removed via virtfn_remove to make sure ref to PF is put back.

OK, I'm lost here. I think this is the scenario where Yinghai saw a regression (please correct me if not):

   00:03.0 Root port to [bus 02]
   02:00.0 SR-IOV NIC (PF in slot 2)
   02:00.1 VF (for example)

   # echo -n 0>  /sys/bus/pci/slots/2/power
   02:00.0 PF is powered off
   02:00.0 PF pci_dev is released, but VF 02:00.1 pci_dev still exists

and holds a reference to the PF pci_dev, so the 02:00.0 pci_dev is not actually deallocated

   # echo -n 1>  /sys/bus/pci/slots/2/power
   pciehp 0000:00:03.0:pcie04: Device 0000:02:00.0 already exists at
0000:02:00, cannot hot-add

Prior to dc087f2f6 ("Simplify IOV implementation and fix reference count races"), this scenario (powering the slot off then back on) apparently worked, and hot-adding 02:00.0 worked fine.

Quoted 6 lines

But what about the VF pci_devs? Prior to dc087f2f6, I assume they still existed even after we removed power from the PF. But obviously the hardware VFs are disabled when we power the slot back up. It doesn't make sense to me to have pci_devs for these VFs that are no longer enabled, so maybe I'm missing something here.

Quoted 21 lines

Quoted 20 lines

Quoted 7 lines

Quoted 4 lines

Some driver (like ixgbe) does not call pci_disable_sriov() if sriov is enabled via /sys/.../sriov_numvfs setting. ixgbe does allow driver for PF get detached, but still have VFs around.

Is this something ixgbe should be doing differently?

Quoted 8 lines

I'm not 100% sold on the idea of the VFs staying active after the driver releases the PF. It seems asymmetric because I think the driver has to claim the PF to *enable* the VFs, but we don't disable them when releasing the PF.

What's the use case for detaching the PF driver while the VFs are active?

VF's assigned to (KVM) guest (via device-assignment). Virtually, it's as if the enet cable is unplugged to the VF in the guest --
the device is still there (the PF wasn't unplugged, just the driver de-configured).

Quoted 4 lines

OK, but why is it necessary to detach and reattach the PF driver at all? Are you trying to update the PF driver or something? Change the number of VFs?

One case is to update the PF driver; the other is 'operator error' :-/ Want to not crash the guest w/VFs, and allow operator to 'mend their error',
i.e., re-load the PF driver back.

Quoted 18 lines

Quoted 4 lines

Pre-sysfs-based configuration, the std way to configure the VFs into a system was to unload the PF driver, and reload it with a vf-enabling parameter
(like max_vfs=<n> in the case of ixgbe, igb).

Yes. But I assume the first time the PF was loaded, there were no VFs enabled. So disabling VFs at unload wouldn't cause any problem there.
Then you reload the driver with "max_vfs=<n>". The driver enables
VFs. Is there any reason to unload the PF driver again?

Quoted 7 lines

Now, if someone unloaded the PF driver in the host, the unplanned removal
of the PF enabled the VF to crash the host (maybe AlexD can provide the details how that occurred).
So, the solution was to 'pause' the VF operation and let packets drop in the guest, and re-enable the VF operation if the PF driver was re-configured.

Quoted 4 lines

I don't get this. Why should we tolerate "unplanned removal of the PF"? If you yank the card, I would *expect* anything using it to crash.

Last time I checked, PCIe handles surprise removal, electrically, on unplanned removal -- it's capacitively coupled so crashes due to odd signalling transition can be handled (malformed pcie packet, w/c, which aer handles/dismisses).

Note, the i(x)gb(e) code was designed to handle operator error around PF driver removal. hot plug/unplug was probably not tested against the 'save the VF state' code in the i(x)gb(e)vf drivers. Again, try asking the driver owner(s).

commit dc087f2f6a2925e81831f3016b9cbb6e470e7423
(PCI: Simplify IOV implementation and fix reference count races) broke the pcie native hotplug with SRIOV enabled: PF is not freed during hot-remove, and later hot-add do not work as old pci_dev is still around, and can not create new pci_dev.

That commit need VFs to be removed via pci_disable_sriov/virtfn_remove to make sure ref to PF is put back.

So we can not call stop_and_remove for VF before PF, that will make VF get removed directly before PF's driver try to use pci_disable_sriov/virtfn_remove to do it.

Solution is separating stop and remove in two iterations.

In first iteration, we stop VF driver at first with iterating devices reversely, and later during stop PF driver, disable_sriov will use virtfn_remove to remove VFs.

Also some driver (like mlx4_core) need VF's driver get stopped before PF's.

Need this one for v3.11.

-v2: separate VGA checking to another patch according to Bjorn.
and after patches that make pciehp to be built-in

Signed-off-by Yinghai Lu <[email protected]>

Cc Yijing Wang <[email protected]>

Cc Jiang Liu <[email protected]>

---
 drivers/pci/hotplug/pciehp_pci.c |   15 +++++++++++++--
 drivers/pci/pci.h                |    3 +++
 drivers/pci/remove.c             |    4 ++--
 3 files changed, 18 insertions(+), 4 deletions(-)

Index: linux-2.6/drivers/pci/hotplug/pciehp_pci.c
===================================================================

drivers/pci/hotplug/pciehp_pci.c +8 -2

@@ -109,6 +109,13 @@ int pciehp_unconfigure_device(struct slo
 	}
 
 	/*
+	 * Now VF need to be removed via virtfn_remove to make
+	 * sure ref to PF is put back. Some driver (mlx4_core) need
+	 * VF's driver get stopped before PF.
+	 * So we need to stop VF driver at first, that means
+	 * loop reversely, and later during stop PF driver,
+	 * disable_sriov will use virtfn_remove to remove VFs.
+	 *
 	 * Stopping an SR-IOV PF device removes all the associated VFs,
 	 * which will update the bus->devices list and confuse the
 	 * iterator.  Therefore, iterate in reverse so we remove the VFs

@@ -116,8 +123,7 @@ int pciehp_unconfigure_device(struct slo
 	 */
 	list_for_each_entry_safe_reverse(dev, temp, &parent->devices,
 					 bus_list) {
-		pci_dev_get(dev);
-		pci_stop_and_remove_bus_device(dev);
+		pci_stop_bus_device(dev);
 		/*
 		 * Ensure that no new Requests will be generated from
 		 * the device.

drivers/pci/hotplug/pciehp_pci.c +0 -0

 	}
 

Index: linux-2.6/drivers/pci/remove.c
===================================================================

drivers/pci/remove.c +2 -2

@@ -56,7 +56,7 @@ void pci_remove_bus(struct pci_bus *bus)
 }
 EXPORT_SYMBOL(pci_remove_bus);
 
-static void pci_stop_bus_device(struct pci_dev *dev)
+void pci_stop_bus_device(struct pci_dev *dev)
 {
 	struct pci_bus *bus = dev->subordinate;
 	struct pci_dev *child, *tmp;

@@ -76,7 +76,7 @@ static void pci_stop_bus_device(struct p
 	pci_stop_dev(dev);
 }
 
-static void pci_remove_bus_device(struct pci_dev *dev)
+void pci_remove_bus_device(struct pci_dev *dev)
 {
 	struct pci_bus *bus = dev->subordinate;
 	struct pci_dev *child, *tmp;

Index: linux-2.6/drivers/pci/pci.h
===================================================================

drivers/pci/pci.h +3 -0

@@ -317,4 +317,7 @@ static inline int pci_dev_specific_reset
 }
 #endif
 
+void pci_stop_bus_device(struct pci_dev *dev);
+void pci_remove_bus_device(struct pci_dev *dev);
+
 #endif /* DRIVERS_PCI_H */

[+cc Kenji]

Quoted 6 lines · Yinghai Lu

commit dc087f2f6a2925e81831f3016b9cbb6e470e7423
(PCI: Simplify IOV implementation and fix reference count races) broke the pcie native hotplug with SRIOV enabled: PF is not freed during hot-remove, and later hot-add do not work as old pci_dev is still around, and can not create new pci_dev.

Quoted 2 lines

That commit need VFs to be removed via pci_disable_sriov/virtfn_remove to make sure ref to PF is put back.

Huh. I wish we didn't have virtfn_remove() at all. I wish the normal device removal path, i.e., pci_stop_and_remove_bus_device(), could deal with VFs directly. I don't know all the history there, so maybe there's some reason that's not feasible.

Review

Quoted 6 lines

(quoted diff) +5 -0

+     }
+
+     list_for_each_entry_safe(dev, temp, &parent->devices, bus_list) {
+             pci_dev_get(dev);
+             pci_remove_bus_device(dev);
              pci_dev_put(dev);

I don't see the point of the pci_dev_get()/pci_dev_put() here. It doesn't do anything useful, does it?

The pci_dev_get() doesn't help: it will keep a racing path from removing the dev *after* we call pci_dev_get(), but that racing path could just as easily remove the dev *before* we call pci_dev_get().

And there's no reason to hold onto a reference after we call pci_remove_bus_device(), because we don't do anything else with the device before we call pci_dev_put().

Quoted 5 lines · Bjorn Helgaas

Huh. I wish we didn't have virtfn_remove() at all. I wish the normal device removal path, i.e., pci_stop_and_remove_bus_device(), could deal with VFs directly. I don't know all the history there, so maybe there's some reason that's not feasible.

I had one draft version, but looks more confusing.

Quoted 7 lines · Yinghai Lu

Quoted 5 lines · Bjorn Helgaas

Huh. I wish we didn't have virtfn_remove() at all. I wish the normal device removal path, i.e., pci_stop_and_remove_bus_device(), could deal with VFs directly. I don't know all the history there, so maybe there's some reason that's not feasible.

Quoted 1 line

I had one draft version, but looks more confusing.

please check attached that make pci_stop_and_remove_bus_device() could be used with VF.

It could replace
https://patchwork.kernel.org/patch/2834638/
https://patchwork.kernel.org/patch/2834639/

Please choose one of the solutions.

but we still need
https://patchwork.kernel.org/patch/2834640/
as VFs that does not use ARI could be on other virtual bus. so they will not be removed directly.

Quoted 8 lines · Yinghai Lu

Quoted 5 lines · Bjorn Helgaas

Huh. I wish we didn't have virtfn_remove() at all. I wish the normal device removal path, i.e., pci_stop_and_remove_bus_device(), could deal with VFs directly. I don't know all the history there, so maybe there's some reason that's not feasible.

I had one draft version, but looks more confusing.

please check attached that make pci_stop_and_remove_bus_device() could be used with VF.

It could replace
https://patchwork.kernel.org/patch/2834638/
https://patchwork.kernel.org/patch/2834639/

Please choose one of the solutions.

but we still need
https://patchwork.kernel.org/patch/2834640/
as VFs that does not use ARI could be on other virtual bus. so they will not be removed directly.

Subject: [PATCH] PCI: Release PF ref during removing VF
From: Yinghai Lu <[email protected]>

commit dc087f2f6a2925e81831f3016b9cbb6e470e7423
(PCI: Simplify IOV implementation and fix reference count races) broke the pcie native hotplug with SRIOV enabled: PF is not freed during hot-remove, and later hot-add do not work as old pci_dev is still around, and can not create new pci_dev.

That commit need VFs to be removed via pci_disable_sriov/virtfn_remove to make sure ref to PF is put back.

So we can not call stop_and_remove for VF before PF, that will make VF get removed directly before PF's driver try to use pci_disable_sriov/virtfn_remove to do it.

Solution would be
1. separating stop and remove in two iterations.
2. or make pci_stop_and_remove_bus_device could be used on VF.

This patch is second solution:
Separate PF ref releasing from virtfn_remove, all that during pci_destroy_dev for VFs.

Signed-off-by Yinghai Lu <[email protected]>

---
 drivers/pci/iov.c    |   34 +++++++++++++++++++++++++++++-----
 drivers/pci/pci.h    |    4 ++++
 drivers/pci/remove.c |   17 +++++++++++++++++
 3 files changed, 50 insertions(+), 5 deletions(-)

Index: linux-2.6/drivers/pci/iov.c
===================================================================

drivers/pci/iov.c +29 -5

@@ -132,9 +132,37 @@ failed:
 	return rc;
 }
 
-static void virtfn_remove(struct pci_dev *dev, int id, int reset)
+void virtfn_release(struct pci_dev *virtfn)
 {
+	int i;
+	struct pci_dev *dev;
 	char buf[VIRTFN_ID_LEN];
+
+	if (!virtfn->is_virtfn)
+		return;
+
+	dev = virtfn->physfn;
+
+	/*
+	 * Remove link to virtfn for PF.
+	 * pci_disable_sriov() could be called after pci_stop_dev() is
+	 * called for PF. So check sd to avoid spurious sfsfs warning.
+	 */
+	if (dev->dev.kobj.sd)
+		for (i = 0; i < dev->sriov->num_VFs; i++)
+			if ((virtfn_bus(dev, i) == virtfn->bus->number) &&
+			    (virtfn_devfn(dev, i) == virtfn->devfn)) {
+				sprintf(buf, "virtfn%u", i);
+				sysfs_remove_link(&dev->dev.kobj, buf);
+				break;
+			}
+
+	virtfn_remove_bus(dev->bus, virtfn->bus);
+	pci_dev_put(dev);
+}
+
+static void virtfn_remove(struct pci_dev *dev, int id, int reset)
+{
 	struct pci_dev *virtfn;
 	struct pci_sriov *iov = dev->sriov;
 

@@ -149,8 +177,6 @@ static void virtfn_remove(struct pci_dev
 		__pci_reset_function(virtfn);
 	}
 
-	sprintf(buf, "virtfn%u", id);
-	sysfs_remove_link(&dev->dev.kobj, buf);
 	/*
 	 * pci_stop_dev() could have been called for this virtfn already,
 	 * so the directory for the virtfn may have been removed before.

@@ -161,12 +187,10 @@ static void virtfn_remove(struct pci_dev
 
 	mutex_lock(&iov->dev->sriov->lock);
 	pci_stop_and_remove_bus_device(virtfn);
-	virtfn_remove_bus(dev->bus, virtfn->bus);
 	mutex_unlock(&iov->dev->sriov->lock);
 
 	/* balance pci_get_domain_bus_and_slot() */
 	pci_dev_put(virtfn);
-	pci_dev_put(dev);
 }
 
 static int sriov_migration(struct pci_dev *dev)

Index: linux-2.6/drivers/pci/pci.h
===================================================================

drivers/pci/pci.h +4 -0

@@ -262,6 +262,7 @@ int pci_iov_resource_bar(struct pci_dev
 resource_size_t pci_sriov_resource_alignment(struct pci_dev *dev, int resno);
 void pci_restore_iov_state(struct pci_dev *dev);
 int pci_iov_bus_range(struct pci_bus *bus);
+void virtfn_release(struct pci_dev *dev);
 
 #else
 static inline int pci_iov_init(struct pci_dev *dev)

@@ -284,6 +285,9 @@ static inline int pci_iov_bus_range(stru
 {
 	return 0;
 }
+void virtfn_release(struct pci_dev *dev)
+{
+}
 
 #endif /* CONFIG_PCI_IOV */
 

Index: linux-2.6/drivers/pci/remove.c
===================================================================

drivers/pci/remove.c +17 -0

@@ -38,6 +38,8 @@ static void pci_destroy_dev(struct pci_d
 	list_del(&dev->bus_list);
 	up_write(&pci_bus_sem);
 
+	virtfn_release(dev);
+
 	pci_free_resources(dev);
 	put_device(&dev->dev);
 }

@@ -107,8 +109,23 @@ static void pci_remove_bus_device(struct
  */
 void pci_stop_and_remove_bus_device(struct pci_dev *dev)
 {
+#ifdef CONFIG_PCI_ATS
+	struct pci_sriov *iov = NULL;
+	int locked = 0;
+
+	if (dev->is_virtfn) {
+		iov = dev->physfn->sriov;
+		locked = mutex_trylock(&iov->dev->sriov->lock);
+	}
+#endif
+
 	pci_stop_bus_device(dev);
 	pci_remove_bus_device(dev);
+
+#ifdef CONFIG_PCI_ATS
+	if (locked)
+		mutex_unlock(&iov->dev->sriov->lock);
+#endif
 }
 EXPORT_SYMBOL(pci_stop_and_remove_bus_device);
 

Quoted 7 lines

Solution would be
1. separating stop and remove in two iterations.
2. or make pci_stop_and_remove_bus_device could be used on VF.

This patch is second solution:
Separate PF ref releasing from virtfn_remove, all that during pci_destroy_dev for VFs.

I like the second approach better, but I think the call path and locking is way too messy:

    virtfn_remove
      mutex_lock(&iov->dev->sriov->lock)          <--
      pci_stop_and_remove_bus_device
        mutex_trylock(&iov->dev->sriov->lock)     <--

pci_stop_bus_device
pci_remove_bus_device
pci_destroy_dev
virtfn_release
virtfn_remove_bus
pci_dev_put

I think it could be fixed, but it would require significant SR-IOV rework, and I'm dubious that we can get it done in time for v3.11.

What would happen if we just reverted dc087f2f6a2 for now? Jiang?

Quoted 1 line · Bjorn Helgaas

What would happen if we just reverted dc087f2f6a2 for now? Jiang?

[+cc Don]

Quoted 3 lines · Yinghai Lu

Quoted 1 line · Bjorn Helgaas

What would happen if we just reverted dc087f2f6a2 for now? Jiang?

I tried to reproduce the problem you reported ("Cannot add device at 0000:02:00"), but I haven't been able to. I have an ixgbe SR-IOV device in an external hotplug-capable chassis, and I'm using v3.11-rc4 with a patch to add a little instrumentation. Here's what I see:

    # echo -n 8 > /sys/bus/pci/devices/0000:08:00.0/sriov_numvfs
    ixgbe 0000:08:00.0 eth1: SR-IOV enabled with 8 VFs
    pci 0000:08:10.0: [8086:1515] type 00 class 0x020000        # VF1
    pci 0000:08:10.2: [8086:1515] type 00 class 0x020000        # VF2
    ...
    pci 0000:08:11.6: [8086:1515] type 00 class 0x020000        # VF8

Created 8 VFs; no problem there.

    # echo -n 0 > /sys/bus/pci/slots/4-1/power
    pciehp 0000:04:03.0:pcie24: disable_slot: physical_slot = 4-1
    pciehp 0000:04:03.0:pcie24: pciehp_unconfigure_device:
domain:bus:dev = 0000:08:00
    ixgbevf 0000:08:11.6: pci_stop_and_remove_bus_device
    pci 0000:08:11.6: pci_release_dev                   # release VF8
    ixgbevf 0000:08:11.4: pci_stop_and_remove_bus_device
    pci 0000:08:11.4: pci_release_dev                   # release VF7
    ...
    ixgbevf 0000:08:10.0: pci_stop_and_remove_bus_device
    pci 0000:08:10.0: pci_release_dev                   # release VF1
    ixgbe 0000:08:00.1: pci_stop_and_remove_bus_device
    pci 0000:08:00.1: pci_release_dev                   # release PF1
    ixgbe 0000:08:00.0: pci_stop_and_remove_bus_device  # stop/remove PF0
                                                        # PF0 not released

When I turn off power to the slot, pciehp stops and removes all the devices, starting with the VFs. The PF 08:00.0 is stopped but not released, because we didn't call virtfn_remove(), so the reference held by the VFs was never released. This is a problem.

   # echo -n 1 > /sys/bus/pci/slots/4-1/power
    pci 0000:08:00.0: [8086:1528] type 00 class 0x020000
    pci 0000:08:00.1: [8086:1528] type 00 class 0x020000

When I turn the slot back on, we find both PFs; no apparent problem there.

On v3.10 (which does not contain dc087f2f6a), I see essentially the same thing. The PFs are stopped before the VFs because v3.10 doesn't contain 29ed1f29b68, which removes devices in reverse order. But even in v3.10, PF0 is stopped but not released because the VFs still hold references to it. And there's no complaint when turning the slot back on.

So we definitely have a problem: when we remove the PF, we don't release it correctly because we don't release the references held by VFs. But this is not a regression, and doesn't seem urgent enough to rush something for v3.11.

I think we're headed toward tearing down all VFs when removing a PF. Obviously, we *have* to do that when powering off the device, and I think we also need to do it when removing the PF driver just to keep all the bookkeeping sane. I don't know how much heartburn this will cause Don.

So tell me if you still think we need to do something for v3.11.

Quoted 66 lines · Bjorn Helgaas

[+cc Don]

Quoted 3 lines · Yinghai Lu

Quoted 1 line · Bjorn Helgaas

What would happen if we just reverted dc087f2f6a2 for now? Jiang?

I tried to reproduce the problem you reported ("Cannot add device at 0000:02:00"), but I haven't been able to. I have an ixgbe SR-IOV device in an external hotplug-capable chassis, and I'm using v3.11-rc4 with a patch to add a little instrumentation. Here's what I see:

    # echo -n 8 > /sys/bus/pci/devices/0000:08:00.0/sriov_numvfs
    ixgbe 0000:08:00.0 eth1: SR-IOV enabled with 8 VFs
    pci 0000:08:10.0: [8086:1515] type 00 class 0x020000        # VF1
    pci 0000:08:10.2: [8086:1515] type 00 class 0x020000        # VF2
    ...
    pci 0000:08:11.6: [8086:1515] type 00 class 0x020000        # VF8

Created 8 VFs; no problem there.

    # echo -n 0 > /sys/bus/pci/slots/4-1/power
    pciehp 0000:04:03.0:pcie24: disable_slot: physical_slot = 4-1
    pciehp 0000:04:03.0:pcie24: pciehp_unconfigure_device:
domain:bus:dev = 0000:08:00
    ixgbevf 0000:08:11.6: pci_stop_and_remove_bus_device
    pci 0000:08:11.6: pci_release_dev                   # release VF8
    ixgbevf 0000:08:11.4: pci_stop_and_remove_bus_device
    pci 0000:08:11.4: pci_release_dev                   # release VF7
    ...
    ixgbevf 0000:08:10.0: pci_stop_and_remove_bus_device
    pci 0000:08:10.0: pci_release_dev                   # release VF1
    ixgbe 0000:08:00.1: pci_stop_and_remove_bus_device
    pci 0000:08:00.1: pci_release_dev                   # release PF1
    ixgbe 0000:08:00.0: pci_stop_and_remove_bus_device  # stop/remove PF0
                                                        # PF0 not released

When I turn off power to the slot, pciehp stops and removes all the devices, starting with the VFs. The PF 08:00.0 is stopped but not released, because we didn't call virtfn_remove(), so the reference held by the VFs was never released. This is a problem.

   # echo -n 1 > /sys/bus/pci/slots/4-1/power
    pci 0000:08:00.0: [8086:1528] type 00 class 0x020000
    pci 0000:08:00.1: [8086:1528] type 00 class 0x020000

When I turn the slot back on, we find both PFs; no apparent problem there.

On v3.10 (which does not contain dc087f2f6a), I see essentially the same thing. The PFs are stopped before the VFs because v3.10 doesn't contain 29ed1f29b68, which removes devices in reverse order. But even in v3.10, PF0 is stopped but not released because the VFs still hold references to it. And there's no complaint when turning the slot back on.

So we definitely have a problem: when we remove the PF, we don't release it correctly because we don't release the references held by VFs. But this is not a regression, and doesn't seem urgent enough to rush something for v3.11.

I think we're headed toward tearing down all VFs when removing a PF. Obviously, we *have* to do that when powering off the device, and I think we also need to do it when removing the PF driver just to keep all the bookkeeping sane. I don't know how much heartburn this will cause Don.

Quoted 1 line

So tell me if you still think we need to do something for v3.11.

in your test: the PF is not get freed but it is removed for bus's device list. so pciehp_configure_device will create new one even the old is not really released.

Please check with attached patch, that only release that from link-list in pci_release device. I posted it a while back.
After it get applied, you will not have chance to create new device in pciehp_configure_device.

Subject: [PATCH 2/7] PCI: move resources and bus_list releasing to pci_release_dev
From: Yinghai Lu <[email protected]>

We should not release resource in pci_destroy that is too early as there could be still other use hold reference.

release them or remove it from bus devices list at last in pci_release_dev instead.

Signed-off-by Yinghai Lu <[email protected]>

---
 drivers/pci/probe.c  |   22 ++++++++++++++++++++--
 drivers/pci/remove.c |   19 -------------------
 2 files changed, 20 insertions(+), 21 deletions(-)

Index: linux-2.6/drivers/pci/probe.c
===================================================================

drivers/pci/probe.c +20 -2

@@ -1150,6 +1150,20 @@ static void pci_release_capabilities(str
 	pci_free_cap_save_buffers(dev);
 }
 
+static void pci_free_resources(struct pci_dev *dev)
+{
+	int i;
+
+	msi_remove_pci_irq_vectors(dev);
+
+	pci_cleanup_rom(dev);
+	for (i = 0; i < PCI_NUM_RESOURCES; i++) {
+		struct resource *res = dev->resource + i;
+		if (res->parent)
+			release_resource(res);
+	}
+}
+
 /**
  * pci_release_dev - free a pci device structure when all users of it are finished.
  * @dev: device that's been disconnected

@@ -1159,9 +1173,13 @@ static void pci_release_capabilities(str
  */
 static void pci_release_dev(struct device *dev)
 {
-	struct pci_dev *pci_dev;
+	struct pci_dev *pci_dev = to_pci_dev(dev);
+
+	down_write(&pci_bus_sem);
+	list_del(&pci_dev->bus_list);
+	up_write(&pci_bus_sem);
+	pci_free_resources(pci_dev);
 
-	pci_dev = to_pci_dev(dev);
 	pci_release_capabilities(pci_dev);
 	pci_release_of_node(pci_dev);
 	pcibios_release_device(pci_dev);

Index: linux-2.6/drivers/pci/remove.c
===================================================================

drivers/pci/remove.c +0 -19

@@ -3,20 +3,6 @@
 #include <linux/pci-aspm.h>
 #include "pci.h"
 
-static void pci_free_resources(struct pci_dev *dev)
-{
-	int i;
-
- 	msi_remove_pci_irq_vectors(dev);
-
-	pci_cleanup_rom(dev);
-	for (i = 0; i < PCI_NUM_RESOURCES; i++) {
-		struct resource *res = dev->resource + i;
-		if (res->parent)
-			release_resource(res);
-	}
-}
-
 static void pci_stop_dev(struct pci_dev *dev)
 {
 	pci_pme_active(dev, false);

@@ -36,11 +22,6 @@ static void pci_stop_dev(struct pci_dev
 
 static void pci_destroy_dev(struct pci_dev *dev)
 {
-	down_write(&pci_bus_sem);
-	list_del(&dev->bus_list);
-	up_write(&pci_bus_sem);
-
-	pci_free_resources(dev);
 	put_device(&dev->dev);
 }
 

Quoted 3 lines

in your test: the PF is not get freed but it is removed for bus's device list. so pciehp_configure_device will create new one even the old is not really released.

Yes. As I said above, I do see the leak of the PF pci_dev, and we certainly do need to fix that, but it's not a regression and doesn't seem urgent.

Quoted 4 lines

Please check with attached patch, that only release that from link-list in pci_release device. I posted it a while back.
After it get applied, you will not have chance to create new device in pciehp_configure_device.

[I inlined your patch for convenience]

I agree that the patch below looks like a good thing to do. However, it is not in v3.11. So if the problem only happens when the patch below is applied, we don't need to fix the problem in v3.11.

I've spent a LOT of time trying to reproduce this problem and convince myself that we need to do something in v3.11. If you're telling me that my time was wasted because the problem does not happen in v3.11-rc, but only happens with your out-of-tree patch, I'm going to be very upset.