From: Lu Baolu <baolu.lu@linux.intel.com>
To: iommu@lists.linux.dev
Cc: Joerg Roedel <joro@8bytes.org>, Jason Gunthorpe <jgg@nvidia.com>,
Christoph Hellwig <hch@infradead.org>,
Kevin Tian <kevin.tian@intel.com>, Will Deacon <will@kernel.org>,
Robin Murphy <robin.murphy@arm.com>,
linux-kernel@vger.kernel.org, Lu Baolu <baolu.lu@linux.intel.com>
Subject: [PATCH v3 5/6] iommu: Replace device_lock() with group->mutex
Date: Mon, 6 Mar 2023 10:58:03 +0800 [thread overview]
Message-ID: <20230306025804.13912-6-baolu.lu@linux.intel.com> (raw)
In-Reply-To: <20230306025804.13912-1-baolu.lu@linux.intel.com>
device_lock() was used in iommu_group_store_type() to prevent the
devices in an iommu group from being attached by any device driver.
On the other hand, in order to avoid lock race between group->mutex
and device_lock(), it limited the usage scenario to the singleton
groups.
We already have the DMA ownership scheme to avoid driver attachment
and group->mutex ensures that device ops are always valid, there's
no need for device_lock() anymore. Remove device_lock() and the
singleton group limitation.
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
drivers/iommu/iommu.c | 78 +++++++++----------------------------------
1 file changed, 16 insertions(+), 62 deletions(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index f8f400548a10..9fe6d149f281 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3011,14 +3011,6 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
goto out;
}
- /* We can bring up a flush queue without tearing down the domain */
- if (type == IOMMU_DOMAIN_DMA_FQ && prev_dom->type == IOMMU_DOMAIN_DMA) {
- ret = iommu_dma_init_fq(prev_dom);
- if (!ret)
- prev_dom->type = IOMMU_DOMAIN_DMA_FQ;
- goto out;
- }
-
/* Sets group->default_domain to the newly allocated domain */
ret = iommu_group_alloc_default_domain(group, dev, type);
if (ret)
@@ -3051,7 +3043,7 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
* transition. Return failure if this isn't met.
*
* We need to consider the race between this and the device release path.
- * device_lock(dev) is used here to guarantee that the device release path
+ * group->mutex is used here to guarantee that the device release path
* will not be entered at the same time.
*/
static ssize_t iommu_group_store_type(struct iommu_group *group,
@@ -3077,60 +3069,27 @@ static ssize_t iommu_group_store_type(struct iommu_group *group,
else
return -EINVAL;
- /*
- * Lock/Unlock the group mutex here before device lock to
- * 1. Make sure that the iommu group has only one device (this is a
- * prerequisite for step 2)
- * 2. Get struct *dev which is needed to lock device
- */
mutex_lock(&group->mutex);
- if (iommu_group_device_count(group) != 1) {
+ /* We can bring up a flush queue without tearing down the domain. */
+ if (req_type == IOMMU_DOMAIN_DMA_FQ &&
+ group->default_domain->type == IOMMU_DOMAIN_DMA) {
+ ret = iommu_dma_init_fq(group->default_domain);
+ if (!ret)
+ group->default_domain->type = IOMMU_DOMAIN_DMA_FQ;
mutex_unlock(&group->mutex);
- pr_err_ratelimited("Cannot change default domain: Group has more than one device\n");
- return -EINVAL;
+
+ return ret ?: count;
}
- /* Since group has only one device */
+ /* Otherwise, ensure that device exists and no driver is bound. */
dev = iommu_group_first_dev(group);
- get_device(dev);
-
- /*
- * Don't hold the group mutex because taking group mutex first and then
- * the device lock could potentially cause a deadlock as below. Assume
- * two threads T1 and T2. T1 is trying to change default domain of an
- * iommu group and T2 is trying to hot unplug a device or release [1] VF
- * of a PCIe device which is in the same iommu group. T1 takes group
- * mutex and before it could take device lock assume T2 has taken device
- * lock and is yet to take group mutex. Now, both the threads will be
- * waiting for the other thread to release lock. Below, lock order was
- * suggested.
- * device_lock(dev);
- * mutex_lock(&group->mutex);
- * iommu_change_dev_def_domain();
- * mutex_unlock(&group->mutex);
- * device_unlock(dev);
- *
- * [1] Typical device release path
- * device_lock() from device/driver core code
- * -> bus_notifier()
- * -> iommu_bus_notifier()
- * -> iommu_release_device()
- * -> ops->release_device() vendor driver calls back iommu core code
- * -> mutex_lock() from iommu core code
- */
- mutex_unlock(&group->mutex);
-
- /* Check if the device in the group still has a driver bound to it */
- device_lock(dev);
- if (device_is_bound(dev) && !(req_type == IOMMU_DOMAIN_DMA_FQ &&
- group->default_domain->type == IOMMU_DOMAIN_DMA)) {
- pr_err_ratelimited("Device is still bound to driver\n");
- ret = -EBUSY;
- goto out;
+ if (!dev || group->owner_cnt) {
+ mutex_unlock(&group->mutex);
+ return -EPERM;
}
- mutex_lock(&group->mutex);
ret = iommu_change_dev_def_domain(group, dev, req_type);
+
/*
* Release the mutex here because ops->probe_finalize() call-back of
* some vendor IOMMU drivers calls arm_iommu_attach_device() which
@@ -3141,14 +3100,9 @@ static ssize_t iommu_group_store_type(struct iommu_group *group,
/* Make sure dma_ops is appropriatley set */
if (!ret)
- iommu_group_do_probe_finalize(dev, group->default_domain);
- ret = ret ?: count;
-
-out:
- device_unlock(dev);
- put_device(dev);
+ __iommu_group_dma_finalize(group);
- return ret;
+ return ret ?: count;
}
static bool iommu_is_default_domain(struct iommu_group *group)
--
2.34.1
next prev parent reply other threads:[~2023-03-06 2:59 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-06 2:57 [PATCH v3 0/6] iommu: Extend changing default domain to normal group Lu Baolu
2023-03-06 2:57 ` [PATCH v3 1/6] ARM/dma-mapping: Add arm_iommu_release_device() Lu Baolu
2023-03-10 1:00 ` Jason Gunthorpe
2023-03-10 22:04 ` Robin Murphy
2023-03-12 3:53 ` Baolu Lu
2023-03-06 2:58 ` [PATCH v3 2/6] iommu: Split iommu_group_remove_device() into helpers Lu Baolu
2023-03-10 1:01 ` Jason Gunthorpe
2023-03-06 2:58 ` [PATCH v3 3/6] iommu: Same critical region for device release and removal Lu Baolu
2023-03-10 1:08 ` Jason Gunthorpe
2023-03-06 2:58 ` [PATCH v3 4/6] iommu: Move lock from iommu_change_dev_def_domain() to its caller Lu Baolu
2023-03-10 1:16 ` Jason Gunthorpe
2023-03-06 2:58 ` Lu Baolu [this message]
2023-03-10 1:30 ` [PATCH v3 5/6] iommu: Replace device_lock() with group->mutex Jason Gunthorpe
2023-03-06 2:58 ` [PATCH v3 6/6] iommu: Cleanup iommu_change_dev_def_domain() Lu Baolu
2023-03-10 1:30 ` Jason Gunthorpe
2023-03-10 1:32 ` [PATCH v3 0/6] iommu: Extend changing default domain to normal group Jason Gunthorpe
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=20230306025804.13912-6-baolu.lu@linux.intel.com \
--to=baolu.lu@linux.intel.com \
--cc=hch@infradead.org \
--cc=iommu@lists.linux.dev \
--cc=jgg@nvidia.com \
--cc=joro@8bytes.org \
--cc=kevin.tian@intel.com \
--cc=linux-kernel@vger.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).