From: Sricharan R <hidden> Date: 2017-05-18 10:07:34
Now with IOMMU probe deferral, we return -EPROBE_DEFER
for masters that are connected to an IOMMU which is not
probed yet, but going to get probed, so that we can attach
the correct dma_ops. So while trying to defer the probe of
the master, check if the of_iommu node that it is connected
to is marked in DT as 'status=disabled', then the IOMMU is never
is going to get probed. So simply return NULL and let the master
work without an IOMMU.
Fixes: 7b07cbefb68d ("iommu: of: Handle IOMMU lookup failure with deferred probing or error")
Signed-off-by: Sricharan R <redacted>
Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Tested-by: Will Deacon <redacted>
Tested-by: Magnus Damn <redacted>
Acked-by: Will Deacon <redacted>
---
[V2] Corrected spelling/case in commit log
drivers/iommu/of_iommu.c | 1 +
1 file changed, 1 insertion(+)
From: Sricharan R <hidden> Date: 2017-05-18 10:07:48
While deferring the probe of IOMMU masters,
xlate and add_device callback can pass back error values
like -ENODEV, which means IOMMU cannot be connected
with that master for real reasons. So rather than
killing the master's probe for such errors, just
ignore the errors and let the master work without
an IOMMU.
Fixes: 7b07cbefb68d ("iommu: of: Handle IOMMU lookup failure with deferred probing or error")
Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Tested-by: Magnus Damn <redacted>
Signed-off-by: Sricharan R <redacted>
---
[V2] Corrected spelling/case in commit log
drivers/iommu/of_iommu.c | 6 ++++++
1 file changed, 6 insertions(+)
From: Sricharan R <hidden> Date: 2017-05-18 10:08:01
From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
arch_setup_dma_ops() is used in device probe code paths to create an
IOMMU mapping and attach it to the device. The function assumes that the
device is attached to a device-specific IOMMU instance (or at least a
device-specific TLB in a shared IOMMU instance) and thus creates a
separate mapping for every device.
On several systems (Renesas R-Car Gen2 being one of them), that
assumption is not true, and IOMMU mappings must be shared between
multiple devices. In those cases the IOMMU driver knows better than the
generic ARM dma-mapping layer and attaches mapping to devices manually
with arm_iommu_attach_device(), which sets the DMA ops for the device.
The arch_setup_dma_ops() function takes this into account and bails out
immediately if the device already has DMA ops assigned. However, the
corresponding arch_teardown_dma_ops() function, called from driver
unbind code paths (including probe deferral), will tear the mapping down
regardless of who created it. When the device is reprobed
arch_setup_dma_ops() will be called again but won't perform any
operation as the DMA ops will still be set.
We need to reset the DMA ops in arch_teardown_dma_ops() to fix this.
However, we can't do so unconditionally, as then a new mapping would be
created by arch_setup_dma_ops() when the device is reprobed, regardless
of whether the device needs to share a mapping or not. We must thus keep
track of whether arch_setup_dma_ops() created the mapping, and only in
that case tear it down in arch_teardown_dma_ops().
Keep track of that information in the dev_archdata structure. As the
structure is embedded in all instances of struct device let's not grow
it, but turn the existing dma_coherent bool field into a bitfield that
can be used for other purposes.
Fixes: 09515ef5ddad ("of/acpi: Configure dma operations at probe time for platform/amba/pci bus devices")
Reviewed-by: Robin Murphy <robin.murphy@arm.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
arch/arm/include/asm/device.h | 3 ++-
arch/arm/mm/dma-mapping.c | 4 ++++
2 files changed, 6 insertions(+), 1 deletion(-)
Hi Sricharan,
Thank you for the patch.
On Thursday 18 May 2017 15:37:09 Sricharan R wrote:
While deferring the probe of IOMMU masters,
xlate and add_device callback can pass back error values
like -ENODEV, which means IOMMU cannot be connected
with that master for real reasons. So rather than
killing the master's probe for such errors, just
ignore the errors and let the master work without
an IOMMU.
I don't think this is a good idea. Why should we allow IOMMU drivers to return
an error if we're always going to ignore the error value ? That will lead to
drivers implementing slightly different behaviours, which will be painful the
day we'll need to start acting based on the error value.
At the very least, if you want to give a specific meaning to -ENODEV, you
should check for that value specifically and not ignore all errors other than
-EPROBE_DEFER. You also need to document the meaning of the error value. This
can be done in the documentation of the of_xlate operation in
include/linux/iommu.h:
@@ -224,6 +223,11 @@ struct iommu_ops {/* Get the number of windows per domain */u32(*domain_get_windows)(structiommu_domain*domain);+/**+*@of_xlate:+*+*AddOFmasterIDstoiommugrouping.+*/int(*of_xlate)(structdevice*dev,structof_phandle_args*args);unsignedlongpgsize_bitmap;
And add documentation for the error codes there.
If you want to ignore some errors returned from the add_device operation you
should document it similarly, and in particular document which error check(s)
need to be performed by of_xlate and which are the responsibility of
add_device.
quoted hunk
Fixes: 7b07cbefb68d ("iommu: of: Handle IOMMU lookup failure with deferred
probing or error")
Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Tested-by: Magnus Damn <redacted>
Signed-off-by: Sricharan R <redacted>
---
[V2] Corrected spelling/case in commit log
drivers/iommu/of_iommu.c | 6 ++++++
1 file changed, 6 insertions(+)
Hi Sricharan,
Thank you for the patch.
On Thursday 18 May 2017 15:37:08 Sricharan R wrote:
Now with IOMMU probe deferral, we return -EPROBE_DEFER
for masters that are connected to an IOMMU which is not
probed yet, but going to get probed, so that we can attach
the correct dma_ops. So while trying to defer the probe of
the master, check if the of_iommu node that it is connected
to is marked in DT as 'status=disabled', then the IOMMU is never
is going to get probed. So simply return NULL and let the master
work without an IOMMU.
Fixes: 7b07cbefb68d ("iommu: of: Handle IOMMU lookup failure with deferred
probing or error") Signed-off-by: Sricharan R [off-list ref]
Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Tested-by: Will Deacon <redacted>
Tested-by: Magnus Damn <redacted>
Acked-by: Will Deacon <redacted>
From: Sricharan R <hidden> Date: 2017-05-18 11:56:41
Hi Laurent,
On 5/18/2017 4:09 PM, Laurent Pinchart wrote:
Hi Sricharan,
Thank you for the patch.
On Thursday 18 May 2017 15:37:09 Sricharan R wrote:
quoted
While deferring the probe of IOMMU masters,
xlate and add_device callback can pass back error values
like -ENODEV, which means IOMMU cannot be connected
with that master for real reasons. So rather than
killing the master's probe for such errors, just
ignore the errors and let the master work without
an IOMMU.
I don't think this is a good idea. Why should we allow IOMMU drivers to return
an error if we're always going to ignore the error value ? That will lead to
drivers implementing slightly different behaviours, which will be painful the
day we'll need to start acting based on the error value.
The of_iommu_configure interface, before this series, was returning either
correct 'iommu_ops' or NULL. Also there was no return value from
of_dma_configure which calls of_iommu_configure. This means that if we block
only -ENODEV now and let the other errors, the probe of the master devices
can be killed for reasons apart from deferring. This would be a change in
behavior introduced. All of xlate, add_device, of_pci_map_rid and others
can return values apart from -ENODEV. So was thinking that restoring the
old behavior, except for returning EPROBE_DEFER was the better thing to do ?
Regards,
Sricharan
quoted hunk
At the very least, if you want to give a specific meaning to -ENODEV, you
should check for that value specifically and not ignore all errors other than
-EPROBE_DEFER. You also need to document the meaning of the error value. This
can be done in the documentation of the of_xlate operation in
include/linux/iommu.h:
@@ -224,6 +223,11 @@ struct iommu_ops {/* Get the number of windows per domain */u32(*domain_get_windows)(structiommu_domain*domain);+/**+*@of_xlate:+*+*AddOFmasterIDstoiommugrouping.+*/int(*of_xlate)(structdevice*dev,structof_phandle_args*args);unsignedlongpgsize_bitmap;
And add documentation for the error codes there.
If you want to ignore some errors returned from the add_device operation you
should document it similarly, and in particular document which error check(s)
need to be performed by of_xlate and which are the responsibility of
add_device.
quoted
Fixes: 7b07cbefb68d ("iommu: of: Handle IOMMU lookup failure with deferred
probing or error")
Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Tested-by: Magnus Damn <redacted>
Signed-off-by: Sricharan R <redacted>
---
[V2] Corrected spelling/case in commit log
drivers/iommu/of_iommu.c | 6 ++++++
1 file changed, 6 insertions(+)
Hi Sricharan,
On Thursday 18 May 2017 17:26:14 Sricharan R wrote:
On 5/18/2017 4:09 PM, Laurent Pinchart wrote:
quoted
On Thursday 18 May 2017 15:37:09 Sricharan R wrote:
quoted
While deferring the probe of IOMMU masters,
xlate and add_device callback can pass back error values
like -ENODEV, which means IOMMU cannot be connected
with that master for real reasons. So rather than
killing the master's probe for such errors, just
ignore the errors and let the master work without
an IOMMU.
I don't think this is a good idea. Why should we allow IOMMU drivers to
return an error if we're always going to ignore the error value ? That
will lead to drivers implementing slightly different behaviours, which
will be painful the day we'll need to start acting based on the error
value.
The of_iommu_configure interface, before this series, was returning either
correct 'iommu_ops' or NULL. Also there was no return value from
of_dma_configure which calls of_iommu_configure. This means that if we block
only -ENODEV now and let the other errors, the probe of the master devices
can be killed for reasons apart from deferring. This would be a change in
behavior introduced. All of xlate, add_device, of_pci_map_rid and others
can return values apart from -ENODEV. So was thinking that restoring the
old behavior, except for returning EPROBE_DEFER was the better thing to do
?
We went from a situation where of_iommu_configure() could return either valid
operations in the case the device was to be handled by the IOMMU or NULL
otherwise, to a situation where we needed a third option for probe deferral.
The way we've done this, through error pointers, allows lots of other errors
to be returned as well from the of_xlate and add_device operations.
There is currently no use for returning error codes other than -EPROBE_DEFFER
from of_iommu_configure(), so your proposal is to block errors returned from
the of_xlate and add_device operations inside of_iommu_configure(). My point
is that, by doing so, we allow IOMMU drivers to return random error codes that
are then ignored. I believe this can cause problems in the future when we will
need to extend the API and standardize more error codes, as by then IOMMU
drivers will return random errors (they actually do so already to some
extent).
For of_xlate I agree with you to some extent. v4.11 just checked whether
of_xlate succeeded or not, and just didn't use the IOMMU in case it failed.
The exact error value was ignored, and drivers already return random errors.
Going back to the v4.11 behaviour is what we need to do in the short-term,
even if I believe we should standardize the error values returned from
of_xlate after v4.12.
For add_device, however, the situation is a bit different. The add_device
operation is called from the IOMMU bus notifier, and the -ENODEV error is
ignored by add_iommu_group(). Any other error will cause bus_set_iommu() to
fail, which makes IOMMU probing fail for the drivers that check the return
value of bus_set_iommu() (some of them don't :-/).
Fixing all this properly requires standardizing the error codes, and going
through the existing IOMMU drivers to comply with the standardized behaviour.
While this shouldn't be very difficult, it's likely not material for a v4.12-
rc fix. We will thus likely need to merge this patch (or something very
similar to it), but I'd really like to see this fixed properly for v4.13.
quoted
At the very least, if you want to give a specific meaning to -ENODEV, you
should check for that value specifically and not ignore all errors other
than -EPROBE_DEFER. You also need to document the meaning of the error
value. This can be done in the documentation of the of_xlate operation in
include/linux/iommu.h:
@@ -224,6 +223,11 @@ struct iommu_ops {/* Get the number of windows per domain */u32(*domain_get_windows)(structiommu_domain*domain);+/**+*@of_xlate:+*+*AddOFmasterIDstoiommugrouping.+*/int(*of_xlate)(structdevice*dev,structof_phandle_args*args);unsignedlongpgsize_bitmap;
And add documentation for the error codes there.
If you want to ignore some errors returned from the add_device operation
you should document it similarly, and in particular document which error
check(s) need to be performed by of_xlate and which are the
responsibility of add_device.
quoted
Fixes: 7b07cbefb68d ("iommu: of: Handle IOMMU lookup failure with
deferred probing or error")
Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Tested-by: Magnus Damn <redacted>
Signed-off-by: Sricharan R <redacted>
---
[V2] Corrected spelling/case in commit log
drivers/iommu/of_iommu.c | 6 ++++++
1 file changed, 6 insertions(+)
From: Sricharan R <hidden> Date: 2017-05-18 13:38:37
Hi Laurent,
On 5/18/2017 6:00 PM, Laurent Pinchart wrote:
Hi Sricharan,
On Thursday 18 May 2017 17:26:14 Sricharan R wrote:
quoted
On 5/18/2017 4:09 PM, Laurent Pinchart wrote:
quoted
On Thursday 18 May 2017 15:37:09 Sricharan R wrote:
quoted
While deferring the probe of IOMMU masters,
xlate and add_device callback can pass back error values
like -ENODEV, which means IOMMU cannot be connected
with that master for real reasons. So rather than
killing the master's probe for such errors, just
ignore the errors and let the master work without
an IOMMU.
I don't think this is a good idea. Why should we allow IOMMU drivers to
return an error if we're always going to ignore the error value ? That
will lead to drivers implementing slightly different behaviours, which
will be painful the day we'll need to start acting based on the error
value.
The of_iommu_configure interface, before this series, was returning either
correct 'iommu_ops' or NULL. Also there was no return value from
of_dma_configure which calls of_iommu_configure. This means that if we block
only -ENODEV now and let the other errors, the probe of the master devices
can be killed for reasons apart from deferring. This would be a change in
behavior introduced. All of xlate, add_device, of_pci_map_rid and others
can return values apart from -ENODEV. So was thinking that restoring the
old behavior, except for returning EPROBE_DEFER was the better thing to do
?
We went from a situation where of_iommu_configure() could return either valid
operations in the case the device was to be handled by the IOMMU or NULL
otherwise, to a situation where we needed a third option for probe deferral.
The way we've done this, through error pointers, allows lots of other errors
to be returned as well from the of_xlate and add_device operations.
right, this was difference in the behavior now.
There is currently no use for returning error codes other than -EPROBE_DEFFER
from of_iommu_configure(), so your proposal is to block errors returned from
the of_xlate and add_device operations inside of_iommu_configure(). My point
is that, by doing so, we allow IOMMU drivers to return random error codes that
are then ignored. I believe this can cause problems in the future when we will
need to extend the API and standardize more error codes, as by then IOMMU
drivers will return random errors (they actually do so already to some
extent).
For of_xlate I agree with you to some extent. v4.11 just checked whether
of_xlate succeeded or not, and just didn't use the IOMMU in case it failed.
The exact error value was ignored, and drivers already return random errors.
Going back to the v4.11 behaviour is what we need to do in the short-term,
even if I believe we should standardize the error values returned from
of_xlate after v4.12.
For add_device, however, the situation is a bit different. The add_device
operation is called from the IOMMU bus notifier, and the -ENODEV error is
ignored by add_iommu_group(). Any other error will cause bus_set_iommu() to
fail, which makes IOMMU probing fail for the drivers that check the return
value of bus_set_iommu() (some of them don't :-/).
Fixing all this properly requires standardizing the error codes, and going
through the existing IOMMU drivers to comply with the standardized behaviour.
I understand your concern on standardizing the error codes from xlate,
add_device, others and handling them properly. As you said there are quite some
errors returned from them today. Also another thing is standardizing the
behavior of of_iommu_configure itself. So that API serves to connect a device
to its correct iommu_ops. When that's not possible, what should be the output
and how should that be handled by the caller. The current behavior is to
either 1) connect to correct ops or 2) wait for it or 3) progress further
with plain/default dma_ops. Anyways as you said standardizing the iommu api
ops, would make the of_iommu_configure handling more specific. Having said that
i think similar fix needs to be done for acpi's iort_iommu_configure as
well.
While this shouldn't be very difficult, it's likely not material for a v4.12-
rc fix. We will thus likely need to merge this patch (or something very
similar to it), but I'd really like to see this fixed properly for v4.13.
When you say "merge this patch (or something similar)", is that about
documenting the error values for of_xlate and add_device that you showed
down below (or) about the patch in discussion ?
Regards,
Sricharan
quoted
quoted
At the very least, if you want to give a specific meaning to -ENODEV, you
should check for that value specifically and not ignore all errors other
than -EPROBE_DEFER. You also need to document the meaning of the error
value. This can be done in the documentation of the of_xlate operation in
include/linux/iommu.h:
@@ -224,6 +223,11 @@ struct iommu_ops {/* Get the number of windows per domain */u32(*domain_get_windows)(structiommu_domain*domain);+/**+*@of_xlate:+*+*AddOFmasterIDstoiommugrouping.+*/int(*of_xlate)(structdevice*dev,structof_phandle_args*args);unsignedlongpgsize_bitmap;
And add documentation for the error codes there.
If you want to ignore some errors returned from the add_device operation
you should document it similarly, and in particular document which error
check(s) need to be performed by of_xlate and which are the
responsibility of add_device.
quoted
Fixes: 7b07cbefb68d ("iommu: of: Handle IOMMU lookup failure with
deferred probing or error")
Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Tested-by: Magnus Damn <redacted>
Signed-off-by: Sricharan R <redacted>
---
[V2] Corrected spelling/case in commit log
drivers/iommu/of_iommu.c | 6 ++++++
1 file changed, 6 insertions(+)
Hi Sricharan
On Thursday 18 May 2017 19:08:12 Sricharan R wrote:
On 5/18/2017 6:00 PM, Laurent Pinchart wrote:
quoted
On Thursday 18 May 2017 17:26:14 Sricharan R wrote:
quoted
On 5/18/2017 4:09 PM, Laurent Pinchart wrote:
quoted
On Thursday 18 May 2017 15:37:09 Sricharan R wrote:
quoted
While deferring the probe of IOMMU masters,
xlate and add_device callback can pass back error values
like -ENODEV, which means IOMMU cannot be connected
with that master for real reasons. So rather than
killing the master's probe for such errors, just
ignore the errors and let the master work without
an IOMMU.
I don't think this is a good idea. Why should we allow IOMMU drivers to
return an error if we're always going to ignore the error value ? That
will lead to drivers implementing slightly different behaviours, which
will be painful the day we'll need to start acting based on the error
value.
The of_iommu_configure interface, before this series, was returning
either correct 'iommu_ops' or NULL. Also there was no return value from
of_dma_configure which calls of_iommu_configure. This means that if we
block only -ENODEV now and let the other errors, the probe of the master
devices can be killed for reasons apart from deferring. This would be a
change in behavior introduced. All of xlate, add_device, of_pci_map_rid
and others can return values apart from -ENODEV. So was thinking that
restoring the old behavior, except for returning EPROBE_DEFER was the
better thing to do ?
We went from a situation where of_iommu_configure() could return either
valid operations in the case the device was to be handled by the IOMMU or
NULL otherwise, to a situation where we needed a third option for probe
deferral. The way we've done this, through error pointers, allows lots of
other errors to be returned as well from the of_xlate and add_device
operations.
right, this was difference in the behavior now.
quoted
There is currently no use for returning error codes other than
-EPROBE_DEFFER from of_iommu_configure(), so your proposal is to block
errors returned from the of_xlate and add_device operations inside
of_iommu_configure(). My point is that, by doing so, we allow IOMMU
drivers to return random error codes that are then ignored. I believe
this can cause problems in the future when we will need to extend the API
and standardize more error codes, as by then IOMMU drivers will return
random errors (they actually do so already to some extent).
For of_xlate I agree with you to some extent. v4.11 just checked whether
of_xlate succeeded or not, and just didn't use the IOMMU in case it
failed. The exact error value was ignored, and drivers already return
random errors. Going back to the v4.11 behaviour is what we need to do in
the short-term, even if I believe we should standardize the error values
returned from of_xlate after v4.12.
For add_device, however, the situation is a bit different. The add_device
operation is called from the IOMMU bus notifier, and the -ENODEV error is
ignored by add_iommu_group(). Any other error will cause bus_set_iommu()
to fail, which makes IOMMU probing fail for the drivers that check the
return value of bus_set_iommu() (some of them don't :-/).
Fixing all this properly requires standardizing the error codes, and going
through the existing IOMMU drivers to comply with the standardized
behaviour.
I understand your concern on standardizing the error codes from xlate,
add_device, others and handling them properly. As you said there are quite
some errors returned from them today. Also another thing is standardizing
the behavior of of_iommu_configure itself. So that API serves to connect a
device to its correct iommu_ops. When that's not possible, what should be
the output and how should that be handled by the caller. The current
behavior is to either 1) connect to correct ops or 2) wait for it or 3)
progress further with plain/default dma_ops. Anyways as you said
standardizing the iommu api ops, would make the of_iommu_configure handling
more specific. Having said that i think similar fix needs to be done for
acpi's iort_iommu_configure as well.
I'm less knowledgeable about ACPI but I think you're right. Would you like to
tackle this for v4.13 ? :-)
quoted
While this shouldn't be very difficult, it's likely not material for a
v4.12- rc fix. We will thus likely need to merge this patch (or something
very similar to it), but I'd really like to see this fixed properly for
v4.13.
When you say "merge this patch (or something similar)", is that about
documenting the error values for of_xlate and add_device that you showed
down below (or) about the patch in discussion ?
I meant the patch we're discussing, "[PATCH V2 2/3] iommu: of: Ignore all
errors except EPROBE_DEFER". Sorry for the confusion. One more comment about
this below.
quoted
quoted
quoted
At the very least, if you want to give a specific meaning to -ENODEV,
you should check for that value specifically and not ignore all errors
other than -EPROBE_DEFER. You also need to document the meaning of the
error value. This can be done in the documentation of the of_xlate
operation in include/linux/iommu.h:
@@ -224,6 +223,11 @@ struct iommu_ops {/* Get the number of windows per domain */u32(*domain_get_windows)(structiommu_domain*domain);+/**+*@of_xlate:+*+*AddOFmasterIDstoiommugrouping.+*/int(*of_xlate)(structdevice*dev,structof_phandle_args*args);unsignedlongpgsize_bitmap;
And add documentation for the error codes there.
If you want to ignore some errors returned from the add_device operation
you should document it similarly, and in particular document which error
check(s) need to be performed by of_xlate and which are the
responsibility of add_device.
quoted
Fixes: 7b07cbefb68d ("iommu: of: Handle IOMMU lookup failure with
deferred probing or error")
Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Tested-by: Magnus Damn <redacted>
Signed-off-by: Sricharan R <redacted>
---
[V2] Corrected spelling/case in commit log
drivers/iommu/of_iommu.c | 6 ++++++
1 file changed, 6 insertions(+)
device *dev,
ops = ERR_PTR(err);
}
+ /* Ignore all other errors apart from EPROBE_DEFER */
+ if (IS_ERR(ops) && (PTR_ERR(ops) != -EPROBE_DEFER)) {
+ dev_info(dev, "Adding to IOMMU failed: %ld\n",
PTR_ERR(ops));
I would turn this into a dev_dbg(), as at least the -ENODEV error returned
from add_device has a defined meaning (see the comment in add_iommu_group())
and is in my opinion not a condition that should be reported in the kernel log
by default.
From: Sricharan R <hidden> Date: 2017-05-18 14:05:25
Hi Laurent,
On 5/18/2017 7:13 PM, Laurent Pinchart wrote:
Hi Sricharan
On Thursday 18 May 2017 19:08:12 Sricharan R wrote:
quoted
On 5/18/2017 6:00 PM, Laurent Pinchart wrote:
quoted
On Thursday 18 May 2017 17:26:14 Sricharan R wrote:
quoted
On 5/18/2017 4:09 PM, Laurent Pinchart wrote:
quoted
On Thursday 18 May 2017 15:37:09 Sricharan R wrote:
quoted
While deferring the probe of IOMMU masters,
xlate and add_device callback can pass back error values
like -ENODEV, which means IOMMU cannot be connected
with that master for real reasons. So rather than
killing the master's probe for such errors, just
ignore the errors and let the master work without
an IOMMU.
I don't think this is a good idea. Why should we allow IOMMU drivers to
return an error if we're always going to ignore the error value ? That
will lead to drivers implementing slightly different behaviours, which
will be painful the day we'll need to start acting based on the error
value.
The of_iommu_configure interface, before this series, was returning
either correct 'iommu_ops' or NULL. Also there was no return value from
of_dma_configure which calls of_iommu_configure. This means that if we
block only -ENODEV now and let the other errors, the probe of the master
devices can be killed for reasons apart from deferring. This would be a
change in behavior introduced. All of xlate, add_device, of_pci_map_rid
and others can return values apart from -ENODEV. So was thinking that
restoring the old behavior, except for returning EPROBE_DEFER was the
better thing to do ?
We went from a situation where of_iommu_configure() could return either
valid operations in the case the device was to be handled by the IOMMU or
NULL otherwise, to a situation where we needed a third option for probe
deferral. The way we've done this, through error pointers, allows lots of
other errors to be returned as well from the of_xlate and add_device
operations.
right, this was difference in the behavior now.
quoted
There is currently no use for returning error codes other than
-EPROBE_DEFFER from of_iommu_configure(), so your proposal is to block
errors returned from the of_xlate and add_device operations inside
of_iommu_configure(). My point is that, by doing so, we allow IOMMU
drivers to return random error codes that are then ignored. I believe
this can cause problems in the future when we will need to extend the API
and standardize more error codes, as by then IOMMU drivers will return
random errors (they actually do so already to some extent).
For of_xlate I agree with you to some extent. v4.11 just checked whether
of_xlate succeeded or not, and just didn't use the IOMMU in case it
failed. The exact error value was ignored, and drivers already return
random errors. Going back to the v4.11 behaviour is what we need to do in
the short-term, even if I believe we should standardize the error values
returned from of_xlate after v4.12.
For add_device, however, the situation is a bit different. The add_device
operation is called from the IOMMU bus notifier, and the -ENODEV error is
ignored by add_iommu_group(). Any other error will cause bus_set_iommu()
to fail, which makes IOMMU probing fail for the drivers that check the
return value of bus_set_iommu() (some of them don't :-/).
Fixing all this properly requires standardizing the error codes, and going
through the existing IOMMU drivers to comply with the standardized
behaviour.
I understand your concern on standardizing the error codes from xlate,
add_device, others and handling them properly. As you said there are quite
some errors returned from them today. Also another thing is standardizing
the behavior of of_iommu_configure itself. So that API serves to connect a
device to its correct iommu_ops. When that's not possible, what should be
the output and how should that be handled by the caller. The current
behavior is to either 1) connect to correct ops or 2) wait for it or 3)
progress further with plain/default dma_ops. Anyways as you said
standardizing the iommu api ops, would make the of_iommu_configure handling
more specific. Having said that i think similar fix needs to be done for
acpi's iort_iommu_configure as well.
I'm less knowledgeable about ACPI but I think you're right. Would you like to
tackle this for v4.13 ? :-)
Will add the fix for ACPI now in this series. ok, Will really see if i can
address the standardizing part for 4.13.
quoted
quoted
While this shouldn't be very difficult, it's likely not material for a
v4.12- rc fix. We will thus likely need to merge this patch (or something
very similar to it), but I'd really like to see this fixed properly for
v4.13.
When you say "merge this patch (or something similar)", is that about
documenting the error values for of_xlate and add_device that you showed
down below (or) about the patch in discussion ?
I meant the patch we're discussing, "[PATCH V2 2/3] iommu: of: Ignore all
errors except EPROBE_DEFER". Sorry for the confusion. One more comment about
this below.
ok.
quoted
quoted
quoted
quoted
At the very least, if you want to give a specific meaning to -ENODEV,
you should check for that value specifically and not ignore all errors
other than -EPROBE_DEFER. You also need to document the meaning of the
error value. This can be done in the documentation of the of_xlate
operation in include/linux/iommu.h:
@@ -224,6 +223,11 @@ struct iommu_ops {/* Get the number of windows per domain */u32(*domain_get_windows)(structiommu_domain*domain);+/**+*@of_xlate:+*+*AddOFmasterIDstoiommugrouping.+*/int(*of_xlate)(structdevice*dev,structof_phandle_args*args);unsignedlongpgsize_bitmap;
And add documentation for the error codes there.
If you want to ignore some errors returned from the add_device operation
you should document it similarly, and in particular document which error
check(s) need to be performed by of_xlate and which are the
responsibility of add_device.
quoted
Fixes: 7b07cbefb68d ("iommu: of: Handle IOMMU lookup failure with
deferred probing or error")
Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Tested-by: Magnus Damn <redacted>
Signed-off-by: Sricharan R <redacted>
---
[V2] Corrected spelling/case in commit log
drivers/iommu/of_iommu.c | 6 ++++++
1 file changed, 6 insertions(+)
device *dev,
ops = ERR_PTR(err);
}
+ /* Ignore all other errors apart from EPROBE_DEFER */
+ if (IS_ERR(ops) && (PTR_ERR(ops) != -EPROBE_DEFER)) {
+ dev_info(dev, "Adding to IOMMU failed: %ld\n",
PTR_ERR(ops));
I would turn this into a dev_dbg(), as at least the -ENODEV error returned
from add_device has a defined meaning (see the comment in add_iommu_group())
and is in my opinion not a condition that should be reported in the kernel log
by default.
ok, will turn this in to a dev_dbg.
Regards,
Sricharan
quoted
quoted
quoted
quoted
quoted
+ ops = NULL;+ }+ return ops; }
--
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation