[PATCH v2 0/4] Fixes for Exynos IOMMU driver

[PATCH v2 0/4] Fixes for Exynos IOMMU driver

[Marek Szyprowski]

Hello,

This is collection of fixes for Exynos IOMMU driver. Patches 1-2 are
resend and update of the patches, which got lost on mainline list.
Patches 3-4 solve the issue, which arises when IOMMU deferred probe
patches will be merged.

Patches are based on current iommu/next branch.

Best regards
Marek Szyprowski
Samsung R&D Institute Poland

Changelog:

v2:
- dropped "iommu/exynos: Add default_domain check in iommu_attach_device"
  patch, this is handled in iommu core
- replaced BUG_ON with proper error propagation

v1: http://www.spinics.net/lists/linux-samsung-soc/msg56354.html
- initial version

Patch summary:

Marek Szyprowski (4):
  iommu/exynos: Improve page fault debug message
  iommu/exynos: Fix warnings from DMA-debug
  iommu/exynos: Ensure that SYSMMU is added only once to its master
    device
  iommu/exynos: Properly release device from the default domain in
    ->remove

 drivers/iommu/exynos-iommu.c | 39 +++++++++++++++++++++++++++++++++------
 1 file changed, 33 insertions(+), 6 deletions(-)

-- 
1.9.1

[PATCH v2 1/4] iommu/exynos: Improve page fault debug message

[Marek Szyprowski]

Add master device name to default IOMMU fault message to make easier to
find which device triggered the fault. While at it, move printing some
information (like page table base and first level entry addresses) to
dev_dbg(), because those are typically not very useful for typical device
driver user/developer not equipped with hardware debugging tools.

Signed-off-by: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 drivers/iommu/exynos-iommu.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index b79e4c452b8b..058ee8425f35 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -381,13 +381,14 @@ static void show_fault_information(struct sysmmu_drvdata *data,
 {
 	sysmmu_pte_t *ent;
 
-	dev_err(data->sysmmu, "%s FAULT occurred at %#x (page table base: %pa)\n",
-		finfo->name, fault_addr, &data->pgtable);
+	dev_err(data->sysmmu, "%s: %s FAULT occurred at %#x\n",
+		dev_name(data->master), finfo->name, fault_addr);
+	dev_dbg(data->sysmmu, "Page table base: %pa\n", &data->pgtable);
 	ent = section_entry(phys_to_virt(data->pgtable), fault_addr);
-	dev_err(data->sysmmu, "\tLv1 entry: %#x\n", *ent);
+	dev_dbg(data->sysmmu, "\tLv1 entry: %#x\n", *ent);
 	if (lv1ent_page(ent)) {
 		ent = page_entry(ent, fault_addr);
-		dev_err(data->sysmmu, "\t Lv2 entry: %#x\n", *ent);
+		dev_dbg(data->sysmmu, "\t Lv2 entry: %#x\n", *ent);
 	}
 }
 
-- 
1.9.1

[PATCH v2 2/4] iommu/exynos: Fix warnings from DMA-debug

[Marek Szyprowski]

Add a simple checks for dma_map_single() return value to make DMA-debug
checker happly. Exynos IOMMU on Samsung Exynos SoCs always use device,
which has linear DMA mapping ops (dma address is equal to physical memory
address), so no failures are returned from dma_map_single().

Signed-off-by: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 drivers/iommu/exynos-iommu.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index 058ee8425f35..b0d537e6a445 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -744,6 +744,8 @@ static struct iommu_domain *exynos_iommu_domain_alloc(unsigned type)
 				DMA_TO_DEVICE);
 	/* For mapping page table entries we rely on dma == phys */
 	BUG_ON(handle != virt_to_phys(domain->pgtable));
+	if (dma_mapping_error(dma_dev, handle))
+		goto err_lv2ent;
 
 	spin_lock_init(&domain->lock);
 	spin_lock_init(&domain->pgtablelock);
@@ -755,6 +757,8 @@ static struct iommu_domain *exynos_iommu_domain_alloc(unsigned type)
 
 	return &domain->domain;
 
+err_lv2ent:
+	free_pages((unsigned long)domain->lv2entcnt, 1);
 err_counter:
 	free_pages((unsigned long)domain->pgtable, 2);
 err_dma_cookie:
@@ -898,6 +902,7 @@ static sysmmu_pte_t *alloc_lv2entry(struct exynos_iommu_domain *domain,
 	}
 
 	if (lv1ent_fault(sent)) {
+		dma_addr_t handle;
 		sysmmu_pte_t *pent;
 		bool need_flush_flpd_cache = lv1ent_zero(sent);
 
@@ -909,7 +914,12 @@ static sysmmu_pte_t *alloc_lv2entry(struct exynos_iommu_domain *domain,
 		update_pte(sent, mk_lv1ent_page(virt_to_phys(pent)));
 		kmemleak_ignore(pent);
 		*pgcounter = NUM_LV2ENTRIES;
-		dma_map_single(dma_dev, pent, LV2TABLE_SIZE, DMA_TO_DEVICE);
+		handle = dma_map_single(dma_dev, pent, LV2TABLE_SIZE,
+					DMA_TO_DEVICE);
+		if (dma_mapping_error(dma_dev, handle)) {
+			kmem_cache_free(lv2table_kmem_cache, pent);
+			return ERR_PTR(-EADDRINUSE);
+		}
 
 		/*
 		 * If pre-fetched SLPD is a faulty SLPD in zero_l2_table,
-- 
1.9.1

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

[Marek Szyprowski]

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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 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;
 
-- 
1.9.1

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

[Robin Murphy]

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-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org/msg15333.html

> Signed-off-by: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---
>  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;
>  
> 

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

[Sricharan]

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

[PATCH v2 4/4] iommu/exynos: Properly release device from the default domain in ->remove

[Marek Szyprowski]

IOMMU core doesn't detach device from the default domain before calling
->iommu_remove_device, so check that and do the proper cleanup or
warn if device is still attached to non-default domain.

Signed-off-by: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 drivers/iommu/exynos-iommu.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index 8bf5a06a6c01..37d9738bd023 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -1242,9 +1242,21 @@ static int exynos_iommu_add_device(struct device *dev)
 
 static void exynos_iommu_remove_device(struct device *dev)
 {
+	struct exynos_iommu_owner *owner = dev->archdata.iommu;
+
 	if (!has_sysmmu(dev))
 		return;
 
+	if (owner->domain) {
+		struct iommu_group *group = iommu_group_get(dev);
+
+		if (group) {
+			WARN_ON(owner->domain !=
+				iommu_group_default_domain(group));
+			exynos_iommu_detach_device(owner->domain, dev);
+			iommu_group_put(group);
+		}
+	}
 	iommu_group_remove_device(dev);
 }
 
-- 
1.9.1

Re: [PATCH v2 0/4] Fixes for Exynos IOMMU driver

[Joerg Roedel]

On Mon, Jan 09, 2017 at 01:03:52PM +0100, Marek Szyprowski wrote:
> Marek Szyprowski (4):
>   iommu/exynos: Improve page fault debug message
>   iommu/exynos: Fix warnings from DMA-debug
>   iommu/exynos: Ensure that SYSMMU is added only once to its master
>     device
>   iommu/exynos: Properly release device from the default domain in
>     ->remove
> 
>  drivers/iommu/exynos-iommu.c | 39 +++++++++++++++++++++++++++++++++------
>  1 file changed, 33 insertions(+), 6 deletions(-)

Applied, thanks.