nvme: avoid race-conditions when enabling devices

RFC PATCH In review Marta Rybczynska 5 participants 10 messages Mar 21, 2018 – Mar 23, 2018

Subsystems (2)

PCI NVMe

lore

Share & tools

lore

Marta Rybczynska

author

2018-03-21 11:00 UTC #0 lore raw

RFC PATCH 1/1 nvme: avoid race-conditions when enabling devices

NVMe driver uses threads for the work at device reset, including enabling the PCIe device. When multiple NVMe devices are initialized, their reset works may be scheduled in parallel. Then pci_enable_device_mem can be called in parallel on multiple cores.

This causes a loop of enabling of all upstream bridges in pci_enable_bridge(). pci_enable_bridge() causes multiple operations including __pci_set_master and architecture-specific functions that call ones like and pci_enable_resources(). Both __pci_set_master() and pci_enable_resources() read PCI_COMMAND field in the PCIe space and change it. This is done as read/modify/write.

Imagine that the PCIe tree looks like:
A - B - switch - C - D

               \- E - F

D and F are two NVMe disks and all devices from B are not enabled and bus mastering is not set. If their reset work are scheduled in parallel the two modifications of PCI_COMMAND may happen in parallel without locking and the system may end up with the part of PCIe tree not enabled.

The problem may also happen if other device is initialized in parallel to a nvme disk.

This fix moves pci_enable_device_mem to the probe part of the driver that is run sequentially to avoid the issue.

Signed-off-by Marta Rybczynska <marta.rybczynska at kalray.eu>

Signed-off-by Pierre-Yves Kerbrat <pkerbrat at kalray.eu>

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

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

index b6f43b7..af53854 100644
@@ -2515,6 +2515,14 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	dev_info(dev->ctrl.device, "pci function %s\n", dev_name(&pdev->dev));
 
+	/*
+	 * Enable the device now to make sure that all accesses to bridges above
+	 * are done without races
+	 */
+	result = pci_enable_device_mem(pdev);
+	if (result)
+		goto release_pools;
+
 	nvme_reset_ctrl(&dev->ctrl);
 
 	return 0;

Signature · 1 line

1.8.3.1

Ming Lei

2018-03-21 11:50 UTC #1 lore raw

Quoted 12 lines · Marta Rybczynska

NVMe driver uses threads for the work at device reset, including enabling the PCIe device. When multiple NVMe devices are initialized, their reset works may be scheduled in parallel. Then pci_enable_device_mem can be called in parallel on multiple cores.
This causes a loop of enabling of all upstream bridges in pci_enable_bridge(). pci_enable_bridge() causes multiple operations including __pci_set_master and architecture-specific functions that call ones like and pci_enable_resources(). Both __pci_set_master() and pci_enable_resources() read PCI_COMMAND field in the PCIe space and change it. This is done as read/modify/write.

Quoted 8 lines

Imagine that the PCIe tree looks like:
A - B - switch - C - D
               \- E - F
D and F are two NVMe disks and all devices from B are not enabled and bus mastering is not set. If their reset work are scheduled in parallel the two modifications of PCI_COMMAND may happen in parallel without locking and the system may end up with the part of PCIe tree not enabled.

Then looks serialized reset should be used, and I did see the commit 79c48ccf2fe ("nvme-pci: serialize pci resets") fixes issue of 'failed to mark controller state' in reset stress test.

But that commit only covers case of PCI reset from sysfs attribute, and maybe other cases need to be dealt with in similar way too.

Thanks,
Ming

Marta Rybczynska

author

2018-03-21 12:10 UTC #2 lore raw

Quoted 22 lines · Marta Rybczynska

Quoted 12 lines
NVMe driver uses threads for the work at device reset, including enabling the PCIe device. When multiple NVMe devices are initialized, their reset works may be scheduled in parallel. Then pci_enable_device_mem can be called in parallel on multiple cores.
This causes a loop of enabling of all upstream bridges in pci_enable_bridge(). pci_enable_bridge() causes multiple operations including __pci_set_master and architecture-specific functions that call ones like and pci_enable_resources(). Both __pci_set_master() and pci_enable_resources() read PCI_COMMAND field in the PCIe space and change it. This is done as read/modify/write.
Quoted 8 lines
Imagine that the PCIe tree looks like:
A - B - switch - C - D
               \- E - F
D and F are two NVMe disks and all devices from B are not enabled and bus mastering is not set. If their reset work are scheduled in parallel the two modifications of PCI_COMMAND may happen in parallel without locking and the system may end up with the part of PCIe tree not enabled.

Quoted 7 lines

Then looks serialized reset should be used, and I did see the commit 79c48ccf2fe ("nvme-pci: serialize pci resets") fixes issue of 'failed to mark controller state' in reset stress test.
But that commit only covers case of PCI reset from sysfs attribute, and maybe other cases need to be dealt with in similar way too.

It seems to me that the serialized reset works for multiple resets of the same device, doesn't it? Our problem is linked to resets of different devices that share the same PCIe tree.

You're right that the problem we face might also come with manual resets under certain conditions (I think that all devices in a subtree would need to be disabled).

Thanks,
Marta

Ming Lei

2018-03-21 15:48 UTC #3 lore raw

Quoted 26 lines · Marta Rybczynska

Quoted 26 lines · Marta Rybczynska
Quoted 12 lines
NVMe driver uses threads for the work at device reset, including enabling the PCIe device. When multiple NVMe devices are initialized, their reset works may be scheduled in parallel. Then pci_enable_device_mem can be called in parallel on multiple cores.
This causes a loop of enabling of all upstream bridges in pci_enable_bridge(). pci_enable_bridge() causes multiple operations including __pci_set_master and architecture-specific functions that call ones like and pci_enable_resources(). Both __pci_set_master() and pci_enable_resources() read PCI_COMMAND field in the PCIe space and change it. This is done as read/modify/write.
Quoted 8 lines
Imagine that the PCIe tree looks like:
A - B - switch - C - D
               \- E - F
D and F are two NVMe disks and all devices from B are not enabled and bus mastering is not set. If their reset work are scheduled in parallel the two modifications of PCI_COMMAND may happen in parallel without locking and the system may end up with the part of PCIe tree not enabled.
Then looks serialized reset should be used, and I did see the commit 79c48ccf2fe ("nvme-pci: serialize pci resets") fixes issue of 'failed to mark controller state' in reset stress test.

Quoted 7 lines

Quoted 3 lines
But that commit only covers case of PCI reset from sysfs attribute, and maybe other cases need to be dealt with in similar way too.
It seems to me that the serialized reset works for multiple resets of the same device, doesn't it? Our problem is linked to resets of different devices that share the same PCIe tree.

Given reset shouldn't be a frequent action, it might be fine to serialize all reset from different devices.

Thanks,
Ming

Keith Busch

2018-03-21 16:00 UTC #4 lore raw

Quoted 31 lines · Ming Lei

Quoted 30 lines · Marta Rybczynska
Quoted 29 lines · Marta Rybczynska
Quoted 12 lines
NVMe driver uses threads for the work at device reset, including enabling the PCIe device. When multiple NVMe devices are initialized, their reset works may be scheduled in parallel. Then pci_enable_device_mem can be called in parallel on multiple cores.
This causes a loop of enabling of all upstream bridges in pci_enable_bridge(). pci_enable_bridge() causes multiple operations including __pci_set_master and architecture-specific functions that call ones like and pci_enable_resources(). Both __pci_set_master() and pci_enable_resources() read PCI_COMMAND field in the PCIe space and change it. This is done as read/modify/write.
Quoted 8 lines
Imagine that the PCIe tree looks like:
A - B - switch - C - D
               \- E - F
D and F are two NVMe disks and all devices from B are not enabled and bus mastering is not set. If their reset work are scheduled in parallel the two modifications of PCI_COMMAND may happen in parallel without locking and the system may end up with the part of PCIe tree not enabled.
Then looks serialized reset should be used, and I did see the commit 79c48ccf2fe ("nvme-pci: serialize pci resets") fixes issue of 'failed to mark controller state' in reset stress test.
But that commit only covers case of PCI reset from sysfs attribute, and maybe other cases need to be dealt with in similar way too.

Quoted 6 lines

Quoted 3 lines
It seems to me that the serialized reset works for multiple resets of the same device, doesn't it? Our problem is linked to resets of different devices that share the same PCIe tree.
Given reset shouldn't be a frequent action, it might be fine to serialize all reset from different devices.

The driver was much simpler when we had serialized resets in line with probe, but that had a bigger problems with certain init systems when you put enough nvme devices in your server, making them unbootable.

Would it be okay to serialize just the pci_enable_device across all other tasks messing with the PCI topology?

---

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

index cef5ce851a92..e0a2f6c0f1cf 100644
@@ -2094,8 +2094,11 @@ static int nvme_pci_enable(struct nvme_dev *dev)

	int result = -ENOMEM;
	struct pci_dev *pdev = to_pci_dev(dev->dev);

- if (pci_enable_device_mem(pdev))
- return result;
+ pci_lock_rescan_remove();
+ result = pci_enable_device_mem(pdev);
+ pci_unlock_rescan_remove();
+ if (result)
+ return -ENODEV;

	pci_set_master(pdev);

Marta Rybczynska

author

2018-03-21 16:10 UTC #5 lore raw

Quoted 55 lines · Ming Lei

Quoted 31 lines · Marta Rybczynska
Quoted 30 lines
Quoted 29 lines · Marta Rybczynska
Quoted 12 lines
NVMe driver uses threads for the work at device reset, including enabling the PCIe device. When multiple NVMe devices are initialized, their reset works may be scheduled in parallel. Then pci_enable_device_mem can be called in parallel on multiple cores.
This causes a loop of enabling of all upstream bridges in pci_enable_bridge(). pci_enable_bridge() causes multiple operations including __pci_set_master and architecture-specific functions that call ones like and pci_enable_resources(). Both __pci_set_master() and pci_enable_resources() read PCI_COMMAND field in the PCIe space and change it. This is done as read/modify/write.
Quoted 8 lines
Imagine that the PCIe tree looks like:
A - B - switch - C - D
               \- E - F
D and F are two NVMe disks and all devices from B are not enabled and bus mastering is not set. If their reset work are scheduled in parallel the two modifications of PCI_COMMAND may happen in parallel without locking and the system may end up with the part of PCIe tree not enabled.
Then looks serialized reset should be used, and I did see the commit 79c48ccf2fe ("nvme-pci: serialize pci resets") fixes issue of 'failed to mark controller state' in reset stress test.
But that commit only covers case of PCI reset from sysfs attribute, and maybe other cases need to be dealt with in similar way too.
Quoted 6 lines
Quoted 3 lines
It seems to me that the serialized reset works for multiple resets of the same device, doesn't it? Our problem is linked to resets of different devices that share the same PCIe tree.
Given reset shouldn't be a frequent action, it might be fine to serialize all reset from different devices.
The driver was much simpler when we had serialized resets in line with probe, but that had a bigger problems with certain init systems when you put enough nvme devices in your server, making them unbootable.
Would it be okay to serialize just the pci_enable_device across all other tasks messing with the PCI topology?
---
drivers/nvme/host/pci.c +0 -0
index cef5ce851a92..e0a2f6c0f1cf 100644
@@ -2094,8 +2094,11 @@ static int nvme_pci_enable(struct nvme_dev *dev)
 	int result = -ENOMEM;
 	struct pci_dev *pdev = to_pci_dev(dev->dev);
 

drivers/nvme/host/pci.c static int nvme_pci_enable(struct nvme_dev *dev) · line 2104

Quoted 10 lines

drivers/nvme/host/pci.c +5 -2

-	if (pci_enable_device_mem(pdev))
-		return result;
+	pci_lock_rescan_remove();
+	result = pci_enable_device_mem(pdev);
+	pci_unlock_rescan_remove();
+	if (result)
+		return -ENODEV;
 
 	pci_set_master(pdev);

The problem may happen also with other device doing its probe and nvme running its workqueue (and we probably have seen it in practice too). We were thinking about a lock in the pci generic code too, that's why I've put the linux-pci@ list in copy.

Marta

Bjorn Helgaas

2018-03-21 21:53 UTC #6 lore raw

[+cc Srinath]

Quoted 63 lines · Marta Rybczynska

Quoted 63 lines · Ming Lei
Quoted 31 lines · Marta Rybczynska
Quoted 30 lines
Quoted 29 lines · Marta Rybczynska
Quoted 12 lines
NVMe driver uses threads for the work at device reset, including enabling the PCIe device. When multiple NVMe devices are initialized, their reset works may be scheduled in parallel. Then pci_enable_device_mem can be called in parallel on multiple cores.
This causes a loop of enabling of all upstream bridges in pci_enable_bridge(). pci_enable_bridge() causes multiple operations including __pci_set_master and architecture-specific functions that call ones like and pci_enable_resources(). Both __pci_set_master() and pci_enable_resources() read PCI_COMMAND field in the PCIe space and change it. This is done as read/modify/write.
Quoted 8 lines
Imagine that the PCIe tree looks like:
A - B - switch - C - D
               \- E - F
D and F are two NVMe disks and all devices from B are not enabled and bus mastering is not set. If their reset work are scheduled in parallel the two modifications of PCI_COMMAND may happen in parallel without locking and the system may end up with the part of PCIe tree not enabled.
Then looks serialized reset should be used, and I did see the commit 79c48ccf2fe ("nvme-pci: serialize pci resets") fixes issue of 'failed to mark controller state' in reset stress test.
But that commit only covers case of PCI reset from sysfs attribute, and maybe other cases need to be dealt with in similar way too.
Quoted 6 lines
Quoted 3 lines
It seems to me that the serialized reset works for multiple resets of the same device, doesn't it? Our problem is linked to resets of different devices that share the same PCIe tree.
Given reset shouldn't be a frequent action, it might be fine to serialize all reset from different devices.
The driver was much simpler when we had serialized resets in line with probe, but that had a bigger problems with certain init systems when you put enough nvme devices in your server, making them unbootable.
Would it be okay to serialize just the pci_enable_device across all other tasks messing with the PCI topology?
---
drivers/nvme/host/pci.c +5 -2
index cef5ce851a92..e0a2f6c0f1cf 100644
@@ -2094,8 +2094,11 @@ static int nvme_pci_enable(struct nvme_dev *dev)
 	int result = -ENOMEM;
 	struct pci_dev *pdev = to_pci_dev(dev->dev);
 
-	if (pci_enable_device_mem(pdev))
-		return result;
+	pci_lock_rescan_remove();
+	result = pci_enable_device_mem(pdev);
+	pci_unlock_rescan_remove();
+	if (result)
+		return -ENODEV;
 

Quoted 6 lines

drivers/nvme/host/pci.c static int nvme_pci_enable(struct nvme_dev *dev) · line 2103
Quoted 1 line
drivers/nvme/host/pci.c +0 -0
 	pci_set_master(pdev);
The problem may happen also with other device doing its probe and nvme running its workqueue (and we probably have seen it in practice too). We were thinking about a lock in the pci generic code too, that's why I've put the linux-pci@ list in copy.

Yes, this is a generic problem in the PCI core. We've tried to fix it in the past but haven't figured it out yet.

See 40f11adc7cd9 ("PCI: Avoid race while enabling upstream bridges") and 0f50a49e3008 ("Revert "PCI: Avoid race while enabling upstream bridges"").

It's not trivial, but if you figure out a good way to fix this, I'd be thrilled.

Bjorn

Marta Rybczynska

author

2018-03-23 07:28 UTC #7 lore raw

Quoted 11 lines · Marta Rybczynska

Quoted 5 lines
The problem may happen also with other device doing its probe and nvme running its workqueue (and we probably have seen it in practice too). We were thinking about a lock in the pci generic code too, that's why I've put the linux-pci@ list in copy.
Yes, this is a generic problem in the PCI core. We've tried to fix it in the past but haven't figured it out yet.

Quoted 7 lines

See 40f11adc7cd9 ("PCI: Avoid race while enabling upstream bridges") and 0f50a49e3008 ("Revert "PCI: Avoid race while enabling upstream bridges"").
It's not trivial, but if you figure out a good way to fix this, I'd be thrilled.

Bjorn, Srinath, are you aware of anyone working on an updated fix for this one?

Marta

Srinath Mannam

2018-03-23 08:44 UTC #8 lore raw

Hi Marta,

I could not get time to work on this.
The present patch works for our platforms. so we continue with that. I will update new changes little later.
If you have time, please try the same patch and let us know if any issue you see.

Regards,
Srinath.

-----Original Message-----
From: Marta Rybczynska [mailto:[email protected]]
Sent: Friday, March 23, 2018 12:58 PM
To: Bjorn Helgaas <helgaas at kernel.org>
Cc: Keith Busch <keith.busch at intel.com>; Ming Lei <ming.lei at redhat.com>; axboe at fb.com; hch at lst.de; sagi at grimberg.me; linux-nvme at lists.infradead.org; linux-kernel at vger.kernel.org; bhelgaas at google.com;
linux-pci at vger.kernel.org; Pierre-Yves Kerbrat <pkerbrat at kalray.eu>; Srinath Mannam <srinath.mannam at broadcom.com>
Subject: Re: [RFC PATCH] nvme: avoid race-conditions when enabling devices

Quoted 10 lines · Marta Rybczynska

Quoted 5 lines
The problem may happen also with other device doing its probe and nvme running its workqueue (and we probably have seen it in practice too). We were thinking about a lock in the pci generic code too, that's why I've put the linux-pci@ list in copy.
Yes, this is a generic problem in the PCI core. We've tried to fix it in the past but haven't figured it out yet.

Quoted 7 lines

See 40f11adc7cd9 ("PCI: Avoid race while enabling upstream bridges") and 0f50a49e3008 ("Revert "PCI: Avoid race while enabling upstream bridges"").
It's not trivial, but if you figure out a good way to fix this, I'd be thrilled.

Bjorn, Srinath, are you aware of anyone working on an updated fix for this one?

Marta

Marta Rybczynska

author

2018-03-23 07:44 UTC #9 lore raw

In reply to Marta Rybczynska

----- Mail original -----

Quoted 71 lines

De: "Marta Rybczynska" <mrybczyn at kalray.eu>
?: "Keith Busch" <keith.busch at intel.com>
Cc: "Ming Lei" <ming.lei at redhat.com>, axboe at fb.com, hch at lst.de, sagi at grimberg.me, linux-nvme at lists.infradead.org, linux-kernel at vger.kernel.org, bhelgaas at google.com, linux-pci at vger.kernel.org, "Pierre-Yves Kerbrat" <pkerbrat at kalray.eu>
Envoy?: Mercredi 21 Mars 2018 17:10:56
Objet: Re: [RFC PATCH] nvme: avoid race-conditions when enabling devices
Quoted 63 lines · Ming Lei
Quoted 31 lines · Marta Rybczynska
Quoted 30 lines
Quoted 29 lines · Marta Rybczynska
Quoted 12 lines
NVMe driver uses threads for the work at device reset, including enabling the PCIe device. When multiple NVMe devices are initialized, their reset works may be scheduled in parallel. Then pci_enable_device_mem can be called in parallel on multiple cores.
This causes a loop of enabling of all upstream bridges in pci_enable_bridge(). pci_enable_bridge() causes multiple operations including __pci_set_master and architecture-specific functions that call ones like and pci_enable_resources(). Both __pci_set_master() and pci_enable_resources() read PCI_COMMAND field in the PCIe space and change it. This is done as read/modify/write.
Quoted 8 lines
Imagine that the PCIe tree looks like:
A - B - switch - C - D
               \- E - F
D and F are two NVMe disks and all devices from B are not enabled and bus mastering is not set. If their reset work are scheduled in parallel the two modifications of PCI_COMMAND may happen in parallel without locking and the system may end up with the part of PCIe tree not enabled.
Then looks serialized reset should be used, and I did see the commit 79c48ccf2fe ("nvme-pci: serialize pci resets") fixes issue of 'failed to mark controller state' in reset stress test.
But that commit only covers case of PCI reset from sysfs attribute, and maybe other cases need to be dealt with in similar way too.
Quoted 6 lines
Quoted 3 lines
It seems to me that the serialized reset works for multiple resets of the same device, doesn't it? Our problem is linked to resets of different devices that share the same PCIe tree.
Given reset shouldn't be a frequent action, it might be fine to serialize all reset from different devices.
The driver was much simpler when we had serialized resets in line with probe, but that had a bigger problems with certain init systems when you put enough nvme devices in your server, making them unbootable.
Would it be okay to serialize just the pci_enable_device across all other tasks messing with the PCI topology?
---
drivers/nvme/host/pci.c +5 -2
index cef5ce851a92..e0a2f6c0f1cf 100644
@@ -2094,8 +2094,11 @@ static int nvme_pci_enable(struct nvme_dev *dev)
 	int result = -ENOMEM;
 	struct pci_dev *pdev = to_pci_dev(dev->dev);
 
-	if (pci_enable_device_mem(pdev))
-		return result;
+	pci_lock_rescan_remove();
+	result = pci_enable_device_mem(pdev);
+	pci_unlock_rescan_remove();
+	if (result)
+		return -ENODEV;
 

Quoted 9 lines

drivers/nvme/host/pci.c static int nvme_pci_enable(struct nvme_dev *dev) · line 2104
Quoted 2 lines
drivers/nvme/host/pci.c +0 -0
 	pci_set_master(pdev);
 
The problem may happen also with other device doing its probe and nvme running its
workqueue (and we probably have seen it in practice too). We were thinking about a lock
in the pci generic code too, that's why I've put the linux-pci@ list in copy.

Keith, it looks to me that this is going to fix the issue between two nvme driver instances at hotplug time. This is the one we didn't cover in the first patch.

We can see the issue at driver load (so at boot) and the lock isn't taken by the generic non-rescan code. Other calls to pci_enable_device_mem aren't protected neither (see Bjorns message).

What do you think about applying both for now until we have a generic fix in pci?

Marta