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