From: Thierry Reding <thierry.reding@gmail.com>
To: Nicolin Chen <nicoleotsuka@gmail.com>
Cc: joro@8bytes.org, krzk@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 v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev
Date: Thu, 1 Oct 2020 11:47:12 +0200 [thread overview]
Message-ID: <20201001094712.GC3919720@ulmo> (raw)
In-Reply-To: <20200930203618.GC2110@Asurada-Nvidia>
[-- Attachment #1: Type: text/plain, Size: 6370 bytes --]
On Wed, Sep 30, 2020 at 01:36:18PM -0700, Nicolin Chen wrote:
> On Wed, Sep 30, 2020 at 05:31:31PM +0200, Thierry Reding wrote:
> > On Wed, Sep 30, 2020 at 01:42:57AM -0700, Nicolin Chen wrote:
> > > Previously the driver relies on bus_set_iommu() in .probe() to call
> > > in .probe_device() function so each client can poll iommus property
> > > in DTB to configure fwspec via tegra_smmu_configure(). According to
> > > the comments in .probe(), this is a bit of a hack. And this doesn't
> > > work for a client that doesn't exist in DTB, PCI device for example.
> > >
> > > Actually when a device/client gets probed, the of_iommu_configure()
> > > will call in .probe_device() function again, with a prepared fwspec
> > > from of_iommu_configure() that reads the SWGROUP id in DTB as we do
> > > in tegra-smmu driver.
> > >
> > > Additionally, as a new helper devm_tegra_get_memory_controller() is
> > > introduced, there's no need to poll the iommus property in order to
> > > get mc->smmu pointers or SWGROUP id.
> > >
> > > This patch reworks .probe_device() and .attach_dev() by doing:
> > > 1) Using fwspec to get swgroup id in .attach_dev/.dettach_dev()
> > > 2) Removing DT polling code, tegra_smmu_find/tegra_smmu_configure()
> > > 3) Calling devm_tegra_get_memory_controller() in .probe_device()
> > > 4) Also dropping the hack in .probe() that's no longer needed.
> > >
> > > Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
> [...]
> > > 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;
> > > - }
> > > + struct tegra_mc *mc = devm_tegra_get_memory_controller(dev);
> > > + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> >
> > It looks to me like the only reason why you need this new global API is
> > because PCI devices may not have a device tree node with a phandle to
> > the IOMMU. However, SMMU support for PCI will only be enabled if the
> > root complex has an iommus property, right? In that case, can't we
> > simply do something like this:
> >
> > if (dev_is_pci(dev))
> > np = find_host_bridge(dev)->of_node;
> > else
> > np = dev->of_node;
> >
> > ? I'm not sure exactly what find_host_bridge() is called, but I'm pretty
> > sure that exists.
> >
> > Once we have that we can still iterate over the iommus property and do
> > not need to rely on this global variable.
>
> I agree that it'd work. But I was hoping to simplify the code
> here if it's possible. Looks like we have an argument on this
> so I will choose to go with your suggestion above for now.
>
> > > - of_node_put(args.np);
> > > - index++;
> > > - }
> > > + /* An invalid mc pointer means mc and smmu drivers are not ready */
> > > + if (IS_ERR(mc))
> > > + return ERR_PTR(-EPROBE_DEFER);
> > >
> > > - if (!smmu)
> > > + /*
> > > + * IOMMU core allows -ENODEV return to carry on. So bypass any call
> > > + * from bus_set_iommu() during tegra_smmu_probe(), as a device will
> > > + * call in again via of_iommu_configure when fwspec is prepared.
> > > + */
> > > + if (!mc->smmu || !fwspec || fwspec->ops != &tegra_smmu_ops)
> > > return ERR_PTR(-ENODEV);
> > >
> > > - return &smmu->iommu;
> > > + dev_iommu_priv_set(dev, mc->smmu);
> > > +
> > > + return &mc->smmu->iommu;
> > > }
> > >
> > > static void tegra_smmu_release_device(struct device *dev)
> > > @@ -1089,16 +1027,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;
> >
> > I don't think this is going to work. I distinctly remember putting this
> > here because we needed access to this before ->probe_device() had been
> > called for any of the devices.
>
> Do you remember which exact part of code needs to access mc->smmu
> before ->probe_device() is called?
>
> What I understood is that IOMMU core didn't allow ERR_PTR(-ENODEV)
> return value from ->probe_device(), previously ->add_device(), to
> carry on when you added this code/driver:
> commit 8918465163171322c77a19d5258a95f56d89d2e4
> Author: Thierry Reding <treding@nvidia.com>
> Date: Wed Apr 16 09:24:44 2014 +0200
> memory: Add NVIDIA Tegra memory controller support
>
> ..until the core had a change one year later:
> commit 38667f18900afe172a4fe44279b132b4140f920f
> Author: Joerg Roedel <jroedel@suse.de>
> Date: Mon Jun 29 10:16:08 2015 +0200
> iommu: Ignore -ENODEV errors from add_device call-back
>
> As my commit message of this change states, ->probe_device() will
> be called in from both bus_set_iommu() and really_probe() of each
> device through of_iommu_configure() -- the later one initializes
> an fwspec by polling the iommus property in the IOMMU core, same
> as what we do here in tegra-smmu. If this works, we can probably
> drop the hack here and get rid of tegra_smmu_configure().
I think this may have been important primarily for DMA/IOMMU integration
and may not currently exhibit because we don't enable that yet.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2020-10-01 9:47 UTC|newest]
Thread overview: 66+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-30 8:42 [PATCH v3 0/3] iommu/tegra-smmu: Add PCI support Nicolin Chen
2020-09-30 8:42 ` [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller() Nicolin Chen
2020-09-30 9:07 ` Krzysztof Kozlowski
2020-09-30 9:41 ` Nicolin Chen
2020-09-30 10:27 ` Krzysztof Kozlowski
2020-09-30 14:41 ` Dmitry Osipenko
2020-09-30 14:45 ` Krzysztof Kozlowski
2020-09-30 15:22 ` Dmitry Osipenko
2020-09-30 15:23 ` Thierry Reding
2020-09-30 15:27 ` Dmitry Osipenko
2020-09-30 15:53 ` Dmitry Osipenko
2020-09-30 16:03 ` Thierry Reding
2020-09-30 16:06 ` Dmitry Osipenko
2020-09-30 16:15 ` Thierry Reding
2020-09-30 16:26 ` Dmitry Osipenko
2020-09-30 16:38 ` Thierry Reding
2020-09-30 17:32 ` Dmitry Osipenko
2020-09-30 8:42 ` [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev Nicolin Chen
2020-09-30 9:21 ` Krzysztof Kozlowski
2020-09-30 9:40 ` Nicolin Chen
2020-09-30 10:19 ` Krzysztof Kozlowski
2020-09-30 14:41 ` Dmitry Osipenko
2020-09-30 15:09 ` Dmitry Osipenko
2020-09-30 20:51 ` Nicolin Chen
2020-09-30 15:31 ` Thierry Reding
2020-09-30 15:36 ` Dmitry Osipenko
2020-09-30 16:06 ` Thierry Reding
2020-09-30 16:25 ` Dmitry Osipenko
2020-09-30 16:47 ` Thierry Reding
2020-10-01 2:11 ` Dmitry Osipenko
2020-10-01 7:58 ` Thierry Reding
2020-10-01 19:04 ` Dmitry Osipenko
2020-10-05 9:16 ` Thierry Reding
2020-10-05 9:38 ` Dmitry Osipenko
2020-10-05 10:27 ` Thierry Reding
2020-09-30 16:10 ` Thierry Reding
2020-09-30 16:29 ` Dmitry Osipenko
2020-10-01 7:59 ` Thierry Reding
2020-09-30 20:36 ` Nicolin Chen
2020-09-30 21:24 ` Dmitry Osipenko
2020-09-30 21:32 ` Nicolin Chen
2020-09-30 21:56 ` Dmitry Osipenko
2020-10-01 1:26 ` Nicolin Chen
2020-10-01 2:06 ` Dmitry Osipenko
2020-10-01 2:48 ` Nicolin Chen
2020-10-01 4:04 ` Dmitry Osipenko
2020-10-01 10:23 ` Thierry Reding
2020-10-01 11:04 ` Nicolin Chen
2020-10-01 20:33 ` Dmitry Osipenko
2020-10-02 1:07 ` Nicolin Chen
2020-10-02 1:55 ` Dmitry Osipenko
2020-10-02 2:54 ` Nicolin Chen
2020-10-05 7:24 ` Thierry Reding
2020-10-05 7:13 ` Thierry Reding
2020-10-05 8:14 ` Dmitry Osipenko
2020-10-05 9:31 ` Thierry Reding
2020-10-01 9:54 ` Thierry Reding
2020-10-01 9:51 ` Thierry Reding
2020-10-01 10:33 ` Nicolin Chen
2020-10-01 10:42 ` Thierry Reding
2020-10-01 9:47 ` Thierry Reding [this message]
2020-10-01 10:46 ` Thierry Reding
2020-10-02 1:29 ` Nicolin Chen
2020-09-30 8:42 ` [PATCH v3 3/3] iommu/tegra-smmu: Add PCI support Nicolin Chen
2020-09-30 14:53 ` Dmitry Osipenko
2020-09-30 20:03 ` 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=20201001094712.GC3919720@ulmo \
--to=thierry.reding@gmail.com \
--cc=digetx@gmail.com \
--cc=iommu@lists.linux-foundation.org \
--cc=jonathanh@nvidia.com \
--cc=joro@8bytes.org \
--cc=krzk@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-tegra@vger.kernel.org \
--cc=nicoleotsuka@gmail.com \
--cc=vdumpa@nvidia.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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).