[PATCH 0/5] Fixes for Exynos IOMMU driver

[PATCH 0/5] Fixes for Exynos IOMMU driver

[Marek Szyprowski]

Hello,

This is collection of fixes for Exynos IOMMU driver. Patches 1-2 are
resend of the patches, which got lost on mainline list. Patch 3 adds
a protection for the issue, which arises when IOMMU deferred probe
patches will be merged. Patches 4-5 are fixes for default_domain
handling.

Patches are based on current iommu/next branch.

Best regards
Marek Szyprowski
Samsung R&D Institute Poland

Patch summary:

Marek Szyprowski (5):
  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: Add default_domain check in iommu_attach_device
  iommu/exynos: Properly release device from the default domain in
    ->remove

 drivers/iommu/exynos-iommu.c | 44 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 37 insertions(+), 7 deletions(-)

-- 
1.9.1

[PATCH 1/5] 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 57ba0d3091ea..ac726e1760de 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 2/5] 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 | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index ac726e1760de..e7851cffbbee 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -744,6 +744,7 @@ 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));
+	BUG_ON(dma_mapping_error(dma_dev, handle));
 
 	spin_lock_init(&domain->lock);
 	spin_lock_init(&domain->pgtablelock);
@@ -898,6 +899,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 +911,9 @@ 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);
+		BUG_ON(dma_mapping_error(dma_dev, handle));
 
 		/*
 		 * If pre-fetched SLPD is a faulty SLPD in zero_l2_table,
-- 
1.9.1

Re: [PATCH 2/5] iommu/exynos: Fix warnings from DMA-debug

[Joerg Roedel]

On Thu, Nov 24, 2016 at 12:20:17PM +0100, Marek Szyprowski wrote:
> @@ -744,6 +744,7 @@ 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));
> +	BUG_ON(dma_mapping_error(dma_dev, handle));

A BUG_ON is a bad way of handling this. Please propagate the error
upwards, there is no need to crash the kernel because of a failure like
this.


	Joerg

[PATCH 3/5] 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 e7851cffbbee..426b1534d4d3 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -1247,7 +1247,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;
@@ -1266,6 +1266,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

[PATCH 4/5] iommu/exynos: Add default_domain check in iommu_attach_device

[Marek Szyprowski]

This patch adds default_domain check before calling
exynos_iommu_detach_device. This path was intended only to cope with
default domains, which are automatically attached by the iommu core, so
return error if user tries to attach device, which is already attached
to other (non-default) domain.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/iommu/exynos-iommu.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index 426b1534d4d3..63d9358a6d9c 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -859,8 +859,17 @@ static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain,
 	if (!has_sysmmu(dev))
 		return -ENODEV;
 
-	if (owner->domain)
+	if (owner->domain) {
+		struct iommu_group *group = iommu_group_get(dev);
+
+		if (!group ||
+		    owner->domain != iommu_group_default_domain(group)) {
+			iommu_group_put(group);
+			return -EINVAL;
+		}
+		iommu_group_put(group);
 		exynos_iommu_detach_device(owner->domain, dev);
+	}
 
 	mutex_lock(&owner->rpm_lock);
 
-- 
1.9.1

Re: [PATCH 4/5] iommu/exynos: Add default_domain check in iommu_attach_device

[Robin Murphy]

Hi Marek,

On 24/11/16 11:20, Marek Szyprowski wrote:
> This patch adds default_domain check before calling
> exynos_iommu_detach_device. This path was intended only to cope with
> default domains, which are automatically attached by the iommu core, so
> return error if user tries to attach device, which is already attached
> to other (non-default) domain.

I think this only applies to the current state of 32-bit ARM, where the
initial allocation of a default domain fails without CONFIG_IOMMU_DMA.
Since the device has a group, iommu_attach_device() will end up calling
into __iommu_attach_group(), which does this check before calling the
driver's .attach_dev:

	if (group->default_domain && group->domain != group->default_domain)
		return -EBUSY;

which if group->default_domain is non-NULL would make the new check
below redundant unless owner->domain can change independently of
dev->iommu_group->domain, but that sounds like it would be a bigger
problem anyway.

> Signed-off-by: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---
>  drivers/iommu/exynos-iommu.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> index 426b1534d4d3..63d9358a6d9c 100644
> --- a/drivers/iommu/exynos-iommu.c
> +++ b/drivers/iommu/exynos-iommu.c
> @@ -859,8 +859,17 @@ static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain,
>  	if (!has_sysmmu(dev))
>  		return -ENODEV;
>  
> -	if (owner->domain)
> +	if (owner->domain) {
> +		struct iommu_group *group = iommu_group_get(dev);
> +
> +		if (!group ||
> +		    owner->domain != iommu_group_default_domain(group)) {

Similarly, I think this prevents reattaching to a default domain from
iommu_detach_device(), since __iommu_detach_group() won't call
.detach_dev first if default_domain is non-NULL, therefore owner->domain
is presumably still pointing to the outgoing non-default domain.

Robin.

> +			iommu_group_put(group);
> +			return -EINVAL;
> +		}
> +		iommu_group_put(group);
>  		exynos_iommu_detach_device(owner->domain, dev);
> +	}
>  
>  	mutex_lock(&owner->rpm_lock);
>  
> 

Re: [PATCH 4/5] iommu/exynos: Add default_domain check in iommu_attach_device

[Marek Szyprowski]

Hi Robin,


On 2016-11-24 13:25, Robin Murphy wrote:
> Hi Marek,
>
> On 24/11/16 11:20, Marek Szyprowski wrote:
>> This patch adds default_domain check before calling
>> exynos_iommu_detach_device. This path was intended only to cope with
>> default domains, which are automatically attached by the iommu core, so
>> return error if user tries to attach device, which is already attached
>> to other (non-default) domain.
> I think this only applies to the current state of 32-bit ARM, where the
> initial allocation of a default domain fails without CONFIG_IOMMU_DMA.
> Since the device has a group, iommu_attach_device() will end up calling
> into __iommu_attach_group(), which does this check before calling the
> driver's .attach_dev:
>
> 	if (group->default_domain && group->domain != group->default_domain)
> 		return -EBUSY;
>
> which if group->default_domain is non-NULL would make the new check
> below redundant unless owner->domain can change independently of
> dev->iommu_group->domain, but that sounds like it would be a bigger
> problem anyway.

Thanks for your review. Default domains are used on ARM64 and they are
automatically attached by IOMMU core. This was the reason for looking into
this. But you are right, that there is no need for the additional check, as
this is already handled in the iommu core.

>> Signed-off-by: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>> ---
>>   drivers/iommu/exynos-iommu.c | 11 ++++++++++-
>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
>> index 426b1534d4d3..63d9358a6d9c 100644
>> --- a/drivers/iommu/exynos-iommu.c
>> +++ b/drivers/iommu/exynos-iommu.c
>> @@ -859,8 +859,17 @@ static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain,
>>   	if (!has_sysmmu(dev))
>>   		return -ENODEV;
>>   
>> -	if (owner->domain)
>> +	if (owner->domain) {
>> +		struct iommu_group *group = iommu_group_get(dev);
>> +
>> +		if (!group ||
>> +		    owner->domain != iommu_group_default_domain(group)) {
> Similarly, I think this prevents reattaching to a default domain from
> iommu_detach_device(), since __iommu_detach_group() won't call
> .detach_dev first if default_domain is non-NULL, therefore owner->domain
> is presumably still pointing to the outgoing non-default domain.

Right, I forgot about reattaching to the default domain. Please drop 
this patch,
the previous code was correct.

>
>> +			iommu_group_put(group);
>> +			return -EINVAL;
>> +		}
>> +		iommu_group_put(group);
>>   		exynos_iommu_detach_device(owner->domain, dev);
>> +	}
>>   
>>   	mutex_lock(&owner->rpm_lock);
>>

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 4/5] iommu/exynos: Add default_domain check in iommu_attach_device

[Joerg Roedel]

On Thu, Nov 24, 2016 at 12:20:19PM +0100, Marek Szyprowski wrote:
> This patch adds default_domain check before calling
> exynos_iommu_detach_device. This path was intended only to cope with
> default domains, which are automatically attached by the iommu core, so
> return error if user tries to attach device, which is already attached
> to other (non-default) domain.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---
>  drivers/iommu/exynos-iommu.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> index 426b1534d4d3..63d9358a6d9c 100644
> --- a/drivers/iommu/exynos-iommu.c
> +++ b/drivers/iommu/exynos-iommu.c
> @@ -859,8 +859,17 @@ static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain,
>  	if (!has_sysmmu(dev))
>  		return -ENODEV;
>  
> -	if (owner->domain)
> +	if (owner->domain) {
> +		struct iommu_group *group = iommu_group_get(dev);
> +
> +		if (!group ||
> +		    owner->domain != iommu_group_default_domain(group)) {
> +			iommu_group_put(group);
> +			return -EINVAL;
> +		}
> +		iommu_group_put(group);
>  		exynos_iommu_detach_device(owner->domain, dev);
> +	}

Does this fix any actual bug? The iommu core should take care that the
above never happens. See __iommu_attach_group() function for details.



	Joerg

Re: [PATCH 4/5] iommu/exynos: Add default_domain check in iommu_attach_device

[Marek Szyprowski]

Hi Joerg,


On 2016-11-29 17:48, Joerg Roedel wrote:
> On Thu, Nov 24, 2016 at 12:20:19PM +0100, Marek Szyprowski wrote:
>> This patch adds default_domain check before calling
>> exynos_iommu_detach_device. This path was intended only to cope with
>> default domains, which are automatically attached by the iommu core, so
>> return error if user tries to attach device, which is already attached
>> to other (non-default) domain.
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>> ---
>>   drivers/iommu/exynos-iommu.c | 11 ++++++++++-
>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
>> index 426b1534d4d3..63d9358a6d9c 100644
>> --- a/drivers/iommu/exynos-iommu.c
>> +++ b/drivers/iommu/exynos-iommu.c
>> @@ -859,8 +859,17 @@ static int exynos_iommu_attach_device(struct iommu_domain *iommu_domain,
>>   	if (!has_sysmmu(dev))
>>   		return -ENODEV;
>>   
>> -	if (owner->domain)
>> +	if (owner->domain) {
>> +		struct iommu_group *group = iommu_group_get(dev);
>> +
>> +		if (!group ||
>> +		    owner->domain != iommu_group_default_domain(group)) {
>> +			iommu_group_put(group);
>> +			return -EINVAL;
>> +		}
>> +		iommu_group_put(group);
>>   		exynos_iommu_detach_device(owner->domain, dev);
>> +	}
> Does this fix any actual bug? The iommu core should take care that the
> above never happens. See __iommu_attach_group() function for details.

I've made this patch, when I was adding default domain handling in
exynos_iommu_remove_device(), but as it has been already pointed by Robin,
there is no point for the above check in exynos_iommu_attach_device(),
because it is handled in the iommu core. Please ignore this patch.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

[PATCH 5/5] 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@samsung.com>
---
 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 63d9358a6d9c..0223db074d03 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -1245,9 +1245,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