I've had these minor iommu cleanups lying around for a while, but the ugly
dmesg logs from [1] prompted me to finally post them. Take what you like,
ignore the rest, and tell me so I can clear out my queue of old stuff.
These fix no actual bugs.
[1] https://lore.kernel.org/linux-pci/[email protected]
---
Bjorn Helgaas (7):
iommu: Use dev_printk() when possible
iommu/amd: Use dev_printk() when possible
iommu/vt-d: Use dev_printk() when possible
iommu/vt-d: Remove unnecessary local variable initializations
iommu/vt-d: Remove unused dmar_remove_one_dev_info() argument
iommu/vt-d: Remove misleading "domain 0" test from domain_exit()
iommu/vt-d: Simplify control flow
drivers/iommu/amd_iommu.c | 25 +++----
drivers/iommu/amd_iommu_init.c | 20 +++---
drivers/iommu/intel-iommu.c | 136 +++++++++++++++++-----------------------
drivers/iommu/iommu.c | 8 +-
4 files changed, 84 insertions(+), 105 deletions(-)
From: Bjorn Helgaas <[email protected]>
Use dev_printk() when possible so the IOMMU messages are more consistent
with other messages related to the device.
E.g., I think these messages related to surprise hotplug:
pciehp 0000:80:10.0:pcie004: Slot(36): Link Down
iommu: Removing device 0000:87:00.0 from group 12
pciehp 0000:80:10.0:pcie004: Slot(36): Card present
pcieport 0000:80:10.0: Data Link Layer Link Active not set in 1000 msec
would be easier to read as these (also requires some PCI changes not
included here):
pci 0000:80:10.0: Slot(36): Link Down
pci 0000:87:00.0: Removing from iommu group 12
pci 0000:80:10.0: Slot(36): Card present
pci 0000:80:10.0: Data Link Layer Link Active not set in 1000 msec
Signed-off-by: Bjorn Helgaas <[email protected]>
---
drivers/iommu/iommu.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 3ed4db334341..54c9d18fe31d 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -668,7 +668,7 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)
trace_add_device_to_group(group->id, dev);
- pr_info("Adding device %s to group %d\n", dev_name(dev), group->id);
+ dev_info(dev, "Adding to iommu group %d\n", group->id);
return 0;
@@ -684,7 +684,7 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)
sysfs_remove_link(&dev->kobj, "iommu_group");
err_free_device:
kfree(device);
- pr_err("Failed to add device %s to group %d: %d\n", dev_name(dev), group->id, ret);
+ dev_err(dev, "Failed to add to iommu group %d: %d\n", group->id, ret);
return ret;
}
EXPORT_SYMBOL_GPL(iommu_group_add_device);
@@ -701,7 +701,7 @@ void iommu_group_remove_device(struct device *dev)
struct iommu_group *group = dev->iommu_group;
struct group_device *tmp_device, *device = NULL;
- pr_info("Removing device %s from group %d\n", dev_name(dev), group->id);
+ dev_info(dev, "Removing from iommu group %d\n", group->id);
/* Pre-notify listeners that a device is being removed. */
blocking_notifier_call_chain(&group->notifier,
@@ -1951,7 +1951,7 @@ int iommu_request_dm_for_dev(struct device *dev)
iommu_domain_free(group->default_domain);
group->default_domain = dm_domain;
- pr_info("Using direct mapping for device %s\n", dev_name(dev));
+ dev_info(dev, "Using iommu direct mapping\n");
ret = 0;
out:
From: Bjorn Helgaas <[email protected]>
Use dev_printk() when possible so the IOMMU messages are more consistent
with other messages related to the device.
Signed-off-by: Bjorn Helgaas <[email protected]>
---
drivers/iommu/amd_iommu.c | 25 +++++++++++--------------
drivers/iommu/amd_iommu_init.c | 20 ++++++++++----------
2 files changed, 21 insertions(+), 24 deletions(-)
diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 87ba23a75b38..fee9c9049d7a 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -18,6 +18,7 @@
*/
#define pr_fmt(fmt) "AMD-Vi: " fmt
+#define dev_fmt(fmt) pr_fmt(fmt)
#include <linux/ratelimit.h>
#include <linux/pci.h>
@@ -279,10 +280,10 @@ static u16 get_alias(struct device *dev)
return pci_alias;
}
- pr_info("Using IVRS reported alias %02x:%02x.%d "
- "for device %s[%04x:%04x], kernel reported alias "
+ pci_info(pdev, "Using IVRS reported alias %02x:%02x.%d "
+ "for device [%04x:%04x], kernel reported alias "
"%02x:%02x.%d\n", PCI_BUS_NUM(ivrs_alias), PCI_SLOT(ivrs_alias),
- PCI_FUNC(ivrs_alias), dev_name(dev), pdev->vendor, pdev->device,
+ PCI_FUNC(ivrs_alias), pdev->vendor, pdev->device,
PCI_BUS_NUM(pci_alias), PCI_SLOT(pci_alias),
PCI_FUNC(pci_alias));
@@ -293,9 +294,8 @@ static u16 get_alias(struct device *dev)
if (pci_alias == devid &&
PCI_BUS_NUM(ivrs_alias) == pdev->bus->number) {
pci_add_dma_alias(pdev, ivrs_alias & 0xff);
- pr_info("Added PCI DMA alias %02x.%d for %s\n",
- PCI_SLOT(ivrs_alias), PCI_FUNC(ivrs_alias),
- dev_name(dev));
+ pci_info(pdev, "Added PCI DMA alias %02x.%d\n",
+ PCI_SLOT(ivrs_alias), PCI_FUNC(ivrs_alias));
}
return ivrs_alias;
@@ -545,7 +545,7 @@ static void amd_iommu_report_page_fault(u16 devid, u16 domain_id,
dev_data = get_dev_data(&pdev->dev);
if (dev_data && __ratelimit(&dev_data->rs)) {
- dev_err(&pdev->dev, "Event logged [IO_PAGE_FAULT domain=0x%04x address=0x%llx flags=0x%04x]\n",
+ pci_err(pdev, "Event logged [IO_PAGE_FAULT domain=0x%04x address=0x%llx flags=0x%04x]\n",
domain_id, address, flags);
} else if (printk_ratelimit()) {
pr_err("Event logged [IO_PAGE_FAULT device=%02x:%02x.%x domain=0x%04x address=0x%llx flags=0x%04x]\n",
@@ -2251,8 +2251,7 @@ static int amd_iommu_add_device(struct device *dev)
ret = iommu_init_device(dev);
if (ret) {
if (ret != -ENOTSUPP)
- pr_err("Failed to initialize device %s - trying to proceed anyway\n",
- dev_name(dev));
+ dev_err(dev, "Failed to initialize - trying to proceed anyway\n");
iommu_ignore_device(dev);
dev->dma_ops = NULL;
@@ -2605,8 +2604,7 @@ static int map_sg(struct device *dev, struct scatterlist *sglist,
return nelems;
out_unmap:
- pr_err("%s: IOMMU mapping error in map_sg (io-pages: %d)\n",
- dev_name(dev), npages);
+ dev_err(dev, "IOMMU mapping error in map_sg (io-pages: %d)\n", npages);
for_each_sg(sglist, s, nelems, i) {
int j, pages = iommu_num_pages(sg_phys(s), s->length, PAGE_SIZE);
@@ -2800,7 +2798,7 @@ static int init_reserved_iova_ranges(void)
IOVA_PFN(r->start),
IOVA_PFN(r->end));
if (!val) {
- pr_err("Reserve pci-resource range failed\n");
+ pci_err(pdev, "Reserve pci-resource range %pR failed\n", r);
return -ENOMEM;
}
}
@@ -3170,8 +3168,7 @@ static void amd_iommu_get_resv_regions(struct device *dev,
length, prot,
IOMMU_RESV_DIRECT);
if (!region) {
- pr_err("Out of memory allocating dm-regions for %s\n",
- dev_name(dev));
+ dev_err(dev, "Out of memory allocating dm-regions\n");
return;
}
list_add_tail(®ion->list, head);
diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index 66123b911ec8..f773792d77fd 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -18,6 +18,7 @@
*/
#define pr_fmt(fmt) "AMD-Vi: " fmt
+#define dev_fmt(fmt) pr_fmt(fmt)
#include <linux/pci.h>
#include <linux/acpi.h>
@@ -1457,8 +1458,7 @@ static void amd_iommu_erratum_746_workaround(struct amd_iommu *iommu)
pci_write_config_dword(iommu->dev, 0xf0, 0x90 | (1 << 8));
pci_write_config_dword(iommu->dev, 0xf4, value | 0x4);
- pr_info("Applying erratum 746 workaround for IOMMU at %s\n",
- dev_name(&iommu->dev->dev));
+ pci_info(iommu->dev, "Applying erratum 746 workaround\n");
/* Clear the enable writing bit */
pci_write_config_dword(iommu->dev, 0xf0, 0x90);
@@ -1488,8 +1488,7 @@ static void amd_iommu_ats_write_check_workaround(struct amd_iommu *iommu)
/* Set L2_DEBUG_3[AtsIgnoreIWDis] = 1 */
iommu_write_l2(iommu, 0x47, value | BIT(0));
- pr_info("Applying ATS write check workaround for IOMMU at %s\n",
- dev_name(&iommu->dev->dev));
+ pci_info(iommu->dev, "Applying ATS write check workaround\n");
}
/*
@@ -1665,6 +1664,7 @@ static int iommu_pc_get_set_reg(struct amd_iommu *iommu, u8 bank, u8 cntr,
static void init_iommu_perf_ctr(struct amd_iommu *iommu)
{
+ struct pci_dev *pdev = iommu->dev;
u64 val = 0xabcd, val2 = 0;
if (!iommu_feature(iommu, FEATURE_PC))
@@ -1676,12 +1676,12 @@ static void init_iommu_perf_ctr(struct amd_iommu *iommu)
if ((iommu_pc_get_set_reg(iommu, 0, 0, 0, &val, true)) ||
(iommu_pc_get_set_reg(iommu, 0, 0, 0, &val2, false)) ||
(val != val2)) {
- pr_err("Unable to write to IOMMU perf counter.\n");
+ pci_err(pdev, "Unable to write to IOMMU perf counter.\n");
amd_iommu_pc_present = false;
return;
}
- pr_info("IOMMU performance counters supported\n");
+ pci_info(pdev, "IOMMU performance counters supported\n");
val = readl(iommu->mmio_base + MMIO_CNTR_CONF_OFFSET);
iommu->max_banks = (u8) ((val >> 12) & 0x3f);
@@ -1840,14 +1840,14 @@ static void print_iommu_info(void)
struct amd_iommu *iommu;
for_each_iommu(iommu) {
+ struct pci_dev *pdev = iommu->dev;
int i;
- pr_info("Found IOMMU at %s cap 0x%hx\n",
- dev_name(&iommu->dev->dev), iommu->cap_ptr);
+ pci_info(pdev, "Found IOMMU cap 0x%hx\n", iommu->cap_ptr);
if (iommu->cap & (1 << IOMMU_CAP_EFR)) {
- pr_info("Extended features (%#llx):\n",
- iommu->features);
+ pci_info(pdev, "Extended features (%#llx):\n",
+ iommu->features);
for (i = 0; i < ARRAY_SIZE(feat_str); ++i) {
if (iommu_feature(iommu, (1ULL << i)))
pr_cont(" %s", feat_str[i]);
From: Bjorn Helgaas <[email protected]>
Use dev_printk() when possible so the IOMMU messages are more consistent
with other messages related to the device.
Signed-off-by: Bjorn Helgaas <[email protected]>
---
drivers/iommu/intel-iommu.c | 54 +++++++++++++++++++------------------------
1 file changed, 24 insertions(+), 30 deletions(-)
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 2bd9ac285c0d..81077803880f 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -19,6 +19,7 @@
*/
#define pr_fmt(fmt) "DMAR: " fmt
+#define dev_fmt(fmt) pr_fmt(fmt)
#include <linux/init.h>
#include <linux/bitmap.h>
@@ -1815,7 +1816,7 @@ static int dmar_init_reserved_ranges(void)
IOVA_PFN(r->start),
IOVA_PFN(r->end));
if (!iova) {
- pr_err("Reserve iova failed\n");
+ pci_err(pdev, "Reserve iova for %pR failed\n", r);
return -ENODEV;
}
}
@@ -2544,8 +2545,7 @@ static struct dmar_domain *dmar_insert_one_dev_info(struct intel_iommu *iommu,
if (dev && dev_is_pci(dev) && sm_supported(iommu)) {
ret = intel_pasid_alloc_table(dev);
if (ret) {
- pr_err("PASID table allocation for %s failed\n",
- dev_name(dev));
+ dev_err(dev, "PASID table allocation failed\n");
dmar_remove_one_dev_info(domain, dev);
return NULL;
}
@@ -2560,15 +2560,14 @@ static struct dmar_domain *dmar_insert_one_dev_info(struct intel_iommu *iommu,
dev, PASID_RID2PASID);
spin_unlock(&iommu->lock);
if (ret) {
- pr_err("Setup RID2PASID for %s failed\n",
- dev_name(dev));
+ dev_err(dev, "Setup RID2PASID failed\n");
dmar_remove_one_dev_info(domain, dev);
return NULL;
}
}
if (dev && domain_context_mapping(domain, dev)) {
- pr_err("Domain context map for %s failed\n", dev_name(dev));
+ dev_err(dev, "Domain context map failed\n");
dmar_remove_one_dev_info(domain, dev);
return NULL;
}
@@ -2723,13 +2722,12 @@ static int domain_prepare_identity_map(struct device *dev,
range which is reserved in E820, so which didn't get set
up to start with in si_domain */
if (domain == si_domain && hw_pass_through) {
- pr_warn("Ignoring identity map for HW passthrough device %s [0x%Lx - 0x%Lx]\n",
- dev_name(dev), start, end);
+ dev_warn(dev, "Ignoring identity map for HW passthrough [0x%Lx - 0x%Lx]\n",
+ start, end);
return 0;
}
- pr_info("Setting identity map for device %s [0x%Lx - 0x%Lx]\n",
- dev_name(dev), start, end);
+ dev_info(dev, "Setting identity map [0x%Lx - 0x%Lx]\n", start, end);
if (end < start) {
WARN(1, "Your BIOS is broken; RMRR ends before it starts!\n"
@@ -3016,8 +3014,8 @@ static int __init dev_prepare_static_identity_mapping(struct device *dev, int hw
ret = domain_add_dev_info(si_domain, dev);
if (!ret)
- pr_info("%s identity mapping for device %s\n",
- hw ? "Hardware" : "Software", dev_name(dev));
+ dev_info(dev, "%s identity mapping\n",
+ hw ? "Hardware" : "Software");
else if (ret == -ENODEV)
/* device not associated with an iommu */
ret = 0;
@@ -3550,8 +3548,7 @@ static unsigned long intel_alloc_iova(struct device *dev,
iova_pfn = alloc_iova_fast(&domain->iovad, nrpages,
IOVA_PFN(dma_mask), true);
if (unlikely(!iova_pfn)) {
- pr_err("Allocating %ld-page iova for %s failed",
- nrpages, dev_name(dev));
+ dev_err(dev, "Allocating %ld-page iova failed", nrpages);
return 0;
}
@@ -3599,7 +3596,7 @@ struct dmar_domain *get_valid_domain_for_dev(struct device *dev)
out:
if (!domain)
- pr_err("Allocating domain for %s failed\n", dev_name(dev));
+ dev_err(dev, "Allocating domain failed\n");
return domain;
@@ -3626,8 +3623,7 @@ static int iommu_no_mapping(struct device *dev)
* to non-identity mapping.
*/
dmar_remove_one_dev_info(si_domain, dev);
- pr_info("32bit %s uses non-identity mapping\n",
- dev_name(dev));
+ dev_info(dev, "32bit DMA uses non-identity mapping\n");
return 0;
}
} else {
@@ -3639,8 +3635,7 @@ static int iommu_no_mapping(struct device *dev)
int ret;
ret = domain_add_dev_info(si_domain, dev);
if (!ret) {
- pr_info("64bit %s uses identity mapping\n",
- dev_name(dev));
+ dev_info(dev, "64bit DMA uses identity mapping\n");
return 1;
}
}
@@ -3705,8 +3700,8 @@ static dma_addr_t __intel_map_page(struct device *dev, struct page *page,
error:
if (iova_pfn)
free_iova_fast(&domain->iovad, iova_pfn, dma_to_mm_pfn(size));
- pr_err("Device %s request: %zx@%llx dir %d --- failed\n",
- dev_name(dev), size, (unsigned long long)paddr, dir);
+ dev_err(dev, "Device request: %zx@%llx dir %d --- failed\n",
+ size, (unsigned long long)paddr, dir);
return DMA_MAPPING_ERROR;
}
@@ -3741,8 +3736,7 @@ static void intel_unmap(struct device *dev, dma_addr_t dev_addr, size_t size)
start_pfn = mm_to_dma_pfn(iova_pfn);
last_pfn = start_pfn + nrpages - 1;
- pr_debug("Device %s unmapping: pfn %lx-%lx\n",
- dev_name(dev), start_pfn, last_pfn);
+ dev_dbg(dev, "Device unmapping: pfn %lx-%lx\n", start_pfn, last_pfn);
freelist = domain_unmap(domain, start_pfn, last_pfn);
@@ -5096,9 +5090,9 @@ static int intel_iommu_attach_device(struct iommu_domain *domain,
addr_width = cap_mgaw(iommu->cap);
if (dmar_domain->max_addr > (1LL << addr_width)) {
- pr_err("%s: iommu width (%d) is not "
- "sufficient for the mapped address (%llx)\n",
- __func__, addr_width, dmar_domain->max_addr);
+ dev_err(dev, "%s: iommu width (%d) is not "
+ "sufficient for the mapped address (%llx)\n",
+ __func__, addr_width, dmar_domain->max_addr);
return -EFAULT;
}
dmar_domain->gaw = addr_width;
@@ -5399,7 +5393,7 @@ const struct iommu_ops intel_iommu_ops = {
static void quirk_iommu_g4x_gfx(struct pci_dev *dev)
{
/* G4x/GM45 integrated gfx dmar support is totally busted. */
- pr_info("Disabling IOMMU for graphics on this chipset\n");
+ pci_info(dev, "Disabling IOMMU for graphics on this chipset\n");
dmar_map_gfx = 0;
}
@@ -5417,7 +5411,7 @@ static void quirk_iommu_rwbf(struct pci_dev *dev)
* Mobile 4 Series Chipset neglects to set RWBF capability,
* but needs it. Same seems to hold for the desktop versions.
*/
- pr_info("Forcing write-buffer flush capability\n");
+ pci_info(dev, "Forcing write-buffer flush capability\n");
rwbf_quirk = 1;
}
@@ -5447,11 +5441,11 @@ static void quirk_calpella_no_shadow_gtt(struct pci_dev *dev)
return;
if (!(ggc & GGC_MEMORY_VT_ENABLED)) {
- pr_info("BIOS has allocated no shadow GTT; disabling IOMMU for graphics\n");
+ pci_info(dev, "BIOS has allocated no shadow GTT; disabling IOMMU for graphics\n");
dmar_map_gfx = 0;
} else if (dmar_map_gfx) {
/* we have to ensure the gfx device is idle before we flush */
- pr_info("Disabling batched IOTLB flush on Ironlake\n");
+ pci_info(dev, "Disabling batched IOTLB flush on Ironlake\n");
intel_iommu_strict = 1;
}
}
From: Bjorn Helgaas <[email protected]>
A local variable initialization is a hint that the variable will be used in
an unusual way. If the initialization is unnecessary, that hint becomes a
distraction.
Remove unnecessary initializations. No functional change intended.
Signed-off-by: Bjorn Helgaas <[email protected]>
---
drivers/iommu/intel-iommu.c | 27 +++++++++++++--------------
1 file changed, 13 insertions(+), 14 deletions(-)
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 81077803880f..2acd08c82cdc 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -865,7 +865,7 @@ static void free_context_table(struct intel_iommu *iommu)
static struct dma_pte *pfn_to_dma_pte(struct dmar_domain *domain,
unsigned long pfn, int *target_level)
{
- struct dma_pte *parent, *pte = NULL;
+ struct dma_pte *parent, *pte;
int level = agaw_to_level(domain->agaw);
int offset;
@@ -922,7 +922,7 @@ static struct dma_pte *dma_pfn_level_pte(struct dmar_domain *domain,
unsigned long pfn,
int level, int *large_page)
{
- struct dma_pte *parent, *pte = NULL;
+ struct dma_pte *parent, *pte;
int total = agaw_to_level(domain->agaw);
int offset;
@@ -954,7 +954,7 @@ static void dma_pte_clear_range(struct dmar_domain *domain,
unsigned long start_pfn,
unsigned long last_pfn)
{
- unsigned int large_page = 1;
+ unsigned int large_page;
struct dma_pte *first_pte, *pte;
BUG_ON(!domain_pfn_supported(domain, start_pfn));
@@ -1132,7 +1132,7 @@ static struct page *domain_unmap(struct dmar_domain *domain,
unsigned long start_pfn,
unsigned long last_pfn)
{
- struct page *freelist = NULL;
+ struct page *freelist;
BUG_ON(!domain_pfn_supported(domain, start_pfn));
BUG_ON(!domain_pfn_supported(domain, last_pfn));
@@ -1763,7 +1763,7 @@ static int domain_attach_iommu(struct dmar_domain *domain,
static int domain_detach_iommu(struct dmar_domain *domain,
struct intel_iommu *iommu)
{
- int num, count = INT_MAX;
+ int num, count;
assert_spin_locked(&device_domain_lock);
assert_spin_locked(&iommu->lock);
@@ -1902,7 +1902,7 @@ static int domain_init(struct dmar_domain *domain, struct intel_iommu *iommu,
static void domain_exit(struct dmar_domain *domain)
{
- struct page *freelist = NULL;
+ struct page *freelist;
/* Domain 0 is reserved, so dont process it */
if (!domain)
@@ -2583,7 +2583,7 @@ static int get_last_alias(struct pci_dev *pdev, u16 alias, void *opaque)
static struct dmar_domain *find_or_alloc_domain(struct device *dev, int gaw)
{
- struct device_domain_info *info = NULL;
+ struct device_domain_info *info;
struct dmar_domain *domain = NULL;
struct intel_iommu *iommu;
u16 dma_alias;
@@ -2807,7 +2807,7 @@ static int md_domain_init(struct dmar_domain *domain, int guest_width);
static int __init si_domain_init(int hw)
{
- int nid, ret = 0;
+ int nid, ret;
si_domain = alloc_domain(DOMAIN_FLAG_STATIC_IDENTITY);
if (!si_domain)
@@ -2931,7 +2931,6 @@ static bool device_is_rmrr_locked(struct device *dev)
static int iommu_should_identity_map(struct device *dev, int startup)
{
-
if (dev_is_pci(dev)) {
struct pci_dev *pdev = to_pci_dev(dev);
@@ -3527,7 +3526,7 @@ static unsigned long intel_alloc_iova(struct device *dev,
struct dmar_domain *domain,
unsigned long nrpages, uint64_t dma_mask)
{
- unsigned long iova_pfn = 0;
+ unsigned long iova_pfn;
/* Restrict dma_mask to the width that the iommu can handle */
dma_mask = min_t(uint64_t, DOMAIN_MAX_ADDR(domain->gaw), dma_mask);
@@ -4333,7 +4332,7 @@ int dmar_check_one_atsr(struct acpi_dmar_header *hdr, void *arg)
static int intel_iommu_add(struct dmar_drhd_unit *dmaru)
{
- int sp, ret = 0;
+ int sp, ret;
struct intel_iommu *iommu = dmaru->iommu;
if (g_iommus[iommu->seq_id])
@@ -4497,7 +4496,7 @@ int dmar_find_matched_atsr_unit(struct pci_dev *dev)
int dmar_iommu_notify_scope_dev(struct dmar_pci_notify_info *info)
{
- int ret = 0;
+ int ret;
struct dmar_rmrr_unit *rmrru;
struct dmar_atsr_unit *atsru;
struct acpi_dmar_atsr *atsr;
@@ -4514,7 +4513,7 @@ int dmar_iommu_notify_scope_dev(struct dmar_pci_notify_info *info)
((void *)rmrr) + rmrr->header.length,
rmrr->segment, rmrru->devices,
rmrru->devices_cnt);
- if(ret < 0)
+ if (ret < 0)
return ret;
} else if (info->event == BUS_NOTIFY_REMOVED_DEVICE) {
dmar_remove_dev_scope(info, rmrr->segment,
@@ -4534,7 +4533,7 @@ int dmar_iommu_notify_scope_dev(struct dmar_pci_notify_info *info)
atsru->devices_cnt);
if (ret > 0)
break;
- else if(ret < 0)
+ else if (ret < 0)
return ret;
} else if (info->event == BUS_NOTIFY_REMOVED_DEVICE) {
if (dmar_remove_dev_scope(info, atsr->segment,
From: Bjorn Helgaas <[email protected]>
Simplify control flow by returning immediately when we know the result.
No functional change intended.
Signed-off-by: Bjorn Helgaas <[email protected]>
---
drivers/iommu/intel-iommu.c | 31 +++++++++++++------------------
1 file changed, 13 insertions(+), 18 deletions(-)
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index b0860a8c48d4..6eaa4ada6e1d 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -509,12 +509,12 @@ static void set_iommu_domain(struct intel_iommu *iommu, u16 did,
void *alloc_pgtable_page(int node)
{
struct page *page;
- void *vaddr = NULL;
page = alloc_pages_node(node, GFP_ATOMIC | __GFP_ZERO, 0);
- if (page)
- vaddr = page_address(page);
- return vaddr;
+ if (!page)
+ return NULL;
+
+ return page_address(page);
}
void free_pgtable_page(void *vaddr)
@@ -2606,20 +2606,19 @@ static struct dmar_domain *find_or_alloc_domain(struct device *dev, int gaw)
/* DMA alias already has a domain, use it */
if (info)
- goto out;
+ return domain;
}
/* Allocate and initialize new domain for the device */
domain = alloc_domain(0);
if (!domain)
return NULL;
+
if (domain_init(domain, iommu, gaw)) {
domain_exit(domain);
return NULL;
}
-out:
-
return domain;
}
@@ -2665,11 +2664,11 @@ static struct dmar_domain *get_domain_for_dev(struct device *dev, int gaw)
domain = find_domain(dev);
if (domain)
- goto out;
+ return domain;
domain = find_or_alloc_domain(dev, gaw);
if (!domain)
- goto out;
+ return NULL;
tmp = set_domain_for_dev(dev, domain);
if (!tmp || domain != tmp) {
@@ -2677,8 +2676,6 @@ static struct dmar_domain *get_domain_for_dev(struct device *dev, int gaw)
domain = tmp;
}
-out:
-
return domain;
}
@@ -3558,11 +3555,13 @@ struct dmar_domain *get_valid_domain_for_dev(struct device *dev)
domain = find_domain(dev);
if (domain)
- goto out;
+ return domain;
domain = find_or_alloc_domain(dev, DEFAULT_DOMAIN_ADDRESS_WIDTH);
- if (!domain)
- goto out;
+ if (!domain) {
+ dev_err(dev, "Allocating domain failed\n");
+ return NULL;
+ }
/* We have a new domain - setup possible RMRRs for the device */
rcu_read_lock();
@@ -3587,12 +3586,8 @@ struct dmar_domain *get_valid_domain_for_dev(struct device *dev)
domain = tmp;
}
-out:
-
if (!domain)
dev_err(dev, "Allocating domain failed\n");
-
-
return domain;
}
From: Bjorn Helgaas <[email protected]>
domain_remove_dev_info() takes a struct dmar_domain * argument, but doesn't
use it. Remove it. No functional change intended.
The last use of this argument was removed by 127c761598f7 ("iommu/vt-d:
Pass device_domain_info to __dmar_remove_one_dev_info").
Signed-off-by: Bjorn Helgaas <[email protected]>
---
drivers/iommu/intel-iommu.c | 20 +++++++++-----------
1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 2acd08c82cdc..6e9f277bfd6d 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -343,8 +343,7 @@ static int g_num_of_iommus;
static void domain_exit(struct dmar_domain *domain);
static void domain_remove_dev_info(struct dmar_domain *domain);
-static void dmar_remove_one_dev_info(struct dmar_domain *domain,
- struct device *dev);
+static void dmar_remove_one_dev_info(struct device *dev);
static void __dmar_remove_one_dev_info(struct device_domain_info *info);
static void domain_context_clear(struct intel_iommu *iommu,
struct device *dev);
@@ -2546,7 +2545,7 @@ static struct dmar_domain *dmar_insert_one_dev_info(struct intel_iommu *iommu,
ret = intel_pasid_alloc_table(dev);
if (ret) {
dev_err(dev, "PASID table allocation failed\n");
- dmar_remove_one_dev_info(domain, dev);
+ dmar_remove_one_dev_info(dev);
return NULL;
}
@@ -2561,14 +2560,14 @@ static struct dmar_domain *dmar_insert_one_dev_info(struct intel_iommu *iommu,
spin_unlock(&iommu->lock);
if (ret) {
dev_err(dev, "Setup RID2PASID failed\n");
- dmar_remove_one_dev_info(domain, dev);
+ dmar_remove_one_dev_info(dev);
return NULL;
}
}
if (dev && domain_context_mapping(domain, dev)) {
dev_err(dev, "Domain context map failed\n");
- dmar_remove_one_dev_info(domain, dev);
+ dmar_remove_one_dev_info(dev);
return NULL;
}
@@ -3621,7 +3620,7 @@ static int iommu_no_mapping(struct device *dev)
* 32 bit DMA is removed from si_domain and fall back
* to non-identity mapping.
*/
- dmar_remove_one_dev_info(si_domain, dev);
+ dmar_remove_one_dev_info(dev);
dev_info(dev, "32bit DMA uses non-identity mapping\n");
return 0;
}
@@ -4567,7 +4566,7 @@ static int device_notifier(struct notifier_block *nb,
if (!domain)
return 0;
- dmar_remove_one_dev_info(domain, dev);
+ dmar_remove_one_dev_info(dev);
if (!domain_type_is_vm_or_si(domain) && list_empty(&domain->devices))
domain_exit(domain);
@@ -4980,8 +4979,7 @@ static void __dmar_remove_one_dev_info(struct device_domain_info *info)
free_devinfo_mem(info);
}
-static void dmar_remove_one_dev_info(struct dmar_domain *domain,
- struct device *dev)
+static void dmar_remove_one_dev_info(struct device *dev)
{
struct device_domain_info *info;
unsigned long flags;
@@ -5070,7 +5068,7 @@ static int intel_iommu_attach_device(struct iommu_domain *domain,
old_domain = find_domain(dev);
if (old_domain) {
rcu_read_lock();
- dmar_remove_one_dev_info(old_domain, dev);
+ dmar_remove_one_dev_info(dev);
rcu_read_unlock();
if (!domain_type_is_vm_or_si(old_domain) &&
@@ -5117,7 +5115,7 @@ static int intel_iommu_attach_device(struct iommu_domain *domain,
static void intel_iommu_detach_device(struct iommu_domain *domain,
struct device *dev)
{
- dmar_remove_one_dev_info(to_dmar_domain(domain), dev);
+ dmar_remove_one_dev_info(dev);
}
static int intel_iommu_map(struct iommu_domain *domain,
From: Bjorn Helgaas <[email protected]>
The "Domain 0 is reserved, so dont process it" comment suggests that a NULL
pointer corresponds to domain 0. I don't think that's true, and in any
case, every caller supplies a non-NULL domain pointer that has already been
dereferenced, so the test is unnecessary.
Remove the test for a null "domain" pointer. No functional change
intended.
This null pointer check was added by 5e98c4b1d6e8 ("Allocation and free
functions of virtual machine domain").
Signed-off-by: Bjorn Helgaas <[email protected]>
---
drivers/iommu/intel-iommu.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 6e9f277bfd6d..b0860a8c48d4 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -1903,10 +1903,6 @@ static void domain_exit(struct dmar_domain *domain)
{
struct page *freelist;
- /* Domain 0 is reserved, so dont process it */
- if (!domain)
- return;
-
/* Remove associated devices and clear attached or cached domains */
rcu_read_lock();
domain_remove_dev_info(domain);
Hi,
On 2/9/19 6:06 AM, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <[email protected]>
>
> A local variable initialization is a hint that the variable will be used in
> an unusual way. If the initialization is unnecessary, that hint becomes a
> distraction.
>
> Remove unnecessary initializations. No functional change intended.
>
> Signed-off-by: Bjorn Helgaas <[email protected]>
> ---
> drivers/iommu/intel-iommu.c | 27 +++++++++++++--------------
> 1 file changed, 13 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 81077803880f..2acd08c82cdc 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -865,7 +865,7 @@ static void free_context_table(struct intel_iommu *iommu)
> static struct dma_pte *pfn_to_dma_pte(struct dmar_domain *domain,
> unsigned long pfn, int *target_level)
> {
> - struct dma_pte *parent, *pte = NULL;
> + struct dma_pte *parent, *pte;
> int level = agaw_to_level(domain->agaw);
> int offset;
>
> @@ -922,7 +922,7 @@ static struct dma_pte *dma_pfn_level_pte(struct dmar_domain *domain,
> unsigned long pfn,
> int level, int *large_page)
> {
> - struct dma_pte *parent, *pte = NULL;
> + struct dma_pte *parent, *pte;
> int total = agaw_to_level(domain->agaw);
> int offset;
>
> @@ -954,7 +954,7 @@ static void dma_pte_clear_range(struct dmar_domain *domain,
> unsigned long start_pfn,
> unsigned long last_pfn)
> {
> - unsigned int large_page = 1;
> + unsigned int large_page;
> struct dma_pte *first_pte, *pte;
>
> BUG_ON(!domain_pfn_supported(domain, start_pfn));
> @@ -1132,7 +1132,7 @@ static struct page *domain_unmap(struct dmar_domain *domain,
> unsigned long start_pfn,
> unsigned long last_pfn)
> {
> - struct page *freelist = NULL;
> + struct page *freelist;
I am afraid this change might cause problem. "freelist" might go through
dma_pte_clear_level() without any touches.
Best regards,
Lu Baolu
On Sun, Feb 10, 2019 at 8:00 PM Lu Baolu <[email protected]> wrote:
>
> Hi,
>
> On 2/9/19 6:06 AM, Bjorn Helgaas wrote:
> > From: Bjorn Helgaas <[email protected]>
> >
> > A local variable initialization is a hint that the variable will be used in
> > an unusual way. If the initialization is unnecessary, that hint becomes a
> > distraction.
> >
> > Remove unnecessary initializations. No functional change intended.
> >
> > Signed-off-by: Bjorn Helgaas <[email protected]>
> > ---
> > drivers/iommu/intel-iommu.c | 27 +++++++++++++--------------
> > 1 file changed, 13 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> > index 81077803880f..2acd08c82cdc 100644
> > --- a/drivers/iommu/intel-iommu.c
> > +++ b/drivers/iommu/intel-iommu.c
> > @@ -865,7 +865,7 @@ static void free_context_table(struct intel_iommu *iommu)
> > static struct dma_pte *pfn_to_dma_pte(struct dmar_domain *domain,
> > unsigned long pfn, int *target_level)
> > {
> > - struct dma_pte *parent, *pte = NULL;
> > + struct dma_pte *parent, *pte;
> > int level = agaw_to_level(domain->agaw);
> > int offset;
> >
> > @@ -922,7 +922,7 @@ static struct dma_pte *dma_pfn_level_pte(struct dmar_domain *domain,
> > unsigned long pfn,
> > int level, int *large_page)
> > {
> > - struct dma_pte *parent, *pte = NULL;
> > + struct dma_pte *parent, *pte;
> > int total = agaw_to_level(domain->agaw);
> > int offset;
> >
> > @@ -954,7 +954,7 @@ static void dma_pte_clear_range(struct dmar_domain *domain,
> > unsigned long start_pfn,
> > unsigned long last_pfn)
> > {
> > - unsigned int large_page = 1;
> > + unsigned int large_page;
> > struct dma_pte *first_pte, *pte;
> >
> > BUG_ON(!domain_pfn_supported(domain, start_pfn));
> > @@ -1132,7 +1132,7 @@ static struct page *domain_unmap(struct dmar_domain *domain,
> > unsigned long start_pfn,
> > unsigned long last_pfn)
> > {
> > - struct page *freelist = NULL;
> > + struct page *freelist;
>
> I am afraid this change might cause problem. "freelist" might go through
> dma_pte_clear_level() without any touches.
Thanks for your review! Can you clarify your concern? "freelist"
isn't passed into dma_pte_clear_level(). Here's the existing code
(before this patch). I don't see a use of "freelist" before we assign
the result of dma_pte_clear_level() to it. But maybe I'm overlooking
something.
static struct page *domain_unmap(struct dmar_domain *domain,
unsigned long start_pfn,
unsigned long last_pfn)
{
struct page *freelist = NULL;
BUG_ON(!domain_pfn_supported(domain, start_pfn));
BUG_ON(!domain_pfn_supported(domain, last_pfn));
BUG_ON(start_pfn > last_pfn);
/* we don't need lock here; nobody else touches the iova range */
freelist = dma_pte_clear_level(domain, agaw_to_level(domain->agaw),
domain->pgd, 0, start_pfn,
last_pfn, NULL);
Hi,
On 2/11/19 11:33 AM, Bjorn Helgaas wrote:
> On Sun, Feb 10, 2019 at 8:00 PM Lu Baolu <[email protected]> wrote:
>>
>> Hi,
>>
>> On 2/9/19 6:06 AM, Bjorn Helgaas wrote:
>>> From: Bjorn Helgaas <[email protected]>
>>>
>>> A local variable initialization is a hint that the variable will be used in
>>> an unusual way. If the initialization is unnecessary, that hint becomes a
>>> distraction.
>>>
>>> Remove unnecessary initializations. No functional change intended.
>>>
>>> Signed-off-by: Bjorn Helgaas <[email protected]>
>>> ---
>>> drivers/iommu/intel-iommu.c | 27 +++++++++++++--------------
>>> 1 file changed, 13 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
>>> index 81077803880f..2acd08c82cdc 100644
>>> --- a/drivers/iommu/intel-iommu.c
>>> +++ b/drivers/iommu/intel-iommu.c
>>> @@ -865,7 +865,7 @@ static void free_context_table(struct intel_iommu *iommu)
>>> static struct dma_pte *pfn_to_dma_pte(struct dmar_domain *domain,
>>> unsigned long pfn, int *target_level)
>>> {
>>> - struct dma_pte *parent, *pte = NULL;
>>> + struct dma_pte *parent, *pte;
>>> int level = agaw_to_level(domain->agaw);
>>> int offset;
>>>
>>> @@ -922,7 +922,7 @@ static struct dma_pte *dma_pfn_level_pte(struct dmar_domain *domain,
>>> unsigned long pfn,
>>> int level, int *large_page)
>>> {
>>> - struct dma_pte *parent, *pte = NULL;
>>> + struct dma_pte *parent, *pte;
>>> int total = agaw_to_level(domain->agaw);
>>> int offset;
>>>
>>> @@ -954,7 +954,7 @@ static void dma_pte_clear_range(struct dmar_domain *domain,
>>> unsigned long start_pfn,
>>> unsigned long last_pfn)
>>> {
>>> - unsigned int large_page = 1;
>>> + unsigned int large_page;
>>> struct dma_pte *first_pte, *pte;
>>>
>>> BUG_ON(!domain_pfn_supported(domain, start_pfn));
>>> @@ -1132,7 +1132,7 @@ static struct page *domain_unmap(struct dmar_domain *domain,
>>> unsigned long start_pfn,
>>> unsigned long last_pfn)
>>> {
>>> - struct page *freelist = NULL;
>>> + struct page *freelist;
>>
>> I am afraid this change might cause problem. "freelist" might go through
>> dma_pte_clear_level() without any touches.
>
> Thanks for your review! Can you clarify your concern? "freelist"
> isn't passed into dma_pte_clear_level(). Here's the existing code
Oh!
Yes, you are right. I confused it with another function. Sorry about it.
Best regards,
Lu Baolu
On Fri, Feb 08, 2019 at 04:05:33PM -0600, Bjorn Helgaas wrote:
> I've had these minor iommu cleanups lying around for a while, but the ugly
> dmesg logs from [1] prompted me to finally post them. Take what you like,
> ignore the rest, and tell me so I can clear out my queue of old stuff.
>
> These fix no actual bugs.
>
> [1] https://lore.kernel.org/linux-pci/[email protected]
Applied patches 1-6 to their respective branches, thanks Bjorn.