[PATCH RFC] pci: report surprise removal events

[PATCH RFC] pci: report surprise removal events

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

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.

Given this is by design kind of asynchronous with normal probe/remove
callbacks, I added it in the pci_error_handlers callback.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

Warning: build-tested only at this point.

Posting for early flames/feedback.

Cc a bunch of people who discussed this problem specifically in the
virtio blk driver.

 drivers/pci/pci.h   | 9 +++++++++
 include/linux/pci.h | 3 +++
 2 files changed, 12 insertions(+)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index b81e99cd4b62..78b064be10d5 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -549,6 +549,15 @@ 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);
 
+	/* Notify driver of surprise removal */
+	device_lock(&dev->dev);
+
+	if (dev->driver && dev->driver->err_handler &&
+	    dev->driver->err_handler->disconnect)
+		dev->driver->err_handler->disconnect(dev);
+
+	device_unlock(&dev->dev);
+
 	return 0;
 }
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 51e2bd6405cd..30a8c7ee09f6 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -878,6 +878,9 @@ struct pci_error_handlers {
 	/* PCI slot has been reset */
 	pci_ers_result_t (*slot_reset)(struct pci_dev *dev);
 
+	/* PCI slot has been disconnected */
+        void (*disconnect)(struct pci_dev *dev);
+
 	/* PCI function reset prepare or completed */
 	void (*reset_prepare)(struct pci_dev *dev);
 	void (*reset_done)(struct pci_dev *dev);
-- 
MST

Re: [PATCH RFC] pci: report surprise removal events

[Lukas Wunner]

On Sat, Jun 28, 2025 at 02:58:49PM -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.
> 
> 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.
[...]
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -549,6 +549,15 @@ 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);
>  
> +	/* Notify driver of surprise removal */
> +	device_lock(&dev->dev);
> +
> +	if (dev->driver && dev->driver->err_handler &&
> +	    dev->driver->err_handler->disconnect)
> +		dev->driver->err_handler->disconnect(dev);
> +
> +	device_unlock(&dev->dev);
> +
>  	return 0;
>  }

No, that's not good:

1/ The device_lock() will reintroduce the issues solved by 74ff8864cc84.

2/ pci_dev_set_disconnected() needs to be fast so that devices are marked
   unplugged as quickly as possible.  We want to minimize the time window
   where MMIO and Config Space reads already return "all ones" and writes
   go to nirvana, but pci_dev_is_disconnected() still returns false.
   Hence invoking some driver callback which may take arbitrarily long or
   even sleeps is not an option.

The driver is already notified of removal through invocation of the
->remove() callback.  The use case you're describing is arguably
a corner case.  I do think that a timeout is a better approach
than the one proposed here.  How long does it take for the interrupt
to arrive?  If it's not just a few msec, consider polling the device
and breaking out of the pool loop as soon as pci_dev_is_disconnected()
returns true (or the MMIO read returns PCI_POSSIBLE_ERROR()).

If/when respinning, please explain the use case in more detail,
i.e. which driver, which device, pointers to code...

Thanks!

Lukas

Re: [PATCH RFC] pci: report surprise removal events

[Michael S. Tsirkin]

On Sun, Jun 29, 2025 at 03:36:27PM +0200, Lukas Wunner wrote:
> On Sat, Jun 28, 2025 at 02:58:49PM -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.
> > 
> > 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.
> [...]
> > --- a/drivers/pci/pci.h
> > +++ b/drivers/pci/pci.h
> > @@ -549,6 +549,15 @@ 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);
> >  
> > +	/* Notify driver of surprise removal */
> > +	device_lock(&dev->dev);
> > +
> > +	if (dev->driver && dev->driver->err_handler &&
> > +	    dev->driver->err_handler->disconnect)
> > +		dev->driver->err_handler->disconnect(dev);
> > +
> > +	device_unlock(&dev->dev);
> > +
> >  	return 0;
> >  }

thanks for the feedback. Would appreciate a couple more hints:

> No, that's not good:
> 
> 1/ The device_lock() will reintroduce the issues solved by 74ff8864cc84.

I see. What other way is there to prevent dev->driver from going away,
though? I guess I can add a new spinlock and take it both here and when
dev->driver changes? Acceptable?

> 2/ pci_dev_set_disconnected() needs to be fast so that devices are marked
>    unplugged as quickly as possible.  We want to minimize the time window
>    where MMIO and Config Space reads already return "all ones" and writes
>    go to nirvana, but pci_dev_is_disconnected() still returns false.
>    Hence invoking some driver callback which may take arbitrarily long or
>    even sleeps is not an option.

Well, there's no plan to do that there - just to wake up some wq so
things can be completed. I can add code comments.

> The driver is already notified of removal through invocation of the
> ->remove() callback.  The use case you're describing is arguably
> a corner case.  I do think that a timeout is a better approach
> than the one proposed here.  How long does it take for the interrupt
> to arrive?

It's a virtual device - kind of unpredictable.

>  If it's not just a few msec, consider polling the device
> and breaking out of the pool loop as soon as pci_dev_is_disconnected()
> returns true (or the MMIO read returns PCI_POSSIBLE_ERROR()).

Yes but with no callback, we don't know when to do it.
The config reads in pci_dev_is_disconnected are also expensive
on VMs...

> If/when respinning, please explain the use case in more detail,
> i.e. which driver, which device, pointers to code...
> 
> Thanks!
> 
> Lukas


It's virtio-blk. 

Re: [PATCH RFC] pci: report surprise removal events

[Keith Busch]

On Sun, Jun 29, 2025 at 01:28:08PM -0400, Michael S. Tsirkin wrote:
> On Sun, Jun 29, 2025 at 03:36:27PM +0200, Lukas Wunner wrote:
> > On Sat, Jun 28, 2025 at 02:58:49PM -0400, Michael S. Tsirkin wrote:
> > 
> > 1/ The device_lock() will reintroduce the issues solved by 74ff8864cc84.
> 
> I see. What other way is there to prevent dev->driver from going away,
> though? I guess I can add a new spinlock and take it both here and when
> dev->driver changes? Acceptable?

You're already holding the pci_bus_sem here, so the final device 'put'
can't have been called yet, so the device is valid and thread safe in
this context. I think maintaining the desired lifetime of the
instantiated driver is just a matter of reference counting within your
driver.

Just a thought on your patch, instead of introducing a new callback, you
could call the existing '->error_detected()' callback with the
previously set 'pci_channel_io_perm_failure' status. That would totally
work for nvme to kick its cleanup much quicker than the blk_mq timeout
handling we currently rely on for this scenario.

RE: [PATCH RFC] pci: report surprise removal events

[Parav Pandit]

> From: Keith Busch <kbusch@kernel.org>
> Sent: 30 June 2025 05:10 AM
> 
> On Sun, Jun 29, 2025 at 01:28:08PM -0400, Michael S. Tsirkin wrote:
> > On Sun, Jun 29, 2025 at 03:36:27PM +0200, Lukas Wunner wrote:
> > > On Sat, Jun 28, 2025 at 02:58:49PM -0400, Michael S. Tsirkin wrote:
> > >
> > > 1/ The device_lock() will reintroduce the issues solved by 74ff8864cc84.
> >
> > I see. What other way is there to prevent dev->driver from going away,
> > though? I guess I can add a new spinlock and take it both here and
> > when
> > dev->driver changes? Acceptable?
> 
> You're already holding the pci_bus_sem here, so the final device 'put'
> can't have been called yet, so the device is valid and thread safe in this
> context. I think maintaining the desired lifetime of the instantiated driver is
> just a matter of reference counting within your driver.
> 
> Just a thought on your patch, instead of introducing a new callback, you could
> call the existing '->error_detected()' callback with the previously set
> 'pci_channel_io_perm_failure' status. That would totally work for nvme to
> kick its cleanup much quicker than the blk_mq timeout handling we currently
> rely on for this scenario.

error_detected() callback is also called while holding the device_lock() by report_error_detected().
So when remove() callback is ongoing for graceful removal and driver is waiting for the request completions,

If the error_detected() will be stuck on device lock.

We likely need async notification from driver kernel core without holding the device lock, so driver can initiate cleanup in this corner case.

Re: [PATCH RFC] pci: report surprise removal events

[Michael S. Tsirkin]

On Sun, Jun 29, 2025 at 05:39:58PM -0600, Keith Busch wrote:
> On Sun, Jun 29, 2025 at 01:28:08PM -0400, Michael S. Tsirkin wrote:
> > On Sun, Jun 29, 2025 at 03:36:27PM +0200, Lukas Wunner wrote:
> > > On Sat, Jun 28, 2025 at 02:58:49PM -0400, Michael S. Tsirkin wrote:
> > > 
> > > 1/ The device_lock() will reintroduce the issues solved by 74ff8864cc84.
> > 
> > I see. What other way is there to prevent dev->driver from going away,
> > though? I guess I can add a new spinlock and take it both here and when
> > dev->driver changes? Acceptable?
> 
> You're already holding the pci_bus_sem here, so the final device 'put'
> can't have been called yet, so the device is valid and thread safe in
> this context. I think maintaining the desired lifetime of the
> instantiated driver is just a matter of reference counting within your
> driver.
> 
> Just a thought on your patch, instead of introducing a new callback, you
> could call the existing '->error_detected()' callback with the
> previously set 'pci_channel_io_perm_failure' status. That would totally
> work for nvme to kick its cleanup much quicker than the blk_mq timeout
> handling we currently rely on for this scenario.

That's even easier, sure. However, Lukas raised the issue that
pci_dev_set_disconnected must be fast, and drivers might do silly things
in their callbacks. So, I was working on adding ability to schedule work
on such an event, so prevent such misuse.

At the same time, it's somewhat hard to abstract it all away in
a driver independent manner, a callback is certainly easier.

WDYT?

-- 
MST

Re: [PATCH RFC] pci: report surprise removal events

[Keith Busch]

On Mon, Jun 30, 2025 at 04:07:55AM +0000, Parav Pandit wrote:
> 
> > From: Keith Busch <kbusch@kernel.org>
> > Sent: 30 June 2025 05:10 AM
> > 
> > On Sun, Jun 29, 2025 at 01:28:08PM -0400, Michael S. Tsirkin wrote:
> > > On Sun, Jun 29, 2025 at 03:36:27PM +0200, Lukas Wunner wrote:
> > > > On Sat, Jun 28, 2025 at 02:58:49PM -0400, Michael S. Tsirkin wrote:
> > > >
> > > > 1/ The device_lock() will reintroduce the issues solved by 74ff8864cc84.
> > >
> > > I see. What other way is there to prevent dev->driver from going away,
> > > though? I guess I can add a new spinlock and take it both here and
> > > when
> > > dev->driver changes? Acceptable?
> > 
> > You're already holding the pci_bus_sem here, so the final device 'put'
> > can't have been called yet, so the device is valid and thread safe in this
> > context. I think maintaining the desired lifetime of the instantiated driver is
> > just a matter of reference counting within your driver.
> > 
> > Just a thought on your patch, instead of introducing a new callback, you could
> > call the existing '->error_detected()' callback with the previously set
> > 'pci_channel_io_perm_failure' status. That would totally work for nvme to
> > kick its cleanup much quicker than the blk_mq timeout handling we currently
> > rely on for this scenario.
> 
> error_detected() callback is also called while holding the device_lock() by report_error_detected().
> So when remove() callback is ongoing for graceful removal and driver is waiting for the request completions,
> 
> If the error_detected() will be stuck on device lock.

But I didn't suggest calling error_detected from report_error_detected.
Just call it directly without device_lock. It's not very feasible to
enforce a non-blocking callback, though, if speed is really a concern
here.

RE: [PATCH RFC] pci: report surprise removal events

[Parav Pandit]

> From: Keith Busch <kbusch@kernel.org>
> Sent: 30 June 2025 07:14 PM
> 
> On Mon, Jun 30, 2025 at 04:07:55AM +0000, Parav Pandit wrote:
> >
> > > From: Keith Busch <kbusch@kernel.org>
> > > Sent: 30 June 2025 05:10 AM
> > >
> > > On Sun, Jun 29, 2025 at 01:28:08PM -0400, Michael S. Tsirkin wrote:
> > > > On Sun, Jun 29, 2025 at 03:36:27PM +0200, Lukas Wunner wrote:
> > > > > On Sat, Jun 28, 2025 at 02:58:49PM -0400, Michael S. Tsirkin wrote:
> > > > >
> > > > > 1/ The device_lock() will reintroduce the issues solved by
> 74ff8864cc84.
> > > >
> > > > I see. What other way is there to prevent dev->driver from going
> > > > away, though? I guess I can add a new spinlock and take it both
> > > > here and when
> > > > dev->driver changes? Acceptable?
> > >
> > > You're already holding the pci_bus_sem here, so the final device 'put'
> > > can't have been called yet, so the device is valid and thread safe
> > > in this context. I think maintaining the desired lifetime of the
> > > instantiated driver is just a matter of reference counting within your driver.
> > >
> > > Just a thought on your patch, instead of introducing a new callback,
> > > you could call the existing '->error_detected()' callback with the
> > > previously set 'pci_channel_io_perm_failure' status. That would
> > > totally work for nvme to kick its cleanup much quicker than the
> > > blk_mq timeout handling we currently rely on for this scenario.
> >
> > error_detected() callback is also called while holding the device_lock() by
> report_error_detected().
> > So when remove() callback is ongoing for graceful removal and driver
> > is waiting for the request completions,
> >
> > If the error_detected() will be stuck on device lock.
> 
> But I didn't suggest calling error_detected from report_error_detected.
> Just call it directly without device_lock. It's not very feasible to enforce a non-
> blocking callback, though, if speed is really a concern here.
Yeah, it would better to either always call a callback with or without the lock.
In some flows with lock and in some flows without lock would likely be
very bad as one cannot establish a sane locking order.

For time efficiency, large part of the processing is
happening in the individual driver itself as opposed to the core place.
So as long as individual driver can reliably know about error, and act swiftly, it should be good enough.

Re: [PATCH RFC] pci: report surprise removal events

[Keith Busch]

On Mon, Jun 30, 2025 at 01:52:26PM +0000, Parav Pandit wrote:
> > 
> > But I didn't suggest calling error_detected from report_error_detected.
> > Just call it directly without device_lock. It's not very feasible to enforce a non-
> > blocking callback, though, if speed is really a concern here.
> Yeah, it would better to either always call a callback with or without the lock.
> In some flows with lock and in some flows without lock would likely be
> very bad as one cannot establish a sane locking order.

On closer look, my suggestion without the device_lock may be racy, but
using the device_lock prevents the notification that needs to happen.
Hm, not as easy as I thought. :(

Re: [PATCH RFC] pci: report surprise removal events

[Michael S. Tsirkin]

On Mon, Jun 30, 2025 at 10:57:35AM -0600, Keith Busch wrote:
> On Mon, Jun 30, 2025 at 01:52:26PM +0000, Parav Pandit wrote:
> > > 
> > > But I didn't suggest calling error_detected from report_error_detected.
> > > Just call it directly without device_lock. It's not very feasible to enforce a non-
> > > blocking callback, though, if speed is really a concern here.
> > Yeah, it would better to either always call a callback with or without the lock.
> > In some flows with lock and in some flows without lock would likely be
> > very bad as one cannot establish a sane locking order.
> 
> On closer look, my suggestion without the device_lock may be racy, but
> using the device_lock prevents the notification that needs to happen.
> Hm, not as easy as I thought. :(

I think I will just add a work_struct and a flag that the driver can set
to schedule it on surprise removal then. Hmm?

-- 
MST