* [PATCH/RFC] iommu/ipmmu-vmsa: Update ->add_device() return value @ 2016-09-20 12:41 ` Magnus Damm 0 siblings, 0 replies; 6+ messages in thread From: Magnus Damm @ 2016-09-20 12:41 UTC (permalink / raw) To: iommu Cc: linux-renesas-soc, Magnus Damm, laurent.pinchart+renesas, joro, geert+renesas From: Magnus Damm <damm+renesas@opensource.se> Update the IPMMU driver to return -ENODEV when adding devices not hooked up a particular IPMMU instance. Currently the ->add_device() callback implementation in the IPMMU driver returns -ENODEV for devices with no "iommus" property, however the function ipmmu_find_utlbs() may return -EINVAL. This patch updates the ipmmu_find_utlbs() return value to -ENODEV for the case when multiple IPMMU instances exist. That way the code matches the expected behaviour described in the comment of the add_iommu_group() function in iommu.c: /* * We ignore -ENODEV errors for now, as they just mean that the * device is not translated by an IOMMU. We still care about * other errors and fail to initialize when they happen. */ Signed-off-by: Magnus Damm <damm+renesas@opensource.se> --- Applies to next-20160920 on top of: b1e2afc iommu/ipmmu-vmsa: Fix wrong error handle of ipmmu_add_device drivers/iommu/ipmmu-vmsa.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- 0002/drivers/iommu/ipmmu-vmsa.c +++ work/drivers/iommu/ipmmu-vmsa.c 2016-09-08 18:20:06.270607110 +0900 @@ -781,7 +781,7 @@ static int ipmmu_find_utlbs(struct ipmmu of_node_put(args.np); if (args.np != mmu->dev->of_node || args.args_count != 1) - return -EINVAL; + return -ENODEV; utlbs[i] = args.args[0]; } ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH/RFC] iommu/ipmmu-vmsa: Update ->add_device() return value @ 2016-09-20 12:41 ` Magnus Damm 0 siblings, 0 replies; 6+ messages in thread From: Magnus Damm @ 2016-09-20 12:41 UTC (permalink / raw) To: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA Cc: linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA, laurent.pinchart+renesas-ryLnwIuWjnjg/C1BVhZhaw, Magnus Damm, geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ From: Magnus Damm <damm+renesas-yzvPICuk2ACczHhG9Qg4qA@public.gmane.org> Update the IPMMU driver to return -ENODEV when adding devices not hooked up a particular IPMMU instance. Currently the ->add_device() callback implementation in the IPMMU driver returns -ENODEV for devices with no "iommus" property, however the function ipmmu_find_utlbs() may return -EINVAL. This patch updates the ipmmu_find_utlbs() return value to -ENODEV for the case when multiple IPMMU instances exist. That way the code matches the expected behaviour described in the comment of the add_iommu_group() function in iommu.c: /* * We ignore -ENODEV errors for now, as they just mean that the * device is not translated by an IOMMU. We still care about * other errors and fail to initialize when they happen. */ Signed-off-by: Magnus Damm <damm+renesas-yzvPICuk2ACczHhG9Qg4qA@public.gmane.org> --- Applies to next-20160920 on top of: b1e2afc iommu/ipmmu-vmsa: Fix wrong error handle of ipmmu_add_device drivers/iommu/ipmmu-vmsa.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- 0002/drivers/iommu/ipmmu-vmsa.c +++ work/drivers/iommu/ipmmu-vmsa.c 2016-09-08 18:20:06.270607110 +0900 @@ -781,7 +781,7 @@ static int ipmmu_find_utlbs(struct ipmmu of_node_put(args.np); if (args.np != mmu->dev->of_node || args.args_count != 1) - return -EINVAL; + return -ENODEV; utlbs[i] = args.args[0]; } ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH/RFC] iommu/ipmmu-vmsa: Update ->add_device() return value 2016-09-20 12:41 ` Magnus Damm @ 2016-09-20 13:18 ` Robin Murphy -1 siblings, 0 replies; 6+ messages in thread From: Robin Murphy @ 2016-09-20 13:18 UTC (permalink / raw) To: Magnus Damm, iommu Cc: linux-renesas-soc, laurent.pinchart+renesas, geert+renesas Hi Magnus, On 20/09/16 13:41, Magnus Damm wrote: > From: Magnus Damm <damm+renesas@opensource.se> > > Update the IPMMU driver to return -ENODEV when adding devices > not hooked up a particular IPMMU instance. > > Currently the ->add_device() callback implementation in the IPMMU > driver returns -ENODEV for devices with no "iommus" property, > however the function ipmmu_find_utlbs() may return -EINVAL. If there were no "iommus" property at all, of_parse_phandle_with_args() should return -ENOENT - that probably does want to be caught and passed back as -ENODEV to imply an untranslated device. On the other hand, -EINVAL would stem from the existence of the property, but in a somehow erroneous manner - other than the "args.np != mmu->dev->of_node" check (which could legitimately fail and be safely ignored if there are multiple IOMMUs in the system), any other reason implies a DT error which probably shouldn't be papered over. Robin. > This patch updates the ipmmu_find_utlbs() return value to -ENODEV > for the case when multiple IPMMU instances exist. That way the > code matches the expected behaviour described in the comment of > the add_iommu_group() function in iommu.c: > > /* > * We ignore -ENODEV errors for now, as they just mean that the > * device is not translated by an IOMMU. We still care about > * other errors and fail to initialize when they happen. > */ > > Signed-off-by: Magnus Damm <damm+renesas@opensource.se> > --- > > Applies to next-20160920 on top of: > b1e2afc iommu/ipmmu-vmsa: Fix wrong error handle of ipmmu_add_device > > drivers/iommu/ipmmu-vmsa.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > --- 0002/drivers/iommu/ipmmu-vmsa.c > +++ work/drivers/iommu/ipmmu-vmsa.c 2016-09-08 18:20:06.270607110 +0900 > @@ -781,7 +781,7 @@ static int ipmmu_find_utlbs(struct ipmmu > of_node_put(args.np); > > if (args.np != mmu->dev->of_node || args.args_count != 1) > - return -EINVAL; > + return -ENODEV; > > utlbs[i] = args.args[0]; > } > _______________________________________________ > iommu mailing list > iommu@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/iommu > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH/RFC] iommu/ipmmu-vmsa: Update ->add_device() return value @ 2016-09-20 13:18 ` Robin Murphy 0 siblings, 0 replies; 6+ messages in thread From: Robin Murphy @ 2016-09-20 13:18 UTC (permalink / raw) To: Magnus Damm, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA Cc: linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA, laurent.pinchart+renesas-ryLnwIuWjnjg/C1BVhZhaw, geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ Hi Magnus, On 20/09/16 13:41, Magnus Damm wrote: > From: Magnus Damm <damm+renesas-yzvPICuk2ACczHhG9Qg4qA@public.gmane.org> > > Update the IPMMU driver to return -ENODEV when adding devices > not hooked up a particular IPMMU instance. > > Currently the ->add_device() callback implementation in the IPMMU > driver returns -ENODEV for devices with no "iommus" property, > however the function ipmmu_find_utlbs() may return -EINVAL. If there were no "iommus" property at all, of_parse_phandle_with_args() should return -ENOENT - that probably does want to be caught and passed back as -ENODEV to imply an untranslated device. On the other hand, -EINVAL would stem from the existence of the property, but in a somehow erroneous manner - other than the "args.np != mmu->dev->of_node" check (which could legitimately fail and be safely ignored if there are multiple IOMMUs in the system), any other reason implies a DT error which probably shouldn't be papered over. Robin. > This patch updates the ipmmu_find_utlbs() return value to -ENODEV > for the case when multiple IPMMU instances exist. That way the > code matches the expected behaviour described in the comment of > the add_iommu_group() function in iommu.c: > > /* > * We ignore -ENODEV errors for now, as they just mean that the > * device is not translated by an IOMMU. We still care about > * other errors and fail to initialize when they happen. > */ > > Signed-off-by: Magnus Damm <damm+renesas-yzvPICuk2ACczHhG9Qg4qA@public.gmane.org> > --- > > Applies to next-20160920 on top of: > b1e2afc iommu/ipmmu-vmsa: Fix wrong error handle of ipmmu_add_device > > drivers/iommu/ipmmu-vmsa.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > --- 0002/drivers/iommu/ipmmu-vmsa.c > +++ work/drivers/iommu/ipmmu-vmsa.c 2016-09-08 18:20:06.270607110 +0900 > @@ -781,7 +781,7 @@ static int ipmmu_find_utlbs(struct ipmmu > of_node_put(args.np); > > if (args.np != mmu->dev->of_node || args.args_count != 1) > - return -EINVAL; > + return -ENODEV; > > utlbs[i] = args.args[0]; > } > _______________________________________________ > iommu mailing list > iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org > https://lists.linuxfoundation.org/mailman/listinfo/iommu > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH/RFC] iommu/ipmmu-vmsa: Update ->add_device() return value 2016-09-20 13:18 ` Robin Murphy (?) @ 2016-09-20 15:03 ` Magnus Damm 2016-09-22 15:56 ` Robin Murphy -1 siblings, 1 reply; 6+ messages in thread From: Magnus Damm @ 2016-09-20 15:03 UTC (permalink / raw) To: Robin Murphy; +Cc: iommu, Linux-Renesas, Laurent Pinchart, Geert Uytterhoeven Hi Robin, Thanks for your feedback!! On Tue, Sep 20, 2016 at 10:18 PM, Robin Murphy <robin.murphy@arm.com> wrote: > Hi Magnus, > > On 20/09/16 13:41, Magnus Damm wrote: >> From: Magnus Damm <damm+renesas@opensource.se> >> >> Update the IPMMU driver to return -ENODEV when adding devices >> not hooked up a particular IPMMU instance. >> >> Currently the ->add_device() callback implementation in the IPMMU >> driver returns -ENODEV for devices with no "iommus" property, >> however the function ipmmu_find_utlbs() may return -EINVAL. > > If there were no "iommus" property at all, of_parse_phandle_with_args() > should return -ENOENT - that probably does want to be caught and passed > back as -ENODEV to imply an untranslated device. On the other hand, > -EINVAL would stem from the existence of the property, but in a somehow > erroneous manner - other than the "args.np != mmu->dev->of_node" check > (which could legitimately fail and be safely ignored if there are > multiple IOMMUs in the system), any other reason implies a DT error > which probably shouldn't be papered over. Regarding -ENOENT to -ENODEV, I agree but I believe this case is already handled. The ->add_device() callback seems to be using of_count_phandle_with_args() to check for the presence of "iommus" property before calling ipmmu_find_utlbs(). So for the case with no "iommus" property at all it is returned as -ENODEV as you suggest. The ->add_device() callback has the ret variable initialised to -ENODEV by default. In case there is only one IPMMU device on the ipmmu_device list and it matches the struct device passed to the ipmmu_add_device() function then all is fine. However when there are more than one IPMMU device on the ipmmu_device list then ipmmu_find_utlbs() may return -EINVAL. Judging by the code this seems to happen when the order of the IPMMU devices on the "iommus" property does not match the IPMMU order on the ipmmu_device list. Hm, I wonder if all this should be replaced with ->xlate() somehow? Thanks, / magnus ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH/RFC] iommu/ipmmu-vmsa: Update ->add_device() return value 2016-09-20 15:03 ` Magnus Damm @ 2016-09-22 15:56 ` Robin Murphy 0 siblings, 0 replies; 6+ messages in thread From: Robin Murphy @ 2016-09-22 15:56 UTC (permalink / raw) To: Magnus Damm; +Cc: iommu, Linux-Renesas, Laurent Pinchart, Geert Uytterhoeven On 20/09/16 16:03, Magnus Damm wrote: > Hi Robin, > > Thanks for your feedback!! > > On Tue, Sep 20, 2016 at 10:18 PM, Robin Murphy <robin.murphy@arm.com> wrote: >> Hi Magnus, >> >> On 20/09/16 13:41, Magnus Damm wrote: >>> From: Magnus Damm <damm+renesas@opensource.se> >>> >>> Update the IPMMU driver to return -ENODEV when adding devices >>> not hooked up a particular IPMMU instance. >>> >>> Currently the ->add_device() callback implementation in the IPMMU >>> driver returns -ENODEV for devices with no "iommus" property, >>> however the function ipmmu_find_utlbs() may return -EINVAL. >> >> If there were no "iommus" property at all, of_parse_phandle_with_args() >> should return -ENOENT - that probably does want to be caught and passed >> back as -ENODEV to imply an untranslated device. On the other hand, >> -EINVAL would stem from the existence of the property, but in a somehow >> erroneous manner - other than the "args.np != mmu->dev->of_node" check >> (which could legitimately fail and be safely ignored if there are >> multiple IOMMUs in the system), any other reason implies a DT error >> which probably shouldn't be papered over. > > Regarding -ENOENT to -ENODEV, I agree but I believe this case is > already handled. The ->add_device() callback seems to be using > of_count_phandle_with_args() to check for the presence of "iommus" > property before calling ipmmu_find_utlbs(). So for the case with no > "iommus" property at all it is returned as -ENODEV as you suggest. Ah, right you are, I missed that there was a separate check earlier. > The ->add_device() callback has the ret variable initialised to > -ENODEV by default. In case there is only one IPMMU device on the > ipmmu_device list and it matches the struct device passed to the > ipmmu_add_device() function then all is fine. However when there are > more than one IPMMU device on the ipmmu_device list then > ipmmu_find_utlbs() may return -EINVAL. Judging by the code this seems > to happen when the order of the IPMMU devices on the "iommus" property > does not match the IPMMU order on the ipmmu_device list. > > Hm, I wonder if all this should be replaced with ->xlate() somehow? Ideally, yes - the core code already has most of this covered, so taking advantage of it would be good. I think the only slight hiccup is that the 32-bit DMA code is then going to call attach_dev() with a domain you probably don't want, before you get your add_device() call. Other than handling that vs. group-based default domains for 64-bit, though, there shouldn't be anything else to special-case, I don't think. I'm finally starting to have a look into converting the arch/arm code over to use groups and default domains sensibly, but I suspect that's ultimately going to have some dependency on the probe deferral stuff, rather than introduce the same bus notifier bodge we currently have on arm64. Robin. > > Thanks, > > / magnus > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-09-22 15:56 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-09-20 12:41 [PATCH/RFC] iommu/ipmmu-vmsa: Update ->add_device() return value Magnus Damm 2016-09-20 12:41 ` Magnus Damm 2016-09-20 13:18 ` Robin Murphy 2016-09-20 13:18 ` Robin Murphy 2016-09-20 15:03 ` Magnus Damm 2016-09-22 15:56 ` Robin Murphy
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.