[RFC V2] iommu: correct group reference count

[RFC V2] iommu: correct group reference count

[Peng Fan]

The basic flow for iommu_group_for_dev is:
iommu_group_get_for_dev
        |->  iommu_group_get   : increase reference count by 1.
             return group;
        |->  ops->device_group : Init/increase reference count to/by 1.
             iommu_group_add_device  : Increase reference count by 1.
             return group;

We can see that ops->device_group and iommu_group_add_device will together
increase the iommu group reference count by 2. Actually we only need 1,
but not 2. So we need add iommu_group_put after iommu_group_add_device
to make sure iommu_group_get_for_dev only increase reference count by 1.

Signed-off-by: Peng Fan <van.freenix@gmail.com>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: Will Deacon <will.deacon@arm.com>
---

V1 thread: https://lkml.org/lkml/2015/11/3/304
Changes V2:
 I did not see the update about device_group when I worked out V1. So
 redo the patch and refine commit msg and rebased to latest linus'
 linux master tree.

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

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index abae363..9c1971b 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -852,10 +852,10 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev)
 	}
 
 	ret = iommu_group_add_device(group, dev);
-	if (ret) {
-		iommu_group_put(group);
+	iommu_group_put(group);
+
+	if (ret)
 		return ERR_PTR(ret);
-	}
 
 	return group;
 }
-- 
1.8.4

Re: [RFC V2] iommu: correct group reference count

[Will Deacon]

On Mon, Nov 09, 2015 at 02:13:28PM +0800, Peng Fan wrote:
> The basic flow for iommu_group_for_dev is:
> iommu_group_get_for_dev
>         |->  iommu_group_get   : increase reference count by 1.
>              return group;
>         |->  ops->device_group : Init/increase reference count to/by 1.
>              iommu_group_add_device  : Increase reference count by 1.
>              return group;
> 
> We can see that ops->device_group and iommu_group_add_device will together
> increase the iommu group reference count by 2. Actually we only need 1,
> but not 2. So we need add iommu_group_put after iommu_group_add_device
> to make sure iommu_group_get_for_dev only increase reference count by 1.
> 
> Signed-off-by: Peng Fan <van.freenix@gmail.com>
> Cc: Joerg Roedel <joro@8bytes.org>
> Cc: Will Deacon <will.deacon@arm.com>
> ---
> 
> V1 thread: https://lkml.org/lkml/2015/11/3/304
> Changes V2:
>  I did not see the update about device_group when I worked out V1. So
>  redo the patch and refine commit msg and rebased to latest linus'
>  linux master tree.
> 
>  drivers/iommu/iommu.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index abae363..9c1971b 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -852,10 +852,10 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev)
>  	}
>  
>  	ret = iommu_group_add_device(group, dev);
> -	if (ret) {
> -		iommu_group_put(group);
> +	iommu_group_put(group);
> +
> +	if (ret)
>  		return ERR_PTR(ret);
> -	}

Hmm, I don't think this is correct either:

  (1) It doesn't help the arm-smmu driver (which deals with platform
      devices too)

  (2) It breaks IOMMU drivers that currently have the explicit put already
      (e.g. amd-iommu.c:init_iommu_group)

The easiest way for you to proceed is to extend your original patch so
that it also adds an iommu_group_put to the arm-smmu-v3 driver after
a successful iommu_group_get_for_dev.

Will

Re: [RFC V2] iommu: correct group reference count

[Peng Fan]

Hi Will,

On Mon, Nov 09, 2015 at 10:10:59AM +0000, Will Deacon wrote:
>On Mon, Nov 09, 2015 at 02:13:28PM +0800, Peng Fan wrote:
>> The basic flow for iommu_group_for_dev is:
>> iommu_group_get_for_dev
>>         |->  iommu_group_get   : increase reference count by 1.
>>              return group;
>>         |->  ops->device_group : Init/increase reference count to/by 1.
>>              iommu_group_add_device  : Increase reference count by 1.
>>              return group;
>> 
>> We can see that ops->device_group and iommu_group_add_device will together
>> increase the iommu group reference count by 2. Actually we only need 1,
>> but not 2. So we need add iommu_group_put after iommu_group_add_device
>> to make sure iommu_group_get_for_dev only increase reference count by 1.
>> 
>> Signed-off-by: Peng Fan <van.freenix@gmail.com>
>> Cc: Joerg Roedel <joro@8bytes.org>
>> Cc: Will Deacon <will.deacon@arm.com>
>> ---
>> 
>> V1 thread: https://lkml.org/lkml/2015/11/3/304
>> Changes V2:
>>  I did not see the update about device_group when I worked out V1. So
>>  redo the patch and refine commit msg and rebased to latest linus'
>>  linux master tree.
>> 
>>  drivers/iommu/iommu.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index abae363..9c1971b 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -852,10 +852,10 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev)
>>  	}
>>  
>>  	ret = iommu_group_add_device(group, dev);
>> -	if (ret) {
>> -		iommu_group_put(group);
>> +	iommu_group_put(group);
>> +
>> +	if (ret)
>>  		return ERR_PTR(ret);
>> -	}
>
>Hmm, I don't think this is correct either:
>
>  (1) It doesn't help the arm-smmu driver (which deals with platform
>      devices too)
>
>  (2) It breaks IOMMU drivers that currently have the explicit put already
>      (e.g. amd-iommu.c:init_iommu_group)
>
>The easiest way for you to proceed is to extend your original patch so
>that it also adds an iommu_group_put to the arm-smmu-v3 driver after
>a successful iommu_group_get_for_dev.

I am not sure if the following patch is what you preferred. But this means
that iommu_grou_get_for_dev will increase the group reference count by 2, when
need to create a new group?

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 4e5118a..9c88301 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -1855,6 +1855,9 @@ static int arm_smmu_add_device(struct device *dev)
 	/* Add the new SID */
 	sids[smmu_group->num_sids - 1] = sid;
 	smmu_group->sids = sids;
+
+	iommu_group_put(group);
+
 	return 0;
 
 out_put_group:
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 47dc7a7..af6afce 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1357,6 +1357,8 @@ static int arm_smmu_add_device(struct device *dev)
 	if (IS_ERR(group))
 		return PTR_ERR(group);
 
+	iommu_group_put(group);
+
 	return 0;
 }

Thanks,
Peng.

>
>Will

Re: [RFC V2] iommu: correct group reference count

[Alex Williamson]

On Mon, 2015-11-09 at 14:13 +0800, Peng Fan wrote:
> The basic flow for iommu_group_for_dev is:
> iommu_group_get_for_dev
>         |->  iommu_group_get   : increase reference count by 1.
>              return group;
>         |->  ops->device_group : Init/increase reference count to/by 1.
>              iommu_group_add_device  : Increase reference count by 1.
>              return group;
> 
> We can see that ops->device_group and iommu_group_add_device will together
> increase the iommu group reference count by 2. Actually we only need 1,
> but not 2. So we need add iommu_group_put after iommu_group_add_device
> to make sure iommu_group_get_for_dev only increase reference count by 1.

The premise seems incorrect to me.  In the first case where
iommu_group_get() provides a group, the reference is increased by 1, but
the device is already a member of the group and therefore already
increases the reference count of the group by 1.  The minimum group
reference at that point is therefore 2.  In the second case, the group
is created, which gives us our first reference, and the device is added,
giving us our second reference.  Therefore the minimum reference count
is also 2.  If we decrement it, allowing the caller to get the group
with a single reference and they release that reference, the group will
be destroyed even though it has a device member.  One reference to the
group is for the caller, the other reference is for the device contained
in the group.  Thanks,

Alex

> Signed-off-by: Peng Fan <van.freenix@gmail.com>
> Cc: Joerg Roedel <joro@8bytes.org>
> Cc: Will Deacon <will.deacon@arm.com>
> ---
> 
> V1 thread: https://lkml.org/lkml/2015/11/3/304
> Changes V2:
>  I did not see the update about device_group when I worked out V1. So
>  redo the patch and refine commit msg and rebased to latest linus'
>  linux master tree.
> 
>  drivers/iommu/iommu.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index abae363..9c1971b 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -852,10 +852,10 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev)
>  	}
>  
>  	ret = iommu_group_add_device(group, dev);
> -	if (ret) {
> -		iommu_group_put(group);
> +	iommu_group_put(group);
> +
> +	if (ret)
>  		return ERR_PTR(ret);
> -	}
>  
>  	return group;
>  }

Re: [RFC V2] iommu: correct group reference count

[Peng Fan]

Hi Alex,
On Mon, Nov 09, 2015 at 08:28:09AM -0700, Alex Williamson wrote:
>On Mon, 2015-11-09 at 14:13 +0800, Peng Fan wrote:
>> The basic flow for iommu_group_for_dev is:
>> iommu_group_get_for_dev
>>         |->  iommu_group_get   : increase reference count by 1.
>>              return group;
>>         |->  ops->device_group : Init/increase reference count to/by 1.
>>              iommu_group_add_device  : Increase reference count by 1.
>>              return group;
>> 
>> We can see that ops->device_group and iommu_group_add_device will together
>> increase the iommu group reference count by 2. Actually we only need 1,
>> but not 2. So we need add iommu_group_put after iommu_group_add_device
>> to make sure iommu_group_get_for_dev only increase reference count by 1.
>

Thanks for comments.
>The premise seems incorrect to me.  In the first case where
>iommu_group_get() provides a group, the reference is increased by 1, but
>the device is already a member of the group and therefore already
>increases the reference count of the group by 1.  The minimum group
>reference at that point is therefore 2.
Yeah. The minimum group reference at that point is 2.

>In the second case, the group
>is created, which gives us our first reference, and the device is added,
>giving us our second reference.  Therefore the minimum reference count
>is also 2.

Yeah.

>If we decrement it, allowing the caller to get the group
>with a single reference and they release that reference, the group will
>be destroyed even though it has a device member.

Thanks. Then Caller of iommu_group_get_for_dev should decrease the count by 1.

Thanks,
Peng.

>One reference to the
>group is for the caller, the other reference is for the device contained
>in the group.  Thanks,
>
>Alex
>
>> Signed-off-by: Peng Fan <van.freenix@gmail.com>
>> Cc: Joerg Roedel <joro@8bytes.org>
>> Cc: Will Deacon <will.deacon@arm.com>
>> ---
>> 
>> V1 thread: https://lkml.org/lkml/2015/11/3/304
>> Changes V2:
>>  I did not see the update about device_group when I worked out V1. So
>>  redo the patch and refine commit msg and rebased to latest linus'
>>  linux master tree.
>> 
>>  drivers/iommu/iommu.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index abae363..9c1971b 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -852,10 +852,10 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev)
>>  	}
>>  
>>  	ret = iommu_group_add_device(group, dev);
>> -	if (ret) {
>> -		iommu_group_put(group);
>> +	iommu_group_put(group);
>> +
>> +	if (ret)
>>  		return ERR_PTR(ret);
>> -	}
>>  
>>  	return group;
>>  }
>
>
>