* [RFC V3] iommu: arm-smmu: correct group reference count
@ 2015-11-10 1:56 Peng Fan
2015-11-17 16:17 ` Will Deacon
0 siblings, 1 reply; 3+ messages in thread
From: Peng Fan @ 2015-11-10 1:56 UTC (permalink / raw)
To: will.deacon; +Cc: joro, iommu, linux-kernel, linux-arm-kernel, Peng Fan
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;
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;
}
--
1.8.4
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [RFC V3] iommu: arm-smmu: correct group reference count
2015-11-10 1:56 [RFC V3] iommu: arm-smmu: correct group reference count Peng Fan
@ 2015-11-17 16:17 ` Will Deacon
2015-11-20 6:24 ` Peng Fan
0 siblings, 1 reply; 3+ messages in thread
From: Will Deacon @ 2015-11-17 16:17 UTC (permalink / raw)
To: Peng Fan; +Cc: joro, iommu, linux-kernel, linux-arm-kernel, robin.murphy
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
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;
}
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [RFC V3] iommu: arm-smmu: correct group reference count
2015-11-17 16:17 ` Will Deacon
@ 2015-11-20 6:24 ` Peng Fan
0 siblings, 0 replies; 3+ messages in thread
From: Peng Fan @ 2015-11-20 6:24 UTC (permalink / raw)
To: Will Deacon; +Cc: joro, iommu, linux-kernel, linux-arm-kernel, robin.murphy
Hi Will,
On Tue, Nov 17, 2015 at 04:17:46PM +0000, Will Deacon wrote:
>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.
Thanks for your comments. I missed to handle the failure path.
>
>The best bet is probably something like the diff below.
Yeah.
I'll send a new version patch with your diff.
Thanks,
Peng.
>
>Thoughts?
>
>Will
>
>--->8
>
>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;
> }
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-11-20 6:24 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-11-10 1:56 [RFC V3] iommu: arm-smmu: correct group reference count Peng Fan
2015-11-17 16:17 ` Will Deacon
2015-11-20 6:24 ` Peng Fan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).