[PATCH 0/8] PCI: Fix ATS deadlock

[PATCH 0/8] PCI: Fix ATS deadlock

[Bjorn Helgaas]

Gregor reported a deadlock [1] when enabling a VF that supports ATS.
This series is intended to fix that.  The first patch should be enough to
fix the deadlock; the rest are simplification and cleanup.

These are based on v4.2-rc2.

[1] http://permalink.gmane.org/gmane.linux.kernel.iommu/9433

---

Bjorn Helgaas (8):
      PCI: Allocate ATS struct during enumeration
      PCI: Embed ATS info directly into struct pci_dev
      PCI: Reduce size of ATS structure elements
      PCI: Rationalize pci_ats_queue_depth() error checking
      PCI: Inline the ATS setup code into pci_ats_init()
      PCI: Use pci_physfn() rather than looking up physfn by hand
      PCI: Clean up ATS error handling
      PCI: Move ATS declarations to linux/pci.h so they're all together


 drivers/pci/ats.c       |  110 +++++++++++------------------------------------
 drivers/pci/probe.c     |    3 +
 drivers/pci/remove.c    |    1 
 include/linux/pci-ats.h |   49 ---------------------
 include/linux/pci.h     |   20 ++++++++-
 5 files changed, 49 insertions(+), 134 deletions(-)

[PATCH 1/8] PCI: Allocate ATS struct during enumeration

[Bjorn Helgaas]

Previously, we allocated pci_ats structures when an IOMMU driver called
pci_enable_ats().  An SR-IOV VF shares the STU setting with its PF, so when
enabling ATS on the VF, we allocated a pci_ats struct for the PF if it
didn't already have one.  We held the sriov->lock to serialize threads
concurrently enabling ATS on several VFS so only one would allocate the PF
pci_ats.

Gregor reported a deadlock here:

  pci_enable_sriov
    sriov_enable
      virtfn_add
        mutex_lock(dev->sriov->lock)      # acquire sriov->lock
        pci_device_add
          device_add
            BUS_NOTIFY_ADD_DEVICE notifier chain
            iommu_bus_notifier
              amd_iommu_add_device        # iommu_ops.add_device
                init_iommu_group
                  iommu_group_get_for_dev
                    iommu_group_add_device
                      __iommu_attach_device
                        amd_iommu_attach_device  # iommu_ops.attach_device
                          attach_device
                            pci_enable_ats
                              mutex_lock(dev->sriov->lock) # deadlock

There's no reason to delay allocating the pci_ats struct, and if we
allocate it for each device at enumeration-time, there's no need for
locking in pci_enable_ats().

Allocate pci_ats struct during enumeration, when we initialize other
capabilities.

Note that this implementation requires ATS to be enabled on the PF first,
before on any of the VFs because the PF controls the STU for all the VFs.

Link: http://permalink.gmane.org/gmane.linux.kernel.iommu/9433
Reported-by: Gregor Dick <gdick@solarflare.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/ats.c       |   81 +++++++++++++++--------------------------------
 drivers/pci/probe.c     |    3 ++
 drivers/pci/remove.c    |    1 +
 include/linux/pci-ats.h |    1 -
 include/linux/pci.h     |    9 +++++
 5 files changed, 38 insertions(+), 57 deletions(-)

diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index a8099d4..923759f 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -17,7 +17,7 @@
 
 #include "pci.h"
 
-static int ats_alloc_one(struct pci_dev *dev, int ps)
+static void ats_alloc_one(struct pci_dev *dev)
 {
 	int pos;
 	u16 cap;
@@ -25,20 +25,17 @@ static int ats_alloc_one(struct pci_dev *dev, int ps)
 
 	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ATS);
 	if (!pos)
-		return -ENODEV;
+		return;
 
 	ats = kzalloc(sizeof(*ats), GFP_KERNEL);
 	if (!ats)
-		return -ENOMEM;
+		return;
 
 	ats->pos = pos;
-	ats->stu = ps;
 	pci_read_config_word(dev, pos + PCI_ATS_CAP, &cap);
 	ats->qdep = PCI_ATS_CAP_QDEP(cap) ? PCI_ATS_CAP_QDEP(cap) :
 					    PCI_ATS_MAX_QDEP;
 	dev->ats = ats;
-
-	return 0;
 }
 
 static void ats_free_one(struct pci_dev *dev)
@@ -47,6 +44,16 @@ static void ats_free_one(struct pci_dev *dev)
 	dev->ats = NULL;
 }
 
+void pci_ats_init(struct pci_dev *dev)
+{
+	ats_alloc_one(dev);
+}
+
+void pci_ats_free(struct pci_dev *dev)
+{
+	ats_free_one(dev);
+}
+
 /**
  * pci_enable_ats - enable the ATS capability
  * @dev: the PCI device
@@ -56,43 +63,29 @@ static void ats_free_one(struct pci_dev *dev)
  */
 int pci_enable_ats(struct pci_dev *dev, int ps)
 {
-	int rc;
 	u16 ctrl;
 
 	BUG_ON(dev->ats && dev->ats->is_enabled);
 
-	if (ps < PCI_ATS_MIN_STU)
+	if (!dev->ats)
 		return -EINVAL;
 
-	if (dev->is_physfn || dev->is_virtfn) {
-		struct pci_dev *pdev = dev->is_physfn ? dev : dev->physfn;
-
-		mutex_lock(&pdev->sriov->lock);
-		if (pdev->ats)
-			rc = pdev->ats->stu == ps ? 0 : -EINVAL;
-		else
-			rc = ats_alloc_one(pdev, ps);
+	if (ps < PCI_ATS_MIN_STU)
+		return -EINVAL;
 
-		if (!rc)
-			pdev->ats->ref_cnt++;
-		mutex_unlock(&pdev->sriov->lock);
-		if (rc)
-			return rc;
-	}
+	ctrl = PCI_ATS_CTRL_ENABLE;
+	if (dev->is_virtfn) {
+		struct pci_dev *pdev = dev->physfn;
 
-	if (!dev->is_physfn) {
-		rc = ats_alloc_one(dev, ps);
-		if (rc)
-			return rc;
+		if (pdev->ats->stu != ps)
+			return -EINVAL;
+	} else {
+		dev->ats->stu = ps;
+		ctrl |= PCI_ATS_CTRL_STU(dev->ats->stu - PCI_ATS_MIN_STU);
 	}
-
-	ctrl = PCI_ATS_CTRL_ENABLE;
-	if (!dev->is_virtfn)
-		ctrl |= PCI_ATS_CTRL_STU(ps - PCI_ATS_MIN_STU);
 	pci_write_config_word(dev, dev->ats->pos + PCI_ATS_CTRL, ctrl);
 
 	dev->ats->is_enabled = 1;
-
 	return 0;
 }
 EXPORT_SYMBOL_GPL(pci_enable_ats);
@@ -112,19 +105,6 @@ void pci_disable_ats(struct pci_dev *dev)
 	pci_write_config_word(dev, dev->ats->pos + PCI_ATS_CTRL, ctrl);
 
 	dev->ats->is_enabled = 0;
-
-	if (dev->is_physfn || dev->is_virtfn) {
-		struct pci_dev *pdev = dev->is_physfn ? dev : dev->physfn;
-
-		mutex_lock(&pdev->sriov->lock);
-		pdev->ats->ref_cnt--;
-		if (!pdev->ats->ref_cnt)
-			ats_free_one(pdev);
-		mutex_unlock(&pdev->sriov->lock);
-	}
-
-	if (!dev->is_physfn)
-		ats_free_one(dev);
 }
 EXPORT_SYMBOL_GPL(pci_disable_ats);
 
@@ -140,7 +120,6 @@ void pci_restore_ats_state(struct pci_dev *dev)
 	ctrl = PCI_ATS_CTRL_ENABLE;
 	if (!dev->is_virtfn)
 		ctrl |= PCI_ATS_CTRL_STU(dev->ats->stu - PCI_ATS_MIN_STU);
-
 	pci_write_config_word(dev, dev->ats->pos + PCI_ATS_CTRL, ctrl);
 }
 EXPORT_SYMBOL_GPL(pci_restore_ats_state);
@@ -159,23 +138,13 @@ EXPORT_SYMBOL_GPL(pci_restore_ats_state);
  */
 int pci_ats_queue_depth(struct pci_dev *dev)
 {
-	int pos;
-	u16 cap;
-
 	if (dev->is_virtfn)
 		return 0;
 
 	if (dev->ats)
 		return dev->ats->qdep;
 
-	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ATS);
-	if (!pos)
-		return -ENODEV;
-
-	pci_read_config_word(dev, pos + PCI_ATS_CAP, &cap);
-
-	return PCI_ATS_CAP_QDEP(cap) ? PCI_ATS_CAP_QDEP(cap) :
-				       PCI_ATS_MAX_QDEP;
+	return -ENODEV;
 }
 EXPORT_SYMBOL_GPL(pci_ats_queue_depth);
 
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index cefd636..c206398 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1540,6 +1540,9 @@ static void pci_init_capabilities(struct pci_dev *dev)
 	/* Single Root I/O Virtualization */
 	pci_iov_init(dev);
 
+	/* Address Translation Services */
+	pci_ats_init(dev);
+
 	/* Enable ACS P2P upstream forwarding */
 	pci_enable_acs(dev);
 }
diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index 8a280e9..27617b8 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -26,6 +26,7 @@ static void pci_stop_dev(struct pci_dev *dev)
 		dev->is_added = 0;
 	}
 
+	pci_ats_free(dev);
 	if (dev->bus->self)
 		pcie_aspm_exit_link_state(dev);
 }
diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h
index 7203178..d2e8170 100644
--- a/include/linux/pci-ats.h
+++ b/include/linux/pci-ats.h
@@ -8,7 +8,6 @@ struct pci_ats {
 	int pos;        /* capability position */
 	int stu;        /* Smallest Translation Unit */
 	int qdep;       /* Invalidate Queue Depth */
-	int ref_cnt;    /* Physical Function reference count */
 	unsigned int is_enabled:1;      /* Enable bit is set */
 };
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 8a0321a..5c277f1 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1294,6 +1294,15 @@ int  ht_create_irq(struct pci_dev *dev, int idx);
 void ht_destroy_irq(unsigned int irq);
 #endif /* CONFIG_HT_IRQ */
 
+#ifdef CONFIG_PCI_ATS
+/* Address Translation Service */
+void pci_ats_init(struct pci_dev *dev);
+void pci_ats_free(struct pci_dev *dev);
+#else
+static inline void pci_ats_init(struct pci_dev *) { }
+static inline void pci_ats_free(struct pci_dev *) { }
+#endif
+
 void pci_cfg_access_lock(struct pci_dev *dev);
 bool pci_cfg_access_trylock(struct pci_dev *dev);
 void pci_cfg_access_unlock(struct pci_dev *dev);

[PATCH 2/8] PCI: Embed ATS info directly into struct pci_dev

[Bjorn Helgaas]

There are only 33 bits of information in the pci_ats struct, and we
previously allocated it dynamically and maintained a pointer to it from the
pci_dev struct.

Move the ATS fields into struct pci_dev so we don't need the pointer and we
don't have to allocate anything extra.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/ats.c       |   53 ++++++++++++++++-------------------------------
 include/linux/pci-ats.h |    9 +-------
 include/linux/pci.h     |    7 ++++--
 3 files changed, 23 insertions(+), 46 deletions(-)

diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index 923759f..2f1fb9c 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -21,27 +21,15 @@ static void ats_alloc_one(struct pci_dev *dev)
 {
 	int pos;
 	u16 cap;
-	struct pci_ats *ats;
 
 	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ATS);
 	if (!pos)
 		return;
 
-	ats = kzalloc(sizeof(*ats), GFP_KERNEL);
-	if (!ats)
-		return;
-
-	ats->pos = pos;
-	pci_read_config_word(dev, pos + PCI_ATS_CAP, &cap);
-	ats->qdep = PCI_ATS_CAP_QDEP(cap) ? PCI_ATS_CAP_QDEP(cap) :
+	dev->ats_cap = pos;
+	pci_read_config_word(dev, dev->ats_cap + PCI_ATS_CAP, &cap);
+	dev->ats_qdep = PCI_ATS_CAP_QDEP(cap) ? PCI_ATS_CAP_QDEP(cap) :
 					    PCI_ATS_MAX_QDEP;
-	dev->ats = ats;
-}
-
-static void ats_free_one(struct pci_dev *dev)
-{
-	kfree(dev->ats);
-	dev->ats = NULL;
 }
 
 void pci_ats_init(struct pci_dev *dev)
@@ -49,11 +37,6 @@ void pci_ats_init(struct pci_dev *dev)
 	ats_alloc_one(dev);
 }
 
-void pci_ats_free(struct pci_dev *dev)
-{
-	ats_free_one(dev);
-}
-
 /**
  * pci_enable_ats - enable the ATS capability
  * @dev: the PCI device
@@ -65,9 +48,9 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
 {
 	u16 ctrl;
 
-	BUG_ON(dev->ats && dev->ats->is_enabled);
+	BUG_ON(dev->ats_cap && dev->ats_enabled);
 
-	if (!dev->ats)
+	if (!dev->ats_cap)
 		return -EINVAL;
 
 	if (ps < PCI_ATS_MIN_STU)
@@ -77,15 +60,15 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
 	if (dev->is_virtfn) {
 		struct pci_dev *pdev = dev->physfn;
 
-		if (pdev->ats->stu != ps)
+		if (pdev->ats_stu != ps)
 			return -EINVAL;
 	} else {
-		dev->ats->stu = ps;
-		ctrl |= PCI_ATS_CTRL_STU(dev->ats->stu - PCI_ATS_MIN_STU);
+		dev->ats_stu = ps;
+		ctrl |= PCI_ATS_CTRL_STU(dev->ats_stu - PCI_ATS_MIN_STU);
 	}
-	pci_write_config_word(dev, dev->ats->pos + PCI_ATS_CTRL, ctrl);
+	pci_write_config_word(dev, dev->ats_cap + PCI_ATS_CTRL, ctrl);
 
-	dev->ats->is_enabled = 1;
+	dev->ats_enabled = 1;
 	return 0;
 }
 EXPORT_SYMBOL_GPL(pci_enable_ats);
@@ -98,13 +81,13 @@ void pci_disable_ats(struct pci_dev *dev)
 {
 	u16 ctrl;
 
-	BUG_ON(!dev->ats || !dev->ats->is_enabled);
+	BUG_ON(!dev->ats_cap || !dev->ats_enabled);
 
-	pci_read_config_word(dev, dev->ats->pos + PCI_ATS_CTRL, &ctrl);
+	pci_read_config_word(dev, dev->ats_cap + PCI_ATS_CTRL, &ctrl);
 	ctrl &= ~PCI_ATS_CTRL_ENABLE;
-	pci_write_config_word(dev, dev->ats->pos + PCI_ATS_CTRL, ctrl);
+	pci_write_config_word(dev, dev->ats_cap + PCI_ATS_CTRL, ctrl);
 
-	dev->ats->is_enabled = 0;
+	dev->ats_enabled = 0;
 }
 EXPORT_SYMBOL_GPL(pci_disable_ats);
 
@@ -119,8 +102,8 @@ void pci_restore_ats_state(struct pci_dev *dev)
 
 	ctrl = PCI_ATS_CTRL_ENABLE;
 	if (!dev->is_virtfn)
-		ctrl |= PCI_ATS_CTRL_STU(dev->ats->stu - PCI_ATS_MIN_STU);
-	pci_write_config_word(dev, dev->ats->pos + PCI_ATS_CTRL, ctrl);
+		ctrl |= PCI_ATS_CTRL_STU(dev->ats_stu - PCI_ATS_MIN_STU);
+	pci_write_config_word(dev, dev->ats_cap + PCI_ATS_CTRL, ctrl);
 }
 EXPORT_SYMBOL_GPL(pci_restore_ats_state);
 
@@ -141,8 +124,8 @@ int pci_ats_queue_depth(struct pci_dev *dev)
 	if (dev->is_virtfn)
 		return 0;
 
-	if (dev->ats)
-		return dev->ats->qdep;
+	if (dev->ats_cap)
+		return dev->ats_qdep;
 
 	return -ENODEV;
 }
diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h
index d2e8170..5d81d47 100644
--- a/include/linux/pci-ats.h
+++ b/include/linux/pci-ats.h
@@ -4,13 +4,6 @@
 #include <linux/pci.h>
 
 /* Address Translation Service */
-struct pci_ats {
-	int pos;        /* capability position */
-	int stu;        /* Smallest Translation Unit */
-	int qdep;       /* Invalidate Queue Depth */
-	unsigned int is_enabled:1;      /* Enable bit is set */
-};
-
 #ifdef CONFIG_PCI_ATS
 
 int pci_enable_ats(struct pci_dev *dev, int ps);
@@ -25,7 +18,7 @@ int pci_ats_queue_depth(struct pci_dev *dev);
  */
 static inline int pci_ats_enabled(struct pci_dev *dev)
 {
-	return dev->ats && dev->ats->is_enabled;
+	return dev->ats_cap && dev->ats_enabled;
 }
 
 #else /* CONFIG_PCI_ATS */
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 5c277f1..ba4a23b 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -343,6 +343,7 @@ struct pci_dev {
 	unsigned int	msi_enabled:1;
 	unsigned int	msix_enabled:1;
 	unsigned int	ari_enabled:1;	/* ARI forwarding */
+	unsigned int	ats_enabled:1;	/* Address Translation Service */
 	unsigned int	is_managed:1;
 	unsigned int    needs_freset:1; /* Dev requires fundamental reset */
 	unsigned int	state_saved:1;
@@ -375,7 +376,9 @@ struct pci_dev {
 		struct pci_sriov *sriov;	/* SR-IOV capability related */
 		struct pci_dev *physfn;	/* the PF this VF is associated with */
 	};
-	struct pci_ats	*ats;	/* Address Translation Service */
+	int		ats_cap;	/* ATS Capability offset */
+	int		ats_stu;	/* ATS Smallest Translation Unit */
+	int		ats_qdep;	/* ATS Invalidate Queue Depth */
 #endif
 	phys_addr_t rom; /* Physical address of ROM if it's not from the BAR */
 	size_t romlen; /* Length of ROM if it's not from the BAR */
@@ -1297,10 +1300,8 @@ void ht_destroy_irq(unsigned int irq);
 #ifdef CONFIG_PCI_ATS
 /* Address Translation Service */
 void pci_ats_init(struct pci_dev *dev);
-void pci_ats_free(struct pci_dev *dev);
 #else
 static inline void pci_ats_init(struct pci_dev *) { }
-static inline void pci_ats_free(struct pci_dev *) { }
 #endif
 
 void pci_cfg_access_lock(struct pci_dev *dev);

[PATCH 3/8] PCI: Reduce size of ATS structure elements

[Bjorn Helgaas]

The extended capabilities list is linked with 12-bit pointers, and the ATS
Smallest Translation Unit and Invalidate Queue Depth fields are both 5
bits.

Use u16 and u8 to hold the extended capability address and the stu and qdep
values.  No functional change.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 include/linux/pci.h |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/pci.h b/include/linux/pci.h
index ba4a23b..1997853 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -376,9 +376,9 @@ struct pci_dev {
 		struct pci_sriov *sriov;	/* SR-IOV capability related */
 		struct pci_dev *physfn;	/* the PF this VF is associated with */
 	};
-	int		ats_cap;	/* ATS Capability offset */
-	int		ats_stu;	/* ATS Smallest Translation Unit */
-	int		ats_qdep;	/* ATS Invalidate Queue Depth */
+	u16		ats_cap;	/* ATS Capability offset */
+	u8		ats_stu;	/* ATS Smallest Translation Unit */
+	u8		ats_qdep;	/* ATS Invalidate Queue Depth */
 #endif
 	phys_addr_t rom; /* Physical address of ROM if it's not from the BAR */
 	size_t romlen; /* Length of ROM if it's not from the BAR */

[PATCH 4/8] PCI: Rationalize pci_ats_queue_depth() error checking

[Bjorn Helgaas]

We previously returned -ENODEV for devices that don't support ATS (except
that we always returned 0 for VFs, whether or not they support ATS).

For consistency, always return -EINVAL (not -ENODEV) if the device doesn't
support ATS.  Return zero for VFs that support ATS.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/ats.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index 2f1fb9c..ea459b3 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -121,13 +121,13 @@ EXPORT_SYMBOL_GPL(pci_restore_ats_state);
  */
 int pci_ats_queue_depth(struct pci_dev *dev)
 {
+	if (!dev->ats_cap)
+		return -EINVAL;
+
 	if (dev->is_virtfn)
 		return 0;
 
-	if (dev->ats_cap)
-		return dev->ats_qdep;
-
-	return -ENODEV;
+	return dev->ats_qdep;
 }
 EXPORT_SYMBOL_GPL(pci_ats_queue_depth);
 

[PATCH 5/8] PCI: Inline the ATS setup code into pci_ats_init()

[Bjorn Helgaas]

The ATS setup code in ats_alloc_one() is only used by pci_ats_init(), so
inline it there.  No functional change.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/ats.c |    7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index ea459b3..fe59182 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -17,7 +17,7 @@
 
 #include "pci.h"
 
-static void ats_alloc_one(struct pci_dev *dev)
+void pci_ats_init(struct pci_dev *dev)
 {
 	int pos;
 	u16 cap;
@@ -32,11 +32,6 @@ static void ats_alloc_one(struct pci_dev *dev)
 					    PCI_ATS_MAX_QDEP;
 }
 
-void pci_ats_init(struct pci_dev *dev)
-{
-	ats_alloc_one(dev);
-}
-
 /**
  * pci_enable_ats - enable the ATS capability
  * @dev: the PCI device

[PATCH 6/8] PCI: Use pci_physfn() rather than looking up physfn by hand

[Bjorn Helgaas]

Use the pci_physfn() helper rather than looking up physfn by hand.
No functional change.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/ats.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index fe59182..c35de8e 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -53,9 +53,7 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
 
 	ctrl = PCI_ATS_CTRL_ENABLE;
 	if (dev->is_virtfn) {
-		struct pci_dev *pdev = dev->physfn;
-
-		if (pdev->ats_stu != ps)
+		if (pci_physfn(dev)->ats_stu != ps)
 			return -EINVAL;
 	} else {
 		dev->ats_stu = ps;

[PATCH 7/8] PCI: Clean up ATS error handling

[Bjorn Helgaas]

There's no need to BUG() if we enable ATS when it's already enabled.  We
don't need to BUG() when disabling ATS on a device that doesn't support ATS
or if it's already disabled.  If ATS is enabled, certainly we found an ATS
capability in the past, so it should still be there now.

Clean up these error paths.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/ats.c |    7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index c35de8e..9069126 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -43,8 +43,6 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
 {
 	u16 ctrl;
 
-	BUG_ON(dev->ats_cap && dev->ats_enabled);
-
 	if (!dev->ats_cap)
 		return -EINVAL;
 
@@ -74,7 +72,8 @@ void pci_disable_ats(struct pci_dev *dev)
 {
 	u16 ctrl;
 
-	BUG_ON(!dev->ats_cap || !dev->ats_enabled);
+	if (!dev->ats_cap)
+		return;
 
 	pci_read_config_word(dev, dev->ats_cap + PCI_ATS_CTRL, &ctrl);
 	ctrl &= ~PCI_ATS_CTRL_ENABLE;
@@ -90,8 +89,6 @@ void pci_restore_ats_state(struct pci_dev *dev)
 
 	if (!pci_ats_enabled(dev))
 		return;
-	if (!pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ATS))
-		BUG();
 
 	ctrl = PCI_ATS_CTRL_ENABLE;
 	if (!dev->is_virtfn)

[PATCH 8/8] PCI: Move ATS declarations to linux/pci.h so they're all together

[Bjorn Helgaas]

Move ATS declarations to linux/pci.h so they're all in one place.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 include/linux/pci-ats.h |   41 -----------------------------------------
 include/linux/pci.h     |    8 ++++++++
 2 files changed, 8 insertions(+), 41 deletions(-)

diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h
index 5d81d47..57e0b82 100644
--- a/include/linux/pci-ats.h
+++ b/include/linux/pci-ats.h
@@ -3,47 +3,6 @@
 
 #include <linux/pci.h>
 
-/* Address Translation Service */
-#ifdef CONFIG_PCI_ATS
-
-int pci_enable_ats(struct pci_dev *dev, int ps);
-void pci_disable_ats(struct pci_dev *dev);
-int pci_ats_queue_depth(struct pci_dev *dev);
-
-/**
- * pci_ats_enabled - query the ATS status
- * @dev: the PCI device
- *
- * Returns 1 if ATS capability is enabled, or 0 if not.
- */
-static inline int pci_ats_enabled(struct pci_dev *dev)
-{
-	return dev->ats_cap && dev->ats_enabled;
-}
-
-#else /* CONFIG_PCI_ATS */
-
-static inline int pci_enable_ats(struct pci_dev *dev, int ps)
-{
-	return -ENODEV;
-}
-
-static inline void pci_disable_ats(struct pci_dev *dev)
-{
-}
-
-static inline int pci_ats_queue_depth(struct pci_dev *dev)
-{
-	return -ENODEV;
-}
-
-static inline int pci_ats_enabled(struct pci_dev *dev)
-{
-	return 0;
-}
-
-#endif /* CONFIG_PCI_ATS */
-
 #ifdef CONFIG_PCI_PRI
 
 int pci_enable_pri(struct pci_dev *pdev, u32 reqs);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 1997853..744a5e9 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1300,8 +1300,16 @@ void ht_destroy_irq(unsigned int irq);
 #ifdef CONFIG_PCI_ATS
 /* Address Translation Service */
 void pci_ats_init(struct pci_dev *dev);
+int pci_enable_ats(struct pci_dev *dev, int ps);
+void pci_disable_ats(struct pci_dev *dev);
+int pci_ats_queue_depth(struct pci_dev *dev);
+static inline int pci_ats_enabled(struct pci_dev *dev) { return dev->ats_enabled; }
 #else
 static inline void pci_ats_init(struct pci_dev *) { }
+static inline int pci_enable_ats(struct pci_dev *, int) { return -ENODEV; }
+static inline void pci_disable_ats(struct pci_dev *) { }
+static inline int pci_ats_queue_depth(struct pci_dev *) { return -ENODEV; }
+static inline int pci_ats_enabled(struct pci_dev *dev) { return 0; }
 #endif
 
 void pci_cfg_access_lock(struct pci_dev *dev);

Re: [PATCH 1/8] PCI: Allocate ATS struct during enumeration

[Bjorn Helgaas]

On Fri, Jul 17, 2015 at 04:31:52PM -0500, Bjorn Helgaas wrote:
> Previously, we allocated pci_ats structures when an IOMMU driver called
> pci_enable_ats().  An SR-IOV VF shares the STU setting with its PF, so when
> enabling ATS on the VF, we allocated a pci_ats struct for the PF if it
> didn't already have one.  We held the sriov->lock to serialize threads
> concurrently enabling ATS on several VFS so only one would allocate the PF
> pci_ats.
> 
> Gregor reported a deadlock here:
> 
>   pci_enable_sriov
>     sriov_enable
>       virtfn_add
>         mutex_lock(dev->sriov->lock)      # acquire sriov->lock
>         pci_device_add
>           device_add
>             BUS_NOTIFY_ADD_DEVICE notifier chain
>             iommu_bus_notifier
>               amd_iommu_add_device        # iommu_ops.add_device
>                 init_iommu_group
>                   iommu_group_get_for_dev
>                     iommu_group_add_device
>                       __iommu_attach_device
>                         amd_iommu_attach_device  # iommu_ops.attach_device
>                           attach_device
>                             pci_enable_ats
>                               mutex_lock(dev->sriov->lock) # deadlock
> 
> There's no reason to delay allocating the pci_ats struct, and if we
> allocate it for each device at enumeration-time, there's no need for
> locking in pci_enable_ats().
> 
> Allocate pci_ats struct during enumeration, when we initialize other
> capabilities.
> 
> Note that this implementation requires ATS to be enabled on the PF first,
> before on any of the VFs because the PF controls the STU for all the VFs.
> 
> Link: http://permalink.gmane.org/gmane.linux.kernel.iommu/9433
> Reported-by: Gregor Dick <gdick@solarflare.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/ats.c       |   81 +++++++++++++++--------------------------------
>  drivers/pci/probe.c     |    3 ++
>  drivers/pci/remove.c    |    1 +
>  include/linux/pci-ats.h |    1 -
>  include/linux/pci.h     |    9 +++++
>  5 files changed, 38 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> index a8099d4..923759f 100644
> --- a/drivers/pci/ats.c
> +++ b/drivers/pci/ats.c
> @@ -17,7 +17,7 @@
>  
>  #include "pci.h"
>  
> -static int ats_alloc_one(struct pci_dev *dev, int ps)
> +static void ats_alloc_one(struct pci_dev *dev)
>  {
>  	int pos;
>  	u16 cap;
> @@ -25,20 +25,17 @@ static int ats_alloc_one(struct pci_dev *dev, int ps)
>  
>  	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ATS);
>  	if (!pos)
> -		return -ENODEV;
> +		return;
>  
>  	ats = kzalloc(sizeof(*ats), GFP_KERNEL);
>  	if (!ats)
> -		return -ENOMEM;
> +		return;
>  
>  	ats->pos = pos;
> -	ats->stu = ps;
>  	pci_read_config_word(dev, pos + PCI_ATS_CAP, &cap);
>  	ats->qdep = PCI_ATS_CAP_QDEP(cap) ? PCI_ATS_CAP_QDEP(cap) :
>  					    PCI_ATS_MAX_QDEP;
>  	dev->ats = ats;
> -
> -	return 0;
>  }
>  
>  static void ats_free_one(struct pci_dev *dev)
> @@ -47,6 +44,16 @@ static void ats_free_one(struct pci_dev *dev)
>  	dev->ats = NULL;
>  }
>  
> +void pci_ats_init(struct pci_dev *dev)
> +{
> +	ats_alloc_one(dev);
> +}
> +
> +void pci_ats_free(struct pci_dev *dev)
> +{
> +	ats_free_one(dev);
> +}
> +
>  /**
>   * pci_enable_ats - enable the ATS capability
>   * @dev: the PCI device
> @@ -56,43 +63,29 @@ static void ats_free_one(struct pci_dev *dev)
>   */
>  int pci_enable_ats(struct pci_dev *dev, int ps)
>  {
> -	int rc;
>  	u16 ctrl;
>  
>  	BUG_ON(dev->ats && dev->ats->is_enabled);
>  
> -	if (ps < PCI_ATS_MIN_STU)
> +	if (!dev->ats)
>  		return -EINVAL;
>  
> -	if (dev->is_physfn || dev->is_virtfn) {
> -		struct pci_dev *pdev = dev->is_physfn ? dev : dev->physfn;
> -
> -		mutex_lock(&pdev->sriov->lock);
> -		if (pdev->ats)
> -			rc = pdev->ats->stu == ps ? 0 : -EINVAL;
> -		else
> -			rc = ats_alloc_one(pdev, ps);
> +	if (ps < PCI_ATS_MIN_STU)
> +		return -EINVAL;
>  
> -		if (!rc)
> -			pdev->ats->ref_cnt++;
> -		mutex_unlock(&pdev->sriov->lock);
> -		if (rc)
> -			return rc;
> -	}
> +	ctrl = PCI_ATS_CTRL_ENABLE;
> +	if (dev->is_virtfn) {
> +		struct pci_dev *pdev = dev->physfn;
>  
> -	if (!dev->is_physfn) {
> -		rc = ats_alloc_one(dev, ps);
> -		if (rc)
> -			return rc;
> +		if (pdev->ats->stu != ps)
> +			return -EINVAL;
> +	} else {
> +		dev->ats->stu = ps;
> +		ctrl |= PCI_ATS_CTRL_STU(dev->ats->stu - PCI_ATS_MIN_STU);
>  	}

I forgot to mention in the changelog that this changes the semantics of
pci_enable_ats(): previously, you could enable ATS on a VF before enabling
it on a PF.  The first call on either a PF or VF established the STU, and
subsequent calls for related VFs or PF would fail if they tried to use a
different STU.  If you disabled ATS on all the PF/VFs, you could start over
with a different STU.

With this patch, you have to enable ATS on a PF first, before enabling it
on any of its VFs.  The VFs must use the same STU as the PF.  And there's
no way to change the STU unless the device is removed and re-enumerated.

I think this is sufficient for current usage, but let me know if it's not.

> -
> -	ctrl = PCI_ATS_CTRL_ENABLE;
> -	if (!dev->is_virtfn)
> -		ctrl |= PCI_ATS_CTRL_STU(ps - PCI_ATS_MIN_STU);
>  	pci_write_config_word(dev, dev->ats->pos + PCI_ATS_CTRL, ctrl);
>  
>  	dev->ats->is_enabled = 1;
> -
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(pci_enable_ats);
> @@ -112,19 +105,6 @@ void pci_disable_ats(struct pci_dev *dev)
>  	pci_write_config_word(dev, dev->ats->pos + PCI_ATS_CTRL, ctrl);
>  
>  	dev->ats->is_enabled = 0;
> -
> -	if (dev->is_physfn || dev->is_virtfn) {
> -		struct pci_dev *pdev = dev->is_physfn ? dev : dev->physfn;
> -
> -		mutex_lock(&pdev->sriov->lock);
> -		pdev->ats->ref_cnt--;
> -		if (!pdev->ats->ref_cnt)
> -			ats_free_one(pdev);
> -		mutex_unlock(&pdev->sriov->lock);
> -	}
> -
> -	if (!dev->is_physfn)
> -		ats_free_one(dev);
>  }
>  EXPORT_SYMBOL_GPL(pci_disable_ats);
>  
> @@ -140,7 +120,6 @@ void pci_restore_ats_state(struct pci_dev *dev)
>  	ctrl = PCI_ATS_CTRL_ENABLE;
>  	if (!dev->is_virtfn)
>  		ctrl |= PCI_ATS_CTRL_STU(dev->ats->stu - PCI_ATS_MIN_STU);
> -
>  	pci_write_config_word(dev, dev->ats->pos + PCI_ATS_CTRL, ctrl);
>  }
>  EXPORT_SYMBOL_GPL(pci_restore_ats_state);
> @@ -159,23 +138,13 @@ EXPORT_SYMBOL_GPL(pci_restore_ats_state);
>   */
>  int pci_ats_queue_depth(struct pci_dev *dev)
>  {
> -	int pos;
> -	u16 cap;
> -
>  	if (dev->is_virtfn)
>  		return 0;
>  
>  	if (dev->ats)
>  		return dev->ats->qdep;
>  
> -	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ATS);
> -	if (!pos)
> -		return -ENODEV;
> -
> -	pci_read_config_word(dev, pos + PCI_ATS_CAP, &cap);
> -
> -	return PCI_ATS_CAP_QDEP(cap) ? PCI_ATS_CAP_QDEP(cap) :
> -				       PCI_ATS_MAX_QDEP;
> +	return -ENODEV;
>  }
>  EXPORT_SYMBOL_GPL(pci_ats_queue_depth);
>  
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index cefd636..c206398 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1540,6 +1540,9 @@ static void pci_init_capabilities(struct pci_dev *dev)
>  	/* Single Root I/O Virtualization */
>  	pci_iov_init(dev);
>  
> +	/* Address Translation Services */
> +	pci_ats_init(dev);
> +
>  	/* Enable ACS P2P upstream forwarding */
>  	pci_enable_acs(dev);
>  }
> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
> index 8a280e9..27617b8 100644
> --- a/drivers/pci/remove.c
> +++ b/drivers/pci/remove.c
> @@ -26,6 +26,7 @@ static void pci_stop_dev(struct pci_dev *dev)
>  		dev->is_added = 0;
>  	}
>  
> +	pci_ats_free(dev);
>  	if (dev->bus->self)
>  		pcie_aspm_exit_link_state(dev);
>  }
> diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h
> index 7203178..d2e8170 100644
> --- a/include/linux/pci-ats.h
> +++ b/include/linux/pci-ats.h
> @@ -8,7 +8,6 @@ struct pci_ats {
>  	int pos;        /* capability position */
>  	int stu;        /* Smallest Translation Unit */
>  	int qdep;       /* Invalidate Queue Depth */
> -	int ref_cnt;    /* Physical Function reference count */
>  	unsigned int is_enabled:1;      /* Enable bit is set */
>  };
>  
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 8a0321a..5c277f1 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1294,6 +1294,15 @@ int  ht_create_irq(struct pci_dev *dev, int idx);
>  void ht_destroy_irq(unsigned int irq);
>  #endif /* CONFIG_HT_IRQ */
>  
> +#ifdef CONFIG_PCI_ATS
> +/* Address Translation Service */
> +void pci_ats_init(struct pci_dev *dev);
> +void pci_ats_free(struct pci_dev *dev);
> +#else
> +static inline void pci_ats_init(struct pci_dev *) { }
> +static inline void pci_ats_free(struct pci_dev *) { }
> +#endif
> +
>  void pci_cfg_access_lock(struct pci_dev *dev);
>  bool pci_cfg_access_trylock(struct pci_dev *dev);
>  void pci_cfg_access_unlock(struct pci_dev *dev);
> 

Re: [PATCH 1/8] PCI: Allocate ATS struct during enumeration

[Joerg Roedel]

Hi Bjorn,

On Fri, Jul 17, 2015 at 04:31:52PM -0500, Bjorn Helgaas wrote:
> -static int ats_alloc_one(struct pci_dev *dev, int ps)
> +static void ats_alloc_one(struct pci_dev *dev)
>  {
>  	int pos;
>  	u16 cap;
> @@ -25,20 +25,17 @@ static int ats_alloc_one(struct pci_dev *dev, int ps)
>  
>  	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ATS);
>  	if (!pos)
> -		return -ENODEV;
> +		return;
>  
>  	ats = kzalloc(sizeof(*ats), GFP_KERNEL);
>  	if (!ats)
> -		return -ENOMEM;
> +		return;

I think we should print a warning here when the allocation fails.
Otherwise the user wonders why ATS can't be used despite the available
capability.

> -	if (dev->is_physfn || dev->is_virtfn) {
> -		struct pci_dev *pdev = dev->is_physfn ? dev : dev->physfn;
> -
> -		mutex_lock(&pdev->sriov->lock);
> -		if (pdev->ats)
> -			rc = pdev->ats->stu == ps ? 0 : -EINVAL;
> -		else
> -			rc = ats_alloc_one(pdev, ps);
> +	if (ps < PCI_ATS_MIN_STU)
> +		return -EINVAL;
>  
> -		if (!rc)
> -			pdev->ats->ref_cnt++;
> -		mutex_unlock(&pdev->sriov->lock);
> -		if (rc)
> -			return rc;
> -	}
> +	ctrl = PCI_ATS_CTRL_ENABLE;
> +	if (dev->is_virtfn) {
> +		struct pci_dev *pdev = dev->physfn;
>  
> -	if (!dev->is_physfn) {
> -		rc = ats_alloc_one(dev, ps);
> -		if (rc)
> -			return rc;
> +		if (pdev->ats->stu != ps)
> +			return -EINVAL;
> +	} else {
> +		dev->ats->stu = ps;
> +		ctrl |= PCI_ATS_CTRL_STU(dev->ats->stu - PCI_ATS_MIN_STU);
>  	}

Okay, enabling ATS for the virt_fn will now fail when it is not
already enabled for the phys_fn. The drivers probe function, which might
enable SR-IOV, runs after BUS_NOTIFY_ADD_DEVICE has finished, so this
should be safe.

But I think a comment explaining these dependencies would be good here.


	Joerg

Re: [PATCH 2/8] PCI: Embed ATS info directly into struct pci_dev

[Joerg Roedel]

On Fri, Jul 17, 2015 at 04:32:00PM -0500, Bjorn Helgaas wrote:
> @@ -98,13 +81,13 @@ void pci_disable_ats(struct pci_dev *dev)
>  {
>  	u16 ctrl;
>  
> -	BUG_ON(!dev->ats || !dev->ats->is_enabled);
> +	BUG_ON(!dev->ats_cap || !dev->ats_enabled);

So since dev->ats_enabled can only get set to 1 of dev->ats_cap != 0,
wouldn't it be sufficient to just do a BUG_ON(!dev->ats_enabled)?

>  static inline int pci_ats_enabled(struct pci_dev *dev)
>  {
> -	return dev->ats && dev->ats->is_enabled;
> +	return dev->ats_cap && dev->ats_enabled;
>  }

Here too, can't we just return dev->ats_enabled?



	Joerg

Re: [PATCH 4/8] PCI: Rationalize pci_ats_queue_depth() error checking

[Joerg Roedel]

On Fri, Jul 17, 2015 at 04:32:16PM -0500, Bjorn Helgaas wrote:
> We previously returned -ENODEV for devices that don't support ATS (except
> that we always returned 0 for VFs, whether or not they support ATS).
> 
> For consistency, always return -EINVAL (not -ENODEV) if the device doesn't
> support ATS.  Return zero for VFs that support ATS.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/ats.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> index 2f1fb9c..ea459b3 100644
> --- a/drivers/pci/ats.c
> +++ b/drivers/pci/ats.c
> @@ -121,13 +121,13 @@ EXPORT_SYMBOL_GPL(pci_restore_ats_state);
>   */
>  int pci_ats_queue_depth(struct pci_dev *dev)
>  {
> +	if (!dev->ats_cap)
> +		return -EINVAL;
> +
>  	if (dev->is_virtfn)
>  		return 0;
>  
> -	if (dev->ats_cap)
> -		return dev->ats_qdep;
> -
> -	return -ENODEV;
> +	return dev->ats_qdep;
>  }
>  EXPORT_SYMBOL_GPL(pci_ats_queue_depth);

Reviewed-by: Joerg Roedel <jroedel@suse.de>

Re: [PATCH 5/8] PCI: Inline the ATS setup code into pci_ats_init()

[Joerg Roedel]

On Fri, Jul 17, 2015 at 04:32:23PM -0500, Bjorn Helgaas wrote:
> The ATS setup code in ats_alloc_one() is only used by pci_ats_init(), so
> inline it there.  No functional change.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/ats.c |    7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> index ea459b3..fe59182 100644
> --- a/drivers/pci/ats.c
> +++ b/drivers/pci/ats.c
> @@ -17,7 +17,7 @@
>  
>  #include "pci.h"
>  
> -static void ats_alloc_one(struct pci_dev *dev)
> +void pci_ats_init(struct pci_dev *dev)
>  {
>  	int pos;
>  	u16 cap;
> @@ -32,11 +32,6 @@ static void ats_alloc_one(struct pci_dev *dev)
>  					    PCI_ATS_MAX_QDEP;
>  }
>  
> -void pci_ats_init(struct pci_dev *dev)
> -{
> -	ats_alloc_one(dev);
> -}
> -
>  /**
>   * pci_enable_ats - enable the ATS capability
>   * @dev: the PCI device

Reviewed-by: Joerg Roedel <jroedel@suse.de>

Re: [PATCH 6/8] PCI: Use pci_physfn() rather than looking up physfn by hand

[Joerg Roedel]

On Fri, Jul 17, 2015 at 04:32:32PM -0500, Bjorn Helgaas wrote:
> Use the pci_physfn() helper rather than looking up physfn by hand.
> No functional change.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/ats.c |    4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> index fe59182..c35de8e 100644
> --- a/drivers/pci/ats.c
> +++ b/drivers/pci/ats.c
> @@ -53,9 +53,7 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
>  
>  	ctrl = PCI_ATS_CTRL_ENABLE;
>  	if (dev->is_virtfn) {
> -		struct pci_dev *pdev = dev->physfn;
> -
> -		if (pdev->ats_stu != ps)
> +		if (pci_physfn(dev)->ats_stu != ps)
>  			return -EINVAL;
>  	} else {
>  		dev->ats_stu = ps;

Reviewed-by: Joerg Roedel <jroedel@suse.de>

Re: [PATCH 7/8] PCI: Clean up ATS error handling

[Joerg Roedel]

On Fri, Jul 17, 2015 at 04:32:40PM -0500, Bjorn Helgaas wrote:
> There's no need to BUG() if we enable ATS when it's already enabled.  We
> don't need to BUG() when disabling ATS on a device that doesn't support ATS
> or if it's already disabled.  If ATS is enabled, certainly we found an ATS
> capability in the past, so it should still be there now.
> 
> Clean up these error paths.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/ats.c |    7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> index c35de8e..9069126 100644
> --- a/drivers/pci/ats.c
> +++ b/drivers/pci/ats.c
> @@ -43,8 +43,6 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
>  {
>  	u16 ctrl;
>  
> -	BUG_ON(dev->ats_cap && dev->ats_enabled);
> -
>  	if (!dev->ats_cap)
>  		return -EINVAL;

Hmm, what happens if pci_enable_ats is called twice for the same
device? Can we change the STU without disabling ATS first, for example?

The function should probably return if it finds ATS already enabled (or
at least WARN when it is enabled and ps != last ps value).



	Joerg

Re: [PATCH 8/8] PCI: Move ATS declarations to linux/pci.h so they're all together

[Joerg Roedel]

On Fri, Jul 17, 2015 at 04:32:47PM -0500, Bjorn Helgaas wrote:
> Move ATS declarations to linux/pci.h so they're all in one place.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  include/linux/pci-ats.h |   41 -----------------------------------------
>  include/linux/pci.h     |    8 ++++++++
>  2 files changed, 8 insertions(+), 41 deletions(-)
> 
> diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h
> index 5d81d47..57e0b82 100644
> --- a/include/linux/pci-ats.h
> +++ b/include/linux/pci-ats.h
> @@ -3,47 +3,6 @@
>  
>  #include <linux/pci.h>
>  
> -/* Address Translation Service */
> -#ifdef CONFIG_PCI_ATS
> -
> -int pci_enable_ats(struct pci_dev *dev, int ps);
> -void pci_disable_ats(struct pci_dev *dev);
> -int pci_ats_queue_depth(struct pci_dev *dev);
> -
> -/**
> - * pci_ats_enabled - query the ATS status
> - * @dev: the PCI device
> - *
> - * Returns 1 if ATS capability is enabled, or 0 if not.
> - */
> -static inline int pci_ats_enabled(struct pci_dev *dev)
> -{
> -	return dev->ats_cap && dev->ats_enabled;
> -}
> -
> -#else /* CONFIG_PCI_ATS */
> -
> -static inline int pci_enable_ats(struct pci_dev *dev, int ps)
> -{
> -	return -ENODEV;
> -}
> -
> -static inline void pci_disable_ats(struct pci_dev *dev)
> -{
> -}
> -
> -static inline int pci_ats_queue_depth(struct pci_dev *dev)
> -{
> -	return -ENODEV;
> -}
> -
> -static inline int pci_ats_enabled(struct pci_dev *dev)
> -{
> -	return 0;
> -}
> -
> -#endif /* CONFIG_PCI_ATS */
> -
>  #ifdef CONFIG_PCI_PRI
>  
>  int pci_enable_pri(struct pci_dev *pdev, u32 reqs);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 1997853..744a5e9 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1300,8 +1300,16 @@ void ht_destroy_irq(unsigned int irq);
>  #ifdef CONFIG_PCI_ATS
>  /* Address Translation Service */
>  void pci_ats_init(struct pci_dev *dev);
> +int pci_enable_ats(struct pci_dev *dev, int ps);
> +void pci_disable_ats(struct pci_dev *dev);
> +int pci_ats_queue_depth(struct pci_dev *dev);
> +static inline int pci_ats_enabled(struct pci_dev *dev) { return dev->ats_enabled; }
>  #else
>  static inline void pci_ats_init(struct pci_dev *) { }
> +static inline int pci_enable_ats(struct pci_dev *, int) { return -ENODEV; }
> +static inline void pci_disable_ats(struct pci_dev *) { }
> +static inline int pci_ats_queue_depth(struct pci_dev *) { return -ENODEV; }
> +static inline int pci_ats_enabled(struct pci_dev *dev) { return 0; }
>  #endif
>  
>  void pci_cfg_access_lock(struct pci_dev *dev);

Reviewed-by: Joerg Roedel <jroedel@suse.de>

Re: [PATCH 3/8] PCI: Reduce size of ATS structure elements

[Joerg Roedel]

On Fri, Jul 17, 2015 at 04:32:08PM -0500, Bjorn Helgaas wrote:
> The extended capabilities list is linked with 12-bit pointers, and the ATS
> Smallest Translation Unit and Invalidate Queue Depth fields are both 5
> bits.
> 
> Use u16 and u8 to hold the extended capability address and the stu and qdep
> values.  No functional change.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  include/linux/pci.h |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index ba4a23b..1997853 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -376,9 +376,9 @@ struct pci_dev {
>  		struct pci_sriov *sriov;	/* SR-IOV capability related */
>  		struct pci_dev *physfn;	/* the PF this VF is associated with */
>  	};
> -	int		ats_cap;	/* ATS Capability offset */
> -	int		ats_stu;	/* ATS Smallest Translation Unit */
> -	int		ats_qdep;	/* ATS Invalidate Queue Depth */
> +	u16		ats_cap;	/* ATS Capability offset */
> +	u8		ats_stu;	/* ATS Smallest Translation Unit */
> +	u8		ats_qdep;	/* ATS Invalidate Queue Depth */
>  #endif
>  	phys_addr_t rom; /* Physical address of ROM if it's not from the BAR */
>  	size_t romlen; /* Length of ROM if it's not from the BAR */

I was about to suggest to fold this into the previous patch. But since
the elements in the old ats-struct were also ints, it makes sense to
have this as a seperate patch :)


Reviewed-by: Joerg Roedel <jroedel@suse.de>

Re: [PATCH 1/8] PCI: Allocate ATS struct during enumeration

[Bjorn Helgaas]

On Mon, Jul 20, 2015 at 03:55:16PM +0200, Joerg Roedel wrote:
> Hi Bjorn,
> 
> On Fri, Jul 17, 2015 at 04:31:52PM -0500, Bjorn Helgaas wrote:
> > -static int ats_alloc_one(struct pci_dev *dev, int ps)
> > +static void ats_alloc_one(struct pci_dev *dev)
> >  {
> >  	int pos;
> >  	u16 cap;
> > @@ -25,20 +25,17 @@ static int ats_alloc_one(struct pci_dev *dev, int ps)
> >  
> >  	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ATS);
> >  	if (!pos)
> > -		return -ENODEV;
> > +		return;
> >  
> >  	ats = kzalloc(sizeof(*ats), GFP_KERNEL);
> >  	if (!ats)
> > -		return -ENOMEM;
> > +		return;
> 
> I think we should print a warning here when the allocation fails.
> Otherwise the user wonders why ATS can't be used despite the available
> capability.

Good idea, done (although it gets removed right away in the next patch :))
> 
> > -	if (dev->is_physfn || dev->is_virtfn) {
> > -		struct pci_dev *pdev = dev->is_physfn ? dev : dev->physfn;
> > -
> > -		mutex_lock(&pdev->sriov->lock);
> > -		if (pdev->ats)
> > -			rc = pdev->ats->stu == ps ? 0 : -EINVAL;
> > -		else
> > -			rc = ats_alloc_one(pdev, ps);
> > +	if (ps < PCI_ATS_MIN_STU)
> > +		return -EINVAL;
> >  
> > -		if (!rc)
> > -			pdev->ats->ref_cnt++;
> > -		mutex_unlock(&pdev->sriov->lock);
> > -		if (rc)
> > -			return rc;
> > -	}
> > +	ctrl = PCI_ATS_CTRL_ENABLE;
> > +	if (dev->is_virtfn) {
> > +		struct pci_dev *pdev = dev->physfn;
> >  
> > -	if (!dev->is_physfn) {
> > -		rc = ats_alloc_one(dev, ps);
> > -		if (rc)
> > -			return rc;
> > +		if (pdev->ats->stu != ps)
> > +			return -EINVAL;
> > +	} else {
> > +		dev->ats->stu = ps;
> > +		ctrl |= PCI_ATS_CTRL_STU(dev->ats->stu - PCI_ATS_MIN_STU);
> >  	}
> 
> Okay, enabling ATS for the virt_fn will now fail when it is not
> already enabled for the phys_fn. The drivers probe function, which might
> enable SR-IOV, runs after BUS_NOTIFY_ADD_DEVICE has finished, so this
> should be safe.
> 
> But I think a comment explaining these dependencies would be good here.

Comment added, thanks!

Bjorn

Re: [PATCH 7/8] PCI: Clean up ATS error handling

[Bjorn Helgaas]

On Mon, Jul 20, 2015 at 04:24:00PM +0200, Joerg Roedel wrote:
> On Fri, Jul 17, 2015 at 04:32:40PM -0500, Bjorn Helgaas wrote:
> > There's no need to BUG() if we enable ATS when it's already enabled.  We
> > don't need to BUG() when disabling ATS on a device that doesn't support ATS
> > or if it's already disabled.  If ATS is enabled, certainly we found an ATS
> > capability in the past, so it should still be there now.
> > 
> > Clean up these error paths.
> > 
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > ---
> >  drivers/pci/ats.c |    7 ++-----
> >  1 file changed, 2 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> > index c35de8e..9069126 100644
> > --- a/drivers/pci/ats.c
> > +++ b/drivers/pci/ats.c
> > @@ -43,8 +43,6 @@ int pci_enable_ats(struct pci_dev *dev, int ps)
> >  {
> >  	u16 ctrl;
> >  
> > -	BUG_ON(dev->ats_cap && dev->ats_enabled);
> > -
> >  	if (!dev->ats_cap)
> >  		return -EINVAL;
> 
> Hmm, what happens if pci_enable_ats is called twice for the same
> device? Can we change the STU without disabling ATS first, for example?
> 
> The function should probably return if it finds ATS already enabled (or
> at least WARN when it is enabled and ps != last ps value).

It would probably be nicest if:

  - enabling PF fails if ATS is already enabled
  - enabling VF fails if (PF ATS is disabled || PS != PF PS)
  - disabling PF fails if ATS is enabled on any VF

I added the first (which we basically had before in the BUG_ON()), and I
already had the second.

The third is where it gets messy because we have either add a refcount and
associated locking, or we have to iterate through all the VFs.  I did
consider both of those but thought it was a lot of work for the way it's
currently used: it looks like ATS is enabled for every device when it is
enumerated, with a fixed PS, and never changed.

But I guess a refcount with atomic_inc() probably wouldn't be too hard.
Let me poke at that.

Bjorn

Re: [PATCH 2/8] PCI: Embed ATS info directly into struct pci_dev

[Bjorn Helgaas]

On Mon, Jul 20, 2015 at 04:03:18PM +0200, Joerg Roedel wrote:
> On Fri, Jul 17, 2015 at 04:32:00PM -0500, Bjorn Helgaas wrote:
> > @@ -98,13 +81,13 @@ void pci_disable_ats(struct pci_dev *dev)
> >  {
> >  	u16 ctrl;
> >  
> > -	BUG_ON(!dev->ats || !dev->ats->is_enabled);
> > +	BUG_ON(!dev->ats_cap || !dev->ats_enabled);
> 
> So since dev->ats_enabled can only get set to 1 of dev->ats_cap != 0,
> wouldn't it be sufficient to just do a BUG_ON(!dev->ats_enabled)?
> 
> >  static inline int pci_ats_enabled(struct pci_dev *dev)
> >  {
> > -	return dev->ats && dev->ats->is_enabled;
> > +	return dev->ats_cap && dev->ats_enabled;
> >  }
> 
> Here too, can't we just return dev->ats_enabled?

Yep, that makes sense.  I left ats_cap in to try to make it easier to
review, but I removed it later in the series.

Bjorn