diff mbox

[RFC,V3] iommu: arm-smmu: correct group reference count

Message ID 20151117161745.GD30101@arm.com
State New
Headers show

Commit Message

Will Deacon Nov. 17, 2015, 4:17 p.m. UTC 
On Tue, Nov 10, 2015 at 09:56:26AM +0800, Peng Fan wrote:
> The basic flow for add a device:

>  arm_smmu_add_device

>         |->iommu_group_get_for_dev

>             |->iommu_group_get

>                      return group;  (1)

>             |->ops->device_group : Init/increase reference count to/by 1.

>             |->iommu_group_add_device : Increase reference count by 1.

> 		     return group   (2)

>         |->return 0;

> 

> Since we are adding one device, the flow is (2) and the group reference

> count will be increased by 2. So, we need to add iommu_group_put at the

> end of arm_smmu_add_device to decrease the count by 1.

> 

> Signed-off-by: Peng Fan <van.freenix@gmail.com>

> Cc: Will Deacon <will.deacon@arm.com>

> ---

>  drivers/iommu/arm-smmu-v3.c | 7 ++++++-

>  drivers/iommu/arm-smmu.c    | 2 ++

>  2 files changed, 8 insertions(+), 1 deletion(-)

> 

> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c

> index 4e5118a..ac333ee 100644

> --- a/drivers/iommu/arm-smmu-v3.c

> +++ b/drivers/iommu/arm-smmu-v3.c

> @@ -1825,8 +1825,10 @@ static int arm_smmu_add_device(struct device *dev)

>  	pci_for_each_dma_alias(pdev, __arm_smmu_get_pci_sid, &sid);

>  	for (i = 0; i < smmu_group->num_sids; ++i) {

>  		/* If we already know about this SID, then we're done */

> -		if (smmu_group->sids[i] == sid)

> +		if (smmu_group->sids[i] == sid) {

> +			iommu_group_put(group);

>  			return 0;

> +		}

>  	}

>  

>  	/* Check the SID is in range of the SMMU and our stream table */

> @@ -1855,6 +1857,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;


I still think this is wrong for the failure path. If we fail during
add_device, then we want to put things back like they were, which is
what the out_put_group label is for. That means dropping the refcount
for the group *and* the refcount for the device. The nasty part is that
we don't know if we were responsible for adding the device to the group,
but it looks like we already assume that in ->remove_device.

The best bet is probably something like the diff below.

Thoughts?

Will

--->8

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
diff mbox

Patch

diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 86480480895d..db03c2fb1319 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -1804,13 +1804,13 @@  static int arm_smmu_add_device(struct device *dev)
 		smmu = arm_smmu_get_for_pci_dev(pdev);
 		if (!smmu) {
 			ret = -ENOENT;
-			goto out_put_group;
+			goto out_remove_dev;
 		}
 
 		smmu_group = kzalloc(sizeof(*smmu_group), GFP_KERNEL);
 		if (!smmu_group) {
 			ret = -ENOMEM;
-			goto out_put_group;
+			goto out_remove_dev;
 		}
 
 		smmu_group->ste.valid	= true;
@@ -1826,20 +1826,20 @@  static int arm_smmu_add_device(struct device *dev)
 	for (i = 0; i < smmu_group->num_sids; ++i) {
 		/* If we already know about this SID, then we're done */
 		if (smmu_group->sids[i] == sid)
-			return 0;
+			goto out_put_group;
 	}
 
 	/* Check the SID is in range of the SMMU and our stream table */
 	if (!arm_smmu_sid_in_range(smmu, sid)) {
 		ret = -ERANGE;
-		goto out_put_group;
+		goto out_remove_dev;
 	}
 
 	/* Ensure l2 strtab is initialised */
 	if (smmu->features & ARM_SMMU_FEAT_2_LVL_STRTAB) {
 		ret = arm_smmu_init_l2_strtab(smmu, sid);
 		if (ret)
-			goto out_put_group;
+			goto out_remove_dev;
 	}
 
 	/* Resize the SID array for the group */
@@ -1849,16 +1849,20 @@  static int arm_smmu_add_device(struct device *dev)
 	if (!sids) {
 		smmu_group->num_sids--;
 		ret = -ENOMEM;
-		goto out_put_group;
+		goto out_remove_dev;
 	}
 
 	/* Add the new SID */
 	sids[smmu_group->num_sids - 1] = sid;
 	smmu_group->sids = sids;
-	return 0;
 
 out_put_group:
 	iommu_group_put(group);
+	return 0;
+
+out_remove_dev:
+	iommu_group_remove_device(dev);
+	iommu_group_put(group);
 	return ret;
 }