Re: [PATCH v5 05/14] drivers: iommu: make iommu_fwspec OF agnostic

[Lorenzo Pieralisi]

On Tue, Sep 13, 2016 at 02:38:35PM +0100, Robin Murphy wrote:
> >  static int arm_smmu_match_node(struct device *dev, void *data)
> >  {
> > -   return dev->of_node == data;
> > +   struct fwnode_handle *fwnode;
> > +
> > +   fwnode = dev->of_node ? >of_node->fwnode : dev->fwnode;
> > +
> > +   return fwnode == data;
> >  }
> 
> Maybe we should hoist the dev_fwnode() helper from property.c up to
> property.h so we can just have "return dev_fwnode(dev) == data;" here?

Yes, that's one way of doing it. The other would be initializing
dev->fwnode to >of_node->fwnode in the DT probe path but first
I need to understand why that is not done in the first place.

[...]

> > +int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids)
> > +{
> > +   struct iommu_fwspec *fwspec = arch_get_iommu_fwspec(dev);
> > +   size_t size;
> > +
> > +   if (!fwspec)
> > +   return -EINVAL;
> > +
> > +   size = offsetof(struct iommu_fwspec, ids[fwspec->num_ids + 1]);
> > +   fwspec = krealloc(fwspec, size, GFP_KERNEL);
> > +   if (!fwspec)
> > +   return -ENOMEM;
> > +
> > +   while (num_ids--)
> > +   fwspec->ids[fwspec->num_ids++] = *ids++;
> 
> You've still got the +1 bug and incomprehensible loop from the old code
> here, rather than the fixed version being removed below. Although now
> that I've taken the plunge and done it properly in core code from the
> outset, that should hopefully become moot.

Gah sorry, rebase mistake. I will wait for the dust to settle before
churning out a new series, it is hard to respin without a stable
base (hopefully your series will make this patch useless).

Thanks !
Lorenzo

> Robin.
> 
> > +
> > +   arch_set_iommu_fwspec(dev, fwspec);
> > +   return 0;
> > +}
> > +
> > +inline struct iommu_fwspec *dev_iommu_fwspec(struct device *dev)
> > +{
> > +   return arch_get_iommu_fwspec(dev);
> > +}
> > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> > index 38669b8..ab3c069 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(>list);
> > -   iommu->np = np;
> > -   iommu->ops = ops;
> > -   spin_lock(_iommu_lock);
> > -   list_add_tail(>list, _iommu_list);
> > -   spin_unlock(_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(_iommu_lock);
> > -   list_for_each_entry(node, _iommu_list, list)
> > -   if (node->np == np) {
> > -   ops = node->ops;
> > -   break;
> > -   }
> > -   spin_unlock(_iommu_lock);
> > -   return ops;
> > -}
> > -
> >  static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data)
> >  {
> > struct of_phandle_args *iommu_spec = data;
> > @@ -226,57 +187,3 @@ static int __init of_iommu_init(void)
> > return 0;
> >  }
> >  postcore_initcall_sync(of_iommu_init);
> > -
> > -int iommu_fwspec_init(struct device *dev, struct device_node *iommu_np)
> > -{
> > -   struct iommu_fwspec *fwspec = arch_get_iommu_fwspec(dev);
> > -
> > -   if (fwspec)
> > -   return 0;
> > -
> > -   fwspec = kzalloc(sizeof(*fwspec), GFP_KERNEL);
> > -   if (!fwspec)
> > -   return -ENOMEM;
> > -
> > -   fwspec->iommu_np = of_node_get(iommu_np);
> > -   fwspec->iommu_ops = of_iommu_get_ops(iommu_np);
> > -   arch_set_iommu_fwspec(dev, fwspec);
> > -   return 0;
> > -}
> > -
> > -void iommu_fwspec_free(struct device *dev)
> > -{
> > -   struct iommu_fwspec *fwspec = arch_get_iommu_fwspec(dev);
> > -
> > -   if (fwspec) {
> > -   of_node_put(fwspec->iommu_np);
> > -   kfree(fwspec);
> > -   }
> > -}
> > -
> > -int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids)
> > -{
> > -   struct iommu_fwspec *fwspec = arch_get_iommu_fwspec(dev);
> > -   size_t size;
> > -   int i;
> > -
> > -   if (!fwspec)
> > -   return -EINVAL;
> > -
> > -   size = offsetof(struct iommu_fwspec, ids[fwspec->num_ids + num_ids]);
> > -   fwspec = krealloc(fwspec, size, GFP_KERNEL);
> > -   if (!fwspec)
> > -   return -ENOMEM;
> > -
> > -   for (i = 0; i < num_ids; i++)
> > -   fwspec->ids[fwspec->num_ids + i] = ids[i];
> > -
> > -   fwspec->num_ids += num_ids;
> > -   arch_set_iommu_fwspec(dev, fwspec);
> > -   return 0;
> > -}
> > 

Re: [PATCH v5 05/14] drivers: iommu: make iommu_fwspec OF agnostic

[Robin Murphy]

Hi Lorenzo,

On 09/09/16 15:23, Lorenzo Pieralisi wrote:
> The iommu_fwspec structure, used to hold per device iommu configuration
> data is not OF specific and therefore can be moved to a generic
> and OF independent compilation unit.
> 
> In particular, the iommu_fwspec handling hinges on the device_node
> pointer to identify the IOMMU device associated with the iommu_fwspec
> structure, that is easily converted to a more generic fwnode_handle
> pointer that can cater for OF and non-OF (ie ACPI) systems.
> 
> Create the files and related Kconfig entry to decouple iommu_fwspec
> structure from the OF iommu kernel layer.
> 
> Given that the current iommu_fwspec implementation relies on
> the arch specific struct device.archdata.iommu field in its
> implementation, by making the code standalone and independent
> of the OF layer this patch makes sure that the iommu_fwspec
> kernel code can be selected only on arches implementing the
> struct device.archdata.iommu field by adding an explicit
> arch dependency in its config entry.
> 
> Current drivers using the iommu_fwspec for streamid translation
> are converted to the new iommu_fwspec API by simply converting
> the device_node to its fwnode_handle pointer.
> 
> Signed-off-by: Lorenzo Pieralisi 
> Cc: Will Deacon 
> Cc: Hanjun Guo 
> Cc: Robin Murphy 
> Cc: Joerg Roedel 
> ---
>  drivers/iommu/Kconfig|   4 ++
>  drivers/iommu/Makefile   |   1 +
>  drivers/iommu/arm-smmu-v3.c  |  16 --
>  drivers/iommu/arm-smmu.c |  17 +++---
>  drivers/iommu/iommu-fwspec.c | 126 
> +++
>  drivers/iommu/of_iommu.c |  93 
>  include/linux/iommu-fwspec.h |  70 
>  include/linux/of_iommu.h |  38 -
>  8 files changed, 234 insertions(+), 131 deletions(-)
>  create mode 100644 drivers/iommu/iommu-fwspec.c
>  create mode 100644 include/linux/iommu-fwspec.h
> 
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index 101cb17..873bd41 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -70,6 +70,10 @@ config OF_IOMMU
>  config HAVE_IOMMU_FWSPEC
>   bool
>  
> +config IOMMU_FWSPEC
> +   def_bool y
> +   depends on IOMMU_API
> +
>  # IOMMU-agnostic DMA-mapping layer
>  config IOMMU_DMA
>   bool
> diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
> index 195f7b9..bbbc6d6 100644
> --- a/drivers/iommu/Makefile
> +++ b/drivers/iommu/Makefile
> @@ -6,6 +6,7 @@ obj-$(CONFIG_IOMMU_IO_PGTABLE) += io-pgtable.o
>  obj-$(CONFIG_IOMMU_IO_PGTABLE_ARMV7S) += io-pgtable-arm-v7s.o
>  obj-$(CONFIG_IOMMU_IO_PGTABLE_LPAE) += io-pgtable-arm.o
>  obj-$(CONFIG_IOMMU_IOVA) += iova.o
> +obj-$(CONFIG_IOMMU_FWSPEC) += iommu-fwspec.o
>  obj-$(CONFIG_OF_IOMMU)   += of_iommu.o
>  obj-$(CONFIG_MSM_IOMMU) += msm_iommu.o
>  obj-$(CONFIG_AMD_IOMMU) += amd_iommu.o amd_iommu_init.o
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index be293b5..a7e9de9 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -25,6 +25,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -1720,13 +1721,18 @@ static struct platform_driver arm_smmu_driver;
>  
>  static int arm_smmu_match_node(struct device *dev, void *data)
>  {
> - return dev->of_node == data;
> + struct fwnode_handle *fwnode;
> +
> + fwnode = dev->of_node ? >of_node->fwnode : dev->fwnode;
> +
> + return fwnode == data;
>  }

Maybe we should hoist the dev_fwnode() helper from property.c up to
property.h so we can just have "return dev_fwnode(dev) == data;" here?

>  
> -static struct arm_smmu_device *arm_smmu_get_by_node(struct device_node *np)
> +static struct arm_smmu_device *
> +arm_smmu_get_by_fwnode(struct fwnode_handle *fwnode)
>  {
>   struct device *dev = driver_find_device(_smmu_driver.driver, NULL,
> - np, arm_smmu_match_node);
> + fwnode, arm_smmu_match_node);
>   put_device(dev);
>   return dev ? dev_get_drvdata(dev) : NULL;
>  }
> @@ -1762,7 +1768,7 @@ static int arm_smmu_add_device(struct device *dev)
>   master = fwspec->iommu_priv;
>   smmu = master->smmu;
>   } else {
> - smmu = arm_smmu_get_by_node(fwspec->iommu_np);
> + smmu = arm_smmu_get_by_fwnode(fwspec->iommu_fwnode);
>   if (!smmu)
>   return -ENODEV;
>   master = kzalloc(sizeof(*master), GFP_KERNEL);
> @@ -1874,7 +1880,7 @@ out_unlock:
>  
>  static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args 
> *args)
>  {
> - int ret = iommu_fwspec_init(dev, args->np);
> + int ret = iommu_fwspec_init(dev, >np->fwnode);
>  
>   if (!ret)
>   ret = 

[PATCH v5 05/14] drivers: iommu: make iommu_fwspec OF agnostic

[Lorenzo Pieralisi]

The iommu_fwspec structure, used to hold per device iommu configuration
data is not OF specific and therefore can be moved to a generic
and OF independent compilation unit.

In particular, the iommu_fwspec handling hinges on the device_node
pointer to identify the IOMMU device associated with the iommu_fwspec
structure, that is easily converted to a more generic fwnode_handle
pointer that can cater for OF and non-OF (ie ACPI) systems.

Create the files and related Kconfig entry to decouple iommu_fwspec
structure from the OF iommu kernel layer.

Given that the current iommu_fwspec implementation relies on
the arch specific struct device.archdata.iommu field in its
implementation, by making the code standalone and independent
of the OF layer this patch makes sure that the iommu_fwspec
kernel code can be selected only on arches implementing the
struct device.archdata.iommu field by adding an explicit
arch dependency in its config entry.

Current drivers using the iommu_fwspec for streamid translation
are converted to the new iommu_fwspec API by simply converting
the device_node to its fwnode_handle pointer.

Signed-off-by: Lorenzo Pieralisi 
Cc: Will Deacon 
Cc: Hanjun Guo 
Cc: Robin Murphy 
Cc: Joerg Roedel 
---
 drivers/iommu/Kconfig|   4 ++
 drivers/iommu/Makefile   |   1 +
 drivers/iommu/arm-smmu-v3.c  |  16 --
 drivers/iommu/arm-smmu.c |  17 +++---
 drivers/iommu/iommu-fwspec.c | 126 +++
 drivers/iommu/of_iommu.c |  93 
 include/linux/iommu-fwspec.h |  70 
 include/linux/of_iommu.h |  38 -
 8 files changed, 234 insertions(+), 131 deletions(-)
 create mode 100644 drivers/iommu/iommu-fwspec.c
 create mode 100644 include/linux/iommu-fwspec.h

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index 101cb17..873bd41 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -70,6 +70,10 @@ config OF_IOMMU
 config HAVE_IOMMU_FWSPEC
bool
 
+config IOMMU_FWSPEC
+   def_bool y
+   depends on IOMMU_API
+
 # IOMMU-agnostic DMA-mapping layer
 config IOMMU_DMA
bool
diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
index 195f7b9..bbbc6d6 100644
--- a/drivers/iommu/Makefile
+++ b/drivers/iommu/Makefile
@@ -6,6 +6,7 @@ obj-$(CONFIG_IOMMU_IO_PGTABLE) += io-pgtable.o
 obj-$(CONFIG_IOMMU_IO_PGTABLE_ARMV7S) += io-pgtable-arm-v7s.o
 obj-$(CONFIG_IOMMU_IO_PGTABLE_LPAE) += io-pgtable-arm.o
 obj-$(CONFIG_IOMMU_IOVA) += iova.o
+obj-$(CONFIG_IOMMU_FWSPEC) += iommu-fwspec.o
 obj-$(CONFIG_OF_IOMMU) += of_iommu.o
 obj-$(CONFIG_MSM_IOMMU) += msm_iommu.o
 obj-$(CONFIG_AMD_IOMMU) += amd_iommu.o amd_iommu_init.o
diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index be293b5..a7e9de9 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -25,6 +25,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -1720,13 +1721,18 @@ static struct platform_driver arm_smmu_driver;
 
 static int arm_smmu_match_node(struct device *dev, void *data)
 {
-   return dev->of_node == data;
+   struct fwnode_handle *fwnode;
+
+   fwnode = dev->of_node ? >of_node->fwnode : dev->fwnode;
+
+   return fwnode == data;
 }
 
-static struct arm_smmu_device *arm_smmu_get_by_node(struct device_node *np)
+static struct arm_smmu_device *
+arm_smmu_get_by_fwnode(struct fwnode_handle *fwnode)
 {
struct device *dev = driver_find_device(_smmu_driver.driver, NULL,
-   np, arm_smmu_match_node);
+   fwnode, arm_smmu_match_node);
put_device(dev);
return dev ? dev_get_drvdata(dev) : NULL;
 }
@@ -1762,7 +1768,7 @@ static int arm_smmu_add_device(struct device *dev)
master = fwspec->iommu_priv;
smmu = master->smmu;
} else {
-   smmu = arm_smmu_get_by_node(fwspec->iommu_np);
+   smmu = arm_smmu_get_by_fwnode(fwspec->iommu_fwnode);
if (!smmu)
return -ENODEV;
master = kzalloc(sizeof(*master), GFP_KERNEL);
@@ -1874,7 +1880,7 @@ out_unlock:
 
 static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *args)
 {
-   int ret = iommu_fwspec_init(dev, args->np);
+   int ret = iommu_fwspec_init(dev, >np->fwnode);
 
if (!ret)
ret = iommu_fwspec_add_ids(dev, >args[0], 1);
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 2e20cdc..d453c55 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -517,7 +517,7 @@ static int arm_smmu_register_legacy_master(struct device 
*dev,
it.cur_count = 1;
}
 
-   err = iommu_fwspec_init(dev, smmu_dev->of_node);
+   err =