Re: [PATCH v2 02/10] iommu: Add a lockdep assertion for remaining dev->iommu_group reads
From: Robin Murphy <robin.murphy@arm.com>
Date: 2023-08-08 16:23:35
Also in:
linux-iommu, linux-rockchip, linux-sunxi
Oh, the things that happen if I take holiday... :) On 31/07/2023 6:50 pm, Jason Gunthorpe wrote:
The remaining reads are all in functions called under ops->device_group. Broadly these functions are walking around the device tree (eg going up the PCI bus tree) and are trying to de-duplicate group allocations according to their logic. Since these functions don't hold any particular per-device locks their reads to dev->iommu_group are being locked by the caller's iommu_probe_device_lock, and this explains why iommu_probe_device_lock needs to be a global lock.
This confuzzles me. iommu_probe_device_lock is a global (but
tightly-scoped) lock because its sole purpose is as a point hack to
serialise calls to iommu_probe_device(), which were never expected to be
able to happen concurrently for the same device, but due to the
long-standing "replay" hacks, currently can. It is not meant to have
anything to do with groups, and expanding its scope is a really really
terrible idea.
I finally now have some time to work on IOMMU gubbins again, so I'll be
updating the bus ops removal series ASAP, then the next step after that
is some bus_type callback surgery to pull the
{of,acpi}_iommu_configure() parsing and ops->of_xlate calls to the
proper point in the core iommu_probe_device() path, and all this mess
finally goes away for good.
Thanks,
Robin.
quoted hunk ↗ jump to hunk
Rename iommu_probe_device_lock to dev_iommu_group_lock, make it local to the module and annotate all the device_group helpers with iommu_group_get_locked() that includes a lockdep to indicate that they are special. Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> --- drivers/iommu/iommu.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-)diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index 409090eaac543a..f1c8a333553e3b 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c@@ -44,6 +44,8 @@ static unsigned int iommu_def_domain_type __read_mostly; static bool iommu_dma_strict __read_mostly = IS_ENABLED(CONFIG_IOMMU_DEFAULT_DMA_STRICT); static u32 iommu_cmd_line __read_mostly; +static DEFINE_MUTEX(dev_iommu_group_lock); + struct iommu_group { struct kobject kobj; struct kobject *devices_kobj;@@ -438,7 +440,6 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list { const struct iommu_ops *ops = dev->bus->iommu_ops; struct iommu_group *group; - static DEFINE_MUTEX(iommu_probe_device_lock); struct group_device *gdev; int ret;@@ -451,7 +452,7 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list * probably be able to use device_lock() here to minimise the scope, * but for now enforcing a simple global ordering is fine. */ - mutex_lock(&iommu_probe_device_lock); + mutex_lock(&dev_iommu_group_lock); /* Device is probed already if in a group */ if (dev->iommu_group) {@@ -497,7 +498,7 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list list_add_tail(&group->entry, group_list); } mutex_unlock(&group->mutex); - mutex_unlock(&iommu_probe_device_lock); + mutex_unlock(&dev_iommu_group_lock); if (dev_is_pci(dev)) iommu_dma_set_pci_32bit_workaround(dev);@@ -512,7 +513,7 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list mutex_unlock(&group->mutex); iommu_group_put(group); out_unlock: - mutex_unlock(&iommu_probe_device_lock); + mutex_unlock(&dev_iommu_group_lock); return ret; }@@ -1219,6 +1220,12 @@ struct iommu_group *iommu_group_get(struct device *dev) } EXPORT_SYMBOL_GPL(iommu_group_get); +static struct iommu_group *iommu_group_get_locked(struct device *dev) +{ + lockdep_assert_held(&dev_iommu_group_lock); + return iommu_group_get(dev); +} + /** * iommu_group_ref_get - Increment reference on a group * @group: the group to use, must not be NULL@@ -1532,7 +1539,7 @@ static struct iommu_group *get_pci_alias_group(struct pci_dev *pdev, if (test_and_set_bit(pdev->devfn & 0xff, devfns)) return NULL; - group = iommu_group_get(&pdev->dev); + group = iommu_group_get_locked(&pdev->dev); if (group) return group;@@ -1573,7 +1580,7 @@ static int get_pci_alias_or_group(struct pci_dev *pdev, u16 alias, void *opaque) struct group_for_pci_data *data = opaque; data->pdev = pdev; - data->group = iommu_group_get(&pdev->dev); + data->group = iommu_group_get_locked(&pdev->dev); return data->group != NULL; }@@ -1629,7 +1636,7 @@ struct iommu_group *pci_device_group(struct device *dev) pdev = bus->self; - group = iommu_group_get(&pdev->dev); + group = iommu_group_get_locked(&pdev->dev); if (group) return group; }@@ -1662,7 +1669,7 @@ struct iommu_group *fsl_mc_device_group(struct device *dev) struct device *cont_dev = fsl_mc_cont_dev(dev); struct iommu_group *group; - group = iommu_group_get(cont_dev); + group = iommu_group_get_locked(cont_dev); if (!group) group = iommu_group_alloc(); return group;
_______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel