| Message ID | 20201002060807.32138-2-nicoleotsuka@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iommu/tegra-smmu: Add PCI support | expand |
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; Looks like there is no need to initialize 'index' and 'err' variables anymore.
02.10.2020 09:08, Nicolin Chen пишет: > static void tegra_smmu_detach_dev(struct iommu_domain *domain, struct device *dev) > { > + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev); > struct tegra_smmu_as *as = to_smmu_as(domain); > - struct device_node *np = dev->of_node; > struct tegra_smmu *smmu = as->smmu; > - struct of_phandle_args args; > unsigned int index = 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; When !fwspec could be true?
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?
02.10.2020 17:22, Dmitry Osipenko пишет: > 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; > > Looks like there is no need to initialize 'index' and 'err' variables > anymore. > Same for tegra_smmu_detach_dev().
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.
On Fri, Oct 02, 2020 at 05:52:00PM +0300, Dmitry Osipenko wrote: > 02.10.2020 17:22, Dmitry Osipenko пишет: > > 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; > > > > Looks like there is no need to initialize 'index' and 'err' variables > > anymore. > > > > Same for tegra_smmu_detach_dev(). Can remove them.
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.
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.
03.10.2020 02:53, Nicolin Chen пишет: > 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. > I haven't tried to check every code path very thoroughly, expecting you to do it since you're making this patch. Maybe there is a real reason why majority of drivers do the checks and it would be good to know why. Although, it's not critical in this particular case and indeed the checks could be improved later on. It looks to me that at least will be a bit better/cleaner to check the dev_iommu_priv_get() for NULL instead of fwspec because the private variable depends on the fwspec presence and there is a similar check in probe_device, hence checks will be more consistent.
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c index 6a3ecc334481..a573a5151c69 100644 --- a/drivers/iommu/tegra-smmu.c +++ b/drivers/iommu/tegra-smmu.c @@ -484,60 +484,50 @@ static void tegra_smmu_as_unprepare(struct tegra_smmu *smmu, 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; + for (index = 0; index < fwspec->num_ids; index++) { err = tegra_smmu_as_prepare(smmu, as); - if (err < 0) - return err; + if (err) + goto disable; - tegra_smmu_enable(smmu, swgroup, as->id); - index++; + tegra_smmu_enable(smmu, fwspec->ids[index], as->id); } if (index == 0) return -ENODEV; return 0; + +disable: + while (index--) { + tegra_smmu_disable(smmu, fwspec->ids[index], as->id); + tegra_smmu_as_unprepare(smmu, as); + } + + return err; } static void tegra_smmu_detach_dev(struct iommu_domain *domain, struct device *dev) { + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev); struct tegra_smmu_as *as = to_smmu_as(domain); - struct device_node *np = dev->of_node; struct tegra_smmu *smmu = as->smmu; - struct of_phandle_args args; unsigned int index = 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; - tegra_smmu_disable(smmu, swgroup, as->id); + for (index = 0; index < fwspec->num_ids; index++) { + tegra_smmu_disable(smmu, fwspec->ids[index], as->id); tegra_smmu_as_unprepare(smmu, as); - index++; } }
In tegra_smmu_(de)attach_dev() functions, we poll DTB for each client's iommus property to get swgroup ID in order to prepare "as" and enable smmu. Actually tegra_smmu_configure() prepared an fwspec for each client, and added to the fwspec all swgroup IDs of client DT node in DTB. So this patch uses fwspec in tegra_smmu_(de)attach_dev() so as to replace the redundant DT polling code. Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com> --- Changelog v3->v4: * Seperated the change, as a cleanup, from the rework patch v1->v3: * N/A drivers/iommu/tegra-smmu.c | 50 +++++++++++++++----------------------- 1 file changed, 20 insertions(+), 30 deletions(-)