pci,virtio: report surprise removal event

At the moment, in case of a surprise removal, the regular remove callback is invoked, exclusively. This works well, because mostly, the cleanup would be the same.

However, there's a race: imagine device removal was initiated by a user action, such as driver unbind, and it in turn initiated some cleanup and is now waiting for an interrupt from the device. If the device is now surprise-removed, that never arrives and the remove callback hangs forever.

For example, this was reported for virtio-blk:

1. the graceful removal is ongoing in the remove() callback, where disk deletion del_gendisk() is ongoing, which waits for the requests +to complete,

2. Now few requests are yet to complete, and surprise removal started.

At this point, virtio block driver will not get notified by the driver core layer, because it is likely serializing remove() happening by +user/driver unload and PCI hotplug driver-initiated device removal. So vblk driver doesn't know that device is removed, block layer is waiting for requests completions to arrive which it never gets. So del_gendisk() gets stuck.

Drivers can artificially add timeouts to handle that, but it can be flaky.

Instead, let's add a way for the driver to be notified about the disconnect. It can then do any necessary cleanup, knowing that the device is inactive.

Since cleanups can take a long time, this takes an approach of a work struct that the driver initiates and enables on probe, and tears down on remove.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 drivers/pci/pci.h   |  6 ++++++
 include/linux/pci.h | 45 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+)

drivers/pci/pci.h +6 -0

index 12215ee72afb..3ca4ebfd46be 100644
@@ -553,6 +553,12 @@ static inline int pci_dev_set_disconnected(struct pci_dev *dev, void *unused)
 	pci_dev_set_io_state(dev, pci_channel_io_perm_failure);
 	pci_doe_disconnected(dev);
 
+	if (READ_ONCE(dev->disconnect_work_enable)) {
+		/* Make sure work is up to date. */
+		smp_rmb();
+		schedule_work(&dev->disconnect_work);
+	}
+
 	return 0;
 }
 

include/linux/pci.h +45 -0

index 05e68f35f392..723b17145b62 100644
@@ -548,6 +548,10 @@ struct pci_dev {
 	/* These methods index pci_reset_fn_methods[] */
 	u8 reset_methods[PCI_NUM_RESET_METHODS]; /* In priority order */
 
+	/* Report disconnect events. 0x0 - disable, 0x1 - enable */
+	u8 disconnect_work_enable;
+	struct work_struct disconnect_work;
+
 #ifdef CONFIG_PCIE_TPH
 	u16		tph_cap;	/* TPH capability offset */
 	u8		tph_mode;	/* TPH mode */

@@ -1993,6 +1997,47 @@ pci_release_mem_regions(struct pci_dev *pdev)
 			    pci_select_bars(pdev, IORESOURCE_MEM));
 }
 
+/*
+ * Run this first thing after getting a disconnect work, to prevent it from
+ * running multiple times.
+ * Returns: true if disconnect was enabled, proceed. false if disabled, abort.
+ */
+static inline bool pci_test_and_clear_disconnect_enable(struct pci_dev *pdev)
+{
+	u8 enable = 0x1;
+	u8 disable = 0x0;
+	return try_cmpxchg(&pdev->disconnect_work_enable, &enable, disable);
+}
+
+/*
+ * Caller must initialize @pdev->disconnect_work before invoking this.
+ * The work function must run and check pci_test_and_clear_disconnect_enable.
+ * Note that device can go away right after this call.
+ */
+static inline void pci_set_disconnect_work(struct pci_dev *pdev)
+{
+	/* Make sure WQ has been initialized already */
+	smp_wmb();
+
+	WRITE_ONCE(pdev->disconnect_work_enable, 0x1);
+
+	/* check the device did not go away meanwhile. */
+	mb();
+
+	if (!pci_device_is_present(pdev))
+		schedule_work(&pdev->disconnect_work);
+}
+
+static inline void pci_clear_disconnect_work(struct pci_dev *pdev)
+{
+	WRITE_ONCE(pdev->disconnect_work_enable, 0x0);
+
+	/* Make sure to stop using work from now on. */
+	smp_wmb();
+
+	cancel_work_sync(&pdev->disconnect_work);
+}
+
 #else /* CONFIG_PCI is not enabled */
 
 static inline void pci_set_flags(int flags) { }

Signature · 1 line

MST

This relies on somebody (typically pciehp, I guess) calling pci_dev_set_disconnected() when a surprise remove happens.

Do you think it would be practical for the driver's .remove() method to recognize that the device may stop responding at any point, even if no hotplug driver is present to call pci_dev_set_disconnected()?

Waiting forever for an interrupt seems kind of vulnerable in general. Maybe "artificially adding timeouts" is alluding to *not* waiting forever for interrupts? That doesn't seem artificial to me because it's just a fact of life that devices can disappear at arbitrary times.

Quoted 7 lines · Bjorn Helgaas

This relies on somebody (typically pciehp, I guess) calling pci_dev_set_disconnected() when a surprise remove happens.

Do you think it would be practical for the driver's .remove() method to recognize that the device may stop responding at any point, even if no hotplug driver is present to call pci_dev_set_disconnected()?

Quoted 5 lines

Waiting forever for an interrupt seems kind of vulnerable in general. Maybe "artificially adding timeouts" is alluding to *not* waiting forever for interrupts? That doesn't seem artificial to me because it's just a fact of life that devices can disappear at arbitrary times.

I totally agree here. Every driver's .remove() should be able to guarantee forward progress some way. I put some work in blk-mq and nvme to ensure that happens for those devices at least.

That "forward progress" can come slow though, maybe minutes, so we do have opprotunisitic short cuts sprinkled about the driver. There are still gaps when waiting for interrupt driven IO that need the longer timeouts to trigger. It'd be cool if there was a mechansim to kick in quicker, but this is still an uncommon exceptional condition, right?

Housekeeping: Note subject line convention. Indent with spaces in commit log. Remove spurious plus signs.

Quoted 25 lines

At the moment, in case of a surprise removal, the regular remove callback is invoked, exclusively. This works well, because mostly, the cleanup would be the same.

However, there's a race: imagine device removal was initiated by a user action, such as driver unbind, and it in turn initiated some cleanup and is now waiting for an interrupt from the device. If the device is now surprise-removed, that never arrives and the remove callback hangs forever.

For example, this was reported for virtio-blk:

1. the graceful removal is ongoing in the remove() callback, where disk deletion del_gendisk() is ongoing, which waits for the requests +to complete,

2. Now few requests are yet to complete, and surprise removal started.

At this point, virtio block driver will not get notified by the driver core layer, because it is likely serializing remove() happening by +user/driver unload and PCI hotplug driver-initiated device removal. So vblk driver doesn't know that device is removed, block layer is waiting for requests completions to arrive which it never gets. So del_gendisk() gets stuck.

Quoted 6 lines

Drivers can artificially add timeouts to handle that, but it can be flaky.

Instead, let's add a way for the driver to be notified about the disconnect. It can then do any necessary cleanup, knowing that the device is inactive.

This relies on somebody (typically pciehp, I guess) calling pci_dev_set_disconnected() when a surprise remove happens.

Do you think it would be practical for the driver's .remove() method to recognize that the device may stop responding at any point, even if no hotplug driver is present to call pci_dev_set_disconnected()?

Waiting forever for an interrupt seems kind of vulnerable in general. Maybe "artificially adding timeouts" is alluding to *not* waiting forever for interrupts? That doesn't seem artificial to me because it's just a fact of life that devices can disappear at arbitrary times.

It seems a little fragile to me to depend on some other part of the system to notice the surprise removal and tell you about it or schedule your work function. I think it would be more robust for the driver to check directly, i.e., assume writes to the device may be lost, check for PCI_POSSIBLE_ERROR() after reads from the device, and never wait for an interrupt without a timeout.

Quoted 93 lines

Since cleanups can take a long time, this takes an approach of a work struct that the driver initiates and enables on probe, and tears down on remove.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 drivers/pci/pci.h   |  6 ++++++
 include/linux/pci.h | 45 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+)

drivers/pci/pci.h +6 -0

index 12215ee72afb..3ca4ebfd46be 100644
@@ -553,6 +553,12 @@ static inline int pci_dev_set_disconnected(struct pci_dev *dev, void *unused)
 	pci_dev_set_io_state(dev, pci_channel_io_perm_failure);
 	pci_doe_disconnected(dev);
 
+	if (READ_ONCE(dev->disconnect_work_enable)) {
+		/* Make sure work is up to date. */
+		smp_rmb();
+		schedule_work(&dev->disconnect_work);
+	}
+
 	return 0;
 }
 

include/linux/pci.h +45 -0

index 05e68f35f392..723b17145b62 100644
@@ -548,6 +548,10 @@ struct pci_dev {
 	/* These methods index pci_reset_fn_methods[] */
 	u8 reset_methods[PCI_NUM_RESET_METHODS]; /* In priority order */
 
+	/* Report disconnect events. 0x0 - disable, 0x1 - enable */
+	u8 disconnect_work_enable;
+	struct work_struct disconnect_work;
+
 #ifdef CONFIG_PCIE_TPH
 	u16		tph_cap;	/* TPH capability offset */
 	u8		tph_mode;	/* TPH mode */

@@ -1993,6 +1997,47 @@ pci_release_mem_regions(struct pci_dev *pdev)
 			    pci_select_bars(pdev, IORESOURCE_MEM));
 }
 
+/*
+ * Run this first thing after getting a disconnect work, to prevent it from
+ * running multiple times.
+ * Returns: true if disconnect was enabled, proceed. false if disabled, abort.
+ */
+static inline bool pci_test_and_clear_disconnect_enable(struct pci_dev *pdev)
+{
+	u8 enable = 0x1;
+	u8 disable = 0x0;
+	return try_cmpxchg(&pdev->disconnect_work_enable, &enable, disable);
+}
+
+/*
+ * Caller must initialize @pdev->disconnect_work before invoking this.
+ * The work function must run and check pci_test_and_clear_disconnect_enable.
+ * Note that device can go away right after this call.
+ */
+static inline void pci_set_disconnect_work(struct pci_dev *pdev)
+{
+	/* Make sure WQ has been initialized already */
+	smp_wmb();
+
+	WRITE_ONCE(pdev->disconnect_work_enable, 0x1);
+
+	/* check the device did not go away meanwhile. */
+	mb();
+
+	if (!pci_device_is_present(pdev))
+		schedule_work(&pdev->disconnect_work);
+}
+
+static inline void pci_clear_disconnect_work(struct pci_dev *pdev)
+{
+	WRITE_ONCE(pdev->disconnect_work_enable, 0x0);
+
+	/* Make sure to stop using work from now on. */
+	smp_wmb();
+
+	cancel_work_sync(&pdev->disconnect_work);
+}
+
 #else /* CONFIG_PCI is not enabled */
 
 static inline void pci_set_flags(int flags) { }

Signature · 1 line

MST

Quoted 46 lines · Bjorn Helgaas

Quoted 25 lines · Michael S. Tsirkin

At the moment, in case of a surprise removal, the regular remove callback is invoked, exclusively. This works well, because mostly, the cleanup would be the same.

However, there's a race: imagine device removal was initiated by a user action, such as driver unbind, and it in turn initiated some cleanup and is now waiting for an interrupt from the device. If the device is now surprise-removed, that never arrives and the remove callback hangs forever.

For example, this was reported for virtio-blk:

1. the graceful removal is ongoing in the remove() callback, where disk deletion del_gendisk() is ongoing, which waits for the requests +to complete,

2. Now few requests are yet to complete, and surprise removal started.

At this point, virtio block driver will not get notified by the driver core layer, because it is likely serializing remove() happening by +user/driver unload and PCI hotplug driver-initiated device removal. So vblk driver doesn't know that device is removed, block layer is waiting for requests completions to arrive which it never gets. So del_gendisk() gets stuck.

Quoted 6 lines

Drivers can artificially add timeouts to handle that, but it can be flaky.

Instead, let's add a way for the driver to be notified about the disconnect. It can then do any necessary cleanup, knowing that the device is inactive.

This relies on somebody (typically pciehp, I guess) calling pci_dev_set_disconnected() when a surprise remove happens.

Do you think it would be practical for the driver's .remove() method to recognize that the device may stop responding at any point, even if no hotplug driver is present to call pci_dev_set_disconnected()?

Waiting forever for an interrupt seems kind of vulnerable in general. Maybe "artificially adding timeouts" is alluding to *not* waiting forever for interrupts? That doesn't seem artificial to me because it's just a fact of life that devices can disappear at arbitrary times.

Quoted 6 lines

It seems a little fragile to me to depend on some other part of the system to notice the surprise removal and tell you about it or schedule your work function. I think it would be more robust for the driver to check directly, i.e., assume writes to the device may be lost, check for PCI_POSSIBLE_ERROR() after reads from the device, and never wait for an interrupt without a timeout.

virtio is ... kind of special, in that users already take it for granted that having a device as long as they want to respond still does not lead to errors and data loss.

Makes it a bit harder as our timeout would have to check for presence and retry, we can't just fail as a normal hardware device does.

And there's the overhead thing - poking at the device a lot puts a high load on the host.

Quoted 54 lines · Michael S. Tsirkin

Quoted 46 lines · Bjorn Helgaas

Quoted 25 lines · Michael S. Tsirkin

At the moment, in case of a surprise removal, the regular remove callback is invoked, exclusively. This works well, because mostly, the cleanup would be the same.

However, there's a race: imagine device removal was initiated by a user action, such as driver unbind, and it in turn initiated some cleanup and is now waiting for an interrupt from the device. If the device is now surprise-removed, that never arrives and the remove callback hangs forever.

For example, this was reported for virtio-blk:

1. the graceful removal is ongoing in the remove() callback, where disk deletion del_gendisk() is ongoing, which waits for the requests +to complete,

2. Now few requests are yet to complete, and surprise removal started.

At this point, virtio block driver will not get notified by the driver core layer, because it is likely serializing remove() happening by +user/driver unload and PCI hotplug driver-initiated device removal. So vblk driver doesn't know that device is removed, block layer is waiting for requests completions to arrive which it never gets. So del_gendisk() gets stuck.

Quoted 6 lines

Drivers can artificially add timeouts to handle that, but it can be flaky.

Instead, let's add a way for the driver to be notified about the disconnect. It can then do any necessary cleanup, knowing that the device is inactive.

This relies on somebody (typically pciehp, I guess) calling pci_dev_set_disconnected() when a surprise remove happens.

Do you think it would be practical for the driver's .remove() method to recognize that the device may stop responding at any point, even if no hotplug driver is present to call pci_dev_set_disconnected()?

Waiting forever for an interrupt seems kind of vulnerable in general. Maybe "artificially adding timeouts" is alluding to *not* waiting forever for interrupts? That doesn't seem artificial to me because it's just a fact of life that devices can disappear at arbitrary times.

Quoted 6 lines

It seems a little fragile to me to depend on some other part of the system to notice the surprise removal and tell you about it or schedule your work function. I think it would be more robust for the driver to check directly, i.e., assume writes to the device may be lost, check for PCI_POSSIBLE_ERROR() after reads from the device, and never wait for an interrupt without a timeout.

Quoted 7 lines

virtio is ... kind of special, in that users already take it for granted that having a device as long as they want to respond still does not lead to errors and data loss.

Makes it a bit harder as our timeout would have to check for presence and retry, we can't just fail as a normal hardware device does.

Sorry, I don't know enough about virtio to follow what you said about "having a device as long as they want to respond".

We started with a graceful remove. That must mean the user no longer needs the device.

Quoted 2 lines

And there's the overhead thing - poking at the device a lot puts a high load on the host.

Checking for PCI_POSSIBLE_ERROR() doesn't touch the device. If you did a config read already, and the result happened to be ~0, *then* we have the problem of figuring out whether the actual data from the device was ~0, or if the read failed and the Root Complex synthesized the ~0. In many cases a driver knows that ~0 is not a possible register value. Otherwise it might have to read another register that is known not to be ~0.

Bjorn

Quoted 64 lines · Bjorn Helgaas

Quoted 55 lines · Michael S. Tsirkin

Quoted 47 lines · Bjorn Helgaas

Quoted 26 lines · Michael S. Tsirkin

At the moment, in case of a surprise removal, the regular remove callback is invoked, exclusively. This works well, because mostly, the cleanup would be the same.

However, there's a race: imagine device removal was initiated by a user action, such as driver unbind, and it in turn initiated some cleanup and is now waiting for an interrupt from the device. If the device is now surprise-removed, that never arrives and the remove callback hangs forever.

For example, this was reported for virtio-blk:

1. the graceful removal is ongoing in the remove() callback, where disk deletion del_gendisk() is ongoing, which waits for the requests +to complete,

2. Now few requests are yet to complete, and surprise removal started.

At this point, virtio block driver will not get notified by the driver core layer, because it is likely serializing remove() happening by +user/driver unload and PCI hotplug driver-initiated device removal. So vblk driver doesn't know that device is removed, block layer is waiting for requests completions to arrive which it never gets. So del_gendisk() gets stuck.

Quoted 6 lines

Drivers can artificially add timeouts to handle that, but it can be flaky.

Instead, let's add a way for the driver to be notified about the disconnect. It can then do any necessary cleanup, knowing that the device is inactive.

This relies on somebody (typically pciehp, I guess) calling pci_dev_set_disconnected() when a surprise remove happens.

Do you think it would be practical for the driver's .remove() method to recognize that the device may stop responding at any point, even if no hotplug driver is present to call pci_dev_set_disconnected()?

Waiting forever for an interrupt seems kind of vulnerable in general. Maybe "artificially adding timeouts" is alluding to *not* waiting forever for interrupts? That doesn't seem artificial to me because it's just a fact of life that devices can disappear at arbitrary times.

Quoted 6 lines

It seems a little fragile to me to depend on some other part of the system to notice the surprise removal and tell you about it or schedule your work function. I think it would be more robust for the driver to check directly, i.e., assume writes to the device may be lost, check for PCI_POSSIBLE_ERROR() after reads from the device, and never wait for an interrupt without a timeout.

Quoted 7 lines

virtio is ... kind of special, in that users already take it for granted that having a device as long as they want to respond still does not lead to errors and data loss.

Makes it a bit harder as our timeout would have to check for presence and retry, we can't just fail as a normal hardware device does.

Quoted 5 lines

Sorry, I don't know enough about virtio to follow what you said about "having a device as long as they want to respond".

We started with a graceful remove. That must mean the user no longer needs the device.

I'll try to clarify:

Indeed, the user will not submit new requests,
but users might also not know that there are some old requests being in progress of being processed by the device.
The driver, currently, waits for that processsing to be complete. Cancelling that with a reset on a timeout might be a regression, unless the timeout is very long.

Quoted 10 lines

Quoted 2 lines

And there's the overhead thing - poking at the device a lot puts a high load on the host.

Checking for PCI_POSSIBLE_ERROR() doesn't touch the device. If you did a config read already, and the result happened to be ~0, *then* we have the problem of figuring out whether the actual data from the device was ~0, or if the read failed and the Root Complex synthesized the ~0. In many cases a driver knows that ~0 is not a possible register value. Otherwise it might have to read another register that is known not to be ~0.

To clarify, virtio generally is designed to operate solely by means of DMA and interrupt, completely avoiding any PCI reads. This, due to PCI reads being very expensive in virtualized scenarios.

The extra overhead I refer to is exactly initiating such a read where there would not be one in normal operation.

Quoted 71 lines · Michael S. Tsirkin

Quoted 64 lines · Bjorn Helgaas

Quoted 55 lines · Michael S. Tsirkin

Quoted 47 lines · Bjorn Helgaas

Quoted 26 lines · Michael S. Tsirkin

At the moment, in case of a surprise removal, the regular remove callback is invoked, exclusively. This works well, because mostly, the cleanup would be the same.

However, there's a race: imagine device removal was initiated by a user action, such as driver unbind, and it in turn initiated some cleanup and is now waiting for an interrupt from the device. If the device is now surprise-removed, that never arrives and the remove callback hangs forever.

For example, this was reported for virtio-blk:

1. the graceful removal is ongoing in the remove() callback, where disk deletion del_gendisk() is ongoing, which waits for the requests +to complete,

2. Now few requests are yet to complete, and surprise removal started.

At this point, virtio block driver will not get notified by the driver core layer, because it is likely serializing remove() happening by +user/driver unload and PCI hotplug driver-initiated device removal. So vblk driver doesn't know that device is removed, block layer is waiting for requests completions to arrive which it never gets. So del_gendisk() gets stuck.

Quoted 6 lines

Drivers can artificially add timeouts to handle that, but it can be flaky.

Instead, let's add a way for the driver to be notified about the disconnect. It can then do any necessary cleanup, knowing that the device is inactive.

This relies on somebody (typically pciehp, I guess) calling pci_dev_set_disconnected() when a surprise remove happens.

Do you think it would be practical for the driver's .remove() method to recognize that the device may stop responding at any point, even if no hotplug driver is present to call pci_dev_set_disconnected()?

Waiting forever for an interrupt seems kind of vulnerable in general. Maybe "artificially adding timeouts" is alluding to *not* waiting forever for interrupts? That doesn't seem artificial to me because it's just a fact of life that devices can disappear at arbitrary times.

Quoted 6 lines

It seems a little fragile to me to depend on some other part of the system to notice the surprise removal and tell you about it or schedule your work function. I think it would be more robust for the driver to check directly, i.e., assume writes to the device may be lost, check for PCI_POSSIBLE_ERROR() after reads from the device, and never wait for an interrupt without a timeout.

Quoted 7 lines

virtio is ... kind of special, in that users already take it for granted that having a device as long as they want to respond still does not lead to errors and data loss.

Makes it a bit harder as our timeout would have to check for presence and retry, we can't just fail as a normal hardware device does.

Quoted 5 lines

Sorry, I don't know enough about virtio to follow what you said about "having a device as long as they want to respond".

We started with a graceful remove. That must mean the user no longer needs the device.

Quoted 8 lines

I'll try to clarify:

Indeed, the user will not submit new requests,
but users might also not know that there are some old requests being in progress of being processed by the device.
The driver, currently, waits for that processsing to be complete. Cancelling that with a reset on a timeout might be a regression, unless the timeout is very long.

This seems like a corner case and maybe rare enough that simply making the timeout very long would be a possibility.

Quoted 11 lines

Quoted 10 lines

Quoted 2 lines

And there's the overhead thing - poking at the device a lot puts a high load on the host.

Checking for PCI_POSSIBLE_ERROR() doesn't touch the device. If you did a config read already, and the result happened to be ~0, *then* we have the problem of figuring out whether the actual data from the device was ~0, or if the read failed and the Root Complex synthesized the ~0. In many cases a driver knows that ~0 is not a possible register value. Otherwise it might have to read another register that is known not to be ~0.

Quoted 7 lines

To clarify, virtio generally is designed to operate solely by means of DMA and interrupt, completely avoiding any PCI reads. This, due to PCI reads being very expensive in virtualized scenarios.

The extra overhead I refer to is exactly initiating such a read where there would not be one in normal operation.

Thanks, this part is very helpful. And since config accesses are very expensive in *all* environments, I expect most drivers for high-performance devices work the same way and only do config accesses during at probe time.

If that's true, it will make this more understandable if the commit log approaches it from that direction and omits virtio specifics.

Bjorn

At the moment, in case of a surprise removal, the regular remove callback is invoked, exclusively. This works well, because mostly, the cleanup would be the same.

However, there's a race: imagine device removal was initiated by a user action, such as driver unbind, and it in turn initiated some cleanup and is now waiting for an interrupt from the device. If the device is now surprise-removed, that never arrives and the remove callback hangs forever.

Quoted 9 lines · Michael S. Tsirkin

At the moment, in case of a surprise removal, the regular remove callback is invoked, exclusively. This works well, because mostly, the cleanup would be the same.

However, there's a race: imagine device removal was initiated by a user action, such as driver unbind, and it in turn initiated some cleanup and is now waiting for an interrupt from the device. If the device is now surprise-removed, that never arrives and the remove callback hangs forever.

For PCI devices in a hotplug slot, user space can initiate "safe removal" by writing "0" to the hotplug slot's "power" file in sysfs.

If the PCI device is yanked from the slot while safe removal is ongoing, there is likewise no way for the driver to know that the device is suddenly gone. That's because pciehp_unconfigure_device() only calls pci_dev_set_disconnected() in the surprise removal case, not for safe removal.

The solution proposed here is thus not a complete one: It may work if user space initiated *driver* removal, but not if it initiated *safe* removal of the entire device. For virtio, that may be sufficient.

drivers/pci/pci.h static inline int pci_dev_set_disconnected(struct pci_dev *dev, void *unused) · line 563

Quoted 13 lines

+++ b/drivers/pci/pci.h

drivers/pci/pci.h +6 -0

@@ -553,6 +553,12 @@ static inline int pci_dev_set_disconnected(struct pci_dev *dev, void *unused)
 	pci_dev_set_io_state(dev, pci_channel_io_perm_failure);
 	pci_doe_disconnected(dev);
 
+	if (READ_ONCE(dev->disconnect_work_enable)) {
+		/* Make sure work is up to date. */
+		smp_rmb();
+		schedule_work(&dev->disconnect_work);
+	}
+
 	return 0;
 }

Going through all the callers of pci_dev_set_disconnected(), I suppose the (only) one you're interested in is pciehp_unconfigure_device().

The other callers are related to runtime resume, resume from system sleep and ACPI slots.

Instead of amending pci_dev_set_disconnected(), I'd prefer an approach where pciehp_unconfigure_device() first marks all devices disconnected, then wakes up some global waitqueue, e.g.:

(quoted diff) +1 -1

-	if (!presence)
+	if (!presence) {

		pci_walk_bus(parent, pci_dev_set_disconnected, NULL);

(quoted diff) +2 -0

+		wake_up_all(&pci_disconnected_wq);
+	}

The benefit is that there's no delay when marking devices disconnected. (Granted, the delay is small for smp_rmb() + schedule_work().) And just having a global waitqueue is simpler and may be useful for other use cases.

So instead of adding timeouts when waiting for interrupts, drivers would be woken via the waitqueue.

But again, it's not a complete solution as it doesn't cover the "surprise removal during safe removal" case.

I also agree with Bjorn's and Keith's comments that the driver should use timeouts for robustness, but still wanted to provide additional (hopefully constructive) thoughts.

Thanks!

Lukas

Quoted 9 lines · Michael S. Tsirkin

At the moment, in case of a surprise removal, the regular remove callback is invoked, exclusively. This works well, because mostly, the cleanup would be the same.

However, there's a race: imagine device removal was initiated by a user action, such as driver unbind, and it in turn initiated some cleanup and is now waiting for an interrupt from the device. If the device is now surprise-removed, that never arrives and the remove callback hangs forever.

For PCI devices in a hotplug slot, user space can initiate "safe removal" by writing "0" to the hotplug slot's "power" file in sysfs.

If the PCI device is yanked from the slot while safe removal is ongoing, there is likewise no way for the driver to know that the device is suddenly gone. That's because pciehp_unconfigure_device() only calls pci_dev_set_disconnected() in the surprise removal case, not for safe removal.

The solution proposed here is thus not a complete one: It may work if user space initiated *driver* removal, but not if it initiated *safe* removal of the entire device. For virtio, that may be sufficient.

drivers/pci/pci.h static inline int pci_dev_set_disconnected(struct pci_dev *dev, void *unused) · line 563

Quoted 13 lines

+++ b/drivers/pci/pci.h

drivers/pci/pci.h +6 -0

@@ -553,6 +553,12 @@ static inline int pci_dev_set_disconnected(struct pci_dev *dev, void *unused)
 	pci_dev_set_io_state(dev, pci_channel_io_perm_failure);
 	pci_doe_disconnected(dev);
 
+	if (READ_ONCE(dev->disconnect_work_enable)) {
+		/* Make sure work is up to date. */
+		smp_rmb();
+		schedule_work(&dev->disconnect_work);
+	}
+
 	return 0;
 }

Going through all the callers of pci_dev_set_disconnected(), I suppose the (only) one you're interested in is pciehp_unconfigure_device().

The other callers are related to runtime resume, resume from system sleep and ACPI slots.

Instead of amending pci_dev_set_disconnected(), I'd prefer an approach where pciehp_unconfigure_device() first marks all devices disconnected, then wakes up some global waitqueue, e.g.:

(quoted diff) +1 -1

-	if (!presence)
+	if (!presence) {

		pci_walk_bus(parent, pci_dev_set_disconnected, NULL);

(quoted diff) +2 -0

+		wake_up_all(&pci_disconnected_wq);
+	}

The benefit is that there's no delay when marking devices disconnected. (Granted, the delay is small for smp_rmb() + schedule_work().) And just having a global waitqueue is simpler and may be useful for other use cases.

So instead of adding timeouts when waiting for interrupts, drivers would be woken via the waitqueue.

But again, it's not a complete solution as it doesn't cover the "surprise removal during safe removal" case.

I also agree with Bjorn's and Keith's comments that the driver should use timeouts for robustness,

but still wanted to provide additional
(hopefully constructive) thoughts.

Thanks!

Lukas

Quoted 9 lines · Michael S. Tsirkin

At the moment, in case of a surprise removal, the regular remove callback is invoked, exclusively. This works well, because mostly, the cleanup would be the same.

However, there's a race: imagine device removal was initiated by a user action, such as driver unbind, and it in turn initiated some cleanup and is now waiting for an interrupt from the device. If the device is now surprise-removed, that never arrives and the remove callback hangs forever.

For PCI devices in a hotplug slot, user space can initiate "safe removal" by writing "0" to the hotplug slot's "power" file in sysfs.

If the PCI device is yanked from the slot while safe removal is ongoing, there is likewise no way for the driver to know that the device is suddenly gone. That's because pciehp_unconfigure_device() only calls pci_dev_set_disconnected() in the surprise removal case, not for safe removal.

The solution proposed here is thus not a complete one: It may work if user space initiated *driver* removal, but not if it initiated *safe* removal of the entire device. For virtio, that may be sufficient.

Quoted 20 lines · Lukas Wunner

Quoted 9 lines · Michael S. Tsirkin

At the moment, in case of a surprise removal, the regular remove callback is invoked, exclusively. This works well, because mostly, the cleanup would be the same.

However, there's a race: imagine device removal was initiated by a user action, such as driver unbind, and it in turn initiated some cleanup and is now waiting for an interrupt from the device. If the device is now surprise-removed, that never arrives and the remove callback hangs forever.

For PCI devices in a hotplug slot, user space can initiate "safe removal" by writing "0" to the hotplug slot's "power" file in sysfs.

If the PCI device is yanked from the slot while safe removal is ongoing, there is likewise no way for the driver to know that the device is suddenly gone. That's because pciehp_unconfigure_device() only calls pci_dev_set_disconnected() in the surprise removal case, not for safe removal.

Quoted 3 lines

The solution proposed here is thus not a complete one: It may work if user space initiated *driver* removal, but not if it initiated *safe* removal of the entire device. For virtio, that may be sufficient.

So just as an idea, something like this can work I guess? I'm yet to test this - wrote this on the go -

Quoted 21 lines · Michael S. Tsirkin

Quoted 20 lines · Lukas Wunner

Quoted 9 lines · Michael S. Tsirkin

At the moment, in case of a surprise removal, the regular remove callback is invoked, exclusively. This works well, because mostly, the cleanup would be the same.

However, there's a race: imagine device removal was initiated by a user action, such as driver unbind, and it in turn initiated some cleanup and is now waiting for an interrupt from the device. If the device is now surprise-removed, that never arrives and the remove callback hangs forever.

For PCI devices in a hotplug slot, user space can initiate "safe removal" by writing "0" to the hotplug slot's "power" file in sysfs.

If the PCI device is yanked from the slot while safe removal is ongoing, there is likewise no way for the driver to know that the device is suddenly gone. That's because pciehp_unconfigure_device() only calls pci_dev_set_disconnected() in the surprise removal case, not for safe removal.

Quoted 6 lines

Quoted 3 lines

The solution proposed here is thus not a complete one: It may work if user space initiated *driver* removal, but not if it initiated *safe* removal of the entire device. For virtio, that may be sufficient.

So just as an idea, something like this can work I guess? I'm yet to test this - wrote this on the go -

Don't bother, it won't work:

pciehp_handle_presence_or_link_change() is called from pciehp_ist(), the IRQ thread. During safe removal the IRQ thread is busy in pciehp_unconfigure_device() and waiting for the driver to unbind from devices being safe-removed.

An IRQ thread is always single-threaded. There's no second instance of the IRQ thread being run when another interrupt is signaled. Rather, the IRQ thread is re-run when it has finished.

In *theory* what would be possible is to plumb this into pciehp_isr(). That's the hardirq handler. This one will indeed be run when an interrupt comes in while the IRQ thread is running. Normally the hardirq handler would just collect the events for later consumption by the IRQ thread. The hardirq handler could *theoretically* mark devices gone while they're being safe-removed.

I'm saying "theoretically" because in reality I don't think this is a viable approach either: pciehp_ist() contains code to *ignore* link or presence changes if they were caused by a Secondary Bus Reset or Downstream Port Containment. In that case we do *not* want to mark devices disconnected because they're only *temporarily* inaccessible. This requires waiting for the SBR or DPC to conclude, which can take several seconds. We can't wait in the hardirq handler.

So this cannot be solved with the current architecture of pciehp, at least not easily or in an elegant way. Sorry!

Thanks,

Lukas

Quoted 4 lines · Lukas Wunner

pciehp_handle_presence_or_link_change() is called from pciehp_ist(), the IRQ thread. During safe removal the IRQ thread is busy in pciehp_unconfigure_device() and waiting for the driver to unbind from devices being safe-removed.

Confused. I thought safe removal happens in the userspace thread that wrote into sysfs?

Quoted 8 lines · Michael S. Tsirkin

Quoted 4 lines · Lukas Wunner

pciehp_handle_presence_or_link_change() is called from pciehp_ist(), the IRQ thread. During safe removal the IRQ thread is busy in pciehp_unconfigure_device() and waiting for the driver to unbind from devices being safe-removed.

Confused. I thought safe removal happens in the userspace thread that wrote into sysfs?

No, the userspace thread synthesizes a DISABLE_SLOT event, calls irq_wake_thread(), then waits for the IRQ thread to finish handling that event. See pciehp_sysfs_disable_slot().

Until 2018 we indeed brought down the slot in the userspace thread, but that required locking between the workqueue fed by the interrupt handler on the one hand and the userspace thread on the other hand. It was difficult to reason about the code.

We had bug reports about slots flapping the link or presence bits on slot bringdown that we could easily address by handling everything in the IRQ thread, see 3943af9d01e9. The same was reported for slot bringup and addressed by 6c35a1ac3da6.

This wouldn't have been possible with the architecture prior to 2018, at least not this easily.

Thanks,

Lukas

This series is based on top of MST's RFC v5 ([email protected]) and tries to address the architectural gap identified by Lukas Wunner in that thread.