Linux-Tegra Archive on lore.kernel.org
 help / color / Atom feed
From: Nicolin Chen <nicoleotsuka@gmail.com>
To: Robin Murphy <robin.murphy@arm.com>
Cc: thierry.reding@gmail.com, joro@8bytes.org, digetx@gmail.com,
	linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org,
	iommu@lists.linux-foundation.org, jonathanh@nvidia.com
Subject: Re: [PATCH v7 3/3] iommu/tegra-smmu: Add PCI support
Date: Thu, 15 Oct 2020 20:53:47 -0700
Message-ID: <20201016035347.GA28140@Asurada-Nvidia> (raw)
In-Reply-To: <340afbc0-5513-0742-d2d2-1ab908248af3@arm.com>

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

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

Or do you suggest we need an additional check for pci_device_group?

  reply index

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-09 16:19 [PATCH v7 0/3] " Nicolin Chen
2020-10-09 16:19 ` [PATCH v7 1/3] iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev Nicolin Chen
2020-10-09 16:19 ` [PATCH v7 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device() Nicolin Chen
2020-10-09 16:19 ` [PATCH v7 3/3] iommu/tegra-smmu: Add PCI support Nicolin Chen
2020-10-14 17:42   ` Robin Murphy
2020-10-15  4:13     ` Nicolin Chen
2020-10-15  9:55       ` Robin Murphy
2020-10-16  3:53         ` Nicolin Chen [this message]
2020-10-16 14:10           ` Robin Murphy
2020-10-17  1:56             ` Nicolin Chen
2020-10-19 10:55               ` Robin Murphy
2020-11-07  8:38 ` [PATCH v7 0/3] " Nicolin Chen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201016035347.GA28140@Asurada-Nvidia \
    --to=nicoleotsuka@gmail.com \
    --cc=digetx@gmail.com \
    --cc=iommu@lists.linux-foundation.org \
    --cc=jonathanh@nvidia.com \
    --cc=joro@8bytes.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-tegra@vger.kernel.org \
    --cc=robin.murphy@arm.com \
    --cc=thierry.reding@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Tegra Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-tegra/0 linux-tegra/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-tegra linux-tegra/ https://lore.kernel.org/linux-tegra \
		linux-tegra@vger.kernel.org
	public-inbox-index linux-tegra

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-tegra


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git