From: Jason Gunthorpe <jgg@nvidia.com>
To: David Woodhouse <dwmw2@infradead.org>,
iommu@lists.linux.dev, Joerg Roedel <joro@8bytes.org>,
Len Brown <lenb@kernel.org>,
linux-acpi@vger.kernel.org,
"Rafael J. Wysocki" <rafael@kernel.org>,
Robin Murphy <robin.murphy@arm.com>,
Will Deacon <will@kernel.org>
Cc: Lu Baolu <baolu.lu@linux.intel.com>,
Kevin Tian <kevin.tian@intel.com>,
Nicolin Chen <nicolinc@nvidia.com>,
"Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
Subject: [PATCH v3 05/10] iommu: Add iommu_init/deinit_device() paired functions
Date: Mon, 5 Jun 2023 21:59:43 -0300 [thread overview]
Message-ID: <5-v3-328044aa278c+45e49-iommu_probe_jgg@nvidia.com> (raw)
In-Reply-To: <0-v3-328044aa278c+45e49-iommu_probe_jgg@nvidia.com>
Move the driver init and destruction code into two logically paired
functions.
There is a subtle ordering dependency in how the group's domains are
freed, the current code does the kobject_put() on the group which will
hopefully trigger the free of the domains before the module_put() that
protects the domain->ops.
Reorganize this to be explicit and documented. The domains are cleaned up
by iommu_deinit_device() if it is the last device to be deinit'd from the
group. This must be done in a specific order - after
ops->release_device() and before the module_put(). Make it very clear and
obvious by putting the order directly in one function.
Leave WARN_ON's in case the refcounting gets messed up somehow.
This also moves the module_put() and dev_iommu_free() under the
group->mutex to keep the code simple.
Building paired functions like this helps ensure that error cleanup flows
in __iommu_probe_device() are correct because they share the same code
that handles the normal flow. These details become relavent as following
patches add more error unwind into __iommu_probe_device(), and ultimately
a following series adds fine-grained locking to __iommu_probe_device().
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
drivers/iommu/iommu.c | 191 +++++++++++++++++++++++++-----------------
1 file changed, 112 insertions(+), 79 deletions(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 456c2d2934896c..7e8f5edcff2145 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -332,10 +332,99 @@ static u32 dev_iommu_get_max_pasids(struct device *dev)
return min_t(u32, max_pasids, dev->iommu->iommu_dev->max_pasids);
}
+/*
+ * Init the dev->iommu and dev->iommu_group in the struct device and get the
+ * driver probed
+ */
+static int iommu_init_device(struct device *dev, const struct iommu_ops *ops)
+{
+ struct iommu_device *iommu_dev;
+ struct iommu_group *group;
+ int ret;
+
+ if (!dev_iommu_get(dev))
+ return -ENOMEM;
+
+ if (!try_module_get(ops->owner)) {
+ ret = -EINVAL;
+ goto err_free;
+ }
+
+ iommu_dev = ops->probe_device(dev);
+ if (IS_ERR(iommu_dev)) {
+ ret = PTR_ERR(iommu_dev);
+ goto err_module_put;
+ }
+
+ group = ops->device_group(dev);
+ if (WARN_ON_ONCE(group == NULL))
+ group = ERR_PTR(-EINVAL);
+ if (IS_ERR(group)) {
+ ret = PTR_ERR(group);
+ goto err_release;
+ }
+ dev->iommu_group = group;
+
+ dev->iommu->iommu_dev = iommu_dev;
+ dev->iommu->max_pasids = dev_iommu_get_max_pasids(dev);
+ if (ops->is_attach_deferred)
+ dev->iommu->attach_deferred = ops->is_attach_deferred(dev);
+ return 0;
+
+err_release:
+ if (ops->release_device)
+ ops->release_device(dev);
+err_module_put:
+ module_put(ops->owner);
+err_free:
+ dev_iommu_free(dev);
+ return ret;
+}
+
+static void iommu_deinit_device(struct device *dev)
+{
+ struct iommu_group *group = dev->iommu_group;
+ const struct iommu_ops *ops = dev_iommu_ops(dev);
+
+ lockdep_assert_held(&group->mutex);
+
+ /*
+ * 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.
+ */
+ if (ops->release_device)
+ ops->release_device(dev);
+
+ /*
+ * If this is the last driver to use the group then we must free the
+ * domains before we do the module_put().
+ */
+ if (list_empty(&group->devices)) {
+ if (group->default_domain) {
+ iommu_domain_free(group->default_domain);
+ group->default_domain = NULL;
+ }
+ if (group->blocking_domain) {
+ iommu_domain_free(group->blocking_domain);
+ group->blocking_domain = NULL;
+ }
+ group->domain = NULL;
+ }
+
+ /* Caller must put iommu_group */
+ dev->iommu_group = NULL;
+ module_put(ops->owner);
+ dev_iommu_free(dev);
+}
+
static int __iommu_probe_device(struct device *dev, struct list_head *group_list)
{
const struct iommu_ops *ops = dev->bus->iommu_ops;
- struct iommu_device *iommu_dev;
struct iommu_group *group;
static DEFINE_MUTEX(iommu_probe_device_lock);
int ret;
@@ -357,62 +446,30 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
goto out_unlock;
}
- if (!dev_iommu_get(dev)) {
- ret = -ENOMEM;
+ ret = iommu_init_device(dev, ops);
+ if (ret)
goto out_unlock;
- }
-
- if (!try_module_get(ops->owner)) {
- ret = -EINVAL;
- goto err_free;
- }
-
- iommu_dev = ops->probe_device(dev);
- if (IS_ERR(iommu_dev)) {
- ret = PTR_ERR(iommu_dev);
- goto out_module_put;
- }
-
- dev->iommu->iommu_dev = iommu_dev;
- dev->iommu->max_pasids = dev_iommu_get_max_pasids(dev);
- if (ops->is_attach_deferred)
- dev->iommu->attach_deferred = ops->is_attach_deferred(dev);
-
- group = ops->device_group(dev);
- if (WARN_ON_ONCE(group == NULL))
- group = ERR_PTR(-EINVAL);
- if (IS_ERR(group)) {
- ret = PTR_ERR(group);
- goto out_release;
- }
+ group = dev->iommu_group;
ret = iommu_group_add_device(group, dev);
+ mutex_lock(&group->mutex);
if (ret)
goto err_put_group;
- mutex_lock(&group->mutex);
if (group_list && !group->default_domain && list_empty(&group->entry))
list_add_tail(&group->entry, group_list);
mutex_unlock(&group->mutex);
iommu_group_put(group);
mutex_unlock(&iommu_probe_device_lock);
- iommu_device_link(iommu_dev, dev);
+ iommu_device_link(dev->iommu->iommu_dev, dev);
return 0;
err_put_group:
+ iommu_deinit_device(dev);
+ mutex_unlock(&group->mutex);
iommu_group_put(group);
-out_release:
- if (ops->release_device)
- ops->release_device(dev);
-
-out_module_put:
- module_put(ops->owner);
-
-err_free:
- dev_iommu_free(dev);
-
out_unlock:
mutex_unlock(&iommu_probe_device_lock);
@@ -491,63 +548,45 @@ static void __iommu_group_free_device(struct iommu_group *group,
kfree(grp_dev->name);
kfree(grp_dev);
- dev->iommu_group = NULL;
}
-/*
- * 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;
- lockdep_assert_held(&group->mutex);
+ mutex_lock(&group->mutex);
for_each_group_device(group, device) {
if (device->dev != dev)
continue;
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);
}
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);
}
static int __init iommu_set_def_domain_type(char *str)
@@ -808,10 +847,9 @@ static void iommu_group_release(struct kobject *kobj)
ida_free(&iommu_group_ida, group->id);
- if (group->default_domain)
- iommu_domain_free(group->default_domain);
- if (group->blocking_domain)
- iommu_domain_free(group->blocking_domain);
+ /* Domains are free'd by iommu_deinit_device() */
+ WARN_ON(group->default_domain);
+ WARN_ON(group->blocking_domain);
kfree(group->name);
kfree(group);
@@ -1109,12 +1147,7 @@ void iommu_group_remove_device(struct device *dev)
dev_info(dev, "Removing from iommu group %d\n", group->id);
- mutex_lock(&group->mutex);
__iommu_group_remove_device(dev);
- mutex_unlock(&group->mutex);
-
- /* Pairs with the get in iommu_group_add_device() */
- iommu_group_put(group);
}
EXPORT_SYMBOL_GPL(iommu_group_remove_device);
--
2.40.1
next prev parent reply other threads:[~2023-06-06 1:00 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-06 0:59 [PATCH v3 00/10] Consolidate the probe_device path Jason Gunthorpe
2023-06-06 0:59 ` [PATCH v3 01/10] iommu: Have __iommu_probe_device() check for already probed devices Jason Gunthorpe
2023-06-06 0:59 ` [PATCH v3 02/10] iommu: Use iommu_group_ref_get/put() for dev->iommu_group Jason Gunthorpe
2023-06-06 0:59 ` [PATCH v3 03/10] iommu: Inline iommu_group_get_for_dev() into __iommu_probe_device() Jason Gunthorpe
2023-06-06 0:59 ` [PATCH v3 04/10] iommu: Simplify the __iommu_group_remove_device() flow Jason Gunthorpe
2023-06-06 0:59 ` Jason Gunthorpe [this message]
2023-06-06 0:59 ` [PATCH v3 06/10] iommu: Move the iommu driver sysfs setup into iommu_init/deinit_device() Jason Gunthorpe
2023-06-06 0:59 ` [PATCH v3 07/10] iommu: Do not export iommu_device_link/unlink() Jason Gunthorpe
2023-06-06 0:59 ` [PATCH v3 08/10] iommu: Always destroy the iommu_group during iommu_release_device() Jason Gunthorpe
2023-06-06 0:59 ` [PATCH v3 09/10] iommu: Split iommu_group_add_device() Jason Gunthorpe
2023-06-06 0:59 ` [PATCH v3 10/10] iommu: Avoid locking/unlocking for iommu_probe_device() Jason Gunthorpe
2023-06-19 19:03 ` [PATCH v3 00/10] Consolidate the probe_device path Jason Gunthorpe
2023-07-14 14:14 ` Joerg Roedel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5-v3-328044aa278c+45e49-iommu_probe_jgg@nvidia.com \
--to=jgg@nvidia.com \
--cc=baolu.lu@linux.intel.com \
--cc=dwmw2@infradead.org \
--cc=iommu@lists.linux.dev \
--cc=joro@8bytes.org \
--cc=kevin.tian@intel.com \
--cc=lenb@kernel.org \
--cc=linux-acpi@vger.kernel.org \
--cc=nicolinc@nvidia.com \
--cc=rafael.j.wysocki@intel.com \
--cc=rafael@kernel.org \
--cc=robin.murphy@arm.com \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).