iommu: arm-smmu: correct reference count

RFC In review Peng Fan 3 participants 5 messages Nov 3, 2015 – Nov 9, 2015 IOMMU

Share & tools

Peng Fan

author

2015-11-03 12:58 UTC #0 lore raw

RFC 1/1 iommu: arm-smmu: correct reference count

iommu_group_alloc will initialize the reference count for group to 1. iommu_group_add_device also increase the group reference count, if nothing bad happends. And we need to add iommu_group_put to decrease the reference count for group.

Signed-off-by Peng Fan <[email protected]>

Cc Will Deacon <[email protected]>

Cc Joerg Roedel <[email protected]>

---

Not sure whether my understanding is correct or not. I checked rockchip-iommu.c exynos-iommu.c and fsl_pamu_domain.c, and they all have iommu_group_put after iommu_group_add_device.

 drivers/iommu/arm-smmu.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

drivers/iommu/arm-smmu.c +6 -1

index 48a39df..1cab720 100644
@@ -1344,6 +1344,7 @@ static int arm_smmu_add_platform_device(struct device *dev)
 	struct iommu_group *group;
 	struct arm_smmu_master *master;
 	struct arm_smmu_device *smmu = find_smmu_for_device(dev);
+	int ret;
 
 	if (!smmu)
 		return -ENODEV;

@@ -1358,7 +1359,11 @@ static int arm_smmu_add_platform_device(struct device *dev)
 		return PTR_ERR(group);
 
 	iommu_group_set_iommudata(group, &master->cfg, NULL);
-	return iommu_group_add_device(group, dev);
+
+	ret = iommu_group_add_device(group, dev);
+	iommu_group_put(group);
+
+	return ret;
 }
 
 static int arm_smmu_add_device(struct device *dev)

Signature · 1 line

1.8.4

Will Deacon

2015-11-03 13:17 UTC #1 lore raw

Quoted 5 lines · Peng Fan

iommu_group_alloc will initialize the reference count for group to 1. iommu_group_add_device also increase the group reference count, if nothing bad happends. And we need to add iommu_group_put to decrease the reference count for group.

Quoted 8 lines

Signed-off-by Peng Fan <[email protected]>
Cc Will Deacon <[email protected]>
Cc Joerg Roedel <[email protected]>
---
Not sure whether my understanding is correct or not. I checked rockchip-iommu.c exynos-iommu.c and fsl_pamu_domain.c, and they all have iommu_group_put after iommu_group_add_device.

Doesn't this pair up with the iommu_group_remove_device in arm_smmu_remove_device? Are you actually seeing an issue in practice?

Will

Peng Fan

2015-11-04 02:48 UTC #2 lore raw

Hi Will,

Quoted 11 lines · Will Deacon

Quoted 10 lines · Peng Fan
iommu_group_alloc will initialize the reference count for group to 1. iommu_group_add_device also increase the group reference count, if nothing bad happends. And we need to add iommu_group_put to decrease the reference count for group.
Signed-off-by Peng Fan <[email protected]>
Cc Will Deacon <[email protected]>
Cc Joerg Roedel <[email protected]>
---

Quoted 6 lines

Quoted 3 lines
Not sure whether my understanding is correct or not. I checked rockchip-iommu.c exynos-iommu.c and fsl_pamu_domain.c, and they all have iommu_group_put after iommu_group_add_device.
Doesn't this pair up with the iommu_group_remove_device in arm_smmu_remove_device? Are you actually seeing an issue in practice?

In arm_smmu_add_platform_device,
iommu_group_alloc --> group->device_kobj ref count will be init to 1. iommu_group_add_device --> group->device_kobj ref count will be added 1 to 2.

In arm_smmu_remove_device:
iommu_group_remove_device --> Decrease group->device_kobj ref count by 1. After arm_smmu_remove_device, the ref count of group->device_kobj is not 0.

So I think need to add iommu_group_put after iommu_group_add_device. If I am wrong, please correct me.

Just code inspection, not have a platform to test this.

Regards,
Peng.

Quoted 6 lines

Will
_______________________________________________
iommu mailing list
iommu at lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Will Deacon

2015-11-06 12:23 UTC #3 lore raw

Quoted 23 lines · Peng Fan

Quoted 11 lines · Will Deacon
Quoted 10 lines · Peng Fan
iommu_group_alloc will initialize the reference count for group to 1. iommu_group_add_device also increase the group reference count, if nothing bad happends. And we need to add iommu_group_put to decrease the reference count for group.
Signed-off-by Peng Fan <[email protected]>
Cc Will Deacon <[email protected]>
Cc Joerg Roedel <[email protected]>
---
Quoted 6 lines
Quoted 3 lines
Not sure whether my understanding is correct or not. I checked rockchip-iommu.c exynos-iommu.c and fsl_pamu_domain.c, and they all have iommu_group_put after iommu_group_add_device.
Doesn't this pair up with the iommu_group_remove_device in arm_smmu_remove_device? Are you actually seeing an issue in practice?
In arm_smmu_add_platform_device,
iommu_group_alloc --> group->device_kobj ref count will be init to 1. iommu_group_add_device --> group->device_kobj ref count will be added 1 to 2.

Quoted 6 lines

In arm_smmu_remove_device:
iommu_group_remove_device --> Decrease group->device_kobj ref count by 1. After arm_smmu_remove_device, the ref count of group->device_kobj is not 0.
So I think need to add iommu_group_put after iommu_group_add_device. If I am wrong, please correct me.

Ok, I see what you're saying. iommu_group_get_for_dev has exactly the same issue, so could you update arm-smmu-v3.c as well, please?

Thanks,

Will

Peng Fan

author

2015-11-09 05:41 UTC #4 lore raw

Hi Will,

Quoted 28 lines · Will Deacon

Quoted 27 lines · Peng Fan
Quoted 11 lines · Will Deacon
Quoted 10 lines · Peng Fan
iommu_group_alloc will initialize the reference count for group to 1. iommu_group_add_device also increase the group reference count, if nothing bad happends. And we need to add iommu_group_put to decrease the reference count for group.
Signed-off-by Peng Fan <[email protected]>
Cc Will Deacon <[email protected]>
Cc Joerg Roedel <[email protected]>
---
Quoted 6 lines
Quoted 3 lines
Not sure whether my understanding is correct or not. I checked rockchip-iommu.c exynos-iommu.c and fsl_pamu_domain.c, and they all have iommu_group_put after iommu_group_add_device.
Doesn't this pair up with the iommu_group_remove_device in arm_smmu_remove_device? Are you actually seeing an issue in practice?
In arm_smmu_add_platform_device,
iommu_group_alloc --> group->device_kobj ref count will be init to 1. iommu_group_add_device --> group->device_kobj ref count will be added 1 to 2.
In arm_smmu_remove_device:
iommu_group_remove_device --> Decrease group->device_kobj ref count by 1. After arm_smmu_remove_device, the ref count of group->device_kobj is not 0.

Quoted 5 lines

Quoted 2 lines
So I think need to add iommu_group_put after iommu_group_add_device. If I am wrong, please correct me.
Ok, I see what you're saying. iommu_group_get_for_dev has exactly the same issue, so could you update arm-smmu-v3.c as well, please?

I did not see the update about device_group for iommu, when I worked out this patch. Rebased to latest linux/master, whether to arm-smmu.c or arm-smmu-v3.c, only add 'iommu_group_put' at the end of iommu_group_get_for_dev is ok as the following.

I'll send out V2.

Thanks,
Peng.

Quoted 4 lines

Thanks,
Will