[PATCH 00/04] iommu/ipmmu-vmsa: 32-bit ARM update

[PATCH 00/04] iommu/ipmmu-vmsa: 32-bit ARM update

[Magnus Damm]

iommu/ipmmu-vmsa: 32-bit ARM update

[PATCH 01/04] iommu/ipmmu-vmsa: Use iommu_device_register()/unregister()
[PATCH 02/04] iommu/ipmmu-vmsa: Consistent ->of_xlate() handling
[PATCH 03/04] iommu/ipmmu-vmsa: Use fwspec on both 32 and 64-bit ARM
[PATCH 04/04] iommu/ipmmu-vmsa: Replace local utlb code with fwspec ids

This series updates the IPMMU driver to make use of recent IOMMU framework
changes and also improves code sharing in the driver between the 32-bit and
64-bit dma-mapping architecture glue code.

Suggested-by: Robin Murphy <robin.murphy@arm.com> (Patch 2 and 4)
Signed-off-by: Robin Murphy <robin.murphy@arm.com> (Patch 3)
Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
---

 Developed on renesas-drivers-2017-06-13-v4.12-rc5 and rebased to next-20170614

 drivers/iommu/ipmmu-vmsa.c |  186 +++++++++++---------------------------------
 1 file changed, 49 insertions(+), 137 deletions(-)

[PATCH 01/04] iommu/ipmmu-vmsa: Use iommu_device_register()/unregister()

[Magnus Damm]

From: Magnus Damm <damm+renesas@opensource.se>

Extend the driver to make use of iommu_device_register()/unregister()
functions together with iommu_device_set_ops() and iommu_set_fwnode().

These used to be part of the earlier posted 64-bit ARM (r8a7795) series but
it turns out that these days they are required on 32-bit ARM as well.

Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
---

 drivers/iommu/ipmmu-vmsa.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

--- 0001/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c	2017-06-15 16:57:21.890607110 +0900
@@ -35,6 +35,7 @@
 struct ipmmu_vmsa_device {
 	struct device *dev;
 	void __iomem *base;
+	struct iommu_device iommu;
 	struct list_head list;
 
 	unsigned int num_utlbs;
@@ -1054,6 +1055,13 @@ static int ipmmu_probe(struct platform_d
 
 	ipmmu_device_reset(mmu);
 
+	ret = iommu_device_register(&mmu->iommu);
+	if (ret)
+		return ret;
+
+	iommu_device_set_ops(&mmu->iommu, &ipmmu_ops);
+	iommu_device_set_fwnode(&mmu->iommu, &pdev->dev.of_node->fwnode);
+
 	/*
 	 * We can't create the ARM mapping here as it requires the bus to have
 	 * an IOMMU, which only happens when bus_set_iommu() is called in
@@ -1077,6 +1085,8 @@ static int ipmmu_remove(struct platform_
 	list_del(&mmu->list);
 	spin_unlock(&ipmmu_devices_lock);
 
+	iommu_device_unregister(&mmu->iommu);
+
 #if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)
 	arm_iommu_release_mapping(mmu->mapping);
 #endif

Re: [PATCH 01/04] iommu/ipmmu-vmsa: Use iommu_device_register()/unregister()

[Robin Murphy]

On 15/06/17 11:29, Magnus Damm wrote:
> From: Magnus Damm <damm+renesas@opensource.se>
> 
> Extend the driver to make use of iommu_device_register()/unregister()
> functions together with iommu_device_set_ops() and iommu_set_fwnode().
> 
> These used to be part of the earlier posted 64-bit ARM (r8a7795) series but
> it turns out that these days they are required on 32-bit ARM as well.
> 
> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
> ---
> 
>  drivers/iommu/ipmmu-vmsa.c |   10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> --- 0001/drivers/iommu/ipmmu-vmsa.c
> +++ work/drivers/iommu/ipmmu-vmsa.c	2017-06-15 16:57:21.890607110 +0900
> @@ -35,6 +35,7 @@
>  struct ipmmu_vmsa_device {
>  	struct device *dev;
>  	void __iomem *base;
> +	struct iommu_device iommu;
>  	struct list_head list;
>  
>  	unsigned int num_utlbs;
> @@ -1054,6 +1055,13 @@ static int ipmmu_probe(struct platform_d
>  
>  	ipmmu_device_reset(mmu);
>  
> +	ret = iommu_device_register(&mmu->iommu);
> +	if (ret)
> +		return ret;
> +
> +	iommu_device_set_ops(&mmu->iommu, &ipmmu_ops);
> +	iommu_device_set_fwnode(&mmu->iommu, &pdev->dev.of_node->fwnode);

Nit: I guess it works out OK in practice, but it does look rather weird
to be initialising the iommu_device's members *after* registering it.

Robin.

> +
>  	/*
>  	 * We can't create the ARM mapping here as it requires the bus to have
>  	 * an IOMMU, which only happens when bus_set_iommu() is called in
> @@ -1077,6 +1085,8 @@ static int ipmmu_remove(struct platform_
>  	list_del(&mmu->list);
>  	spin_unlock(&ipmmu_devices_lock);
>  
> +	iommu_device_unregister(&mmu->iommu);
> +
>  #if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)
>  	arm_iommu_release_mapping(mmu->mapping);
>  #endif
> 

[PATCH 02/04] iommu/ipmmu-vmsa: Consistent ->of_xlate() handling

[Magnus Damm]

From: Magnus Damm <damm+renesas@opensource.se>

The 32-bit ARM code gets updated to make use of ->of_xlate() and the
code is shared between 64-bit and 32-bit ARM. The of_device_is_available()
check gets dropped since it is included in of_iommu_xlate().

Suggested-by: Robin Murphy <robin.murphy@arm.com>
Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
---

 drivers/iommu/ipmmu-vmsa.c |   51 ++++++++++++++------------------------------
 1 file changed, 17 insertions(+), 34 deletions(-)

--- 0009/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c	2017-06-15 17:22:27.460607110 +0900
@@ -680,6 +680,10 @@ static int ipmmu_init_platform_device(st
 	int num_utlbs;
 	int ret = -ENODEV;
 
+	/* Initialize once - xlate() will call multiple times */
+	if (to_priv(dev))
+		return 0;
+
 	/* Find the master corresponding to the device. */
 
 	num_utlbs = of_count_phandle_with_args(dev->of_node, "iommus",
@@ -734,6 +738,12 @@ error:
 	return ret;
 }
 
+static int ipmmu_of_xlate(struct device *dev,
+			  struct of_phandle_args *spec)
+{
+	return ipmmu_init_platform_device(dev);
+}
+
 #if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)
 
 static struct iommu_domain *ipmmu_domain_alloc(unsigned type)
@@ -750,11 +760,11 @@ static int ipmmu_add_device(struct devic
 	struct iommu_group *group;
 	int ret;
 
-	if (to_priv(dev)) {
-		dev_warn(dev, "IOMMU driver already assigned to device %s\n",
-			 dev_name(dev));
-		return -EINVAL;
-	}
+	/*
+	 * Only let through devices that have been verified in xlate()
+	 */
+	if (!to_priv(dev))
+		return -ENODEV;
 
 	/* Create a device group and add the device to it. */
 	group = iommu_group_alloc();
@@ -773,10 +783,6 @@ static int ipmmu_add_device(struct devic
 		goto error;
 	}
 
-	ret = ipmmu_init_platform_device(dev);
-	if (ret < 0)
-		goto error;
-
 	/*
 	 * Create the ARM mapping, used by the ARM DMA mapping core to allocate
 	 * VAs. This will allocate a corresponding IOMMU domain.
@@ -817,24 +823,13 @@ error:
 	if (!IS_ERR_OR_NULL(group))
 		iommu_group_remove_device(dev);
 
-	kfree(to_priv(dev)->utlbs);
-	kfree(to_priv(dev));
-	set_priv(dev, NULL);
-
 	return ret;
 }
 
 static void ipmmu_remove_device(struct device *dev)
 {
-	struct ipmmu_vmsa_iommu_priv *priv = to_priv(dev);
-
 	arm_iommu_detach_device(dev);
 	iommu_group_remove_device(dev);
-
-	kfree(priv->utlbs);
-	kfree(priv);
-
-	set_priv(dev, NULL);
 }
 
 static const struct iommu_ops ipmmu_ops = {
@@ -849,6 +844,7 @@ static const struct iommu_ops ipmmu_ops
 	.add_device = ipmmu_add_device,
 	.remove_device = ipmmu_remove_device,
 	.pgsize_bitmap = SZ_1G | SZ_2M | SZ_4K,
+	.of_xlate = ipmmu_of_xlate,
 };
 
 #endif /* !CONFIG_ARM && CONFIG_IOMMU_DMA */
@@ -958,19 +954,6 @@ static struct iommu_group *ipmmu_find_gr
 	return group;
 }
 
-static int ipmmu_of_xlate_dma(struct device *dev,
-			      struct of_phandle_args *spec)
-{
-	/* If the IPMMU device is disabled in DT then return error
-	 * to make sure the of_iommu code does not install ops
-	 * even though the iommu device is disabled
-	 */
-	if (!of_device_is_available(spec->np))
-		return -ENODEV;
-
-	return ipmmu_init_platform_device(dev);
-}
-
 static const struct iommu_ops ipmmu_ops = {
 	.domain_alloc = ipmmu_domain_alloc_dma,
 	.domain_free = ipmmu_domain_free_dma,
@@ -984,7 +967,7 @@ static const struct iommu_ops ipmmu_ops
 	.remove_device = ipmmu_remove_device_dma,
 	.device_group = ipmmu_find_group_dma,
 	.pgsize_bitmap = SZ_1G | SZ_2M | SZ_4K,
-	.of_xlate = ipmmu_of_xlate_dma,
+	.of_xlate = ipmmu_of_xlate,
 };
 
 #endif /* CONFIG_IOMMU_DMA */

Re: [PATCH 02/04] iommu/ipmmu-vmsa: Consistent ->of_xlate() handling

[Geert Uytterhoeven]

Hi Magnus,

On Thu, Jun 15, 2017 at 12:29 PM, Magnus Damm <magnus.damm@gmail.com> wrote:
> From: Magnus Damm <damm+renesas@opensource.se>
>
> The 32-bit ARM code gets updated to make use of ->of_xlate() and the
> code is shared between 64-bit and 32-bit ARM. The of_device_is_available()
> check gets dropped since it is included in of_iommu_xlate().
>
> Suggested-by: Robin Murphy <robin.murphy@arm.com>
> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
> ---
>
>  drivers/iommu/ipmmu-vmsa.c |   51 ++++++++++++++------------------------------
>  1 file changed, 17 insertions(+), 34 deletions(-)
>
> --- 0009/drivers/iommu/ipmmu-vmsa.c
> +++ work/drivers/iommu/ipmmu-vmsa.c     2017-06-15 17:22:27.460607110 +0900
> @@ -680,6 +680,10 @@ static int ipmmu_init_platform_device(st
>         int num_utlbs;
>         int ret = -ENODEV;
>
> +       /* Initialize once - xlate() will call multiple times */
> +       if (to_priv(dev))
> +               return 0;

I think this is more clear if you move this check to ...

> +
>         /* Find the master corresponding to the device. */
>
>         num_utlbs = of_count_phandle_with_args(dev->of_node, "iommus",
> @@ -734,6 +738,12 @@ error:
>         return ret;
>  }
>
> +static int ipmmu_of_xlate(struct device *dev,
> +                         struct of_phandle_args *spec)
> +{

... here

> +       return ipmmu_init_platform_device(dev);
> +}

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH 03/04] iommu/ipmmu-vmsa: Use fwspec on both 32 and 64-bit ARM

[Magnus Damm]

From: Robin Murphy <robin.murphy@arm.com>

Consolidate the 32-bit and 64-bit code to make use of fwspec instead
of archdata for the 32-bit ARM case.

This is a simplified version of the fwspec handling code from Robin
posted as [PATCH] iommu/ipmmu-vmsa: Convert to iommu_fwspec

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
---

 drivers/iommu/ipmmu-vmsa.c |   21 +++------------------
 1 file changed, 3 insertions(+), 18 deletions(-)

--- 0010/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c	2017-06-15 17:29:00.290607110 +0900
@@ -73,22 +73,9 @@ static struct ipmmu_vmsa_domain *to_vmsa
 	return container_of(dom, struct ipmmu_vmsa_domain, io_domain);
 }
 
-
 static struct ipmmu_vmsa_iommu_priv *to_priv(struct device *dev)
 {
-#if defined(CONFIG_ARM)
-	return dev->archdata.iommu;
-#else
-	return dev->iommu_fwspec->iommu_priv;
-#endif
-}
-static void set_priv(struct device *dev, struct ipmmu_vmsa_iommu_priv *p)
-{
-#if defined(CONFIG_ARM)
-	dev->archdata.iommu = p;
-#else
-	dev->iommu_fwspec->iommu_priv = p;
-#endif
+	return dev->iommu_fwspec ? dev->iommu_fwspec->iommu_priv : NULL;
 }
 
 #define TLB_LOOP_TIMEOUT		100	/* 100us */
@@ -730,7 +717,7 @@ static int ipmmu_init_platform_device(st
 	priv->utlbs = utlbs;
 	priv->num_utlbs = num_utlbs;
 	priv->dev = dev;
-	set_priv(dev, priv);
+	dev->iommu_fwspec->iommu_priv = priv;
 	return 0;
 
 error:
@@ -887,14 +874,12 @@ static void ipmmu_domain_free_dma(struct
 
 static int ipmmu_add_device_dma(struct device *dev)
 {
-	struct iommu_fwspec *fwspec = dev->iommu_fwspec;
 	struct iommu_group *group;
 
 	/*
 	 * Only let through devices that have been verified in xlate()
-	 * We may get called with dev->iommu_fwspec set to NULL.
 	 */
-	if (!fwspec || !fwspec->iommu_priv)
+	if (!to_priv(dev))
 		return -ENODEV;
 
 	group = iommu_group_get_for_dev(dev);

[PATCH 04/04] iommu/ipmmu-vmsa: Replace local utlb code with fwspec ids

[Magnus Damm]

From: Magnus Damm <damm+renesas@opensource.se>

Now when both 32-bit and 64-bit code inside the driver is using
fwspec it is possible to replace the utlb handling with fwspec ids
that get populated from ->of_xlate().

Suggested-by: Robin Murphy <robin.murphy@arm.com>
Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
---

 drivers/iommu/ipmmu-vmsa.c |  104 ++++++++------------------------------------
 1 file changed, 19 insertions(+), 85 deletions(-)

--- 0013/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c	2017-06-15 18:32:27.580607110 +0900
@@ -19,6 +19,7 @@
 #include <linux/iommu.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <linux/sizes.h>
 #include <linux/slab.h>
@@ -59,8 +60,6 @@ struct ipmmu_vmsa_domain {
 
 struct ipmmu_vmsa_iommu_priv {
 	struct ipmmu_vmsa_device *mmu;
-	unsigned int *utlbs;
-	unsigned int num_utlbs;
 	struct device *dev;
 	struct list_head list;
 };
@@ -550,13 +549,14 @@ static int ipmmu_attach_device(struct io
 			       struct device *dev)
 {
 	struct ipmmu_vmsa_iommu_priv *priv = to_priv(dev);
+	struct iommu_fwspec *fwspec = dev->iommu_fwspec;
 	struct ipmmu_vmsa_device *mmu = priv->mmu;
 	struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
 	unsigned long flags;
 	unsigned int i;
 	int ret = 0;
 
-	if (!mmu) {
+	if (!priv || !priv->mmu) {
 		dev_err(dev, "Cannot attach to IPMMU\n");
 		return -ENXIO;
 	}
@@ -583,8 +583,8 @@ static int ipmmu_attach_device(struct io
 	if (ret < 0)
 		return ret;
 
-	for (i = 0; i < priv->num_utlbs; ++i)
-		ipmmu_utlb_enable(domain, priv->utlbs[i]);
+	for (i = 0; i < fwspec->num_ids; ++i)
+		ipmmu_utlb_enable(domain, fwspec->ids[i]);
 
 	return 0;
 }
@@ -592,12 +592,12 @@ static int ipmmu_attach_device(struct io
 static void ipmmu_detach_device(struct iommu_domain *io_domain,
 				struct device *dev)
 {
-	struct ipmmu_vmsa_iommu_priv *priv = to_priv(dev);
+	struct iommu_fwspec *fwspec = dev->iommu_fwspec;
 	struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
 	unsigned int i;
 
-	for (i = 0; i < priv->num_utlbs; ++i)
-		ipmmu_utlb_disable(domain, priv->utlbs[i]);
+	for (i = 0; i < fwspec->num_ids; ++i)
+		ipmmu_utlb_disable(domain, fwspec->ids[i]);
 
 	/*
 	 * TODO: Optimize by disabling the context when no device is attached.
@@ -633,102 +633,36 @@ static phys_addr_t ipmmu_iova_to_phys(st
 	return domain->iop->iova_to_phys(domain->iop, iova);
 }
 
-static int ipmmu_find_utlbs(struct ipmmu_vmsa_device *mmu, struct device *dev,
-			    unsigned int *utlbs, unsigned int num_utlbs)
-{
-	unsigned int i;
-
-	for (i = 0; i < num_utlbs; ++i) {
-		struct of_phandle_args args;
-		int ret;
-
-		ret = of_parse_phandle_with_args(dev->of_node, "iommus",
-						 "#iommu-cells", i, &args);
-		if (ret < 0)
-			return ret;
-
-		of_node_put(args.np);
-
-		if (args.np != mmu->dev->of_node || args.args_count != 1)
-			return -EINVAL;
-
-		utlbs[i] = args.args[0];
-	}
-
-	return 0;
-}
-
-static int ipmmu_init_platform_device(struct device *dev)
+static int ipmmu_init_platform_device(struct device *dev,
+				      struct of_phandle_args *args)
 {
+	struct platform_device *ipmmu_pdev;
 	struct ipmmu_vmsa_iommu_priv *priv;
-	struct ipmmu_vmsa_device *mmu;
-	unsigned int *utlbs;
-	unsigned int i;
-	int num_utlbs;
-	int ret = -ENODEV;
 
 	/* Initialize once - xlate() will call multiple times */
 	if (to_priv(dev))
 		return 0;
 
-	/* Find the master corresponding to the device. */
-
-	num_utlbs = of_count_phandle_with_args(dev->of_node, "iommus",
-					       "#iommu-cells");
-	if (num_utlbs < 0)
+	ipmmu_pdev = of_find_device_by_node(args->np);
+	if (!ipmmu_pdev)
 		return -ENODEV;
 
-	utlbs = kcalloc(num_utlbs, sizeof(*utlbs), GFP_KERNEL);
-	if (!utlbs)
-		return -ENOMEM;
-
-	spin_lock(&ipmmu_devices_lock);
-
-	list_for_each_entry(mmu, &ipmmu_devices, list) {
-		ret = ipmmu_find_utlbs(mmu, dev, utlbs, num_utlbs);
-		if (!ret) {
-			/*
-			 * TODO Take a reference to the MMU to protect
-			 * against device removal.
-			 */
-			break;
-		}
-	}
-
-	spin_unlock(&ipmmu_devices_lock);
-
-	if (ret < 0)
-		goto error;
-
-	for (i = 0; i < num_utlbs; ++i) {
-		if (utlbs[i] >= mmu->num_utlbs) {
-			ret = -EINVAL;
-			goto error;
-		}
-	}
-
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
-	if (!priv) {
-		ret = -ENOMEM;
-		goto error;
-	}
+	if (!priv)
+		return -ENOMEM;
 
-	priv->mmu = mmu;
-	priv->utlbs = utlbs;
-	priv->num_utlbs = num_utlbs;
+	priv->mmu = platform_get_drvdata(ipmmu_pdev);
 	priv->dev = dev;
 	dev->iommu_fwspec->iommu_priv = priv;
 	return 0;
-
-error:
-	kfree(utlbs);
-	return ret;
 }
 
 static int ipmmu_of_xlate(struct device *dev,
 			  struct of_phandle_args *spec)
 {
-	return ipmmu_init_platform_device(dev);
+	iommu_fwspec_add_ids(dev, spec->args, 1);
+
+	return ipmmu_init_platform_device(dev, spec);
 }
 
 #if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)

Re: [PATCH 04/04] iommu/ipmmu-vmsa: Replace local utlb code with fwspec ids

[Geert Uytterhoeven]

Hi Magnus,

On Thu, Jun 15, 2017 at 12:29 PM, Magnus Damm <magnus.damm@gmail.com> wrote:
> Now when both 32-bit and 64-bit code inside the driver is using
> fwspec it is possible to replace the utlb handling with fwspec ids
> that get populated from ->of_xlate().

Thanks for your patch!

> --- 0013/drivers/iommu/ipmmu-vmsa.c
> +++ work/drivers/iommu/ipmmu-vmsa.c     2017-06-15 18:32:27.580607110 +0900

>  static int ipmmu_of_xlate(struct device *dev,
>                           struct of_phandle_args *spec)
>  {
> -       return ipmmu_init_platform_device(dev);
> +       iommu_fwspec_add_ids(dev, spec->args, 1);

Does it hurt if iommu_fwspec_add_ids() is called multiple times, as this is
done before the "Initialize once - xlate() will call multiple times" check?

> +
> +       return ipmmu_init_platform_device(dev, spec);
>  }

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 04/04] iommu/ipmmu-vmsa: Replace local utlb code with fwspec ids

[Magnus Damm]

Hi Geert,

On Fri, Jun 16, 2017 at 4:18 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> Hi Magnus,
>
> On Thu, Jun 15, 2017 at 12:29 PM, Magnus Damm <magnus.damm@gmail.com> wrote:
>> Now when both 32-bit and 64-bit code inside the driver is using
>> fwspec it is possible to replace the utlb handling with fwspec ids
>> that get populated from ->of_xlate().
>
> Thanks for your patch!

Thanks for the feedback!

>> --- 0013/drivers/iommu/ipmmu-vmsa.c
>> +++ work/drivers/iommu/ipmmu-vmsa.c     2017-06-15 18:32:27.580607110 +0900
>
>>  static int ipmmu_of_xlate(struct device *dev,
>>                           struct of_phandle_args *spec)
>>  {
>> -       return ipmmu_init_platform_device(dev);
>> +       iommu_fwspec_add_ids(dev, spec->args, 1);
>
> Does it hurt if iommu_fwspec_add_ids() is called multiple times, as this is
> done before the "Initialize once - xlate() will call multiple times" check?

The function needs to be called several times to populate the ids, so
that the "initialize once" check happens later is intentional and
correct. Perhaps a bit unclear though...

>> +
>> +       return ipmmu_init_platform_device(dev, spec);
>>  }

Cheers,

/ magnus

Re: [PATCH 00/04] iommu/ipmmu-vmsa: 32-bit ARM update

[Robin Murphy]

Hi Magnus,

On 15/06/17 11:29, Magnus Damm wrote:
> iommu/ipmmu-vmsa: 32-bit ARM update
> 
> [PATCH 01/04] iommu/ipmmu-vmsa: Use iommu_device_register()/unregister()
> [PATCH 02/04] iommu/ipmmu-vmsa: Consistent ->of_xlate() handling
> [PATCH 03/04] iommu/ipmmu-vmsa: Use fwspec on both 32 and 64-bit ARM
> [PATCH 04/04] iommu/ipmmu-vmsa: Replace local utlb code with fwspec ids
> 
> This series updates the IPMMU driver to make use of recent IOMMU framework
> changes and also improves code sharing in the driver between the 32-bit and
> 64-bit dma-mapping architecture glue code.

Nice! I did try rebasing my original patch myself but it quickly got
very messy. I also had the below beforehand as preliminary cleanup for
getting rid of the ipmmu_vmsa_iommu_priv structure altogether (i.e. so
fwspec->priv can just hold the ipmmu_vmsa_device pointer directly) - if
it does actually work as intended, feel free to take it ;)

Thanks,
Robin.

> Suggested-by: Robin Murphy <robin.murphy@arm.com> (Patch 2 and 4)
> Signed-off-by: Robin Murphy <robin.murphy@arm.com> (Patch 3)
> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
> ---
> 
>  Developed on renesas-drivers-2017-06-13-v4.12-rc5 and rebased to next-20170614
> 
>  drivers/iommu/ipmmu-vmsa.c |  186 +++++++++++---------------------------------
>  1 file changed, 49 insertions(+), 137 deletions(-)
> 

---->8-----
From: Robin Murphy <robin.murphy@arm.com>
Subject: [PATCH] iommu/ipmmu-vmsa: Clean up group allocation

With the new IOMMU group support, we go through quite the merry dance
in order to find masters behind the same IPMMU instance, so that we can
ensure they are grouped together. None of which is really necessary,
since the master's private data already points to the particular IPMMU
it is associated with, and that IPMMU instance data is the perfect place
to keep track of a per-instance group.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/ipmmu-vmsa.c | 53
+++++++---------------------------------------
 1 file changed, 8 insertions(+), 45 deletions(-)

diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index 2a38aa15be17..9d75d7553e20 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -43,6 +43,7 @@ struct ipmmu_vmsa_device {
 	struct ipmmu_vmsa_domain *domains[IPMMU_CTX_MAX];

 	struct dma_iommu_mapping *mapping;
+	struct iommu_group *group;
 };

 struct ipmmu_vmsa_domain {
@@ -60,8 +61,6 @@ struct ipmmu_vmsa_iommu_priv {
 	struct ipmmu_vmsa_device *mmu;
 	unsigned int *utlbs;
 	unsigned int num_utlbs;
-	struct device *dev;
-	struct list_head list;
 };

 static DEFINE_SPINLOCK(ipmmu_devices_lock);
@@ -724,7 +723,6 @@ static int ipmmu_init_platform_device(struct device
*dev)
 	priv->mmu = mmu;
 	priv->utlbs = utlbs;
 	priv->num_utlbs = num_utlbs;
-	priv->dev = dev;
 	set_priv(dev, priv);
 	return 0;

@@ -854,9 +852,6 @@ static const struct iommu_ops ipmmu_ops = {

 #ifdef CONFIG_IOMMU_DMA

-static DEFINE_SPINLOCK(ipmmu_slave_devices_lock);
-static LIST_HEAD(ipmmu_slave_devices);
-
 static struct iommu_domain *ipmmu_domain_alloc_dma(unsigned type)
 {
 	struct iommu_domain *io_domain = NULL;
@@ -904,57 +899,25 @@ static int ipmmu_add_device_dma(struct device *dev)
 	if (IS_ERR(group))
 		return PTR_ERR(group);

-	spin_lock(&ipmmu_slave_devices_lock);
-	list_add(&to_priv(dev)->list, &ipmmu_slave_devices);
-	spin_unlock(&ipmmu_slave_devices_lock);
 	return 0;
 }

 static void ipmmu_remove_device_dma(struct device *dev)
 {
-	struct ipmmu_vmsa_iommu_priv *priv = to_priv(dev);
-
-	spin_lock(&ipmmu_slave_devices_lock);
-	list_del(&priv->list);
-	spin_unlock(&ipmmu_slave_devices_lock);
-
 	iommu_group_remove_device(dev);
 }

-static struct device *ipmmu_find_sibling_device(struct device *dev)
-{
-	struct ipmmu_vmsa_iommu_priv *priv = to_priv(dev);
-	struct ipmmu_vmsa_iommu_priv *sibling_priv = NULL;
-	bool found = false;
-
-	spin_lock(&ipmmu_slave_devices_lock);
-
-	list_for_each_entry(sibling_priv, &ipmmu_slave_devices, list) {
-		if (priv == sibling_priv)
-			continue;
-		if (sibling_priv->mmu == priv->mmu) {
-			found = true;
-			break;
-		}
-	}
-
-	spin_unlock(&ipmmu_slave_devices_lock);
-
-	return found ? sibling_priv->dev : NULL;
-}
-
 static struct iommu_group *ipmmu_find_group_dma(struct device *dev)
 {
-	struct iommu_group *group;
-	struct device *sibling;
+	struct ipmmu_vmsa_iommu_priv *priv = to_priv(dev);
+	struct ipmmu_vmsa_device *mmu = priv->mmu;

-	sibling = ipmmu_find_sibling_device(dev);
-	if (sibling)
-		group = iommu_group_get(sibling);
-	if (!sibling || IS_ERR(group))
-		group = generic_device_group(dev);
+	if (mmu->group)
+		return iommu_group_ref_get(mmu->group);

-	return group;
+	mmu->group = generic_device_group(dev);
+
+	return mmu->group;
 }

 static int ipmmu_of_xlate_dma(struct device *dev,