[PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()
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. Signed-off-by: Nicolin Chen --- Changelog 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 | 90 -- 1 file changed, 9 insertions(+), 81 deletions(-) diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c index a573a5151c69..02d02b0c55c4 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 kerne
Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()
On Fri, Oct 02, 2020 at 12:53:28PM -0700, Nicolin Chen wrote: > On Fri, Oct 02, 2020 at 05:58:29PM +0300, Dmitry Osipenko wrote: > > 02.10.2020 17:22, Dmitry Osipenko пишет: > > > 02.10.2020 09:08, Nicolin Chen пишет: > > >> -static void tegra_smmu_release_device(struct device *dev) > > >> -{ > > >> -dev_iommu_priv_set(dev, NULL); > > >> -} > > >> +static void tegra_smmu_release_device(struct device *dev) {} > > > > > > Please keep the braces as-is. > > > > > > > I noticed that you borrowed this style from the sun50i-iommu driver, but > > this is a bit unusual coding style for the c files. At least to me it's > > unusual to see header-style function stub in a middle of c file. But > > maybe it's just me. > > I don't see a rule in ./Documentation/process/coding-style.rst > against this, and there're plenty of drivers doing so. If you > feel uncomfortable with this style, you may add a rule to that > doc so everyone will follow :) I also prefer braces on separate lines. Even better would be to just drop this entirely and make ->release_device() optional. At least the following drivers could be cleaned up that way: * fsl-pamu * msm-iommu * sun50i-iommu * tegra-gart * tegra-smmu And it looks like mtk-iommu and mtk-iommu-v1 do only iommu_fwspec_free() in their ->release_device() implementations, but that's already done via iommu_release_device(). Thierry signature.asc Description: PGP signature
Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()
On Fri, Oct 02, 2020 at 05:22:41PM +0300, Dmitry Osipenko wrote: > 02.10.2020 09:08, 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]; > > > > + of_node_put(args->np); > > + > > + if (!mc || !mc->smmu) > > + return -EPROBE_DEFER; > > platform_get_drvdata(NULL) will crash. > > > + dev_iommu_priv_set(dev, mc->smmu); > > I think put_device(mc->dev) is missed here, doesn't it? Yeah, I think we'd need that here, otherwise we'd be leaking a reference. Worse, even, mc->dev is the same device that owns the SMMU, so we're basically incrementing our own reference here and never releasing it. We also need that put_device(mc->dev) in the error case above because we already hold the reference there. Thierry signature.asc Description: PGP signature
Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()
On Fri, Oct 02, 2020 at 05:50:08PM +0300, Dmitry Osipenko wrote: > 02.10.2020 17:22, Dmitry Osipenko пишет: > >> 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]; > >> > >> + of_node_put(args->np); > >> + > >> + if (!mc || !mc->smmu) > >> + return -EPROBE_DEFER; > > platform_get_drvdata(NULL) will crash. > > > > Actually, platform_get_drvdata(NULL) can't happen. I overlooked this. How so? It's technically possible for the iommus property to reference a device tree node for which no platform device will ever be created, in which case of_find_device_by_node() will return NULL. That's very unlikely and perhaps worth just crashing on to make sure it gets fixed immediately. Thierry signature.asc Description: PGP signature
Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()
On Fri, Oct 02, 2020 at 11:58:29AM -0700, Nicolin Chen wrote: > On Fri, Oct 02, 2020 at 06:02:18PM +0300, Dmitry Osipenko wrote: > > 02.10.2020 09:08, 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]; > > > > > > + of_node_put(args->np); > > > > of_find_device_by_node() takes device reference and not the np > > reference. This is a bug, please remove of_node_put(). > > Looks like so. Replacing it with put_device(&iommu_pdev->dev); Putting the put_device() here is wrong, though. You need to make sure you keep a reference to it as long as you keep accessing the data that is owned by it. Like I said earlier, this is a bit weird in this case because we're self-referencing, so iommu_pdev->dev is going to stay around as long as the SMMU is. However, it might be worth to properly track the lifetime anyway just so that the code can serve as a good example of how to do things. If you decide to go for the shortcut and not track this reference properly, then at least you need to add a comment as to why it is safe to do in this case. This ensures that readers are away of the circumstances and don't copy this bad code into a context where the circumstances are different. Thierry signature.asc Description: PGP signature
Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()
05.10.2020 12:53, Thierry Reding пишет: > On Fri, Oct 02, 2020 at 05:50:08PM +0300, Dmitry Osipenko wrote: >> 02.10.2020 17:22, Dmitry Osipenko пишет: 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]; + of_node_put(args->np); + + if (!mc || !mc->smmu) + return -EPROBE_DEFER; >>> platform_get_drvdata(NULL) will crash. >>> >> >> Actually, platform_get_drvdata(NULL) can't happen. I overlooked this. > > How so? It's technically possible for the iommus property to reference a > device tree node for which no platform device will ever be created, in > which case of_find_device_by_node() will return NULL. That's very > unlikely and perhaps worth just crashing on to make sure it gets fixed > immediately. The tegra_smmu_ops are registered from the SMMU driver itself and MC driver sets platform data before SMMU is initialized, hence device is guaranteed to exist and mc can't be NULL.
Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()
On Mon, Oct 05, 2020 at 01:36:55PM +0300, Dmitry Osipenko wrote: > 05.10.2020 12:53, Thierry Reding пишет: > > On Fri, Oct 02, 2020 at 05:50:08PM +0300, Dmitry Osipenko wrote: > >> 02.10.2020 17:22, Dmitry Osipenko пишет: > 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]; > > +of_node_put(args->np); > + > +if (!mc || !mc->smmu) > +return -EPROBE_DEFER; > >>> platform_get_drvdata(NULL) will crash. > >>> > >> > >> Actually, platform_get_drvdata(NULL) can't happen. I overlooked this. > > > > How so? It's technically possible for the iommus property to reference a > > device tree node for which no platform device will ever be created, in > > which case of_find_device_by_node() will return NULL. That's very > > unlikely and perhaps worth just crashing on to make sure it gets fixed > > immediately. > > The tegra_smmu_ops are registered from the SMMU driver itself and MC > driver sets platform data before SMMU is initialized, hence device is > guaranteed to exist and mc can't be NULL. Yes, but that assumes that args->np points to the memory controller's device tree node. It's obviously a mistake to do this, but I don't think anyone will prevent you from doing this: iommus = <&{/chosen} 0>; In that case, since no platform device is created for the /chosen node, iommu_pdev will end up being NULL and platform_get_drvdata() will crash. That said, I'm fine with not adding a check for that. If anyone really does end up messing this up they deserve the crash. I'm still a bit undecided about the mc->smmu check because I haven't convinced myself yet that it can't happen. Thierry signature.asc Description: PGP signature
Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()
05.10.2020 14:15, Thierry Reding пишет: > On Mon, Oct 05, 2020 at 01:36:55PM +0300, Dmitry Osipenko wrote: >> 05.10.2020 12:53, Thierry Reding пишет: >>> On Fri, Oct 02, 2020 at 05:50:08PM +0300, Dmitry Osipenko wrote: 02.10.2020 17:22, Dmitry Osipenko пишет: >> 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]; >> >> +of_node_put(args->np); >> + >> +if (!mc || !mc->smmu) >> +return -EPROBE_DEFER; > platform_get_drvdata(NULL) will crash. > Actually, platform_get_drvdata(NULL) can't happen. I overlooked this. >>> >>> How so? It's technically possible for the iommus property to reference a >>> device tree node for which no platform device will ever be created, in >>> which case of_find_device_by_node() will return NULL. That's very >>> unlikely and perhaps worth just crashing on to make sure it gets fixed >>> immediately. >> >> The tegra_smmu_ops are registered from the SMMU driver itself and MC >> driver sets platform data before SMMU is initialized, hence device is >> guaranteed to exist and mc can't be NULL. > > Yes, but that assumes that args->np points to the memory controller's > device tree node. It's obviously a mistake to do this, but I don't think > anyone will prevent you from doing this: > > iommus = <&{/chosen} 0>; > > In that case, since no platform device is created for the /chosen node, > iommu_pdev will end up being NULL and platform_get_drvdata() will crash. But then Tegra SMMU isn't associated with the device's IOMMU path, and thus, tegra_smmu_of_xlate() won't be invoked for this device. > That said, I'm fine with not adding a check for that. If anyone really > does end up messing this up they deserve the crash. > > I'm still a bit undecided about the mc->smmu check because I haven't > convinced myself yet that it can't happen. For now I can't see any realistic situation where mc->smmu could be NULL.
Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()
On Mon, Oct 05, 2020 at 04:28:53PM +0300, Dmitry Osipenko wrote: > 05.10.2020 14:15, Thierry Reding пишет: > > On Mon, Oct 05, 2020 at 01:36:55PM +0300, Dmitry Osipenko wrote: > >> 05.10.2020 12:53, Thierry Reding пишет: > >>> On Fri, Oct 02, 2020 at 05:50:08PM +0300, Dmitry Osipenko wrote: > 02.10.2020 17:22, Dmitry Osipenko пишет: > >> 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]; > >> > >> + of_node_put(args->np); > >> + > >> + if (!mc || !mc->smmu) > >> + return -EPROBE_DEFER; > > platform_get_drvdata(NULL) will crash. > > > > Actually, platform_get_drvdata(NULL) can't happen. I overlooked this. > >>> > >>> How so? It's technically possible for the iommus property to reference a > >>> device tree node for which no platform device will ever be created, in > >>> which case of_find_device_by_node() will return NULL. That's very > >>> unlikely and perhaps worth just crashing on to make sure it gets fixed > >>> immediately. > >> > >> The tegra_smmu_ops are registered from the SMMU driver itself and MC > >> driver sets platform data before SMMU is initialized, hence device is > >> guaranteed to exist and mc can't be NULL. > > > > Yes, but that assumes that args->np points to the memory controller's > > device tree node. It's obviously a mistake to do this, but I don't think > > anyone will prevent you from doing this: > > > > iommus = <&{/chosen} 0>; > > > > In that case, since no platform device is created for the /chosen node, > > iommu_pdev will end up being NULL and platform_get_drvdata() will crash. > > But then Tegra SMMU isn't associated with the device's IOMMU path, and > thus, tegra_smmu_of_xlate() won't be invoked for this device. Indeed, you're right! It used to be that ops were assigned to the bus without any knowledge about the specific instances that might exist, but nowadays there's struct iommu_device which properly encapsulates all of that, so yeah, I don't think this can ever be NULL. Although that makes me wonder why we aren't going one step further and pass struct iommu_device * into ->of_xlate(), which would avoid the need for us to do the look up once more. Thierry signature.asc Description: PGP signature
Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()
On Mon, Oct 05, 2020 at 11:57:54AM +0200, Thierry Reding wrote: > On Fri, Oct 02, 2020 at 11:58:29AM -0700, Nicolin Chen wrote: > > On Fri, Oct 02, 2020 at 06:02:18PM +0300, Dmitry Osipenko wrote: > > > 02.10.2020 09:08, 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]; > > > > > > > > + of_node_put(args->np); > > > > > > of_find_device_by_node() takes device reference and not the np > > > reference. This is a bug, please remove of_node_put(). > > > > Looks like so. Replacing it with put_device(&iommu_pdev->dev); > > Putting the put_device() here is wrong, though. You need to make sure > you keep a reference to it as long as you keep accessing the data that > is owned by it. I am confused. You said in the other reply (to Dmitry) that we do need to put_device(mc->dev), where mc->dev should be the same as iommu_pdev->dev. But here your comments sounds that we should not put_device at all since ->probe_device/group_device/attach_dev() will use it later. > Like I said earlier, this is a bit weird in this case because we're > self-referencing, so iommu_pdev->dev is going to stay around as long as > the SMMU is. However, it might be worth to properly track the lifetime > anyway just so that the code can serve as a good example of how to do > things. What's this "track-the-lifetime"? > If you decide to go for the shortcut and not track this reference > properly, then at least you need to add a comment as to why it is safe > to do in this case. This ensures that readers are away of the > circumstances and don't copy this bad code into a context where the > circumstances are different. I don't quite get this "shortcut" here either...mind elaborating?
Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()
02.10.2020 09:08, 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]; > > + of_node_put(args->np); > + > + if (!mc || !mc->smmu) > + return -EPROBE_DEFER; platform_get_drvdata(NULL) will crash. > + dev_iommu_priv_set(dev, mc->smmu); I think put_device(mc->dev) is missed here, doesn't it? Why sun50i-iommu driver doesn't have this error-checking? Is it really needed at all?
Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()
02.10.2020 09:08, Nicolin Chen пишет: > -static void tegra_smmu_release_device(struct device *dev) > -{ > - dev_iommu_priv_set(dev, NULL); > -} > +static void tegra_smmu_release_device(struct device *dev) {} Please keep the braces as-is.
Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()
02.10.2020 09:08, Nicolin Chen пишет: > 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); The !smmu can't ever be true now, isn't it? Then please remove it.
Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()
02.10.2020 17:22, Dmitry Osipenko пишет: >> 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]; >> >> +of_node_put(args->np); >> + >> +if (!mc || !mc->smmu) >> +return -EPROBE_DEFER; > platform_get_drvdata(NULL) will crash. > Actually, platform_get_drvdata(NULL) can't happen. I overlooked this.
Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()
02.10.2020 17:22, Dmitry Osipenko пишет: > 02.10.2020 09:08, Nicolin Chen пишет: >> -static void tegra_smmu_release_device(struct device *dev) >> -{ >> -dev_iommu_priv_set(dev, NULL); >> -} >> +static void tegra_smmu_release_device(struct device *dev) {} > > Please keep the braces as-is. > I noticed that you borrowed this style from the sun50i-iommu driver, but this is a bit unusual coding style for the c files. At least to me it's unusual to see header-style function stub in a middle of c file. But maybe it's just me.
Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()
02.10.2020 09:08, 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]; > > + of_node_put(args->np); of_find_device_by_node() takes device reference and not the np reference. This is a bug, please remove of_node_put().
Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()
02.10.2020 09:08, Nicolin Chen пишет: > 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. I don't quite see where IOMMU core calls of_xlate(). Have tried to at least boot-test this patch?
Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()
02.10.2020 18:23, Dmitry Osipenko пишет: > 02.10.2020 09:08, Nicolin Chen пишет: >> 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. > > I don't quite see where IOMMU core calls of_xlate(). > > Have tried to at least boot-test this patch? > I don't see how it ever could work because of_xlate() is only invoked from: fsl_mc_dma_configure()->of_dma_configure_id()->of_iommu_configure() Looks like the tegra_smmu_configure() is still needed. I don't know how sun50i driver could work to be honest. Seems IOMMU is broken on sun50i, but maybe I'm missing something. I added Maxime Ripard to this thread, who is the author of the sun50i-iommu driver.
Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()
02.10.2020 19:00, Dmitry Osipenko пишет: > 02.10.2020 18:23, Dmitry Osipenko пишет: >> 02.10.2020 09:08, Nicolin Chen пишет: >>> 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. >> >> I don't quite see where IOMMU core calls of_xlate(). >> >> Have tried to at least boot-test this patch? >> > > I don't see how it ever could work because of_xlate() is only invoked from: > > fsl_mc_dma_configure()->of_dma_configure_id()->of_iommu_configure() > > Looks like the tegra_smmu_configure() is still needed. > > I don't know how sun50i driver could work to be honest. Seems IOMMU is > broken on sun50i, but maybe I'm missing something. > > I added Maxime Ripard to this thread, who is the author of the > sun50i-iommu driver. > Actually, I now see that the other IOMMU drivers (qcom, exynos, etc) do the same. So obviously I'm missing something and it should work..
Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()
02.10.2020 19:37, Dmitry Osipenko пишет: > 02.10.2020 19:00, Dmitry Osipenko пишет: >> 02.10.2020 18:23, Dmitry Osipenko пишет: >>> 02.10.2020 09:08, Nicolin Chen пишет: 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. >>> >>> I don't quite see where IOMMU core calls of_xlate(). >>> >>> Have tried to at least boot-test this patch? >>> >> >> I don't see how it ever could work because of_xlate() is only invoked from: >> >> fsl_mc_dma_configure()->of_dma_configure_id()->of_iommu_configure() >> >> Looks like the tegra_smmu_configure() is still needed. >> >> I don't know how sun50i driver could work to be honest. Seems IOMMU is >> broken on sun50i, but maybe I'm missing something. >> >> I added Maxime Ripard to this thread, who is the author of the >> sun50i-iommu driver. >> > > Actually, I now see that the other IOMMU drivers (qcom, exynos, etc) do > the same. So obviously I'm missing something and it should work.. > Okay, somehow I was oblivious to that of_dma_configure() invokes of_dma_configure_id(). Should be good :)
Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()
On Fri, Oct 02, 2020 at 05:23:14PM +0300, Dmitry Osipenko wrote: > 02.10.2020 09:08, Nicolin Chen пишет: > > 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); > > The !smmu can't ever be true now, isn't it? Then please remove it. How can you be so sure? Have you read my commit message? The whole point of removing the hack in tegra_smmu_probe() is to return the ERR_PTR(-ENODEV) here. The bus_set_iommu() will call this function when mc->smmu is not assigned it, as it's assigned after we return tegra_smmu_probe() while bus_set_iommu() is still in the middle of the tegra_smmu_probe().
Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()
02.10.2020 21:01, Nicolin Chen пишет: > On Fri, Oct 02, 2020 at 05:23:14PM +0300, Dmitry Osipenko wrote: >> 02.10.2020 09:08, Nicolin Chen пишет: >>> 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); >> >> The !smmu can't ever be true now, isn't it? Then please remove it. > > How can you be so sure? Have you read my commit message? The whole > point of removing the hack in tegra_smmu_probe() is to return the > ERR_PTR(-ENODEV) here. The bus_set_iommu() will call this function > when mc->smmu is not assigned it, as it's assigned after we return > tegra_smmu_probe() while bus_set_iommu() is still in the middle of > the tegra_smmu_probe(). > My bad, I probably missed that was looking at the probe_device(), looks good then.
Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()
On Fri, Oct 02, 2020 at 06:02:18PM +0300, Dmitry Osipenko wrote: > 02.10.2020 09:08, 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]; > > > > + of_node_put(args->np); > > of_find_device_by_node() takes device reference and not the np > reference. This is a bug, please remove of_node_put(). Looks like so. Replacing it with put_device(&iommu_pdev->dev); Thanks
Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()
On Fri, Oct 02, 2020 at 05:58:29PM +0300, Dmitry Osipenko wrote: > 02.10.2020 17:22, Dmitry Osipenko пишет: > > 02.10.2020 09:08, Nicolin Chen пишет: > >> -static void tegra_smmu_release_device(struct device *dev) > >> -{ > >> - dev_iommu_priv_set(dev, NULL); > >> -} > >> +static void tegra_smmu_release_device(struct device *dev) {} > > > > Please keep the braces as-is. > > > > I noticed that you borrowed this style from the sun50i-iommu driver, but > this is a bit unusual coding style for the c files. At least to me it's > unusual to see header-style function stub in a middle of c file. But > maybe it's just me. I don't see a rule in ./Documentation/process/coding-style.rst against this, and there're plenty of drivers doing so. If you feel uncomfortable with this style, you may add a rule to that doc so everyone will follow :)
Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()
On Mon, Oct 05, 2020 at 06:05:46PM -0700, Nicolin Chen wrote: > On Mon, Oct 05, 2020 at 11:57:54AM +0200, Thierry Reding wrote: > > On Fri, Oct 02, 2020 at 11:58:29AM -0700, Nicolin Chen wrote: > > > On Fri, Oct 02, 2020 at 06:02:18PM +0300, Dmitry Osipenko wrote: > > > > 02.10.2020 09:08, 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]; > > > > > > > > > > + of_node_put(args->np); > > > > > > > > of_find_device_by_node() takes device reference and not the np > > > > reference. This is a bug, please remove of_node_put(). > > > > > > Looks like so. Replacing it with put_device(&iommu_pdev->dev); > > > > Putting the put_device() here is wrong, though. You need to make sure > > you keep a reference to it as long as you keep accessing the data that > > is owned by it. > > I am confused. You said in the other reply (to Dmitry) that we do > need to put_device(mc->dev), where mc->dev should be the same as > iommu_pdev->dev. But here your comments sounds that we should not > put_device at all since ->probe_device/group_device/attach_dev() > will use it later. You need to call put_device() at some point to release the reference that you acquired by calling of_find_device_by_node(). If you don't release it, you're leaking the reference and the kernel isn't going to know when it's safe to delete the device. So what I'm saying is that we either release it here, which isn't quite right because we do reference data relating to the device later on. And because it isn't quite right there should be a reason to justify it, which is that the SMMU parent device is the same as the MC, so the reference count isn't strictly necessary. But that's not quite obvious, so highlighting it in a comment makes sense. The other alternative is to not call put_device() here and keep on to the reference as long as you keep using "mc". This might be difficult to implement because it may not be obvious where to release it. I think this is the better alternative, but if it's too complicated to implement it might not be worth it. > > Like I said earlier, this is a bit weird in this case because we're > > self-referencing, so iommu_pdev->dev is going to stay around as long as > > the SMMU is. However, it might be worth to properly track the lifetime > > anyway just so that the code can serve as a good example of how to do > > things. > > What's this "track-the-lifetime"? This basically just means that SMMU needs to ensure that MC stays alive (by holding a reference to it) as long as SMMU uses it. If the last reference to MC is dropped, then the mc pointer and potentially anything that it points to will become dangling. If you were to drop the last reference at this point, then on the next line the mc pointer could already be invalid. That's how it generally works, anyway. What's special about this use- case is that the SMMU and MC are the same device, so it should be safe to omit this additional tracking because the IOMMU tracking should take care of that already. > > If you decide to go for the shortcut and not track this reference > > properly, then at least you need to add a comment as to why it is safe > > to do in this case. This ensures that readers are away of the > > circumstances and don't copy this bad code into a context where the > > circumstances are different. > > I don't quite get this "shortcut" here either...mind elaborating? The shortcut is taking advantage of the knowledge that the SMMU and the MC are the same device and therefore not properly track the MC object. Given that their code is located in different locations, this isn't obvious to the casual reader of the code, so they may assume that this is the normal way to do things. To avoid that, the code should have a comment explaining why that is. Thierry signature.asc Description: PGP signature
Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()
On Thu, Oct 08, 2020 at 11:53:43AM +0200, Thierry Reding wrote: > On Mon, Oct 05, 2020 at 06:05:46PM -0700, Nicolin Chen wrote: > > On Mon, Oct 05, 2020 at 11:57:54AM +0200, Thierry Reding wrote: > > > On Fri, Oct 02, 2020 at 11:58:29AM -0700, Nicolin Chen wrote: > > > > On Fri, Oct 02, 2020 at 06:02:18PM +0300, Dmitry Osipenko wrote: > > > > > 02.10.2020 09:08, 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]; > > > > > > > > > > > > + of_node_put(args->np); > > > > > > > > > > of_find_device_by_node() takes device reference and not the np > > > > > reference. This is a bug, please remove of_node_put(). > > > > > > > > Looks like so. Replacing it with put_device(&iommu_pdev->dev); > > > > > > Putting the put_device() here is wrong, though. You need to make sure > > > you keep a reference to it as long as you keep accessing the data that > > > is owned by it. > > > > I am confused. You said in the other reply (to Dmitry) that we do > > need to put_device(mc->dev), where mc->dev should be the same as > > iommu_pdev->dev. But here your comments sounds that we should not > > put_device at all since ->probe_device/group_device/attach_dev() > > will use it later. > > You need to call put_device() at some point to release the reference > that you acquired by calling of_find_device_by_node(). If you don't > release it, you're leaking the reference and the kernel isn't going to > know when it's safe to delete the device. > > So what I'm saying is that we either release it here, which isn't quite > right because we do reference data relating to the device later on. And I see. A small question here by the way: By looking at other IOMMU drivers that are calling driver_find_device_by_fwnode() function, I found that most of them put_device right after the function call, and dev_get_drvdata() after putting the device.. Feels like they are doing it wrongly? > because it isn't quite right there should be a reason to justify it, > which is that the SMMU parent device is the same as the MC, so the > reference count isn't strictly necessary. But that's not quite obvious, > so highlighting it in a comment makes sense. > > The other alternative is to not call put_device() here and keep on to > the reference as long as you keep using "mc". This might be difficult to > implement because it may not be obvious where to release it. I think > this is the better alternative, but if it's too complicated to implement > it might not be worth it. I feel so too. The dev is got at of_xlate() that does not have an obvious counterpart function. So I'll just remove put_device() and put a line of comments, as you suggested. > > > Like I said earlier, this is a bit weird in this case because we're > > > self-referencing, so iommu_pdev->dev is going to stay around as long as > > > the SMMU is. However, it might be worth to properly track the lifetime > > > anyway just so that the code can serve as a good example of how to do > > > things. > > > > What's this "track-the-lifetime"? > > This basically just means that SMMU needs to ensure that MC stays alive > (by holding a reference to it) as long as SMMU uses it. If the last > reference to MC is dropped, then the mc pointer and potentially anything > that it points to will become dangling. If you were to drop the last > reference at this point, then on the next line the mc pointer could > already be invalid. > > That's how it generally works, anyway. What's special about this use- > case is that the SMMU and MC are the same device, so it should be safe > to omit this additional tracking because the IOMMU tracking should take > care of that already. Okay. > > > If you decide to go for the shortcut and not track this reference > > > properly, then at least you need to add a comment as to why it is safe > > > to do in this case. This ensures that readers are away of the > > > circumstances and don't copy this bad code into a context where the > > > circumstances are different. > > > > I don't quite get this "shortcut" here either...mind elaborating? > > The shortcut is taking advantage of the knowledge that the SMMU and the > MC are the same device and therefore not properly track the MC object. > Given that their code is located in different locations, this isn't > obvious to the casual reader of the code, so they may assume that this > is the normal way to do things. To avoid that, the code should have a > comment explaining why that is. Got it. Thanks!
Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()
On Thu, Oct 08, 2020 at 02:12:10PM -0700, Nicolin Chen wrote: > On Thu, Oct 08, 2020 at 11:53:43AM +0200, Thierry Reding wrote: > > On Mon, Oct 05, 2020 at 06:05:46PM -0700, Nicolin Chen wrote: > > > On Mon, Oct 05, 2020 at 11:57:54AM +0200, Thierry Reding wrote: > > > > On Fri, Oct 02, 2020 at 11:58:29AM -0700, Nicolin Chen wrote: > > > > > On Fri, Oct 02, 2020 at 06:02:18PM +0300, Dmitry Osipenko wrote: > > > > > > 02.10.2020 09:08, 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]; > > > > > > > > > > > > > > + of_node_put(args->np); > > > > > > > > > > > > of_find_device_by_node() takes device reference and not the np > > > > > > reference. This is a bug, please remove of_node_put(). > > > > > > > > > > Looks like so. Replacing it with put_device(&iommu_pdev->dev); > > > > > > > > Putting the put_device() here is wrong, though. You need to make sure > > > > you keep a reference to it as long as you keep accessing the data that > > > > is owned by it. > > > > > > I am confused. You said in the other reply (to Dmitry) that we do > > > need to put_device(mc->dev), where mc->dev should be the same as > > > iommu_pdev->dev. But here your comments sounds that we should not > > > put_device at all since ->probe_device/group_device/attach_dev() > > > will use it later. > > > > You need to call put_device() at some point to release the reference > > that you acquired by calling of_find_device_by_node(). If you don't > > release it, you're leaking the reference and the kernel isn't going to > > know when it's safe to delete the device. > > > > So what I'm saying is that we either release it here, which isn't quite > > right because we do reference data relating to the device later on. And > > I see. A small question here by the way: By looking at other IOMMU > drivers that are calling driver_find_device_by_fwnode() function, > I found that most of them put_device right after the function call, > and dev_get_drvdata() after putting the device.. > > Feels like they are doing it wrongly? Well, like I said this is somewhat academic because these are all referencing the IOMMU that by definition still needs to be around when this code is called, and there's locks in place to ensure these don't go away. So it's not like these drivers are doing it wrong, they're just not doing it pedantically right. > > > because it isn't quite right there should be a reason to justify it, > > which is that the SMMU parent device is the same as the MC, so the > > reference count isn't strictly necessary. But that's not quite obvious, > > so highlighting it in a comment makes sense. > > > > The other alternative is to not call put_device() here and keep on to > > the reference as long as you keep using "mc". This might be difficult to > > implement because it may not be obvious where to release it. I think > > this is the better alternative, but if it's too complicated to implement > > it might not be worth it. > > I feel so too. The dev is got at of_xlate() that does not have an > obvious counterpart function. So I'll just remove put_device() and > put a line of comments, as you suggested. I think you misunderstood. Not calling put_device() would be wrong because that leaks a reference to the SMMU that you can't get back. My suggestion was rather to keep put_device() here, but add a comment as to why it's okay to call the put_device() here, even though you keep using its private data later beyond this point, which typically would be wrong to do. Thierry signature.asc Description: PGP signature
Re: [PATCH v4 2/3] iommu/tegra-smmu: Rework tegra_smmu_probe_device()
On Fri, Oct 09, 2020 at 02:25:56PM +0200, Thierry Reding wrote: > On Thu, Oct 08, 2020 at 02:12:10PM -0700, Nicolin Chen wrote: > > On Thu, Oct 08, 2020 at 11:53:43AM +0200, Thierry Reding wrote: > > > On Mon, Oct 05, 2020 at 06:05:46PM -0700, Nicolin Chen wrote: > > > > On Mon, Oct 05, 2020 at 11:57:54AM +0200, Thierry Reding wrote: > > > > > On Fri, Oct 02, 2020 at 11:58:29AM -0700, Nicolin Chen wrote: > > > > > > On Fri, Oct 02, 2020 at 06:02:18PM +0300, Dmitry Osipenko wrote: > > > > > > > 02.10.2020 09:08, 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]; > > > > > > > > > > > > > > > > + of_node_put(args->np); > > > > > > > > > > > > > > of_find_device_by_node() takes device reference and not the np > > > > > > > reference. This is a bug, please remove of_node_put(). > > > > > > > > > > > > Looks like so. Replacing it with put_device(&iommu_pdev->dev); > > > > > > > > > > Putting the put_device() here is wrong, though. You need to make sure > > > > > you keep a reference to it as long as you keep accessing the data that > > > > > is owned by it. > > > > > > > > I am confused. You said in the other reply (to Dmitry) that we do > > > > need to put_device(mc->dev), where mc->dev should be the same as > > > > iommu_pdev->dev. But here your comments sounds that we should not > > > > put_device at all since ->probe_device/group_device/attach_dev() > > > > will use it later. > > > > > > You need to call put_device() at some point to release the reference > > > that you acquired by calling of_find_device_by_node(). If you don't > > > release it, you're leaking the reference and the kernel isn't going to > > > know when it's safe to delete the device. > > > > > > So what I'm saying is that we either release it here, which isn't quite > > > right because we do reference data relating to the device later on. And > > > > I see. A small question here by the way: By looking at other IOMMU > > drivers that are calling driver_find_device_by_fwnode() function, > > I found that most of them put_device right after the function call, > > and dev_get_drvdata() after putting the device.. > > > > Feels like they are doing it wrongly? > > Well, like I said this is somewhat academic because these are all > referencing the IOMMU that by definition still needs to be around > when this code is called, and there's locks in place to ensure > these don't go away. So it's not like these drivers are doing it > wrong, they're just not doing it pedantically right. > > > > > > because it isn't quite right there should be a reason to justify it, > > > which is that the SMMU parent device is the same as the MC, so the > > > reference count isn't strictly necessary. But that's not quite obvious, > > > so highlighting it in a comment makes sense. > > > > > > The other alternative is to not call put_device() here and keep on to > > > the reference as long as you keep using "mc". This might be difficult to > > > implement because it may not be obvious where to release it. I think > > > this is the better alternative, but if it's too complicated to implement > > > it might not be worth it. > > > > I feel so too. The dev is got at of_xlate() that does not have an > > obvious counterpart function. So I'll just remove put_device() and > > put a line of comments, as you suggested. > > I think you misunderstood. Not calling put_device() would be wrong > because that leaks a reference to the SMMU that you can't get back. My > suggestion was rather to keep put_device() here, but add a comment as to > why it's okay to call the put_device() here, even though you keep using > its private data later beyond this point, which typically would be wrong > to do. I see. Thanks for clarification! Will send v6 soon.