* [PATCH v5 0/3] iommu/tegra-smmu: Add PCI support @ 2020-10-03 6:59 Nicolin Chen 2020-10-03 6:59 ` [PATCH v5 1/3] iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev Nicolin Chen ` (2 more replies) 0 siblings, 3 replies; 15+ messages in thread From: Nicolin Chen @ 2020-10-03 6:59 UTC (permalink / raw) To: thierry.reding, joro, digetx Cc: vdumpa, jonathanh, linux-tegra, iommu, linux-kernel This series is to add PCI support in tegra-smmu driver. Changelog (Detail in each patch) v4->v5 * PATCH-1 Cleaned two variables inits * PATCH-2 Fixed put() in ->of_xlate() and Updated commit message * PATCH-3 Added Dmitry's Reviewed-by v3->v4 * Dropped helper function * Found another way to get smmu pointer v2->v3 * Replaced with devm_tegra_get_memory_controller * Updated changes by following Dmitry's comments v1->v2 * Added PATCH-1 suggested by Dmitry * Reworked PATCH-2 to unify certain code Nicolin Chen (3): iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev iommu/tegra-smmu: Rework tegra_smmu_probe_device() iommu/tegra-smmu: Add PCI support drivers/iommu/tegra-smmu.c | 183 ++++++++++++------------------------- 1 file changed, 59 insertions(+), 124 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v5 1/3] iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev 2020-10-03 6:59 [PATCH v5 0/3] iommu/tegra-smmu: Add PCI support Nicolin Chen @ 2020-10-03 6:59 ` Nicolin Chen 2020-10-03 14:14 ` Dmitry Osipenko 2020-10-03 6:59 ` [PATCH v5 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device() Nicolin Chen 2020-10-03 6:59 ` [PATCH v5 3/3] iommu/tegra-smmu: Add PCI support Nicolin Chen 2 siblings, 1 reply; 15+ messages in thread From: Nicolin Chen @ 2020-10-03 6:59 UTC (permalink / raw) To: thierry.reding, joro, digetx Cc: vdumpa, jonathanh, linux-tegra, iommu, linux-kernel 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 v4->v5: * Removed "index" and "err" assigning to 0 v3->v4: * Seperated the change, as a cleanup, from the rework patch v1->v3: * N/A drivers/iommu/tegra-smmu.c | 56 ++++++++++++++++---------------------- 1 file changed, 23 insertions(+), 33 deletions(-) diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c index 6a3ecc334481..297d49f3f80e 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; - } + unsigned int index; + int err; - 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]; + unsigned int index; - 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++; } } -- 2.17.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v5 1/3] iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev 2020-10-03 6:59 ` [PATCH v5 1/3] iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev Nicolin Chen @ 2020-10-03 14:14 ` Dmitry Osipenko 0 siblings, 0 replies; 15+ messages in thread From: Dmitry Osipenko @ 2020-10-03 14:14 UTC (permalink / raw) To: Nicolin Chen, thierry.reding, joro Cc: vdumpa, jonathanh, linux-tegra, iommu, linux-kernel 03.10.2020 09:59, Nicolin Chen пишет: > 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> > --- I'm still not highly impressed by seeing the !fwspec check in this patch. But I'm not a maintainer of the SMMU driver, hence will leave it up to Thierry and Joerg to decide whether this is good or needs to be improved. Otherwise this patch is good to me, thanks. I tested it on Nexus 7, which is Tegra30. Reviewed-by: Dmitry Osipenko <digetx@gmail.com> Tested-by: Dmitry Osipenko <digetx@gmail.com> ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v5 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device() 2020-10-03 6:59 [PATCH v5 0/3] iommu/tegra-smmu: Add PCI support Nicolin Chen 2020-10-03 6:59 ` [PATCH v5 1/3] iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev Nicolin Chen @ 2020-10-03 6:59 ` Nicolin Chen 2020-10-03 14:05 ` Dmitry Osipenko ` (2 more replies) 2020-10-03 6:59 ` [PATCH v5 3/3] iommu/tegra-smmu: Add PCI support Nicolin Chen 2 siblings, 3 replies; 15+ messages in thread From: Nicolin Chen @ 2020-10-03 6:59 UTC (permalink / raw) To: thierry.reding, joro, digetx Cc: vdumpa, jonathanh, linux-tegra, iommu, linux-kernel The bus_set_iommu() in tegra_smmu_probe() enumerates all clients to call in tegra_smmu_probe_device() where each client searches its DT node for smmu pointer and swgroup ID, so as to configure an fwspec. But this requires a valid smmu pointer even before mc and smmu drivers are probed. So in tegra_smmu_probe() we added a line of code to fill mc->smmu, marking "a bit of a hack". This works for most of clients in the DTB, however, doesn't work for a client that doesn't exist in DTB, a PCI device for example. Actually, if we return ERR_PTR(-ENODEV) in ->probe_device() when it's called from bus_set_iommu(), iommu core will let everything carry on. Then when a client gets probed, of_iommu_configure() in iommu core will search DTB for swgroup ID and call ->of_xlate() to prepare an fwspec, similar to tegra_smmu_probe_device() and tegra_smmu_configure(). Then it'll call tegra_smmu_probe_device() again, and this time we shall return smmu->iommu pointer properly. So we can get rid of tegra_smmu_find() and tegra_smmu_configure() along with DT polling code by letting the iommu core handle every thing, except a problem that we search iommus property in DTB not only for swgroup ID but also for mc node to get mc->smmu pointer to call dev_iommu_priv_set() and return the smmu->iommu pointer. So we'll need to find another way to get smmu pointer. Referencing the implementation of sun50i-iommu driver, of_xlate() has client's dev pointer, mc node and swgroup ID. This means that we can call dev_iommu_priv_set() in of_xlate() instead, so we can simply get smmu pointer in ->probe_device(). This patch reworks tegra_smmu_probe_device() by: 1) Removing mc->smmu hack in tegra_smmu_probe() so as to return ERR_PTR(-ENODEV) in tegra_smmu_probe_device() during stage of tegra_smmu_probe/tegra_mc_probe(). 2) Moving dev_iommu_priv_set() to of_xlate() so we can get smmu pointer in tegra_smmu_probe_device() to replace DTB polling. 3) Removing tegra_smmu_configure() accordingly since iommu core takes care of it. This also fixes a problem that previously we added all clients to iommu groups before iommu core initializes its default domain: ubuntu@jetson:~$ dmesg | grep iommu platform smmu_benchmark: Adding to iommu group 0 platform 1003000.pcie: Adding to iommu group 1 platform 50000000.host1x: Adding to iommu group 2 platform 57000000.gpu: Adding to iommu group 3 platform 7000c400.i2c: Adding to iommu group 4 platform 7000c500.i2c: Adding to iommu group 4 platform 7000c700.i2c: Adding to iommu group 4 platform 7000d000.i2c: Adding to iommu group 4 iommu: Default domain type: Translated Though it works fine with IOMMU_DOMAIN_UNMANAGED, but will have warnings if switching to IOMMU_DOMAIN_DMA: iommu: Failed to allocate default IOMMU domain of type 0 for group (null) - Falling back to IOMMU_DOMAIN_DMA iommu: Failed to allocate default IOMMU domain of type 0 for group (null) - Falling back to IOMMU_DOMAIN_DMA Now, bypassing the first probe_device() call from bus_set_iommu() fixes the sequence: ubuntu@jetson:~$ dmesg | grep iommu iommu: Default domain type: Translated tegra-i2c 7000c400.i2c: Adding to iommu group 0 tegra-i2c 7000c500.i2c: Adding to iommu group 0 tegra-i2c 7000d000.i2c: Adding to iommu group 0 tegra-pcie 1003000.pcie: Adding to iommu group 1 ... Note that dmesg log above is testing with IOMMU_DOMAIN_UNMANAGED. Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com> --- Changelog v4->v5 * Replaced of_node_put() with put_device() in of_xlate() * Added test result in commit message v3->v4 * Moved dev_iommu_priv_set() to of_xlate() so we don't need to poll DTB for smmu pointer. * Removed the hack in tegra_smmu_probe() by returning ERR_PTR( -ENODEV) in tegra_smmu_probe_device() to let iommu core call in again. * Removed tegra_smmu_find() and tegra_smmu_configure() as iommu core takes care of fwspec. v2->v3 * Used devm_tegra_get_memory_controller() to get mc pointer * Replaced IS_ERR_OR_NULL with IS_ERR in .probe_device() v1->v2 * Replaced in .probe_device() tegra_smmu_find/tegra_smmu_configure() with tegra_get_memory_controller call. * Dropped the hack in tegra_smmu_probe(). drivers/iommu/tegra-smmu.c | 92 +++++--------------------------------- 1 file changed, 11 insertions(+), 81 deletions(-) diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c index 297d49f3f80e..73b091facae0 100644 --- a/drivers/iommu/tegra-smmu.c +++ b/drivers/iommu/tegra-smmu.c @@ -797,75 +797,9 @@ static phys_addr_t tegra_smmu_iova_to_phys(struct iommu_domain *domain, return SMMU_PFN_PHYS(pfn) + SMMU_OFFSET_IN_PAGE(iova); } -static struct tegra_smmu *tegra_smmu_find(struct device_node *np) -{ - struct platform_device *pdev; - struct tegra_mc *mc; - - pdev = of_find_device_by_node(np); - if (!pdev) - return NULL; - - mc = platform_get_drvdata(pdev); - if (!mc) - return NULL; - - return mc->smmu; -} - -static int tegra_smmu_configure(struct tegra_smmu *smmu, struct device *dev, - struct of_phandle_args *args) -{ - const struct iommu_ops *ops = smmu->iommu.ops; - int err; - - err = iommu_fwspec_init(dev, &dev->of_node->fwnode, ops); - if (err < 0) { - dev_err(dev, "failed to initialize fwspec: %d\n", err); - return err; - } - - err = ops->of_xlate(dev, args); - if (err < 0) { - dev_err(dev, "failed to parse SW group ID: %d\n", err); - iommu_fwspec_free(dev); - return err; - } - - return 0; -} - 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; - } - - of_node_put(args.np); - index++; - } + struct tegra_smmu *smmu = dev_iommu_priv_get(dev); if (!smmu) return ERR_PTR(-ENODEV); @@ -873,10 +807,7 @@ static struct iommu_device *tegra_smmu_probe_device(struct device *dev) return &smmu->iommu; } -static void tegra_smmu_release_device(struct device *dev) -{ - dev_iommu_priv_set(dev, NULL); -} +static void tegra_smmu_release_device(struct device *dev) {} static const struct tegra_smmu_group_soc * tegra_smmu_find_group(struct tegra_smmu *smmu, unsigned int swgroup) @@ -953,8 +884,17 @@ static struct iommu_group *tegra_smmu_device_group(struct device *dev) static int tegra_smmu_of_xlate(struct device *dev, struct of_phandle_args *args) { + struct platform_device *iommu_pdev = of_find_device_by_node(args->np); + struct tegra_mc *mc = platform_get_drvdata(iommu_pdev); u32 id = args->args[0]; + put_device(&iommu_pdev->dev); + + if (!mc || !mc->smmu) + return -EPROBE_DEFER; + + dev_iommu_priv_set(dev, mc->smmu); + return iommu_fwspec_add_ids(dev, &id, 1); } @@ -1079,16 +1019,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; - size = BITS_TO_LONGS(soc->num_asids) * sizeof(long); smmu->asids = devm_kzalloc(dev, size, GFP_KERNEL); -- 2.17.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v5 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device() 2020-10-03 6:59 ` [PATCH v5 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device() Nicolin Chen @ 2020-10-03 14:05 ` Dmitry Osipenko 2020-10-04 22:04 ` Nicolin Chen 2020-10-03 14:06 ` Dmitry Osipenko 2020-10-03 14:19 ` Dmitry Osipenko 2 siblings, 1 reply; 15+ messages in thread From: Dmitry Osipenko @ 2020-10-03 14:05 UTC (permalink / raw) To: Nicolin Chen, thierry.reding, joro Cc: vdumpa, jonathanh, linux-tegra, iommu, linux-kernel 03.10.2020 09:59, Nicolin Chen пишет: > ubuntu@jetson:~$ dmesg | grep iommu > iommu: Default domain type: Translated > tegra-i2c 7000c400.i2c: Adding to iommu group 0 > tegra-i2c 7000c500.i2c: Adding to iommu group 0 > tegra-i2c 7000d000.i2c: Adding to iommu group 0 > tegra-pcie 1003000.pcie: Adding to iommu group 1 Could you please explain how you got I2C into IOMMU? Are you testing vanilla upstream kerne? Upstream DT doesn't assign AHB group to I2C controllers, nor to APB DMA controller. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v5 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device() 2020-10-03 14:05 ` Dmitry Osipenko @ 2020-10-04 22:04 ` Nicolin Chen 0 siblings, 0 replies; 15+ messages in thread From: Nicolin Chen @ 2020-10-04 22:04 UTC (permalink / raw) To: Dmitry Osipenko Cc: thierry.reding, joro, vdumpa, jonathanh, linux-tegra, iommu, linux-kernel On Sat, Oct 03, 2020 at 05:05:52PM +0300, Dmitry Osipenko wrote: > 03.10.2020 09:59, Nicolin Chen пишет: > > ubuntu@jetson:~$ dmesg | grep iommu > > iommu: Default domain type: Translated > > tegra-i2c 7000c400.i2c: Adding to iommu group 0 > > tegra-i2c 7000c500.i2c: Adding to iommu group 0 > > tegra-i2c 7000d000.i2c: Adding to iommu group 0 > > tegra-pcie 1003000.pcie: Adding to iommu group 1 > > Could you please explain how you got I2C into IOMMU? > > Are you testing vanilla upstream kerne? Upstream DT doesn't assign AHB > group to I2C controllers, nor to APB DMA controller. I have local DT changes adding iommus property in i2c/pcie nodes to test group_device() and pci device, yet neither of them is in vanilla kernel. The log is merely to show to people the sequence of iommu core prior to clients. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v5 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device() 2020-10-03 6:59 ` [PATCH v5 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device() Nicolin Chen 2020-10-03 14:05 ` Dmitry Osipenko @ 2020-10-03 14:06 ` Dmitry Osipenko 2020-10-04 21:57 ` Nicolin Chen 2020-10-03 14:19 ` Dmitry Osipenko 2 siblings, 1 reply; 15+ messages in thread From: Dmitry Osipenko @ 2020-10-03 14:06 UTC (permalink / raw) To: Nicolin Chen, thierry.reding, joro Cc: vdumpa, jonathanh, linux-tegra, iommu, linux-kernel 03.10.2020 09:59, Nicolin Chen пишет: > static int tegra_smmu_of_xlate(struct device *dev, > struct of_phandle_args *args) > { > + struct platform_device *iommu_pdev = of_find_device_by_node(args->np); > + struct tegra_mc *mc = platform_get_drvdata(iommu_pdev); > u32 id = args->args[0]; > > + put_device(&iommu_pdev->dev); > + > + if (!mc || !mc->smmu) > + return -EPROBE_DEFER; I'm not very excited by seeing code in the patches that can't be explained by the patch authors and will appreciate if you could provide a detailed explanation about why this NULL checking is needed because I think it is unneeded, especially given that other IOMMU drivers don't have such check. I'm asking this question second time now, please don't ignore review comments next time. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v5 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device() 2020-10-03 14:06 ` Dmitry Osipenko @ 2020-10-04 21:57 ` Nicolin Chen 2020-10-05 8:41 ` Dmitry Osipenko 0 siblings, 1 reply; 15+ messages in thread From: Nicolin Chen @ 2020-10-04 21:57 UTC (permalink / raw) To: Dmitry Osipenko Cc: thierry.reding, joro, vdumpa, jonathanh, linux-tegra, iommu, linux-kernel On Sat, Oct 03, 2020 at 05:06:42PM +0300, Dmitry Osipenko wrote: > 03.10.2020 09:59, Nicolin Chen пишет: > > static int tegra_smmu_of_xlate(struct device *dev, > > struct of_phandle_args *args) > > { > > + struct platform_device *iommu_pdev = of_find_device_by_node(args->np); > > + struct tegra_mc *mc = platform_get_drvdata(iommu_pdev); > > u32 id = args->args[0]; > > > > + put_device(&iommu_pdev->dev); > > + > > + if (!mc || !mc->smmu) > > + return -EPROBE_DEFER; > > I'm not very excited by seeing code in the patches that can't be > explained by the patch authors and will appreciate if you could provide > a detailed explanation about why this NULL checking is needed because I > think it is unneeded, especially given that other IOMMU drivers don't > have such check. This function could be called from of_iommu_configure(), which is a part of other driver's probe(). So I think it's safer to have a check. Yet, given mc driver is added to the "arch_initcall" stage, you are probably right that there's no really need at this moment because all clients should be called after mc/smmu are inited. So I'll resend a v6, if that makes you happy. > I'm asking this question second time now, please don't ignore review > comments next time. I think I missed your reply or misunderstood it. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v5 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device() 2020-10-04 21:57 ` Nicolin Chen @ 2020-10-05 8:41 ` Dmitry Osipenko 2020-10-05 10:56 ` Thierry Reding 0 siblings, 1 reply; 15+ messages in thread From: Dmitry Osipenko @ 2020-10-05 8:41 UTC (permalink / raw) To: Nicolin Chen Cc: thierry.reding, joro, vdumpa, jonathanh, linux-tegra, iommu, linux-kernel 05.10.2020 00:57, Nicolin Chen пишет: > On Sat, Oct 03, 2020 at 05:06:42PM +0300, Dmitry Osipenko wrote: >> 03.10.2020 09:59, Nicolin Chen пишет: >>> static int tegra_smmu_of_xlate(struct device *dev, >>> struct of_phandle_args *args) >>> { >>> + struct platform_device *iommu_pdev = of_find_device_by_node(args->np); >>> + struct tegra_mc *mc = platform_get_drvdata(iommu_pdev); >>> u32 id = args->args[0]; >>> >>> + put_device(&iommu_pdev->dev); >>> + >>> + if (!mc || !mc->smmu) >>> + return -EPROBE_DEFER; >> >> I'm not very excited by seeing code in the patches that can't be >> explained by the patch authors and will appreciate if you could provide >> a detailed explanation about why this NULL checking is needed because I >> think it is unneeded, especially given that other IOMMU drivers don't >> have such check. > > This function could be called from of_iommu_configure(), which is > a part of other driver's probe(). So I think it's safer to have a > check. Yet, given mc driver is added to the "arch_initcall" stage, > you are probably right that there's no really need at this moment > because all clients should be called after mc/smmu are inited. So > I'll resend a v6, if that makes you happy. I wanted to get the explanation :) I'm very curious why it's actually needed because I'm not 100% sure whether it's not needed at all. I'd assume that the only possible problem could be if some device is created in parallel with the MC probing and there is no locking that could prevent this in the drivers core. It's not apparent to me whether this situation could happen at all in practice. The MC is created early and at that time everything is sequential, so it's indeed should be safe to remove the check. >> I'm asking this question second time now, please don't ignore review >> comments next time. > > I think I missed your reply or misunderstood it. > Okay! ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v5 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device() 2020-10-05 8:41 ` Dmitry Osipenko @ 2020-10-05 10:56 ` Thierry Reding 2020-10-06 1:14 ` Nicolin Chen 0 siblings, 1 reply; 15+ messages in thread From: Thierry Reding @ 2020-10-05 10:56 UTC (permalink / raw) To: Dmitry Osipenko Cc: Nicolin Chen, joro, vdumpa, jonathanh, linux-tegra, iommu, linux-kernel [-- Attachment #1: Type: text/plain, Size: 3150 bytes --] On Mon, Oct 05, 2020 at 11:41:08AM +0300, Dmitry Osipenko wrote: > 05.10.2020 00:57, Nicolin Chen пишет: > > On Sat, Oct 03, 2020 at 05:06:42PM +0300, Dmitry Osipenko wrote: > >> 03.10.2020 09:59, Nicolin Chen пишет: > >>> static int tegra_smmu_of_xlate(struct device *dev, > >>> struct of_phandle_args *args) > >>> { > >>> + struct platform_device *iommu_pdev = of_find_device_by_node(args->np); > >>> + struct tegra_mc *mc = platform_get_drvdata(iommu_pdev); > >>> u32 id = args->args[0]; > >>> > >>> + put_device(&iommu_pdev->dev); > >>> + > >>> + if (!mc || !mc->smmu) > >>> + return -EPROBE_DEFER; > >> > >> I'm not very excited by seeing code in the patches that can't be > >> explained by the patch authors and will appreciate if you could provide > >> a detailed explanation about why this NULL checking is needed because I > >> think it is unneeded, especially given that other IOMMU drivers don't > >> have such check. > > > > This function could be called from of_iommu_configure(), which is > > a part of other driver's probe(). So I think it's safer to have a > > check. Yet, given mc driver is added to the "arch_initcall" stage, > > you are probably right that there's no really need at this moment > > because all clients should be called after mc/smmu are inited. So > > I'll resend a v6, if that makes you happy. > > I wanted to get the explanation :) I'm very curious why it's actually > needed because I'm not 100% sure whether it's not needed at all. > > I'd assume that the only possible problem could be if some device is > created in parallel with the MC probing and there is no locking that > could prevent this in the drivers core. It's not apparent to me whether > this situation could happen at all in practice. > > The MC is created early and at that time everything is sequential, so > it's indeed should be safe to remove the check. I think I now remember exactly why the "hack" in tegra_smmu_probe() exists. The reason is that the MC driver does this: mc->smmu = tegra_smmu_probe(...); That means that mc->smmu is going to be NULL until tegra_smmu_probe() has finished. But tegra_smmu_probe() calls bus_set_iommu() and that in turn calls ->probe_device(). So the purpose of the "hack" in the tegra_smmu_probe() function was to make sure mc->smmu was available at that point, because, well, it is already known, but we haven't gotten around to storing it yet. ->of_xlate() can theoretically be called as early as right after bus_set_iommu() via of_iommu_configure() if that is called in parallel with tegra_smmu_probe(). I think that's very unlikely, but I'm not 100% sure that it can't happen. In any case, I do agree with Dmitry that we should have a comment here explaining why this is necessary. Even if we're completely certain that this is necessary, it's not obvious and therefore should get that comment. And if we're not certain that it's necessary, it's probably also good to mention that in the comment so that eventually it can be determined or the check removed if it proves to be unnecessary. Thierry [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v5 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device() 2020-10-05 10:56 ` Thierry Reding @ 2020-10-06 1:14 ` Nicolin Chen 0 siblings, 0 replies; 15+ messages in thread From: Nicolin Chen @ 2020-10-06 1:14 UTC (permalink / raw) To: Thierry Reding Cc: Dmitry Osipenko, joro, vdumpa, jonathanh, linux-tegra, iommu, linux-kernel On Mon, Oct 05, 2020 at 12:56:38PM +0200, Thierry Reding wrote: > On Mon, Oct 05, 2020 at 11:41:08AM +0300, Dmitry Osipenko wrote: > > 05.10.2020 00:57, Nicolin Chen пишет: > > > On Sat, Oct 03, 2020 at 05:06:42PM +0300, Dmitry Osipenko wrote: > > >> 03.10.2020 09:59, Nicolin Chen пишет: > > >>> static int tegra_smmu_of_xlate(struct device *dev, > > >>> struct of_phandle_args *args) > > >>> { > > >>> + struct platform_device *iommu_pdev = of_find_device_by_node(args->np); > > >>> + struct tegra_mc *mc = platform_get_drvdata(iommu_pdev); > > >>> u32 id = args->args[0]; > > >>> > > >>> + put_device(&iommu_pdev->dev); > > >>> + > > >>> + if (!mc || !mc->smmu) > > >>> + return -EPROBE_DEFER; > > >> > > >> I'm not very excited by seeing code in the patches that can't be > > >> explained by the patch authors and will appreciate if you could provide > > >> a detailed explanation about why this NULL checking is needed because I > > >> think it is unneeded, especially given that other IOMMU drivers don't > > >> have such check. > > > > > > This function could be called from of_iommu_configure(), which is > > > a part of other driver's probe(). So I think it's safer to have a > > > check. Yet, given mc driver is added to the "arch_initcall" stage, > > > you are probably right that there's no really need at this moment > > > because all clients should be called after mc/smmu are inited. So > > > I'll resend a v6, if that makes you happy. > > > > I wanted to get the explanation :) I'm very curious why it's actually > > needed because I'm not 100% sure whether it's not needed at all. > > > > I'd assume that the only possible problem could be if some device is > > created in parallel with the MC probing and there is no locking that > > could prevent this in the drivers core. It's not apparent to me whether > > this situation could happen at all in practice. > > > > The MC is created early and at that time everything is sequential, so > > it's indeed should be safe to remove the check. > > I think I now remember exactly why the "hack" in tegra_smmu_probe() > exists. The reason is that the MC driver does this: > > mc->smmu = tegra_smmu_probe(...); > > That means that mc->smmu is going to be NULL until tegra_smmu_probe() > has finished. But tegra_smmu_probe() calls bus_set_iommu() and that in > turn calls ->probe_device(). So the purpose of the "hack" in the > tegra_smmu_probe() function was to make sure mc->smmu was available at > that point, because, well, it is already known, but we haven't gotten > around to storing it yet. Yea, that's exactly what I described in the commit message of a later version of this patch. Yet, we can drop it now as a device will probe() and call ->probe_device() again. > ->of_xlate() can theoretically be called as early as right after > bus_set_iommu() via of_iommu_configure() if that is called in parallel > with tegra_smmu_probe(). I think that's very unlikely, but I'm not 100% > sure that it can't happen. As my previous reply to Dmitry above, I don't think it'd happen, given that mc/smmu drivers are inited during arch_initcall stage while all clients should be probed during device_initcall stage, once this rework patch gets merged. > In any case, I do agree with Dmitry that we should have a comment here > explaining why this is necessary. Even if we're completely certain that > this is necessary, it's not obvious and therefore should get that > comment. And if we're not certain that it's necessary, it's probably > also good to mention that in the comment so that eventually it can be > determined or the check removed if it proves to be unnecessary. Well, I have answered him, and also removed the mc/smmu check in v6 already. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v5 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device() 2020-10-03 6:59 ` [PATCH v5 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device() Nicolin Chen 2020-10-03 14:05 ` Dmitry Osipenko 2020-10-03 14:06 ` Dmitry Osipenko @ 2020-10-03 14:19 ` Dmitry Osipenko 2 siblings, 0 replies; 15+ messages in thread From: Dmitry Osipenko @ 2020-10-03 14:19 UTC (permalink / raw) To: Nicolin Chen, thierry.reding, joro Cc: vdumpa, jonathanh, linux-tegra, iommu, linux-kernel 03.10.2020 09:59, Nicolin Chen пишет: > The bus_set_iommu() in tegra_smmu_probe() enumerates all clients > to call in tegra_smmu_probe_device() where each client searches > its DT node for smmu pointer and swgroup ID, so as to configure > an fwspec. But this requires a valid smmu pointer even before mc > and smmu drivers are probed. So in tegra_smmu_probe() we added a > line of code to fill mc->smmu, marking "a bit of a hack". > > This works for most of clients in the DTB, however, doesn't work > for a client that doesn't exist in DTB, a PCI device for example. > > Actually, if we return ERR_PTR(-ENODEV) in ->probe_device() when > it's called from bus_set_iommu(), iommu core will let everything > carry on. Then when a client gets probed, of_iommu_configure() in > iommu core will search DTB for swgroup ID and call ->of_xlate() > to prepare an fwspec, similar to tegra_smmu_probe_device() and > tegra_smmu_configure(). Then it'll call tegra_smmu_probe_device() > again, and this time we shall return smmu->iommu pointer properly. > > So we can get rid of tegra_smmu_find() and tegra_smmu_configure() > along with DT polling code by letting the iommu core handle every > thing, except a problem that we search iommus property in DTB not > only for swgroup ID but also for mc node to get mc->smmu pointer > to call dev_iommu_priv_set() and return the smmu->iommu pointer. > So we'll need to find another way to get smmu pointer. > > Referencing the implementation of sun50i-iommu driver, of_xlate() > has client's dev pointer, mc node and swgroup ID. This means that > we can call dev_iommu_priv_set() in of_xlate() instead, so we can > simply get smmu pointer in ->probe_device(). > > This patch reworks tegra_smmu_probe_device() by: > 1) Removing mc->smmu hack in tegra_smmu_probe() so as to return > ERR_PTR(-ENODEV) in tegra_smmu_probe_device() during stage of > tegra_smmu_probe/tegra_mc_probe(). > 2) Moving dev_iommu_priv_set() to of_xlate() so we can get smmu > pointer in tegra_smmu_probe_device() to replace DTB polling. > 3) Removing tegra_smmu_configure() accordingly since iommu core > takes care of it. > > This also fixes a problem that previously we added all clients to > iommu groups before iommu core initializes its default domain: > ubuntu@jetson:~$ dmesg | grep iommu > platform smmu_benchmark: Adding to iommu group 0 > platform 1003000.pcie: Adding to iommu group 1 > platform 50000000.host1x: Adding to iommu group 2 > platform 57000000.gpu: Adding to iommu group 3 > platform 7000c400.i2c: Adding to iommu group 4 > platform 7000c500.i2c: Adding to iommu group 4 > platform 7000c700.i2c: Adding to iommu group 4 > platform 7000d000.i2c: Adding to iommu group 4 > iommu: Default domain type: Translated > > Though it works fine with IOMMU_DOMAIN_UNMANAGED, but will have > warnings if switching to IOMMU_DOMAIN_DMA: > iommu: Failed to allocate default IOMMU domain of type 0 for > group (null) - Falling back to IOMMU_DOMAIN_DMA > iommu: Failed to allocate default IOMMU domain of type 0 for > group (null) - Falling back to IOMMU_DOMAIN_DMA > > Now, bypassing the first probe_device() call from bus_set_iommu() > fixes the sequence: > ubuntu@jetson:~$ dmesg | grep iommu > iommu: Default domain type: Translated > tegra-i2c 7000c400.i2c: Adding to iommu group 0 > tegra-i2c 7000c500.i2c: Adding to iommu group 0 > tegra-i2c 7000d000.i2c: Adding to iommu group 0 > tegra-pcie 1003000.pcie: Adding to iommu group 1 > ... > > Note that dmesg log above is testing with IOMMU_DOMAIN_UNMANAGED. > > Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com> > --- Everything looks good to me, apart from the very minor pending question about the NULL-checking. Thanks! Reviewed-by: Dmitry Osipenko <digetx@gmail.com> Tested-by: Dmitry Osipenko <digetx@gmail.com> ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v5 3/3] iommu/tegra-smmu: Add PCI support 2020-10-03 6:59 [PATCH v5 0/3] iommu/tegra-smmu: Add PCI support Nicolin Chen 2020-10-03 6:59 ` [PATCH v5 1/3] iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev Nicolin Chen 2020-10-03 6:59 ` [PATCH v5 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device() Nicolin Chen @ 2020-10-03 6:59 ` Nicolin Chen 2020-10-03 14:16 ` Dmitry Osipenko 2 siblings, 1 reply; 15+ messages in thread From: Nicolin Chen @ 2020-10-03 6:59 UTC (permalink / raw) To: thierry.reding, joro, digetx Cc: vdumpa, jonathanh, linux-tegra, iommu, linux-kernel This patch simply adds support for PCI devices. Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com> Reviewed-by: Dmitry Osipenko <digetx@gmail.com> --- Changelog 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 73b091facae0..babab6d51360 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); + else + group->group = generic_device_group(dev); + if (IS_ERR(group->group)) { devm_kfree(smmu->dev, group); mutex_unlock(&smmu->lock); @@ -1071,22 +1076,32 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev, iommu_device_set_fwnode(&smmu->iommu, dev->fwnode); err = iommu_device_register(&smmu->iommu); - if (err) { - iommu_device_sysfs_remove(&smmu->iommu); - return ERR_PTR(err); - } + if (err) + goto err_sysfs; err = bus_set_iommu(&platform_bus_type, &tegra_smmu_ops); - if (err < 0) { - iommu_device_unregister(&smmu->iommu); - iommu_device_sysfs_remove(&smmu->iommu); - return ERR_PTR(err); - } + if (err < 0) + goto err_unregister; + +#ifdef CONFIG_PCI + err = bus_set_iommu(&pci_bus_type, &tegra_smmu_ops); + if (err < 0) + goto err_bus_set; +#endif if (IS_ENABLED(CONFIG_DEBUG_FS)) tegra_smmu_debugfs_init(smmu); return smmu; + +err_bus_set: __maybe_unused; + bus_set_iommu(&platform_bus_type, NULL); +err_unregister: + iommu_device_unregister(&smmu->iommu); +err_sysfs: + iommu_device_sysfs_remove(&smmu->iommu); + + return ERR_PTR(err); } void tegra_smmu_remove(struct tegra_smmu *smmu) -- 2.17.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v5 3/3] iommu/tegra-smmu: Add PCI support 2020-10-03 6:59 ` [PATCH v5 3/3] iommu/tegra-smmu: Add PCI support Nicolin Chen @ 2020-10-03 14:16 ` Dmitry Osipenko 2020-10-04 22:08 ` Nicolin Chen 0 siblings, 1 reply; 15+ messages in thread From: Dmitry Osipenko @ 2020-10-03 14:16 UTC (permalink / raw) To: Nicolin Chen, thierry.reding, joro Cc: vdumpa, jonathanh, linux-tegra, iommu, linux-kernel 03.10.2020 09:59, Nicolin Chen пишет: > This patch simply adds support for PCI devices. > > Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com> > Reviewed-by: Dmitry Osipenko <digetx@gmail.com> Small nit: yours s-b tag always should be the last line of the commit message because you're "signing up" words that were written by you. Tested-by: Dmitry Osipenko <digetx@gmail.com> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v5 3/3] iommu/tegra-smmu: Add PCI support 2020-10-03 14:16 ` Dmitry Osipenko @ 2020-10-04 22:08 ` Nicolin Chen 0 siblings, 0 replies; 15+ messages in thread From: Nicolin Chen @ 2020-10-04 22:08 UTC (permalink / raw) To: Dmitry Osipenko Cc: thierry.reding, joro, vdumpa, jonathanh, linux-tegra, iommu, linux-kernel On Sat, Oct 03, 2020 at 05:16:20PM +0300, Dmitry Osipenko wrote: > 03.10.2020 09:59, Nicolin Chen пишет: > > This patch simply adds support for PCI devices. > > > > Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com> > > Reviewed-by: Dmitry Osipenko <digetx@gmail.com> > > Small nit: yours s-b tag always should be the last line of the commit > message because you're "signing up" words that were written by you. > > Tested-by: Dmitry Osipenko <digetx@gmail.com> OK. Thanks for testing, btw. ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2020-10-06 1:21 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-03 6:59 [PATCH v5 0/3] iommu/tegra-smmu: Add PCI support Nicolin Chen 2020-10-03 6:59 ` [PATCH v5 1/3] iommu/tegra-smmu: Use fwspec in tegra_smmu_(de)attach_dev Nicolin Chen 2020-10-03 14:14 ` Dmitry Osipenko 2020-10-03 6:59 ` [PATCH v5 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device() Nicolin Chen 2020-10-03 14:05 ` Dmitry Osipenko 2020-10-04 22:04 ` Nicolin Chen 2020-10-03 14:06 ` Dmitry Osipenko 2020-10-04 21:57 ` Nicolin Chen 2020-10-05 8:41 ` Dmitry Osipenko 2020-10-05 10:56 ` Thierry Reding 2020-10-06 1:14 ` Nicolin Chen 2020-10-03 14:19 ` Dmitry Osipenko 2020-10-03 6:59 ` [PATCH v5 3/3] iommu/tegra-smmu: Add PCI support Nicolin Chen 2020-10-03 14:16 ` Dmitry Osipenko 2020-10-04 22:08 ` Nicolin Chen
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).