diff mbox

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

Message ID 1479986420-30859-5-git-send-email-m.szyprowski@samsung.com
State New
Headers show

Commit Message

Marek Szyprowski Nov. 24, 2016, 11:20 a.m. UTC 
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(-)

-- 
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 Nov. 24, 2016, 12:25 p.m. UTC | #1 
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@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)) {


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

>  

> 


--
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
Marek Szyprowski Nov. 24, 2016, 1:05 p.m. UTC | #2 
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@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)) {

> 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

--
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
Joerg Roedel Nov. 29, 2016, 4:48 p.m. UTC | #3 
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@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);

> +	}


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

--
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
Marek Szyprowski Nov. 30, 2016, 9:05 a.m. UTC | #4 
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@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);

>> +	}

> 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

--
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
diff mbox

Patch

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