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

[PATCH v4 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)
v3->v4
 * Dropped helper function
 * 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 | 177 ++++++++++++-------------------------
 1 file changed, 56 insertions(+), 121 deletions(-)

-- 
2.17.1

[PATCH v4 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.

Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
---

Changelog
v3->v4:
 * Seperated the change, as a cleanup, from the rework patch
v1->v3:
 * N/A

 drivers/iommu/tegra-smmu.c | 50 +++++++++++++++-----------------------
 1 file changed, 20 insertions(+), 30 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 6a3ecc334481..a573a5151c69 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;
-		}
-
-		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];
-
-		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

[PATCH v4 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.

Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
---

Changelog
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 | 90 ++++----------------------------------
 1 file changed, 9 insertions(+), 81 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index a573a5151c69..02d02b0c55c4 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,17 @@ 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];
 
+	of_node_put(args->np);
+
+	if (!mc || !mc->smmu)
+		return -EPROBE_DEFER;
+
+	dev_iommu_priv_set(dev, mc->smmu);
+
 	return iommu_fwspec_add_ids(dev, &id, 1);
 }
 
@@ -1079,16 +1017,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

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

[Nicolin Chen]

This patch simply adds support for PCI devices.

Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
---

Changelog
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 | 37 +++++++++++++++++++++++++++----------
 1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 02d02b0c55c4..b701a7b55e84 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);
@@ -1069,22 +1074,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 err_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 err_unregister;
+
+#ifdef CONFIG_PCI
+	err = bus_set_iommu(&pci_bus_type, &tegra_smmu_ops);
+	if (err < 0)
+		goto err_bus_set;
+#endif
 
 	if (IS_ENABLED(CONFIG_DEBUG_FS))
 		tegra_smmu_debugfs_init(smmu);
 
 	return smmu;
+
+err_bus_set: __maybe_unused;
+	bus_set_iommu(&platform_bus_type, NULL);
+err_unregister:
+	iommu_device_unregister(&smmu->iommu);
+err_sysfs:
+	iommu_device_sysfs_remove(&smmu->iommu);
+
+	return ERR_PTR(err);
 }
 
 void tegra_smmu_remove(struct tegra_smmu *smmu)
-- 
2.17.1

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

[Dmitry Osipenko]

02.10.2020 09:08, Nicolin Chen пишет:
>  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;

Looks like there is no need to initialize 'index' and 'err' variables
anymore.

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

[Dmitry Osipenko]

02.10.2020 09:08, Nicolin Chen пишет:
> -static void tegra_smmu_release_device(struct device *dev)
> -{
> -	dev_iommu_priv_set(dev, NULL);
> -}
> +static void tegra_smmu_release_device(struct device *dev) {}

Please keep the braces as-is.

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

[Dmitry Osipenko]

02.10.2020 09:08, Nicolin Chen пишет:
>  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];
>  
> +	of_node_put(args->np);
> +
> +	if (!mc || !mc->smmu)
> +		return -EPROBE_DEFER;

platform_get_drvdata(NULL) will crash.

> +	dev_iommu_priv_set(dev, mc->smmu);

I think put_device(mc->dev) is missed here, doesn't it?

Why sun50i-iommu driver doesn't have this error-checking? Is it really
needed at all?

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

[Dmitry Osipenko]

02.10.2020 09:08, Nicolin Chen пишет:
>  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);

The !smmu can't ever be true now, isn't it? Then please remove it.

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

[Dmitry Osipenko]

02.10.2020 09:08, Nicolin Chen пишет:
>  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];
> -
> -		if (args.np != smmu->dev->of_node) {
> -			of_node_put(args.np);
> -			continue;
> -		}
> -
> -		of_node_put(args.np);
> +	if (!fwspec)
> +		return;

When !fwspec could be true?

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

[Dmitry Osipenko]

02.10.2020 09:08, Nicolin Chen пишет:
> @@ -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);
> @@ -1069,22 +1074,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 err_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 err_unregister;
> +
> +#ifdef CONFIG_PCI
> +	err = bus_set_iommu(&pci_bus_type, &tegra_smmu_ops);
> +	if (err < 0)
> +		goto err_bus_set;
> +#endif
>  
>  	if (IS_ENABLED(CONFIG_DEBUG_FS))
>  		tegra_smmu_debugfs_init(smmu);
>  
>  	return smmu;
> +
> +err_bus_set: __maybe_unused;

__maybe_unused?

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

[Dmitry Osipenko]

02.10.2020 09:08, Nicolin Chen пишет:
>  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;
> -		}
> -
> -		of_node_put(args.np);
> +	if (!fwspec)
> +		return -ENOENT;

Could the !fwspec ever be true here as well?

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

[Dmitry Osipenko]

02.10.2020 17:22, Dmitry Osipenko пишет:
>>  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];
>>  
>> +	of_node_put(args->np);
>> +
>> +	if (!mc || !mc->smmu)
>> +		return -EPROBE_DEFER;
> platform_get_drvdata(NULL) will crash.
> 

Actually, platform_get_drvdata(NULL) can't happen. I overlooked this.

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

[Dmitry Osipenko]

02.10.2020 17:22, Dmitry Osipenko пишет:
> 02.10.2020 09:08, Nicolin Chen пишет:
>>  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;
> 
> Looks like there is no need to initialize 'index' and 'err' variables
> anymore.
> 

Same for tegra_smmu_detach_dev().

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

[Dmitry Osipenko]

02.10.2020 17:22, Dmitry Osipenko пишет:
> 02.10.2020 09:08, Nicolin Chen пишет:
>> -static void tegra_smmu_release_device(struct device *dev)
>> -{
>> -	dev_iommu_priv_set(dev, NULL);
>> -}
>> +static void tegra_smmu_release_device(struct device *dev) {}
> 
> Please keep the braces as-is.
> 

I noticed that you borrowed this style from the sun50i-iommu driver, but
this is a bit unusual coding style for the c files. At least to me it's
unusual to see header-style function stub in a middle of c file. But
maybe it's just me.

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

[Dmitry Osipenko]

02.10.2020 09:08, Nicolin Chen пишет:
>  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];
>  
> +	of_node_put(args->np);

of_find_device_by_node() takes device reference and not the np
reference. This is a bug, please remove of_node_put().

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

[Dmitry Osipenko]

02.10.2020 09:08, Nicolin Chen пишет:
> 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.

I don't quite see where IOMMU core calls of_xlate().

Have tried to at least boot-test this patch?

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

[Dmitry Osipenko]

02.10.2020 18:23, Dmitry Osipenko пишет:
> 02.10.2020 09:08, Nicolin Chen пишет:
>> 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.
> 
> I don't quite see where IOMMU core calls of_xlate().
> 
> Have tried to at least boot-test this patch?
> 

I don't see how it ever could work because of_xlate() is only invoked from:

fsl_mc_dma_configure()->of_dma_configure_id()->of_iommu_configure()

Looks like the tegra_smmu_configure() is still needed.

I don't know how sun50i driver could work to be honest. Seems IOMMU is
broken on sun50i, but maybe I'm missing something.

I added Maxime Ripard to this thread, who is the author of the
sun50i-iommu driver.

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

[Dmitry Osipenko]

02.10.2020 19:00, Dmitry Osipenko пишет:
> 02.10.2020 18:23, Dmitry Osipenko пишет:
>> 02.10.2020 09:08, Nicolin Chen пишет:
>>> 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.
>>
>> I don't quite see where IOMMU core calls of_xlate().
>>
>> Have tried to at least boot-test this patch?
>>
> 
> I don't see how it ever could work because of_xlate() is only invoked from:
> 
> fsl_mc_dma_configure()->of_dma_configure_id()->of_iommu_configure()
> 
> Looks like the tegra_smmu_configure() is still needed.
> 
> I don't know how sun50i driver could work to be honest. Seems IOMMU is
> broken on sun50i, but maybe I'm missing something.
> 
> I added Maxime Ripard to this thread, who is the author of the
> sun50i-iommu driver.
> 

Actually, I now see that the other IOMMU drivers (qcom, exynos, etc) do
the same. So obviously I'm missing something and it should work..

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

[Dmitry Osipenko]

02.10.2020 19:37, Dmitry Osipenko пишет:
> 02.10.2020 19:00, Dmitry Osipenko пишет:
>> 02.10.2020 18:23, Dmitry Osipenko пишет:
>>> 02.10.2020 09:08, Nicolin Chen пишет:
>>>> 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.
>>>
>>> I don't quite see where IOMMU core calls of_xlate().
>>>
>>> Have tried to at least boot-test this patch?
>>>
>>
>> I don't see how it ever could work because of_xlate() is only invoked from:
>>
>> fsl_mc_dma_configure()->of_dma_configure_id()->of_iommu_configure()
>>
>> Looks like the tegra_smmu_configure() is still needed.
>>
>> I don't know how sun50i driver could work to be honest. Seems IOMMU is
>> broken on sun50i, but maybe I'm missing something.
>>
>> I added Maxime Ripard to this thread, who is the author of the
>> sun50i-iommu driver.
>>
> 
> Actually, I now see that the other IOMMU drivers (qcom, exynos, etc) do
> the same. So obviously I'm missing something and it should work..
> 

Okay, somehow I was oblivious to that of_dma_configure() invokes
of_dma_configure_id(). Should be good :)

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

[Nicolin Chen]

On Fri, Oct 02, 2020 at 05:35:24PM +0300, Dmitry Osipenko wrote:
> 02.10.2020 09:08, Nicolin Chen пишет:
> > @@ -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);
> > @@ -1069,22 +1074,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 err_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 err_unregister;
> > +
> > +#ifdef CONFIG_PCI
> > +	err = bus_set_iommu(&pci_bus_type, &tegra_smmu_ops);
> > +	if (err < 0)
> > +		goto err_bus_set;
> > +#endif
> >  
> >  	if (IS_ENABLED(CONFIG_DEBUG_FS))
> >  		tegra_smmu_debugfs_init(smmu);
> >  
> >  	return smmu;
> > +
> > +err_bus_set: __maybe_unused;
> 
> __maybe_unused?

In order to mute a build warning when CONFIG_PCI=n...

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

[Nicolin Chen]

On Fri, Oct 02, 2020 at 05:23:14PM +0300, Dmitry Osipenko wrote:
> 02.10.2020 09:08, Nicolin Chen пишет:
> >  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);
> 
> The !smmu can't ever be true now, isn't it? Then please remove it.

How can you be so sure? Have you read my commit message? The whole
point of removing the hack in tegra_smmu_probe() is to return the
ERR_PTR(-ENODEV) here. The bus_set_iommu() will call this function
when mc->smmu is not assigned it, as it's assigned after we return
tegra_smmu_probe() while bus_set_iommu() is still in the middle of
the tegra_smmu_probe().

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

[Dmitry Osipenko]

02.10.2020 20:45, Nicolin Chen пишет:
> On Fri, Oct 02, 2020 at 05:35:24PM +0300, Dmitry Osipenko wrote:
>> 02.10.2020 09:08, Nicolin Chen пишет:
>>> @@ -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);
>>> @@ -1069,22 +1074,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 err_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 err_unregister;
>>> +
>>> +#ifdef CONFIG_PCI
>>> +	err = bus_set_iommu(&pci_bus_type, &tegra_smmu_ops);
>>> +	if (err < 0)
>>> +		goto err_bus_set;
>>> +#endif
>>>  
>>>  	if (IS_ENABLED(CONFIG_DEBUG_FS))
>>>  		tegra_smmu_debugfs_init(smmu);
>>>  
>>>  	return smmu;
>>> +
>>> +err_bus_set: __maybe_unused;
>>
>> __maybe_unused?
> 
> In order to mute a build warning when CONFIG_PCI=n...
> 

okay

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

[Dmitry Osipenko]

02.10.2020 09:08, Nicolin Chen пишет:
> This patch simply adds support for PCI devices.
> 
> Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
> ---

Reviewed-by: Dmitry Osipenko <digetx@gmail.com>

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

[Dmitry Osipenko]

02.10.2020 21:01, Nicolin Chen пишет:
> On Fri, Oct 02, 2020 at 05:23:14PM +0300, Dmitry Osipenko wrote:
>> 02.10.2020 09:08, Nicolin Chen пишет:
>>>  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);
>>
>> The !smmu can't ever be true now, isn't it? Then please remove it.
> 
> How can you be so sure? Have you read my commit message? The whole
> point of removing the hack in tegra_smmu_probe() is to return the
> ERR_PTR(-ENODEV) here. The bus_set_iommu() will call this function
> when mc->smmu is not assigned it, as it's assigned after we return
> tegra_smmu_probe() while bus_set_iommu() is still in the middle of
> the tegra_smmu_probe().
> 

My bad, I probably missed that was looking at the probe_device(), looks
good then.

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

[Nicolin Chen]

On Fri, Oct 02, 2020 at 06:02:18PM +0300, Dmitry Osipenko wrote:
> 02.10.2020 09:08, Nicolin Chen пишет:
> >  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];
> >  
> > +	of_node_put(args->np);
> 
> of_find_device_by_node() takes device reference and not the np
> reference. This is a bug, please remove of_node_put().

Looks like so. Replacing it with put_device(&iommu_pdev->dev);

Thanks

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

[Nicolin Chen]

On Fri, Oct 02, 2020 at 05:41:50PM +0300, Dmitry Osipenko wrote:
> 02.10.2020 09:08, Nicolin Chen пишет:
> >  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;
> > -		}
> > -
> > -		of_node_put(args.np);
> > +	if (!fwspec)
> > +		return -ENOENT;
> 
> Could the !fwspec ever be true here as well?

There are multiple callers of this function. It's really not that
straightforward to track every one of them. So I'd rather have it
here as other iommu drivers do. We are human beings, so we could
have missed something somewhere, especially callers are not from
tegra-* drivers.

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

[Nicolin Chen]

On Fri, Oct 02, 2020 at 05:58:29PM +0300, Dmitry Osipenko wrote:
> 02.10.2020 17:22, Dmitry Osipenko пишет:
> > 02.10.2020 09:08, Nicolin Chen пишет:
> >> -static void tegra_smmu_release_device(struct device *dev)
> >> -{
> >> -	dev_iommu_priv_set(dev, NULL);
> >> -}
> >> +static void tegra_smmu_release_device(struct device *dev) {}
> > 
> > Please keep the braces as-is.
> > 
> 
> I noticed that you borrowed this style from the sun50i-iommu driver, but
> this is a bit unusual coding style for the c files. At least to me it's
> unusual to see header-style function stub in a middle of c file. But
> maybe it's just me.

I don't see a rule in ./Documentation/process/coding-style.rst
against this, and there're plenty of drivers doing so. If you
feel uncomfortable with this style, you may add a rule to that
doc so everyone will follow :)

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

[Nicolin Chen]

On Fri, Oct 02, 2020 at 05:52:00PM +0300, Dmitry Osipenko wrote:
> 02.10.2020 17:22, Dmitry Osipenko пишет:
> > 02.10.2020 09:08, Nicolin Chen пишет:
> >>  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;
> > 
> > Looks like there is no need to initialize 'index' and 'err' variables
> > anymore.
> > 
> 
> Same for tegra_smmu_detach_dev().

Can remove them.

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

[Dmitry Osipenko]

02.10.2020 22:45, Nicolin Chen пишет:
> On Fri, Oct 02, 2020 at 05:41:50PM +0300, Dmitry Osipenko wrote:
>> 02.10.2020 09:08, Nicolin Chen пишет:
>>>  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;
>>> -		}
>>> -
>>> -		of_node_put(args.np);
>>> +	if (!fwspec)
>>> +		return -ENOENT;
>>
>> Could the !fwspec ever be true here as well?
> 
> There are multiple callers of this function. It's really not that
> straightforward to track every one of them. So I'd rather have it
> here as other iommu drivers do. We are human beings, so we could
> have missed something somewhere, especially callers are not from
> tegra-* drivers.
> 

I'm looking at the IOMMU core and it requires device to be in IOMMU
group before attach_dev() could be called.

The group can't be assigned to device without the fwspec, see
tegra_smmu_device_group().

Seems majority of IOMMU drivers are checking dev_iommu_priv_get() for
NULL in attach_dev(), some not checking anything, some check both and
only arm-smmu checks the fwspec.

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

[Nicolin Chen]

On Fri, Oct 02, 2020 at 11:12:18PM +0300, Dmitry Osipenko wrote:
> 02.10.2020 22:45, Nicolin Chen пишет:
> > On Fri, Oct 02, 2020 at 05:41:50PM +0300, Dmitry Osipenko wrote:
> >> 02.10.2020 09:08, Nicolin Chen пишет:
> >>>  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;
> >>> -		}
> >>> -
> >>> -		of_node_put(args.np);
> >>> +	if (!fwspec)
> >>> +		return -ENOENT;
> >>
> >> Could the !fwspec ever be true here as well?
> > 
> > There are multiple callers of this function. It's really not that
> > straightforward to track every one of them. So I'd rather have it
> > here as other iommu drivers do. We are human beings, so we could
> > have missed something somewhere, especially callers are not from
> > tegra-* drivers.
> > 
> 
> I'm looking at the IOMMU core and it requires device to be in IOMMU
> group before attach_dev() could be called.
> 
> The group can't be assigned to device without the fwspec, see
> tegra_smmu_device_group().
>
> Seems majority of IOMMU drivers are checking dev_iommu_priv_get() for
> NULL in attach_dev(), some not checking anything, some check both and
> only arm-smmu checks the fwspec.

As I said a couple of days ago, I don't like to assume that the
callers won't change. And this time, it's from open code. So I
don't want to assume that there won't be a change.

If you are confident that there is no need to add such a check,
please send patches to remove those checks in those drivers to
see if others would agree. I would be willing to remove it after
that. Otherwise, I'd like to keep this.

Thanks for the review.

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

[Dmitry Osipenko]

03.10.2020 02:53, Nicolin Chen пишет:
> On Fri, Oct 02, 2020 at 11:12:18PM +0300, Dmitry Osipenko wrote:
>> 02.10.2020 22:45, Nicolin Chen пишет:
>>> On Fri, Oct 02, 2020 at 05:41:50PM +0300, Dmitry Osipenko wrote:
>>>> 02.10.2020 09:08, Nicolin Chen пишет:
>>>>>  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;
>>>>> -		}
>>>>> -
>>>>> -		of_node_put(args.np);
>>>>> +	if (!fwspec)
>>>>> +		return -ENOENT;
>>>>
>>>> Could the !fwspec ever be true here as well?
>>>
>>> There are multiple callers of this function. It's really not that
>>> straightforward to track every one of them. So I'd rather have it
>>> here as other iommu drivers do. We are human beings, so we could
>>> have missed something somewhere, especially callers are not from
>>> tegra-* drivers.
>>>
>>
>> I'm looking at the IOMMU core and it requires device to be in IOMMU
>> group before attach_dev() could be called.
>>
>> The group can't be assigned to device without the fwspec, see
>> tegra_smmu_device_group().
>>
>> Seems majority of IOMMU drivers are checking dev_iommu_priv_get() for
>> NULL in attach_dev(), some not checking anything, some check both and
>> only arm-smmu checks the fwspec.
> 
> As I said a couple of days ago, I don't like to assume that the
> callers won't change. And this time, it's from open code. So I
> don't want to assume that there won't be a change.
> 
> If you are confident that there is no need to add such a check,
> please send patches to remove those checks in those drivers to
> see if others would agree. I would be willing to remove it after
> that. Otherwise, I'd like to keep this.
> 
> Thanks for the review.
> 

I haven't tried to check every code path very thoroughly, expecting you
to do it since you're making this patch. Maybe there is a real reason
why majority of drivers do the checks and it would be good to know why.
Although, it's not critical in this particular case and indeed the
checks could be improved later on.

It looks to me that at least will be a bit better/cleaner to check the
dev_iommu_priv_get() for NULL instead of fwspec because the private
variable depends on the fwspec presence and there is a similar check in
probe_device, hence checks will be more consistent.

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

[Thierry Reding]

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

On Fri, Oct 02, 2020 at 12:53:28PM -0700, Nicolin Chen wrote:
> On Fri, Oct 02, 2020 at 05:58:29PM +0300, Dmitry Osipenko wrote:
> > 02.10.2020 17:22, Dmitry Osipenko пишет:
> > > 02.10.2020 09:08, Nicolin Chen пишет:
> > >> -static void tegra_smmu_release_device(struct device *dev)
> > >> -{
> > >> -	dev_iommu_priv_set(dev, NULL);
> > >> -}
> > >> +static void tegra_smmu_release_device(struct device *dev) {}
> > > 
> > > Please keep the braces as-is.
> > > 
> > 
> > I noticed that you borrowed this style from the sun50i-iommu driver, but
> > this is a bit unusual coding style for the c files. At least to me it's
> > unusual to see header-style function stub in a middle of c file. But
> > maybe it's just me.
> 
> I don't see a rule in ./Documentation/process/coding-style.rst
> against this, and there're plenty of drivers doing so. If you
> feel uncomfortable with this style, you may add a rule to that
> doc so everyone will follow :)

I also prefer braces on separate lines. Even better would be to just
drop this entirely and make ->release_device() optional. At least the
following drivers could be cleaned up that way:

    * fsl-pamu
    * msm-iommu
    * sun50i-iommu
    * tegra-gart
    * tegra-smmu

And it looks like mtk-iommu and mtk-iommu-v1 do only iommu_fwspec_free()
in their ->release_device() implementations, but that's already done via
iommu_release_device().

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Thierry Reding]

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

On Fri, Oct 02, 2020 at 05:22:41PM +0300, Dmitry Osipenko wrote:
> 02.10.2020 09:08, Nicolin Chen пишет:
> >  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];
> >  
> > +	of_node_put(args->np);
> > +
> > +	if (!mc || !mc->smmu)
> > +		return -EPROBE_DEFER;
> 
> platform_get_drvdata(NULL) will crash.
> 
> > +	dev_iommu_priv_set(dev, mc->smmu);
> 
> I think put_device(mc->dev) is missed here, doesn't it?

Yeah, I think we'd need that here, otherwise we'd be leaking a
reference. Worse, even, mc->dev is the same device that owns the SMMU,
so we're basically incrementing our own reference here and never
releasing it. We also need that put_device(mc->dev) in the error case
above because we already hold the reference there.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Thierry Reding]

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

On Fri, Oct 02, 2020 at 05:50:08PM +0300, Dmitry Osipenko wrote:
> 02.10.2020 17:22, Dmitry Osipenko пишет:
> >>  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];
> >>  
> >> +	of_node_put(args->np);
> >> +
> >> +	if (!mc || !mc->smmu)
> >> +		return -EPROBE_DEFER;
> > platform_get_drvdata(NULL) will crash.
> > 
> 
> Actually, platform_get_drvdata(NULL) can't happen. I overlooked this.

How so? It's technically possible for the iommus property to reference a
device tree node for which no platform device will ever be created, in
which case of_find_device_by_node() will return NULL. That's very
unlikely and perhaps worth just crashing on to make sure it gets fixed
immediately.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Thierry Reding]

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

On Fri, Oct 02, 2020 at 11:58:29AM -0700, Nicolin Chen wrote:
> On Fri, Oct 02, 2020 at 06:02:18PM +0300, Dmitry Osipenko wrote:
> > 02.10.2020 09:08, Nicolin Chen пишет:
> > >  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];
> > >  
> > > +	of_node_put(args->np);
> > 
> > of_find_device_by_node() takes device reference and not the np
> > reference. This is a bug, please remove of_node_put().
> 
> Looks like so. Replacing it with put_device(&iommu_pdev->dev);

Putting the put_device() here is wrong, though. You need to make sure
you keep a reference to it as long as you keep accessing the data that
is owned by it.

Like I said earlier, this is a bit weird in this case because we're
self-referencing, so iommu_pdev->dev is going to stay around as long as
the SMMU is. However, it might be worth to properly track the lifetime
anyway just so that the code can serve as a good example of how to do
things.

If you decide to go for the shortcut and not track this reference
properly, then at least you need to add a comment as to why it is safe
to do in this case. This ensures that readers are away of the
circumstances and don't copy this bad code into a context where the
circumstances are different.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Thierry Reding]

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

On Thu, Oct 01, 2020 at 11:08:07PM -0700, Nicolin Chen wrote:
> This patch simply adds support for PCI devices.
> 
> Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
> ---
> 
> Changelog
> 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 | 37 +++++++++++++++++++++++++++----------
>  1 file changed, 27 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> index 02d02b0c55c4..b701a7b55e84 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);
> @@ -1069,22 +1074,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 err_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 err_unregister;
> +
> +#ifdef CONFIG_PCI
> +	err = bus_set_iommu(&pci_bus_type, &tegra_smmu_ops);
> +	if (err < 0)
> +		goto err_bus_set;
> +#endif
>  
>  	if (IS_ENABLED(CONFIG_DEBUG_FS))
>  		tegra_smmu_debugfs_init(smmu);
>  
>  	return smmu;
> +
> +err_bus_set: __maybe_unused;
> +	bus_set_iommu(&platform_bus_type, NULL);
> +err_unregister:
> +	iommu_device_unregister(&smmu->iommu);
> +err_sysfs:
> +	iommu_device_sysfs_remove(&smmu->iommu);

Can you please switch to label names that are more consistent with the
others in this driver? Notably the ones in tegra_smmu_domain_alloc().
The idea is to describe in the name of the label what's happening at the
label. Something like this, for example:

unset_platform_bus:
	bus_set_iommu(&platform_bus_type, NULL);
unregister:
	iommu_device_unregister(&smmu->iommu);
remove_sysfs:
	iommu_device_sysfs_remove(&smmu->iommu);

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Dmitry Osipenko]

05.10.2020 12:53, Thierry Reding пишет:
> On Fri, Oct 02, 2020 at 05:50:08PM +0300, Dmitry Osipenko wrote:
>> 02.10.2020 17:22, Dmitry Osipenko пишет:
>>>>  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];
>>>>  
>>>> +	of_node_put(args->np);
>>>> +
>>>> +	if (!mc || !mc->smmu)
>>>> +		return -EPROBE_DEFER;
>>> platform_get_drvdata(NULL) will crash.
>>>
>>
>> Actually, platform_get_drvdata(NULL) can't happen. I overlooked this.
> 
> How so? It's technically possible for the iommus property to reference a
> device tree node for which no platform device will ever be created, in
> which case of_find_device_by_node() will return NULL. That's very
> unlikely and perhaps worth just crashing on to make sure it gets fixed
> immediately.

The tegra_smmu_ops are registered from the SMMU driver itself and MC
driver sets platform data before SMMU is initialized, hence device is
guaranteed to exist and mc can't be NULL.

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

[Thierry Reding]

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

On Mon, Oct 05, 2020 at 01:36:55PM +0300, Dmitry Osipenko wrote:
> 05.10.2020 12:53, Thierry Reding пишет:
> > On Fri, Oct 02, 2020 at 05:50:08PM +0300, Dmitry Osipenko wrote:
> >> 02.10.2020 17:22, Dmitry Osipenko пишет:
> >>>>  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];
> >>>>  
> >>>> +	of_node_put(args->np);
> >>>> +
> >>>> +	if (!mc || !mc->smmu)
> >>>> +		return -EPROBE_DEFER;
> >>> platform_get_drvdata(NULL) will crash.
> >>>
> >>
> >> Actually, platform_get_drvdata(NULL) can't happen. I overlooked this.
> > 
> > How so? It's technically possible for the iommus property to reference a
> > device tree node for which no platform device will ever be created, in
> > which case of_find_device_by_node() will return NULL. That's very
> > unlikely and perhaps worth just crashing on to make sure it gets fixed
> > immediately.
> 
> The tegra_smmu_ops are registered from the SMMU driver itself and MC
> driver sets platform data before SMMU is initialized, hence device is
> guaranteed to exist and mc can't be NULL.

Yes, but that assumes that args->np points to the memory controller's
device tree node. It's obviously a mistake to do this, but I don't think
anyone will prevent you from doing this:

	iommus = <&{/chosen} 0>;

In that case, since no platform device is created for the /chosen node,
iommu_pdev will end up being NULL and platform_get_drvdata() will crash.

That said, I'm fine with not adding a check for that. If anyone really
does end up messing this up they deserve the crash.

I'm still a bit undecided about the mc->smmu check because I haven't
convinced myself yet that it can't happen.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Dmitry Osipenko]

05.10.2020 14:15, Thierry Reding пишет:
> On Mon, Oct 05, 2020 at 01:36:55PM +0300, Dmitry Osipenko wrote:
>> 05.10.2020 12:53, Thierry Reding пишет:
>>> On Fri, Oct 02, 2020 at 05:50:08PM +0300, Dmitry Osipenko wrote:
>>>> 02.10.2020 17:22, Dmitry Osipenko пишет:
>>>>>>  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];
>>>>>>  
>>>>>> +	of_node_put(args->np);
>>>>>> +
>>>>>> +	if (!mc || !mc->smmu)
>>>>>> +		return -EPROBE_DEFER;
>>>>> platform_get_drvdata(NULL) will crash.
>>>>>
>>>>
>>>> Actually, platform_get_drvdata(NULL) can't happen. I overlooked this.
>>>
>>> How so? It's technically possible for the iommus property to reference a
>>> device tree node for which no platform device will ever be created, in
>>> which case of_find_device_by_node() will return NULL. That's very
>>> unlikely and perhaps worth just crashing on to make sure it gets fixed
>>> immediately.
>>
>> The tegra_smmu_ops are registered from the SMMU driver itself and MC
>> driver sets platform data before SMMU is initialized, hence device is
>> guaranteed to exist and mc can't be NULL.
> 
> Yes, but that assumes that args->np points to the memory controller's
> device tree node. It's obviously a mistake to do this, but I don't think
> anyone will prevent you from doing this:
> 
> 	iommus = <&{/chosen} 0>;
> 
> In that case, since no platform device is created for the /chosen node,
> iommu_pdev will end up being NULL and platform_get_drvdata() will crash.

But then Tegra SMMU isn't associated with the device's IOMMU path, and
thus, tegra_smmu_of_xlate() won't be invoked for this device.

> That said, I'm fine with not adding a check for that. If anyone really
> does end up messing this up they deserve the crash.
> 
> I'm still a bit undecided about the mc->smmu check because I haven't
> convinced myself yet that it can't happen.

For now I can't see any realistic situation where mc->smmu could be NULL.

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

[Thierry Reding]

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

On Mon, Oct 05, 2020 at 04:28:53PM +0300, Dmitry Osipenko wrote:
> 05.10.2020 14:15, Thierry Reding пишет:
> > On Mon, Oct 05, 2020 at 01:36:55PM +0300, Dmitry Osipenko wrote:
> >> 05.10.2020 12:53, Thierry Reding пишет:
> >>> On Fri, Oct 02, 2020 at 05:50:08PM +0300, Dmitry Osipenko wrote:
> >>>> 02.10.2020 17:22, Dmitry Osipenko пишет:
> >>>>>>  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];
> >>>>>>  
> >>>>>> +	of_node_put(args->np);
> >>>>>> +
> >>>>>> +	if (!mc || !mc->smmu)
> >>>>>> +		return -EPROBE_DEFER;
> >>>>> platform_get_drvdata(NULL) will crash.
> >>>>>
> >>>>
> >>>> Actually, platform_get_drvdata(NULL) can't happen. I overlooked this.
> >>>
> >>> How so? It's technically possible for the iommus property to reference a
> >>> device tree node for which no platform device will ever be created, in
> >>> which case of_find_device_by_node() will return NULL. That's very
> >>> unlikely and perhaps worth just crashing on to make sure it gets fixed
> >>> immediately.
> >>
> >> The tegra_smmu_ops are registered from the SMMU driver itself and MC
> >> driver sets platform data before SMMU is initialized, hence device is
> >> guaranteed to exist and mc can't be NULL.
> > 
> > Yes, but that assumes that args->np points to the memory controller's
> > device tree node. It's obviously a mistake to do this, but I don't think
> > anyone will prevent you from doing this:
> > 
> > 	iommus = <&{/chosen} 0>;
> > 
> > In that case, since no platform device is created for the /chosen node,
> > iommu_pdev will end up being NULL and platform_get_drvdata() will crash.
> 
> But then Tegra SMMU isn't associated with the device's IOMMU path, and
> thus, tegra_smmu_of_xlate() won't be invoked for this device.

Indeed, you're right! It used to be that ops were assigned to the bus
without any knowledge about the specific instances that might exist, but
nowadays there's struct iommu_device which properly encapsulates all of
that, so yeah, I don't think this can ever be NULL.

Although that makes me wonder why we aren't going one step further and
pass struct iommu_device * into ->of_xlate(), which would avoid the need
for us to do the look up once more.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Nicolin Chen]

On Mon, Oct 05, 2020 at 12:04:19PM +0200, Thierry Reding wrote:
> > +err_bus_set: __maybe_unused;
> > +	bus_set_iommu(&platform_bus_type, NULL);
> > +err_unregister:
> > +	iommu_device_unregister(&smmu->iommu);
> > +err_sysfs:
> > +	iommu_device_sysfs_remove(&smmu->iommu);
> 
> Can you please switch to label names that are more consistent with the
> others in this driver? Notably the ones in tegra_smmu_domain_alloc().
> The idea is to describe in the name of the label what's happening at the
> label. Something like this, for example:
> 
> unset_platform_bus:
> 	bus_set_iommu(&platform_bus_type, NULL);
> unregister:
> 	iommu_device_unregister(&smmu->iommu);
> remove_sysfs:
> 	iommu_device_sysfs_remove(&smmu->iommu);

Okay. Will update in v7.

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

[Nicolin Chen]

On Mon, Oct 05, 2020 at 11:57:54AM +0200, Thierry Reding wrote:
> On Fri, Oct 02, 2020 at 11:58:29AM -0700, Nicolin Chen wrote:
> > On Fri, Oct 02, 2020 at 06:02:18PM +0300, Dmitry Osipenko wrote:
> > > 02.10.2020 09:08, Nicolin Chen пишет:
> > > >  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];
> > > >  
> > > > +	of_node_put(args->np);
> > > 
> > > of_find_device_by_node() takes device reference and not the np
> > > reference. This is a bug, please remove of_node_put().
> > 
> > Looks like so. Replacing it with put_device(&iommu_pdev->dev);
> 
> Putting the put_device() here is wrong, though. You need to make sure
> you keep a reference to it as long as you keep accessing the data that
> is owned by it.

I am confused. You said in the other reply (to Dmitry) that we do
need to put_device(mc->dev), where mc->dev should be the same as
iommu_pdev->dev. But here your comments sounds that we should not
put_device at all since ->probe_device/group_device/attach_dev()
will use it later.

> Like I said earlier, this is a bit weird in this case because we're
> self-referencing, so iommu_pdev->dev is going to stay around as long as
> the SMMU is. However, it might be worth to properly track the lifetime
> anyway just so that the code can serve as a good example of how to do
> things.

What's this "track-the-lifetime"?

> If you decide to go for the shortcut and not track this reference
> properly, then at least you need to add a comment as to why it is safe
> to do in this case. This ensures that readers are away of the
> circumstances and don't copy this bad code into a context where the
> circumstances are different.

I don't quite get this "shortcut" here either...mind elaborating?

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

[Thierry Reding]

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

On Mon, Oct 05, 2020 at 06:05:46PM -0700, Nicolin Chen wrote:
> On Mon, Oct 05, 2020 at 11:57:54AM +0200, Thierry Reding wrote:
> > On Fri, Oct 02, 2020 at 11:58:29AM -0700, Nicolin Chen wrote:
> > > On Fri, Oct 02, 2020 at 06:02:18PM +0300, Dmitry Osipenko wrote:
> > > > 02.10.2020 09:08, Nicolin Chen пишет:
> > > > >  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];
> > > > >  
> > > > > +	of_node_put(args->np);
> > > > 
> > > > of_find_device_by_node() takes device reference and not the np
> > > > reference. This is a bug, please remove of_node_put().
> > > 
> > > Looks like so. Replacing it with put_device(&iommu_pdev->dev);
> > 
> > Putting the put_device() here is wrong, though. You need to make sure
> > you keep a reference to it as long as you keep accessing the data that
> > is owned by it.
> 
> I am confused. You said in the other reply (to Dmitry) that we do
> need to put_device(mc->dev), where mc->dev should be the same as
> iommu_pdev->dev. But here your comments sounds that we should not
> put_device at all since ->probe_device/group_device/attach_dev()
> will use it later.

You need to call put_device() at some point to release the reference
that you acquired by calling of_find_device_by_node(). If you don't
release it, you're leaking the reference and the kernel isn't going to
know when it's safe to delete the device.

So what I'm saying is that we either release it here, which isn't quite
right because we do reference data relating to the device later on. And
because it isn't quite right there should be a reason to justify it,
which is that the SMMU parent device is the same as the MC, so the
reference count isn't strictly necessary. But that's not quite obvious,
so highlighting it in a comment makes sense.

The other alternative is to not call put_device() here and keep on to
the reference as long as you keep using "mc". This might be difficult to
implement because it may not be obvious where to release it. I think
this is the better alternative, but if it's too complicated to implement
it might not be worth it.

> > Like I said earlier, this is a bit weird in this case because we're
> > self-referencing, so iommu_pdev->dev is going to stay around as long as
> > the SMMU is. However, it might be worth to properly track the lifetime
> > anyway just so that the code can serve as a good example of how to do
> > things.
> 
> What's this "track-the-lifetime"?

This basically just means that SMMU needs to ensure that MC stays alive
(by holding a reference to it) as long as SMMU uses it. If the last
reference to MC is dropped, then the mc pointer and potentially anything
that it points to will become dangling. If you were to drop the last
reference at this point, then on the next line the mc pointer could
already be invalid.

That's how it generally works, anyway. What's special about this use-
case is that the SMMU and MC are the same device, so it should be safe
to omit this additional tracking because the IOMMU tracking should take
care of that already.

> > If you decide to go for the shortcut and not track this reference
> > properly, then at least you need to add a comment as to why it is safe
> > to do in this case. This ensures that readers are away of the
> > circumstances and don't copy this bad code into a context where the
> > circumstances are different.
> 
> I don't quite get this "shortcut" here either...mind elaborating?

The shortcut is taking advantage of the knowledge that the SMMU and the
MC are the same device and therefore not properly track the MC object.
Given that their code is located in different locations, this isn't
obvious to the casual reader of the code, so they may assume that this
is the normal way to do things. To avoid that, the code should have a
comment explaining why that is.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Nicolin Chen]

On Thu, Oct 08, 2020 at 11:53:43AM +0200, Thierry Reding wrote:
> On Mon, Oct 05, 2020 at 06:05:46PM -0700, Nicolin Chen wrote:
> > On Mon, Oct 05, 2020 at 11:57:54AM +0200, Thierry Reding wrote:
> > > On Fri, Oct 02, 2020 at 11:58:29AM -0700, Nicolin Chen wrote:
> > > > On Fri, Oct 02, 2020 at 06:02:18PM +0300, Dmitry Osipenko wrote:
> > > > > 02.10.2020 09:08, Nicolin Chen пишет:
> > > > > >  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];
> > > > > >  
> > > > > > +	of_node_put(args->np);
> > > > > 
> > > > > of_find_device_by_node() takes device reference and not the np
> > > > > reference. This is a bug, please remove of_node_put().
> > > > 
> > > > Looks like so. Replacing it with put_device(&iommu_pdev->dev);
> > > 
> > > Putting the put_device() here is wrong, though. You need to make sure
> > > you keep a reference to it as long as you keep accessing the data that
> > > is owned by it.
> > 
> > I am confused. You said in the other reply (to Dmitry) that we do
> > need to put_device(mc->dev), where mc->dev should be the same as
> > iommu_pdev->dev. But here your comments sounds that we should not
> > put_device at all since ->probe_device/group_device/attach_dev()
> > will use it later.
> 
> You need to call put_device() at some point to release the reference
> that you acquired by calling of_find_device_by_node(). If you don't
> release it, you're leaking the reference and the kernel isn't going to
> know when it's safe to delete the device.
> 
> So what I'm saying is that we either release it here, which isn't quite
> right because we do reference data relating to the device later on. And

I see. A small question here by the way: By looking at other IOMMU
drivers that are calling driver_find_device_by_fwnode() function,
I found that most of them put_device right after the function call,
and dev_get_drvdata() after putting the device..

Feels like they are doing it wrongly?

> because it isn't quite right there should be a reason to justify it,
> which is that the SMMU parent device is the same as the MC, so the
> reference count isn't strictly necessary. But that's not quite obvious,
> so highlighting it in a comment makes sense.
> 
> The other alternative is to not call put_device() here and keep on to
> the reference as long as you keep using "mc". This might be difficult to
> implement because it may not be obvious where to release it. I think
> this is the better alternative, but if it's too complicated to implement
> it might not be worth it.

I feel so too. The dev is got at of_xlate() that does not have an
obvious counterpart function. So I'll just remove put_device() and
put a line of comments, as you suggested.

> > > Like I said earlier, this is a bit weird in this case because we're
> > > self-referencing, so iommu_pdev->dev is going to stay around as long as
> > > the SMMU is. However, it might be worth to properly track the lifetime
> > > anyway just so that the code can serve as a good example of how to do
> > > things.
> > 
> > What's this "track-the-lifetime"?
> 
> This basically just means that SMMU needs to ensure that MC stays alive
> (by holding a reference to it) as long as SMMU uses it. If the last
> reference to MC is dropped, then the mc pointer and potentially anything
> that it points to will become dangling. If you were to drop the last
> reference at this point, then on the next line the mc pointer could
> already be invalid.
> 
> That's how it generally works, anyway. What's special about this use-
> case is that the SMMU and MC are the same device, so it should be safe
> to omit this additional tracking because the IOMMU tracking should take
> care of that already.

Okay.

> > > If you decide to go for the shortcut and not track this reference
> > > properly, then at least you need to add a comment as to why it is safe
> > > to do in this case. This ensures that readers are away of the
> > > circumstances and don't copy this bad code into a context where the
> > > circumstances are different.
> > 
> > I don't quite get this "shortcut" here either...mind elaborating?
> 
> The shortcut is taking advantage of the knowledge that the SMMU and the
> MC are the same device and therefore not properly track the MC object.
> Given that their code is located in different locations, this isn't
> obvious to the casual reader of the code, so they may assume that this
> is the normal way to do things. To avoid that, the code should have a
> comment explaining why that is.

Got it. Thanks!

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

[Thierry Reding]

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

On Thu, Oct 08, 2020 at 02:12:10PM -0700, Nicolin Chen wrote:
> On Thu, Oct 08, 2020 at 11:53:43AM +0200, Thierry Reding wrote:
> > On Mon, Oct 05, 2020 at 06:05:46PM -0700, Nicolin Chen wrote:
> > > On Mon, Oct 05, 2020 at 11:57:54AM +0200, Thierry Reding wrote:
> > > > On Fri, Oct 02, 2020 at 11:58:29AM -0700, Nicolin Chen wrote:
> > > > > On Fri, Oct 02, 2020 at 06:02:18PM +0300, Dmitry Osipenko wrote:
> > > > > > 02.10.2020 09:08, Nicolin Chen пишет:
> > > > > > >  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];
> > > > > > >  
> > > > > > > +	of_node_put(args->np);
> > > > > > 
> > > > > > of_find_device_by_node() takes device reference and not the np
> > > > > > reference. This is a bug, please remove of_node_put().
> > > > > 
> > > > > Looks like so. Replacing it with put_device(&iommu_pdev->dev);
> > > > 
> > > > Putting the put_device() here is wrong, though. You need to make sure
> > > > you keep a reference to it as long as you keep accessing the data that
> > > > is owned by it.
> > > 
> > > I am confused. You said in the other reply (to Dmitry) that we do
> > > need to put_device(mc->dev), where mc->dev should be the same as
> > > iommu_pdev->dev. But here your comments sounds that we should not
> > > put_device at all since ->probe_device/group_device/attach_dev()
> > > will use it later.
> > 
> > You need to call put_device() at some point to release the reference
> > that you acquired by calling of_find_device_by_node(). If you don't
> > release it, you're leaking the reference and the kernel isn't going to
> > know when it's safe to delete the device.
> > 
> > So what I'm saying is that we either release it here, which isn't quite
> > right because we do reference data relating to the device later on. And
> 
> I see. A small question here by the way: By looking at other IOMMU
> drivers that are calling driver_find_device_by_fwnode() function,
> I found that most of them put_device right after the function call,
> and dev_get_drvdata() after putting the device..
> 
> Feels like they are doing it wrongly?

Well, like I said this is somewhat academic because these are all
referencing the IOMMU that by definition still needs to be around
when this code is called, and there's locks in place to ensure
these don't go away. So it's not like these drivers are doing it
wrong, they're just not doing it pedantically right.

> 
> > because it isn't quite right there should be a reason to justify it,
> > which is that the SMMU parent device is the same as the MC, so the
> > reference count isn't strictly necessary. But that's not quite obvious,
> > so highlighting it in a comment makes sense.
> > 
> > The other alternative is to not call put_device() here and keep on to
> > the reference as long as you keep using "mc". This might be difficult to
> > implement because it may not be obvious where to release it. I think
> > this is the better alternative, but if it's too complicated to implement
> > it might not be worth it.
> 
> I feel so too. The dev is got at of_xlate() that does not have an
> obvious counterpart function. So I'll just remove put_device() and
> put a line of comments, as you suggested.

I think you misunderstood. Not calling put_device() would be wrong
because that leaks a reference to the SMMU that you can't get back. My
suggestion was rather to keep put_device() here, but add a comment as to
why it's okay to call the put_device() here, even though you keep using
its private data later beyond this point, which typically would be wrong
to do.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Nicolin Chen]

On Fri, Oct 09, 2020 at 02:25:56PM +0200, Thierry Reding wrote:
> On Thu, Oct 08, 2020 at 02:12:10PM -0700, Nicolin Chen wrote:
> > On Thu, Oct 08, 2020 at 11:53:43AM +0200, Thierry Reding wrote:
> > > On Mon, Oct 05, 2020 at 06:05:46PM -0700, Nicolin Chen wrote:
> > > > On Mon, Oct 05, 2020 at 11:57:54AM +0200, Thierry Reding wrote:
> > > > > On Fri, Oct 02, 2020 at 11:58:29AM -0700, Nicolin Chen wrote:
> > > > > > On Fri, Oct 02, 2020 at 06:02:18PM +0300, Dmitry Osipenko wrote:
> > > > > > > 02.10.2020 09:08, Nicolin Chen пишет:
> > > > > > > >  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];
> > > > > > > >  
> > > > > > > > +	of_node_put(args->np);
> > > > > > > 
> > > > > > > of_find_device_by_node() takes device reference and not the np
> > > > > > > reference. This is a bug, please remove of_node_put().
> > > > > > 
> > > > > > Looks like so. Replacing it with put_device(&iommu_pdev->dev);
> > > > > 
> > > > > Putting the put_device() here is wrong, though. You need to make sure
> > > > > you keep a reference to it as long as you keep accessing the data that
> > > > > is owned by it.
> > > > 
> > > > I am confused. You said in the other reply (to Dmitry) that we do
> > > > need to put_device(mc->dev), where mc->dev should be the same as
> > > > iommu_pdev->dev. But here your comments sounds that we should not
> > > > put_device at all since ->probe_device/group_device/attach_dev()
> > > > will use it later.
> > > 
> > > You need to call put_device() at some point to release the reference
> > > that you acquired by calling of_find_device_by_node(). If you don't
> > > release it, you're leaking the reference and the kernel isn't going to
> > > know when it's safe to delete the device.
> > > 
> > > So what I'm saying is that we either release it here, which isn't quite
> > > right because we do reference data relating to the device later on. And
> > 
> > I see. A small question here by the way: By looking at other IOMMU
> > drivers that are calling driver_find_device_by_fwnode() function,
> > I found that most of them put_device right after the function call,
> > and dev_get_drvdata() after putting the device..
> > 
> > Feels like they are doing it wrongly?
> 
> Well, like I said this is somewhat academic because these are all
> referencing the IOMMU that by definition still needs to be around
> when this code is called, and there's locks in place to ensure
> these don't go away. So it's not like these drivers are doing it
> wrong, they're just not doing it pedantically right.
> 
> > 
> > > because it isn't quite right there should be a reason to justify it,
> > > which is that the SMMU parent device is the same as the MC, so the
> > > reference count isn't strictly necessary. But that's not quite obvious,
> > > so highlighting it in a comment makes sense.
> > > 
> > > The other alternative is to not call put_device() here and keep on to
> > > the reference as long as you keep using "mc". This might be difficult to
> > > implement because it may not be obvious where to release it. I think
> > > this is the better alternative, but if it's too complicated to implement
> > > it might not be worth it.
> > 
> > I feel so too. The dev is got at of_xlate() that does not have an
> > obvious counterpart function. So I'll just remove put_device() and
> > put a line of comments, as you suggested.
> 
> I think you misunderstood. Not calling put_device() would be wrong
> because that leaks a reference to the SMMU that you can't get back. My
> suggestion was rather to keep put_device() here, but add a comment as to
> why it's okay to call the put_device() here, even though you keep using
> its private data later beyond this point, which typically would be wrong
> to do.

I see. Thanks for clarification! Will send v6 soon.