On 2022-05-05 17:15, Jason Gunthorpe via iommu wrote:
Call iommu_group_do_attach_device() only from
__iommu_group_attach_domain() which should be used to attach any domain to
the group.
The only unique thing __iommu_attach_group() does is to check if the group
is already attached to some caller specified group. Put this test into
__iommu_group_is_core_domain(), matching the
__iommu_group_attach_core_domain() nomenclature.
Change the two callers to directly call __iommu_group_attach_domain() and
test __iommu_group_is_core_domain().
iommu_attach_device() should trigger a WARN_ON if the group is attached as
the caller is using the function wrong.
Nit: if that's true then it's equally true for iommu_attach_group() as well.
Suggested-by: "Tian, Kevin" <[email protected]>
Signed-off-by: Jason Gunthorpe <[email protected]>
---
drivers/iommu/iommu.c | 42 +++++++++++++++++++-----------------------
1 file changed, 19 insertions(+), 23 deletions(-)
This goes after "iommu: iommu_group_claim_dma_owner() must always assign a
domain" and simplifies some of the remaining duplication.
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index c1bdec807ea381..09d00be887f924 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -81,9 +81,10 @@ static struct iommu_domain *__iommu_domain_alloc(struct
bus_type *bus,
unsigned type);
static int __iommu_attach_device(struct iommu_domain *domain,
struct device *dev);
-static int __iommu_attach_group(struct iommu_domain *domain,
- struct iommu_group *group);
+static int __iommu_group_attach_domain(struct iommu_group *group,
+ struct iommu_domain *new_domain);
static void __iommu_group_attach_core_domain(struct iommu_group *group);
+static bool __iommu_group_is_core_domain(struct iommu_group *group);
static int iommu_create_device_direct_mappings(struct iommu_group *group,
struct device *dev);
static struct iommu_group *iommu_group_get_for_dev(struct device *dev);
@@ -1938,10 +1939,11 @@ int iommu_attach_device(struct iommu_domain *domain,
struct device *dev)
*/
mutex_lock(&group->mutex);
ret = -EINVAL;
- if (iommu_group_device_count(group) != 1)
+ if (iommu_group_device_count(group) != 1 ||
+ WARN_ON(!__iommu_group_is_core_domain(group)))
goto out_unlock;
- ret = __iommu_attach_group(domain, group);
+ ret = __iommu_group_attach_domain(group, domain);
out_unlock:
mutex_unlock(&group->mutex);
@@ -2032,31 +2034,19 @@ static int iommu_group_do_attach_device(struct device
*dev, void *data)
return __iommu_attach_device(domain, dev);
}
-static int __iommu_attach_group(struct iommu_domain *domain,
- struct iommu_group *group)
-{
- int ret;
-
- if (group->domain && group->domain != group->default_domain &&
- group->domain != group->blocking_domain)
- return -EBUSY;
-
- ret = __iommu_group_for_each_dev(group, domain,
- iommu_group_do_attach_device);
- if (ret == 0)
- group->domain = domain;
-
- return ret;
-}
-
int iommu_attach_group(struct iommu_domain *domain, struct iommu_group *group)
{
int ret;
mutex_lock(&group->mutex);
- ret = __iommu_attach_group(domain, group);
- mutex_unlock(&group->mutex);
+ if (!__iommu_group_is_core_domain(group)) {
+ ret = -EBUSY;
+ goto out_unlock;
+ }
+ ret = __iommu_group_attach_domain(group, domain);
+out_unlock:
+ mutex_unlock(&group->mutex);
return ret;
}
EXPORT_SYMBOL_GPL(iommu_attach_group);
@@ -2110,6 +2100,12 @@ static int __iommu_group_attach_domain(struct
iommu_group *group,
return 0;
}
+static bool __iommu_group_is_core_domain(struct iommu_group *group)
I can see the thought process behind it, but once we've had some time
away from actively working on this area, this is clearly going to be a
terrible name. "Is this group a core domain? Er, no, it's a group; what
an odd question to ask :/" Even getting past that, does it make sense to
say NULL is a core domain? I'm not convinced. For the sake of future
readability, I'd prefer to call this something more purpose-related like
__iommu_group_can_attach() (and also just define it earlier to avoid the
forward-declaration).
In fact at that point I'd also be tempted to rename
__iommu_group_attach_domain() to __iommu_group_set_domain(), to further
clarify that attach/detach still reflects the external API, but the
internal mechanism is really a bit different since default/core domains
came in.
Thanks,
Robin.
+{
+ return !group->domain || group->domain == group->default_domain ||
+ group->domain == group->blocking_domain;
+}
+
/*
* Put the group's domain back to the appropriate core-owned domain - either
the
* standard kernel-mode DMA configuration or an all-DMA-blocked domain.
base-commit: f9169ee95787fe691287eeed1a1c269ea72c8fb4
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu
