Re: [PATCH v2 04/10] iommu: Simplify the __iommu_group_remove_device() flow

[Jason Gunthorpe <jgg@nvidia.com>]

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@intel.com>
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> > ---
> >  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