From: Nicolin Chen <nicoleotsuka@gmail.com>
To: Dmitry Osipenko <digetx@gmail.com>
Cc: linux-kernel@vger.kernel.org, iommu@lists.linux-foundation.org,
jonathanh@nvidia.com, thierry.reding@gmail.com,
linux-tegra@vger.kernel.org
Subject: Re: [PATCH v4 1/3] iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev
Date: Fri, 2 Oct 2020 16:53:30 -0700 [thread overview]
Message-ID: <20201002235329.GA11409@Asurada-Nvidia> (raw)
In-Reply-To: <e594374b-d701-fb6f-93f2-4efb9c5eb608@gmail.com>
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.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
WARNING: multiple messages have this Message-ID (diff)
From: Nicolin Chen <nicoleotsuka@gmail.com>
To: Dmitry Osipenko <digetx@gmail.com>
Cc: thierry.reding@gmail.com, joro@8bytes.org, vdumpa@nvidia.com,
jonathanh@nvidia.com, linux-tegra@vger.kernel.org,
iommu@lists.linux-foundation.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 1/3] iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev
Date: Fri, 2 Oct 2020 16:53:30 -0700 [thread overview]
Message-ID: <20201002235329.GA11409@Asurada-Nvidia> (raw)
In-Reply-To: <e594374b-d701-fb6f-93f2-4efb9c5eb608@gmail.com>
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.
next prev parent reply other threads:[~2020-10-02 23:59 UTC|newest]
Thread overview: 92+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-02 6:08 [PATCH v4 0/3] iommu/tegra-smmu: Add PCI support Nicolin Chen
2020-10-02 6:08 ` Nicolin Chen
2020-10-02 6:08 ` [PATCH v4 1/3] iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev Nicolin Chen
2020-10-02 6:08 ` Nicolin Chen
2020-10-02 14:22 ` Dmitry Osipenko
2020-10-02 14:22 ` Dmitry Osipenko
2020-10-02 14:52 ` Dmitry Osipenko
2020-10-02 14:52 ` Dmitry Osipenko
2020-10-02 19:56 ` Nicolin Chen
2020-10-02 19:56 ` Nicolin Chen
2020-10-02 14:26 ` Dmitry Osipenko
2020-10-02 14:26 ` Dmitry Osipenko
2020-10-02 14:41 ` Dmitry Osipenko
2020-10-02 14:41 ` Dmitry Osipenko
2020-10-02 19:45 ` Nicolin Chen
2020-10-02 19:45 ` Nicolin Chen
2020-10-02 20:12 ` Dmitry Osipenko
2020-10-02 20:12 ` Dmitry Osipenko
2020-10-02 23:53 ` Nicolin Chen [this message]
2020-10-02 23:53 ` Nicolin Chen
2020-10-03 4:01 ` Dmitry Osipenko
2020-10-03 4:01 ` Dmitry Osipenko
2020-10-02 6:08 ` [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device() Nicolin Chen
2020-10-02 6:08 ` Nicolin Chen
2020-10-02 14:22 ` Dmitry Osipenko
2020-10-02 14:22 ` Dmitry Osipenko
2020-10-02 14:58 ` Dmitry Osipenko
2020-10-02 14:58 ` Dmitry Osipenko
2020-10-02 19:53 ` Nicolin Chen
2020-10-02 19:53 ` Nicolin Chen
2020-10-05 9:47 ` Thierry Reding
2020-10-05 9:47 ` Thierry Reding
2020-10-02 14:22 ` Dmitry Osipenko
2020-10-02 14:22 ` Dmitry Osipenko
2020-10-02 14:50 ` Dmitry Osipenko
2020-10-02 14:50 ` Dmitry Osipenko
2020-10-05 9:53 ` Thierry Reding
2020-10-05 9:53 ` Thierry Reding
2020-10-05 10:36 ` Dmitry Osipenko
2020-10-05 10:36 ` Dmitry Osipenko
2020-10-05 11:15 ` Thierry Reding
2020-10-05 11:15 ` Thierry Reding
2020-10-05 13:28 ` Dmitry Osipenko
2020-10-05 13:28 ` Dmitry Osipenko
2020-10-05 14:22 ` Thierry Reding
2020-10-05 14:22 ` Thierry Reding
2020-10-05 9:51 ` Thierry Reding
2020-10-05 9:51 ` Thierry Reding
2020-10-02 14:23 ` Dmitry Osipenko
2020-10-02 14:23 ` Dmitry Osipenko
2020-10-02 18:01 ` Nicolin Chen
2020-10-02 18:01 ` Nicolin Chen
2020-10-02 18:20 ` Dmitry Osipenko
2020-10-02 18:20 ` Dmitry Osipenko
2020-10-02 15:02 ` Dmitry Osipenko
2020-10-02 15:02 ` Dmitry Osipenko
2020-10-02 18:58 ` Nicolin Chen
2020-10-02 18:58 ` Nicolin Chen
2020-10-05 9:57 ` Thierry Reding
2020-10-05 9:57 ` Thierry Reding
2020-10-06 1:05 ` Nicolin Chen
2020-10-06 1:05 ` Nicolin Chen
2020-10-08 9:53 ` Thierry Reding
2020-10-08 9:53 ` Thierry Reding
2020-10-08 21:12 ` Nicolin Chen
2020-10-08 21:12 ` Nicolin Chen
2020-10-09 12:25 ` Thierry Reding
2020-10-09 12:25 ` Thierry Reding
2020-10-09 15:54 ` Nicolin Chen
2020-10-09 15:54 ` Nicolin Chen
2020-10-02 15:23 ` Dmitry Osipenko
2020-10-02 15:23 ` Dmitry Osipenko
2020-10-02 16:00 ` Dmitry Osipenko
2020-10-02 16:00 ` Dmitry Osipenko
2020-10-02 16:37 ` Dmitry Osipenko
2020-10-02 16:37 ` Dmitry Osipenko
2020-10-02 16:50 ` Dmitry Osipenko
2020-10-02 16:50 ` Dmitry Osipenko
2020-10-02 6:08 ` [PATCH v4 3/3] iommu/tegra-smmu: Add PCI support Nicolin Chen
2020-10-02 6:08 ` Nicolin Chen
2020-10-02 14:35 ` Dmitry Osipenko
2020-10-02 14:35 ` Dmitry Osipenko
2020-10-02 17:45 ` Nicolin Chen
2020-10-02 17:45 ` Nicolin Chen
2020-10-02 18:04 ` Dmitry Osipenko
2020-10-02 18:04 ` Dmitry Osipenko
2020-10-02 18:04 ` Dmitry Osipenko
2020-10-02 18:04 ` Dmitry Osipenko
2020-10-05 10:04 ` Thierry Reding
2020-10-05 10:04 ` Thierry Reding
2020-10-06 0:54 ` Nicolin Chen
2020-10-06 0:54 ` 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=20201002235329.GA11409@Asurada-Nvidia \
--to=nicoleotsuka@gmail.com \
--cc=digetx@gmail.com \
--cc=iommu@lists.linux-foundation.org \
--cc=jonathanh@nvidia.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-tegra@vger.kernel.org \
--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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.