I revisited this patch. And I still have some questions.
On 5/20/23 2:42 AM, Jason Gunthorpe wrote:
-/*
- * Remove the iommu_group from the struct device. The attached group must be put
- * by the caller after releaseing the group->mutex.
- */
+/* Remove the iommu_group from the struct device. */
static void __iommu_group_remove_device(struct device *dev)
{
struct iommu_group *group = dev->iommu_group;
struct group_device *device;
+ mutex_lock(&group->mutex);
lockdep_assert_held(&group->mutex);
By moving mutex_lock/unlock into this helper, above
lockdep_assert_held() is unnecessary.
for_each_group_device(group, device) {
if (device->dev != dev)
@@ -510,44 +564,30 @@ static void __iommu_group_remove_device(struct device *dev)
list_del(&device->list);
__iommu_group_free_device(group, device);
- /* Caller must put iommu_group */
- return;
+ if (dev->iommu && dev->iommu->iommu_dev)
+ iommu_deinit_device(dev);
+ else
+ dev->iommu_group = NULL;
+ goto out;
}
WARN(true, "Corrupted iommu_group device_list");
+out:
+ mutex_unlock(&group->mutex);
+
+ /* Pairs with the get in iommu_group_add_device() */
+ iommu_group_put(group);
The group->devices_kobj was increased on the probe device path twice:
- iommu_init_device() - allocate the group
- iommu_group_add_device() - add device to the group
But, on the release path, it seems that group->devices_kobj is only
decreased once.
Did I overlook anything? Otherwise, the group will never be released,
right?
}
static void iommu_release_device(struct device *dev)
{
struct iommu_group *group = dev->iommu_group;
- const struct iommu_ops *ops;
if (!dev->iommu || !group)
return;
iommu_device_unlink(dev->iommu->iommu_dev, dev);
- mutex_lock(&group->mutex);
__iommu_group_remove_device(dev);
-
- /*
- * release_device() must stop using any attached domain on the device.
- * If there are still other devices in the group they are not effected
- * by this callback.
- *
- * The IOMMU driver must set the device to either an identity or
- * blocking translation and stop using any domain pointer, as it is
- * going to be freed.
- */
- ops = dev_iommu_ops(dev);
- if (ops->release_device)
- ops->release_device(dev);
- mutex_unlock(&group->mutex);
-
- /* Pairs with the get in iommu_group_add_device() */
- iommu_group_put(group);
-
- module_put(ops->owner);
- dev_iommu_free(dev);
}
Best regards,
baolu
