On Mon, May 22, 2023 at 10:35:49AM +0200, Niklas Schnelle wrote: > On Fri, 2023-05-19 at 15:42 -0300, Jason Gunthorpe wrote: > > Instead of returning the struct group_device and then later freeing it, do > > the entire free under the group->mutex and defer only putting the > > iommu_group. > > > > It is safe to remove the sysfs_links and free memory while holding that > > mutex. > > > > Move the sanity assert of the group status into > > __iommu_group_free_device(). > > > > The next patch will improve upon this and consolidate the group put and > > the mutex into __iommu_group_remove_device(). > > > > __iommu_group_free_device() is close to being the paired undo of > > iommu_group_add_device(), following patches will improve on that. > > > > Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx> > > Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxx> > > --- > > drivers/iommu/iommu.c | 83 ++++++++++++++++++++----------------------- > > 1 file changed, 39 insertions(+), 44 deletions(-) > > > ---8<--- > > + > > +/* > > + * Remove the iommu_group from the struct device. The attached group must be put > > + * by the caller after releaseing the group->mutex. > > + */ > > +static void __iommu_group_remove_device(struct device *dev) > > +{ > > + struct iommu_group *group = dev->iommu_group; > > + struct group_device *device; > > + > > + lockdep_assert_held(&group->mutex); > > + for_each_group_device(group, device) { > > + if (device->dev != dev) > > + continue; > > + > > + list_del(&device->list); > > for_each_group_device() uses list_for_each_entry() but here you are > deleting from the list, don't we need a ..._safe() variant then? As a general statement, yes > > + __iommu_group_free_device(group, device); > > + /* Caller must put iommu_group */ > > + return; But the loop immediately returns before going to the next iteration so this is safe. Jason