Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1518190 > unrolled thread

[PATCH v7 04/16] drivers: iommu: make of_iommu_set/get_ops() DT agnostic

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v7 04/16] drivers: iommu: make of_iommu_set/get_ops() DT agnostic Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> - 2016-11-09 15:20 +0100
    Re: [PATCH v7 04/16] drivers: iommu: make of_iommu_set/get_ops() DT  agnostic Robin Murphy <robin.murphy@arm.com> - 2016-11-09 15:50 +0100
      Re: [PATCH v7 04/16] drivers: iommu: make of_iommu_set/get_ops() DT  agnostic Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> - 2016-11-10 12:20 +0100
    Re: [PATCH v7 04/16] drivers: iommu: make of_iommu_set/get_ops() DT  agnostic Joerg Roedel <joro@8bytes.org> - 2016-11-11 16:30 +0100
      Re: [PATCH v7 04/16] drivers: iommu: make of_iommu_set/get_ops() DT  agnostic Robin Murphy <robin.murphy@arm.com> - 2016-11-11 17:20 +0100
        Re: [PATCH v7 04/16] drivers: iommu: make of_iommu_set/get_ops() DT  agnostic Joerg Roedel <joro@8bytes.org> - 2016-11-11 17:30 +0100
          Re: [PATCH v7 04/16] drivers: iommu: make of_iommu_set/get_ops() DT  agnostic Robin Murphy <robin.murphy@arm.com> - 2016-11-11 18:50 +0100

#1518190 — [PATCH v7 04/16] drivers: iommu: make of_iommu_set/get_ops() DT agnostic

The of_iommu_{set/get}_ops() API is used to associate a device
tree node with a specific set of IOMMU operations. The same
kernel interface is required on systems booting with ACPI, where
devices are not associated with a device tree node, therefore
the interface requires generalization.

The struct device fwnode member represents the fwnode token
associated with the device and the struct it points at is firmware
specific; regardless, it is initialized on both ACPI and DT systems
and makes an ideal candidate to use it to associate a set of IOMMU
operations to a given device, through its struct device.fwnode member
pointer.

Convert the DT specific of_iommu_{set/get}_ops() interface to
use struct device.fwnode as a look-up token, making the interface
usable on ACPI systems.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Tested-by: Hanjun Guo <hanjun.guo@linaro.org>
Tested-by: Tomasz Nowicki <tn@semihalf.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Hanjun Guo <hanjun.guo@linaro.org>
Cc: Robin Murphy <robin.murphy@arm.com>
Cc: Joerg Roedel <joro@8bytes.org>
---
 drivers/iommu/iommu.c    | 42 ++++++++++++++++++++++++++++++++++++++++++
 drivers/iommu/of_iommu.c | 39 ---------------------------------------
 include/linux/iommu.h    | 14 ++++++++++++++
 include/linux/of_iommu.h | 12 ++++++++++--
 4 files changed, 66 insertions(+), 41 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 9a2f196..5c97c01 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1615,6 +1615,48 @@ int iommu_request_dm_for_dev(struct device *dev)
 	return ret;
 }
 
+struct iommu_fwentry {
+	struct list_head list;
+	struct fwnode_handle *fwnode;
+	const struct iommu_ops *ops;
+};
+static LIST_HEAD(iommu_fwentry_list);
+static DEFINE_SPINLOCK(iommu_fwentry_lock);
+
+void fwnode_iommu_set_ops(struct fwnode_handle *fwnode,
+			  const struct iommu_ops *ops)
+{
+	struct iommu_fwentry *iommu = kzalloc(sizeof(*iommu), GFP_KERNEL);
+
+	if (WARN_ON(!iommu))
+		return;
+
+	if (is_of_node(fwnode))
+		of_node_get(to_of_node(fwnode));
+
+	INIT_LIST_HEAD(&iommu->list);
+	iommu->fwnode = fwnode;
+	iommu->ops = ops;
+	spin_lock(&iommu_fwentry_lock);
+	list_add_tail(&iommu->list, &iommu_fwentry_list);
+	spin_unlock(&iommu_fwentry_lock);
+}
+
+const struct iommu_ops *fwnode_iommu_get_ops(struct fwnode_handle *fwnode)
+{
+	struct iommu_fwentry *node;
+	const struct iommu_ops *ops = NULL;
+
+	spin_lock(&iommu_fwentry_lock);
+	list_for_each_entry(node, &iommu_fwentry_list, list)
+		if (node->fwnode == fwnode) {
+			ops = node->ops;
+			break;
+		}
+	spin_unlock(&iommu_fwentry_lock);
+	return ops;
+}
+
 int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
 		      const struct iommu_ops *ops)
 {
diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index 5b82862..0f57ddc 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -96,45 +96,6 @@ int of_get_dma_window(struct device_node *dn, const char *prefix, int index,
 }
 EXPORT_SYMBOL_GPL(of_get_dma_window);
 
-struct of_iommu_node {
-	struct list_head list;
-	struct device_node *np;
-	const struct iommu_ops *ops;
-};
-static LIST_HEAD(of_iommu_list);
-static DEFINE_SPINLOCK(of_iommu_lock);
-
-void of_iommu_set_ops(struct device_node *np, const struct iommu_ops *ops)
-{
-	struct of_iommu_node *iommu = kzalloc(sizeof(*iommu), GFP_KERNEL);
-
-	if (WARN_ON(!iommu))
-		return;
-
-	of_node_get(np);
-	INIT_LIST_HEAD(&iommu->list);
-	iommu->np = np;
-	iommu->ops = ops;
-	spin_lock(&of_iommu_lock);
-	list_add_tail(&iommu->list, &of_iommu_list);
-	spin_unlock(&of_iommu_lock);
-}
-
-const struct iommu_ops *of_iommu_get_ops(struct device_node *np)
-{
-	struct of_iommu_node *node;
-	const struct iommu_ops *ops = NULL;
-
-	spin_lock(&of_iommu_lock);
-	list_for_each_entry(node, &of_iommu_list, list)
-		if (node->np == np) {
-			ops = node->ops;
-			break;
-		}
-	spin_unlock(&of_iommu_lock);
-	return ops;
-}
-
 static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data)
 {
 	struct of_phandle_args *iommu_spec = data;
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 436dc21..15d5478 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -351,6 +351,9 @@ 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 fwnode_iommu_set_ops(struct fwnode_handle *fwnode,
+			  const struct iommu_ops *ops);
+const struct iommu_ops *fwnode_iommu_get_ops(struct fwnode_handle *fwnode);
 
 #else /* CONFIG_IOMMU_API */
 
@@ -580,6 +583,17 @@ static inline int iommu_fwspec_add_ids(struct device *dev, u32 *ids,
 	return -ENODEV;
 }
 
+static inline void fwnode_iommu_set_ops(struct fwnode_handle *fwnode,
+					const struct iommu_ops *ops)
+{
+}
+
+static inline
+const struct iommu_ops *fwnode_iommu_get_ops(struct fwnode_handle *fwnode)
+{
+	return NULL;
+}
+
 #endif /* CONFIG_IOMMU_API */
 
 #endif /* __LINUX_IOMMU_H */
diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
index e80b9c7..7681007 100644
--- a/include/linux/of_iommu.h
+++ b/include/linux/of_iommu.h
@@ -31,8 +31,16 @@ static inline const struct iommu_ops *of_iommu_configure(struct device *dev,
 
 #endif	/* CONFIG_OF_IOMMU */
 
-void of_iommu_set_ops(struct device_node *np, const struct iommu_ops *ops);
-const struct iommu_ops *of_iommu_get_ops(struct device_node *np);
+static inline void of_iommu_set_ops(struct device_node *np,
+				    const struct iommu_ops *ops)
+{
+	fwnode_iommu_set_ops(&np->fwnode, ops);
+}
+
+static inline const struct iommu_ops *of_iommu_get_ops(struct device_node *np)
+{
+	return fwnode_iommu_get_ops(&np->fwnode);
+}
 
 extern struct of_device_id __iommu_of_table;
 
-- 
2.10.0

[toc] | [next] | [standalone]

#1518227 — Re: [PATCH v7 04/16] drivers: iommu: make of_iommu_set/get_ops() DT agnostic

On 09/11/16 14:19, Lorenzo Pieralisi wrote:
> The of_iommu_{set/get}_ops() API is used to associate a device
> tree node with a specific set of IOMMU operations. The same
> kernel interface is required on systems booting with ACPI, where
> devices are not associated with a device tree node, therefore
> the interface requires generalization.
> 
> The struct device fwnode member represents the fwnode token
> associated with the device and the struct it points at is firmware
> specific; regardless, it is initialized on both ACPI and DT systems
> and makes an ideal candidate to use it to associate a set of IOMMU
> operations to a given device, through its struct device.fwnode member
> pointer.
> 
> Convert the DT specific of_iommu_{set/get}_ops() interface to
> use struct device.fwnode as a look-up token, making the interface
> usable on ACPI systems.
> 
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Tested-by: Hanjun Guo <hanjun.guo@linaro.org>
> Tested-by: Tomasz Nowicki <tn@semihalf.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Hanjun Guo <hanjun.guo@linaro.org>
> Cc: Robin Murphy <robin.murphy@arm.com>
> Cc: Joerg Roedel <joro@8bytes.org>
> ---
>  drivers/iommu/iommu.c    | 42 ++++++++++++++++++++++++++++++++++++++++++
>  drivers/iommu/of_iommu.c | 39 ---------------------------------------
>  include/linux/iommu.h    | 14 ++++++++++++++
>  include/linux/of_iommu.h | 12 ++++++++++--
>  4 files changed, 66 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 9a2f196..5c97c01 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1615,6 +1615,48 @@ int iommu_request_dm_for_dev(struct device *dev)
>  	return ret;
>  }
>  
> +struct iommu_fwentry {

:)

> +	struct list_head list;
> +	struct fwnode_handle *fwnode;
> +	const struct iommu_ops *ops;
> +};
> +static LIST_HEAD(iommu_fwentry_list);
> +static DEFINE_SPINLOCK(iommu_fwentry_lock);
> +
> +void fwnode_iommu_set_ops(struct fwnode_handle *fwnode,
> +			  const struct iommu_ops *ops)
> +{
> +	struct iommu_fwentry *iommu = kzalloc(sizeof(*iommu), GFP_KERNEL);
> +
> +	if (WARN_ON(!iommu))
> +		return;
> +
> +	if (is_of_node(fwnode))

Nit: this check is actually redundant, since to_of_node() already does
the right thing and of_node_get() is NULL-safe - iommu_fwspec_init()
already works that way. On the other hand, though, it is perhaps more
intuitive to see it explicitly, and since to_of_node() is inline it
ought to result in the same object code (I've not checked, though).
Either way:

Reviewed-by: Robin Murphy <robin.murphy@arm.com>

> +		of_node_get(to_of_node(fwnode));
> +
> +	INIT_LIST_HEAD(&iommu->list);
> +	iommu->fwnode = fwnode;
> +	iommu->ops = ops;
> +	spin_lock(&iommu_fwentry_lock);
> +	list_add_tail(&iommu->list, &iommu_fwentry_list);
> +	spin_unlock(&iommu_fwentry_lock);
> +}
> +
> +const struct iommu_ops *fwnode_iommu_get_ops(struct fwnode_handle *fwnode)
> +{
> +	struct iommu_fwentry *node;
> +	const struct iommu_ops *ops = NULL;
> +
> +	spin_lock(&iommu_fwentry_lock);
> +	list_for_each_entry(node, &iommu_fwentry_list, list)
> +		if (node->fwnode == fwnode) {
> +			ops = node->ops;
> +			break;
> +		}
> +	spin_unlock(&iommu_fwentry_lock);
> +	return ops;
> +}
> +
>  int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
>  		      const struct iommu_ops *ops)
>  {
> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> index 5b82862..0f57ddc 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -96,45 +96,6 @@ int of_get_dma_window(struct device_node *dn, const char *prefix, int index,
>  }
>  EXPORT_SYMBOL_GPL(of_get_dma_window);
>  
> -struct of_iommu_node {
> -	struct list_head list;
> -	struct device_node *np;
> -	const struct iommu_ops *ops;
> -};
> -static LIST_HEAD(of_iommu_list);
> -static DEFINE_SPINLOCK(of_iommu_lock);
> -
> -void of_iommu_set_ops(struct device_node *np, const struct iommu_ops *ops)
> -{
> -	struct of_iommu_node *iommu = kzalloc(sizeof(*iommu), GFP_KERNEL);
> -
> -	if (WARN_ON(!iommu))
> -		return;
> -
> -	of_node_get(np);
> -	INIT_LIST_HEAD(&iommu->list);
> -	iommu->np = np;
> -	iommu->ops = ops;
> -	spin_lock(&of_iommu_lock);
> -	list_add_tail(&iommu->list, &of_iommu_list);
> -	spin_unlock(&of_iommu_lock);
> -}
> -
> -const struct iommu_ops *of_iommu_get_ops(struct device_node *np)
> -{
> -	struct of_iommu_node *node;
> -	const struct iommu_ops *ops = NULL;
> -
> -	spin_lock(&of_iommu_lock);
> -	list_for_each_entry(node, &of_iommu_list, list)
> -		if (node->np == np) {
> -			ops = node->ops;
> -			break;
> -		}
> -	spin_unlock(&of_iommu_lock);
> -	return ops;
> -}
> -
>  static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data)
>  {
>  	struct of_phandle_args *iommu_spec = data;
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 436dc21..15d5478 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -351,6 +351,9 @@ 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 fwnode_iommu_set_ops(struct fwnode_handle *fwnode,
> +			  const struct iommu_ops *ops);
> +const struct iommu_ops *fwnode_iommu_get_ops(struct fwnode_handle *fwnode);
>  
>  #else /* CONFIG_IOMMU_API */
>  
> @@ -580,6 +583,17 @@ static inline int iommu_fwspec_add_ids(struct device *dev, u32 *ids,
>  	return -ENODEV;
>  }
>  
> +static inline void fwnode_iommu_set_ops(struct fwnode_handle *fwnode,
> +					const struct iommu_ops *ops)
> +{
> +}
> +
> +static inline
> +const struct iommu_ops *fwnode_iommu_get_ops(struct fwnode_handle *fwnode)
> +{
> +	return NULL;
> +}
> +
>  #endif /* CONFIG_IOMMU_API */
>  
>  #endif /* __LINUX_IOMMU_H */
> diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
> index e80b9c7..7681007 100644
> --- a/include/linux/of_iommu.h
> +++ b/include/linux/of_iommu.h
> @@ -31,8 +31,16 @@ static inline const struct iommu_ops *of_iommu_configure(struct device *dev,
>  
>  #endif	/* CONFIG_OF_IOMMU */
>  
> -void of_iommu_set_ops(struct device_node *np, const struct iommu_ops *ops);
> -const struct iommu_ops *of_iommu_get_ops(struct device_node *np);
> +static inline void of_iommu_set_ops(struct device_node *np,
> +				    const struct iommu_ops *ops)
> +{
> +	fwnode_iommu_set_ops(&np->fwnode, ops);
> +}
> +
> +static inline const struct iommu_ops *of_iommu_get_ops(struct device_node *np)
> +{
> +	return fwnode_iommu_get_ops(&np->fwnode);
> +}
>  
>  extern struct of_device_id __iommu_of_table;
>  
> 

[toc] | [prev] | [next] | [standalone]

#1518871 — Re: [PATCH v7 04/16] drivers: iommu: make of_iommu_set/get_ops() DT agnostic

On Wed, Nov 09, 2016 at 02:40:08PM +0000, Robin Murphy wrote:

[...]

> > +void fwnode_iommu_set_ops(struct fwnode_handle *fwnode,
> > +			  const struct iommu_ops *ops)
> > +{
> > +	struct iommu_fwentry *iommu = kzalloc(sizeof(*iommu), GFP_KERNEL);
> > +
> > +	if (WARN_ON(!iommu))
> > +		return;
> > +
> > +	if (is_of_node(fwnode))
> 
> Nit: this check is actually redundant, since to_of_node() already does
> the right thing and of_node_get() is NULL-safe - iommu_fwspec_init()
> already works that way. On the other hand, though, it is perhaps more
> intuitive to see it explicitly, and since to_of_node() is inline it
> ought to result in the same object code (I've not checked, though).

I can easily fold this change in the final code and I think we
should keep this consistent so I am happy to change my code and
make the is_of_node() check implicit.

> Either way:
> 
> Reviewed-by: Robin Murphy <robin.murphy@arm.com>

Thank you !
Lorenzo

> > +		of_node_get(to_of_node(fwnode));
> > +
> > +	INIT_LIST_HEAD(&iommu->list);
> > +	iommu->fwnode = fwnode;
> > +	iommu->ops = ops;
> > +	spin_lock(&iommu_fwentry_lock);
> > +	list_add_tail(&iommu->list, &iommu_fwentry_list);
> > +	spin_unlock(&iommu_fwentry_lock);
> > +}
> > +
> > +const struct iommu_ops *fwnode_iommu_get_ops(struct fwnode_handle *fwnode)
> > +{
> > +	struct iommu_fwentry *node;
> > +	const struct iommu_ops *ops = NULL;
> > +
> > +	spin_lock(&iommu_fwentry_lock);
> > +	list_for_each_entry(node, &iommu_fwentry_list, list)
> > +		if (node->fwnode == fwnode) {
> > +			ops = node->ops;
> > +			break;
> > +		}
> > +	spin_unlock(&iommu_fwentry_lock);
> > +	return ops;
> > +}
> > +
> >  int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
> >  		      const struct iommu_ops *ops)
> >  {
> > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> > index 5b82862..0f57ddc 100644
> > --- a/drivers/iommu/of_iommu.c
> > +++ b/drivers/iommu/of_iommu.c
> > @@ -96,45 +96,6 @@ int of_get_dma_window(struct device_node *dn, const char *prefix, int index,
> >  }
> >  EXPORT_SYMBOL_GPL(of_get_dma_window);
> >  
> > -struct of_iommu_node {
> > -	struct list_head list;
> > -	struct device_node *np;
> > -	const struct iommu_ops *ops;
> > -};
> > -static LIST_HEAD(of_iommu_list);
> > -static DEFINE_SPINLOCK(of_iommu_lock);
> > -
> > -void of_iommu_set_ops(struct device_node *np, const struct iommu_ops *ops)
> > -{
> > -	struct of_iommu_node *iommu = kzalloc(sizeof(*iommu), GFP_KERNEL);
> > -
> > -	if (WARN_ON(!iommu))
> > -		return;
> > -
> > -	of_node_get(np);
> > -	INIT_LIST_HEAD(&iommu->list);
> > -	iommu->np = np;
> > -	iommu->ops = ops;
> > -	spin_lock(&of_iommu_lock);
> > -	list_add_tail(&iommu->list, &of_iommu_list);
> > -	spin_unlock(&of_iommu_lock);
> > -}
> > -
> > -const struct iommu_ops *of_iommu_get_ops(struct device_node *np)
> > -{
> > -	struct of_iommu_node *node;
> > -	const struct iommu_ops *ops = NULL;
> > -
> > -	spin_lock(&of_iommu_lock);
> > -	list_for_each_entry(node, &of_iommu_list, list)
> > -		if (node->np == np) {
> > -			ops = node->ops;
> > -			break;
> > -		}
> > -	spin_unlock(&of_iommu_lock);
> > -	return ops;
> > -}
> > -
> >  static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data)
> >  {
> >  	struct of_phandle_args *iommu_spec = data;
> > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > index 436dc21..15d5478 100644
> > --- a/include/linux/iommu.h
> > +++ b/include/linux/iommu.h
> > @@ -351,6 +351,9 @@ 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 fwnode_iommu_set_ops(struct fwnode_handle *fwnode,
> > +			  const struct iommu_ops *ops);
> > +const struct iommu_ops *fwnode_iommu_get_ops(struct fwnode_handle *fwnode);
> >  
> >  #else /* CONFIG_IOMMU_API */
> >  
> > @@ -580,6 +583,17 @@ static inline int iommu_fwspec_add_ids(struct device *dev, u32 *ids,
> >  	return -ENODEV;
> >  }
> >  
> > +static inline void fwnode_iommu_set_ops(struct fwnode_handle *fwnode,
> > +					const struct iommu_ops *ops)
> > +{
> > +}
> > +
> > +static inline
> > +const struct iommu_ops *fwnode_iommu_get_ops(struct fwnode_handle *fwnode)
> > +{
> > +	return NULL;
> > +}
> > +
> >  #endif /* CONFIG_IOMMU_API */
> >  
> >  #endif /* __LINUX_IOMMU_H */
> > diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h
> > index e80b9c7..7681007 100644
> > --- a/include/linux/of_iommu.h
> > +++ b/include/linux/of_iommu.h
> > @@ -31,8 +31,16 @@ static inline const struct iommu_ops *of_iommu_configure(struct device *dev,
> >  
> >  #endif	/* CONFIG_OF_IOMMU */
> >  
> > -void of_iommu_set_ops(struct device_node *np, const struct iommu_ops *ops);
> > -const struct iommu_ops *of_iommu_get_ops(struct device_node *np);
> > +static inline void of_iommu_set_ops(struct device_node *np,
> > +				    const struct iommu_ops *ops)
> > +{
> > +	fwnode_iommu_set_ops(&np->fwnode, ops);
> > +}
> > +
> > +static inline const struct iommu_ops *of_iommu_get_ops(struct device_node *np)
> > +{
> > +	return fwnode_iommu_get_ops(&np->fwnode);
> > +}
> >  
> >  extern struct of_device_id __iommu_of_table;
> >  
> > 
> 

[toc] | [prev] | [next] | [standalone]

#1519848 — Re: [PATCH v7 04/16] drivers: iommu: make of_iommu_set/get_ops() DT agnostic

On Wed, Nov 09, 2016 at 02:19:36PM +0000, Lorenzo Pieralisi wrote:
> +struct iommu_fwentry {
> +	struct list_head list;
> +	struct fwnode_handle *fwnode;
> +	const struct iommu_ops *ops;
> +};

Is there a reason the iommu_ops need to be stored there? Currently it
seems that the ops are only needed to get the of_xlate fn-ptr later. And
the place where it is called the iommu-ops should also be available
through pdev->dev->bus->iommu_ops.


	Joerg

[toc] | [prev] | [next] | [standalone]

#1519886 — Re: [PATCH v7 04/16] drivers: iommu: make of_iommu_set/get_ops() DT agnostic

On 11/11/16 15:22, Joerg Roedel wrote:
> On Wed, Nov 09, 2016 at 02:19:36PM +0000, Lorenzo Pieralisi wrote:
>> +struct iommu_fwentry {
>> +	struct list_head list;
>> +	struct fwnode_handle *fwnode;
>> +	const struct iommu_ops *ops;
>> +};
> 
> Is there a reason the iommu_ops need to be stored there? Currently it
> seems that the ops are only needed to get the of_xlate fn-ptr later. And
> the place where it is called the iommu-ops should also be available
> through pdev->dev->bus->iommu_ops.

In the original of_iommu_configure design, the thought was that an ops
structure could be IOMMU-instance-specific (hence the later-removed
"priv" member), so I suppose right now it is mostly a hangover from
that. However, it's also what we initialise a device's fwspec with, so
becomes important again if we're ever going to get past the limitations
of buses-which-are-not-actually-buses[1].

Robin.

[1]:http://www.mail-archive.com/iommu@lists.linux-foundation.org/msg14576.html

[toc] | [prev] | [next] | [standalone]

#1519890 — Re: [PATCH v7 04/16] drivers: iommu: make of_iommu_set/get_ops() DT agnostic

On Fri, Nov 11, 2016 at 04:17:37PM +0000, Robin Murphy wrote:
> In the original of_iommu_configure design, the thought was that an ops
> structure could be IOMMU-instance-specific (hence the later-removed
> "priv" member), so I suppose right now it is mostly a hangover from
> that. However, it's also what we initialise a device's fwspec with, so
> becomes important again if we're ever going to get past the limitations
> of buses-which-are-not-actually-buses[1].

Yeah, I discussed this with a few others at LPC. My current idea is to
tell the iommu-core which hardware-iommus exist in the system and a
seperate iommu_ops ptr for each of them. Then every struct device can
link to the iommu-instance it is translated by.

We are not there yet, but this will give you the same per-device
iommu-ops as implemented here.



	Joerg

[toc] | [prev] | [next] | [standalone]

#1519935 — Re: [PATCH v7 04/16] drivers: iommu: make of_iommu_set/get_ops() DT agnostic

On 11/11/16 16:27, Joerg Roedel wrote:
> On Fri, Nov 11, 2016 at 04:17:37PM +0000, Robin Murphy wrote:
>> In the original of_iommu_configure design, the thought was that an ops
>> structure could be IOMMU-instance-specific (hence the later-removed
>> "priv" member), so I suppose right now it is mostly a hangover from
>> that. However, it's also what we initialise a device's fwspec with, so
>> becomes important again if we're ever going to get past the limitations
>> of buses-which-are-not-actually-buses[1].
> 
> Yeah, I discussed this with a few others at LPC. My current idea is to
> tell the iommu-core which hardware-iommus exist in the system and a
> seperate iommu_ops ptr for each of them. Then every struct device can
> link to the iommu-instance it is translated by.

Er, that sounds very much like a description of what we already have in
4.9-rc. Every struct device now has an iommu_fwspec which encapsulates
both an iommu_ops pointer (which can perfectly well be per-instance if
the IOMMU driver wants) and a place for the IOMMU-private data to
replace the mess of archdata.iommu and driver-internal globals.

> We are not there yet, but this will give you the same per-device
> iommu-ops as implemented here.

With those two patches I linked to, which make the bulk of the IOMMU
core code per-device-ops-aware off the bat, I'd say we *are* already
pretty much there. It's only iommu_domain_alloc() which needs a
device-based alternative, and the non-of_xlate-based IOMMU drivers to
either call iommu_fwspec_init() for themselves, or perhaps for x86
plumbing in DMAR/IVRS equivalents of the IORT parsing to the
infrastructure provided by this series.

Robin.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web