[PATCH v7 0/3] iommu/tegra-smmu: Add PCI support

[PATCH v7 0/3] iommu/tegra-smmu: Add PCI support

[Nicolin Chen]

This series is to add PCI support in tegra-smmu driver.

Changelog (Detail in each patch)
v6->v7
 * Added comments for put_device in PATCH-2
 * Renamed goto labels in PATCH-3
 * Kept Dmitry's Reviewed-by and Tested-by as no function change
v5->v6
 * Dropped a NULL check, per Dmitry's comments
 * Added Dmitry's Reviewed-by and Tested-by
v4->v5
 * PATCH-1 Cleaned two variables inits
 * PATCH-2 Fixed put() in ->of_xlate() and Updated commit message
 * PATCH-3 Added Dmitry's Reviewed-by
v3->v4
 * Dropped helper function, per Thierry's comments
 * Found another way to get smmu pointer
v2->v3
 * Replaced with devm_tegra_get_memory_controller
 * Updated changes by following Dmitry's comments
v1->v2
 * Added PATCH-1 suggested by Dmitry
 * Reworked PATCH-2 to unify certain code

Nicolin Chen (3):
  iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev
  iommu/tegra-smmu: Rework tegra_smmu_probe_device()
  iommu/tegra-smmu: Add PCI support

 drivers/iommu/tegra-smmu.c | 187 +++++++++++++------------------------
 1 file changed, 63 insertions(+), 124 deletions(-)

-- 
2.17.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v7 1/3] iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev

[Nicolin Chen]

In tegra_smmu_(de)attach_dev() functions, we poll DTB for each
client's iommus property to get swgroup ID in order to prepare
"as" and enable smmu. Actually tegra_smmu_configure() prepared
an fwspec for each client, and added to the fwspec all swgroup
IDs of client DT node in DTB.

So this patch uses fwspec in tegra_smmu_(de)attach_dev() so as
to replace the redundant DT polling code.

Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
Tested-by: Dmitry Osipenko <digetx@gmail.com>
Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
---

Changelog
v6->v7:
 * No change
v5->v6:
 * Added Dmitry's Reviewed-by and Tested-by
v4->v5:
 * Removed "index" and "err" assigning to 0
v3->v4:
 * Seperated the change, as a cleanup, from the rework patch
v1->v3:
 * N/A

 drivers/iommu/tegra-smmu.c | 56 ++++++++++++++++----------------------
 1 file changed, 23 insertions(+), 33 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 6a3ecc334481..297d49f3f80e 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -484,60 +484,50 @@ static void tegra_smmu_as_unprepare(struct tegra_smmu *smmu,
 static int tegra_smmu_attach_dev(struct iommu_domain *domain,
 				 struct device *dev)
 {
+	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
 	struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
 	struct tegra_smmu_as *as = to_smmu_as(domain);
-	struct device_node *np = dev->of_node;
-	struct of_phandle_args args;
-	unsigned int index = 0;
-	int err = 0;
-
-	while (!of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
-					   &args)) {
-		unsigned int swgroup = args.args[0];
-
-		if (args.np != smmu->dev->of_node) {
-			of_node_put(args.np);
-			continue;
-		}
+	unsigned int index;
+	int err;
 
-		of_node_put(args.np);
+	if (!fwspec)
+		return -ENOENT;
 
+	for (index = 0; index < fwspec->num_ids; index++) {
 		err = tegra_smmu_as_prepare(smmu, as);
-		if (err < 0)
-			return err;
+		if (err)
+			goto disable;
 
-		tegra_smmu_enable(smmu, swgroup, as->id);
-		index++;
+		tegra_smmu_enable(smmu, fwspec->ids[index], as->id);
 	}
 
 	if (index == 0)
 		return -ENODEV;
 
 	return 0;
+
+disable:
+	while (index--) {
+		tegra_smmu_disable(smmu, fwspec->ids[index], as->id);
+		tegra_smmu_as_unprepare(smmu, as);
+	}
+
+	return err;
 }
 
 static void tegra_smmu_detach_dev(struct iommu_domain *domain, struct device *dev)
 {
+	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
 	struct tegra_smmu_as *as = to_smmu_as(domain);
-	struct device_node *np = dev->of_node;
 	struct tegra_smmu *smmu = as->smmu;
-	struct of_phandle_args args;
-	unsigned int index = 0;
-
-	while (!of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
-					   &args)) {
-		unsigned int swgroup = args.args[0];
+	unsigned int index;
 
-		if (args.np != smmu->dev->of_node) {
-			of_node_put(args.np);
-			continue;
-		}
-
-		of_node_put(args.np);
+	if (!fwspec)
+		return;
 
-		tegra_smmu_disable(smmu, swgroup, as->id);
+	for (index = 0; index < fwspec->num_ids; index++) {
+		tegra_smmu_disable(smmu, fwspec->ids[index], as->id);
 		tegra_smmu_as_unprepare(smmu, as);
-		index++;
 	}
 }
 
-- 
2.17.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v7 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()

[Nicolin Chen]

The bus_set_iommu() in tegra_smmu_probe() enumerates all clients
to call in tegra_smmu_probe_device() where each client searches
its DT node for smmu pointer and swgroup ID, so as to configure
an fwspec. But this requires a valid smmu pointer even before mc
and smmu drivers are probed. So in tegra_smmu_probe() we added a
line of code to fill mc->smmu, marking "a bit of a hack".

This works for most of clients in the DTB, however, doesn't work
for a client that doesn't exist in DTB, a PCI device for example.

Actually, if we return ERR_PTR(-ENODEV) in ->probe_device() when
it's called from bus_set_iommu(), iommu core will let everything
carry on. Then when a client gets probed, of_iommu_configure() in
iommu core will search DTB for swgroup ID and call ->of_xlate()
to prepare an fwspec, similar to tegra_smmu_probe_device() and
tegra_smmu_configure(). Then it'll call tegra_smmu_probe_device()
again, and this time we shall return smmu->iommu pointer properly.

So we can get rid of tegra_smmu_find() and tegra_smmu_configure()
along with DT polling code by letting the iommu core handle every
thing, except a problem that we search iommus property in DTB not
only for swgroup ID but also for mc node to get mc->smmu pointer
to call dev_iommu_priv_set() and return the smmu->iommu pointer.
So we'll need to find another way to get smmu pointer.

Referencing the implementation of sun50i-iommu driver, of_xlate()
has client's dev pointer, mc node and swgroup ID. This means that
we can call dev_iommu_priv_set() in of_xlate() instead, so we can
simply get smmu pointer in ->probe_device().

This patch reworks tegra_smmu_probe_device() by:
1) Removing mc->smmu hack in tegra_smmu_probe() so as to return
   ERR_PTR(-ENODEV) in tegra_smmu_probe_device() during stage of
   tegra_smmu_probe/tegra_mc_probe().
2) Moving dev_iommu_priv_set() to of_xlate() so we can get smmu
   pointer in tegra_smmu_probe_device() to replace DTB polling.
3) Removing tegra_smmu_configure() accordingly since iommu core
   takes care of it.

This also fixes a problem that previously we could add clients to
iommu groups before iommu core initializes its default domain:
    ubuntu@jetson:~$ dmesg | grep iommu
    platform 50000000.host1x: Adding to iommu group 1
    platform 57000000.gpu: Adding to iommu group 2
    iommu: Default domain type: Translated
    platform 54200000.dc: Adding to iommu group 3
    platform 54240000.dc: Adding to iommu group 3
    platform 54340000.vic: Adding to iommu group 4

Though it works fine with IOMMU_DOMAIN_UNMANAGED, but will have
warnings if switching to IOMMU_DOMAIN_DMA:
    iommu: Failed to allocate default IOMMU domain of type 0 for
           group (null) - Falling back to IOMMU_DOMAIN_DMA
    iommu: Failed to allocate default IOMMU domain of type 0 for
           group (null) - Falling back to IOMMU_DOMAIN_DMA

Now, bypassing the first probe_device() call from bus_set_iommu()
fixes the sequence:
    ubuntu@jetson:~$ dmesg | grep iommu
    iommu: Default domain type: Translated
    tegra-host1x 50000000.host1x: Adding to iommu group 0
    tegra-dc 54200000.dc: Adding to iommu group 1
    tegra-dc 54240000.dc: Adding to iommu group 1
    tegra-vic 54340000.vic: Adding to iommu group 2
    nouveau 57000000.gpu: Adding to iommu group 3

Note that dmesg log above is testing with IOMMU_DOMAIN_UNMANAGED.

Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
Tested-by: Dmitry Osipenko <digetx@gmail.com>
Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
---

Changelog
v6->v7
 * Added comments for put_device(), suggested by Thierry.
v5->v6
 * Dropped NULL mc/smmu pointer check, suggested by Dmitry.
 * Updated commit message with test results with vanilla DTB.
 * Added Dmitry's Reviewed-by and Tested-by.
v4->v5
 * Replaced of_node_put() with put_device() in of_xlate().
 * Added test result in commit message.
v3->v4
 * Moved dev_iommu_priv_set() to of_xlate() so we don't need
   to poll DTB for smmu pointer.
 * Removed the hack in tegra_smmu_probe() by returning ERR_PTR(
   -ENODEV) in tegra_smmu_probe_device() to let iommu core call
   in again.
 * Removed tegra_smmu_find() and tegra_smmu_configure() as iommu
   core takes care of fwspec.
v2->v3
 * Used devm_tegra_get_memory_controller() to get mc pointer
 * Replaced IS_ERR_OR_NULL with IS_ERR in .probe_device()
v1->v2
 * Replaced in .probe_device() tegra_smmu_find/tegra_smmu_configure()
   with tegra_get_memory_controller call.
 * Dropped the hack in tegra_smmu_probe().

 drivers/iommu/tegra-smmu.c | 96 ++++++--------------------------------
 1 file changed, 15 insertions(+), 81 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 297d49f3f80e..be29f5977145 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -797,75 +797,9 @@ static phys_addr_t tegra_smmu_iova_to_phys(struct iommu_domain *domain,
 	return SMMU_PFN_PHYS(pfn) + SMMU_OFFSET_IN_PAGE(iova);
 }
 
-static struct tegra_smmu *tegra_smmu_find(struct device_node *np)
-{
-	struct platform_device *pdev;
-	struct tegra_mc *mc;
-
-	pdev = of_find_device_by_node(np);
-	if (!pdev)
-		return NULL;
-
-	mc = platform_get_drvdata(pdev);
-	if (!mc)
-		return NULL;
-
-	return mc->smmu;
-}
-
-static int tegra_smmu_configure(struct tegra_smmu *smmu, struct device *dev,
-				struct of_phandle_args *args)
-{
-	const struct iommu_ops *ops = smmu->iommu.ops;
-	int err;
-
-	err = iommu_fwspec_init(dev, &dev->of_node->fwnode, ops);
-	if (err < 0) {
-		dev_err(dev, "failed to initialize fwspec: %d\n", err);
-		return err;
-	}
-
-	err = ops->of_xlate(dev, args);
-	if (err < 0) {
-		dev_err(dev, "failed to parse SW group ID: %d\n", err);
-		iommu_fwspec_free(dev);
-		return err;
-	}
-
-	return 0;
-}
-
 static struct iommu_device *tegra_smmu_probe_device(struct device *dev)
 {
-	struct device_node *np = dev->of_node;
-	struct tegra_smmu *smmu = NULL;
-	struct of_phandle_args args;
-	unsigned int index = 0;
-	int err;
-
-	while (of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
-					  &args) == 0) {
-		smmu = tegra_smmu_find(args.np);
-		if (smmu) {
-			err = tegra_smmu_configure(smmu, dev, &args);
-			of_node_put(args.np);
-
-			if (err < 0)
-				return ERR_PTR(err);
-
-			/*
-			 * Only a single IOMMU master interface is currently
-			 * supported by the Linux kernel, so abort after the
-			 * first match.
-			 */
-			dev_iommu_priv_set(dev, smmu);
-
-			break;
-		}
-
-		of_node_put(args.np);
-		index++;
-	}
+	struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
 
 	if (!smmu)
 		return ERR_PTR(-ENODEV);
@@ -873,10 +807,7 @@ static struct iommu_device *tegra_smmu_probe_device(struct device *dev)
 	return &smmu->iommu;
 }
 
-static void tegra_smmu_release_device(struct device *dev)
-{
-	dev_iommu_priv_set(dev, NULL);
-}
+static void tegra_smmu_release_device(struct device *dev) {}
 
 static const struct tegra_smmu_group_soc *
 tegra_smmu_find_group(struct tegra_smmu *smmu, unsigned int swgroup)
@@ -953,8 +884,21 @@ static struct iommu_group *tegra_smmu_device_group(struct device *dev)
 static int tegra_smmu_of_xlate(struct device *dev,
 			       struct of_phandle_args *args)
 {
+	struct platform_device *iommu_pdev = of_find_device_by_node(args->np);
+	struct tegra_mc *mc = platform_get_drvdata(iommu_pdev);
 	u32 id = args->args[0];
 
+	/*
+	 * Note: we are here releasing the reference of &iommu_pdev->dev, which
+	 * is mc->dev. Although some functions in tegra_smmu_ops may keep using
+	 * its private data beyond this point, it's still safe to do so because
+	 * the SMMU parent device is the same as the MC, so the reference count
+	 * isn't strictly necessary.
+	 */
+	put_device(&iommu_pdev->dev);
+
+	dev_iommu_priv_set(dev, mc->smmu);
+
 	return iommu_fwspec_add_ids(dev, &id, 1);
 }
 
@@ -1079,16 +1023,6 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev,
 	if (!smmu)
 		return ERR_PTR(-ENOMEM);
 
-	/*
-	 * This is a bit of a hack. Ideally we'd want to simply return this
-	 * value. However the IOMMU registration process will attempt to add
-	 * all devices to the IOMMU when bus_set_iommu() is called. In order
-	 * not to rely on global variables to track the IOMMU instance, we
-	 * set it here so that it can be looked up from the .probe_device()
-	 * callback via the IOMMU device's .drvdata field.
-	 */
-	mc->smmu = smmu;
-
 	size = BITS_TO_LONGS(soc->num_asids) * sizeof(long);
 
 	smmu->asids = devm_kzalloc(dev, size, GFP_KERNEL);
-- 
2.17.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v7 3/3] iommu/tegra-smmu: Add PCI support

[Nicolin Chen]

This patch simply adds support for PCI devices.

Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
Tested-by: Dmitry Osipenko <digetx@gmail.com>
Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
---

Changelog
v6->v7
 * Renamed goto labels, suggested by Thierry.
v5->v6
 * Added Dmitry's Reviewed-by and Tested-by.
v4->v5
 * Added Dmitry's Reviewed-by
v3->v4
 * Dropped !iommu_present() check
 * Added CONFIG_PCI check in the exit path
v2->v3
 * Replaced ternary conditional operator with if-else in .device_group()
 * Dropped change in tegra_smmu_remove()
v1->v2
 * Added error-out labels in tegra_smmu_probe()
 * Dropped pci_request_acs() since IOMMU core would call it.

 drivers/iommu/tegra-smmu.c | 35 +++++++++++++++++++++++++----------
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index be29f5977145..2941d6459076 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -10,6 +10,7 @@
 #include <linux/kernel.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/pci.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
@@ -865,7 +866,11 @@ static struct iommu_group *tegra_smmu_device_group(struct device *dev)
 	group->smmu = smmu;
 	group->soc = soc;
 
-	group->group = iommu_group_alloc();
+	if (dev_is_pci(dev))
+		group->group = pci_device_group(dev);
+	else
+		group->group = generic_device_group(dev);
+
 	if (IS_ERR(group->group)) {
 		devm_kfree(smmu->dev, group);
 		mutex_unlock(&smmu->lock);
@@ -1075,22 +1080,32 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev,
 	iommu_device_set_fwnode(&smmu->iommu, dev->fwnode);
 
 	err = iommu_device_register(&smmu->iommu);
-	if (err) {
-		iommu_device_sysfs_remove(&smmu->iommu);
-		return ERR_PTR(err);
-	}
+	if (err)
+		goto remove_sysfs;
 
 	err = bus_set_iommu(&platform_bus_type, &tegra_smmu_ops);
-	if (err < 0) {
-		iommu_device_unregister(&smmu->iommu);
-		iommu_device_sysfs_remove(&smmu->iommu);
-		return ERR_PTR(err);
-	}
+	if (err < 0)
+		goto unregister;
+
+#ifdef CONFIG_PCI
+	err = bus_set_iommu(&pci_bus_type, &tegra_smmu_ops);
+	if (err < 0)
+		goto unset_platform_bus;
+#endif
 
 	if (IS_ENABLED(CONFIG_DEBUG_FS))
 		tegra_smmu_debugfs_init(smmu);
 
 	return smmu;
+
+unset_platform_bus: __maybe_unused;
+	bus_set_iommu(&platform_bus_type, NULL);
+unregister:
+	iommu_device_unregister(&smmu->iommu);
+remove_sysfs:
+	iommu_device_sysfs_remove(&smmu->iommu);
+
+	return ERR_PTR(err);
 }
 
 void tegra_smmu_remove(struct tegra_smmu *smmu)
-- 
2.17.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v7 3/3] iommu/tegra-smmu: Add PCI support

[Robin Murphy]

On 2020-10-09 17:19, Nicolin Chen wrote:
> This patch simply adds support for PCI devices.
> 
> Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
> Tested-by: Dmitry Osipenko <digetx@gmail.com>
> Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
> ---
> 
> Changelog
> v6->v7
>   * Renamed goto labels, suggested by Thierry.
> v5->v6
>   * Added Dmitry's Reviewed-by and Tested-by.
> v4->v5
>   * Added Dmitry's Reviewed-by
> v3->v4
>   * Dropped !iommu_present() check
>   * Added CONFIG_PCI check in the exit path
> v2->v3
>   * Replaced ternary conditional operator with if-else in .device_group()
>   * Dropped change in tegra_smmu_remove()
> v1->v2
>   * Added error-out labels in tegra_smmu_probe()
>   * Dropped pci_request_acs() since IOMMU core would call it.
> 
>   drivers/iommu/tegra-smmu.c | 35 +++++++++++++++++++++++++----------
>   1 file changed, 25 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> index be29f5977145..2941d6459076 100644
> --- a/drivers/iommu/tegra-smmu.c
> +++ b/drivers/iommu/tegra-smmu.c
> @@ -10,6 +10,7 @@
>   #include <linux/kernel.h>
>   #include <linux/of.h>
>   #include <linux/of_device.h>
> +#include <linux/pci.h>
>   #include <linux/platform_device.h>
>   #include <linux/slab.h>
>   #include <linux/spinlock.h>
> @@ -865,7 +866,11 @@ static struct iommu_group *tegra_smmu_device_group(struct device *dev)
>   	group->smmu = smmu;
>   	group->soc = soc;
>   
> -	group->group = iommu_group_alloc();
> +	if (dev_is_pci(dev))
> +		group->group = pci_device_group(dev);

Just to check, is it OK to have two or more swgroups "owning" the same 
iommu_group if an existing one gets returned here? It looks like that 
might not play nice with the use of iommu_group_set_iommudata().

Robin.

> +	else
> +		group->group = generic_device_group(dev);
> +
>   	if (IS_ERR(group->group)) {
>   		devm_kfree(smmu->dev, group);
>   		mutex_unlock(&smmu->lock);
> @@ -1075,22 +1080,32 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev,
>   	iommu_device_set_fwnode(&smmu->iommu, dev->fwnode);
>   
>   	err = iommu_device_register(&smmu->iommu);
> -	if (err) {
> -		iommu_device_sysfs_remove(&smmu->iommu);
> -		return ERR_PTR(err);
> -	}
> +	if (err)
> +		goto remove_sysfs;
>   
>   	err = bus_set_iommu(&platform_bus_type, &tegra_smmu_ops);
> -	if (err < 0) {
> -		iommu_device_unregister(&smmu->iommu);
> -		iommu_device_sysfs_remove(&smmu->iommu);
> -		return ERR_PTR(err);
> -	}
> +	if (err < 0)
> +		goto unregister;
> +
> +#ifdef CONFIG_PCI
> +	err = bus_set_iommu(&pci_bus_type, &tegra_smmu_ops);
> +	if (err < 0)
> +		goto unset_platform_bus;
> +#endif
>   
>   	if (IS_ENABLED(CONFIG_DEBUG_FS))
>   		tegra_smmu_debugfs_init(smmu);
>   
>   	return smmu;
> +
> +unset_platform_bus: __maybe_unused;
> +	bus_set_iommu(&platform_bus_type, NULL);
> +unregister:
> +	iommu_device_unregister(&smmu->iommu);
> +remove_sysfs:
> +	iommu_device_sysfs_remove(&smmu->iommu);
> +
> +	return ERR_PTR(err);
>   }
>   
>   void tegra_smmu_remove(struct tegra_smmu *smmu)
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v7 3/3] iommu/tegra-smmu: Add PCI support

[Nicolin Chen]

On Wed, Oct 14, 2020 at 06:42:36PM +0100, Robin Murphy wrote:
> On 2020-10-09 17:19, Nicolin Chen wrote:
> > This patch simply adds support for PCI devices.
> > 
> > Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
> > Tested-by: Dmitry Osipenko <digetx@gmail.com>
> > Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
> > ---
> > 
> > Changelog
> > v6->v7
> >   * Renamed goto labels, suggested by Thierry.
> > v5->v6
> >   * Added Dmitry's Reviewed-by and Tested-by.
> > v4->v5
> >   * Added Dmitry's Reviewed-by
> > v3->v4
> >   * Dropped !iommu_present() check
> >   * Added CONFIG_PCI check in the exit path
> > v2->v3
> >   * Replaced ternary conditional operator with if-else in .device_group()
> >   * Dropped change in tegra_smmu_remove()
> > v1->v2
> >   * Added error-out labels in tegra_smmu_probe()
> >   * Dropped pci_request_acs() since IOMMU core would call it.
> > 
> >   drivers/iommu/tegra-smmu.c | 35 +++++++++++++++++++++++++----------
> >   1 file changed, 25 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> > index be29f5977145..2941d6459076 100644
> > --- a/drivers/iommu/tegra-smmu.c
> > +++ b/drivers/iommu/tegra-smmu.c
> > @@ -10,6 +10,7 @@
> >   #include <linux/kernel.h>
> >   #include <linux/of.h>
> >   #include <linux/of_device.h>
> > +#include <linux/pci.h>
> >   #include <linux/platform_device.h>
> >   #include <linux/slab.h>
> >   #include <linux/spinlock.h>
> > @@ -865,7 +866,11 @@ static struct iommu_group *tegra_smmu_device_group(struct device *dev)
> >   	group->smmu = smmu;
> >   	group->soc = soc;
> > -	group->group = iommu_group_alloc();
> > +	if (dev_is_pci(dev))
> > +		group->group = pci_device_group(dev);
> 
> Just to check, is it OK to have two or more swgroups "owning" the same
> iommu_group if an existing one gets returned here? It looks like that might
> not play nice with the use of iommu_group_set_iommudata().

Do you mean by "gets returned here" the "IS_ERR" check below?

> Robin.
> 
> > +	else
> > +		group->group = generic_device_group(dev);
> > +
> >   	if (IS_ERR(group->group)) {
> >   		devm_kfree(smmu->dev, group);
> >   		mutex_unlock(&smmu->lock);
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v7 3/3] iommu/tegra-smmu: Add PCI support

[Robin Murphy]

On 2020-10-15 05:13, Nicolin Chen wrote:
> On Wed, Oct 14, 2020 at 06:42:36PM +0100, Robin Murphy wrote:
>> On 2020-10-09 17:19, Nicolin Chen wrote:
>>> This patch simply adds support for PCI devices.
>>>
>>> Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
>>> Tested-by: Dmitry Osipenko <digetx@gmail.com>
>>> Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
>>> ---
>>>
>>> Changelog
>>> v6->v7
>>>    * Renamed goto labels, suggested by Thierry.
>>> v5->v6
>>>    * Added Dmitry's Reviewed-by and Tested-by.
>>> v4->v5
>>>    * Added Dmitry's Reviewed-by
>>> v3->v4
>>>    * Dropped !iommu_present() check
>>>    * Added CONFIG_PCI check in the exit path
>>> v2->v3
>>>    * Replaced ternary conditional operator with if-else in .device_group()
>>>    * Dropped change in tegra_smmu_remove()
>>> v1->v2
>>>    * Added error-out labels in tegra_smmu_probe()
>>>    * Dropped pci_request_acs() since IOMMU core would call it.
>>>
>>>    drivers/iommu/tegra-smmu.c | 35 +++++++++++++++++++++++++----------
>>>    1 file changed, 25 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
>>> index be29f5977145..2941d6459076 100644
>>> --- a/drivers/iommu/tegra-smmu.c
>>> +++ b/drivers/iommu/tegra-smmu.c
>>> @@ -10,6 +10,7 @@
>>>    #include <linux/kernel.h>
>>>    #include <linux/of.h>
>>>    #include <linux/of_device.h>
>>> +#include <linux/pci.h>
>>>    #include <linux/platform_device.h>
>>>    #include <linux/slab.h>
>>>    #include <linux/spinlock.h>
>>> @@ -865,7 +866,11 @@ static struct iommu_group *tegra_smmu_device_group(struct device *dev)
>>>    	group->smmu = smmu;
>>>    	group->soc = soc;
>>> -	group->group = iommu_group_alloc();
>>> +	if (dev_is_pci(dev))
>>> +		group->group = pci_device_group(dev);
>>
>> Just to check, is it OK to have two or more swgroups "owning" the same
>> iommu_group if an existing one gets returned here? It looks like that might
>> not play nice with the use of iommu_group_set_iommudata().
> 
> Do you mean by "gets returned here" the "IS_ERR" check below?

I mean that unlike iommu_group_alloc()/generic_device_group(), 
pci_device_group() may give you back a group that already contains 
another device and has already been set up from that device's 
perspective. This can happen for topological reasons like requester ID 
aliasing through a PCI-PCIe bridge or lack of isolation between functions.

Robin.

>>> +	else
>>> +		group->group = generic_device_group(dev);
>>> +
>>>    	if (IS_ERR(group->group)) {
>>>    		devm_kfree(smmu->dev, group);
>>>    		mutex_unlock(&smmu->lock);
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v7 3/3] iommu/tegra-smmu: Add PCI support

[Nicolin Chen]

On Thu, Oct 15, 2020 at 10:55:52AM +0100, Robin Murphy wrote:
> On 2020-10-15 05:13, Nicolin Chen wrote:
> > On Wed, Oct 14, 2020 at 06:42:36PM +0100, Robin Murphy wrote:
> > > On 2020-10-09 17:19, Nicolin Chen wrote:
> > > > This patch simply adds support for PCI devices.
> > > > 
> > > > Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
> > > > Tested-by: Dmitry Osipenko <digetx@gmail.com>
> > > > Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
> > > > ---
> > > > 
> > > > Changelog
> > > > v6->v7
> > > >    * Renamed goto labels, suggested by Thierry.
> > > > v5->v6
> > > >    * Added Dmitry's Reviewed-by and Tested-by.
> > > > v4->v5
> > > >    * Added Dmitry's Reviewed-by
> > > > v3->v4
> > > >    * Dropped !iommu_present() check
> > > >    * Added CONFIG_PCI check in the exit path
> > > > v2->v3
> > > >    * Replaced ternary conditional operator with if-else in .device_group()
> > > >    * Dropped change in tegra_smmu_remove()
> > > > v1->v2
> > > >    * Added error-out labels in tegra_smmu_probe()
> > > >    * Dropped pci_request_acs() since IOMMU core would call it.
> > > > 
> > > >    drivers/iommu/tegra-smmu.c | 35 +++++++++++++++++++++++++----------
> > > >    1 file changed, 25 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> > > > index be29f5977145..2941d6459076 100644
> > > > --- a/drivers/iommu/tegra-smmu.c
> > > > +++ b/drivers/iommu/tegra-smmu.c
> > > > @@ -10,6 +10,7 @@
> > > >    #include <linux/kernel.h>
> > > >    #include <linux/of.h>
> > > >    #include <linux/of_device.h>
> > > > +#include <linux/pci.h>
> > > >    #include <linux/platform_device.h>
> > > >    #include <linux/slab.h>
> > > >    #include <linux/spinlock.h>
> > > > @@ -865,7 +866,11 @@ static struct iommu_group *tegra_smmu_device_group(struct device *dev)
> > > >    	group->smmu = smmu;
> > > >    	group->soc = soc;
> > > > -	group->group = iommu_group_alloc();
> > > > +	if (dev_is_pci(dev))
> > > > +		group->group = pci_device_group(dev);
> > > 
> > > Just to check, is it OK to have two or more swgroups "owning" the same
> > > iommu_group if an existing one gets returned here? It looks like that might
> > > not play nice with the use of iommu_group_set_iommudata().
> > 
> > Do you mean by "gets returned here" the "IS_ERR" check below?
> 
> I mean that unlike iommu_group_alloc()/generic_device_group(),
> pci_device_group() may give you back a group that already contains another
> device and has already been set up from that device's perspective. This can
> happen for topological reasons like requester ID aliasing through a PCI-PCIe
> bridge or lack of isolation between functions.

Okay..but we don't really have two swgroups owning the same groups
in case of PCI devices. For Tegra210, all PCI devices inherit the
same swgroup from the PCI controller. And I'd think previous chips
do the same. The only use case currently of 2+ swgroups owning the
same iommu_group is for display controller.

Or do you suggest we need an additional check for pci_device_group?
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v7 3/3] iommu/tegra-smmu: Add PCI support

[Robin Murphy]

On 2020-10-16 04:53, Nicolin Chen wrote:
> On Thu, Oct 15, 2020 at 10:55:52AM +0100, Robin Murphy wrote:
>> On 2020-10-15 05:13, Nicolin Chen wrote:
>>> On Wed, Oct 14, 2020 at 06:42:36PM +0100, Robin Murphy wrote:
>>>> On 2020-10-09 17:19, Nicolin Chen wrote:
>>>>> This patch simply adds support for PCI devices.
>>>>>
>>>>> Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
>>>>> Tested-by: Dmitry Osipenko <digetx@gmail.com>
>>>>> Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
>>>>> ---
>>>>>
>>>>> Changelog
>>>>> v6->v7
>>>>>     * Renamed goto labels, suggested by Thierry.
>>>>> v5->v6
>>>>>     * Added Dmitry's Reviewed-by and Tested-by.
>>>>> v4->v5
>>>>>     * Added Dmitry's Reviewed-by
>>>>> v3->v4
>>>>>     * Dropped !iommu_present() check
>>>>>     * Added CONFIG_PCI check in the exit path
>>>>> v2->v3
>>>>>     * Replaced ternary conditional operator with if-else in .device_group()
>>>>>     * Dropped change in tegra_smmu_remove()
>>>>> v1->v2
>>>>>     * Added error-out labels in tegra_smmu_probe()
>>>>>     * Dropped pci_request_acs() since IOMMU core would call it.
>>>>>
>>>>>     drivers/iommu/tegra-smmu.c | 35 +++++++++++++++++++++++++----------
>>>>>     1 file changed, 25 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
>>>>> index be29f5977145..2941d6459076 100644
>>>>> --- a/drivers/iommu/tegra-smmu.c
>>>>> +++ b/drivers/iommu/tegra-smmu.c
>>>>> @@ -10,6 +10,7 @@
>>>>>     #include <linux/kernel.h>
>>>>>     #include <linux/of.h>
>>>>>     #include <linux/of_device.h>
>>>>> +#include <linux/pci.h>
>>>>>     #include <linux/platform_device.h>
>>>>>     #include <linux/slab.h>
>>>>>     #include <linux/spinlock.h>
>>>>> @@ -865,7 +866,11 @@ static struct iommu_group *tegra_smmu_device_group(struct device *dev)
>>>>>     	group->smmu = smmu;
>>>>>     	group->soc = soc;
>>>>> -	group->group = iommu_group_alloc();
>>>>> +	if (dev_is_pci(dev))
>>>>> +		group->group = pci_device_group(dev);
>>>>
>>>> Just to check, is it OK to have two or more swgroups "owning" the same
>>>> iommu_group if an existing one gets returned here? It looks like that might
>>>> not play nice with the use of iommu_group_set_iommudata().
>>>
>>> Do you mean by "gets returned here" the "IS_ERR" check below?
>>
>> I mean that unlike iommu_group_alloc()/generic_device_group(),
>> pci_device_group() may give you back a group that already contains another
>> device and has already been set up from that device's perspective. This can
>> happen for topological reasons like requester ID aliasing through a PCI-PCIe
>> bridge or lack of isolation between functions.
> 
> Okay..but we don't really have two swgroups owning the same groups
> in case of PCI devices. For Tegra210, all PCI devices inherit the
> same swgroup from the PCI controller. And I'd think previous chips
> do the same. The only use case currently of 2+ swgroups owning the
> same iommu_group is for display controller.
> 
> Or do you suggest we need an additional check for pci_device_group?

Ah, OK - I still don't have the best comprehension of what exactly 
swgroups are, and the path through .of_xlate looked like you might be 
using the PCI requester ID as the swgroup identifier, but I guess that 
ultimately depends on what your "iommu-map" is supposed to look like. If 
pci_device_group() will effectively only ever get called once regardless 
of how many endpoints exist, then indeed this won't be a concern 
(although if that's *guaranteed* to be the case then you may as well 
just stick with calling iommu_group_alloc() directly). Thanks for 
clarifying.

Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v7 3/3] iommu/tegra-smmu: Add PCI support

[Nicolin Chen]

On Fri, Oct 16, 2020 at 03:10:26PM +0100, Robin Murphy wrote:
> On 2020-10-16 04:53, Nicolin Chen wrote:
> > On Thu, Oct 15, 2020 at 10:55:52AM +0100, Robin Murphy wrote:
> > > On 2020-10-15 05:13, Nicolin Chen wrote:
> > > > On Wed, Oct 14, 2020 at 06:42:36PM +0100, Robin Murphy wrote:
> > > > > On 2020-10-09 17:19, Nicolin Chen wrote:
> > > > > > This patch simply adds support for PCI devices.
> > > > > > 
> > > > > > Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
> > > > > > Tested-by: Dmitry Osipenko <digetx@gmail.com>
> > > > > > Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
> > > > > > ---
> > > > > > 
> > > > > > Changelog
> > > > > > v6->v7
> > > > > >     * Renamed goto labels, suggested by Thierry.
> > > > > > v5->v6
> > > > > >     * Added Dmitry's Reviewed-by and Tested-by.
> > > > > > v4->v5
> > > > > >     * Added Dmitry's Reviewed-by
> > > > > > v3->v4
> > > > > >     * Dropped !iommu_present() check
> > > > > >     * Added CONFIG_PCI check in the exit path
> > > > > > v2->v3
> > > > > >     * Replaced ternary conditional operator with if-else in .device_group()
> > > > > >     * Dropped change in tegra_smmu_remove()
> > > > > > v1->v2
> > > > > >     * Added error-out labels in tegra_smmu_probe()
> > > > > >     * Dropped pci_request_acs() since IOMMU core would call it.
> > > > > > 
> > > > > >     drivers/iommu/tegra-smmu.c | 35 +++++++++++++++++++++++++----------
> > > > > >     1 file changed, 25 insertions(+), 10 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> > > > > > index be29f5977145..2941d6459076 100644
> > > > > > --- a/drivers/iommu/tegra-smmu.c
> > > > > > +++ b/drivers/iommu/tegra-smmu.c
> > > > > > @@ -10,6 +10,7 @@
> > > > > >     #include <linux/kernel.h>
> > > > > >     #include <linux/of.h>
> > > > > >     #include <linux/of_device.h>
> > > > > > +#include <linux/pci.h>
> > > > > >     #include <linux/platform_device.h>
> > > > > >     #include <linux/slab.h>
> > > > > >     #include <linux/spinlock.h>
> > > > > > @@ -865,7 +866,11 @@ static struct iommu_group *tegra_smmu_device_group(struct device *dev)
> > > > > >     	group->smmu = smmu;
> > > > > >     	group->soc = soc;
> > > > > > -	group->group = iommu_group_alloc();
> > > > > > +	if (dev_is_pci(dev))
> > > > > > +		group->group = pci_device_group(dev);
> > > > > 
> > > > > Just to check, is it OK to have two or more swgroups "owning" the same
> > > > > iommu_group if an existing one gets returned here? It looks like that might
> > > > > not play nice with the use of iommu_group_set_iommudata().
> > > > 
> > > > Do you mean by "gets returned here" the "IS_ERR" check below?
> > > 
> > > I mean that unlike iommu_group_alloc()/generic_device_group(),
> > > pci_device_group() may give you back a group that already contains another
> > > device and has already been set up from that device's perspective. This can
> > > happen for topological reasons like requester ID aliasing through a PCI-PCIe
> > > bridge or lack of isolation between functions.
> > 
> > Okay..but we don't really have two swgroups owning the same groups
> > in case of PCI devices. For Tegra210, all PCI devices inherit the
> > same swgroup from the PCI controller. And I'd think previous chips
> > do the same. The only use case currently of 2+ swgroups owning the
> > same iommu_group is for display controller.
> > 
> > Or do you suggest we need an additional check for pci_device_group?
> 
> Ah, OK - I still don't have the best comprehension of what exactly swgroups

The "swgroup" stands for "software group", which associates with
an ASID (Address Space Identifier) for SMMU to determine whether
this client is going through SMMU translation or not.

> are, and the path through .of_xlate looked like you might be using the PCI
> requester ID as the swgroup identifier, but I guess that ultimately depends
> on what your "iommu-map" is supposed to look like. If pci_device_group()

This is copied from pcie node in our downstream dtsi file:

		iommus = <&mc TEGRA_SWGROUP_AFI>;
		iommu-map = <0x0 &mc TEGRA_SWGROUP_AFI 0x1000>;
		iommu-map-mask = <0x0>;

> will effectively only ever get called once regardless of how many endpoints
> exist, then indeed this won't be a concern (although if that's *guaranteed*
> to be the case then you may as well just stick with calling
> iommu_group_alloc() directly). Thanks for clarifying.

All PCI devices are supposed to get this swgroup of the pcie node
above. So the function will return the existing group of the pcie
controller, before giving a chance to call iommu_group_alloc().

Thanks for the review and input.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v7 3/3] iommu/tegra-smmu: Add PCI support

[Robin Murphy]

On 2020-10-17 02:56, Nicolin Chen wrote:
> On Fri, Oct 16, 2020 at 03:10:26PM +0100, Robin Murphy wrote:
>> On 2020-10-16 04:53, Nicolin Chen wrote:
>>> On Thu, Oct 15, 2020 at 10:55:52AM +0100, Robin Murphy wrote:
>>>> On 2020-10-15 05:13, Nicolin Chen wrote:
>>>>> On Wed, Oct 14, 2020 at 06:42:36PM +0100, Robin Murphy wrote:
>>>>>> On 2020-10-09 17:19, Nicolin Chen wrote:
>>>>>>> This patch simply adds support for PCI devices.
>>>>>>>
>>>>>>> Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
>>>>>>> Tested-by: Dmitry Osipenko <digetx@gmail.com>
>>>>>>> Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
>>>>>>> ---
>>>>>>>
>>>>>>> Changelog
>>>>>>> v6->v7
>>>>>>>      * Renamed goto labels, suggested by Thierry.
>>>>>>> v5->v6
>>>>>>>      * Added Dmitry's Reviewed-by and Tested-by.
>>>>>>> v4->v5
>>>>>>>      * Added Dmitry's Reviewed-by
>>>>>>> v3->v4
>>>>>>>      * Dropped !iommu_present() check
>>>>>>>      * Added CONFIG_PCI check in the exit path
>>>>>>> v2->v3
>>>>>>>      * Replaced ternary conditional operator with if-else in .device_group()
>>>>>>>      * Dropped change in tegra_smmu_remove()
>>>>>>> v1->v2
>>>>>>>      * Added error-out labels in tegra_smmu_probe()
>>>>>>>      * Dropped pci_request_acs() since IOMMU core would call it.
>>>>>>>
>>>>>>>      drivers/iommu/tegra-smmu.c | 35 +++++++++++++++++++++++++----------
>>>>>>>      1 file changed, 25 insertions(+), 10 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
>>>>>>> index be29f5977145..2941d6459076 100644
>>>>>>> --- a/drivers/iommu/tegra-smmu.c
>>>>>>> +++ b/drivers/iommu/tegra-smmu.c
>>>>>>> @@ -10,6 +10,7 @@
>>>>>>>      #include <linux/kernel.h>
>>>>>>>      #include <linux/of.h>
>>>>>>>      #include <linux/of_device.h>
>>>>>>> +#include <linux/pci.h>
>>>>>>>      #include <linux/platform_device.h>
>>>>>>>      #include <linux/slab.h>
>>>>>>>      #include <linux/spinlock.h>
>>>>>>> @@ -865,7 +866,11 @@ static struct iommu_group *tegra_smmu_device_group(struct device *dev)
>>>>>>>      	group->smmu = smmu;
>>>>>>>      	group->soc = soc;
>>>>>>> -	group->group = iommu_group_alloc();
>>>>>>> +	if (dev_is_pci(dev))
>>>>>>> +		group->group = pci_device_group(dev);
>>>>>>
>>>>>> Just to check, is it OK to have two or more swgroups "owning" the same
>>>>>> iommu_group if an existing one gets returned here? It looks like that might
>>>>>> not play nice with the use of iommu_group_set_iommudata().
>>>>>
>>>>> Do you mean by "gets returned here" the "IS_ERR" check below?
>>>>
>>>> I mean that unlike iommu_group_alloc()/generic_device_group(),
>>>> pci_device_group() may give you back a group that already contains another
>>>> device and has already been set up from that device's perspective. This can
>>>> happen for topological reasons like requester ID aliasing through a PCI-PCIe
>>>> bridge or lack of isolation between functions.
>>>
>>> Okay..but we don't really have two swgroups owning the same groups
>>> in case of PCI devices. For Tegra210, all PCI devices inherit the
>>> same swgroup from the PCI controller. And I'd think previous chips
>>> do the same. The only use case currently of 2+ swgroups owning the
>>> same iommu_group is for display controller.
>>>
>>> Or do you suggest we need an additional check for pci_device_group?
>>
>> Ah, OK - I still don't have the best comprehension of what exactly swgroups
> 
> The "swgroup" stands for "software group", which associates with
> an ASID (Address Space Identifier) for SMMU to determine whether
> this client is going through SMMU translation or not.

So in Arm SMMU analogy terms it's more like a context bank index than a 
stream ID - got it.

>> are, and the path through .of_xlate looked like you might be using the PCI
>> requester ID as the swgroup identifier, but I guess that ultimately depends
>> on what your "iommu-map" is supposed to look like. If pci_device_group()
> 
> This is copied from pcie node in our downstream dtsi file:
> 
> 		iommus = <&mc TEGRA_SWGROUP_AFI>;
> 		iommu-map = <0x0 &mc TEGRA_SWGROUP_AFI 0x1000>;
> 		iommu-map-mask = <0x0>;

Aha, indeed that iommu-map-mask is the trick :)

>> will effectively only ever get called once regardless of how many endpoints
>> exist, then indeed this won't be a concern (although if that's *guaranteed*
>> to be the case then you may as well just stick with calling
>> iommu_group_alloc() directly). Thanks for clarifying.
> 
> All PCI devices are supposed to get this swgroup of the pcie node
> above. So the function will return the existing group of the pcie
> controller, before giving a chance to call iommu_group_alloc().

Yes, the "iommus" property will mean that the group always gets created 
first for the platform device owning the host bridge, and thus won't be 
visible to pci_device_group() anyway. Subsequent tegra_smmu_group_get() 
calls for the PCI devices (including the PCI side of the host bridge 
itself) are then going to match TEGRA_SWGROUP_AFI in the smmu->groups 
list lookup and return early, so the dev_is_pci() condition will never 
be true, and the call to pci_device_group() is in fact entirely dead code.

(I was assuming a case where you didn't have the "iommus" property, in 
which case you would reach that path exactly once for the first PCI 
device probed, wherein pci_device_group() is still only going to fall 
through to calling iommu_group_alloc() anyway).

Cheers,
Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v7 0/3] iommu/tegra-smmu: Add PCI support

[Nicolin Chen]

On Fri, Oct 09, 2020 at 09:19:33AM -0700, Nicolin Chen wrote:
> This series is to add PCI support in tegra-smmu driver.

This v7 has fixed all the existing comments and been in the
maillist for nearly a month. Can anyone please apply them?

Thanks
Nic

> Changelog (Detail in each patch)
> v6->v7
>  * Added comments for put_device in PATCH-2
>  * Renamed goto labels in PATCH-3
>  * Kept Dmitry's Reviewed-by and Tested-by as no function change
> v5->v6
>  * Dropped a NULL check, per Dmitry's comments
>  * Added Dmitry's Reviewed-by and Tested-by
> v4->v5
>  * PATCH-1 Cleaned two variables inits
>  * PATCH-2 Fixed put() in ->of_xlate() and Updated commit message
>  * PATCH-3 Added Dmitry's Reviewed-by
> v3->v4
>  * Dropped helper function, per Thierry's comments
>  * Found another way to get smmu pointer
> v2->v3
>  * Replaced with devm_tegra_get_memory_controller
>  * Updated changes by following Dmitry's comments
> v1->v2
>  * Added PATCH-1 suggested by Dmitry
>  * Reworked PATCH-2 to unify certain code
> 
> Nicolin Chen (3):
>   iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev
>   iommu/tegra-smmu: Rework tegra_smmu_probe_device()
>   iommu/tegra-smmu: Add PCI support
> 
>  drivers/iommu/tegra-smmu.c | 187 +++++++++++++------------------------
>  1 file changed, 63 insertions(+), 124 deletions(-)
> 
> -- 
> 2.17.1
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu