All of lore.kernel.org
 help / color / mirror / Atom feed
* [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.