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
