[v2,3/4] iommu/exynos: Ensure that SYSMMU is added only once to its master device

Message ID 1483963436-29803-4-git-send-email-m.szyprowski@samsung.com
State New
Headers show

Commit Message

Marek Szyprowski Jan. 9, 2017, 12:03 p.m.
This patch prepares Exynos IOMMU driver for deferred probing
support. Once it gets added, of_xlate() callback might be called
more than once for the same SYSMMU controller and master device
(for example it happens when masters device driver fails with
EPROBE_DEFER). This patch adds a check, which ensures that SYSMMU
controller is added to its master device (owner) only once.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

---
 drivers/iommu/exynos-iommu.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Robin Murphy Jan. 9, 2017, 2:30 p.m. | #1
Hi Marek,

On 09/01/17 12:03, Marek Szyprowski wrote:
> This patch prepares Exynos IOMMU driver for deferred probing

> support. Once it gets added, of_xlate() callback might be called

> more than once for the same SYSMMU controller and master device

> (for example it happens when masters device driver fails with

> EPROBE_DEFER). This patch adds a check, which ensures that SYSMMU

> controller is added to its master device (owner) only once.


FWIW, I think it would be better to address that issue in the
probe-deferral code itself, rather than fixing up individual IOMMU
drivers. In fact, I *think* it's already covered in the latest version
of the of_iommu_configure() changes[1], specifically this bit:

	...
	if (fwspec) {
		if (fwspec->ops)
			return fwspec->ops;
	...

Which should ensure that a successful IOMMU configuration happens
exactly once between device creation and destruction.

That said, this patch would still add robustness against actual error
conditions, e.g. an erroneous DT which managed to specify the same
phandle twice, although making it "if (WARN_ON(entry == data))" might be
a good idea.

Robin.

[1]:https://www.mail-archive.com/iommu@lists.linux-foundation.org/msg15333.html

> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

> ---

>  drivers/iommu/exynos-iommu.c | 6 +++++-

>  1 file changed, 5 insertions(+), 1 deletion(-)

> 

> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c

> index b0d537e6a445..8bf5a06a6c01 100644

> --- a/drivers/iommu/exynos-iommu.c

> +++ b/drivers/iommu/exynos-iommu.c

> @@ -1253,7 +1253,7 @@ static int exynos_iommu_of_xlate(struct device *dev,

>  {

>  	struct exynos_iommu_owner *owner = dev->archdata.iommu;

>  	struct platform_device *sysmmu = of_find_device_by_node(spec->np);

> -	struct sysmmu_drvdata *data;

> +	struct sysmmu_drvdata *data, *entry;

>  

>  	if (!sysmmu)

>  		return -ENODEV;

> @@ -1272,6 +1272,10 @@ static int exynos_iommu_of_xlate(struct device *dev,

>  		dev->archdata.iommu = owner;

>  	}

>  

> +	list_for_each_entry(entry, &owner->controllers, owner_node)

> +		if (entry == data)

> +			return 0;

> +

>  	list_add_tail(&data->owner_node, &owner->controllers);

>  	data->master = dev;

>  

> 


--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sricharan R Jan. 10, 2017, 4:34 a.m. | #2
Hi,

>Hi Marek,

>

>On 09/01/17 12:03, Marek Szyprowski wrote:

>> This patch prepares Exynos IOMMU driver for deferred probing

>> support. Once it gets added, of_xlate() callback might be called

>> more than once for the same SYSMMU controller and master device

>> (for example it happens when masters device driver fails with

>> EPROBE_DEFER). This patch adds a check, which ensures that SYSMMU

>> controller is added to its master device (owner) only once.

>

>FWIW, I think it would be better to address that issue in the

>probe-deferral code itself, rather than fixing up individual IOMMU

>drivers. In fact, I *think* it's already covered in the latest version

>of the of_iommu_configure() changes[1], specifically this bit:

>

>	...

>	if (fwspec) {

>		if (fwspec->ops)

>			return fwspec->ops;

>	...

>

>Which should ensure that a successful IOMMU configuration happens

>exactly once between device creation and destruction.

>

>That said, this patch would still add robustness against actual error

>conditions, e.g. an erroneous DT which managed to specify the same

>phandle twice, although making it "if (WARN_ON(entry == data))" might be

>a good idea.

>


Yes, the latest version has the above mentioned fix of just returning back
from of_iommu_configure, if already configured. But anyways since this
patch is doing the check in xlate, which still can be called multiple times
even without deferring, incase of say multiple sids as well. But i
remember that sometime back Marek mentioned that the exynos
iommu currently always has "#iommu-cells = <0>" , but this patch might
be required if that changes as well.

Regards,
 Sricharan

>Robin.

>

>[1]:https://www.mail-archive.com/iommu@lists.linux-foundation.org/msg15333.html

>

>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

>> ---

>>  drivers/iommu/exynos-iommu.c | 6 +++++-

>>  1 file changed, 5 insertions(+), 1 deletion(-)

>>

>> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c

>> index b0d537e6a445..8bf5a06a6c01 100644

>> --- a/drivers/iommu/exynos-iommu.c

>> +++ b/drivers/iommu/exynos-iommu.c

>> @@ -1253,7 +1253,7 @@ static int exynos_iommu_of_xlate(struct device *dev,

>>  {

>>  	struct exynos_iommu_owner *owner = dev->archdata.iommu;

>>  	struct platform_device *sysmmu = of_find_device_by_node(spec->np);

>> -	struct sysmmu_drvdata *data;

>> +	struct sysmmu_drvdata *data, *entry;

>>

>>  	if (!sysmmu)

>>  		return -ENODEV;

>> @@ -1272,6 +1272,10 @@ static int exynos_iommu_of_xlate(struct device *dev,

>>  		dev->archdata.iommu = owner;

>>  	}

>>

>> +	list_for_each_entry(entry, &owner->controllers, owner_node)

>> +		if (entry == data)

>> +			return 0;

>> +

>>  	list_add_tail(&data->owner_node, &owner->controllers);

>>  	data->master = dev;

>>

>>

>

>_______________________________________________

>iommu mailing list

>iommu@lists.linux-foundation.org

>https://lists.linuxfoundation.org/mailman/listinfo/iommu


--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch hide | download patch | download mbox

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index b0d537e6a445..8bf5a06a6c01 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -1253,7 +1253,7 @@  static int exynos_iommu_of_xlate(struct device *dev,
 {
 	struct exynos_iommu_owner *owner = dev->archdata.iommu;
 	struct platform_device *sysmmu = of_find_device_by_node(spec->np);
-	struct sysmmu_drvdata *data;
+	struct sysmmu_drvdata *data, *entry;
 
 	if (!sysmmu)
 		return -ENODEV;
@@ -1272,6 +1272,10 @@  static int exynos_iommu_of_xlate(struct device *dev,
 		dev->archdata.iommu = owner;
 	}
 
+	list_for_each_entry(entry, &owner->controllers, owner_node)
+		if (entry == data)
+			return 0;
+
 	list_add_tail(&data->owner_node, &owner->controllers);
 	data->master = dev;