[PATCH v1 0/7] iommu: Minor cleanups and dev_printk() usage

[PATCH v1 0/7] iommu: Minor cleanups and dev_printk() usage

[Bjorn Helgaas]

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/1547649064-19019-1-git-send-email-liudongdong3@huawei.com

---

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(-)

[PATCH v1 1/7] iommu: Use dev_printk() when possible

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

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 <bhelgaas@google.com>
---
 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:

[PATCH v1 2/7] iommu/amd: Use dev_printk() when possible

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Use dev_printk() when possible so the IOMMU messages are more consistent
with other messages related to the device.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 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(&region->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]);

[PATCH v1 3/7] iommu/vt-d: Use dev_printk() when possible

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Use dev_printk() when possible so the IOMMU messages are more consistent
with other messages related to the device.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 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;
        }
 }

[PATCH v1 4/7] iommu/vt-d: Remove unnecessary local variable initializations

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

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 <bhelgaas@google.com>
---
 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,

[PATCH v1 5/7] iommu/vt-d: Remove unused dmar_remove_one_dev_info() argument

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

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 <bhelgaas@google.com>
---
 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,

[PATCH v1 6/7] iommu/vt-d: Remove misleading "domain 0" test from domain_exit()

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

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 <bhelgaas@google.com>
---
 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);

[PATCH v1 7/7] iommu/vt-d: Simplify control flow

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Simplify control flow by returning immediately when we know the result.
No functional change intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 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;
 }
 

Re: [PATCH v1 4/7] iommu/vt-d: Remove unnecessary local variable initializations

[Lu Baolu]

Hi,

On 2/9/19 6:06 AM, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> 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 <bhelgaas@google.com>
> ---
>   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

Re: [PATCH v1 4/7] iommu/vt-d: Remove unnecessary local variable initializations

[Bjorn Helgaas]

On Sun, Feb 10, 2019 at 8:00 PM Lu Baolu <baolu.lu@linux.intel.com> wrote:
>
> Hi,
>
> On 2/9/19 6:06 AM, Bjorn Helgaas wrote:
> > From: Bjorn Helgaas <bhelgaas@google.com>
> >
> > 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 <bhelgaas@google.com>
> > ---
> >   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);

Re: [PATCH v1 4/7] iommu/vt-d: Remove unnecessary local variable initializations

[Lu Baolu]

Hi,

On 2/11/19 11:33 AM, Bjorn Helgaas wrote:
> On Sun, Feb 10, 2019 at 8:00 PM Lu Baolu <baolu.lu@linux.intel.com> wrote:
>>
>> Hi,
>>
>> On 2/9/19 6:06 AM, Bjorn Helgaas wrote:
>>> From: Bjorn Helgaas <bhelgaas@google.com>
>>>
>>> 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 <bhelgaas@google.com>
>>> ---
>>>    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

Re: [PATCH v1 0/7] iommu: Minor cleanups and dev_printk() usage

[Joerg Roedel]

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/1547649064-19019-1-git-send-email-liudongdong3@huawei.com

Applied patches 1-6 to their respective branches, thanks Bjorn.