[PATCH v8 0/6] iommu/tegra-smmu: Add pagetable mappings to debugfs

[PATCH v8 0/6] iommu/tegra-smmu: Add pagetable mappings to debugfs

[Nicolin Chen]

This series of patches adds a new mappings node to debugfs for
tegra-smmu driver. The first five patches are all preparational
changes for PATCH-6, based on Thierry's review feedback against
v5.

Changelog
v8:
 * No changes for PATCH 1-4
 * PATCH-5:
 * * bypassed "group->as == as" to fix KMSG bug reported by Dmitry
 * PATCH-6:
 * * changed to use u32 casting for IOVA outputs
 * * squashed Dmitry's change to list all swgroup names in the same
     group_soc since they share the same as pointer
v7: https://lore.kernel.org/linux-iommu/20211208084732.23363-1-nicolinc@nvidia.com/T/
 * Added "Acked-by" from Thierry to PATCH1,4,5
 * No other changes for PATCH1,3,4,5
 * PATCH-2: dropped "s/soc/group_soc" change
 * PATCH-6:
 * * avoided forward declaration
 * * dropped castings in pd_pt_index_iova()
 * * used "'-' : 'S'" for non-secure attribute
 * * changed multi-line outputs to single-line format
v6: https://lore.kernel.org/linux-iommu/20210915043806.GA19185@Asurada-Nvidia/t/
 * Added PATCH1-3 for better naming conventions
 * Added PATCH4-5 to embed previous struct tegra_smmu_group_debug
   into struct tegra_smmu_group
 * Dropped parentheses at SMMU_PTE_ATTR_SHIFT
 * Dropped swgrp->reg print
 * Replaced ptb_reg contents with as->attr and as->pd_dma
 * Added "index" and "count" in the PD entries for readability
 * Removed Dmitry's Tested-by and Reviewed-by for the big change
   from v5 to v6.
v5: https://lore.kernel.org/linux-iommu/20210315203631.24990-1-nicoleotsuka@gmail.com/
 * Fixed a typo in commit message
 * Split a long line into two lines
 * Rearranged variable defines by length
 * Added Tested-by and Reviewed-by from Dmitry
v4: https://lore.kernel.org/lkml/20210315033504.23937-1-nicoleotsuka@gmail.com/
 * Changed %d to %u for unsigned variables
 * Fixed print format mismatch warnings on ARM32
v3: https://lore.kernel.org/linux-iommu/20210315031530.GA15245@Asurada-Nvidia/T/
 * Fixed PHYS and IOVA print formats
 * Changed variables to unsigned int type
 * Changed the table outputs to be compact
v2: https://lore.kernel.org/linux-iommu/20210312010932.GB29926@Asurada-Nvidia/T/
 * Expanded mutex range to the entire function
 * Added as->lock to protect pagetable walkthrough
 * Replaced devm_kzalloc with devm_kcalloc for group_debug
 * Added "PTE RANGE" and "SIZE" columns to group contiguous mappings
 * Dropped as->count check
 * Added WARN_ON when as->count mismatches pd[pd_index]
v1: https://lkml.org/lkml/2020/9/26/70

Nicolin Chen (6):
  iommu/tegra-smmu: Rename struct iommu_group *group to *grp
  iommu/tegra-smmu: Rename tegra_smmu_find_group to
    tegra_smmu_find_group_soc
  iommu/tegra-smmu: Rename struct tegra_smmu_swgroup *group to *swgrp
  iommu/tegra-smmu: Use swgrp pointer instead of swgroup id
  iommu/tegra-smmu: Attach as pointer to tegra_smmu_group
  iommu/tegra-smmu: Add pagetable mappings to debugfs

 drivers/iommu/tegra-smmu.c | 345 ++++++++++++++++++++++++++++++++-----
 1 file changed, 299 insertions(+), 46 deletions(-)

-- 
2.17.1

[PATCH v8 1/6] iommu/tegra-smmu: Rename struct iommu_group *group to *grp

[Nicolin Chen]

There are a few structs using "group" for their pointer instances.
This gets confusing sometimes. The instance of struct iommu_group
is used in local function with an alias "grp", which can separate
it from others.

So this patch simply renames "group" to "grp" as a cleanup.

Acked-by: Thierry Reding <treding@nvidia.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/tegra-smmu.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 8e906504882d..fd9ef08cb7d9 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -24,7 +24,7 @@ struct tegra_smmu_group {
 	struct list_head list;
 	struct tegra_smmu *smmu;
 	const struct tegra_smmu_group_soc *soc;
-	struct iommu_group *group;
+	struct iommu_group *grp;
 	unsigned int swgroup;
 };
 
@@ -911,7 +911,7 @@ static struct iommu_group *tegra_smmu_device_group(struct device *dev)
 	/* Find existing iommu_group associating with swgroup or group_soc */
 	list_for_each_entry(group, &smmu->groups, list)
 		if ((group->swgroup == swgroup) || (soc && group->soc == soc)) {
-			grp = iommu_group_ref_get(group->group);
+			grp = iommu_group_ref_get(group->grp);
 			mutex_unlock(&smmu->lock);
 			return grp;
 		}
@@ -928,23 +928,23 @@ static struct iommu_group *tegra_smmu_device_group(struct device *dev)
 	group->soc = soc;
 
 	if (dev_is_pci(dev))
-		group->group = pci_device_group(dev);
+		group->grp = pci_device_group(dev);
 	else
-		group->group = generic_device_group(dev);
+		group->grp = generic_device_group(dev);
 
-	if (IS_ERR(group->group)) {
+	if (IS_ERR(group->grp)) {
 		devm_kfree(smmu->dev, group);
 		mutex_unlock(&smmu->lock);
 		return NULL;
 	}
 
-	iommu_group_set_iommudata(group->group, group, tegra_smmu_group_release);
+	iommu_group_set_iommudata(group->grp, group, tegra_smmu_group_release);
 	if (soc)
-		iommu_group_set_name(group->group, soc->name);
+		iommu_group_set_name(group->grp, soc->name);
 	list_add_tail(&group->list, &smmu->groups);
 	mutex_unlock(&smmu->lock);
 
-	return group->group;
+	return group->grp;
 }
 
 static int tegra_smmu_of_xlate(struct device *dev,
-- 
2.17.1

[PATCH v8 2/6] iommu/tegra-smmu: Rename tegra_smmu_find_group to tegra_smmu_find_group_soc

[Nicolin Chen]

The existing function tegra_smmu_find_group really finds group->soc
pointer, so naming it "find_group" might not be clear by looking at
it alone. This patch renames it to tegra_smmu_group_soc in order to
disambiguate the use of "group" in this driver.

Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/tegra-smmu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index fd9ef08cb7d9..5628865c04b0 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -872,7 +872,7 @@ static struct iommu_device *tegra_smmu_probe_device(struct device *dev)
 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)
+tegra_smmu_find_group_soc(struct tegra_smmu *smmu, unsigned int swgroup)
 {
 	unsigned int i, j;
 
@@ -904,7 +904,7 @@ static struct iommu_group *tegra_smmu_device_group(struct device *dev)
 	struct iommu_group *grp;
 
 	/* Find group_soc associating with swgroup */
-	soc = tegra_smmu_find_group(smmu, swgroup);
+	soc = tegra_smmu_find_group_soc(smmu, swgroup);
 
 	mutex_lock(&smmu->lock);
 
-- 
2.17.1

[PATCH v8 3/6] iommu/tegra-smmu: Rename struct tegra_smmu_swgroup *group to *swgrp

[Nicolin Chen]

There are both tegra_smmu_swgroup and tegra_smmu_group structs
using "group" for their pointer instances. This gets confusing
to read the driver sometimes.

So this patch renames "group" of struct tegra_smmu_swgroup to
"swgrp" as a cleanup. Also renames its "find" function.

Note that we already have "swgroup" being used for an unsigned
int type variable that is inside struct tegra_smmu_swgroup, so
it's not able to use "swgroup" but only something like "swgrp".

Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/tegra-smmu.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 5628865c04b0..05a386036fce 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -336,35 +336,35 @@ static void tegra_smmu_domain_free(struct iommu_domain *domain)
 }
 
 static const struct tegra_smmu_swgroup *
-tegra_smmu_find_swgroup(struct tegra_smmu *smmu, unsigned int swgroup)
+tegra_smmu_find_swgrp(struct tegra_smmu *smmu, unsigned int swgroup)
 {
-	const struct tegra_smmu_swgroup *group = NULL;
+	const struct tegra_smmu_swgroup *swgrp = NULL;
 	unsigned int i;
 
 	for (i = 0; i < smmu->soc->num_swgroups; i++) {
 		if (smmu->soc->swgroups[i].swgroup == swgroup) {
-			group = &smmu->soc->swgroups[i];
+			swgrp = &smmu->soc->swgroups[i];
 			break;
 		}
 	}
 
-	return group;
+	return swgrp;
 }
 
 static void tegra_smmu_enable(struct tegra_smmu *smmu, unsigned int swgroup,
 			      unsigned int asid)
 {
-	const struct tegra_smmu_swgroup *group;
+	const struct tegra_smmu_swgroup *swgrp;
 	unsigned int i;
 	u32 value;
 
-	group = tegra_smmu_find_swgroup(smmu, swgroup);
-	if (group) {
-		value = smmu_readl(smmu, group->reg);
+	swgrp = tegra_smmu_find_swgrp(smmu, swgroup);
+	if (swgrp) {
+		value = smmu_readl(smmu, swgrp->reg);
 		value &= ~SMMU_ASID_MASK;
 		value |= SMMU_ASID_VALUE(asid);
 		value |= SMMU_ASID_ENABLE;
-		smmu_writel(smmu, value, group->reg);
+		smmu_writel(smmu, value, swgrp->reg);
 	} else {
 		pr_warn("%s group from swgroup %u not found\n", __func__,
 				swgroup);
@@ -387,17 +387,17 @@ static void tegra_smmu_enable(struct tegra_smmu *smmu, unsigned int swgroup,
 static void tegra_smmu_disable(struct tegra_smmu *smmu, unsigned int swgroup,
 			       unsigned int asid)
 {
-	const struct tegra_smmu_swgroup *group;
+	const struct tegra_smmu_swgroup *swgrp;
 	unsigned int i;
 	u32 value;
 
-	group = tegra_smmu_find_swgroup(smmu, swgroup);
-	if (group) {
-		value = smmu_readl(smmu, group->reg);
+	swgrp = tegra_smmu_find_swgrp(smmu, swgroup);
+	if (swgrp) {
+		value = smmu_readl(smmu, swgrp->reg);
 		value &= ~SMMU_ASID_MASK;
 		value |= SMMU_ASID_VALUE(asid);
 		value &= ~SMMU_ASID_ENABLE;
-		smmu_writel(smmu, value, group->reg);
+		smmu_writel(smmu, value, swgrp->reg);
 	}
 
 	for (i = 0; i < smmu->soc->num_clients; i++) {
@@ -1009,11 +1009,11 @@ static int tegra_smmu_swgroups_show(struct seq_file *s, void *data)
 	seq_printf(s, "------------------------\n");
 
 	for (i = 0; i < smmu->soc->num_swgroups; i++) {
-		const struct tegra_smmu_swgroup *group = &smmu->soc->swgroups[i];
+		const struct tegra_smmu_swgroup *swgrp = &smmu->soc->swgroups[i];
 		const char *status;
 		unsigned int asid;
 
-		value = smmu_readl(smmu, group->reg);
+		value = smmu_readl(smmu, swgrp->reg);
 
 		if (value & SMMU_ASID_ENABLE)
 			status = "yes";
@@ -1022,7 +1022,7 @@ static int tegra_smmu_swgroups_show(struct seq_file *s, void *data)
 
 		asid = value & SMMU_ASID_MASK;
 
-		seq_printf(s, "%-9s  %-7s  %#04x\n", group->name, status,
+		seq_printf(s, "%-9s  %-7s  %#04x\n", swgrp->name, status,
 			   asid);
 	}
 
-- 
2.17.1

[PATCH v8 4/6] iommu/tegra-smmu: Use swgrp pointer instead of swgroup id

[Nicolin Chen]

This patch changes in struct tegra_smmu_group to use swgrp
pointer instead of swgroup, as a preparational change for
the "mappings" debugfs feature.

Acked-by: Thierry Reding <treding@nvidia.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/tegra-smmu.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 05a386036fce..532c843eb631 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -24,8 +24,8 @@ struct tegra_smmu_group {
 	struct list_head list;
 	struct tegra_smmu *smmu;
 	const struct tegra_smmu_group_soc *soc;
+	const struct tegra_smmu_swgroup *swgrp;
 	struct iommu_group *grp;
-	unsigned int swgroup;
 };
 
 struct tegra_smmu {
@@ -899,18 +899,22 @@ static struct iommu_group *tegra_smmu_device_group(struct device *dev)
 	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
 	struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
 	const struct tegra_smmu_group_soc *soc;
+	const struct tegra_smmu_swgroup *swgrp;
 	unsigned int swgroup = fwspec->ids[0];
 	struct tegra_smmu_group *group;
 	struct iommu_group *grp;
 
+	/* Find swgrp according to the swgroup id */
+	swgrp = tegra_smmu_find_swgrp(smmu, swgroup);
+
 	/* Find group_soc associating with swgroup */
 	soc = tegra_smmu_find_group_soc(smmu, swgroup);
 
 	mutex_lock(&smmu->lock);
 
-	/* Find existing iommu_group associating with swgroup or group_soc */
+	/* Find existing iommu_group associating with swgrp or group_soc */
 	list_for_each_entry(group, &smmu->groups, list)
-		if ((group->swgroup == swgroup) || (soc && group->soc == soc)) {
+		if ((swgrp && group->swgrp == swgrp) || (soc && group->soc == soc)) {
 			grp = iommu_group_ref_get(group->grp);
 			mutex_unlock(&smmu->lock);
 			return grp;
@@ -923,7 +927,7 @@ static struct iommu_group *tegra_smmu_device_group(struct device *dev)
 	}
 
 	INIT_LIST_HEAD(&group->list);
-	group->swgroup = swgroup;
+	group->swgrp = swgrp;
 	group->smmu = smmu;
 	group->soc = soc;
 
-- 
2.17.1

[PATCH v8 5/6] iommu/tegra-smmu: Attach as pointer to tegra_smmu_group

[Nicolin Chen]

This could ease driver to access corresponding as pointer
when having tegra_smmu_group pointer only, which can help
new mappings debugfs nodes.

Also moving tegra_smmu_find_group_soc() upward, for using
it in new tegra_smmu_attach_as(); and it's better to have
all tegra_smmu_find_* functions together.

Acked-by: Thierry Reding <treding@nvidia.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/tegra-smmu.c | 96 +++++++++++++++++++++++++++++++-------
 1 file changed, 80 insertions(+), 16 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 532c843eb631..454504aa6602 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -25,6 +25,7 @@ struct tegra_smmu_group {
 	struct tegra_smmu *smmu;
 	const struct tegra_smmu_group_soc *soc;
 	const struct tegra_smmu_swgroup *swgrp;
+	struct tegra_smmu_as *as;
 	struct iommu_group *grp;
 };
 
@@ -351,6 +352,19 @@ tegra_smmu_find_swgrp(struct tegra_smmu *smmu, unsigned int swgroup)
 	return swgrp;
 }
 
+static const struct tegra_smmu_group_soc *
+tegra_smmu_find_group_soc(struct tegra_smmu *smmu, unsigned int swgroup)
+{
+	unsigned int i, j;
+
+	for (i = 0; i < smmu->soc->num_groups; i++)
+		for (j = 0; j < smmu->soc->groups[i].num_swgroups; j++)
+			if (smmu->soc->groups[i].swgroups[j] == swgroup)
+				return &smmu->soc->groups[i];
+
+	return NULL;
+}
+
 static void tegra_smmu_enable(struct tegra_smmu *smmu, unsigned int swgroup,
 			      unsigned int asid)
 {
@@ -484,6 +498,59 @@ static void tegra_smmu_as_unprepare(struct tegra_smmu *smmu,
 	mutex_unlock(&smmu->lock);
 }
 
+static void tegra_smmu_attach_as(struct tegra_smmu *smmu,
+				 struct tegra_smmu_as *as,
+				 unsigned int swgroup)
+{
+	const struct tegra_smmu_swgroup *swgrp;
+	struct tegra_smmu_group *group;
+
+	/* Find swgrp according to the swgroup id */
+	swgrp = tegra_smmu_find_swgrp(smmu, swgroup);
+	if (!swgrp)
+		return;
+
+	mutex_lock(&smmu->lock);
+
+	list_for_each_entry(group, &smmu->groups, list) {
+		if (group->swgrp != swgrp)
+			continue;
+		if (group->as == as)
+			break;
+
+		if (group->as)
+			dev_warn(smmu->dev,
+				 "overwriting group->as for swgroup: %s\n", swgrp->name);
+		group->as = as;
+		break;
+	}
+
+	mutex_unlock(&smmu->lock);
+}
+
+static void tegra_smmu_detach_as(struct tegra_smmu *smmu,
+				 unsigned int swgroup)
+{
+	const struct tegra_smmu_swgroup *swgrp;
+	struct tegra_smmu_group *group;
+
+	/* Find swgrp according to the swgroup id */
+	swgrp = tegra_smmu_find_swgrp(smmu, swgroup);
+	if (!swgrp)
+		return;
+
+	mutex_lock(&smmu->lock);
+
+	list_for_each_entry(group, &smmu->groups, list) {
+		if (group->swgrp != swgrp)
+			continue;
+		group->as = NULL;
+		break;
+	}
+
+	mutex_unlock(&smmu->lock);
+}
+
 static int tegra_smmu_attach_dev(struct iommu_domain *domain,
 				 struct device *dev)
 {
@@ -497,11 +564,15 @@ static int tegra_smmu_attach_dev(struct iommu_domain *domain,
 		return -ENOENT;
 
 	for (index = 0; index < fwspec->num_ids; index++) {
+		unsigned int swgroup = fwspec->ids[index];
+
 		err = tegra_smmu_as_prepare(smmu, as);
 		if (err)
 			goto disable;
 
-		tegra_smmu_enable(smmu, fwspec->ids[index], as->id);
+		tegra_smmu_attach_as(smmu, as, swgroup);
+
+		tegra_smmu_enable(smmu, swgroup, as->id);
 	}
 
 	if (index == 0)
@@ -511,7 +582,10 @@ static int tegra_smmu_attach_dev(struct iommu_domain *domain,
 
 disable:
 	while (index--) {
-		tegra_smmu_disable(smmu, fwspec->ids[index], as->id);
+		unsigned int swgroup = fwspec->ids[index];
+
+		tegra_smmu_disable(smmu, swgroup, as->id);
+		tegra_smmu_detach_as(smmu, swgroup);
 		tegra_smmu_as_unprepare(smmu, as);
 	}
 
@@ -529,7 +603,10 @@ static void tegra_smmu_detach_dev(struct iommu_domain *domain, struct device *de
 		return;
 
 	for (index = 0; index < fwspec->num_ids; index++) {
-		tegra_smmu_disable(smmu, fwspec->ids[index], as->id);
+		unsigned int swgroup = fwspec->ids[index];
+
+		tegra_smmu_disable(smmu, swgroup, as->id);
+		tegra_smmu_detach_as(smmu, swgroup);
 		tegra_smmu_as_unprepare(smmu, as);
 	}
 }
@@ -871,19 +948,6 @@ static struct iommu_device *tegra_smmu_probe_device(struct device *dev)
 
 static void tegra_smmu_release_device(struct device *dev) {}
 
-static const struct tegra_smmu_group_soc *
-tegra_smmu_find_group_soc(struct tegra_smmu *smmu, unsigned int swgroup)
-{
-	unsigned int i, j;
-
-	for (i = 0; i < smmu->soc->num_groups; i++)
-		for (j = 0; j < smmu->soc->groups[i].num_swgroups; j++)
-			if (smmu->soc->groups[i].swgroups[j] == swgroup)
-				return &smmu->soc->groups[i];
-
-	return NULL;
-}
-
 static void tegra_smmu_group_release(void *iommu_data)
 {
 	struct tegra_smmu_group *group = iommu_data;
-- 
2.17.1

[PATCH v8 6/6] iommu/tegra-smmu: Add pagetable mappings to debugfs

[Nicolin Chen]

This patch dumps all active mapping entries from pagetable to a
debugfs directory named "mappings".

Part of this patch for listing all swgroup names in a group_soc
is provided by Dmitry Osipenko <digetx@gmail.com>

Attaching an example:

[SWGROUP: xusb_host] [as: (id: 5), (attr: R|W|-), (pd_dma: 0x0000000080005000)]
{
        [index: 1023] 0xf0080007 (count: 52)
        {
                PTE RANGE      | ATTR | PHYS               | IOVA       | SIZE
                [#913 , #913 ] | 0x7  | 0x0000000101fbe000 | 0xfff91000 | 0x1000
                [#914 , #914 ] | 0x7  | 0x0000000101fbd000 | 0xfff92000 | 0x1000
                [#915 , #915 ] | 0x7  | 0x0000000101fbc000 | 0xfff93000 | 0x1000
                [#916 , #916 ] | 0x7  | 0x0000000101fbb000 | 0xfff94000 | 0x1000
                [#921 , #921 ] | 0x7  | 0x00000000fcc02000 | 0xfff99000 | 0x1000
                [#922 , #922 ] | 0x7  | 0x0000000101fb7000 | 0xfff9a000 | 0x1000
                [#923 , #923 ] | 0x7  | 0x0000000101fb5000 | 0xfff9b000 | 0x1000
                [#948 , #948 ] | 0x7  | 0x0000000101fb2000 | 0xfffb4000 | 0x1000
                [#949 , #949 ] | 0x7  | 0x0000000101fb1000 | 0xfffb5000 | 0x1000
                [#950 , #950 ] | 0x7  | 0x0000000101faf000 | 0xfffb6000 | 0x1000
                [#951 , #951 ] | 0x7  | 0x0000000101fae000 | 0xfffb7000 | 0x1000
                [#952 , #952 ] | 0x7  | 0x000000010263d000 | 0xfffb8000 | 0x1000
                [#953 , #953 ] | 0x7  | 0x000000010263c000 | 0xfffb9000 | 0x1000
                [#954 , #954 ] | 0x7  | 0x000000010263b000 | 0xfffba000 | 0x1000
                [#955 , #955 ] | 0x7  | 0x000000010263a000 | 0xfffbb000 | 0x1000
                [#956 , #956 ] | 0x7  | 0x0000000102639000 | 0xfffbc000 | 0x1000
                [#957 , #957 ] | 0x7  | 0x0000000102638000 | 0xfffbd000 | 0x1000
                [#958 , #958 ] | 0x7  | 0x0000000102637000 | 0xfffbe000 | 0x1000
                [#959 , #959 ] | 0x7  | 0x0000000102636000 | 0xfffbf000 | 0x1000
                [#960 , #992 ] | 0x7  | 0x0000000102613000 | 0xfffc0000 | 0x21000
        }
}
Total PDEs: 1, total PTEs: 52

Note that the example above was output after I locally enabled
IOMMU_DOMAIN_DMA, which is not merged to mainline yet due to a
known framebuffer issue.

Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/tegra-smmu.c | 185 +++++++++++++++++++++++++++++++++++++
 1 file changed, 185 insertions(+)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 454504aa6602..cbd1a52f2a9f 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -47,6 +47,7 @@ struct tegra_smmu {
 	struct list_head list;
 
 	struct dentry *debugfs;
+	struct dentry *debugfs_mappings;
 
 	struct iommu_device iommu;	/* IOMMU Core code handle */
 };
@@ -154,6 +155,9 @@ static inline u32 smmu_readl(struct tegra_smmu *smmu, unsigned long offset)
 
 #define SMMU_PDE_ATTR		(SMMU_PDE_READABLE | SMMU_PDE_WRITABLE | \
 				 SMMU_PDE_NONSECURE)
+#define SMMU_PTE_ATTR		(SMMU_PTE_READABLE | SMMU_PTE_WRITABLE | \
+				 SMMU_PTE_NONSECURE)
+#define SMMU_PTE_ATTR_SHIFT	29
 
 static unsigned int iova_pd_index(unsigned long iova)
 {
@@ -165,6 +169,12 @@ static unsigned int iova_pt_index(unsigned long iova)
 	return (iova >> SMMU_PTE_SHIFT) & (SMMU_NUM_PTE - 1);
 }
 
+static unsigned long pd_pt_index_iova(unsigned int pd_index, unsigned int pt_index)
+{
+	return (pd_index & (SMMU_NUM_PDE - 1)) << SMMU_PDE_SHIFT |
+	       (pt_index & (SMMU_NUM_PTE - 1)) << SMMU_PTE_SHIFT;
+}
+
 static bool smmu_dma_addr_valid(struct tegra_smmu *smmu, dma_addr_t addr)
 {
 	addr >>= 12;
@@ -498,6 +508,156 @@ static void tegra_smmu_as_unprepare(struct tegra_smmu *smmu,
 	mutex_unlock(&smmu->lock);
 }
 
+static int tegra_smmu_debugfs_mappings_show(struct seq_file *s, void *data)
+{
+	struct tegra_smmu_group *group = s->private;
+	const struct tegra_smmu_group_soc *soc;
+	const struct tegra_smmu_swgroup *swgrp;
+	struct tegra_smmu_as *as;
+	struct tegra_smmu *smmu;
+	unsigned int pd_index;
+	unsigned int pt_index;
+	unsigned long flags;
+	u64 pte_count = 0;
+	u32 pde_count = 0;
+	u32 *pd, val;
+
+	if (!group || !group->as || !group->swgrp)
+		return 0;
+
+	swgrp = group->swgrp;
+	smmu = group->smmu;
+	soc = group->soc;
+	as = group->as;
+
+	mutex_lock(&smmu->lock);
+
+	val = smmu_readl(smmu, swgrp->reg);
+	if (!(val & SMMU_ASID_ENABLE))
+		goto unlock;
+
+	pd = page_address(as->pd);
+	if (!pd)
+		goto unlock;
+
+	seq_puts(s, "[SWGROUP: ");
+	/* List all the swgroup names in the same group_soc */
+	if (soc) {
+		bool first_swgroup = true;
+		unsigned int i;
+
+		for (i = 0; i < soc->num_swgroups; i++) {
+			swgrp = tegra_smmu_find_swgrp(smmu, soc->swgroups[i]);
+			if (WARN_ON(!swgrp))
+				goto unlock;
+
+			val = smmu_readl(smmu, swgrp->reg);
+			if (!(val & SMMU_ASID_ENABLE))
+				continue;
+
+			if (WARN_ON((val & SMMU_ASID_MASK) != as->id))
+				continue;
+
+			if (first_swgroup)
+				first_swgroup = false;
+			else
+				seq_puts(s, ", ");
+
+			seq_printf(s, "%s", swgrp->name);
+		}
+	} else {
+		WARN_ON((val & SMMU_ASID_MASK) != as->id);
+		seq_printf(s, "%s", swgrp->name);
+	}
+	seq_puts(s, "] ");
+
+	seq_printf(s, "[as: (id: %d), ", as->id);
+	seq_printf(s, "(attr: %c|%c|%c), ",
+		   as->attr & SMMU_PD_READABLE ? 'R' : '-',
+		   as->attr & SMMU_PD_WRITABLE ? 'W' : '-',
+		   as->attr & SMMU_PD_NONSECURE ? '-' : 'S');
+	seq_printf(s, "(pd_dma: %pad)]\n", &as->pd_dma);
+	seq_puts(s, "{\n");
+
+	spin_lock_irqsave(&as->lock, flags);
+
+	for (pd_index = 0; pd_index < SMMU_NUM_PDE; pd_index++) {
+		struct page *pt_page;
+		unsigned int i;
+		u32 *addr;
+
+		/* An empty PDE should not have a pte use count */
+		WARN_ON_ONCE(!pd[pd_index] ^ !as->count[pd_index]);
+
+		/* Skip this empty PDE */
+		if (!pd[pd_index])
+			continue;
+
+		pde_count++;
+		pte_count += as->count[pd_index];
+		seq_printf(s, "\t[index: %u] 0x%x (count: %d)\n",
+			   pd_index, pd[pd_index], as->count[pd_index]);
+		pt_page = as->pts[pd_index];
+		addr = page_address(pt_page);
+
+		seq_puts(s, "\t{\n");
+		seq_printf(s, "\t\t%-14s | %-4s | %-10s%s | %-10s | %-11s\n",
+			   "PTE RANGE", "ATTR",
+			   "PHYS", sizeof(phys_addr_t) > 4 ? "        " : "",
+			   "IOVA", "SIZE");
+		for (pt_index = 0; pt_index < SMMU_NUM_PTE; pt_index += i) {
+			size_t size = SMMU_SIZE_PT;
+			dma_addr_t iova;
+			phys_addr_t pa;
+
+			i = 1;
+
+			if (!addr[pt_index])
+				continue;
+
+			iova = pd_pt_index_iova(pd_index, pt_index);
+			pa = SMMU_PFN_PHYS(addr[pt_index] & ~SMMU_PTE_ATTR);
+
+			/* Check contiguous mappings and increase size */
+			while (pt_index + i < SMMU_NUM_PTE) {
+				dma_addr_t next_iova;
+				phys_addr_t next_pa;
+
+				if (!addr[pt_index + i])
+					break;
+
+				next_iova = pd_pt_index_iova(pd_index, pt_index + i);
+				next_pa = SMMU_PFN_PHYS(addr[pt_index + i] & ~SMMU_PTE_ATTR);
+
+				/* Break at the end of a linear mapping */
+				if ((next_iova - iova != SMMU_SIZE_PT * i) ||
+				    (next_pa - pa != SMMU_SIZE_PT * i))
+					break;
+
+				i++;
+			}
+
+			seq_printf(s, "\t\t[#%-4u, #%-4u] | 0x%-2x | %pa | 0x%-8x | 0x%-9zx\n",
+				   pt_index, pt_index + i - 1,
+				   addr[pt_index] >> SMMU_PTE_ATTR_SHIFT,
+				   &pa, (u32)iova, size * i);
+		}
+		seq_puts(s, "\t}\n");
+	}
+
+	spin_unlock_irqrestore(&as->lock, flags);
+
+	seq_puts(s, "}\n");
+	seq_printf(s, "Total PDEs: %u, total PTEs: %llu\n ", pde_count, pte_count);
+
+unlock:
+	mutex_unlock(&smmu->lock);
+
+	return 0;
+}
+
+DEFINE_SHOW_ATTRIBUTE(tegra_smmu_debugfs_mappings);
+
 static void tegra_smmu_attach_as(struct tegra_smmu *smmu,
 				 struct tegra_smmu_as *as,
 				 unsigned int swgroup)
@@ -522,6 +682,20 @@ static void tegra_smmu_attach_as(struct tegra_smmu *smmu,
 			dev_warn(smmu->dev,
 				 "overwriting group->as for swgroup: %s\n", swgrp->name);
 		group->as = as;
+
+		if (smmu->debugfs_mappings) {
+			const char *name;
+
+			if (group->soc)
+				name = group->soc->name;
+			else
+				name = group->swgrp->name;
+
+			debugfs_create_file(name, 0444,
+					    smmu->debugfs_mappings, group,
+					    &tegra_smmu_debugfs_mappings_fops);
+		}
+
 		break;
 	}
 
@@ -545,6 +719,15 @@ static void tegra_smmu_detach_as(struct tegra_smmu *smmu,
 		if (group->swgrp != swgrp)
 			continue;
 		group->as = NULL;
+
+		if (smmu->debugfs_mappings) {
+			struct dentry *d;
+
+			d = debugfs_lookup(group->swgrp->name,
+					   smmu->debugfs_mappings);
+			debugfs_remove(d);
+		}
+
 		break;
 	}
 
@@ -1137,6 +1320,8 @@ static void tegra_smmu_debugfs_init(struct tegra_smmu *smmu)
 			    &tegra_smmu_swgroups_fops);
 	debugfs_create_file("clients", S_IRUGO, smmu->debugfs, smmu,
 			    &tegra_smmu_clients_fops);
+
+	smmu->debugfs_mappings = debugfs_create_dir("mappings", smmu->debugfs);
 }
 
 static void tegra_smmu_debugfs_exit(struct tegra_smmu *smmu)
-- 
2.17.1

Re: [PATCH v8 6/6] iommu/tegra-smmu: Add pagetable mappings to debugfs

[Dmitry Osipenko]

09.12.2021 10:38, Nicolin Chen пишет:
> @@ -545,6 +719,15 @@ static void tegra_smmu_detach_as(struct tegra_smmu *smmu,
>  		if (group->swgrp != swgrp)
>  			continue;
>  		group->as = NULL;
> +
> +		if (smmu->debugfs_mappings) {

Do we really need this check?

Looks like all debugfs_create_dir() usages in this driver are incorrect,
that function never returns NULL. Please fix this.

> +			struct dentry *d;

The file name is wrong here.

			if (group->soc)
				name = group->soc->name;
			else
				name = group->swgrp->name;

> +			d = debugfs_lookup(group->swgrp->name,
> +					   smmu->debugfs_mappings);
> +			debugfs_remove(d);
> +		}

This now looks problematic to me. You created debugfs file when the
first member of the shared group was attached to AS, now you remove this
file when any device is detached. The shared debugfs file should be
refcounted or something.

Re: [PATCH v8 6/6] iommu/tegra-smmu: Add pagetable mappings to debugfs

[Dmitry Osipenko]

09.12.2021 10:38, Nicolin Chen пишет:
> +static unsigned long pd_pt_index_iova(unsigned int pd_index, unsigned int pt_index)
> +{
> +	return (pd_index & (SMMU_NUM_PDE - 1)) << SMMU_PDE_SHIFT |
> +	       (pt_index & (SMMU_NUM_PTE - 1)) << SMMU_PTE_SHIFT;
> +}

I'd change the return type to u32 here, for consistency.

Re: [PATCH v8 6/6] iommu/tegra-smmu: Add pagetable mappings to debugfs

[Nicolin Chen]

On Thu, Dec 09, 2021 at 05:49:09PM +0300, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
> 
> 
> 09.12.2021 10:38, Nicolin Chen пишет:
> > +static unsigned long pd_pt_index_iova(unsigned int pd_index, unsigned int pt_index)
> > +{
> > +     return (pd_index & (SMMU_NUM_PDE - 1)) << SMMU_PDE_SHIFT |
> > +            (pt_index & (SMMU_NUM_PTE - 1)) << SMMU_PTE_SHIFT;
> > +}
> 
> I'd change the return type to u32 here, for consistency.

The whole file defines iova using "unsigned long", which I see
as the consistency... If we change it to u32, it'd be probably
necessary to change all iova types to u32 too... So I'd rather
keep it "unsigned long" here. If you see a big necessity to do
that, an additional patch would be nicer IMHO.

Re: [PATCH v8 6/6] iommu/tegra-smmu: Add pagetable mappings to debugfs

[Nicolin Chen]

On Thu, Dec 09, 2021 at 05:47:18PM +0300, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
> 
> 
> 09.12.2021 10:38, Nicolin Chen пишет:
> > @@ -545,6 +719,15 @@ static void tegra_smmu_detach_as(struct tegra_smmu *smmu,
> >               if (group->swgrp != swgrp)
> >                       continue;
> >               group->as = NULL;
> > +
> > +             if (smmu->debugfs_mappings) {
> 
> Do we really need this check?
> 
> Looks like all debugfs_create_dir() usages in this driver are incorrect,
> that function never returns NULL. Please fix this.

debugfs_create_dir returns ERR_PTR on failure. So here should be
to check !IS_ERR. Thanks for pointing it out!

> > +                     struct dentry *d;
> 
> The file name is wrong here.
> 
>                         if (group->soc)
>                                 name = group->soc->name;
>                         else
>                                 name = group->swgrp->name;

Yea, I'll add this.

> 
> > +                     d = debugfs_lookup(group->swgrp->name,
> > +                                        smmu->debugfs_mappings);
> > +                     debugfs_remove(d);
> > +             }
> 
> This now looks problematic to me. You created debugfs file when the
> first member of the shared group was attached to AS, now you remove this
> file when any device is detached. The shared debugfs file should be
> refcounted or something.a

Will see how to handle it.

Thanks
Nic

Re: [PATCH v8 6/6] iommu/tegra-smmu: Add pagetable mappings to debugfs

[Dmitry Osipenko]

09.12.2021 22:32, Nicolin Chen пишет:
> On Thu, Dec 09, 2021 at 05:47:18PM +0300, Dmitry Osipenko wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> 09.12.2021 10:38, Nicolin Chen пишет:
>>> @@ -545,6 +719,15 @@ static void tegra_smmu_detach_as(struct tegra_smmu *smmu,
>>>               if (group->swgrp != swgrp)
>>>                       continue;
>>>               group->as = NULL;
>>> +
>>> +             if (smmu->debugfs_mappings) {
>> Do we really need this check?
>>
>> Looks like all debugfs_create_dir() usages in this driver are incorrect,
>> that function never returns NULL. Please fix this.
> debugfs_create_dir returns ERR_PTR on failure. So here should be
> to check !IS_ERR. Thanks for pointing it out!
> 

All debugfs functions handle IS_ERR(). GregKH removes all such checks
all over the kernel. So the check shouldn't be needed at all, please
remove it if it's unneeded or prove that it's needed.

Re: [PATCH v8 6/6] iommu/tegra-smmu: Add pagetable mappings to debugfs

[Dmitry Osipenko]

09.12.2021 22:24, Nicolin Chen пишет:
> On Thu, Dec 09, 2021 at 05:49:09PM +0300, Dmitry Osipenko wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> 09.12.2021 10:38, Nicolin Chen пишет:
>>> +static unsigned long pd_pt_index_iova(unsigned int pd_index, unsigned int pt_index)
>>> +{
>>> +     return (pd_index & (SMMU_NUM_PDE - 1)) << SMMU_PDE_SHIFT |
>>> +            (pt_index & (SMMU_NUM_PTE - 1)) << SMMU_PTE_SHIFT;
>>> +}
>>
>> I'd change the return type to u32 here, for consistency.
> 
> The whole file defines iova using "unsigned long", which I see
> as the consistency... If we change it to u32, it'd be probably
> necessary to change all iova types to u32 too... So I'd rather
> keep it "unsigned long" here. If you see a big necessity to do
> that, an additional patch would be nicer IMHO.
> 

In general IOVA is "unsigned long", of course. But in case of Tegra
SMMU, we know that is always u32.

Alright, I see now that there are other places in the driver code that
use "unsigned long", so need to change it in this patch.

Re: [PATCH v8 6/6] iommu/tegra-smmu: Add pagetable mappings to debugfs

[Nicolin Chen]

On Thu, Dec 09, 2021 at 10:40:42PM +0300, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
> 
> 
> 09.12.2021 22:32, Nicolin Chen пишет:
> > On Thu, Dec 09, 2021 at 05:47:18PM +0300, Dmitry Osipenko wrote:
> >> External email: Use caution opening links or attachments
> >>
> >>
> >> 09.12.2021 10:38, Nicolin Chen пишет:
> >>> @@ -545,6 +719,15 @@ static void tegra_smmu_detach_as(struct tegra_smmu *smmu,
> >>>               if (group->swgrp != swgrp)
> >>>                       continue;
> >>>               group->as = NULL;
> >>> +
> >>> +             if (smmu->debugfs_mappings) {
> >> Do we really need this check?
> >>
> >> Looks like all debugfs_create_dir() usages in this driver are incorrect,
> >> that function never returns NULL. Please fix this.
> > debugfs_create_dir returns ERR_PTR on failure. So here should be
> > to check !IS_ERR. Thanks for pointing it out!
> >
> 
> All debugfs functions handle IS_ERR(). GregKH removes all such checks
> all over the kernel. So the check shouldn't be needed at all, please
> remove it if it's unneeded or prove that it's needed.

debugfs_create_file can handle a NULL parent, but not ERR_PTR one,
and then it puts the new node under the root. So either passing an
ERR_PTR parent or creating orphan nodes here doesn't sound good...

Re: [PATCH v8 6/6] iommu/tegra-smmu: Add pagetable mappings to debugfs

[Nicolin Chen]

On Thu, Dec 09, 2021 at 10:44:25PM +0300, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
> 
> 
> 09.12.2021 22:24, Nicolin Chen пишет:
> > On Thu, Dec 09, 2021 at 05:49:09PM +0300, Dmitry Osipenko wrote:
> >> External email: Use caution opening links or attachments
> >>
> >>
> >> 09.12.2021 10:38, Nicolin Chen пишет:
> >>> +static unsigned long pd_pt_index_iova(unsigned int pd_index, unsigned int pt_index)
> >>> +{
> >>> +     return (pd_index & (SMMU_NUM_PDE - 1)) << SMMU_PDE_SHIFT |
> >>> +            (pt_index & (SMMU_NUM_PTE - 1)) << SMMU_PTE_SHIFT;
> >>> +}
> >>
> >> I'd change the return type to u32 here, for consistency.
> >
> > The whole file defines iova using "unsigned long", which I see
> > as the consistency... If we change it to u32, it'd be probably
> > necessary to change all iova types to u32 too... So I'd rather
> > keep it "unsigned long" here. If you see a big necessity to do
> > that, an additional patch would be nicer IMHO.
> >
> 
> In general IOVA is "unsigned long", of course. But in case of Tegra
> SMMU, we know that is always u32.
> 
> Alright, I see now that there are other places in the driver code that
> use "unsigned long", so need to change it in this patch.

I think we should do that in a separate patch that changes the iova
type in the entire tegra-smmu file. I can add one in this series, if
this makes you happy...

Re: [PATCH v8 6/6] iommu/tegra-smmu: Add pagetable mappings to debugfs

[Dmitry Osipenko]

09.12.2021 22:51, Nicolin Chen пишет:
> On Thu, Dec 09, 2021 at 10:40:42PM +0300, Dmitry Osipenko wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> 09.12.2021 22:32, Nicolin Chen пишет:
>>> On Thu, Dec 09, 2021 at 05:47:18PM +0300, Dmitry Osipenko wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> 09.12.2021 10:38, Nicolin Chen пишет:
>>>>> @@ -545,6 +719,15 @@ static void tegra_smmu_detach_as(struct tegra_smmu *smmu,
>>>>>               if (group->swgrp != swgrp)
>>>>>                       continue;
>>>>>               group->as = NULL;
>>>>> +
>>>>> +             if (smmu->debugfs_mappings) {
>>>> Do we really need this check?
>>>>
>>>> Looks like all debugfs_create_dir() usages in this driver are incorrect,
>>>> that function never returns NULL. Please fix this.
>>> debugfs_create_dir returns ERR_PTR on failure. So here should be
>>> to check !IS_ERR. Thanks for pointing it out!
>>>
>>
>> All debugfs functions handle IS_ERR(). GregKH removes all such checks
>> all over the kernel. So the check shouldn't be needed at all, please
>> remove it if it's unneeded or prove that it's needed.
> 
> debugfs_create_file can handle a NULL parent, but not ERR_PTR one,
> and then it puts the new node under the root. So either passing an
> ERR_PTR parent or creating orphan nodes here doesn't sound good...
> 

What makes you say so? Please show the exact source code that will cause
the problem.

The smmu->debugfs_mappings can't ever be NULL and debugfs_create_file
handles the ERR_PTR [1][2].

[1] https://elixir.bootlin.com/linux/latest/source/fs/debugfs/inode.c#L397

[2] https://elixir.bootlin.com/linux/latest/source/fs/debugfs/inode.c#L330

Re: [PATCH v8 6/6] iommu/tegra-smmu: Add pagetable mappings to debugfs

[Dmitry Osipenko]

09.12.2021 22:54, Nicolin Chen пишет:
> On Thu, Dec 09, 2021 at 10:44:25PM +0300, Dmitry Osipenko wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> 09.12.2021 22:24, Nicolin Chen пишет:
>>> On Thu, Dec 09, 2021 at 05:49:09PM +0300, Dmitry Osipenko wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> 09.12.2021 10:38, Nicolin Chen пишет:
>>>>> +static unsigned long pd_pt_index_iova(unsigned int pd_index, unsigned int pt_index)
>>>>> +{
>>>>> +     return (pd_index & (SMMU_NUM_PDE - 1)) << SMMU_PDE_SHIFT |
>>>>> +            (pt_index & (SMMU_NUM_PTE - 1)) << SMMU_PTE_SHIFT;
>>>>> +}
>>>>
>>>> I'd change the return type to u32 here, for consistency.
>>>
>>> The whole file defines iova using "unsigned long", which I see
>>> as the consistency... If we change it to u32, it'd be probably
>>> necessary to change all iova types to u32 too... So I'd rather
>>> keep it "unsigned long" here. If you see a big necessity to do
>>> that, an additional patch would be nicer IMHO.
>>>
>>
>> In general IOVA is "unsigned long", of course. But in case of Tegra
>> SMMU, we know that is always u32.
>>
>> Alright, I see now that there are other places in the driver code that
>> use "unsigned long", so need to change it in this patch.
> 
> I think we should do that in a separate patch that changes the iova
> type in the entire tegra-smmu file. I can add one in this series, if
> this makes you happy...
> 

Please keep it "unsigned long", no need for extra patches.

Re: [PATCH v8 6/6] iommu/tegra-smmu: Add pagetable mappings to debugfs

[Nicolin Chen]

On Thu, Dec 09, 2021 at 10:58:32PM +0300, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
> 
> 
> 09.12.2021 22:54, Nicolin Chen пишет:
> > On Thu, Dec 09, 2021 at 10:44:25PM +0300, Dmitry Osipenko wrote:
> >> External email: Use caution opening links or attachments
> >>
> >>
> >> 09.12.2021 22:24, Nicolin Chen пишет:
> >>> On Thu, Dec 09, 2021 at 05:49:09PM +0300, Dmitry Osipenko wrote:
> >>>> External email: Use caution opening links or attachments
> >>>>
> >>>>
> >>>> 09.12.2021 10:38, Nicolin Chen пишет:
> >>>>> +static unsigned long pd_pt_index_iova(unsigned int pd_index, unsigned int pt_index)
> >>>>> +{
> >>>>> +     return (pd_index & (SMMU_NUM_PDE - 1)) << SMMU_PDE_SHIFT |
> >>>>> +            (pt_index & (SMMU_NUM_PTE - 1)) << SMMU_PTE_SHIFT;
> >>>>> +}
> >>>>
> >>>> I'd change the return type to u32 here, for consistency.
> >>>
> >>> The whole file defines iova using "unsigned long", which I see
> >>> as the consistency... If we change it to u32, it'd be probably
> >>> necessary to change all iova types to u32 too... So I'd rather
> >>> keep it "unsigned long" here. If you see a big necessity to do
> >>> that, an additional patch would be nicer IMHO.
> >>>
> >>
> >> In general IOVA is "unsigned long", of course. But in case of Tegra
> >> SMMU, we know that is always u32.
> >>
> >> Alright, I see now that there are other places in the driver code that
> >> use "unsigned long", so need to change it in this patch.
> >
> > I think we should do that in a separate patch that changes the iova
> > type in the entire tegra-smmu file. I can add one in this series, if
> > this makes you happy...
> >
> 
> Please keep it "unsigned long", no need for extra patches.

Oh, I guess that "so need to change it in this patch" should be
"so (no) need to change it in this patch" then?

Re: [PATCH v8 6/6] iommu/tegra-smmu: Add pagetable mappings to debugfs

[Dmitry Osipenko]

09.12.2021 23:01, Nicolin Chen пишет:
> On Thu, Dec 09, 2021 at 10:58:32PM +0300, Dmitry Osipenko wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> 09.12.2021 22:54, Nicolin Chen пишет:
>>> On Thu, Dec 09, 2021 at 10:44:25PM +0300, Dmitry Osipenko wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> 09.12.2021 22:24, Nicolin Chen пишет:
>>>>> On Thu, Dec 09, 2021 at 05:49:09PM +0300, Dmitry Osipenko wrote:
>>>>>> External email: Use caution opening links or attachments
>>>>>>
>>>>>>
>>>>>> 09.12.2021 10:38, Nicolin Chen пишет:
>>>>>>> +static unsigned long pd_pt_index_iova(unsigned int pd_index, unsigned int pt_index)
>>>>>>> +{
>>>>>>> +     return (pd_index & (SMMU_NUM_PDE - 1)) << SMMU_PDE_SHIFT |
>>>>>>> +            (pt_index & (SMMU_NUM_PTE - 1)) << SMMU_PTE_SHIFT;
>>>>>>> +}
>>>>>>
>>>>>> I'd change the return type to u32 here, for consistency.
>>>>>
>>>>> The whole file defines iova using "unsigned long", which I see
>>>>> as the consistency... If we change it to u32, it'd be probably
>>>>> necessary to change all iova types to u32 too... So I'd rather
>>>>> keep it "unsigned long" here. If you see a big necessity to do
>>>>> that, an additional patch would be nicer IMHO.
>>>>>
>>>>
>>>> In general IOVA is "unsigned long", of course. But in case of Tegra
>>>> SMMU, we know that is always u32.
>>>>
>>>> Alright, I see now that there are other places in the driver code that
>>>> use "unsigned long", so need to change it in this patch.
>>>
>>> I think we should do that in a separate patch that changes the iova
>>> type in the entire tegra-smmu file. I can add one in this series, if
>>> this makes you happy...
>>>
>>
>> Please keep it "unsigned long", no need for extra patches.
> 
> Oh, I guess that "so need to change it in this patch" should be
> "so (no) need to change it in this patch" then?
> 

Indeed, I missed that typo, sorry.

Re: [PATCH v8 6/6] iommu/tegra-smmu: Add pagetable mappings to debugfs

[Nicolin Chen]

On Thu, Dec 09, 2021 at 10:58:15PM +0300, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
> 
> 
> 09.12.2021 22:51, Nicolin Chen пишет:
> > On Thu, Dec 09, 2021 at 10:40:42PM +0300, Dmitry Osipenko wrote:
> >> External email: Use caution opening links or attachments
> >>
> >>
> >> 09.12.2021 22:32, Nicolin Chen пишет:
> >>> On Thu, Dec 09, 2021 at 05:47:18PM +0300, Dmitry Osipenko wrote:
> >>>> External email: Use caution opening links or attachments
> >>>>
> >>>>
> >>>> 09.12.2021 10:38, Nicolin Chen пишет:
> >>>>> @@ -545,6 +719,15 @@ static void tegra_smmu_detach_as(struct tegra_smmu *smmu,
> >>>>>               if (group->swgrp != swgrp)
> >>>>>                       continue;
> >>>>>               group->as = NULL;
> >>>>> +
> >>>>> +             if (smmu->debugfs_mappings) {
> >>>> Do we really need this check?
> >>>>
> >>>> Looks like all debugfs_create_dir() usages in this driver are incorrect,
> >>>> that function never returns NULL. Please fix this.
> >>> debugfs_create_dir returns ERR_PTR on failure. So here should be
> >>> to check !IS_ERR. Thanks for pointing it out!
> >>>
> >>
> >> All debugfs functions handle IS_ERR(). GregKH removes all such checks
> >> all over the kernel. So the check shouldn't be needed at all, please
> >> remove it if it's unneeded or prove that it's needed.
> >
> > debugfs_create_file can handle a NULL parent, but not ERR_PTR one,
> > and then it puts the new node under the root. So either passing an
> > ERR_PTR parent or creating orphan nodes here doesn't sound good...
> >
> 
> What makes you say so? Please show the exact source code that will cause
> the problem.
> 
> The smmu->debugfs_mappings can't ever be NULL and debugfs_create_file
> handles the ERR_PTR [1][2].

Ah...my tool jumps to start_creating in fs/tracefs/inode.c instead.

Thanks for the reply. I will remove the if line then.