On Thu, Sep 08, 2022 at 10:27:06PM +0100, Robin Murphy wrote: > Oh, because s390 is using iommu_get_domain_for_dev() in its release_device > callback, which needs to dereference the group to work, and the current > domain may also be a non-default one which we can't prevent from > disappearing racily, that was why :( Hum, the issue there is the use of device->iommu_group - but that just means I didn't split properly. How about this incremental: diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index c451bf715182ac..99ef799f3fe6b5 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -351,6 +351,7 @@ void iommu_release_device(struct device *dev) * them until the have been detached. release_device() is expected to * detach all domains connected to the dev. */ + dev->iommu_group = NULL; kobject_put(group->devices_kobj); module_put(ops->owner); @@ -980,7 +981,6 @@ static void __iommu_group_remove_device(struct device *dev) kfree(device->name); kfree(device); - dev->iommu_group = NULL; } /** @@ -995,6 +995,7 @@ void iommu_group_remove_device(struct device *dev) struct iommu_group *group = dev->iommu_group; __iommu_group_remove_device(dev); + dev->iommu_group = NULL; kobject_put(group->devices_kobj); } EXPORT_SYMBOL_GPL(iommu_group_remove_device); To me it makes sense that the driver should be able to continue to query the iommu_group during release anyhow.. And to your other question, the reason I split the function is because I couldn't really say WTF iommu_group_remove_device() was supposed to do. The __ version make ssense as part of the remove_device, due to the sequencing with ops->release() But the other one doesn't have that. So I want to put in a: WARN_ON(group->blocking_domain || group->default_domain); Because calling it after those domains are allocated looks broken to me. Jason
