[PATCH 00/11 v3] Let IOMMU core know about individual IOMMUs

[PATCH 00/11 v3] Let IOMMU core know about individual IOMMUs

[Joerg Roedel]

Changes since v2:

	* Fixed build issues resulting from wrong patch
	  order, everything should be nicely bisectable now
	* Introduced wrapper functions around
	  'struct iommu_device' members, to keep the struct
	  empty when !CONFIG_IOMMU_API
	* Renamed the iommu-devices on Exynos as suggested
	  by Marek Szyprowski

Changes since v1:

	* Rebased to v4.10-rc7
	* Fixed build failures found by the
	  kbuild test robot

Hi,

the IOMMU core code already has two ways of representing
individual hardware IOMMUs. One is the sysfs code and the
other is the newer iommu_register_instance interface. These
two interfaces are special purpose and can be unified.

This unification is attempted in this patch-set. It
introduces an extensible 'struct iommu_device' which
represents a hardware IOMMU in the IOMMU core code.

For now the struct contains a pointer to the iommu_ops, which
is a step to get rid of the per-bus iommu_ops, and a pointer
to a firmware-node structure.

The patches have been tested on x86 hardware and an AMD Seattle
ARM64 system.

Please review, test and provide feedback!

Thanks a lot,

       Joerg

Joerg Roedel (11):
  iommu: Rename iommu_get_instance()
  iommu: Rename struct iommu_device
  iommu: Introduce new 'struct iommu_device'
  iommu: Add sysfs bindings for struct iommu_device
  iommu: Make iommu_device_link/unlink take a struct iommu_device
  iommu: Add iommu_device_set_fwnode() interface
  iommu/arm-smmu: Make use of the iommu_register interface
  iommu/msm: Make use of iommu_device_register interface
  iommu/mediatek: Make use of iommu_device_register interface
  iommu/exynos: Make use of iommu_device_register interface
  iommu: Remove iommu_register_instance interface

 drivers/acpi/arm64/iort.c       |  2 +-
 drivers/iommu/amd_iommu.c       | 18 +++++----
 drivers/iommu/amd_iommu_init.c  |  9 +++--
 drivers/iommu/amd_iommu_types.h |  4 +-
 drivers/iommu/arm-smmu-v3.c     | 22 ++++++++++-
 drivers/iommu/arm-smmu.c        | 31 ++++++++++++++-
 drivers/iommu/dmar.c            | 20 ++++++----
 drivers/iommu/exynos-iommu.c    | 16 +++++++-
 drivers/iommu/intel-iommu.c     | 19 +++++----
 drivers/iommu/iommu-sysfs.c     | 61 +++++++++++++----------------
 drivers/iommu/iommu.c           | 68 +++++++++++++++------------------
 drivers/iommu/msm_iommu.c       | 73 ++++++++++++++++++++++++++++++++++-
 drivers/iommu/msm_iommu.h       |  3 ++
 drivers/iommu/mtk_iommu.c       | 27 ++++++++++++-
 drivers/iommu/mtk_iommu.h       |  2 +
 include/linux/intel-iommu.h     |  3 +-
 include/linux/iommu.h           | 85 +++++++++++++++++++++++++++++++----------
 include/linux/of_iommu.h        |  8 +---
 18 files changed, 335 insertions(+), 136 deletions(-)

-- 
1.9.1

[PATCH 01/11] iommu: Rename iommu_get_instance()

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

Rename the function to iommu_ops_from_fwnode(), because that
is what the function actually does. The new name is much
more descriptive about what the function does.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/acpi/arm64/iort.c | 2 +-
 drivers/iommu/iommu.c     | 2 +-
 include/linux/iommu.h     | 4 ++--
 include/linux/of_iommu.h  | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index e0d2e6e..3752521 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -536,7 +536,7 @@ static const struct iommu_ops *iort_iommu_xlate(struct device *dev,
 		if (!iort_fwnode)
 			return NULL;
 
-		ops = iommu_get_instance(iort_fwnode);
+		ops = iommu_ops_from_fwnode(iort_fwnode);
 		if (!ops)
 			return NULL;
 
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index dbe7f65..0ee05bb 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1653,7 +1653,7 @@ void iommu_register_instance(struct fwnode_handle *fwnode,
 	spin_unlock(&iommu_instance_lock);
 }
 
-const struct iommu_ops *iommu_get_instance(struct fwnode_handle *fwnode)
+const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode)
 {
 	struct iommu_instance *instance;
 	const struct iommu_ops *ops = NULL;
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 0ff5111..085e1f0 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -354,7 +354,7 @@ int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
 int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids);
 void iommu_register_instance(struct fwnode_handle *fwnode,
 			     const struct iommu_ops *ops);
-const struct iommu_ops *iommu_get_instance(struct fwnode_handle *fwnode);
+const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode);
 
 #else /* CONFIG_IOMMU_API */
 
@@ -590,7 +590,7 @@ static inline void iommu_register_instance(struct fwnode_handle *fwnode,
 }
 
 static inline
-const struct iommu_ops *iommu_get_instance(struct fwnode_handle *fwnode)
+const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode)
 {
 	return NULL;
 }
diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
index 6a7fc50..66fcbc9 100644
--- a/include/linux/of_iommu.h
+++ b/include/linux/of_iommu.h
@@ -39,7 +39,7 @@ static inline void of_iommu_set_ops(struct device_node *np,
 
 static inline const struct iommu_ops *of_iommu_get_ops(struct device_node *np)
 {
-	return iommu_get_instance(&np->fwnode);
+	return iommu_ops_from_fwnode(&np->fwnode);
 }
 
 extern struct of_device_id __iommu_of_table;
-- 
1.9.1

[PATCH 02/11] iommu: Rename struct iommu_device

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

The struct is used to link devices to iommu-groups, so
'struct group_device' is a better name. Further this makes
the name iommu_device available for a struct representing
hardware iommus.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/iommu.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 0ee05bb..e4e8a93 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -55,7 +55,7 @@ struct iommu_group {
 	struct iommu_domain *domain;
 };
 
-struct iommu_device {
+struct group_device {
 	struct list_head list;
 	struct device *dev;
 	char *name;
@@ -374,7 +374,7 @@ static int iommu_group_create_direct_mappings(struct iommu_group *group,
 int iommu_group_add_device(struct iommu_group *group, struct device *dev)
 {
 	int ret, i = 0;
-	struct iommu_device *device;
+	struct group_device *device;
 
 	device = kzalloc(sizeof(*device), GFP_KERNEL);
 	if (!device)
@@ -449,7 +449,7 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)
 void iommu_group_remove_device(struct device *dev)
 {
 	struct iommu_group *group = dev->iommu_group;
-	struct iommu_device *tmp_device, *device = NULL;
+	struct group_device *tmp_device, *device = NULL;
 
 	pr_info("Removing device %s from group %d\n", dev_name(dev), group->id);
 
@@ -484,7 +484,7 @@ void iommu_group_remove_device(struct device *dev)
 
 static int iommu_group_device_count(struct iommu_group *group)
 {
-	struct iommu_device *entry;
+	struct group_device *entry;
 	int ret = 0;
 
 	list_for_each_entry(entry, &group->devices, list)
@@ -507,7 +507,7 @@ static int iommu_group_device_count(struct iommu_group *group)
 static int __iommu_group_for_each_dev(struct iommu_group *group, void *data,
 				      int (*fn)(struct device *, void *))
 {
-	struct iommu_device *device;
+	struct group_device *device;
 	int ret = 0;
 
 	list_for_each_entry(device, &group->devices, list) {
-- 
1.9.1

[PATCH 03/11] iommu: Introduce new 'struct iommu_device'

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

This struct represents one hardware iommu in the iommu core
code. For now it only has the iommu-ops associated with it,
but that will be extended soon.

The register/unregister interface is also added, as well as
making use of it in the Intel and AMD IOMMU drivers.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/amd_iommu.c       |  4 ++--
 drivers/iommu/amd_iommu_init.c  |  5 +++++
 drivers/iommu/amd_iommu_types.h |  3 +++
 drivers/iommu/dmar.c            |  9 +++++++++
 drivers/iommu/intel-iommu.c     |  4 ++--
 drivers/iommu/iommu.c           | 19 +++++++++++++++++++
 include/linux/intel-iommu.h     |  2 ++
 include/linux/iommu.h           | 35 +++++++++++++++++++++++++++++------
 8 files changed, 71 insertions(+), 10 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 3ef0f42..7c11c36 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -112,7 +112,7 @@ struct flush_queue {
  * Domain for untranslated devices - only allocated
  * if iommu=pt passed on kernel cmd line.
  */
-static const struct iommu_ops amd_iommu_ops;
+const struct iommu_ops amd_iommu_ops;
 
 static ATOMIC_NOTIFIER_HEAD(ppr_notifier);
 int amd_iommu_max_glx_val = -1;
@@ -3217,7 +3217,7 @@ static void amd_iommu_apply_dm_region(struct device *dev,
 	WARN_ON_ONCE(reserve_iova(&dma_dom->iovad, start, end) == NULL);
 }
 
-static const struct iommu_ops amd_iommu_ops = {
+const struct iommu_ops amd_iommu_ops = {
 	.capable = amd_iommu_capable,
 	.domain_alloc = amd_iommu_domain_alloc,
 	.domain_free  = amd_iommu_domain_free,
diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index 6799cf9..b7ccfb2 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -94,6 +94,8 @@
  * out of it.
  */
 
+extern const struct iommu_ops amd_iommu_ops;
+
 /*
  * structure describing one IOMMU in the ACPI table. Typically followed by one
  * or more ivhd_entrys.
@@ -1639,6 +1641,9 @@ static int iommu_init_pci(struct amd_iommu *iommu)
 					       amd_iommu_groups, "ivhd%d",
 					       iommu->index);
 
+	iommu_device_set_ops(&iommu->iommu, &amd_iommu_ops);
+	iommu_device_register(&iommu->iommu);
+
 	return pci_enable_device(iommu->dev);
 }
 
diff --git a/drivers/iommu/amd_iommu_types.h b/drivers/iommu/amd_iommu_types.h
index 0d91785..0683505 100644
--- a/drivers/iommu/amd_iommu_types.h
+++ b/drivers/iommu/amd_iommu_types.h
@@ -538,6 +538,9 @@ struct amd_iommu {
 	/* IOMMU sysfs device */
 	struct device *iommu_dev;
 
+	/* Handle for IOMMU core code */
+	struct iommu_device iommu;
+
 	/*
 	 * We can't rely on the BIOS to restore all values on reinit, so we
 	 * need to stash them
diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
index 8ccbd70..8e8a48e 100644
--- a/drivers/iommu/dmar.c
+++ b/drivers/iommu/dmar.c
@@ -74,6 +74,8 @@ struct dmar_res_callback {
 static int alloc_iommu(struct dmar_drhd_unit *drhd);
 static void free_iommu(struct intel_iommu *iommu);
 
+extern const struct iommu_ops intel_iommu_ops;
+
 static void dmar_register_drhd_unit(struct dmar_drhd_unit *drhd)
 {
 	/*
@@ -1086,6 +1088,12 @@ static int alloc_iommu(struct dmar_drhd_unit *drhd)
 			err = PTR_ERR(iommu->iommu_dev);
 			goto err_unmap;
 		}
+
+		iommu_device_set_ops(&iommu->iommu, &intel_iommu_ops);
+
+		err = iommu_device_register(&iommu->iommu);
+		if (err)
+			goto err_unmap;
 	}
 
 	drhd->iommu = iommu;
@@ -1104,6 +1112,7 @@ static int alloc_iommu(struct dmar_drhd_unit *drhd)
 static void free_iommu(struct intel_iommu *iommu)
 {
 	iommu_device_destroy(iommu->iommu_dev);
+	iommu_device_unregister(&iommu->iommu);
 
 	if (iommu->irq) {
 		if (iommu->pr_irq) {
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 8a18525..ca22872 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -547,7 +547,7 @@ static int domain_detach_iommu(struct dmar_domain *domain,
 static DEFINE_SPINLOCK(device_domain_lock);
 static LIST_HEAD(device_domain_list);
 
-static const struct iommu_ops intel_iommu_ops;
+const struct iommu_ops intel_iommu_ops;
 
 static bool translation_pre_enabled(struct intel_iommu *iommu)
 {
@@ -5332,7 +5332,7 @@ struct intel_iommu *intel_svm_device_to_iommu(struct device *dev)
 }
 #endif /* CONFIG_INTEL_IOMMU_SVM */
 
-static const struct iommu_ops intel_iommu_ops = {
+const struct iommu_ops intel_iommu_ops = {
 	.capable	= intel_iommu_capable,
 	.domain_alloc	= intel_iommu_domain_alloc,
 	.domain_free	= intel_iommu_domain_free,
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index e4e8a93..21061da 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -77,6 +77,25 @@ struct iommu_group_attribute iommu_group_attr_##_name =		\
 #define to_iommu_group(_kobj)		\
 	container_of(_kobj, struct iommu_group, kobj)
 
+static LIST_HEAD(iommu_device_list);
+static DEFINE_SPINLOCK(iommu_device_lock);
+
+int iommu_device_register(struct iommu_device *iommu)
+{
+	spin_lock(&iommu_device_lock);
+	list_add_tail(&iommu->list, &iommu_device_list);
+	spin_unlock(&iommu_device_lock);
+
+	return 0;
+}
+
+void iommu_device_unregister(struct iommu_device *iommu)
+{
+	spin_lock(&iommu_device_lock);
+	list_del(&iommu->list);
+	spin_unlock(&iommu_device_lock);
+}
+
 static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
 						 unsigned type);
 static int __iommu_attach_device(struct iommu_domain *domain,
diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
index d49e26c..99a65a3 100644
--- a/include/linux/intel-iommu.h
+++ b/include/linux/intel-iommu.h
@@ -29,6 +29,7 @@
 #include <linux/dma_remapping.h>
 #include <linux/mmu_notifier.h>
 #include <linux/list.h>
+#include <linux/iommu.h>
 #include <asm/cacheflush.h>
 #include <asm/iommu.h>
 
@@ -440,6 +441,7 @@ struct intel_iommu {
 	struct irq_domain *ir_msi_domain;
 #endif
 	struct device	*iommu_dev; /* IOMMU-sysfs device */
+	struct iommu_device iommu;  /* IOMMU core code handle */
 	int		node;
 	u32		flags;      /* Software defined flags */
 };
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 085e1f0..5803c1a 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -204,6 +204,26 @@ struct iommu_ops {
 	unsigned long pgsize_bitmap;
 };
 
+/**
+ * struct iommu_device - IOMMU core representation of one IOMMU hardware
+ *			 instance
+ * @list: Used by the iommu-core to keep a list of registered iommus
+ * @ops: iommu-ops for talking to this iommu
+ */
+struct iommu_device {
+	struct list_head list;
+	const struct iommu_ops *ops;
+};
+
+int  iommu_device_register(struct iommu_device *iommu);
+void iommu_device_unregister(struct iommu_device *iommu);
+
+static inline void iommu_device_set_ops(struct iommu_device *iommu,
+					const struct iommu_ops *ops)
+{
+	iommu->ops = ops;
+}
+
 #define IOMMU_GROUP_NOTIFY_ADD_DEVICE		1 /* Device added */
 #define IOMMU_GROUP_NOTIFY_DEL_DEVICE		2 /* Pre Device removed */
 #define IOMMU_GROUP_NOTIFY_BIND_DRIVER		3 /* Pre Driver bind */
@@ -361,6 +381,7 @@ void iommu_register_instance(struct fwnode_handle *fwnode,
 struct iommu_ops {};
 struct iommu_group {};
 struct iommu_fwspec {};
+struct iommu_device {};
 
 static inline bool iommu_present(struct bus_type *bus)
 {
@@ -546,15 +567,17 @@ static inline int iommu_domain_set_attr(struct iommu_domain *domain,
 	return -EINVAL;
 }
 
-static inline struct device *iommu_device_create(struct device *parent,
-					void *drvdata,
-					const struct attribute_group **groups,
-					const char *fmt, ...)
+static inline int  iommu_device_register(struct iommu_device *iommu)
+{
+	return -ENODEV;
+}
+
+static inline void iommu_device_set_ops(struct iommu_device *iommu,
+					const struct iommu_ops *ops)
 {
-	return ERR_PTR(-ENODEV);
 }
 
-static inline void iommu_device_destroy(struct device *dev)
+static inline void iommu_device_unregister(struct iommu_device *iommu)
 {
 }
 
-- 
1.9.1

[PATCH 04/11] iommu: Add sysfs bindings for struct iommu_device

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

There is currently support for iommu sysfs bindings, but
those need to be implemented in the IOMMU drivers. Add a
more generic version of this by adding a struct device to
struct iommu_device and use that for the sysfs bindings.

Also convert the AMD and Intel IOMMU driver to make use of
it.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/amd_iommu.c       | 14 ++++++++-----
 drivers/iommu/amd_iommu_init.c  |  6 ++----
 drivers/iommu/amd_iommu_types.h |  3 ---
 drivers/iommu/dmar.c            | 13 +++++-------
 drivers/iommu/intel-iommu.c     | 15 ++++++++------
 drivers/iommu/iommu-sysfs.c     | 45 +++++++++++++++++------------------------
 include/linux/intel-iommu.h     |  1 -
 include/linux/iommu.h           | 23 +++++++++++++++++----
 8 files changed, 62 insertions(+), 58 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 7c11c36..a584ac2 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -445,6 +445,7 @@ static void init_iommu_group(struct device *dev)
 static int iommu_init_device(struct device *dev)
 {
 	struct iommu_dev_data *dev_data;
+	struct amd_iommu *iommu;
 	int devid;
 
 	if (dev->archdata.iommu)
@@ -454,6 +455,8 @@ static int iommu_init_device(struct device *dev)
 	if (devid < 0)
 		return devid;
 
+	iommu = amd_iommu_rlookup_table[devid];
+
 	dev_data = find_dev_data(devid);
 	if (!dev_data)
 		return -ENOMEM;
@@ -469,8 +472,7 @@ static int iommu_init_device(struct device *dev)
 
 	dev->archdata.iommu = dev_data;
 
-	iommu_device_link(amd_iommu_rlookup_table[dev_data->devid]->iommu_dev,
-			  dev);
+	iommu_device_link(&iommu->iommu.dev, dev);
 
 	return 0;
 }
@@ -495,13 +497,16 @@ static void iommu_ignore_device(struct device *dev)
 
 static void iommu_uninit_device(struct device *dev)
 {
-	int devid;
 	struct iommu_dev_data *dev_data;
+	struct amd_iommu *iommu;
+	int devid;
 
 	devid = get_device_id(dev);
 	if (devid < 0)
 		return;
 
+	iommu = amd_iommu_rlookup_table[devid];
+
 	dev_data = search_dev_data(devid);
 	if (!dev_data)
 		return;
@@ -509,8 +514,7 @@ static void iommu_uninit_device(struct device *dev)
 	if (dev_data->domain)
 		detach_device(dev);
 
-	iommu_device_unlink(amd_iommu_rlookup_table[dev_data->devid]->iommu_dev,
-			    dev);
+	iommu_device_unlink(&iommu->iommu.dev, dev);
 
 	iommu_group_remove_device(dev);
 
diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index b7ccfb2..6b9e661 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -1637,10 +1637,8 @@ static int iommu_init_pci(struct amd_iommu *iommu)
 	amd_iommu_erratum_746_workaround(iommu);
 	amd_iommu_ats_write_check_workaround(iommu);
 
-	iommu->iommu_dev = iommu_device_create(&iommu->dev->dev, iommu,
-					       amd_iommu_groups, "ivhd%d",
-					       iommu->index);
-
+	iommu_device_sysfs_add(&iommu->iommu, &iommu->dev->dev,
+			       amd_iommu_groups, "ivhd%d", iommu->index);
 	iommu_device_set_ops(&iommu->iommu, &amd_iommu_ops);
 	iommu_device_register(&iommu->iommu);
 
diff --git a/drivers/iommu/amd_iommu_types.h b/drivers/iommu/amd_iommu_types.h
index 0683505..af00f38 100644
--- a/drivers/iommu/amd_iommu_types.h
+++ b/drivers/iommu/amd_iommu_types.h
@@ -535,9 +535,6 @@ struct amd_iommu {
 	/* if one, we need to send a completion wait command */
 	bool need_sync;
 
-	/* IOMMU sysfs device */
-	struct device *iommu_dev;
-
 	/* Handle for IOMMU core code */
 	struct iommu_device iommu;
 
diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
index 8e8a48e..d9c0dec 100644
--- a/drivers/iommu/dmar.c
+++ b/drivers/iommu/dmar.c
@@ -1080,14 +1080,11 @@ static int alloc_iommu(struct dmar_drhd_unit *drhd)
 	raw_spin_lock_init(&iommu->register_lock);
 
 	if (intel_iommu_enabled) {
-		iommu->iommu_dev = iommu_device_create(NULL, iommu,
-						       intel_iommu_groups,
-						       "%s", iommu->name);
-
-		if (IS_ERR(iommu->iommu_dev)) {
-			err = PTR_ERR(iommu->iommu_dev);
+		err = iommu_device_sysfs_add(&iommu->iommu, NULL,
+					     intel_iommu_groups,
+					     "%s", iommu->name);
+		if (err)
 			goto err_unmap;
-		}
 
 		iommu_device_set_ops(&iommu->iommu, &intel_iommu_ops);
 
@@ -1111,7 +1108,7 @@ static int alloc_iommu(struct dmar_drhd_unit *drhd)
 
 static void free_iommu(struct intel_iommu *iommu)
 {
-	iommu_device_destroy(iommu->iommu_dev);
+	iommu_device_sysfs_remove(&iommu->iommu);
 	iommu_device_unregister(&iommu->iommu);
 
 	if (iommu->irq) {
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index ca22872..0e52086 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -4853,10 +4853,13 @@ int __init intel_iommu_init(void)
 
 	init_iommu_pm_ops();
 
-	for_each_active_iommu(iommu, drhd)
-		iommu->iommu_dev = iommu_device_create(NULL, iommu,
-						       intel_iommu_groups,
-						       "%s", iommu->name);
+	for_each_active_iommu(iommu, drhd) {
+		iommu_device_sysfs_add(&iommu->iommu, NULL,
+				       intel_iommu_groups,
+				       "%s", iommu->name);
+		iommu_device_set_ops(&iommu->iommu, &intel_iommu_ops);
+		iommu_device_register(&iommu->iommu);
+	}
 
 	bus_set_iommu(&pci_bus_type, &intel_iommu_ops);
 	bus_register_notifier(&pci_bus_type, &device_nb);
@@ -5178,7 +5181,7 @@ static int intel_iommu_add_device(struct device *dev)
 	if (!iommu)
 		return -ENODEV;
 
-	iommu_device_link(iommu->iommu_dev, dev);
+	iommu_device_link(&iommu->iommu.dev, dev);
 
 	group = iommu_group_get_for_dev(dev);
 
@@ -5200,7 +5203,7 @@ static void intel_iommu_remove_device(struct device *dev)
 
 	iommu_group_remove_device(dev);
 
-	iommu_device_unlink(iommu->iommu_dev, dev);
+	iommu_device_unlink(&iommu->iommu.dev, dev);
 }
 
 #ifdef CONFIG_INTEL_IOMMU_SVM
diff --git a/drivers/iommu/iommu-sysfs.c b/drivers/iommu/iommu-sysfs.c
index 39b2d91..bb87d35 100644
--- a/drivers/iommu/iommu-sysfs.c
+++ b/drivers/iommu/iommu-sysfs.c
@@ -50,54 +50,45 @@ static int __init iommu_dev_init(void)
 postcore_initcall(iommu_dev_init);
 
 /*
- * Create an IOMMU device and return a pointer to it.  IOMMU specific
- * attributes can be provided as an attribute group, allowing a unique
- * namespace per IOMMU type.
+ * Init the struct device for the IOMMU. IOMMU specific attributes can
+ * be provided as an attribute group, allowing a unique namespace per
+ * IOMMU type.
  */
-struct device *iommu_device_create(struct device *parent, void *drvdata,
-				   const struct attribute_group **groups,
-				   const char *fmt, ...)
+int iommu_device_sysfs_add(struct iommu_device *iommu,
+			   struct device *parent,
+			   const struct attribute_group **groups,
+			   const char *fmt, ...)
 {
-	struct device *dev;
 	va_list vargs;
 	int ret;
 
-	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
-	if (!dev)
-		return ERR_PTR(-ENOMEM);
+	device_initialize(&iommu->dev);
 
-	device_initialize(dev);
-
-	dev->class = &iommu_class;
-	dev->parent = parent;
-	dev->groups = groups;
-	dev_set_drvdata(dev, drvdata);
+	iommu->dev.class = &iommu_class;
+	iommu->dev.parent = parent;
+	iommu->dev.groups = groups;
 
 	va_start(vargs, fmt);
-	ret = kobject_set_name_vargs(&dev->kobj, fmt, vargs);
+	ret = kobject_set_name_vargs(&iommu->dev.kobj, fmt, vargs);
 	va_end(vargs);
 	if (ret)
 		goto error;
 
-	ret = device_add(dev);
+	ret = device_add(&iommu->dev);
 	if (ret)
 		goto error;
 
-	return dev;
+	return 0;
 
 error:
-	put_device(dev);
-	return ERR_PTR(ret);
+	put_device(&iommu->dev);
+	return ret;
 }
 
-void iommu_device_destroy(struct device *dev)
+void iommu_device_sysfs_remove(struct iommu_device *iommu)
 {
-	if (!dev || IS_ERR(dev))
-		return;
-
-	device_unregister(dev);
+	device_unregister(&iommu->dev);
 }
-
 /*
  * IOMMU drivers can indicate a device is managed by a given IOMMU using
  * this interface.  A link to the device will be created in the "devices"
diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
index 99a65a3..3ba9b53 100644
--- a/include/linux/intel-iommu.h
+++ b/include/linux/intel-iommu.h
@@ -440,7 +440,6 @@ struct intel_iommu {
 	struct irq_domain *ir_domain;
 	struct irq_domain *ir_msi_domain;
 #endif
-	struct device	*iommu_dev; /* IOMMU-sysfs device */
 	struct iommu_device iommu;  /* IOMMU core code handle */
 	int		node;
 	u32		flags;      /* Software defined flags */
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 5803c1a..c578ca1 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -209,14 +209,21 @@ struct iommu_ops {
  *			 instance
  * @list: Used by the iommu-core to keep a list of registered iommus
  * @ops: iommu-ops for talking to this iommu
+ * @dev: struct device for sysfs handling
  */
 struct iommu_device {
 	struct list_head list;
 	const struct iommu_ops *ops;
+	struct device dev;
 };
 
 int  iommu_device_register(struct iommu_device *iommu);
 void iommu_device_unregister(struct iommu_device *iommu);
+int  iommu_device_sysfs_add(struct iommu_device *iommu,
+			    struct device *parent,
+			    const struct attribute_group **groups,
+			    const char *fmt, ...) __printf(4, 5);
+void iommu_device_sysfs_remove(struct iommu_device *iommu);
 
 static inline void iommu_device_set_ops(struct iommu_device *iommu,
 					const struct iommu_ops *ops)
@@ -287,10 +294,6 @@ extern int iommu_domain_get_attr(struct iommu_domain *domain, enum iommu_attr,
 				 void *data);
 extern int iommu_domain_set_attr(struct iommu_domain *domain, enum iommu_attr,
 				 void *data);
-struct device *iommu_device_create(struct device *parent, void *drvdata,
-				   const struct attribute_group **groups,
-				   const char *fmt, ...) __printf(4, 5);
-void iommu_device_destroy(struct device *dev);
 int iommu_device_link(struct device *dev, struct device *link);
 void iommu_device_unlink(struct device *dev, struct device *link);
 
@@ -581,6 +584,18 @@ static inline void iommu_device_unregister(struct iommu_device *iommu)
 {
 }
 
+static inline int  iommu_device_sysfs_add(struct iommu_device *iommu,
+					  struct device *parent,
+					  const struct attribute_group **groups,
+					  const char *fmt, ...)
+{
+	return -ENODEV;
+}
+
+static inline void iommu_device_sysfs_remove(struct iommu_device *iommu)
+{
+}
+
 static inline int iommu_device_link(struct device *dev, struct device *link)
 {
 	return -EINVAL;
-- 
1.9.1

[PATCH 05/11] iommu: Make iommu_device_link/unlink take a struct iommu_device

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

This makes the interface more consistent with
iommu_device_sysfs_add/remove.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/amd_iommu.c   |  4 ++--
 drivers/iommu/intel-iommu.c |  4 ++--
 drivers/iommu/iommu-sysfs.c | 16 ++++++++--------
 include/linux/iommu.h       |  4 ++--
 4 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index a584ac2..b8adfcc 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -472,7 +472,7 @@ static int iommu_init_device(struct device *dev)
 
 	dev->archdata.iommu = dev_data;
 
-	iommu_device_link(&iommu->iommu.dev, dev);
+	iommu_device_link(&iommu->iommu, dev);
 
 	return 0;
 }
@@ -514,7 +514,7 @@ static void iommu_uninit_device(struct device *dev)
 	if (dev_data->domain)
 		detach_device(dev);
 
-	iommu_device_unlink(&iommu->iommu.dev, dev);
+	iommu_device_unlink(&iommu->iommu, dev);
 
 	iommu_group_remove_device(dev);
 
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 0e52086..e1e8ec4 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -5181,7 +5181,7 @@ static int intel_iommu_add_device(struct device *dev)
 	if (!iommu)
 		return -ENODEV;
 
-	iommu_device_link(&iommu->iommu.dev, dev);
+	iommu_device_link(&iommu->iommu, dev);
 
 	group = iommu_group_get_for_dev(dev);
 
@@ -5203,7 +5203,7 @@ static void intel_iommu_remove_device(struct device *dev)
 
 	iommu_group_remove_device(dev);
 
-	iommu_device_unlink(&iommu->iommu.dev, dev);
+	iommu_device_unlink(&iommu->iommu, dev);
 }
 
 #ifdef CONFIG_INTEL_IOMMU_SVM
diff --git a/drivers/iommu/iommu-sysfs.c b/drivers/iommu/iommu-sysfs.c
index bb87d35..c58351e 100644
--- a/drivers/iommu/iommu-sysfs.c
+++ b/drivers/iommu/iommu-sysfs.c
@@ -95,31 +95,31 @@ void iommu_device_sysfs_remove(struct iommu_device *iommu)
  * directory of the IOMMU device in sysfs and an "iommu" link will be
  * created under the linked device, pointing back at the IOMMU device.
  */
-int iommu_device_link(struct device *dev, struct device *link)
+int iommu_device_link(struct iommu_device *iommu, struct device *link)
 {
 	int ret;
 
-	if (!dev || IS_ERR(dev))
+	if (!iommu || IS_ERR(iommu))
 		return -ENODEV;
 
-	ret = sysfs_add_link_to_group(&dev->kobj, "devices",
+	ret = sysfs_add_link_to_group(&iommu->dev.kobj, "devices",
 				      &link->kobj, dev_name(link));
 	if (ret)
 		return ret;
 
-	ret = sysfs_create_link_nowarn(&link->kobj, &dev->kobj, "iommu");
+	ret = sysfs_create_link_nowarn(&link->kobj, &iommu->dev.kobj, "iommu");
 	if (ret)
-		sysfs_remove_link_from_group(&dev->kobj, "devices",
+		sysfs_remove_link_from_group(&iommu->dev.kobj, "devices",
 					     dev_name(link));
 
 	return ret;
 }
 
-void iommu_device_unlink(struct device *dev, struct device *link)
+void iommu_device_unlink(struct iommu_device *iommu, struct device *link)
 {
-	if (!dev || IS_ERR(dev))
+	if (!iommu || IS_ERR(iommu))
 		return;
 
 	sysfs_remove_link(&link->kobj, "iommu");
-	sysfs_remove_link_from_group(&dev->kobj, "devices", dev_name(link));
+	sysfs_remove_link_from_group(&iommu->dev.kobj, "devices", dev_name(link));
 }
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index c578ca1..bae3cfc 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -224,6 +224,8 @@ int  iommu_device_sysfs_add(struct iommu_device *iommu,
 			    const struct attribute_group **groups,
 			    const char *fmt, ...) __printf(4, 5);
 void iommu_device_sysfs_remove(struct iommu_device *iommu);
+int  iommu_device_link(struct iommu_device   *iommu, struct device *link);
+void iommu_device_unlink(struct iommu_device *iommu, struct device *link);
 
 static inline void iommu_device_set_ops(struct iommu_device *iommu,
 					const struct iommu_ops *ops)
@@ -294,8 +296,6 @@ extern int iommu_domain_get_attr(struct iommu_domain *domain, enum iommu_attr,
 				 void *data);
 extern int iommu_domain_set_attr(struct iommu_domain *domain, enum iommu_attr,
 				 void *data);
-int iommu_device_link(struct device *dev, struct device *link);
-void iommu_device_unlink(struct device *dev, struct device *link);
 
 /* Window handling function prototypes */
 extern int iommu_domain_window_enable(struct iommu_domain *domain, u32 wnd_nr,
-- 
1.9.1

[PATCH 06/11] iommu: Add iommu_device_set_fwnode() interface

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

Allow to store a fwnode in 'struct iommu_device';

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 include/linux/iommu.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index bae3cfc..626c935 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -214,6 +214,7 @@ struct iommu_ops {
 struct iommu_device {
 	struct list_head list;
 	const struct iommu_ops *ops;
+	struct fwnode_handle *fwnode;
 	struct device dev;
 };
 
@@ -233,6 +234,12 @@ static inline void iommu_device_set_ops(struct iommu_device *iommu,
 	iommu->ops = ops;
 }
 
+static inline void iommu_device_set_fwnode(struct iommu_device *iommu,
+					   struct fwnode_handle *fwnode)
+{
+	iommu->fwnode = fwnode;
+}
+
 #define IOMMU_GROUP_NOTIFY_ADD_DEVICE		1 /* Device added */
 #define IOMMU_GROUP_NOTIFY_DEL_DEVICE		2 /* Pre Device removed */
 #define IOMMU_GROUP_NOTIFY_BIND_DRIVER		3 /* Pre Driver bind */
@@ -580,6 +587,11 @@ static inline void iommu_device_set_ops(struct iommu_device *iommu,
 {
 }
 
+static inline void iommu_device_set_fwnode(struct iommu_device *iommu,
+					   struct fwnode_handle *fwnode)
+{
+}
+
 static inline void iommu_device_unregister(struct iommu_device *iommu)
 {
 }
-- 
1.9.1

[PATCH 07/11] iommu/arm-smmu: Make use of the iommu_register interface

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

Also add the smmu devices to sysfs.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/arm-smmu-v3.c | 22 +++++++++++++++++++++-
 drivers/iommu/arm-smmu.c    | 30 ++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 4d6ec44..32133e2 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -616,6 +616,9 @@ struct arm_smmu_device {
 	unsigned int			sid_bits;
 
 	struct arm_smmu_strtab_cfg	strtab_cfg;
+
+	/* IOMMU core code handle */
+	struct iommu_device		iommu;
 };
 
 /* SMMU private data for each master */
@@ -1795,8 +1798,10 @@ static int arm_smmu_add_device(struct device *dev)
 	}
 
 	group = iommu_group_get_for_dev(dev);
-	if (!IS_ERR(group))
+	if (!IS_ERR(group)) {
 		iommu_group_put(group);
+		iommu_device_link(&smmu->iommu, dev);
+	}
 
 	return PTR_ERR_OR_ZERO(group);
 }
@@ -1805,14 +1810,17 @@ static void arm_smmu_remove_device(struct device *dev)
 {
 	struct iommu_fwspec *fwspec = dev->iommu_fwspec;
 	struct arm_smmu_master_data *master;
+	struct arm_smmu_device *smmu;
 
 	if (!fwspec || fwspec->ops != &arm_smmu_ops)
 		return;
 
 	master = fwspec->iommu_priv;
+	smmu = master->smmu;
 	if (master && master->ste.valid)
 		arm_smmu_detach_dev(dev);
 	iommu_group_remove_device(dev);
+	iommu_device_unlink(&smmu->iommu, dev);
 	kfree(master);
 	iommu_fwspec_free(dev);
 }
@@ -2613,6 +2621,7 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 {
 	int irq, ret;
 	struct resource *res;
+	resource_size_t ioaddr;
 	struct arm_smmu_device *smmu;
 	struct device *dev = &pdev->dev;
 	bool bypass;
@@ -2630,6 +2639,7 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 		dev_err(dev, "MMIO region too small (%pr)\n", res);
 		return -EINVAL;
 	}
+	ioaddr = res->start;
 
 	smmu->base = devm_ioremap_resource(dev, res);
 	if (IS_ERR(smmu->base))
@@ -2682,6 +2692,16 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 		return ret;
 
 	/* And we're up. Go go go! */
+	ret = iommu_device_sysfs_add(&smmu->iommu, dev, NULL,
+				     "smmu3.%pa", &ioaddr);
+	if (ret)
+		return ret;
+
+	iommu_device_set_ops(&smmu->iommu, &arm_smmu_ops);
+	iommu_device_set_fwnode(&smmu->iommu, dev->fwnode);
+
+	ret = iommu_device_register(&smmu->iommu);
+
 	iommu_register_instance(dev->fwnode, &arm_smmu_ops);
 
 #ifdef CONFIG_PCI
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index a60cded..f4ce1e7 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -380,6 +380,9 @@ struct arm_smmu_device {
 	unsigned int			*irqs;
 
 	u32				cavium_id_base; /* Specific to Cavium */
+
+	/* IOMMU core code handle */
+	struct iommu_device		iommu;
 };
 
 enum arm_smmu_context_fmt {
@@ -1444,6 +1447,8 @@ static int arm_smmu_add_device(struct device *dev)
 	if (ret)
 		goto out_free;
 
+	iommu_device_link(&smmu->iommu, dev);
+
 	return 0;
 
 out_free:
@@ -1456,10 +1461,17 @@ static int arm_smmu_add_device(struct device *dev)
 static void arm_smmu_remove_device(struct device *dev)
 {
 	struct iommu_fwspec *fwspec = dev->iommu_fwspec;
+	struct arm_smmu_master_cfg *cfg;
+	struct arm_smmu_device *smmu;
+
 
 	if (!fwspec || fwspec->ops != &arm_smmu_ops)
 		return;
 
+	cfg  = fwspec->iommu_priv;
+	smmu = cfg->smmu;
+
+	iommu_device_unlink(&smmu->iommu, dev);
 	arm_smmu_master_free_smes(fwspec);
 	iommu_group_remove_device(dev);
 	kfree(fwspec->iommu_priv);
@@ -2011,6 +2023,7 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev,
 static int arm_smmu_device_probe(struct platform_device *pdev)
 {
 	struct resource *res;
+	resource_size_t ioaddr;
 	struct arm_smmu_device *smmu;
 	struct device *dev = &pdev->dev;
 	int num_irqs, i, err;
@@ -2031,6 +2044,7 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 		return err;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	ioaddr = res->start;
 	smmu->base = devm_ioremap_resource(dev, res);
 	if (IS_ERR(smmu->base))
 		return PTR_ERR(smmu->base);
@@ -2091,6 +2105,22 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 		}
 	}
 
+	err = iommu_device_sysfs_add(&smmu->iommu, smmu->dev, NULL,
+				     "smmu.%pa", &ioaddr);
+	if (err) {
+		dev_err(dev, "Failed to register iommu in sysfs\n");
+		return err;
+	}
+
+	iommu_device_set_ops(&smmu->iommu, &arm_smmu_ops);
+	iommu_device_set_fwnode(&smmu->iommu, dev->fwnode);
+
+	err = iommu_device_register(&smmu->iommu);
+	if (err) {
+		dev_err(dev, "Failed to register iommu\n");
+		return err;
+	}
+
 	iommu_register_instance(dev->fwnode, &arm_smmu_ops);
 	platform_set_drvdata(pdev, smmu);
 	arm_smmu_device_reset(smmu);
-- 
1.9.1

[PATCH 08/11] iommu/msm: Make use of iommu_device_register interface

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

Register the MSM IOMMUs to the iommu core and add sysfs
entries for that driver.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/msm_iommu.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/iommu/msm_iommu.h |  3 ++
 2 files changed, 76 insertions(+)

diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c
index b09692b..30795cb 100644
--- a/drivers/iommu/msm_iommu.c
+++ b/drivers/iommu/msm_iommu.c
@@ -371,6 +371,58 @@ static int msm_iommu_domain_config(struct msm_priv *priv)
 	return 0;
 }
 
+/* Must be called under msm_iommu_lock */
+static struct msm_iommu_dev *find_iommu_for_dev(struct device *dev)
+{
+	struct msm_iommu_dev *iommu, *ret = NULL;
+	struct msm_iommu_ctx_dev *master;
+
+	list_for_each_entry(iommu, &qcom_iommu_devices, dev_node) {
+		master = list_first_entry(&iommu->ctx_list,
+					  struct msm_iommu_ctx_dev,
+					  list);
+		if (master->of_node == dev->of_node) {
+			ret = iommu;
+			break;
+		}
+	}
+
+	return ret;
+}
+
+static int msm_iommu_add_device(struct device *dev)
+{
+	struct msm_iommu_dev *iommu;
+	unsigned long flags;
+	int ret = 0;
+
+	spin_lock_irqsave(&msm_iommu_lock, flags);
+
+	iommu = find_iommu_for_dev(dev);
+	if (iommu)
+		iommu_device_link(&iommu->iommu, dev);
+	else
+		ret = -ENODEV;
+
+	spin_unlock_irqrestore(&msm_iommu_lock, flags);
+
+	return ret;
+}
+
+static void msm_iommu_remove_device(struct device *dev)
+{
+	struct msm_iommu_dev *iommu;
+	unsigned long flags;
+
+	spin_lock_irqsave(&msm_iommu_lock, flags);
+
+	iommu = find_iommu_for_dev(dev);
+	if (iommu)
+		iommu_device_unlink(&iommu->iommu, dev);
+
+	spin_unlock_irqrestore(&msm_iommu_lock, flags);
+}
+
 static int msm_iommu_attach_dev(struct iommu_domain *domain, struct device *dev)
 {
 	int ret = 0;
@@ -646,6 +698,8 @@ irqreturn_t msm_iommu_fault_handler(int irq, void *dev_id)
 	.unmap = msm_iommu_unmap,
 	.map_sg = default_iommu_map_sg,
 	.iova_to_phys = msm_iommu_iova_to_phys,
+	.add_device = msm_iommu_add_device,
+	.remove_device = msm_iommu_remove_device,
 	.pgsize_bitmap = MSM_IOMMU_PGSIZES,
 	.of_xlate = qcom_iommu_of_xlate,
 };
@@ -653,6 +707,7 @@ irqreturn_t msm_iommu_fault_handler(int irq, void *dev_id)
 static int msm_iommu_probe(struct platform_device *pdev)
 {
 	struct resource *r;
+	resource_size_t ioaddr;
 	struct msm_iommu_dev *iommu;
 	int ret, par, val;
 
@@ -696,6 +751,7 @@ static int msm_iommu_probe(struct platform_device *pdev)
 		ret = PTR_ERR(iommu->base);
 		goto fail;
 	}
+	ioaddr = r->start;
 
 	iommu->irq = platform_get_irq(pdev, 0);
 	if (iommu->irq < 0) {
@@ -737,6 +793,23 @@ static int msm_iommu_probe(struct platform_device *pdev)
 	}
 
 	list_add(&iommu->dev_node, &qcom_iommu_devices);
+
+	ret = iommu_device_sysfs_add(&iommu->iommu, iommu->dev, NULL,
+				     "msm-smmu.%pa", &ioaddr);
+	if (ret) {
+		pr_err("Could not add msm-smmu at %pa to sysfs\n", &ioaddr);
+		goto fail;
+	}
+
+	iommu_device_set_ops(&iommu->iommu, &msm_iommu_ops);
+	iommu_device_set_fwnode(&iommu->iommu, &pdev->dev.of_node->fwnode);
+
+	ret = iommu_device_register(&iommu->iommu);
+	if (ret) {
+		pr_err("Could not register msm-smmu at %pa\n", &ioaddr);
+		goto fail;
+	}
+
 	of_iommu_set_ops(pdev->dev.of_node, &msm_iommu_ops);
 
 	pr_info("device mapped at %p, irq %d with %d ctx banks\n",
diff --git a/drivers/iommu/msm_iommu.h b/drivers/iommu/msm_iommu.h
index 4ca25d5..ae92d27 100644
--- a/drivers/iommu/msm_iommu.h
+++ b/drivers/iommu/msm_iommu.h
@@ -19,6 +19,7 @@
 #define MSM_IOMMU_H
 
 #include <linux/interrupt.h>
+#include <linux/iommu.h>
 #include <linux/clk.h>
 
 /* Sharability attributes of MSM IOMMU mappings */
@@ -68,6 +69,8 @@ struct msm_iommu_dev {
 	struct list_head dom_node;
 	struct list_head ctx_list;
 	DECLARE_BITMAP(context_map, IOMMU_MAX_CBS);
+
+	struct iommu_device iommu;
 };
 
 /**
-- 
1.9.1

[PATCH 09/11] iommu/mediatek: Make use of iommu_device_register interface

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

Register individual Mediatek IOMMUs to the iommu core and
add sysfs entries.

Cc: Matthias Brugger <matthias.bgg@gmail.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-mediatek@lists.infradead.org
Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/mtk_iommu.c | 26 ++++++++++++++++++++++++++
 drivers/iommu/mtk_iommu.h |  2 ++
 2 files changed, 28 insertions(+)

diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 1479c76..d484fa6 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -360,11 +360,15 @@ static phys_addr_t mtk_iommu_iova_to_phys(struct iommu_domain *domain,
 
 static int mtk_iommu_add_device(struct device *dev)
 {
+	struct mtk_iommu_data *data;
 	struct iommu_group *group;
 
 	if (!dev->iommu_fwspec || dev->iommu_fwspec->ops != &mtk_iommu_ops)
 		return -ENODEV; /* Not a iommu client device */
 
+	data = dev->iommu_fwspec->iommu_priv;
+	iommu_device_link(&data->iommu, dev);
+
 	group = iommu_group_get_for_dev(dev);
 	if (IS_ERR(group))
 		return PTR_ERR(group);
@@ -375,9 +379,14 @@ static int mtk_iommu_add_device(struct device *dev)
 
 static void mtk_iommu_remove_device(struct device *dev)
 {
+	struct mtk_iommu_data *data;
+
 	if (!dev->iommu_fwspec || dev->iommu_fwspec->ops != &mtk_iommu_ops)
 		return;
 
+	data = dev->iommu_fwspec->iommu_priv;
+	iommu_device_unlink(&data->iommu, dev);
+
 	iommu_group_remove_device(dev);
 	iommu_fwspec_free(dev);
 }
@@ -497,6 +506,7 @@ static int mtk_iommu_probe(struct platform_device *pdev)
 	struct mtk_iommu_data   *data;
 	struct device           *dev = &pdev->dev;
 	struct resource         *res;
+	resource_size_t		ioaddr;
 	struct component_match  *match = NULL;
 	void                    *protect;
 	int                     i, larb_nr, ret;
@@ -519,6 +529,7 @@ static int mtk_iommu_probe(struct platform_device *pdev)
 	data->base = devm_ioremap_resource(dev, res);
 	if (IS_ERR(data->base))
 		return PTR_ERR(data->base);
+	ioaddr = res->start;
 
 	data->irq = platform_get_irq(pdev, 0);
 	if (data->irq < 0)
@@ -567,6 +578,18 @@ static int mtk_iommu_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	ret = iommu_device_sysfs_add(&data->iommu, dev, NULL,
+				     "mtk-iommu.%pa", &ioaddr);
+	if (ret)
+		return ret;
+
+	iommu_device_set_ops(&data->iommu, &mtk_iommu_ops);
+	iommu_device_set_fwnode(&data->iommu, &pdev->dev.of_node->fwnode);
+
+	ret = iommu_device_register(&data->iommu);
+	if (ret)
+		return ret;
+
 	if (!iommu_present(&platform_bus_type))
 		bus_set_iommu(&platform_bus_type, &mtk_iommu_ops);
 
@@ -577,6 +600,9 @@ static int mtk_iommu_remove(struct platform_device *pdev)
 {
 	struct mtk_iommu_data *data = platform_get_drvdata(pdev);
 
+	iommu_device_sysfs_remove(&data->iommu);
+	iommu_device_unregister(&data->iommu);
+
 	if (iommu_present(&platform_bus_type))
 		bus_set_iommu(&platform_bus_type, NULL);
 
diff --git a/drivers/iommu/mtk_iommu.h b/drivers/iommu/mtk_iommu.h
index 50177f7..2a28ead 100644
--- a/drivers/iommu/mtk_iommu.h
+++ b/drivers/iommu/mtk_iommu.h
@@ -47,6 +47,8 @@ struct mtk_iommu_data {
 	struct iommu_group		*m4u_group;
 	struct mtk_smi_iommu		smi_imu;      /* SMI larb iommu info */
 	bool                            enable_4GB;
+
+	struct iommu_device		iommu;
 };
 
 static inline int compare_of(struct device *dev, void *data)
-- 
1.9.1

[PATCH 10/11] iommu/exynos: Make use of iommu_device_register interface

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

Register Exynos IOMMUs to the IOMMU core and make them
visible in sysfs. This patch does not add the links between
IOMMUs and translated devices yet.

Cc: Marek Szyprowski <m.szyprowski@samsung.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-samsung-soc@vger.kernel.org
Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/exynos-iommu.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index 57ba0d3..64325d8 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -276,6 +276,8 @@ struct sysmmu_drvdata {
 	struct list_head owner_node;	/* node for owner controllers list */
 	phys_addr_t pgtable;		/* assigned page table structure */
 	unsigned int version;		/* our version */
+
+	struct iommu_device iommu;	/* IOMMU core handle */
 };
 
 static struct exynos_iommu_domain *to_exynos_domain(struct iommu_domain *dom)
@@ -611,6 +613,18 @@ static int __init exynos_sysmmu_probe(struct platform_device *pdev)
 	data->sysmmu = dev;
 	spin_lock_init(&data->lock);
 
+	ret = iommu_device_sysfs_add(&data->iommu, &pdev->dev, NULL,
+				     dev_name(data->sysmmu));
+	if (ret)
+		return ret;
+
+	iommu_device_set_ops(&data->iommu, &exynos_iommu_ops);
+	iommu_device_set_fwnode(&data->iommu, &dev->of_node->fwnode);
+
+	ret = iommu_device_register(&data->iommu);
+	if (ret)
+		return ret;
+
 	platform_set_drvdata(pdev, data);
 
 	__sysmmu_get_version(data);
-- 
1.9.1

[PATCH 11/11] iommu: Remove iommu_register_instance interface

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

And also move its remaining functionality to
iommu_device_register() and 'struct iommu_device'.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <frowand.list@gmail.com>
Cc: Matthias Brugger <matthias.bgg@gmail.com>
Cc: Marek Szyprowski <m.szyprowski@samsung.com>
Cc: devicetree@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/arm-smmu-v3.c  |  2 --
 drivers/iommu/arm-smmu.c     |  1 -
 drivers/iommu/exynos-iommu.c |  2 --
 drivers/iommu/iommu.c        | 37 ++++++-------------------------------
 drivers/iommu/msm_iommu.c    |  2 --
 drivers/iommu/mtk_iommu.c    |  1 -
 include/linux/iommu.h        |  7 -------
 include/linux/of_iommu.h     |  6 ------
 8 files changed, 6 insertions(+), 52 deletions(-)

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 32133e2..5375137 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -2702,8 +2702,6 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 
 	ret = iommu_device_register(&smmu->iommu);
 
-	iommu_register_instance(dev->fwnode, &arm_smmu_ops);
-
 #ifdef CONFIG_PCI
 	if (pci_bus_type.iommu_ops != &arm_smmu_ops) {
 		pci_request_acs();
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index f4ce1e7..8fb4af2 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -2121,7 +2121,6 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 		return err;
 	}
 
-	iommu_register_instance(dev->fwnode, &arm_smmu_ops);
 	platform_set_drvdata(pdev, smmu);
 	arm_smmu_device_reset(smmu);
 
diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index 64325d8..778eccc 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -642,8 +642,6 @@ static int __init exynos_sysmmu_probe(struct platform_device *pdev)
 
 	pm_runtime_enable(dev);
 
-	of_iommu_set_ops(dev->of_node, &exynos_iommu_ops);
-
 	return 0;
 }
 
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 21061da..a7e14b4 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1647,43 +1647,18 @@ int iommu_request_dm_for_dev(struct device *dev)
 	return ret;
 }
 
-struct iommu_instance {
-	struct list_head list;
-	struct fwnode_handle *fwnode;
-	const struct iommu_ops *ops;
-};
-static LIST_HEAD(iommu_instance_list);
-static DEFINE_SPINLOCK(iommu_instance_lock);
-
-void iommu_register_instance(struct fwnode_handle *fwnode,
-			     const struct iommu_ops *ops)
-{
-	struct iommu_instance *iommu = kzalloc(sizeof(*iommu), GFP_KERNEL);
-
-	if (WARN_ON(!iommu))
-		return;
-
-	of_node_get(to_of_node(fwnode));
-	INIT_LIST_HEAD(&iommu->list);
-	iommu->fwnode = fwnode;
-	iommu->ops = ops;
-	spin_lock(&iommu_instance_lock);
-	list_add_tail(&iommu->list, &iommu_instance_list);
-	spin_unlock(&iommu_instance_lock);
-}
-
 const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode)
 {
-	struct iommu_instance *instance;
 	const struct iommu_ops *ops = NULL;
+	struct iommu_device *iommu;
 
-	spin_lock(&iommu_instance_lock);
-	list_for_each_entry(instance, &iommu_instance_list, list)
-		if (instance->fwnode == fwnode) {
-			ops = instance->ops;
+	spin_lock(&iommu_device_lock);
+	list_for_each_entry(iommu, &iommu_device_list, list)
+		if (iommu->fwnode == fwnode) {
+			ops = iommu->ops;
 			break;
 		}
-	spin_unlock(&iommu_instance_lock);
+	spin_unlock(&iommu_device_lock);
 	return ops;
 }
 
diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c
index 30795cb..d044835 100644
--- a/drivers/iommu/msm_iommu.c
+++ b/drivers/iommu/msm_iommu.c
@@ -810,8 +810,6 @@ static int msm_iommu_probe(struct platform_device *pdev)
 		goto fail;
 	}
 
-	of_iommu_set_ops(pdev->dev.of_node, &msm_iommu_ops);
-
 	pr_info("device mapped at %p, irq %d with %d ctx banks\n",
 		iommu->base, iommu->irq, iommu->ncb);
 
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index d484fa6..5d14cd1 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -681,7 +681,6 @@ static int mtk_iommu_init_fn(struct device_node *np)
 		return ret;
 	}
 
-	of_iommu_set_ops(np, &mtk_iommu_ops);
 	return 0;
 }
 
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 626c935..9e82fc8 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -382,8 +382,6 @@ int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
 		      const struct iommu_ops *ops);
 void iommu_fwspec_free(struct device *dev);
 int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids);
-void iommu_register_instance(struct fwnode_handle *fwnode,
-			     const struct iommu_ops *ops);
 const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode);
 
 #else /* CONFIG_IOMMU_API */
@@ -634,11 +632,6 @@ static inline int iommu_fwspec_add_ids(struct device *dev, u32 *ids,
 	return -ENODEV;
 }
 
-static inline void iommu_register_instance(struct fwnode_handle *fwnode,
-					   const struct iommu_ops *ops)
-{
-}
-
 static inline
 const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode)
 {
diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
index 66fcbc9..fc4add3 100644
--- a/include/linux/of_iommu.h
+++ b/include/linux/of_iommu.h
@@ -31,12 +31,6 @@ static inline const struct iommu_ops *of_iommu_configure(struct device *dev,
 
 #endif	/* CONFIG_OF_IOMMU */
 
-static inline void of_iommu_set_ops(struct device_node *np,
-				    const struct iommu_ops *ops)
-{
-	iommu_register_instance(&np->fwnode, ops);
-}
-
 static inline const struct iommu_ops *of_iommu_get_ops(struct device_node *np)
 {
 	return iommu_ops_from_fwnode(&np->fwnode);
-- 
1.9.1

Re: [PATCH 03/11] iommu: Introduce new 'struct iommu_device'

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 5463 bytes --]

Hi Joerg,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.10-rc7]
[cannot apply to iommu/next next-20170209]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Joerg-Roedel/Let-IOMMU-core-know-about-individual-IOMMUs/20170209-233505
config: x86_64-randconfig-s0-02100256 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

Note: the linux-review/Joerg-Roedel/Let-IOMMU-core-know-about-individual-IOMMUs/20170209-233505 HEAD f657360e06e4d71eaae41b18f425745d6b737d22 builds fine.
      It only hurts bisectibility.

All error/warnings (new ones prefixed by >>):

   drivers/iommu/dmar.c: In function 'alloc_iommu':
>> drivers/iommu/dmar.c:1083:22: error: implicit declaration of function 'iommu_device_create' [-Werror=implicit-function-declaration]
      iommu->iommu_dev = iommu_device_create(NULL, iommu,
                         ^~~~~~~~~~~~~~~~~~~
>> drivers/iommu/dmar.c:1083:20: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
      iommu->iommu_dev = iommu_device_create(NULL, iommu,
                       ^
   drivers/iommu/dmar.c: In function 'free_iommu':
>> drivers/iommu/dmar.c:1114:2: error: implicit declaration of function 'iommu_device_destroy' [-Werror=implicit-function-declaration]
     iommu_device_destroy(iommu->iommu_dev);
     ^~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/iommu_device_create +1083 drivers/iommu/dmar.c

3a93c841 drivers/iommu/dmar.c Takao Indoh     2013-04-23  1077  	if (sts & DMA_GSTS_QIES)
3a93c841 drivers/iommu/dmar.c Takao Indoh     2013-04-23  1078  		iommu->gcmd |= DMA_GCMD_QIE;
3a93c841 drivers/iommu/dmar.c Takao Indoh     2013-04-23  1079  
1f5b3c3f drivers/iommu/dmar.c Thomas Gleixner 2011-07-19  1080  	raw_spin_lock_init(&iommu->register_lock);
e61d98d8 drivers/pci/dmar.c   Suresh Siddha   2008-07-10  1081  
bc847454 drivers/iommu/dmar.c Joerg Roedel    2016-01-07  1082  	if (intel_iommu_enabled) {
a5459cfe drivers/iommu/dmar.c Alex Williamson 2014-06-12 @1083  		iommu->iommu_dev = iommu_device_create(NULL, iommu,
a5459cfe drivers/iommu/dmar.c Alex Williamson 2014-06-12  1084  						       intel_iommu_groups,
2439d4aa drivers/iommu/dmar.c Kees Cook       2015-07-24  1085  						       "%s", iommu->name);
a5459cfe drivers/iommu/dmar.c Alex Williamson 2014-06-12  1086  
59203379 drivers/iommu/dmar.c Nicholas Krause 2016-01-04  1087  		if (IS_ERR(iommu->iommu_dev)) {
59203379 drivers/iommu/dmar.c Nicholas Krause 2016-01-04  1088  			err = PTR_ERR(iommu->iommu_dev);
59203379 drivers/iommu/dmar.c Nicholas Krause 2016-01-04  1089  			goto err_unmap;
59203379 drivers/iommu/dmar.c Nicholas Krause 2016-01-04  1090  		}
0d1395f4 drivers/iommu/dmar.c Joerg Roedel    2017-02-09  1091  
0d1395f4 drivers/iommu/dmar.c Joerg Roedel    2017-02-09  1092  		iommu_device_set_ops(&iommu->iommu, &intel_iommu_ops);
0d1395f4 drivers/iommu/dmar.c Joerg Roedel    2017-02-09  1093  
0d1395f4 drivers/iommu/dmar.c Joerg Roedel    2017-02-09  1094  		err = iommu_device_register(&iommu->iommu);
0d1395f4 drivers/iommu/dmar.c Joerg Roedel    2017-02-09  1095  		if (err)
0d1395f4 drivers/iommu/dmar.c Joerg Roedel    2017-02-09  1096  			goto err_unmap;
bc847454 drivers/iommu/dmar.c Joerg Roedel    2016-01-07  1097  	}
bc847454 drivers/iommu/dmar.c Joerg Roedel    2016-01-07  1098  
bc847454 drivers/iommu/dmar.c Joerg Roedel    2016-01-07  1099  	drhd->iommu = iommu;
59203379 drivers/iommu/dmar.c Nicholas Krause 2016-01-04  1100  
1886e8a9 drivers/pci/dmar.c   Suresh Siddha   2008-07-10  1101  	return 0;
0815565a drivers/pci/dmar.c   David Woodhouse 2009-08-04  1102  
0815565a drivers/pci/dmar.c   David Woodhouse 2009-08-04  1103  err_unmap:
6f5cf521 drivers/iommu/dmar.c Donald Dutile   2012-06-04  1104  	unmap_iommu(iommu);
78d8e704 drivers/iommu/dmar.c Jiang Liu       2014-11-09  1105  error_free_seq_id:
78d8e704 drivers/iommu/dmar.c Jiang Liu       2014-11-09  1106  	dmar_free_seq_id(iommu);
e61d98d8 drivers/pci/dmar.c   Suresh Siddha   2008-07-10  1107  error:
e61d98d8 drivers/pci/dmar.c   Suresh Siddha   2008-07-10  1108  	kfree(iommu);
6f5cf521 drivers/iommu/dmar.c Donald Dutile   2012-06-04  1109  	return err;
e61d98d8 drivers/pci/dmar.c   Suresh Siddha   2008-07-10  1110  }
e61d98d8 drivers/pci/dmar.c   Suresh Siddha   2008-07-10  1111  
a868e6b7 drivers/iommu/dmar.c Jiang Liu       2014-01-06  1112  static void free_iommu(struct intel_iommu *iommu)
e61d98d8 drivers/pci/dmar.c   Suresh Siddha   2008-07-10  1113  {
a5459cfe drivers/iommu/dmar.c Alex Williamson 2014-06-12 @1114  	iommu_device_destroy(iommu->iommu_dev);
0d1395f4 drivers/iommu/dmar.c Joerg Roedel    2017-02-09  1115  	iommu_device_unregister(&iommu->iommu);
a5459cfe drivers/iommu/dmar.c Alex Williamson 2014-06-12  1116  
a868e6b7 drivers/iommu/dmar.c Jiang Liu       2014-01-06  1117  	if (iommu->irq) {

:::::: The code at line 1083 was first introduced by commit
:::::: a5459cfece880e82778a60e6290ad6c0dd688a06 iommu/vt-d: Make use of IOMMU sysfs support

:::::: TO: Alex Williamson <alex.williamson@redhat.com>
:::::: CC: Joerg Roedel <jroedel@suse.de>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 23560 bytes --]

Re: [PATCH 10/11] iommu/exynos: Make use of iommu_device_register interface

[Marek Szyprowski]

Hi

On 2017-02-09 12:32, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
>
> Register Exynos IOMMUs to the IOMMU core and make them
> visible in sysfs. This patch does not add the links between
> IOMMUs and translated devices yet.
>
> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-samsung-soc@vger.kernel.org
> Signed-off-by: Joerg Roedel <jroedel@suse.de>

Acked-by: Marek Szyprowski <m.szyprowski@samsung.com>
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

> ---
>   drivers/iommu/exynos-iommu.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
>
> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> index 57ba0d3..64325d8 100644
> --- a/drivers/iommu/exynos-iommu.c
> +++ b/drivers/iommu/exynos-iommu.c
> @@ -276,6 +276,8 @@ struct sysmmu_drvdata {
>   	struct list_head owner_node;	/* node for owner controllers list */
>   	phys_addr_t pgtable;		/* assigned page table structure */
>   	unsigned int version;		/* our version */
> +
> +	struct iommu_device iommu;	/* IOMMU core handle */
>   };
>   
>   static struct exynos_iommu_domain *to_exynos_domain(struct iommu_domain *dom)
> @@ -611,6 +613,18 @@ static int __init exynos_sysmmu_probe(struct platform_device *pdev)
>   	data->sysmmu = dev;
>   	spin_lock_init(&data->lock);
>   
> +	ret = iommu_device_sysfs_add(&data->iommu, &pdev->dev, NULL,
> +				     dev_name(data->sysmmu));
> +	if (ret)
> +		return ret;
> +
> +	iommu_device_set_ops(&data->iommu, &exynos_iommu_ops);
> +	iommu_device_set_fwnode(&data->iommu, &dev->of_node->fwnode);
> +
> +	ret = iommu_device_register(&data->iommu);
> +	if (ret)
> +		return ret;
> +
>   	platform_set_drvdata(pdev, data);
>   
>   	__sysmmu_get_version(data);

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 10/11] iommu/exynos: Make use of iommu_device_register interface

[Joerg Roedel]

On Fri, Feb 10, 2017 at 02:46:59PM +0100, Marek Szyprowski wrote:
> Hi
> 
> On 2017-02-09 12:32, Joerg Roedel wrote:
> >From: Joerg Roedel <jroedel@suse.de>
> >
> >Register Exynos IOMMUs to the IOMMU core and make them
> >visible in sysfs. This patch does not add the links between
> >IOMMUs and translated devices yet.
> >
> >Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> >Cc: linux-arm-kernel@lists.infradead.org
> >Cc: linux-samsung-soc@vger.kernel.org
> >Signed-off-by: Joerg Roedel <jroedel@suse.de>
> 
> Acked-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

Thanks a lot for testing!

Re: [PATCH 01/11] iommu: Rename iommu_get_instance()

[Robin Murphy]

Hi Joerg,

I'm really liking this series! Superficially it doesn't seem to break
anything on my Juno, but I'll give it a more thorough workout soon.

Just a few comments from skimming through...

On 09/02/17 11:32, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
> 
> Rename the function to iommu_ops_from_fwnode(), because that
> is what the function actually does. The new name is much
> more descriptive about what the function does.
> 
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>  drivers/acpi/arm64/iort.c | 2 +-
>  drivers/iommu/iommu.c     | 2 +-
>  include/linux/iommu.h     | 4 ++--
>  include/linux/of_iommu.h  | 2 +-
>  4 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index e0d2e6e..3752521 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -536,7 +536,7 @@ static const struct iommu_ops *iort_iommu_xlate(struct device *dev,
>  		if (!iort_fwnode)
>  			return NULL;
>  
> -		ops = iommu_get_instance(iort_fwnode);
> +		ops = iommu_ops_from_fwnode(iort_fwnode);
>  		if (!ops)
>  			return NULL;
>  
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index dbe7f65..0ee05bb 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1653,7 +1653,7 @@ void iommu_register_instance(struct fwnode_handle *fwnode,
>  	spin_unlock(&iommu_instance_lock);
>  }
>  
> -const struct iommu_ops *iommu_get_instance(struct fwnode_handle *fwnode)
> +const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode)
>  {
>  	struct iommu_instance *instance;
>  	const struct iommu_ops *ops = NULL;
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 0ff5111..085e1f0 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -354,7 +354,7 @@ int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
>  int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids);
>  void iommu_register_instance(struct fwnode_handle *fwnode,
>  			     const struct iommu_ops *ops);
> -const struct iommu_ops *iommu_get_instance(struct fwnode_handle *fwnode);
> +const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode);
>  
>  #else /* CONFIG_IOMMU_API */
>  
> @@ -590,7 +590,7 @@ static inline void iommu_register_instance(struct fwnode_handle *fwnode,
>  }
>  
>  static inline
> -const struct iommu_ops *iommu_get_instance(struct fwnode_handle *fwnode)
> +const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode)
>  {
>  	return NULL;
>  }
> diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
> index 6a7fc50..66fcbc9 100644
> --- a/include/linux/of_iommu.h
> +++ b/include/linux/of_iommu.h
> @@ -39,7 +39,7 @@ static inline void of_iommu_set_ops(struct device_node *np,
>  
>  static inline const struct iommu_ops *of_iommu_get_ops(struct device_node *np)
>  {
> -	return iommu_get_instance(&np->fwnode);
> +	return iommu_ops_from_fwnode(&np->fwnode);
>  }

Note that you've already got Lorenzo's patch queued to remove these of_
wrappers.

Robin.

>  
>  extern struct of_device_id __iommu_of_table;
> 

Re: [PATCH 06/11] iommu: Add iommu_device_set_fwnode() interface

[Robin Murphy]

On 09/02/17 11:32, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
> 
> Allow to store a fwnode in 'struct iommu_device';
> 
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>  include/linux/iommu.h | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index bae3cfc..626c935 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -214,6 +214,7 @@ struct iommu_ops {
>  struct iommu_device {
>  	struct list_head list;
>  	const struct iommu_ops *ops;
> +	struct fwnode_handle *fwnode;
>  	struct device dev;
>  };
>  
> @@ -233,6 +234,12 @@ static inline void iommu_device_set_ops(struct iommu_device *iommu,
>  	iommu->ops = ops;
>  }
>  
> +static inline void iommu_device_set_fwnode(struct iommu_device *iommu,
> +					   struct fwnode_handle *fwnode)
> +{
> +	iommu->fwnode = fwnode;
> +}

Would it make sense to simply make the ops and fwnode additional
arguments to iommu_device_register() (permitting fwnode to be NULL)?
AFAICS they should typically all have the same effective lifetime so
there doesn't seem to be any real need to handle everything separately.

Robin.

> +
>  #define IOMMU_GROUP_NOTIFY_ADD_DEVICE		1 /* Device added */
>  #define IOMMU_GROUP_NOTIFY_DEL_DEVICE		2 /* Pre Device removed */
>  #define IOMMU_GROUP_NOTIFY_BIND_DRIVER		3 /* Pre Driver bind */
> @@ -580,6 +587,11 @@ static inline void iommu_device_set_ops(struct iommu_device *iommu,
>  {
>  }
>  
> +static inline void iommu_device_set_fwnode(struct iommu_device *iommu,
> +					   struct fwnode_handle *fwnode)
> +{
> +}
> +
>  static inline void iommu_device_unregister(struct iommu_device *iommu)
>  {
>  }
> 

Re: [PATCH 07/11] iommu/arm-smmu: Make use of the iommu_register interface

[Robin Murphy]

On 09/02/17 11:32, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
> 
> Also add the smmu devices to sysfs.
> 
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>  drivers/iommu/arm-smmu-v3.c | 22 +++++++++++++++++++++-
>  drivers/iommu/arm-smmu.c    | 30 ++++++++++++++++++++++++++++++
>  2 files changed, 51 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index 4d6ec44..32133e2 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -616,6 +616,9 @@ struct arm_smmu_device {
>  	unsigned int			sid_bits;
>  
>  	struct arm_smmu_strtab_cfg	strtab_cfg;
> +
> +	/* IOMMU core code handle */
> +	struct iommu_device		iommu;
>  };
>  
>  /* SMMU private data for each master */
> @@ -1795,8 +1798,10 @@ static int arm_smmu_add_device(struct device *dev)
>  	}
>  
>  	group = iommu_group_get_for_dev(dev);
> -	if (!IS_ERR(group))
> +	if (!IS_ERR(group)) {
>  		iommu_group_put(group);
> +		iommu_device_link(&smmu->iommu, dev);

Given the coupling evident from this and the other patches, might it
work to simply do the linking/unlinking automatically in
iommu_group_{add,remove}_device()?

Robin.

> +	}
>  
>  	return PTR_ERR_OR_ZERO(group);
>  }
> @@ -1805,14 +1810,17 @@ static void arm_smmu_remove_device(struct device *dev)
>  {
>  	struct iommu_fwspec *fwspec = dev->iommu_fwspec;
>  	struct arm_smmu_master_data *master;
> +	struct arm_smmu_device *smmu;
>  
>  	if (!fwspec || fwspec->ops != &arm_smmu_ops)
>  		return;
>  
>  	master = fwspec->iommu_priv;
> +	smmu = master->smmu;
>  	if (master && master->ste.valid)
>  		arm_smmu_detach_dev(dev);
>  	iommu_group_remove_device(dev);
> +	iommu_device_unlink(&smmu->iommu, dev);
>  	kfree(master);
>  	iommu_fwspec_free(dev);
>  }
> @@ -2613,6 +2621,7 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>  {
>  	int irq, ret;
>  	struct resource *res;
> +	resource_size_t ioaddr;
>  	struct arm_smmu_device *smmu;
>  	struct device *dev = &pdev->dev;
>  	bool bypass;
> @@ -2630,6 +2639,7 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>  		dev_err(dev, "MMIO region too small (%pr)\n", res);
>  		return -EINVAL;
>  	}
> +	ioaddr = res->start;
>  
>  	smmu->base = devm_ioremap_resource(dev, res);
>  	if (IS_ERR(smmu->base))
> @@ -2682,6 +2692,16 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>  		return ret;
>  
>  	/* And we're up. Go go go! */
> +	ret = iommu_device_sysfs_add(&smmu->iommu, dev, NULL,
> +				     "smmu3.%pa", &ioaddr);
> +	if (ret)
> +		return ret;
> +
> +	iommu_device_set_ops(&smmu->iommu, &arm_smmu_ops);
> +	iommu_device_set_fwnode(&smmu->iommu, dev->fwnode);
> +
> +	ret = iommu_device_register(&smmu->iommu);
> +
>  	iommu_register_instance(dev->fwnode, &arm_smmu_ops);
>  
>  #ifdef CONFIG_PCI
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index a60cded..f4ce1e7 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -380,6 +380,9 @@ struct arm_smmu_device {
>  	unsigned int			*irqs;
>  
>  	u32				cavium_id_base; /* Specific to Cavium */
> +
> +	/* IOMMU core code handle */
> +	struct iommu_device		iommu;
>  };
>  
>  enum arm_smmu_context_fmt {
> @@ -1444,6 +1447,8 @@ static int arm_smmu_add_device(struct device *dev)
>  	if (ret)
>  		goto out_free;
>  
> +	iommu_device_link(&smmu->iommu, dev);
> +
>  	return 0;
>  
>  out_free:
> @@ -1456,10 +1461,17 @@ static int arm_smmu_add_device(struct device *dev)
>  static void arm_smmu_remove_device(struct device *dev)
>  {
>  	struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> +	struct arm_smmu_master_cfg *cfg;
> +	struct arm_smmu_device *smmu;
> +
>  
>  	if (!fwspec || fwspec->ops != &arm_smmu_ops)
>  		return;
>  
> +	cfg  = fwspec->iommu_priv;
> +	smmu = cfg->smmu;
> +
> +	iommu_device_unlink(&smmu->iommu, dev);
>  	arm_smmu_master_free_smes(fwspec);
>  	iommu_group_remove_device(dev);
>  	kfree(fwspec->iommu_priv);
> @@ -2011,6 +2023,7 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev,
>  static int arm_smmu_device_probe(struct platform_device *pdev)
>  {
>  	struct resource *res;
> +	resource_size_t ioaddr;
>  	struct arm_smmu_device *smmu;
>  	struct device *dev = &pdev->dev;
>  	int num_irqs, i, err;
> @@ -2031,6 +2044,7 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>  		return err;
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	ioaddr = res->start;
>  	smmu->base = devm_ioremap_resource(dev, res);
>  	if (IS_ERR(smmu->base))
>  		return PTR_ERR(smmu->base);
> @@ -2091,6 +2105,22 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	err = iommu_device_sysfs_add(&smmu->iommu, smmu->dev, NULL,
> +				     "smmu.%pa", &ioaddr);
> +	if (err) {
> +		dev_err(dev, "Failed to register iommu in sysfs\n");
> +		return err;
> +	}
> +
> +	iommu_device_set_ops(&smmu->iommu, &arm_smmu_ops);
> +	iommu_device_set_fwnode(&smmu->iommu, dev->fwnode);
> +
> +	err = iommu_device_register(&smmu->iommu);
> +	if (err) {
> +		dev_err(dev, "Failed to register iommu\n");
> +		return err;
> +	}
> +
>  	iommu_register_instance(dev->fwnode, &arm_smmu_ops);
>  	platform_set_drvdata(pdev, smmu);
>  	arm_smmu_device_reset(smmu);
> 

Re: [PATCH 08/11] iommu/msm: Make use of iommu_device_register interface

[Robin Murphy]

On 09/02/17 11:32, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
> 
> Register the MSM IOMMUs to the iommu core and add sysfs
> entries for that driver.
> 
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>  drivers/iommu/msm_iommu.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/iommu/msm_iommu.h |  3 ++
>  2 files changed, 76 insertions(+)
> 
> diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c
> index b09692b..30795cb 100644
> --- a/drivers/iommu/msm_iommu.c
> +++ b/drivers/iommu/msm_iommu.c
> @@ -371,6 +371,58 @@ static int msm_iommu_domain_config(struct msm_priv *priv)
>  	return 0;
>  }
>  
> +/* Must be called under msm_iommu_lock */
> +static struct msm_iommu_dev *find_iommu_for_dev(struct device *dev)
> +{
> +	struct msm_iommu_dev *iommu, *ret = NULL;
> +	struct msm_iommu_ctx_dev *master;
> +
> +	list_for_each_entry(iommu, &qcom_iommu_devices, dev_node) {
> +		master = list_first_entry(&iommu->ctx_list,
> +					  struct msm_iommu_ctx_dev,
> +					  list);
> +		if (master->of_node == dev->of_node) {
> +			ret = iommu;
> +			break;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +static int msm_iommu_add_device(struct device *dev)
> +{
> +	struct msm_iommu_dev *iommu;
> +	unsigned long flags;
> +	int ret = 0;
> +
> +	spin_lock_irqsave(&msm_iommu_lock, flags);
> +
> +	iommu = find_iommu_for_dev(dev);
> +	if (iommu)
> +		iommu_device_link(&iommu->iommu, dev);
> +	else
> +		ret = -ENODEV;
> +
> +	spin_unlock_irqrestore(&msm_iommu_lock, flags);
> +
> +	return ret;
> +}
> +
> +static void msm_iommu_remove_device(struct device *dev)
> +{
> +	struct msm_iommu_dev *iommu;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&msm_iommu_lock, flags);
> +
> +	iommu = find_iommu_for_dev(dev);
> +	if (iommu)
> +		iommu_device_unlink(&iommu->iommu, dev);
> +
> +	spin_unlock_irqrestore(&msm_iommu_lock, flags);
> +}
> +
>  static int msm_iommu_attach_dev(struct iommu_domain *domain, struct device *dev)
>  {
>  	int ret = 0;
> @@ -646,6 +698,8 @@ irqreturn_t msm_iommu_fault_handler(int irq, void *dev_id)
>  	.unmap = msm_iommu_unmap,
>  	.map_sg = default_iommu_map_sg,
>  	.iova_to_phys = msm_iommu_iova_to_phys,
> +	.add_device = msm_iommu_add_device,
> +	.remove_device = msm_iommu_remove_device,
>  	.pgsize_bitmap = MSM_IOMMU_PGSIZES,
>  	.of_xlate = qcom_iommu_of_xlate,
>  };
> @@ -653,6 +707,7 @@ irqreturn_t msm_iommu_fault_handler(int irq, void *dev_id)
>  static int msm_iommu_probe(struct platform_device *pdev)
>  {
>  	struct resource *r;
> +	resource_size_t ioaddr;
>  	struct msm_iommu_dev *iommu;
>  	int ret, par, val;
>  
> @@ -696,6 +751,7 @@ static int msm_iommu_probe(struct platform_device *pdev)
>  		ret = PTR_ERR(iommu->base);
>  		goto fail;
>  	}
> +	ioaddr = r->start;
>  
>  	iommu->irq = platform_get_irq(pdev, 0);
>  	if (iommu->irq < 0) {
> @@ -737,6 +793,23 @@ static int msm_iommu_probe(struct platform_device *pdev)
>  	}
>  
>  	list_add(&iommu->dev_node, &qcom_iommu_devices);
> +
> +	ret = iommu_device_sysfs_add(&iommu->iommu, iommu->dev, NULL,
> +				     "msm-smmu.%pa", &ioaddr);
> +	if (ret) {
> +		pr_err("Could not add msm-smmu at %pa to sysfs\n", &ioaddr);
> +		goto fail;
> +	}

Nit: there's a bit of inconsistency with printing errors between the
various drivers (for both _sysfs_add and _register). I reckon if we want
error messages we may as well just fold them into the helper functions.

> +
> +	iommu_device_set_ops(&iommu->iommu, &msm_iommu_ops);
> +	iommu_device_set_fwnode(&iommu->iommu, &pdev->dev.of_node->fwnode);
> +
> +	ret = iommu_device_register(&iommu->iommu);
> +	if (ret) {
> +		pr_err("Could not register msm-smmu at %pa\n", &ioaddr);
> +		goto fail;
> +	}

I think there's a corresponding unregister missing for
msm_iommu_remove() here (and similarly in the ARM SMMU drivers, looking
back). I know it's not strictly a problem at the moment, but I do now
have IOMMU-drivers-as-modules working on top of the probe deferral
series... ;)

Robin.

> +
>  	of_iommu_set_ops(pdev->dev.of_node, &msm_iommu_ops);
>  
>  	pr_info("device mapped at %p, irq %d with %d ctx banks\n",
> diff --git a/drivers/iommu/msm_iommu.h b/drivers/iommu/msm_iommu.h
> index 4ca25d5..ae92d27 100644
> --- a/drivers/iommu/msm_iommu.h
> +++ b/drivers/iommu/msm_iommu.h
> @@ -19,6 +19,7 @@
>  #define MSM_IOMMU_H
>  
>  #include <linux/interrupt.h>
> +#include <linux/iommu.h>
>  #include <linux/clk.h>
>  
>  /* Sharability attributes of MSM IOMMU mappings */
> @@ -68,6 +69,8 @@ struct msm_iommu_dev {
>  	struct list_head dom_node;
>  	struct list_head ctx_list;
>  	DECLARE_BITMAP(context_map, IOMMU_MAX_CBS);
> +
> +	struct iommu_device iommu;
>  };
>  
>  /**
> 

Re: [PATCH 06/11] iommu: Add iommu_device_set_fwnode() interface

[Joerg Roedel]

Hi Robin,

On Fri, Feb 10, 2017 at 02:16:54PM +0000, Robin Murphy wrote:
> > +static inline void iommu_device_set_fwnode(struct iommu_device *iommu,
> > +					   struct fwnode_handle *fwnode)
> > +{
> > +	iommu->fwnode = fwnode;
> > +}
> 
> Would it make sense to simply make the ops and fwnode additional
> arguments to iommu_device_register() (permitting fwnode to be NULL)?
> AFAICS they should typically all have the same effective lifetime so
> there doesn't seem to be any real need to handle everything separately.

Well, it is not yet clear what other information will end up in
'struct iommu_device', and I don't want to add another parameter to
iommu_device_register for every new struct member.

Also I think having these wrappers is more readable in the code, as it
is clear what the code does without looking up the function prototypes
in the header.

It might make sense to set the mandatory struct members via
iommu_device_register in the future, but we'll see :)


	Joerg

Re: [PATCH 07/11] iommu/arm-smmu: Make use of the iommu_register interface

[Joerg Roedel]

On Fri, Feb 10, 2017 at 02:20:34PM +0000, Robin Murphy wrote:
> > @@ -1795,8 +1798,10 @@ static int arm_smmu_add_device(struct device *dev)
> >  	}
> >  
> >  	group = iommu_group_get_for_dev(dev);
> > -	if (!IS_ERR(group))
> > +	if (!IS_ERR(group)) {
> >  		iommu_group_put(group);
> > +		iommu_device_link(&smmu->iommu, dev);
> 
> Given the coupling evident from this and the other patches, might it
> work to simply do the linking/unlinking automatically in
> iommu_group_{add,remove}_device()?

Yes, this is one of the goals too. But currently we don't have a generic
device->hw_iommu mapping in the iommu-code which would allow to call
the link/unlink functions in generic code too.

But changing this is one of the next things on my list :)



	Joerg

Re: [PATCH 08/11] iommu/msm: Make use of iommu_device_register interface

[Joerg Roedel]

On Fri, Feb 10, 2017 at 02:35:39PM +0000, Robin Murphy wrote:
> On 09/02/17 11:32, Joerg Roedel wrote:
> > +	ret = iommu_device_sysfs_add(&iommu->iommu, iommu->dev, NULL,
> > +				     "msm-smmu.%pa", &ioaddr);
> > +	if (ret) {
> > +		pr_err("Could not add msm-smmu at %pa to sysfs\n", &ioaddr);
> > +		goto fail;
> > +	}
> 
> Nit: there's a bit of inconsistency with printing errors between the
> various drivers (for both _sysfs_add and _register). I reckon if we want
> error messages we may as well just fold them into the helper functions.

Yeah, this could be unified too. For now I looked how verbose the
driver was that I was going to change and added messages to be
consistent inside the drivers.

> 
> > +
> > +	iommu_device_set_ops(&iommu->iommu, &msm_iommu_ops);
> > +	iommu_device_set_fwnode(&iommu->iommu, &pdev->dev.of_node->fwnode);
> > +
> > +	ret = iommu_device_register(&iommu->iommu);
> > +	if (ret) {
> > +		pr_err("Could not register msm-smmu at %pa\n", &ioaddr);
> > +		goto fail;
> > +	}
> 
> I think there's a corresponding unregister missing for
> msm_iommu_remove() here (and similarly in the ARM SMMU drivers, looking
> back). I know it's not strictly a problem at the moment, but I do now
> have IOMMU-drivers-as-modules working on top of the probe deferral
> series... ;)

Well, that there was an iommu_register_instance() without any
unregistration interface at all makes me believe that unregistering
iommus is not really implemented yet.

And in fact, the remove functions for msm and arm-smmu seem to only
disable the hardware, but are not removing the corresponding data
structures.

So I think we are fine from that side.


	Joerg

Re: [PATCH 01/11] iommu: Rename iommu_get_instance()

[Joerg Roedel]

Hi Robin,

On Fri, Feb 10, 2017 at 02:12:54PM +0000, Robin Murphy wrote:
> I'm really liking this series! Superficially it doesn't seem to break
> anything on my Juno, but I'll give it a more thorough workout soon.

Great, thanks for testing! Any problems in this series should show up on
boot anyway.

> >  static inline const struct iommu_ops *of_iommu_get_ops(struct device_node *np)
> >  {
> > -	return iommu_get_instance(&np->fwnode);
> > +	return iommu_ops_from_fwnode(&np->fwnode);
> >  }
> 
> Note that you've already got Lorenzo's patch queued to remove these of_
> wrappers.

Yes, that was among the conflicts I resolved when merging this with the
rest of the iommu-tree.


	Joerg

Re: [PATCH 06/11] iommu: Add iommu_device_set_fwnode() interface

[Robin Murphy]

On 10/02/17 15:22, Joerg Roedel wrote:
> Hi Robin,
> 
> On Fri, Feb 10, 2017 at 02:16:54PM +0000, Robin Murphy wrote:
>>> +static inline void iommu_device_set_fwnode(struct iommu_device *iommu,
>>> +					   struct fwnode_handle *fwnode)
>>> +{
>>> +	iommu->fwnode = fwnode;
>>> +}
>>
>> Would it make sense to simply make the ops and fwnode additional
>> arguments to iommu_device_register() (permitting fwnode to be NULL)?
>> AFAICS they should typically all have the same effective lifetime so
>> there doesn't seem to be any real need to handle everything separately.
> 
> Well, it is not yet clear what other information will end up in
> 'struct iommu_device', and I don't want to add another parameter to
> iommu_device_register for every new struct member.

That's a fair point. I think the ops, as a core piece of the whole API,
would be sufficiently self-explanatory as part of registration, but then
we'd end up with a weird interface with different members initialised
through different paths, and I agree that ends up just as ugly.

> Also I think having these wrappers is more readable in the code, as it
> is clear what the code does without looking up the function prototypes
> in the header.

Yeah, on reflection explicit initialisation is certainly easier to read
than a bunch of arguments handled implicitly by register(), but then
from that angle, even more clear would be to simply have the drivers
write the relevant struct members directly - I'd be quite happy with
that, and we then don't have to add another setter to iommu.h for every
new struct member (and risk it looking like Java code...)

Robin.

> 
> It might make sense to set the mandatory struct members via
> iommu_device_register in the future, but we'll see :)
> 
> 
> 	Joerg
> 

Re: [PATCH 06/11] iommu: Add iommu_device_set_fwnode() interface

[Joerg Roedel]

On Fri, Feb 10, 2017 at 04:03:07PM +0000, Robin Murphy wrote:
> Yeah, on reflection explicit initialisation is certainly easier to read
> than a bunch of arguments handled implicitly by register(), but then
> from that angle, even more clear would be to simply have the drivers
> write the relevant struct members directly - I'd be quite happy with
> that, and we then don't have to add another setter to iommu.h for every
> new struct member (and risk it looking like Java code...)

Yeah, that was my first approach. But there is the Intel VT-d anomaly,
where a part of the driver can be built-in (dmar.c) with
CONFIG_IOMMU_API=N. In this case 'struct iommu_device' is empty, and
trying to access the members directly doesn't compile anymore.

I have to look if this anomaly could be removed, then it is probably the
best to set the struct members directly without wrapper functions.


	Joerg

Re: [PATCH 06/11] iommu: Add iommu_device_set_fwnode() interface

[Robin Murphy]

On 10/02/17 16:11, Joerg Roedel wrote:
> On Fri, Feb 10, 2017 at 04:03:07PM +0000, Robin Murphy wrote:
>> Yeah, on reflection explicit initialisation is certainly easier to read
>> than a bunch of arguments handled implicitly by register(), but then
>> from that angle, even more clear would be to simply have the drivers
>> write the relevant struct members directly - I'd be quite happy with
>> that, and we then don't have to add another setter to iommu.h for every
>> new struct member (and risk it looking like Java code...)
> 
> Yeah, that was my first approach. But there is the Intel VT-d anomaly,
> where a part of the driver can be built-in (dmar.c) with
> CONFIG_IOMMU_API=N. In this case 'struct iommu_device' is empty, and
> trying to access the members directly doesn't compile anymore.
> 
> I have to look if this anomaly could be removed, then it is probably the
> best to set the struct members directly without wrapper functions.

Ah, I hadn't managed to spot that - I assume there probably is some
valid edge case for wanting x2APIC functionality without DMA remapping
which prevents us from just adding the dependency. Looking at the code,
though, that situation does seem to rely on the call never actually
executing at runtime - not only is it conditional on a static variable
which is only ever set by non-present code, it would fail the probe if
it were called - so I think it would be perfectly reasonable to just
address that particular problem as below (untested, but if it lets us
get rid of the dummy !IOMMU_API definitions of the registration
functions I'd say we've done the right thing).

Robin.

----->8-----
diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
index 8ccbd7023194..161641caff79 100644
--- a/drivers/iommu/dmar.c
+++ b/drivers/iommu/dmar.c
@@ -1077,6 +1077,7 @@ static int alloc_iommu(struct dmar_drhd_unit *drhd)

        raw_spin_lock_init(&iommu->register_lock);

+#ifdef CONFIG_IOMMU_API
        if (intel_iommu_enabled) {
                iommu->iommu_dev = iommu_device_create(NULL, iommu,
                                                       intel_iommu_groups,
@@ -1087,6 +1088,7 @@ static int alloc_iommu(struct dmar_drhd_unit *drhd)
                        goto err_unmap;
                }
        }
+#endif

        drhd->iommu = iommu;

Re: [PATCH 07/11] iommu/arm-smmu: Make use of the iommu_register interface

[Robin Murphy]

On 10/02/17 15:25, Joerg Roedel wrote:
> On Fri, Feb 10, 2017 at 02:20:34PM +0000, Robin Murphy wrote:
>>> @@ -1795,8 +1798,10 @@ static int arm_smmu_add_device(struct device *dev)
>>>  	}
>>>  
>>>  	group = iommu_group_get_for_dev(dev);
>>> -	if (!IS_ERR(group))
>>> +	if (!IS_ERR(group)) {
>>>  		iommu_group_put(group);
>>> +		iommu_device_link(&smmu->iommu, dev);
>>
>> Given the coupling evident from this and the other patches, might it
>> work to simply do the linking/unlinking automatically in
>> iommu_group_{add,remove}_device()?
> 
> Yes, this is one of the goals too. But currently we don't have a generic
> device->hw_iommu mapping in the iommu-code which would allow to call
> the link/unlink functions in generic code too.

At some point we should change the iommu_ops pointer in iommu_fwspec for
an iommu_device pointer - that would then give us an easy
dev->fwpec->hw_iommu relationship which is mostly managed by core code
already. In the meantime I was imagining just passing it around,
something like iommu_group_add_device(hw_iommu, group, dev), but now I
suspect that'd be running up against a similar objection to before ;)

Robin.

> 
> But changing this is one of the next things on my list :)
> 
> 
> 
> 	Joerg
> 

Re: [PATCH 08/11] iommu/msm: Make use of iommu_device_register interface

[Robin Murphy]

On 10/02/17 15:33, Joerg Roedel wrote:
> On Fri, Feb 10, 2017 at 02:35:39PM +0000, Robin Murphy wrote:
>> On 09/02/17 11:32, Joerg Roedel wrote:
>>> +	ret = iommu_device_sysfs_add(&iommu->iommu, iommu->dev, NULL,
>>> +				     "msm-smmu.%pa", &ioaddr);
>>> +	if (ret) {
>>> +		pr_err("Could not add msm-smmu at %pa to sysfs\n", &ioaddr);
>>> +		goto fail;
>>> +	}
>>
>> Nit: there's a bit of inconsistency with printing errors between the
>> various drivers (for both _sysfs_add and _register). I reckon if we want
>> error messages we may as well just fold them into the helper functions.
> 
> Yeah, this could be unified too. For now I looked how verbose the
> driver was that I was going to change and added messages to be
> consistent inside the drivers.
> 
>>
>>> +
>>> +	iommu_device_set_ops(&iommu->iommu, &msm_iommu_ops);
>>> +	iommu_device_set_fwnode(&iommu->iommu, &pdev->dev.of_node->fwnode);
>>> +
>>> +	ret = iommu_device_register(&iommu->iommu);
>>> +	if (ret) {
>>> +		pr_err("Could not register msm-smmu at %pa\n", &ioaddr);
>>> +		goto fail;
>>> +	}
>>
>> I think there's a corresponding unregister missing for
>> msm_iommu_remove() here (and similarly in the ARM SMMU drivers, looking
>> back). I know it's not strictly a problem at the moment, but I do now
>> have IOMMU-drivers-as-modules working on top of the probe deferral
>> series... ;)
> 
> Well, that there was an iommu_register_instance() without any
> unregistration interface at all makes me believe that unregistering
> iommus is not really implemented yet.
> 
> And in fact, the remove functions for msm and arm-smmu seem to only
> disable the hardware, but are not removing the corresponding data
> structures.

For the ARM SMMUs at least, the SMMU-specific data is (well, should be)
all devm_* managed, thus freed automatically by the driver core after
remove() returns. It is true that there's an implicit expectation that
the SMMU won't be removed until all domains, groups and masters have
been explicitly torn down by the relevant detach()/remove()/free()
calls, although I guess the sysfs links might actually help enforce that.

> So I think we are fine from that side.

Sure, I mostly just wanted not to lose sight of the future possibility
of unloadable IOMMU drivers (admittedly I've not even tried that yet,
only loading them post-boot).

Robin.

> 
> 
> 	Joerg
>