Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 1 Apr 2021 21:19:56 -0700
From:   Nicolin Chen <nicoleotsuka@gmail.com>
To:     Thierry Reding <thierry.reding@gmail.com>
Cc:     joro@8bytes.org, will@kernel.org, digetx@gmail.com,
        vdumpa@nvidia.com, jonathanh@nvidia.com,
        linux-tegra@vger.kernel.org, iommu@lists.linux-foundation.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5] iommu/tegra-smmu: Add pagetable mappings to debugfs
Message-ID: <20210402041954.GB31146@Asurada-Nvidia>
References: <20210315203631.24990-1-nicoleotsuka@gmail.com>
 <YFCTmwpg9pMQqcSu@orome.fritz.box>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YFCTmwpg9pMQqcSu@orome.fritz.box>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

On Tue, Mar 16, 2021 at 12:16:43PM +0100, Thierry Reding wrote:

> > +struct tegra_smmu_group_debug {
> > +	const struct tegra_smmu_swgroup *group;
> > +	void *priv;
> 
> This always stores the address space, so why not make this:
> 
> 	struct tegra_smmu_as *as;
> 
> ? While at it, perhaps throw in a const to make sure we don't modify
> this structure in the debugfs code.

OK. I will try to change that.

> > @@ -334,7 +350,7 @@ 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_swgroup(struct tegra_smmu *smmu, unsigned int swgroup, int *index)
> >  {
> >  	const struct tegra_smmu_swgroup *group = NULL;
> >  	unsigned int i;
> > @@ -342,6 +358,8 @@ tegra_smmu_find_swgroup(struct tegra_smmu *smmu, unsigned int swgroup)
> >  	for (i = 0; i < smmu->soc->num_swgroups; i++) {
> >  		if (smmu->soc->swgroups[i].swgroup == swgroup) {
> >  			group = &smmu->soc->swgroups[i];
> > +			if (index)
> > +				*index = i;
> 
> This doesn't look like the right place for this. And this also makes
> things hard to follow because it passes out-of-band data in the index
> parameter.
> 
> I'm thinking that this could benefit from a bit of refactoring where
> we could for example embed struct tegra_smmu_group_debug into struct
> tegra_smmu_group and then reference that when necessary instead of
> carrying all that data in an orthogonal array. That should also make
> it easier to track this.
> 
> Come to think of it, everything that's currently in your new struct
> tegra_smmu_group_debug would be useful in struct tegra_smmu_group,
> irrespective of debugfs support.

Will try to embed it or see what I can do following the suggestion.

> > +static int tegra_smmu_mappings_show(struct seq_file *s, void *data)
> > +{

> > +	seq_printf(s, "\nSWGROUP: %s\nASID: %d\nreg: 0x%x\n",
> > +		   group->name, as->id, group->reg);
> 
> Is group->reg really that useful here?

Can drop it.

> > +
> > +	smmu_writel(smmu, as->id & 0x7f, SMMU_PTB_ASID);
> > +	ptb_reg = smmu_readl(smmu, SMMU_PTB_DATA);
> > +
> > +	seq_printf(s, "PTB_ASID: 0x%x\nas->pd_dma: %pad\n",
> > +		   ptb_reg, &as->pd_dma);
> 
> This looks somewhat redundant because as->pd_dma is already part of the
> PTB_ASID register value. Instead, perhaps decode the upper bits of that
> register and simply output as->pdma so that we don't duplicate the base
> address of the page table?

That's a good idea. Will change that too.