nvme/pci: Use async_schedule for initial reset work

This patch schedules the initial controller reset in an async_domain so that it can be synchronized from wait_for_device_probe(). This way the kernel waits for the first nvme controller initialization to complete for all devices before proceeding with the boot sequence, which may have nvme dependencies.

Reported-by Mikulas Patocka <mpatocka at redhat.com>

Signed-off-by Keith Busch <keith.busch at intel.com>

---
 drivers/nvme/host/pci.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

drivers/nvme/host/pci.c +7 -0

index a2f3ad105620..9e04c7aba946 100644
@@ -13,6 +13,7 @@
  */
 
 #include <linux/aer.h>
+#include <linux/async.h>
 #include <linux/blkdev.h>
 #include <linux/blk-mq.h>
 #include <linux/blk-mq-pci.h>

@@ -2489,6 +2490,12 @@ static unsigned long check_vendor_combination_bug(struct pci_dev *pdev)
        return 0;
 }
 
+static void nvme_async_probe(void *data, async_cookie_t cookie)
+{
+       struct nvme_dev *dev = data;
+       nvme_reset_ctrl_sync(&dev->ctrl);
+}
+
 static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
        int node, result = -ENOMEM;

Looks fine,

Reviewed-by Ming Lei <ming.lei at redhat.com>

Quoted 3 lines · Ming Lei

Looks fine,

Reviewed-by Ming Lei <ming.lei at redhat.com>

Thanks, Ming.

Mikulas, would you be able to test this and confirm it works for you? This appears successful in my testing, but want to hear from the source if possible.

Thanks,
Keith

Quoted 5 lines · Keith Busch

Quoted 3 lines · Ming Lei

Looks fine,

Reviewed-by Ming Lei <ming.lei at redhat.com>

Quoted 8 lines

Thanks, Ming.

Mikulas, would you be able to test this and confirm it works for you? This appears successful in my testing, but want to hear from the source if possible.

Thanks,
Keith

The patch is not correct - scan_work is still called from a workqueue and if it's too slow, the nvme device is not found when mounting root.

You can add msleep(10000) at the beginning of nvme_scan_work to test for the race condition on your system.

Here I submit the corrected patch - I added flush_work(&dev->ctrl.scan_work) to nvme_async_probe

Quoted 19 lines · Mikulas Patocka

Quoted 5 lines · Keith Busch

Quoted 3 lines · Ming Lei

Looks fine,

Reviewed-by Ming Lei <ming.lei at redhat.com>

Quoted 8 lines

Thanks, Ming.

Mikulas, would you be able to test this and confirm it works for you? This appears successful in my testing, but want to hear from the source if possible.

Thanks,
Keith

The patch is not correct - scan_work is still called from a workqueue and if it's too slow, the nvme device is not found when mounting root.

Quoted 5 lines

You can add msleep(10000) at the beginning of nvme_scan_work to test for the race condition on your system.

Here I submit the corrected patch - I added flush_work(&dev->ctrl.scan_work) to nvme_async_probe

Roger that. Will incorporate your adjustment and add your Tested-by. Thanks for the confirmation.

I think there is still one more bug:

If nvme_probe is called, it schedules the asynchronous work using async_schedule - now suppose that the pci system calls the "remove", "shutdown" or "suspend" method - this method will race with nvme_async_probe running in the async domain - that will cause misbehavior.

Or - does the PCI subsystem flush the async queues before calling these methods? I'm not sure, but it doesn't seem so.

I think, you need to save the cookie returned by async_schedule and wait for this cookie with async_synchronize_cookie in the other methods.

Quoted 8 lines · Mikulas Patocka

I think there is still one more bug:

If nvme_probe is called, it schedules the asynchronous work using async_schedule - now suppose that the pci system calls the "remove", "shutdown" or "suspend" method - this method will race with nvme_async_probe running in the async domain - that will cause misbehavior.

Quoted 5 lines

Or - does the PCI subsystem flush the async queues before calling these methods? I'm not sure, but it doesn't seem so.

I think, you need to save the cookie returned by async_schedule and wait for this cookie with async_synchronize_cookie in the other methods.

I think we're fine as-is without syncing the cookie.

The remove path should be fine since we already sync with the necessary work queues.

The shutdown, suspend and reset paths will just cause the initial reset work to end early, the same result as what previously would happen.

Suppose this:
task 1: nvme_probe
task 1: calls async_schedule(nvme_async_probe), that queues the work for
task 2
task 1: exits (so the device is active from pci subsystem's point of view) task 3: the pci subsystem calls nvme_remove
task 3: calls nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_DELETING); task 3: cancel_work_sync(&dev->ctrl.reset_work); (does nothing because the
work item hasn't started yet)
task 3: nvme_remove does all the remaining work
task 3: frees the device
task 3: exists nvme_remove
task 2: (in the async domain) runs nvme_async_probe
task 2: calls nvme_reset_ctrl_sync
task 2: nvme_reset_ctrl
task 2: calls nvme_change_ctrl_state and queue_work - on a structure that
was already freed by nvme_remove

This bug is rare - but it may happen if the user too quickly activates and deactivates the device by writing to sysfs.