linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH V4 1/4] iommu: of: Fix check for returning EPROBE_DEFER
@ 2017-05-18 14:54 Sricharan R
  2017-05-18 14:54 ` [PATCH V4 2/4] iommu: of: Ignore all errors except EPROBE_DEFER Sricharan R
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Sricharan R @ 2017-05-18 14:54 UTC (permalink / raw)
  To: robin.murphy, will.deacon, joro, lorenzo.pieralisi, iommu,
	linux-arm-kernel, linux-arm-msm, m.szyprowski, bhelgaas,
	linux-pci, linux-acpi, tn, hanjun.guo, okaya, robh+dt,
	frowand.list, devicetree, linux-kernel, sudeep.holla, rjw, lenb,
	catalin.marinas, arnd, linux-arch, laurent.pinchart,
	j.neuschaefer, geert, magnus.damm
  Cc: sricharan

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 <sricharan@codeaurora.org>
Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Tested-by: Will Deacon <will.deacon@arm.com>
Tested-by: Magnus Damn <magnus.damn@gmail.com>
Acked-by: Will Deacon <will.deacon@arm.com>
---
 drivers/iommu/of_iommu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index 9f44ee8..e6e9bec 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -118,6 +118,7 @@ static bool of_iommu_driver_present(struct device_node *np)
 
 	ops = iommu_ops_from_fwnode(fwnode);
 	if ((ops && !ops->of_xlate) ||
+	    !of_device_is_available(iommu_spec->np) ||
 	    (!ops && !of_iommu_driver_present(iommu_spec->np)))
 		return NULL;
 
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH V4 2/4] iommu: of: Ignore all errors except EPROBE_DEFER
  2017-05-18 14:54 [PATCH V4 1/4] iommu: of: Fix check for returning EPROBE_DEFER Sricharan R
@ 2017-05-18 14:54 ` Sricharan R
  2017-05-18 14:56   ` Laurent Pinchart
  2017-05-18 14:54 ` [PATCH V4 3/4] ACPI/IORT: " Sricharan R
  2017-05-18 14:54 ` [PATCH V4 4/4] ARM: dma-mapping: Don't tear third-party mappings Sricharan R
  2 siblings, 1 reply; 9+ messages in thread
From: Sricharan R @ 2017-05-18 14:54 UTC (permalink / raw)
  To: robin.murphy, will.deacon, joro, lorenzo.pieralisi, iommu,
	linux-arm-kernel, linux-arm-msm, m.szyprowski, bhelgaas,
	linux-pci, linux-acpi, tn, hanjun.guo, okaya, robh+dt,
	frowand.list, devicetree, linux-kernel, sudeep.holla, rjw, lenb,
	catalin.marinas, arnd, linux-arch, laurent.pinchart,
	j.neuschaefer, geert, magnus.damm
  Cc: sricharan

While deferring the probe of IOMMU masters, xlate and
add_device callbacks called from of_iommu_configure
can pass back error values like -ENODEV, which means
the IOMMU cannot be connected with that master for real
reasons. Before the IOMMU probe deferral, all such errors
were ignored. Now all those errors are propagated back,
killing the master's probe for such errors. Instead ignore
all the errors except EPROBE_DEFER, which is the only one
of concern and let the master work without IOMMU, thus
restoring the old behavior.

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 <magnus.damn@gmail.com>
Signed-off-by: Sricharan R <sricharan@codeaurora.org>
---
[V4] Reworded commit log and changed dev_info to dev_dbg

 drivers/iommu/of_iommu.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index e6e9bec..19779b8 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -237,6 +237,12 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
 			ops = ERR_PTR(err);
 	}
 
+	/* Ignore all other errors apart from EPROBE_DEFER */
+	if (IS_ERR(ops) && (PTR_ERR(ops) != -EPROBE_DEFER)) {
+		dev_dbg(dev, "Adding to IOMMU failed: %ld\n", PTR_ERR(ops));
+		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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH V4 3/4] ACPI/IORT: Ignore all errors except EPROBE_DEFER
  2017-05-18 14:54 [PATCH V4 1/4] iommu: of: Fix check for returning EPROBE_DEFER Sricharan R
  2017-05-18 14:54 ` [PATCH V4 2/4] iommu: of: Ignore all errors except EPROBE_DEFER Sricharan R
@ 2017-05-18 14:54 ` Sricharan R
  2017-05-22 10:37   ` Lorenzo Pieralisi
  2017-05-18 14:54 ` [PATCH V4 4/4] ARM: dma-mapping: Don't tear third-party mappings Sricharan R
  2 siblings, 1 reply; 9+ messages in thread
From: Sricharan R @ 2017-05-18 14:54 UTC (permalink / raw)
  To: robin.murphy, will.deacon, joro, lorenzo.pieralisi, iommu,
	linux-arm-kernel, linux-arm-msm, m.szyprowski, bhelgaas,
	linux-pci, linux-acpi, tn, hanjun.guo, okaya, robh+dt,
	frowand.list, devicetree, linux-kernel, sudeep.holla, rjw, lenb,
	catalin.marinas, arnd, linux-arch, laurent.pinchart,
	j.neuschaefer, geert, magnus.damm
  Cc: sricharan

While deferring the probe of IOMMU masters, xlate and
add_device callbacks called from iort_iommu_configure
can pass back error values like -ENODEV, which means
the IOMMU cannot be connected with that master for real
reasons. Before the IOMMU probe deferral, all such errors
were ignored. Now all those errors are propagated back,
killing the master's probe for such errors. Instead ignore
all the errors except EPROBE_DEFER, which is the only one
of concern and let the master work without IOMMU, thus
restoring the old behavior.

Fixes: 5a1bb638d567 ("drivers: acpi: Handle IOMMU lookup failure with deferred probing or error")
Signed-off-by: Sricharan R <sricharan@codeaurora.org>
---
[V4] Added this patch newly.

 drivers/acpi/arm64/iort.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index c5fecf9..16e101f 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -782,6 +782,12 @@ const struct iommu_ops *iort_iommu_configure(struct device *dev)
 	if (err)
 		ops = ERR_PTR(err);
 
+	/* Ignore all other errors apart from EPROBE_DEFER */
+	if (IS_ERR(ops) && (PTR_ERR(ops) != -EPROBE_DEFER)) {
+		dev_dbg(dev, "Adding to IOMMU failed: %ld\n", PTR_ERR(ops));
+		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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH V4 4/4] ARM: dma-mapping: Don't tear third-party mappings
  2017-05-18 14:54 [PATCH V4 1/4] iommu: of: Fix check for returning EPROBE_DEFER Sricharan R
  2017-05-18 14:54 ` [PATCH V4 2/4] iommu: of: Ignore all errors except EPROBE_DEFER Sricharan R
  2017-05-18 14:54 ` [PATCH V4 3/4] ACPI/IORT: " Sricharan R
@ 2017-05-18 14:54 ` Sricharan R
  2 siblings, 0 replies; 9+ messages in thread
From: Sricharan R @ 2017-05-18 14:54 UTC (permalink / raw)
  To: robin.murphy, will.deacon, joro, lorenzo.pieralisi, iommu,
	linux-arm-kernel, linux-arm-msm, m.szyprowski, bhelgaas,
	linux-pci, linux-acpi, tn, hanjun.guo, okaya, robh+dt,
	frowand.list, devicetree, linux-kernel, sudeep.holla, rjw, lenb,
	catalin.marinas, arnd, linux-arch, laurent.pinchart,
	j.neuschaefer, geert, magnus.damm
  Cc: sricharan

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(-)

diff --git a/arch/arm/include/asm/device.h b/arch/arm/include/asm/device.h
index 36ec9c8..3234fe9 100644
--- a/arch/arm/include/asm/device.h
+++ b/arch/arm/include/asm/device.h
@@ -19,7 +19,8 @@ struct dev_archdata {
 #ifdef CONFIG_XEN
 	const struct dma_map_ops *dev_dma_ops;
 #endif
-	bool dma_coherent;
+	unsigned int dma_coherent:1;
+	unsigned int dma_ops_setup:1;
 };
 
 struct omap_device;
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index c742dfd..b48998f 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -2430,9 +2430,13 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 		dev->dma_ops = xen_dma_ops;
 	}
 #endif
+	dev->archdata.dma_ops_setup = true;
 }
 
 void arch_teardown_dma_ops(struct device *dev)
 {
+	if (!dev->archdata.dma_ops_setup)
+		return;
+
 	arm_teardown_iommu_dma_ops(dev);
 }
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH V4 2/4] iommu: of: Ignore all errors except EPROBE_DEFER
  2017-05-18 14:54 ` [PATCH V4 2/4] iommu: of: Ignore all errors except EPROBE_DEFER Sricharan R
@ 2017-05-18 14:56   ` Laurent Pinchart
  0 siblings, 0 replies; 9+ messages in thread
From: Laurent Pinchart @ 2017-05-18 14:56 UTC (permalink / raw)
  To: Sricharan R
  Cc: robin.murphy, will.deacon, joro, lorenzo.pieralisi, iommu,
	linux-arm-kernel, linux-arm-msm, m.szyprowski, bhelgaas,
	linux-pci, linux-acpi, tn, hanjun.guo, okaya, robh+dt,
	frowand.list, devicetree, linux-kernel, sudeep.holla, rjw, lenb,
	catalin.marinas, arnd, linux-arch, j.neuschaefer, geert,
	magnus.damm

Hi Sricharan,

Thank you for the patch.

On Thursday 18 May 2017 20:24:15 Sricharan R wrote:
> While deferring the probe of IOMMU masters, xlate and
> add_device callbacks called from of_iommu_configure
> can pass back error values like -ENODEV, which means
> the IOMMU cannot be connected with that master for real
> reasons. Before the IOMMU probe deferral, all such errors
> were ignored. Now all those errors are propagated back,
> killing the master's probe for such errors. Instead ignore
> all the errors except EPROBE_DEFER, which is the only one
> of concern and let the master work without IOMMU, thus
> restoring the old behavior.
> 
> 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 <magnus.damn@gmail.com>
> Signed-off-by: Sricharan R <sricharan@codeaurora.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> [V4] Reworded commit log and changed dev_info to dev_dbg
> 
>  drivers/iommu/of_iommu.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> index e6e9bec..19779b8 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -237,6 +237,12 @@ const struct iommu_ops *of_iommu_configure(struct
> device *dev, ops = ERR_PTR(err);
>  	}
> 
> +	/* Ignore all other errors apart from EPROBE_DEFER */
> +	if (IS_ERR(ops) && (PTR_ERR(ops) != -EPROBE_DEFER)) {
> +		dev_dbg(dev, "Adding to IOMMU failed: %ld\n", PTR_ERR(ops));
> +		ops = NULL;
> +	}
> +
>  	return ops;
>  }

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH V4 3/4] ACPI/IORT: Ignore all errors except EPROBE_DEFER
  2017-05-18 14:54 ` [PATCH V4 3/4] ACPI/IORT: " Sricharan R
@ 2017-05-22 10:37   ` Lorenzo Pieralisi
  2017-05-22 11:05     ` Sricharan R
  0 siblings, 1 reply; 9+ messages in thread
From: Lorenzo Pieralisi @ 2017-05-22 10:37 UTC (permalink / raw)
  To: Sricharan R
  Cc: robin.murphy, will.deacon, joro, iommu, linux-arm-kernel,
	linux-arm-msm, m.szyprowski, bhelgaas, linux-pci, linux-acpi, tn,
	hanjun.guo, okaya, robh+dt, frowand.list, devicetree,
	linux-kernel, sudeep.holla, rjw, lenb, catalin.marinas, arnd,
	linux-arch, laurent.pinchart, j.neuschaefer, geert, magnus.damm

Hi Sricharan,

On Thu, May 18, 2017 at 08:24:16PM +0530, Sricharan R wrote:
> While deferring the probe of IOMMU masters, xlate and
> add_device callbacks called from iort_iommu_configure
> can pass back error values like -ENODEV, which means
> the IOMMU cannot be connected with that master for real
> reasons. Before the IOMMU probe deferral, all such errors
> were ignored. Now all those errors are propagated back,
> killing the master's probe for such errors. Instead ignore
> all the errors except EPROBE_DEFER, which is the only one
> of concern and let the master work without IOMMU, thus
> restoring the old behavior.
> 
> Fixes: 5a1bb638d567 ("drivers: acpi: Handle IOMMU lookup failure with deferred probing or error")
> Signed-off-by: Sricharan R <sricharan@codeaurora.org>
> ---
> [V4] Added this patch newly.
> 
>  drivers/acpi/arm64/iort.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index c5fecf9..16e101f 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -782,6 +782,12 @@ const struct iommu_ops *iort_iommu_configure(struct device *dev)
>  	if (err)
>  		ops = ERR_PTR(err);
>  
> +	/* Ignore all other errors apart from EPROBE_DEFER */
> +	if (IS_ERR(ops) && (PTR_ERR(ops) != -EPROBE_DEFER)) {
> +		dev_dbg(dev, "Adding to IOMMU failed: %ld\n", PTR_ERR(ops));
> +		ops = NULL;
> +	}
> +
>  	return ops;
>  }
>  

It is getting a bit convoluted. Is not it better to propagate the
error back and let acpi_dma_configure() make a decision accordingly ?

Something like:

-- >8 --
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index e39ec7b..3a10d757 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1371,8 +1371,8 @@ int acpi_dma_configure(struct device *dev, enum dev_dma_attr attr)
 	iort_set_dma_mask(dev);
 
 	iommu = iort_iommu_configure(dev);
-	if (IS_ERR(iommu))
-		return PTR_ERR(iommu);
+	if (IS_ERR(iommu) && PTR_ERR(iommu) == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
 
 	size = max(dev->coherent_dma_mask, dev->coherent_dma_mask + 1);
 	/*

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH V4 3/4] ACPI/IORT: Ignore all errors except EPROBE_DEFER
  2017-05-22 10:37   ` Lorenzo Pieralisi
@ 2017-05-22 11:05     ` Sricharan R
  2017-05-22 11:31       ` Lorenzo Pieralisi
  0 siblings, 1 reply; 9+ messages in thread
From: Sricharan R @ 2017-05-22 11:05 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: robin.murphy, will.deacon, joro, iommu, linux-arm-kernel,
	linux-arm-msm, m.szyprowski, bhelgaas, linux-pci, linux-acpi, tn,
	hanjun.guo, okaya, robh+dt, frowand.list, devicetree,
	linux-kernel, sudeep.holla, rjw, lenb, catalin.marinas, arnd,
	linux-arch, laurent.pinchart, j.neuschaefer, geert, magnus.damm

Hi Lorenzo,

On 5/22/2017 4:07 PM, Lorenzo Pieralisi wrote:
> Hi Sricharan,
> 
> On Thu, May 18, 2017 at 08:24:16PM +0530, Sricharan R wrote:
>> While deferring the probe of IOMMU masters, xlate and
>> add_device callbacks called from iort_iommu_configure
>> can pass back error values like -ENODEV, which means
>> the IOMMU cannot be connected with that master for real
>> reasons. Before the IOMMU probe deferral, all such errors
>> were ignored. Now all those errors are propagated back,
>> killing the master's probe for such errors. Instead ignore
>> all the errors except EPROBE_DEFER, which is the only one
>> of concern and let the master work without IOMMU, thus
>> restoring the old behavior.
>>
>> Fixes: 5a1bb638d567 ("drivers: acpi: Handle IOMMU lookup failure with deferred probing or error")
>> Signed-off-by: Sricharan R <sricharan@codeaurora.org>
>> ---
>> [V4] Added this patch newly.
>>
>>  drivers/acpi/arm64/iort.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
>> index c5fecf9..16e101f 100644
>> --- a/drivers/acpi/arm64/iort.c
>> +++ b/drivers/acpi/arm64/iort.c
>> @@ -782,6 +782,12 @@ const struct iommu_ops *iort_iommu_configure(struct device *dev)
>>  	if (err)
>>  		ops = ERR_PTR(err);
>>  
>> +	/* Ignore all other errors apart from EPROBE_DEFER */
>> +	if (IS_ERR(ops) && (PTR_ERR(ops) != -EPROBE_DEFER)) {
>> +		dev_dbg(dev, "Adding to IOMMU failed: %ld\n", PTR_ERR(ops));
>> +		ops = NULL;
>> +	}
>> +
>>  	return ops;
>>  }
>>  
> 
> It is getting a bit convoluted. Is not it better to propagate the
> error back and let acpi_dma_configure() make a decision accordingly ?
> 

ok, I was trying to keep it in same way as DT, where of_dma_configure
(here acpi_dma_configure) calls of_iommu_configure(here iort_iommu_configure)
which ends up doing the check. So will have to be changed in both places
for symmetry.

> Something like:
> 
> -- >8 --
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index e39ec7b..3a10d757 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -1371,8 +1371,8 @@ int acpi_dma_configure(struct device *dev, enum dev_dma_attr attr)
>  	iort_set_dma_mask(dev);
>  
>  	iommu = iort_iommu_configure(dev);
> -	if (IS_ERR(iommu))
> -		return PTR_ERR(iommu);
> +	if (IS_ERR(iommu) && PTR_ERR(iommu) == -EPROBE_DEFER)
> +		return -EPROBE_DEFER;

In case of other errors apart from EPROBE_DEFER,
iommu = NULL should be set, before passing it on to
arch_setup_dma_ops.

Regards,
 Sricharan

>  
>  	size = max(dev->coherent_dma_mask, dev->coherent_dma_mask + 1);
>  	/*
> 

-- 
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH V4 3/4] ACPI/IORT: Ignore all errors except EPROBE_DEFER
  2017-05-22 11:05     ` Sricharan R
@ 2017-05-22 11:31       ` Lorenzo Pieralisi
  2017-05-22 13:02         ` Sricharan R
  0 siblings, 1 reply; 9+ messages in thread
From: Lorenzo Pieralisi @ 2017-05-22 11:31 UTC (permalink / raw)
  To: Sricharan R
  Cc: robin.murphy, will.deacon, joro, iommu, linux-arm-kernel,
	linux-arm-msm, m.szyprowski, bhelgaas, linux-pci, linux-acpi, tn,
	hanjun.guo, okaya, robh+dt, frowand.list, devicetree,
	linux-kernel, sudeep.holla, rjw, lenb, catalin.marinas, arnd,
	linux-arch, laurent.pinchart, j.neuschaefer, geert, magnus.damm

On Mon, May 22, 2017 at 04:35:43PM +0530, Sricharan R wrote:
> Hi Lorenzo,
> 
> On 5/22/2017 4:07 PM, Lorenzo Pieralisi wrote:
> > Hi Sricharan,
> > 
> > On Thu, May 18, 2017 at 08:24:16PM +0530, Sricharan R wrote:
> >> While deferring the probe of IOMMU masters, xlate and
> >> add_device callbacks called from iort_iommu_configure
> >> can pass back error values like -ENODEV, which means
> >> the IOMMU cannot be connected with that master for real
> >> reasons. Before the IOMMU probe deferral, all such errors
> >> were ignored. Now all those errors are propagated back,
> >> killing the master's probe for such errors. Instead ignore
> >> all the errors except EPROBE_DEFER, which is the only one
> >> of concern and let the master work without IOMMU, thus
> >> restoring the old behavior.
> >>
> >> Fixes: 5a1bb638d567 ("drivers: acpi: Handle IOMMU lookup failure with deferred probing or error")
> >> Signed-off-by: Sricharan R <sricharan@codeaurora.org>
> >> ---
> >> [V4] Added this patch newly.
> >>
> >>  drivers/acpi/arm64/iort.c | 6 ++++++
> >>  1 file changed, 6 insertions(+)
> >>
> >> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> >> index c5fecf9..16e101f 100644
> >> --- a/drivers/acpi/arm64/iort.c
> >> +++ b/drivers/acpi/arm64/iort.c
> >> @@ -782,6 +782,12 @@ const struct iommu_ops *iort_iommu_configure(struct device *dev)
> >>  	if (err)
> >>  		ops = ERR_PTR(err);
> >>  
> >> +	/* Ignore all other errors apart from EPROBE_DEFER */
> >> +	if (IS_ERR(ops) && (PTR_ERR(ops) != -EPROBE_DEFER)) {
> >> +		dev_dbg(dev, "Adding to IOMMU failed: %ld\n", PTR_ERR(ops));
> >> +		ops = NULL;
> >> +	}
> >> +
> >>  	return ops;
> >>  }
> >>  
> > 
> > It is getting a bit convoluted. Is not it better to propagate the
> > error back and let acpi_dma_configure() make a decision accordingly ?
> > 
> 
> ok, I was trying to keep it in same way as DT, where of_dma_configure
> (here acpi_dma_configure) calls of_iommu_configure(here iort_iommu_configure)
> which ends up doing the check. So will have to be changed in both places
> for symmetry.
> 
> > Something like:
> > 
> > -- >8 --
> > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > index e39ec7b..3a10d757 100644
> > --- a/drivers/acpi/scan.c
> > +++ b/drivers/acpi/scan.c
> > @@ -1371,8 +1371,8 @@ int acpi_dma_configure(struct device *dev, enum dev_dma_attr attr)
> >  	iort_set_dma_mask(dev);
> >  
> >  	iommu = iort_iommu_configure(dev);
> > -	if (IS_ERR(iommu))
> > -		return PTR_ERR(iommu);
> > +	if (IS_ERR(iommu) && PTR_ERR(iommu) == -EPROBE_DEFER)
> > +		return -EPROBE_DEFER;
> 
> In case of other errors apart from EPROBE_DEFER,
> iommu = NULL should be set, before passing it on to
> arch_setup_dma_ops.

Ah yes, sorry, that makes it uglier :(

Point is, it does not make sense to me to have of/acpi_dma_configure()
check a return code with (IS_ERR()) whilst we know the only error
code you are allowed to handle is -EPROBE_DEFER, while at it it is
better to make the -EPROBE_DEFER check explicit (otherwise it may
seem that of/acpi_dma_configure() can handle an error code that
is different from -EPROBE_DEFER - but they don't), no big deal either
way though.

Lorenzo

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH V4 3/4] ACPI/IORT: Ignore all errors except EPROBE_DEFER
  2017-05-22 11:31       ` Lorenzo Pieralisi
@ 2017-05-22 13:02         ` Sricharan R
  0 siblings, 0 replies; 9+ messages in thread
From: Sricharan R @ 2017-05-22 13:02 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: robin.murphy, will.deacon, joro, iommu, linux-arm-kernel,
	linux-arm-msm, m.szyprowski, bhelgaas, linux-pci, linux-acpi, tn,
	hanjun.guo, okaya, robh+dt, frowand.list, devicetree,
	linux-kernel, sudeep.holla, rjw, lenb, catalin.marinas, arnd,
	linux-arch, laurent.pinchart, j.neuschaefer, geert, magnus.damm

Hi Lorenzo,

On 5/22/2017 5:01 PM, Lorenzo Pieralisi wrote:
> On Mon, May 22, 2017 at 04:35:43PM +0530, Sricharan R wrote:
>> Hi Lorenzo,
>>
>> On 5/22/2017 4:07 PM, Lorenzo Pieralisi wrote:
>>> Hi Sricharan,
>>>
>>> On Thu, May 18, 2017 at 08:24:16PM +0530, Sricharan R wrote:
>>>> While deferring the probe of IOMMU masters, xlate and
>>>> add_device callbacks called from iort_iommu_configure
>>>> can pass back error values like -ENODEV, which means
>>>> the IOMMU cannot be connected with that master for real
>>>> reasons. Before the IOMMU probe deferral, all such errors
>>>> were ignored. Now all those errors are propagated back,
>>>> killing the master's probe for such errors. Instead ignore
>>>> all the errors except EPROBE_DEFER, which is the only one
>>>> of concern and let the master work without IOMMU, thus
>>>> restoring the old behavior.
>>>>
>>>> Fixes: 5a1bb638d567 ("drivers: acpi: Handle IOMMU lookup failure with deferred probing or error")
>>>> Signed-off-by: Sricharan R <sricharan@codeaurora.org>
>>>> ---
>>>> [V4] Added this patch newly.
>>>>
>>>>  drivers/acpi/arm64/iort.c | 6 ++++++
>>>>  1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
>>>> index c5fecf9..16e101f 100644
>>>> --- a/drivers/acpi/arm64/iort.c
>>>> +++ b/drivers/acpi/arm64/iort.c
>>>> @@ -782,6 +782,12 @@ const struct iommu_ops *iort_iommu_configure(struct device *dev)
>>>>  	if (err)
>>>>  		ops = ERR_PTR(err);
>>>>  
>>>> +	/* Ignore all other errors apart from EPROBE_DEFER */
>>>> +	if (IS_ERR(ops) && (PTR_ERR(ops) != -EPROBE_DEFER)) {
>>>> +		dev_dbg(dev, "Adding to IOMMU failed: %ld\n", PTR_ERR(ops));
>>>> +		ops = NULL;
>>>> +	}
>>>> +
>>>>  	return ops;
>>>>  }
>>>>  
>>>
>>> It is getting a bit convoluted. Is not it better to propagate the
>>> error back and let acpi_dma_configure() make a decision accordingly ?
>>>
>>
>> ok, I was trying to keep it in same way as DT, where of_dma_configure
>> (here acpi_dma_configure) calls of_iommu_configure(here iort_iommu_configure)
>> which ends up doing the check. So will have to be changed in both places
>> for symmetry.
>>
>>> Something like:
>>>
>>> -- >8 --
>>> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
>>> index e39ec7b..3a10d757 100644
>>> --- a/drivers/acpi/scan.c
>>> +++ b/drivers/acpi/scan.c
>>> @@ -1371,8 +1371,8 @@ int acpi_dma_configure(struct device *dev, enum dev_dma_attr attr)
>>>  	iort_set_dma_mask(dev);
>>>  
>>>  	iommu = iort_iommu_configure(dev);
>>> -	if (IS_ERR(iommu))
>>> -		return PTR_ERR(iommu);
>>> +	if (IS_ERR(iommu) && PTR_ERR(iommu) == -EPROBE_DEFER)
>>> +		return -EPROBE_DEFER;
>>
>> In case of other errors apart from EPROBE_DEFER,
>> iommu = NULL should be set, before passing it on to
>> arch_setup_dma_ops.
> 
> Ah yes, sorry, that makes it uglier :(
> 
> Point is, it does not make sense to me to have of/acpi_dma_configure()
> check a return code with (IS_ERR()) whilst we know the only error
> code you are allowed to handle is -EPROBE_DEFER, while at it it is
> better to make the -EPROBE_DEFER check explicit (otherwise it may
> seem that of/acpi_dma_configure() can handle an error code that
> is different from -EPROBE_DEFER - but they don't), no big deal either
> way though.
> 

ok, in which case, the above check that you have shown
to be added additionally to the original patch.

Regards,
 Sricharan

-- 
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2017-05-22 13:02 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-18 14:54 [PATCH V4 1/4] iommu: of: Fix check for returning EPROBE_DEFER Sricharan R
2017-05-18 14:54 ` [PATCH V4 2/4] iommu: of: Ignore all errors except EPROBE_DEFER Sricharan R
2017-05-18 14:56   ` Laurent Pinchart
2017-05-18 14:54 ` [PATCH V4 3/4] ACPI/IORT: " Sricharan R
2017-05-22 10:37   ` Lorenzo Pieralisi
2017-05-22 11:05     ` Sricharan R
2017-05-22 11:31       ` Lorenzo Pieralisi
2017-05-22 13:02         ` Sricharan R
2017-05-18 14:54 ` [PATCH V4 4/4] ARM: dma-mapping: Don't tear third-party mappings Sricharan R

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).