On Fri, Feb 03, 2023 at 11:03:20AM -0400, Jason Gunthorpe wrote:
- */
+int iommu_group_replace_domain(struct iommu_group *group,
struct iommu_domain *new_domain)what actual value does 'replace' give us? It's just a wrapper of __iommu_group_set_domain() then calling it set_domain is probably clearer. You can clarify the 'replace' behavior in the comment.
As the APIs are setup:
attach demands that no domain is currently set (eg the domain must be blocking)
replace permits the domain to be currently set
'set' vs 'attach' is really unclear what the intended difference is.
We could also address this by simply removing the protection from attach, but it is not so clear if that is safe for the few users.
I can add a couple of lines to the commit message to make things clear.
+{
- int ret;
- if (!new_domain)
return -EINVAL;- mutex_lock(&group->mutex);
- ret = __iommu_group_set_domain(group, new_domain);
- if (ret) {
if (__iommu_group_set_domain(group, group->domain))__iommu_group_set_core_domain(group);- }
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.
In this case the API cannot retain a hidden reference to the new domain, so it must be purged, one way or another.
Considering it is a bit different than the existing APIs like iommu_attach_group(), perhaps I should add a line of comments in the fallback routine.
Thanks Nic