pci: Rework is_added race fix and address bridge enable races

This reverts commit 44bda4b7d26e9fffed6d7152d98a2e9edaeb2a76.

The new pci state mutex provides a simpler way of addressing this race and avoids the relative includes added to the powerpc code.

Signed-off-by Benjamin Herrenschmidt <[email protected]>

---
 arch/powerpc/kernel/pci-common.c          |  4 +---
 arch/powerpc/platforms/powernv/pci-ioda.c |  3 +--
 arch/powerpc/platforms/pseries/setup.c    |  3 +--
 drivers/pci/bus.c                         |  6 +++---
 drivers/pci/hotplug/acpiphp_glue.c        |  2 +-
 drivers/pci/pci.h                         | 11 -----------
 drivers/pci/probe.c                       |  4 ++--
 drivers/pci/remove.c                      |  5 ++---
 include/linux/pci.h                       |  1 +
 9 files changed, 12 insertions(+), 27 deletions(-)

arch/powerpc/kernel/pci-common.c +1 -3

index 471aac313b89..fe9733ffffaa 100644
@@ -42,8 +42,6 @@
 #include <asm/ppc-pci.h>
 #include <asm/eeh.h>
 
-#include "../../../drivers/pci/pci.h"
-
 /* hose_spinlock protects accesses to the the phb_bitmap. */
 static DEFINE_SPINLOCK(hose_spinlock);
 LIST_HEAD(hose_list);

@@ -1016,7 +1014,7 @@ void pcibios_setup_bus_devices(struct pci_bus *bus)
 		/* Cardbus can call us to add new devices to a bus, so ignore
 		 * those who are already fully discovered
 		 */
-		if (pci_dev_is_added(dev))
+		if (dev->is_added)
 			continue;
 
 		pcibios_setup_device(dev);

arch/powerpc/platforms/powernv/pci-ioda.c +1 -2

index 70b2e1e0f23c..5bd0eb6681bc 100644
@@ -46,7 +46,6 @@
 
 #include "powernv.h"
 #include "pci.h"
-#include "../../../../drivers/pci/pci.h"
 
 #define PNV_IODA1_M64_NUM	16	/* Number of M64 BARs	*/
 #define PNV_IODA1_M64_SEGS	8	/* Segments per M64 BAR	*/

@@ -3139,7 +3138,7 @@ static void pnv_pci_ioda_fixup_iov_resources(struct pci_dev *pdev)
 	struct pci_dn *pdn;
 	int mul, total_vfs;
 
-	if (!pdev->is_physfn || pci_dev_is_added(pdev))
+	if (!pdev->is_physfn || pdev->is_added)
 		return;
 
 	pdn = pci_get_pdn(pdev);

arch/powerpc/platforms/pseries/setup.c +1 -2

index 8a4868a3964b..139f0af6c3d9 100644
@@ -71,7 +71,6 @@
 #include <asm/security_features.h>
 
 #include "pseries.h"
-#include "../../../../drivers/pci/pci.h"
 
 int CMO_PrPSP = -1;
 int CMO_SecPSP = -1;

@@ -665,7 +664,7 @@ static void pseries_pci_fixup_iov_resources(struct pci_dev *pdev)
 	const int *indexes;
 	struct device_node *dn = pci_device_to_OF_node(pdev);
 
-	if (!pdev->is_physfn || pci_dev_is_added(pdev))
+	if (!pdev->is_physfn || pdev->is_added)
 		return;
 	/*Firmware must support open sriov otherwise dont configure*/
 	indexes = of_get_property(dn, "ibm,open-sriov-vf-bar-info", NULL);

drivers/pci/bus.c +3 -3

index 5cb40b2518f9..35b7fc87eac5 100644
@@ -330,7 +330,7 @@ void pci_bus_add_device(struct pci_dev *dev)
 		return;
 	}
 
-	pci_dev_assign_added(dev, true);
+	dev->is_added = 1;
 }
 EXPORT_SYMBOL_GPL(pci_bus_add_device);
 

@@ -347,14 +347,14 @@ void pci_bus_add_devices(const struct pci_bus *bus)
 
 	list_for_each_entry(dev, &bus->devices, bus_list) {
 		/* Skip already-added devices */
-		if (pci_dev_is_added(dev))
+		if (dev->is_added)
 			continue;
 		pci_bus_add_device(dev);
 	}
 
 	list_for_each_entry(dev, &bus->devices, bus_list) {
 		/* Skip if device attach failed */
-		if (!pci_dev_is_added(dev))
+		if (!dev->is_added)
 			continue;
 		child = dev->subordinate;
 		if (child)

drivers/pci/hotplug/acpiphp_glue.c +1 -1

index ef0b1b6ba86f..3a17b290df5d 100644
@@ -509,7 +509,7 @@ static void enable_slot(struct acpiphp_slot *slot)
 
 	list_for_each_entry(dev, &bus->devices, bus_list) {
 		/* Assume that newly added devices are powered on already. */
-		if (!pci_dev_is_added(dev))
+		if (!dev->is_added)
 			dev->current_state = PCI_D0;
 	}
 

drivers/pci/pci.h +0 -11

index 6e0d1528d471..473aa10a5dbf 100644
@@ -295,7 +295,6 @@ struct pci_sriov {
 
 /* pci_dev priv_flags */
 #define PCI_DEV_DISCONNECTED 0
-#define PCI_DEV_ADDED 1
 
 static inline int pci_dev_set_disconnected(struct pci_dev *dev, void *unused)
 {

@@ -308,16 +307,6 @@ static inline bool pci_dev_is_disconnected(const struct pci_dev *dev)
 	return test_bit(PCI_DEV_DISCONNECTED, &dev->priv_flags);
 }
 
-static inline void pci_dev_assign_added(struct pci_dev *dev, bool added)
-{
-	assign_bit(PCI_DEV_ADDED, &dev->priv_flags, added);
-}
-
-static inline bool pci_dev_is_added(const struct pci_dev *dev)
-{
-	return test_bit(PCI_DEV_ADDED, &dev->priv_flags);
-}
-
 #ifdef CONFIG_PCIEAER
 #include <linux/aer.h>
 

drivers/pci/probe.c +2 -2

index ec784009a36b..440445ac7dfa 100644
@@ -2525,13 +2525,13 @@ int pci_scan_slot(struct pci_bus *bus, int devfn)
 	dev = pci_scan_single_device(bus, devfn);
 	if (!dev)
 		return 0;
-	if (!pci_dev_is_added(dev))
+	if (!dev->is_added)
 		nr++;
 
 	for (fn = next_fn(bus, dev, 0); fn > 0; fn = next_fn(bus, dev, fn)) {
 		dev = pci_scan_single_device(bus, devfn + fn);
 		if (dev) {
-			if (!pci_dev_is_added(dev))
+			if (!dev->is_added)
 				nr++;
 			dev->multifunction = 1;
 		}

drivers/pci/remove.c +2 -3

index 461e7fd2756f..01ec7fcb5634 100644
@@ -18,12 +18,11 @@ static void pci_stop_dev(struct pci_dev *dev)
 {
 	pci_pme_active(dev, false);
 
-	if (pci_dev_is_added(dev)) {
+	if (dev->is_added) {
 		device_release_driver(&dev->dev);
 		pci_proc_detach_device(dev);
 		pci_remove_sysfs_dev_files(dev);
-
-		pci_dev_assign_added(dev, false);
+		dev->is_added = 0;
 	}
 
 	if (dev->bus->self)

include/linux/pci.h +1 -0

index 9b87f1936906..9799109c5e25 100644
@@ -373,6 +373,7 @@ struct pci_dev {
 	unsigned int	transparent:1;		/* Subtractive decode bridge */
 	unsigned int	multifunction:1;	/* Multi-function device */
 
+	unsigned int	is_added:1;
 	unsigned int	is_busmaster:1;		/* Is busmaster */
 	unsigned int	no_msi:1;		/* May not use MSI */
 	unsigned int	no_64bit_msi:1; 	/* May only use 32-bit MSIs */

This reverts commit 44bda4b7d26e9fffed6d7152d98a2e9edaeb2a76.

The new pci state mutex provides a simpler way of addressing this race and avoids the relative includes added to the powerpc code.

Quoted 1 line · Benjamin Herrenschmidt

This reverts commit 44bda4b7d26e9fffed6d7152d98a2e9edaeb2a76.

Just to be clear, if I understand correctly, this is a pure revert of 44bda4b7d26e and as such it reintroduces the problem solved by that commit.

If your solution turns out to be better, that's great, but it would be nice to avoid the bisection hole of reintroducing the problem, then fixing it again later.

Quoted 3 lines

The new pci state mutex provides a simpler way of addressing this race and avoids the relative includes added to the powerpc code.

Quoted 7 lines · Bjorn Helgaas

Quoted 1 line · Benjamin Herrenschmidt

This reverts commit 44bda4b7d26e9fffed6d7152d98a2e9edaeb2a76.

Just to be clear, if I understand correctly, this is a pure revert of 44bda4b7d26e and as such it reintroduces the problem solved by that commit.

Quoted 3 lines

If your solution turns out to be better, that's great, but it would be nice to avoid the bisection hole of reintroducing the problem, then fixing it again later.

There is no way to do that other than merging the revert and the fix into one. That said, the race is sufficiently hard to hit that I wouldn't worry too much about it.

Quoted 3 lines

Quoted 3 lines

The new pci state mutex provides a simpler way of addressing this race and avoids the relative includes added to the powerpc code.

Quoted 12 lines · Benjamin Herrenschmidt

Quoted 10 lines · Bjorn Helgaas

Quoted 1 line · Benjamin Herrenschmidt

This reverts commit 44bda4b7d26e9fffed6d7152d98a2e9edaeb2a76.

Just to be clear, if I understand correctly, this is a pure revert of 44bda4b7d26e and as such it reintroduces the problem solved by that commit.

If your solution turns out to be better, that's great, but it would be nice to avoid the bisection hole of reintroducing the problem, then fixing it again later.

Quoted 3 lines

There is no way to do that other than merging the revert and the fix into one. That said, the race is sufficiently hard to hit that I wouldn't worry too much about it.

OK, then at least mention that in the changelog.

Quoted 13 lines · Bjorn Helgaas

Quoted 12 lines · Benjamin Herrenschmidt

Quoted 10 lines · Bjorn Helgaas

Quoted 1 line · Benjamin Herrenschmidt

This reverts commit 44bda4b7d26e9fffed6d7152d98a2e9edaeb2a76.

Just to be clear, if I understand correctly, this is a pure revert of 44bda4b7d26e and as such it reintroduces the problem solved by that commit.

If your solution turns out to be better, that's great, but it would be nice to avoid the bisection hole of reintroducing the problem, then fixing it again later.

Quoted 5 lines

Quoted 3 lines

There is no way to do that other than merging the revert and the fix into one. That said, the race is sufficiently hard to hit that I wouldn't worry too much about it.

OK, then at least mention that in the changelog.

Sure will do. This is just RFC at this stage :-)

As for the race with enable, what's your take on my approach ? The basic premise is that we need some kind of mutex to make the updates to enable_cnt and the actual config space changes atomic. While at it we can fold pci_set_master vs. is_busmaster as well as those are racy too.

I chose to create a new mutex which we should be able to address other similar races if we find them. The other solutions that I dismissed were:

- Using the device_lock. A explained previously, this is tricky, I
prefer not using this for anything other than locking against concurrent add/remove. The main issue is that drivers will be sometimes called in context where that's already held, so we can't take it inside pci_enable_device() and I'd rather not add new constraints such as "pci_enable_device() must be only called from probe() unless you also take the device lock". It would be tricky to audit everybody...

- Using a global mutex. We could move the bridge lock from AER to core
code for example, and use that. But it doesn't buy us much, and slightly redecuces parallelism. It also makes it a little bit more messy to walk up the bridge chain, we'd have to do a pci_enable_device_unlocked or something, messy.

So are you ok with the approach ? Do you prefer one of the above regardless ? Something else ?

Cheers,
Ben.

Quoted 48 lines · Benjamin Herrenschmidt

Quoted 13 lines · Bjorn Helgaas

Quoted 12 lines · Benjamin Herrenschmidt

Quoted 10 lines · Bjorn Helgaas

Quoted 1 line · Benjamin Herrenschmidt

This reverts commit 44bda4b7d26e9fffed6d7152d98a2e9edaeb2a76.

Just to be clear, if I understand correctly, this is a pure revert of 44bda4b7d26e and as such it reintroduces the problem solved by that commit.

If your solution turns out to be better, that's great, but it would be nice to avoid the bisection hole of reintroducing the problem, then fixing it again later.

Quoted 5 lines

Quoted 3 lines

There is no way to do that other than merging the revert and the fix into one. That said, the race is sufficiently hard to hit that I wouldn't worry too much about it.

OK, then at least mention that in the changelog.

Sure will do. This is just RFC at this stage :-)

As for the race with enable, what's your take on my approach ? The basic premise is that we need some kind of mutex to make the updates to enable_cnt and the actual config space changes atomic. While at it we can fold pci_set_master vs. is_busmaster as well as those are racy too.

I chose to create a new mutex which we should be able to address other similar races if we find them. The other solutions that I dismissed were:

- Using the device_lock. A explained previously, this is tricky, I
prefer not using this for anything other than locking against concurrent add/remove. The main issue is that drivers will be sometimes called in context where that's already held, so we can't take it inside pci_enable_device() and I'd rather not add new constraints such as "pci_enable_device() must be only called from probe() unless you also take the device lock". It would be tricky to audit everybody...

- Using a global mutex. We could move the bridge lock from AER to core
code for example, and use that. But it doesn't buy us much, and slightly redecuces parallelism. It also makes it a little bit more messy to walk up the bridge chain, we'd have to do a pci_enable_device_unlocked or something, messy.

So are you ok with the approach ? Do you prefer one of the above regardless ? Something else ?

Quoted 4 lines

Cheers,
Ben.

Some concern was raised about race situation so just to be more clear about race condition.

Situation is described in Bug 200283 - PCI: Data corruption happening due to a race condition.
Issue was discovered by our broadcom QA team.
Initially commit was to use one separate lock only for avoiding race condition but after review, approach was slightly changed to move is_added to pci_dev private flags as it was completely internal/private variable of pci core driver.
Powerpc legacy arch code was using is_added flag directly which looks bit strange so ../../ type of inclusion of pci.h was required. I know it looks ugly but it is being used in some legacy code still. Anyway, as stated earlier too, problem should be just solved in better way.

Quoted 50 lines · Hari Vyas

Quoted 48 lines · Benjamin Herrenschmidt

Quoted 13 lines · Bjorn Helgaas

Quoted 12 lines · Benjamin Herrenschmidt

Quoted 10 lines · Bjorn Helgaas

Quoted 1 line · Benjamin Herrenschmidt

This reverts commit 44bda4b7d26e9fffed6d7152d98a2e9edaeb2a76.

Just to be clear, if I understand correctly, this is a pure revert of 44bda4b7d26e and as such it reintroduces the problem solved by that commit.

If your solution turns out to be better, that's great, but it would be nice to avoid the bisection hole of reintroducing the problem, then fixing it again later.

Quoted 5 lines

Quoted 3 lines

There is no way to do that other than merging the revert and the fix into one. That said, the race is sufficiently hard to hit that I wouldn't worry too much about it.

OK, then at least mention that in the changelog.

Sure will do. This is just RFC at this stage :-)

As for the race with enable, what's your take on my approach ? The basic premise is that we need some kind of mutex to make the updates to enable_cnt and the actual config space changes atomic. While at it we can fold pci_set_master vs. is_busmaster as well as those are racy too.

I chose to create a new mutex which we should be able to address other similar races if we find them. The other solutions that I dismissed were:

- Using the device_lock. A explained previously, this is tricky, I
prefer not using this for anything other than locking against concurrent add/remove. The main issue is that drivers will be sometimes called in context where that's already held, so we can't take it inside pci_enable_device() and I'd rather not add new constraints such as "pci_enable_device() must be only called from probe() unless you also take the device lock". It would be tricky to audit everybody...

- Using a global mutex. We could move the bridge lock from AER to core
code for example, and use that. But it doesn't buy us much, and slightly redecuces parallelism. It also makes it a little bit more messy to walk up the bridge chain, we'd have to do a pci_enable_device_unlocked or something, messy.

So are you ok with the approach ? Do you prefer one of the above regardless ? Something else ?

Quoted 7 lines

Quoted 4 lines

Cheers,
Ben.

Some concern was raised about race situation so just to be more clear about race condition.

This is not what the above discussion is about.

The race with is is_added is due to the fact that the bit is set too later as discussed previously and in my changelog.

The race I'm talking about here is the race related to multiple subtrees trying to simultaneously enable a parent bridge.

Quoted 11 lines

Situation is described in Bug 200283 - PCI: Data corruption happening due to a race condition.
Issue was discovered by our broadcom QA team.
Initially commit was to use one separate lock only for avoiding race condition but after review, approach was slightly changed to move is_added to pci_dev private flags as it was completely internal/private variable of pci core driver.
Powerpc legacy arch code was using is_added flag directly which looks bit strange so ../../ type of inclusion of pci.h was required. I know it looks ugly but it is being used in some legacy code still. Anyway, as stated earlier too, problem should be just solved in better way.

The is_added race can be fixed with a 3 lines patch moving is_added up to before device_attach() I believe. I yet have to find a scenario where doing so would break something but it's possible that I missed a case.

At this stage, I'm more intested however in us agreeing how to fix the other race, the one with enabling. As I wrote above, I proposed an approach based on a mutex in pci_dev, and this is what I would like discussed.

This race is currently causing our large systems to randomly error out at boot time when probing some PCIe devices. I'm putting a band-aid in the powerpc code for now to pre-enable bridges at boot, but I'd rather have the race fixed in the generic code.

Ben.

I chose to create a new mutex which we should be able to address other similar races if we find them. The other solutions that I dismissed were:

- Using the device_lock. A explained previously, this is tricky, I
prefer not using this for anything other than locking against concurrent add/remove. The main issue is that drivers will be sometimes called in context where that's already held, so we can't take it inside pci_enable_device() and I'd rather not add new constraints such as "pci_enable_device() must be only called from probe() unless you also take the device lock". It would be tricky to audit everybody...

- Using a global mutex. We could move the bridge lock from AER to core
code for example, and use that. But it doesn't buy us much, and slightly redecuces parallelism. It also makes it a little bit more messy to walk up the bridge chain, we'd have to do a pci_enable_device_unlocked or something, messy.

Quoted 12 lines · Benjamin Herrenschmidt

I chose to create a new mutex which we should be able to address other similar races if we find them. The other solutions that I dismissed were:

- Using the device_lock. A explained previously, this is tricky, I
prefer not using this for anything other than locking against concurrent add/remove. The main issue is that drivers will be sometimes called in context where that's already held, so we can't take it inside pci_enable_device() and I'd rather not add new constraints such as "pci_enable_device() must be only called from probe() unless you also take the device lock". It would be tricky to audit everybody...

Quoted 5 lines

- Using a global mutex. We could move the bridge lock from AER to core
code for example, and use that. But it doesn't buy us much, and slightly redecuces parallelism. It also makes it a little bit more messy to walk up the bridge chain, we'd have to do a pci_enable_device_unlocked or something, messy.

+1 from my side for adding a struct mutex to struct pci_dev to protect state changes.

The device_lock() primarily protects binding / unbinding of the device and pci_dev state may have to be changed while binding / unbinding.

A global lock invites deadlocks if multiple devices are added / removed concurrently where one is a parent of the other. (Think hot-removal of multiple devices on a Thunderbolt daisy-chain.)

As said I'd also welcome folding PCI_DEV_DISCONNECTED into enum pci_channel_state, either as an additional state or by using pci_channel_io_perm_failure.

Thanks,

Lukas

This re-fixes the bug reported by Hari Vyas <[email protected]> after my revert of his commit but in a much simpler way.

The main issues is that is_added was being set after the driver got bound and started, and thus setting it could race with other changes to struct pci_dev.

This fixes it by setting the flag first, which also has the advantage of matching the fact that we are clearing it *after* unbinding in the remove path, thus the flag is now symtetric and always set while the driver code is running.

Signed-off-by Benjamin Herrenschmidt <[email protected]>

---
 drivers/pci/bus.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/pci/bus.c +2 -2

index 35b7fc87eac5..48ae63673aa8 100644
@@ -321,16 +321,16 @@ void pci_bus_add_device(struct pci_dev *dev)
 	pci_proc_attach_device(dev);
 	pci_bridge_d3_update(dev);
 
+	dev->is_added = 1;
 	dev->match_driver = true;
 	retval = device_attach(&dev->dev);
 	if (retval < 0 && retval != -EPROBE_DEFER) {
+		dev->is_added = 0;
 		pci_warn(dev, "device attach failed (%d)\n", retval);
 		pci_proc_detach_device(dev);
 		pci_remove_sysfs_dev_files(dev);
 		return;
 	}
-
-	dev->is_added = 1;
 }
 EXPORT_SYMBOL_GPL(pci_bus_add_device);
 

This re-fixes the bug reported by Hari Vyas <[email protected]> after my revert of his commit but in a much simpler way.

The main issues is that is_added was being set after the driver got bound and started, and thus setting it could race with other changes to struct pci_dev.

This fixes it by setting the flag first, which also has the advantage of matching the fact that we are clearing it *after* unbinding in the remove path, thus the flag is now symtetric and always set while the driver code is running.

Signed-off-by Benjamin Herrenschmidt <[email protected]>

---
 drivers/pci/bus.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/pci/bus.c +2 -2

index 35b7fc87eac5..48ae63673aa8 100644
@@ -321,16 +321,16 @@ void pci_bus_add_device(struct pci_dev *dev)
 	pci_proc_attach_device(dev);
 	pci_bridge_d3_update(dev);
 
+	dev->is_added = 1;
 	dev->match_driver = true;
 	retval = device_attach(&dev->dev);
 	if (retval < 0 && retval != -EPROBE_DEFER) {
+		dev->is_added = 0;
 		pci_warn(dev, "device attach failed (%d)\n", retval);
 		pci_proc_detach_device(dev);
 		pci_remove_sysfs_dev_files(dev);
 		return;
 	}
-
-	dev->is_added = 1;
 }
 EXPORT_SYMBOL_GPL(pci_bus_add_device);
 

Signature · 1 line

2.17.1

The "bind driver, then set dev->added = 1" order seems to have been there since the beginning of dev->is_added:

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=8a1bc9013a03

This patch seems reasonable, but I'm a little dubious about the existence of "is_added" in the first place. As far as I can tell, the only other buses with something similar are the MEN Chameleon bus and the Intel Management Engine Interface.

The PCI uses of "is_added" don't seem *that* critical or unique to PCI, so I'm not 100% convinced we need it at all. But I haven't looked into it enough to be able to propose an alternative.

The two steps (enumeration and driver attachment) are protected by pci_lock_rescan_remove(). Initially, when a pci_dev is newly allocated with kzalloc(), is_added is 0. The transition from 0 to 1 happens while under pci_lock_rescan_remove(). When that lock is released, is_added is always 1, barring a device_attach() error, in which case it will remain at 0.

AFAICS, there is no second mutex needed to ensure synchronization of is_added, the existing mutex should be sufficient and the only problem are RMW races when accessing adjacent flags. Those were correctly addressed by Hari's patch. Benjamin seems to be alleging that concurrency issues exist beyond the RMW races, I fail to see them, sorry.

Quoted 6 lines · Benjamin Herrenschmidt

This re-fixes the bug reported by Hari Vyas <[email protected]> after my revert of his commit but in a much simpler way.

The main issues is that is_added was being set after the driver got bound and started, and thus setting it could race with other changes to struct pci_dev.

The "bind driver, then set dev->added = 1" order seems to have been there since the beginning of dev->is_added:

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=8a1bc9013a03

This patch seems reasonable, but I'm a little dubious about the existence of "is_added" in the first place. As far as I can tell, the only other buses with something similar are the MEN Chameleon bus and the Intel Management Engine Interface.

The PCI uses of "is_added" don't seem *that* critical or unique to PCI, so I'm not 100% convinced we need it at all. But I haven't looked into it enough to be able to propose an alternative.

Quoted 36 lines

This fixes it by setting the flag first, which also has the advantage of matching the fact that we are clearing it *after* unbinding in the remove path, thus the flag is now symtetric and always set while the driver code is running.

Signed-off-by Benjamin Herrenschmidt <[email protected]>

---
 drivers/pci/bus.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/pci/bus.c +2 -2

index 35b7fc87eac5..48ae63673aa8 100644
@@ -321,16 +321,16 @@ void pci_bus_add_device(struct pci_dev *dev)
 	pci_proc_attach_device(dev);
 	pci_bridge_d3_update(dev);
 
+	dev->is_added = 1;
 	dev->match_driver = true;
 	retval = device_attach(&dev->dev);
 	if (retval < 0 && retval != -EPROBE_DEFER) {
+		dev->is_added = 0;
 		pci_warn(dev, "device attach failed (%d)\n", retval);
 		pci_proc_detach_device(dev);
 		pci_remove_sysfs_dev_files(dev);
 		return;
 	}
-
-	dev->is_added = 1;
 }
 EXPORT_SYMBOL_GPL(pci_bus_add_device);
 

Signature · 1 line

2.17.1

This protects enable/disable operations using the state mutex to avoid races with, for example, concurrent enables on a bridge.

The bus hierarchy is walked first before taking the lock to avoid lock nesting (though it would be ok if we ensured that we always nest them bottom-up, it is better to just avoid the issue alltogether, especially as we might find cases where we want to take it top-down later).

Signed-off-by Benjamin Herrenschmidt <[email protected]>

static void pci_enable_bridge(struct pci_dev *dev)

{
	struct pci_dev *bridge;

drivers/pci/pci.c +1 -1

-	int retval;
+	int retval, enabled;

	bridge = pci_upstream_bridge(dev);
	if (bridge)
		pci_enable_bridge(bridge);

drivers/pci/pci.c +7 -3

-	if (pci_is_enabled(dev)) {
-		if (!dev->is_busmaster)
-			pci_set_master(dev);
+	/* Already enabled ? */
+	pci_dev_state_lock(dev);
+	enabled = pci_is_enabled(dev);
+	if (enabled && !dev->is_busmaster)
+		pci_set_master(dev);
+	pci_dev_state_unlock(dev);
+	if (enabled)

		return;

Quoted 35 lines · ----- On 17 Aug, 2018, at 06:49, Benjamin Herrenschmidt…

This protects enable/disable operations using the state mutex to avoid races with, for example, concurrent enables on a bridge.

The bus hierarchy is walked first before taking the lock to avoid lock nesting (though it would be ok if we ensured that we always nest them bottom-up, it is better to just avoid the issue alltogether, especially as we might find cases where we want to take it top-down later).

Signed-off-by Benjamin Herrenschmidt <[email protected]>

static void pci_enable_bridge(struct pci_dev *dev)

{
	struct pci_dev *bridge;

drivers/pci/pci.c +1 -1

-	int retval;
+	int retval, enabled;

	bridge = pci_upstream_bridge(dev);
	if (bridge)
		pci_enable_bridge(bridge);

drivers/pci/pci.c +7 -3

-	if (pci_is_enabled(dev)) {
-		if (!dev->is_busmaster)
-			pci_set_master(dev);
+	/* Already enabled ? */
+	pci_dev_state_lock(dev);
+	enabled = pci_is_enabled(dev);
+	if (enabled && !dev->is_busmaster)
+		pci_set_master(dev);
+	pci_dev_state_unlock(dev);
+	if (enabled)

		return;

drivers/pci/pci.c static int do_pci_enable_device(struct pci_dev *dev, int bars) · line 1562

Quoted 1 line

drivers/pci/pci.c +0 -1

-	}

This looks complicated too me especially with the double locking. What do you think about a function doing that check that used an unlocked version of pcie_set_master?

Like:

        dev_state_lock(dev);
        enabled = pci_is_enabled(dev);
        if (enabled &&  !dev->is_busmaster)
                pci_set_master_unlocked();
        pci_dev_state_unlock(dev);

BTW If I remember correctly the code today can set bus mastering multiple times without checking if already done.

Quoted 46 lines · Marta Rybczynska

Quoted 26 lines · ----- On 17 Aug, 2018, at 06:49, Benjamin Herrenschmidt…

This protects enable/disable operations using the state mutex to avoid races with, for example, concurrent enables on a bridge.

The bus hierarchy is walked first before taking the lock to avoid lock nesting (though it would be ok if we ensured that we always nest them bottom-up, it is better to just avoid the issue alltogether, especially as we might find cases where we want to take it top-down later).

Signed-off-by Benjamin Herrenschmidt <[email protected]>

static void pci_enable_bridge(struct pci_dev *dev)

{
    struct pci_dev *bridge;

drivers/pci/pci.c +1 -3

-   int retval;
+   int retval, enabled;
 
    bridge = pci_upstream_bridge(dev);
    if (bridge)
            pci_enable_bridge(bridge);
 
-   if (pci_is_enabled(dev)) {
-           if (!dev->is_busmaster)

drivers/pci/pci.c static int do_pci_enable_device(struct pci_dev *dev, int bars) · line 1562

Quoted 10 lines

drivers/pci/pci.c +7 -2

-                   pci_set_master(dev);
+   /* Already enabled ? */
+   pci_dev_state_lock(dev);
+   enabled = pci_is_enabled(dev);
+   if (enabled && !dev->is_busmaster)
+           pci_set_master(dev);
+   pci_dev_state_unlock(dev);
+   if (enabled)
            return;
-   }

This looks complicated too me especially with the double locking. What do you think about a function doing that check that used an unlocked version of pcie_set_master?

Like:

Quoted 8 lines

        dev_state_lock(dev);
        enabled = pci_is_enabled(dev);
        if (enabled &&  !dev->is_busmaster)
                pci_set_master_unlocked();
        pci_dev_state_unlock(dev);

BTW If I remember correctly the code today can set bus mastering multiple times without checking if already done.

I don't mind but I tend to dislike all those _unlocked() suffixes, I suppose my generation is typing adverse enough that we call these __something instead :)

As for setting multiple times, yes pci_set_master() doesn't check but look at the "-" hunks of my patch, I'm not changing the existing test here. Not that it matters much, it's an optimization.

In fact my original version just completely removed the whole lot and just unconditionally did pci_enable_device() + pci_set_master(), just ignoring the existing state :-)

But I decided to keep the patch functionally equivalent so I added it back.

Cheers,
Ben.

So many mail threads for common issues going so just trying to summarize concern from my side.
1) HW level
PCI Config data overwritten (ex: PCI_COMMAND bits etc) was happening at lower layer so from my
perspective, it is best to handle concern at lower level with locking mechanism and that is what
I proposed in my upcoming patch. Without that, I guess, we may end up in issues later with some weird scenario
which may not be known today and we will again be fine tuning stuff here and there.

2) SW level(internal data structure):
About is_added,is_busmaster: These all are bit fields so infact I too suggested to remove those bit fields and
make separate variables in pci_dev structure.
This will avoid all data-overwritten,concurrency issues but ofcourse at the level of space cost.

Other possibility will be either to use atomic

or locking mechanism
for these. My point here is also same, better
avoid fine tuning later stage.
Moving is_added up and down doesn't look like good as we are just shifting issue up and down.

Please check and decide accordingly. I will hold my to-be-submitted modify() patch about handling hw level
over-written case with locking around read-write operation.

Quoted 11 lines · Hari Vyas

So many mail threads for common issues going so just trying to summarize concern from my side.
1) HW level
PCI Config data overwritten (ex: PCI_COMMAND bits etc) was happening at lower layer so from my
perspective, it is best to handle concern at lower level with locking mechanism and that is what
I proposed in my upcoming patch. Without that, I guess, we may end up in issues later with some weird scenario
which may not be known today and we will again be fine tuning stuff here and there.

Maybe... at this point I'm not trying to address that specific issue. That being said, the PCI_COMMAND register should be under control of the driver at runtime and thus shouldn't be exposed to races like that except in the usual case of races in configuring bridges. Those races definitely need to be handled at the higher level.

So I'm rather dubious of adding a whole new layer of "modify" callbacks to config space accessors for that, especially since they won't do any good on architectures with lockless config accesses such as ... x86

Quoted 6 lines

2) SW level(internal data structure):
About is_added,is_busmaster: These all are bit fields so infact I too suggested to remove those bit fields and
make separate variables in pci_dev structure.
This will avoid all data-overwritten,concurrency issues but ofcourse at the level of space cost.

It's unnecessary to do blanket changes without first understanding the details of what's going on. A lot of these things are never touched outside of the overall single threaded environment of discovery/assignment or under driver control, in which case it's expectd that the driver doesn't call them in racy ways

That said, I'm happy to move some of them under my proposed dev_state_lock.

For is_added specifically, the field is simply set at the wrong time as you can see in my previous patch.

Quoted 1 line

Other possibility will be either to use atomic

Atomic is almost always wrong. It doesn't synchronize anything other than the field, so either it's a big waste of it gives a false sense of security for something that's almost always still racy.

I'd rather keep the indication that a bunch of those fields *are* unprotected and rely on the outer layers being single threaded.

Quoted 5 lines

or locking mechanism
for these. My point here is also same, better
avoid fine tuning later stage.
Moving is_added up and down doesn't look like good as we are just shifting issue up and down.

No, you are wrong. I also originally covered is_added with my new mutex but in hindsight it's the wrong thing to do, and went back to the correct fix at this point which is precisely to move it up.

is_added is essentially owned by the part of the PCI probe code that runs single threaded before the device gets exposed to the device model.

Pretty much all of the code, including all the corresponding fields (struct resources etc...) in pci_dev are unprotected and rely on being accessed in a single threaded manner. is_added is no exception.

It was simply a mistake to set it after device_attach().

Thus moving it back up fixes it *correctly* by making it follow the same rules as all the related fields.

That said, if we want to start adding protection to all those other fields, then this is a different matter alltogether. But at this point, this is the best fix for the problem at hand.

Quoted 3 lines

Please check and decide accordingly. I will hold my to-be-submitted modify() patch about handling hw level
over-written case with locking around read-write operation.

Can you remind us in this thread which specific cases of RMW races of config space you were trying to address ?

Cheers,
Ben.

Quoted 5 lines

So I'm rather dubious of adding a whole new layer of "modify" callbacks to config space accessors for that, especially since they won't do any good on architectures with lockless config accesses such as ... x86

modify() is optional. host/controller layers may override it.
It was just a proposal to avoid race issues which happens in SMP environment and solves other issues. I agree, nothing great anyhow can be done in lockless config

Quoted 13 lines

Quoted 6 lines

2) SW level(internal data structure):
About is_added,is_busmaster: These all are bit fields so infact I too suggested to remove those bit fields and
make separate variables in pci_dev structure.
This will avoid all data-overwritten,concurrency issues but ofcourse at the level of space cost.

It's unnecessary to do blanket changes without first understanding the details of what's going on. A lot of these things are never touched outside of the overall single threaded environment of discovery/assignment or under driver control, in which case it's expectd that the driver doesn't call them in racy ways

Quoted 6 lines

That said, I'm happy to move some of them under my proposed dev_state_lock.

For is_added specifically, the field is simply set at the wrong time as you can see in my previous patch.

Issue needs to be addressed and that is our goal.

Some times simple mistakes need lot of debugging which happened in my case and my suggestion is to just avoid. SMP related issues are popping up now so we just need to be careful.

Quoted 3 lines

Can you remind us in this thread which specific cases of RMW races of config space you were trying to address ?

Same pci bridge master, memory bit setting concern only (which my colleague Srinath figured out after lot of effort some time back) where only one bit in PCI_COMMAND was getting set.
(Bug 200793 - PCI: read-write config operation doesn't look like SMP safe) My approach is to handle with modify operations at lower level so bits are not over-written or lost.

As stated earlier, issue should be just resolved in better way. No issue in going with majority

Regards,
hari

Quoted 4 lines

Cheers,
Ben.

The second part aims at fixing the enable/disable/set_master races, and does so by providing a framework for future device state locking issues.

It introduces a pci_dev->state_mutex which is used at a lower level than the device_lock (the device lock isn't suitable, as explained in the cset comments) and uses it to protect enablement and set_master.