Messages in this thread | | | From | "Tian, Kevin" <> | Subject | RE: [PATCH v2 02/10] iommu: Introduce a new iommu_group_replace_domain() API | Date | Fri, 24 Feb 2023 08:07:41 +0000 |
| |
> From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Friday, February 24, 2023 8:58 AM > > On Wed, Feb 22, 2023 at 02:11:39AM +0000, Tian, Kevin wrote: > > > > > There was an attempt [1] to fix error unwind in iommu_attach_group(), > by > > > > temporarily set group->domain to NULL before calling set_domain(). > > > > > > > > Jason, I wonder why this recovering cannot be done in > > > > __iommu_group_set_domain() directly, e.g.: > > > > > > > > ret = __iommu_group_for_each_dev(group, new_domain, > > > > iommu_group_do_attach_device); > > > > if (ret) { > > > > __iommu_group_for_each_dev(group, group->domain, > > > > iommu_group_do_attach_device); > > > > return ret; > > > > } > > > > group->domain = new_domain; > > > > > > We talked about this already, some times this is not the correct > > > recovery case, eg if we are going to a blocking domain we need to drop > > > all references to the prior domain, not put them back. > > > > > > Failures are WARN_ON events not error recovery. > > > > > > > OK, I remember that. Then here looks we also need temporarily > > set group->domain to NULL before calling set_domain() to recover, > > as [1] does. > > Sigh, this is too much. > > I made a series to clean up all the domain attach logic so the error > handling is all in one place and all the same. > > What do you think? > > https://github.com/jgunthorpe/linux/commits/iommufd_hwpt >
Yeah, that sounds a right cleanup at a glance.
| |