[PATCH RFC v5 1/5] pci: report surprise removal event

[PATCH RFC v5 1/5] pci: report surprise removal event

[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.

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 <mst@redhat.com>
---
 drivers/pci/pci.h   |  6 ++++++
 include/linux/pci.h | 45 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 12215ee72afb..3ca4ebfd46be 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -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;
 }
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 05e68f35f392..723b17145b62 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -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) { }
-- 
MST

Re: [PATCH RFC v5 1/5] pci: report surprise removal event

[Bjorn Helgaas]

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

On Wed, Jul 09, 2025 at 04:55:26PM -0400, Michael S. Tsirkin wrote:
> 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.

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.

> 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 <mst@redhat.com>
> ---
>  drivers/pci/pci.h   |  6 ++++++
>  include/linux/pci.h | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 51 insertions(+)
> 
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 12215ee72afb..3ca4ebfd46be 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -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;
>  }
>  
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 05e68f35f392..723b17145b62 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -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) { }
> -- 
> MST
> 

Re: [PATCH RFC v5 1/5] pci: report surprise removal event

[Keith Busch]

On Wed, Jul 09, 2025 at 06:38:20PM -0500, Bjorn Helgaas wrote:
> 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.

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?

Re: [PATCH RFC v5 1/5] pci: report surprise removal event

[Michael S. Tsirkin]

On Wed, Jul 09, 2025 at 05:55:17PM -0600, Keith Busch wrote:
> On Wed, Jul 09, 2025 at 06:38:20PM -0500, Bjorn Helgaas wrote:
> > 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.
> 
> 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?

It's uncommon, yes.

-- 
MST

Re: [PATCH RFC v5 1/5] pci: report surprise removal event

[Michael S. Tsirkin]

On Wed, Jul 09, 2025 at 06:38:20PM -0500, Bjorn Helgaas wrote:
> Housekeeping: Note subject line convention. Indent with spaces in
> commit log.  Remove spurious plus signs.


Thanks!

> On Wed, Jul 09, 2025 at 04:55:26PM -0400, Michael S. Tsirkin wrote:
> > 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.
> 
> 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.

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.

So I can imagine a very long timeout (minutes?), and then something like
the WQ I am trying to propose here as a shortcut.



> > 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 <mst@redhat.com>
> > ---
> >  drivers/pci/pci.h   |  6 ++++++
> >  include/linux/pci.h | 45 +++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 51 insertions(+)
> > 
> > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > index 12215ee72afb..3ca4ebfd46be 100644
> > --- a/drivers/pci/pci.h
> > +++ b/drivers/pci/pci.h
> > @@ -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;
> >  }
> >  
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 05e68f35f392..723b17145b62 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -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) { }
> > -- 
> > MST
> > 

Re: [PATCH RFC v5 1/5] pci: report surprise removal event

[Bjorn Helgaas]

On Mon, Jul 14, 2025 at 02:26:19AM -0400, Michael S. Tsirkin wrote:
> On Wed, Jul 09, 2025 at 06:38:20PM -0500, Bjorn Helgaas wrote:
> > On Wed, Jul 09, 2025 at 04:55:26PM -0400, Michael S. Tsirkin wrote:
> > > 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.
> > 
> > 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.
> 
> 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.

> 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

Re: [PATCH RFC v5 1/5] pci: report surprise removal event

[Michael S. Tsirkin]

On Mon, Jul 14, 2025 at 04:13:51PM -0500, Bjorn Helgaas wrote:
> On Mon, Jul 14, 2025 at 02:26:19AM -0400, Michael S. Tsirkin wrote:
> > On Wed, Jul 09, 2025 at 06:38:20PM -0500, Bjorn Helgaas wrote:
> > > On Wed, Jul 09, 2025 at 04:55:26PM -0400, Michael S. Tsirkin wrote:
> > > > 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.
> > > 
> > > 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.
> > 
> > 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.

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.

Hope this makes it clear.

> > 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

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.

-- 
MST

Re: [PATCH RFC v5 1/5] pci: report surprise removal event

[Bjorn Helgaas]

On Tue, Jul 15, 2025 at 02:28:20AM -0400, Michael S. Tsirkin wrote:
> On Mon, Jul 14, 2025 at 04:13:51PM -0500, Bjorn Helgaas wrote:
> > On Mon, Jul 14, 2025 at 02:26:19AM -0400, Michael S. Tsirkin wrote:
> > > On Wed, Jul 09, 2025 at 06:38:20PM -0500, Bjorn Helgaas wrote:
> > > > On Wed, Jul 09, 2025 at 04:55:26PM -0400, Michael S. Tsirkin wrote:
> > > > > 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.
> > > > 
> > > > 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.
> > > 
> > > 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.
> 
> 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.

> > > 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.

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

Re: [PATCH RFC v5 1/5] pci: report surprise removal event

[Michael S. Tsirkin]

On Wed, Jul 16, 2025 at 05:29:00PM -0500, Bjorn Helgaas wrote:
> On Tue, Jul 15, 2025 at 02:28:20AM -0400, Michael S. Tsirkin wrote:
> > On Mon, Jul 14, 2025 at 04:13:51PM -0500, Bjorn Helgaas wrote:
> > > On Mon, Jul 14, 2025 at 02:26:19AM -0400, Michael S. Tsirkin wrote:
> > > > On Wed, Jul 09, 2025 at 06:38:20PM -0500, Bjorn Helgaas wrote:
> > > > > On Wed, Jul 09, 2025 at 04:55:26PM -0400, Michael S. Tsirkin wrote:
> > > > > > 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.
> > > > > 
> > > > > 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.
> > > > 
> > > > 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.
> > 
> > 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.

Indeed the timeout needs to be very long, and the average would still be
reasonable, but the worst case is terrible and the user can't insert a
replacement card all this time. The system is perceived as flaky.


> > > > 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.
> 
> 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

Will do, thanks a lot!

-- 
MST

Re: [PATCH RFC v5 1/5] pci: report surprise removal event

[Lukas Wunner]

On Wed, Jul 09, 2025 at 04:55:26PM -0400, Michael S. Tsirkin wrote:
> 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.

> +++ b/drivers/pci/pci.h
> @@ -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.:

-	if (!presence)
+	if (!presence) {
		pci_walk_bus(parent, pci_dev_set_disconnected, NULL);
+		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

Re: [PATCH RFC v5 1/5] pci: report surprise removal event

[Michael S. Tsirkin]

On Mon, Jul 14, 2025 at 08:11:04AM +0200, Lukas Wunner wrote:
> On Wed, Jul 09, 2025 at 04:55:26PM -0400, Michael S. Tsirkin wrote:
> > 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.
> 
> > +++ b/drivers/pci/pci.h
> > @@ -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.:
> 
> -	if (!presence)
> +	if (!presence) {
> 		pci_walk_bus(parent, pci_dev_set_disconnected, NULL);
> +		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.

Did not realize. Will look into addressing this, thanks!


> 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

I'll address these comments in the next version.

-- 
MST

Re: [PATCH RFC v5 1/5] pci: report surprise removal event

[Michael S. Tsirkin]

On Mon, Jul 14, 2025 at 08:11:04AM +0200, Lukas Wunner wrote:
> On Wed, Jul 09, 2025 at 04:55:26PM -0400, Michael S. Tsirkin wrote:
> > 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.


No, I just missed this corner case.

> > +++ b/drivers/pci/pci.h
> > @@ -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.:
> 
> -	if (!presence)
> +	if (!presence) {
> 		pci_walk_bus(parent, pci_dev_set_disconnected, NULL);
> +		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,

Yes - we can consider this an optimization, as robust timeouts
are by necessity minutes.

> but still wanted to provide additional
> (hopefully constructive) thoughts.
> 
> Thanks!
> 
> Lukas

Re: [PATCH RFC v5 1/5] pci: report surprise removal event

[Michael S. Tsirkin]

On Mon, Jul 14, 2025 at 08:11:04AM +0200, Lukas Wunner wrote:
> On Wed, Jul 09, 2025 at 04:55:26PM -0400, Michael S. Tsirkin wrote:
> > 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.

So just as an idea, something like this can work I guess?  I'm yet to
test this - wrote this on the go - and also I'll need to implement for
other hotplug drivers, I need it at least for ACPI additonally.
WDYT?



diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index bcc938d4420f..46468a1f0244 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -231,6 +231,15 @@ void pciehp_handle_disable_request(struct controller *ctrl)
 void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events)
 {
 	int present, link_active;
+	/*
+	 * Always mark downstream devices disconnected on Presence Detect Change.
+	 * Covers device yanked during safe removal.
+	 */
+	if (events & PCI_EXP_SLTSTA_PDC) {
+		struct pci_bus *parent = ctrl->pcie->port->subordinate;
+		if (parent)
+			pci_walk_bus(parent, pci_dev_set_disconnected, NULL);
+	}
 
 	/*
 	 * If the slot is on and presence or link has changed, turn it off.

Re: [PATCH RFC v5 1/5] pci: report surprise removal event

[Lukas Wunner]

On Thu, Jul 17, 2025 at 11:11:44AM -0400, Michael S. Tsirkin wrote:
> On Mon, Jul 14, 2025 at 08:11:04AM +0200, Lukas Wunner wrote:
> > On Wed, Jul 09, 2025 at 04:55:26PM -0400, Michael S. Tsirkin wrote:
> > > 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.
> 
> 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

Re: [PATCH RFC v5 1/5] pci: report surprise removal event

[Michael S. Tsirkin]

On Thu, Jul 17, 2025 at 10:12:03PM +0200, Lukas Wunner wrote:
> On Thu, Jul 17, 2025 at 11:11:44AM -0400, Michael S. Tsirkin wrote:
> > On Mon, Jul 14, 2025 at 08:11:04AM +0200, Lukas Wunner wrote:
> > > On Wed, Jul 09, 2025 at 04:55:26PM -0400, Michael S. Tsirkin wrote:
> > > > 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.
> > 
> > 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.

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

> 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

Re: [PATCH RFC v5 1/5] pci: report surprise removal event

[Lukas Wunner]

On Thu, Jul 17, 2025 at 07:31:57PM -0400, Michael S. Tsirkin wrote:
> On Thu, Jul 17, 2025 at 10:12:03PM +0200, Lukas Wunner wrote:
> > 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

Re: [PATCH RFC v5 1/5] pci: report surprise removal event

[Michael S. Tsirkin]

On Fri, Jul 18, 2025 at 06:35:56AM +0200, Lukas Wunner wrote:
> On Thu, Jul 17, 2025 at 07:31:57PM -0400, Michael S. Tsirkin wrote:
> > On Thu, Jul 17, 2025 at 10:12:03PM +0200, Lukas Wunner wrote:
> > > 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

Got it, thanks!

-- 
MST

[PATCH RFC 0/3] pci_hp: fix surprise removal hang during safe removal

[Abhin Parekadan Jose]

This series is based on top of MST's RFC v5
(cover.1752094439.git.mst@redhat.com) and tries to address the architectural
gap identified by Lukas Wunner in that thread.

Context:

MST's RFC v5 adds disconnect_work infrastructure so drivers can be
notified of surprise removal. Lukas identified a race that the
series cannot address: if safe removal is already in progress when the
device is yanked, pciehp_ist() is blocked and cannot deliver the
disconnect event.

Potential fix:

I reproduced this with a simple edu device driver (patch 1) that blocks
in remove() waiting for an interrupt. Same as blk_mq_freeze_queue_wait().

deadlock in pciehp_ist() is avoided by adding a workaround in pciehp_isr() (patch 3)
where a disconnect_work is scheduled and as this is a different thread it runs and
dispatches the disconnect event to the driver. The driver can then unblock and complete
the remove() and thus breaking the deadlock and also overcoming the problem of
not waiting in pciehp_isr().This is only schduled if the PDS is set to 0
indicating that there is no card attached at this slot in pciehp_isr().

Tested using qemu:
- Hacked the edu device to raise a delayed interrupt.
- Hacked qemu to actually act like suprise removal by adding a simple monitor cmd
  `pcie_surprise_del` to remove the device and genrate PDC=1, DLLSC = 1 and PDS=0.
1. Launched qemu with the edu device:
    `-device pcie-root-port,id=rp1,chassis=1,slot=1   -device edu,bus=rp1,id=edu0`
2. Ran `echo 1 > /sys/bus/pci/devices/0000:01:00.0/remove` to start safe removal
3. Ran `pcie_surprise_del edu0` to surprise remove the device while safe removal
    is in progress.
4. Verified that `edu_remove()` and `edu_disconnect()` are called.

Without the fix: remove() hangs permanently.
With the fix: pciehp_isr() fires, pciehp_disconnect_work() schedules
pci_dev_set_disconnected(), edu_disconnect() completes the wait, remove()
proceeds.

Would this be a viable approach?

Assisted-by: Claude:claude-sonnet-4-6

Abhin Parekadan Jose (3):
  misc: add edu_srpoc surprise removal POC driver
  pciehp: add disconnect_work work_struct
  pciehp_hpc: workaround to not wait in pciehp_isr on surprise removal

 drivers/misc/Makefile            |   1 +
 drivers/misc/edu_srpoc.c         | 180 +++++++++++++++++++++++++++++++
 drivers/pci/hotplug/pciehp.h     |   1 +
 drivers/pci/hotplug/pciehp_hpc.c |  47 ++++++++
 4 files changed, 229 insertions(+)
 create mode 100644 drivers/misc/edu_srpoc.c

--
2.51.1

[PATCH RFC 1/3] misc: add edu_srpoc surprise removal POC driver

[Abhin Parekadan Jose]

From: Abhin Parekadan Jose <abhin@poc.local>

A test driver for the QEMU edu device that reproduces the surprise
removal hang described in MST's RFC v5 thread.

- hacked in a reg to the edu device on qemu to raise a delayed irq
- This driver writes to that reg in remove and waits for the irq to be
  handled. This simulate simulating del_gendisk() blocked in blk_mq_freeze_queue_wait()

Assisted-by: Claude:claude-sonnet-4-6
Signed-off-by: Abhin Parekadan Jose <abhinjoses@gmail.com>
---
 drivers/misc/Makefile    |   1 +
 drivers/misc/edu_srpoc.c | 180 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 181 insertions(+)
 create mode 100644 drivers/misc/edu_srpoc.c

diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index fed47c7672b9..36da0539c215 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_AD525X_DPOT_I2C)	+= ad525x_dpot-i2c.o
 obj-$(CONFIG_AD525X_DPOT_SPI)	+= ad525x_dpot-spi.o
 obj-$(CONFIG_ATMEL_SSC)		+= atmel-ssc.o
 obj-$(CONFIG_DUMMY_IRQ)		+= dummy-irq.o
+obj-y				+= edu_srpoc.o
 obj-$(CONFIG_ICS932S401)	+= ics932s401.o
 obj-$(CONFIG_LKDTM)		+= lkdtm/
 obj-$(CONFIG_TI_FPC202)		+= ti_fpc202.o
diff --git a/drivers/misc/edu_srpoc.c b/drivers/misc/edu_srpoc.c
new file mode 100644
index 000000000000..ce2c254e3362
--- /dev/null
+++ b/drivers/misc/edu_srpoc.c
@@ -0,0 +1,180 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * edu_srpoc.c — Surprise Removal POC driver for the QEMU edu device
+ *
+ * In remove(), schedules a delayed interrupt on the edu device and
+ * blocks waiting for it to complete. This simulates del_gendisk()
+ * blocked in blk_mq_freeze_queue_wait() on slow in-flight I/O.
+ *
+ * Surprise-remove the device during this window to reproduce the hang.
+ *
+ * edu BAR 0 registers used:
+ *   0x08  Factorial: write N to compute N! asynchronously
+ *   0x20  Status: write EDU_STATUS_IRQFACT to enable IRQ on completion
+ *   0x24  IRQ status: bit 0 = FACT_IRQ, bit 9 = DELAY_IRQ
+ *   0x30  Delayed IRQ: write N (ms). Hacked in this functionality(not upstream).
+ *   0x64  IRQ lower: write bitmask to ack
+ */
+
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/interrupt.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
+
+#define PCI_VENDOR_ID_EDU	0x1234
+#define PCI_DEVICE_ID_EDU	0x11e8
+
+#define EDU_REG_FACT		0x08
+#define EDU_REG_STATUS		0x20
+#define EDU_REG_DELAYED_IRQ	0x30
+#define EDU_REG_IRQ_STATUS	0x24
+#define EDU_REG_IRQ_LOWER	0x64
+
+#define EDU_STATUS_IRQFACT	0x80
+#define EDU_FACT_IRQ		BIT(0)
+#define EDU_DELAY_IRQ		BIT(9)
+
+/* Large enough to take several seconds in the QEMU thread */
+#define EDU_SLOW_FACTORIAL	0xffffffff
+
+struct edu_dev {
+	struct pci_dev		*pdev;
+	void __iomem		*regs;
+	struct completion	irq_done;
+};
+
+static irqreturn_t edu_irq_handler(int irq, void *data)
+{
+	struct edu_dev *edu = data;
+	u32 status;
+
+	status = ioread32(edu->regs + EDU_REG_IRQ_STATUS);
+	if (!status)
+		return IRQ_NONE;
+
+	iowrite32(status, edu->regs + EDU_REG_IRQ_LOWER);
+
+	if (status & (EDU_FACT_IRQ | EDU_DELAY_IRQ))
+	{
+		pr_info("complete(&edu->irq_done)\n");
+		complete(&edu->irq_done);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static void edu_disconnect(struct work_struct *work)
+{
+	struct pci_dev *pdev = container_of(work, struct pci_dev,
+					    disconnect_work);
+	struct edu_dev *edu = pci_get_drvdata(pdev);
+
+	dev_info(&pdev->dev, "edu_disconnect()\n");
+	if (!pci_test_and_clear_disconnect_enable(pdev))
+		return;
+
+	if (!edu)
+		return;
+
+	dev_info(&pdev->dev, "disconnect_work fired — unblocking remove()\n");
+	complete(&edu->irq_done);
+}
+
+static int edu_probe(struct pci_dev *pdev, const struct pci_device_id *id)
+{
+	struct edu_dev *edu;
+	int err;
+
+	dev_info(&pdev->dev, "edu_probe(): starting\n");
+
+	edu = devm_kzalloc(&pdev->dev, sizeof(*edu), GFP_KERNEL);
+	if (!edu)
+		return -ENOMEM;
+
+	edu->pdev = pdev;
+	init_completion(&edu->irq_done);
+
+	err = pci_enable_device(pdev);
+	if (err)
+		return err;
+
+	err = pci_request_regions(pdev, "edu_srpoc");
+	if (err)
+		goto err_disable;
+
+	edu->regs = pci_iomap(pdev, 0, 0);
+	if (!edu->regs) {
+		err = -ENOMEM;
+		goto err_release;
+	}
+
+	pci_set_master(pdev);
+
+	err = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSI | PCI_IRQ_INTX);
+	if (err < 0)
+		goto err_iounmap;
+
+	err = request_irq(pci_irq_vector(pdev, 0), edu_irq_handler,
+			  IRQF_SHARED, "edu_srpoc", edu);
+	if (err)
+		goto err_free_vectors;
+
+	pci_set_drvdata(pdev, edu);
+
+	INIT_WORK(&pdev->disconnect_work, edu_disconnect);
+	pci_set_disconnect_work(pdev);
+
+	dev_info(&pdev->dev, "edu_srpoc probed\n");
+	return 0;
+
+err_free_vectors:
+	pci_free_irq_vectors(pdev);
+err_iounmap:
+	pci_iounmap(pdev, edu->regs);
+err_release:
+	pci_release_regions(pdev);
+err_disable:
+	pci_disable_device(pdev);
+	return err;
+}
+
+static void edu_remove(struct pci_dev *pdev)
+{
+	struct edu_dev *edu = pci_get_drvdata(pdev);
+
+	dev_info(&pdev->dev, "remove(): starting factorial — blocking for IRQ\n");
+
+	iowrite32(EDU_STATUS_IRQFACT, edu->regs + EDU_REG_STATUS);
+
+	iowrite32(600000, edu->regs + EDU_REG_DELAYED_IRQ);
+
+	wait_for_completion(&edu->irq_done);
+
+	dev_info(&pdev->dev, "remove(): unblocked, cleaning up\n");
+
+	pci_clear_disconnect_work(pdev);
+	free_irq(pci_irq_vector(pdev, 0), edu);
+	pci_free_irq_vectors(pdev);
+	pci_iounmap(pdev, edu->regs);
+	pci_release_regions(pdev);
+	pci_disable_device(pdev);
+}
+
+static const struct pci_device_id edu_ids[] = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_EDU, PCI_DEVICE_ID_EDU) },
+	{ 0 }
+};
+MODULE_DEVICE_TABLE(pci, edu_ids);
+
+static struct pci_driver edu_driver = {
+	.name		= "edu_srpoc",
+	.id_table	= edu_ids,
+	.probe		= edu_probe,
+	.remove		= edu_remove,
+};
+
+module_pci_driver(edu_driver);
+MODULE_AUTHOR("Abhin Parekadan Jose");
+MODULE_DESCRIPTION("edu surprise removal POC driver");
+MODULE_LICENSE("GPL");
-- 
2.51.1

Re: [PATCH RFC 1/3] misc: add edu_srpoc surprise removal POC driver

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The shared IRQ handler does not correctly handle the `0xffffffff` register read value on surprise removal, leading to spurious `IRQ_HANDLED` returns and false completions.
- [Medium] Missing explicit `cancel_work_sync()` for `disconnect_work` in `remove()` leads to a Use-After-Free of the `edu` structure due to a TOCTOU race.
- [Low] The driver references a non-existent `disconnect_work` member in `struct pci_dev` and calls non-existent PCI functions, causing immediate build failures.
--

--- Patch [1]: [PATCH RFC 1/3] misc: add edu_srpoc surprise removal POC driver ---
commit 1e01da4d29945cd7e9e2a5dc7d46075c3af6e84b
Author: Abhin Parekadan Jose <abhin@poc.local>

misc: add edu_srpoc surprise removal POC driver

This patch adds a test driver for the QEMU edu device to reproduce a surprise
removal hang. It registers a PCI driver, requests regions, and sets up
interrupts and a work queue.

> diff --git a/drivers/misc/edu_srpoc.c b/drivers/misc/edu_srpoc.c
> new file mode 100644
> index 0000000000000..ce2c254e3362f
> --- /dev/null
> +++ b/drivers/misc/edu_srpoc.c
[ ... ]
> +static irqreturn_t edu_irq_handler(int irq, void *data)
> +{
> +	struct edu_dev *edu = data;
> +	u32 status;
> +
> +	status = ioread32(edu->regs + EDU_REG_IRQ_STATUS);
> +	if (!status)
> +		return IRQ_NONE;
> +
> +	iowrite32(status, edu->regs + EDU_REG_IRQ_LOWER);
> +
> +	if (status & (EDU_FACT_IRQ | EDU_DELAY_IRQ))
> +	{

[Severity: High]
When the PCI device is surprise-removed, MMIO reads (like ioread32())
typically return all-ones (0xffffffff). 

Since this is a shared interrupt handler, 0xffffffff will pass the !status
check and the status & (EDU_FACT_IRQ | EDU_DELAY_IRQ) check. 

Will this cause the handler to spuriously complete the wait and return
IRQ_HANDLED, stealing interrupts from other devices sharing the line?

> +		pr_info("complete(&edu->irq_done)\n");
> +		complete(&edu->irq_done);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static void edu_disconnect(struct work_struct *work)
> +{
> +	struct pci_dev *pdev = container_of(work, struct pci_dev,
> +					    disconnect_work);

[Severity: Low]
Is disconnect_work a valid member of struct pci_dev? 

It appears this member does not exist in the PCI subsystem, which would cause
the module to fail to compile.

> +	struct edu_dev *edu = pci_get_drvdata(pdev);
> +
> +	dev_info(&pdev->dev, "edu_disconnect()\n");
> +	if (!pci_test_and_clear_disconnect_enable(pdev))
> +		return;
> +
> +	if (!edu)
> +		return;
> +
> +	dev_info(&pdev->dev, "disconnect_work fired — unblocking remove()\n");
> +	complete(&edu->irq_done);

[Severity: Medium]
If the work function executes concurrently with edu_remove(), could it read
the edu pointer, pass the if (!edu) check, and get preempted before calling
complete()? 

Since edu_remove() does not synchronously cancel the work (e.g. using 
cancel_work_sync()), and edu is allocated with devm_kzalloc(), edu could be 
freed by devres when remove() returns.

Could this lead to a use-after-free on edu->irq_done here?

> +}
> +
> +static int edu_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
[ ... ]
> +	INIT_WORK(&pdev->disconnect_work, edu_disconnect);
> +	pci_set_disconnect_work(pdev);

[Severity: Low]
Does pci_set_disconnect_work() exist in the kernel? 

Calling this undefined function will cause build errors.

> +
> +	dev_info(&pdev->dev, "edu_srpoc probed\n");
> +	return 0;
[ ... ]
> +static void edu_remove(struct pci_dev *pdev)
> +{
> +	struct edu_dev *edu = pci_get_drvdata(pdev);
> +
> +	dev_info(&pdev->dev, "remove(): starting factorial — blocking for IRQ\n");
> +
> +	iowrite32(EDU_STATUS_IRQFACT, edu->regs + EDU_REG_STATUS);
> +
> +	iowrite32(600000, edu->regs + EDU_REG_DELAYED_IRQ);
> +
> +	wait_for_completion(&edu->irq_done);
> +
> +	dev_info(&pdev->dev, "remove(): unblocked, cleaning up\n");
> +
> +	pci_clear_disconnect_work(pdev);

[Severity: Low]
Does pci_clear_disconnect_work() exist in the kernel? 

Calling this undefined function will cause build errors.

> +	free_irq(pci_irq_vector(pdev, 0), edu);
> +	pci_free_irq_vectors(pdev);
> +	pci_iounmap(pdev, edu->regs);
> +	pci_release_regions(pdev);
> +	pci_disable_device(pdev);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260823183458.982699-1-abhinjoses@gmail.com?part=1

[PATCH RFC 2/3] pciehp: add disconnect_work work_struct

[Abhin Parekadan Jose]

- This work_struct is used to dispatch disconnect events directly from the pciehp_isr, as we cannot wait in pciehp_isr

Signed-off-by: Abhin Parekadan Jose <abhinjoses@gmail.com>
---
 drivers/pci/hotplug/pciehp.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
index debc79b0adfb..c8ceb9320e2e 100644
--- a/drivers/pci/hotplug/pciehp.h
+++ b/drivers/pci/hotplug/pciehp.h
@@ -116,6 +116,7 @@ struct controller {
 	unsigned int ist_running;
 	int request_result;
 	wait_queue_head_t requester;
+	struct work_struct disconnect_work;
 };
 
 /**
-- 
2.51.1

Re: [PATCH RFC 2/3] pciehp: add disconnect_work work_struct

[sashiko-bot]

> - This work_struct is used to dispatch disconnect events directly from the pciehp_isr, as we cannot wait in pciehp_isr
> 
> Signed-off-by: Abhin Parekadan Jose <abhinjoses@gmail.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260823183458.982699-1-abhinjoses@gmail.com?part=2

[PATCH RFC 3/3] pciehp_hpc: workaround to not wait in pciehp_isr on surprise removal

[Abhin Parekadan Jose]

- Add pciehp_disconnect_work() , this function is used using schedule_work from pciehp_isr allowing us to not wait in pciehp_isr()
- pciehp_disconnect_work() is scheduled only if the card is not detected (PDS bit = 0).
       The PDS bit can be used to differentiate between a safe and surprise removal.

Signed-off-by: Abhin Parekadan Jose <abhinjoses@gmail.com>
---
 drivers/pci/hotplug/pciehp_hpc.c | 47 ++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 4c62140a3cb4..69ed6b060e9d 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -620,6 +620,40 @@ static void pciehp_ignore_link_change(struct controller *ctrl,
 	up_read(&ctrl->reset_lock);
 }
 
+/*
+ * Workaround to not wait in the isr.
+ */
+static void pciehp_disconnect_work(struct work_struct *work)
+{
+	struct controller *ctrl = container_of(work, struct controller,
+					       disconnect_work);
+	struct pci_dev *pdev = ctrl_dev(ctrl);
+	int events;
+
+	events = atomic_read(&ctrl->pending_events);
+
+	/*
+	 * Ignore Link Down/Up events caused by Downstream Port Containment
+	 * if recovery succeeded, or caused by Secondary Bus Reset,
+	 * suspend to D3cold, firmware update, FPGA reconfiguration, etc.
+	 */
+	if ((events & (PCI_EXP_SLTSTA_PDC | PCI_EXP_SLTSTA_DLLSC)) &&
+	    (pci_dpc_recovered(pdev) || pci_hp_spurious_link_change(pdev)) &&
+	    ctrl->state == ON_STATE) {
+		// Ignore the link change events and return to normal operation.
+		// Could also wait here if needed.
+		return;
+	}
+
+	struct pci_bus *bus = ctrl->pcie->port->subordinate;
+
+	/* the card may have returned. */
+	if (!bus || pciehp_card_present(ctrl) != 0)
+		return;
+
+	pci_walk_bus(bus, pci_dev_set_disconnected, NULL);
+}
+
 static irqreturn_t pciehp_isr(int irq, void *dev_id)
 {
 	struct controller *ctrl = (struct controller *)dev_id;
@@ -722,6 +756,17 @@ static irqreturn_t pciehp_isr(int irq, void *dev_id)
 
 	/* Save pending events for consumption by IRQ thread. */
 	atomic_or(events, &ctrl->pending_events);
+
+	// presence change events
+	if (events & (PCI_EXP_SLTSTA_PDC | PCI_EXP_SLTSTA_DLLSC)) {
+		// Get PDS bit to determine if card is present or not
+		int present = pciehp_card_present(ctrl);
+		if (!present) { // for sure the card is not present
+			schedule_work(&ctrl->disconnect_work);
+			// After this we are no longer in isr and can wait.
+		}
+	}
+
 	return IRQ_WAKE_THREAD;
 }
 
@@ -1036,6 +1081,7 @@ struct controller *pcie_init(struct pcie_device *dev)
 	init_waitqueue_head(&ctrl->requester);
 	init_waitqueue_head(&ctrl->queue);
 	INIT_DELAYED_WORK(&ctrl->button_work, pciehp_queue_pushbutton_work);
+	INIT_WORK(&ctrl->disconnect_work, pciehp_disconnect_work);
 	dbg_ctrl(ctrl);
 
 	down_read(&pci_bus_sem);
@@ -1096,6 +1142,7 @@ struct controller *pcie_init(struct pcie_device *dev)
 void pciehp_release_ctrl(struct controller *ctrl)
 {
 	cancel_delayed_work_sync(&ctrl->button_work);
+	cancel_work_sync(&ctrl->disconnect_work);
 	kfree(ctrl);
 }
 
-- 
2.51.1

Re: [PATCH RFC 3/3] pciehp_hpc: workaround to not wait in pciehp_isr on surprise removal

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Concurrent calls to `pci_hp_spurious_link_change()` corrupt state flags, breaking hot-plug recovery logic.
- [High] Lockless read of `pending_events` races with `atomic_xchg`, bypassing spurious link checks and causing premature disconnections.
- [Medium] Incorrect truthiness evaluation of `pciehp_card_present()` breaks the disconnect workaround during severe surprise removals.
--

--- Patch [3]: [PATCH RFC 3/3] pciehp_hpc: workaround to not wait in pciehp_isr on surprise removal ---
Note: The format of this review has been altered to provide a free-form
summary of the findings due to automated recitation restrictions. The original
patch diff is not quoted inline.

commit df7f3f7b5143de6a32373ccd864f5ca514636a79
Author: Abhin Parekadan Jose <abhinjoses@gmail.com>
pciehp_hpc: workaround to not wait in pciehp_isr on surprise removal

This patch adds a workqueue to handle PCIe hotplug device disconnects without
waiting in the interrupt service routine. It checks the presence detect state
bit to differentiate between safe and surprise removals, scheduling the
disconnect work if the card is no longer present.

[Severity: High]
Could the concurrent evaluation of pci_hp_spurious_link_change() in the new
disconnect workqueue corrupt state flags? The function
pci_hp_spurious_link_change() is stateful and clears the PCI_LINK_CHANGED bit
using test_and_clear_bit(). If the asynchronous workqueue destructively
consumes this flag before the primary hot-plug IRQ thread evaluates it, won't
the core handler erroneously tear down the device tree instead of safely
ignoring a transient link flap?

[Severity: High]
Does the lockless read of pending_events in pciehp_disconnect_work() race
with the IRQ thread? The workqueue reads ctrl->pending_events locklessly.
However, the threaded IRQ handler pciehp_ist() unconditionally clears these
events using atomic_xchg(&ctrl->pending_events, 0). If the workqueue executes
just after pciehp_ist() clears the events, it will observe a value of 0.
Wouldn't this cause the workqueue to completely bypass the spurious link down
safety checks and prematurely disconnect all devices on the bus?

[Severity: Medium]
Will the truthiness checks for pciehp_card_present() fail during an actual
hardware surprise removal? When a device is abruptly removed, configuration
space reads typically return 0xffff, causing pciehp_card_present() to return
-ENODEV. Because -ENODEV is a truthy value in C, the condition (!present) in
the interrupt handler will evaluate to false, meaning the disconnect work is
never scheduled. Furthermore, the check inside the workqueue evaluates
pciehp_card_present(ctrl) != 0, which is true for -ENODEV, causing an early
return. Does this logic inadvertently disable the workaround precisely when a
surprise removal happens?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260823183458.982699-1-abhinjoses@gmail.com?part=3