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

[Niklas Schnelle <schnelle@linux.ibm.com>]

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?

> +		__iommu_group_free_device(group, device);
> +		/* Caller must put iommu_group */
> +		return;
> +	}
> +	WARN(true, "Corrupted iommu_group device_list");
>  }
> 
---8<---