On Mon, Feb 06, 2023 at 09:25:17AM -0400, Jason Gunthorpe wrote:
Can you elaborate the error handling here? Ideally if __iommu_group_set_domain() fails then group->domain shouldn't be changed.
That isn't what it implements though. The internal helper leaves things in a mess, it is for the caller to fix it, and it depends on the caller what that means.
I didn't see any warning of the mess and the caller's responsibility in __iommu_group_set_domain(). Can it be documented clearly so if someone wants to add a new caller on it he can clearly know what to do?
That would be nice..
I'd expect the doc to come with some other patch/series than this replace series, so I think we should be fine without adding a line of comments in this patch?
btw looking at the code __iommu_group_set_domain():
* Note that this is called in error unwind paths, attaching to a * domain that has already been attached cannot fail. */ret = __iommu_group_for_each_dev(group, new_domain, iommu_group_do_attach_device);
with that we don't need fall back to core domain in above error unwinding per this comment.
That does make some sense.
I tried to make a patch to consolidate all this error handling once, that would be the better way to approach this.
Then, I'll drop the core-domain line. Combining my reply above:
+ mutex_lock(&group->mutex); + ret = __iommu_group_set_domain(group, new_domain); + if (ret) + __iommu_group_set_domain(group, group->domain); + mutex_unlock(&group->mutex);
Will wrap things up and send v2 today.
Thanks Nic