From edc90fee916b4f0d14af9c6b5c08666747488ef8 Mon Sep 17 00:00:00 2001 From: Bjorn Helgaas Date: Fri, 17 Jul 2015 15:05:46 -0500 Subject: PCI: Allocate ATS struct during enumeration 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 Signed-off-by: Bjorn Helgaas Reviewed-by: Joerg Roedel --- drivers/pci/ats.c | 98 ++++++++++++++++++++++++------------------------------- 1 file changed, 42 insertions(+), 56 deletions(-) (limited to 'drivers/pci/ats.c') diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c index a8099d4d0c9d..2026f5388796 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,19 @@ 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; + if (!ats) { + dev_warn(&dev->dev, "can't allocate space for ATS state\n"); + 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 +46,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 +65,35 @@ 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 (!dev->ats) + return -EINVAL; + if (ps < PCI_ATS_MIN_STU) return -EINVAL; - if (dev->is_physfn || dev->is_virtfn) { - struct pci_dev *pdev = dev->is_physfn ? dev : dev->physfn; + /* + * Note that enabling ATS on a VF fails unless it's already enabled + * with the same STU on the PF. + */ + ctrl = PCI_ATS_CTRL_ENABLE; + if (dev->is_virtfn) { + struct pci_dev *pdev = 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 (pdev->ats->stu != ps) + return -EINVAL; - if (!rc) - pdev->ats->ref_cnt++; - mutex_unlock(&pdev->sriov->lock); - if (rc) - return rc; - } - - if (!dev->is_physfn) { - rc = ats_alloc_one(dev, ps); - if (rc) - return rc; + atomic_inc(&pdev->ats->ref_cnt); /* count enabled VFs */ + } 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); @@ -107,24 +108,20 @@ void pci_disable_ats(struct pci_dev *dev) BUG_ON(!dev->ats || !dev->ats->is_enabled); + if (atomic_read(&dev->ats->ref_cnt)) + return; /* VFs still enabled */ + + if (dev->is_virtfn) { + struct pci_dev *pdev = dev->physfn; + + atomic_dec(&pdev->ats->ref_cnt); + } + pci_read_config_word(dev, dev->ats->pos + PCI_ATS_CTRL, &ctrl); ctrl &= ~PCI_ATS_CTRL_ENABLE; 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 +137,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 +155,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); -- cgit From d544d75ac96aa1b0a8a378826626a0fbd8ce4380 Mon Sep 17 00:00:00 2001 From: Bjorn Helgaas Date: Fri, 17 Jul 2015 15:15:19 -0500 Subject: PCI: Embed ATS info directly into struct pci_dev The pci_ats struct is small and will get smaller, so I don't think it's worth allocating it separately from the pci_dev struct. Embed the ATS fields directly into struct pci_dev. Signed-off-by: Bjorn Helgaas Reviewed-by: Joerg Roedel --- drivers/pci/ats.c | 61 +++++++++++++++++++------------------------------------ 1 file changed, 21 insertions(+), 40 deletions(-) (limited to 'drivers/pci/ats.c') diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c index 2026f5388796..690ae6e6786c 100644 --- a/drivers/pci/ats.c +++ b/drivers/pci/ats.c @@ -21,29 +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) { - dev_warn(&dev->dev, "can't allocate space for ATS state\n"); - 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) @@ -51,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 @@ -67,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) @@ -83,17 +64,17 @@ 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; - atomic_inc(&pdev->ats->ref_cnt); /* count enabled VFs */ + atomic_inc(&pdev->ats_ref_cnt); /* count enabled VFs */ } 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); @@ -106,22 +87,22 @@ 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); - if (atomic_read(&dev->ats->ref_cnt)) + if (atomic_read(&dev->ats_ref_cnt)) return; /* VFs still enabled */ if (dev->is_virtfn) { struct pci_dev *pdev = dev->physfn; - atomic_dec(&pdev->ats->ref_cnt); + atomic_dec(&pdev->ats_ref_cnt); } - 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); @@ -136,8 +117,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); @@ -158,8 +139,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; } -- cgit From 3c765399524308ab36777a443ce77e19810a97d7 Mon Sep 17 00:00:00 2001 From: Bjorn Helgaas Date: Fri, 17 Jul 2015 15:30:26 -0500 Subject: PCI: Rationalize pci_ats_queue_depth() error checking 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 Reviewed-by: Joerg Roedel --- drivers/pci/ats.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'drivers/pci/ats.c') diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c index 690ae6e6786c..9a98b3a4f983 100644 --- a/drivers/pci/ats.c +++ b/drivers/pci/ats.c @@ -136,13 +136,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); -- cgit From afdd596c42c4540308eae9f4c685d9bccc525cfc Mon Sep 17 00:00:00 2001 From: Bjorn Helgaas Date: Fri, 17 Jul 2015 15:35:18 -0500 Subject: PCI: Inline the ATS setup code into pci_ats_init() 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 Reviewed-by: Joerg Roedel --- drivers/pci/ats.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) (limited to 'drivers/pci/ats.c') diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c index 9a98b3a4f983..95905f3c08f1 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 -- cgit From c39127dbaf6c267f18d1cac14e2b0ecbfaa52d63 Mon Sep 17 00:00:00 2001 From: Bjorn Helgaas Date: Fri, 17 Jul 2015 15:38:13 -0500 Subject: PCI: Use pci_physfn() rather than looking up physfn by hand Use the pci_physfn() helper rather than looking up physfn by hand. No functional change. Signed-off-by: Bjorn Helgaas Reviewed-by: Joerg Roedel --- drivers/pci/ats.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'drivers/pci/ats.c') diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c index 95905f3c08f1..0b5b0ed7a436 100644 --- a/drivers/pci/ats.c +++ b/drivers/pci/ats.c @@ -42,6 +42,7 @@ void pci_ats_init(struct pci_dev *dev) int pci_enable_ats(struct pci_dev *dev, int ps) { u16 ctrl; + struct pci_dev *pdev; BUG_ON(dev->ats_cap && dev->ats_enabled); @@ -57,8 +58,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; - + pdev = pci_physfn(dev); if (pdev->ats_stu != ps) return -EINVAL; @@ -80,6 +80,7 @@ EXPORT_SYMBOL_GPL(pci_enable_ats); */ void pci_disable_ats(struct pci_dev *dev) { + struct pci_dev *pdev; u16 ctrl; BUG_ON(!dev->ats_cap || !dev->ats_enabled); @@ -88,8 +89,7 @@ void pci_disable_ats(struct pci_dev *dev) return; /* VFs still enabled */ if (dev->is_virtfn) { - struct pci_dev *pdev = dev->physfn; - + pdev = pci_physfn(dev); atomic_dec(&pdev->ats_ref_cnt); } -- cgit From a021f3019db7b40f0fe85b0a64c6be21e3665fbf Mon Sep 17 00:00:00 2001 From: Bjorn Helgaas Date: Fri, 17 Jul 2015 15:43:27 -0500 Subject: PCI: Clean up ATS error handling 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 Reviewed-by: Joerg Roedel --- drivers/pci/ats.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) (limited to 'drivers/pci/ats.c') diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c index 0b5b0ed7a436..9355f754c7c2 100644 --- a/drivers/pci/ats.c +++ b/drivers/pci/ats.c @@ -44,11 +44,12 @@ int pci_enable_ats(struct pci_dev *dev, int ps) u16 ctrl; struct pci_dev *pdev; - BUG_ON(dev->ats_cap && dev->ats_enabled); - if (!dev->ats_cap) return -EINVAL; + if (WARN_ON(pci_ats_enabled(dev))) + return -EBUSY; + if (ps < PCI_ATS_MIN_STU) return -EINVAL; @@ -83,7 +84,8 @@ void pci_disable_ats(struct pci_dev *dev) struct pci_dev *pdev; u16 ctrl; - BUG_ON(!dev->ats_cap || !dev->ats_enabled); + if (WARN_ON(!pci_ats_enabled(dev))) + return; if (atomic_read(&dev->ats_ref_cnt)) return; /* VFs still enabled */ @@ -107,8 +109,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) -- cgit From a71f938f3a9a7bc879296cd34ecae9effe5edf3f Mon Sep 17 00:00:00 2001 From: Bjorn Helgaas Date: Mon, 20 Jul 2015 09:24:32 -0500 Subject: PCI: Stop caching ATS Invalidate Queue Depth Stop caching the Invalidate Queue Depth in struct pci_dev. pci_ats_queue_depth() is typically called only once per device, and it returns a fixed value per-device, so callers who need the value frequently can cache it themselves. Signed-off-by: Bjorn Helgaas Reviewed-by: Joerg Roedel --- drivers/pci/ats.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) (limited to 'drivers/pci/ats.c') diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c index 9355f754c7c2..ceda7dc556d4 100644 --- a/drivers/pci/ats.c +++ b/drivers/pci/ats.c @@ -20,16 +20,12 @@ void pci_ats_init(struct pci_dev *dev) { int pos; - u16 cap; pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ATS); if (!pos) return; 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; } /** @@ -131,13 +127,16 @@ EXPORT_SYMBOL_GPL(pci_restore_ats_state); */ int pci_ats_queue_depth(struct pci_dev *dev) { + u16 cap; + if (!dev->ats_cap) return -EINVAL; if (dev->is_virtfn) return 0; - return dev->ats_qdep; + pci_read_config_word(dev, dev->ats_cap + PCI_ATS_CAP, &cap); + return PCI_ATS_CAP_QDEP(cap) ? PCI_ATS_CAP_QDEP(cap) : PCI_ATS_MAX_QDEP; } EXPORT_SYMBOL_GPL(pci_ats_queue_depth); -- cgit From f7ef1340bb501717372a39f4807d0ad519ebd432 Mon Sep 17 00:00:00 2001 From: Bjorn Helgaas Date: Mon, 20 Jul 2015 09:23:37 -0500 Subject: PCI: Remove pci_ats_enabled() Remove pci_ats_enabled(). There are no callers outside the ATS code itself. We don't need to check ats_cap, because if we don't find an ATS capability, we'll never set ats_enabled. Signed-off-by: Bjorn Helgaas Reviewed-by: Joerg Roedel --- drivers/pci/ats.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'drivers/pci/ats.c') diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c index ceda7dc556d4..eeb9fb2b47aa 100644 --- a/drivers/pci/ats.c +++ b/drivers/pci/ats.c @@ -43,7 +43,7 @@ int pci_enable_ats(struct pci_dev *dev, int ps) if (!dev->ats_cap) return -EINVAL; - if (WARN_ON(pci_ats_enabled(dev))) + if (WARN_ON(dev->ats_enabled)) return -EBUSY; if (ps < PCI_ATS_MIN_STU) @@ -80,7 +80,7 @@ void pci_disable_ats(struct pci_dev *dev) struct pci_dev *pdev; u16 ctrl; - if (WARN_ON(!pci_ats_enabled(dev))) + if (WARN_ON(!dev->ats_enabled)) return; if (atomic_read(&dev->ats_ref_cnt)) @@ -103,7 +103,7 @@ void pci_restore_ats_state(struct pci_dev *dev) { u16 ctrl; - if (!pci_ats_enabled(dev)) + if (!dev->ats_enabled) return; ctrl = PCI_ATS_CTRL_ENABLE; -- cgit