[PATCH v2 00/10] Refine the locking for dev->iommu_group

[PATCH v2 00/10] Refine the locking for dev->iommu_group

[Jason Gunthorpe]

This series puts the core code usage of dev->iommu_group under a
consistent set of locking rules. Currently a lot of places in the code
access this value without any locking or READ_ONCE/etc techniques and it
is not clear how or why this is safe.

Make it so the following locking rules are used:

 - It is readable by a probe'd device driver. So long as a device driver
   is probed the dev->iommu_group will be guaranteed stable without further
   locking. The reader should provide some kind of locking that ensure's
   its remove() struct device_driver op cannot progress.

 - Read/Write under the device_lock(), this primarily protects against
   parallel probe of the same device, and parallel probe/remove. This is
   useful for places that don't naturally have a device driver probed, and
   should be rare.

 - Read/Write under the global dev_iommu_group_lock. This is used during
   probe time discovery of groups. Device drivers will scan unlocked
   portions of the device tree to locate an already existing group. These
   scans can access the dev->iommu_group under the global lock to single
   thread determining and installing the group. This ensures that groups
   are reliably formed.

Add locking assertions to enforce this.

Along the way remove a bunch of opencoded group touching in drivers trying
to implement a 'single group per driver' approach by providing a core helper
that does this common algorithm.

Overall this significantly reduces the number of places within
drivers/iommu calling iommu_group_get or touching groups at all.

This goes on top of Joerg's next tree, it needs the prior series
"Consolidate the probe_device path".

This is on github: https://github.com/jgunthorpe/linux/commits/iommu_group_locking

v2:
 - Revise comments
 - NULL singleton_group during iommu_device_unregister() for clarity
v1: https://lore.kernel.org/r/0-v1-3c8177327a47+256-iommu_group_locking_jgg@nvidia.com

Jason Gunthorpe (10):
  iommu: Remove useless group refcounting
  iommu: Add a lockdep assertion for remaining dev->iommu_group reads
  iommu: Add generic_single_device_group()
  iommu/sun50i: Convert to generic_single_device_group()
  iommu/sprd: Convert to generic_single_device_group()
  iommu/rockchip: Convert to generic_single_device_group()
  iommu/ipmmu-vmsa: Convert to generic_single_device_group()
  iommu/omap: Convert to generic_single_device_group()
  iommu: Complete the locking for dev->iommu_group
  iommu/intel: Fix missing locking for show_device_domain_translation()

 drivers/iommu/intel/debugfs.c  |  34 ++++----
 drivers/iommu/iommu.c          | 155 +++++++++++++++++++++------------
 drivers/iommu/ipmmu-vmsa.c     |  22 ++---
 drivers/iommu/omap-iommu.c     |  30 +------
 drivers/iommu/omap-iommu.h     |   2 +-
 drivers/iommu/rockchip-iommu.c |  22 +----
 drivers/iommu/sprd-iommu.c     |  24 +----
 drivers/iommu/sun50i-iommu.c   |  29 ++----
 include/linux/iommu.h          |   3 +
 9 files changed, 138 insertions(+), 183 deletions(-)


base-commit: a5003e75a1714857c01317d04982eef81331fe2f
-- 
2.41.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 01/10] iommu: Remove useless group refcounting

[Jason Gunthorpe]

Several functions obtain the group reference and then release it before
returning. This gives the impression that the refcount is protecting
something for the duration of the function.

In truth all of these functions are called in places that know a device
driver is probed to the device and our locking rules already require
that dev->iommu_group cannot change while a driver is attached to the
struct device.

If this was not the case then this code is already at risk of triggering
UAF as it is racy if the dev->iommu_group is concurrently going to
NULL/free. refcount debugging will throw a WARN if kobject_get() is
called on a 0 refcount object to highlight the bug.

Remove the confusing refcounting and leave behind a comment about the
restriction.

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommu.c | 57 ++++++++++++++++---------------------------
 1 file changed, 21 insertions(+), 36 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 4352a149a935e8..409090eaac543a 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2014,10 +2014,10 @@ static int __iommu_attach_device(struct iommu_domain *domain,
  */
 int iommu_attach_device(struct iommu_domain *domain, struct device *dev)
 {
-	struct iommu_group *group;
+	/* Caller must be a probed driver on dev */
+	struct iommu_group *group = dev->iommu_group;
 	int ret;
 
-	group = iommu_group_get(dev);
 	if (!group)
 		return -ENODEV;
 
@@ -2034,8 +2034,6 @@ int iommu_attach_device(struct iommu_domain *domain, struct device *dev)
 
 out_unlock:
 	mutex_unlock(&group->mutex);
-	iommu_group_put(group);
-
 	return ret;
 }
 EXPORT_SYMBOL_GPL(iommu_attach_device);
@@ -2050,9 +2048,9 @@ int iommu_deferred_attach(struct device *dev, struct iommu_domain *domain)
 
 void iommu_detach_device(struct iommu_domain *domain, struct device *dev)
 {
-	struct iommu_group *group;
+	/* Caller must be a probed driver on dev */
+	struct iommu_group *group = dev->iommu_group;
 
-	group = iommu_group_get(dev);
 	if (!group)
 		return;
 
@@ -2064,24 +2062,18 @@ void iommu_detach_device(struct iommu_domain *domain, struct device *dev)
 
 out_unlock:
 	mutex_unlock(&group->mutex);
-	iommu_group_put(group);
 }
 EXPORT_SYMBOL_GPL(iommu_detach_device);
 
 struct iommu_domain *iommu_get_domain_for_dev(struct device *dev)
 {
-	struct iommu_domain *domain;
-	struct iommu_group *group;
+	/* Caller must be a probed driver on dev */
+	struct iommu_group *group = dev->iommu_group;
 
-	group = iommu_group_get(dev);
 	if (!group)
 		return NULL;
 
-	domain = group->domain;
-
-	iommu_group_put(group);
-
-	return domain;
+	return group->domain;
 }
 EXPORT_SYMBOL_GPL(iommu_get_domain_for_dev);
 
@@ -3044,7 +3036,8 @@ static bool iommu_is_default_domain(struct iommu_group *group)
  */
 int iommu_device_use_default_domain(struct device *dev)
 {
-	struct iommu_group *group = iommu_group_get(dev);
+	/* Caller is the driver core during the pre-probe path */
+	struct iommu_group *group = dev->iommu_group;
 	int ret = 0;
 
 	if (!group)
@@ -3063,8 +3056,6 @@ int iommu_device_use_default_domain(struct device *dev)
 
 unlock_out:
 	mutex_unlock(&group->mutex);
-	iommu_group_put(group);
-
 	return ret;
 }
 
@@ -3078,7 +3069,8 @@ int iommu_device_use_default_domain(struct device *dev)
  */
 void iommu_device_unuse_default_domain(struct device *dev)
 {
-	struct iommu_group *group = iommu_group_get(dev);
+	/* Caller is the driver core during the post-probe path */
+	struct iommu_group *group = dev->iommu_group;
 
 	if (!group)
 		return;
@@ -3088,7 +3080,6 @@ void iommu_device_unuse_default_domain(struct device *dev)
 		group->owner_cnt--;
 
 	mutex_unlock(&group->mutex);
-	iommu_group_put(group);
 }
 
 static int __iommu_group_alloc_blocking_domain(struct iommu_group *group)
@@ -3175,13 +3166,13 @@ EXPORT_SYMBOL_GPL(iommu_group_claim_dma_owner);
  */
 int iommu_device_claim_dma_owner(struct device *dev, void *owner)
 {
-	struct iommu_group *group;
+	/* Caller must be a probed driver on dev */
+	struct iommu_group *group = dev->iommu_group;
 	int ret = 0;
 
 	if (WARN_ON(!owner))
 		return -EINVAL;
 
-	group = iommu_group_get(dev);
 	if (!group)
 		return -ENODEV;
 
@@ -3198,8 +3189,6 @@ int iommu_device_claim_dma_owner(struct device *dev, void *owner)
 	ret = __iommu_take_dma_ownership(group, owner);
 unlock_out:
 	mutex_unlock(&group->mutex);
-	iommu_group_put(group);
-
 	return ret;
 }
 EXPORT_SYMBOL_GPL(iommu_device_claim_dma_owner);
@@ -3237,7 +3226,8 @@ EXPORT_SYMBOL_GPL(iommu_group_release_dma_owner);
  */
 void iommu_device_release_dma_owner(struct device *dev)
 {
-	struct iommu_group *group = iommu_group_get(dev);
+	/* Caller must be a probed driver on dev */
+	struct iommu_group *group = dev->iommu_group;
 
 	mutex_lock(&group->mutex);
 	if (group->owner_cnt > 1)
@@ -3245,7 +3235,6 @@ void iommu_device_release_dma_owner(struct device *dev)
 	else
 		__iommu_release_dma_ownership(group);
 	mutex_unlock(&group->mutex);
-	iommu_group_put(group);
 }
 EXPORT_SYMBOL_GPL(iommu_device_release_dma_owner);
 
@@ -3306,14 +3295,14 @@ static void __iommu_remove_group_pasid(struct iommu_group *group,
 int iommu_attach_device_pasid(struct iommu_domain *domain,
 			      struct device *dev, ioasid_t pasid)
 {
-	struct iommu_group *group;
+	/* Caller must be a probed driver on dev */
+	struct iommu_group *group = dev->iommu_group;
 	void *curr;
 	int ret;
 
 	if (!domain->ops->set_dev_pasid)
 		return -EOPNOTSUPP;
 
-	group = iommu_group_get(dev);
 	if (!group)
 		return -ENODEV;
 
@@ -3331,8 +3320,6 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
 	}
 out_unlock:
 	mutex_unlock(&group->mutex);
-	iommu_group_put(group);
-
 	return ret;
 }
 EXPORT_SYMBOL_GPL(iommu_attach_device_pasid);
@@ -3349,14 +3336,13 @@ EXPORT_SYMBOL_GPL(iommu_attach_device_pasid);
 void iommu_detach_device_pasid(struct iommu_domain *domain, struct device *dev,
 			       ioasid_t pasid)
 {
-	struct iommu_group *group = iommu_group_get(dev);
+	/* Caller must be a probed driver on dev */
+	struct iommu_group *group = dev->iommu_group;
 
 	mutex_lock(&group->mutex);
 	__iommu_remove_group_pasid(group, pasid);
 	WARN_ON(xa_erase(&group->pasid_array, pasid) != domain);
 	mutex_unlock(&group->mutex);
-
-	iommu_group_put(group);
 }
 EXPORT_SYMBOL_GPL(iommu_detach_device_pasid);
 
@@ -3378,10 +3364,10 @@ struct iommu_domain *iommu_get_domain_for_dev_pasid(struct device *dev,
 						    ioasid_t pasid,
 						    unsigned int type)
 {
+	/* Caller must be a probed driver on dev */
+	struct iommu_group *group = dev->iommu_group;
 	struct iommu_domain *domain;
-	struct iommu_group *group;
 
-	group = iommu_group_get(dev);
 	if (!group)
 		return NULL;
 
@@ -3390,7 +3376,6 @@ struct iommu_domain *iommu_get_domain_for_dev_pasid(struct device *dev,
 	if (type && domain && domain->type != type)
 		domain = ERR_PTR(-EBUSY);
 	xa_unlock(&group->pasid_array);
-	iommu_group_put(group);
 
 	return domain;
 }
-- 
2.41.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v2 01/10] iommu: Remove useless group refcounting

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Tuesday, August 1, 2023 1:50 AM
> 
> Several functions obtain the group reference and then release it before
> returning. This gives the impression that the refcount is protecting
> something for the duration of the function.
> 
> In truth all of these functions are called in places that know a device
> driver is probed to the device and our locking rules already require
> that dev->iommu_group cannot change while a driver is attached to the
> struct device.
> 
> If this was not the case then this code is already at risk of triggering
> UAF as it is racy if the dev->iommu_group is concurrently going to
> NULL/free. refcount debugging will throw a WARN if kobject_get() is
> called on a 0 refcount object to highlight the bug.
> 
> Remove the confusing refcounting and leave behind a comment about the
> restriction.
> 
> Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 02/10] iommu: Add a lockdep assertion for remaining dev->iommu_group reads

[Jason Gunthorpe]

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.

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;
-- 
2.41.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v2 02/10] iommu: Add a lockdep assertion for remaining dev->iommu_group reads

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Tuesday, August 1, 2023 1:50 AM
> 
> 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.
> 
> 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>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 02/10] iommu: Add a lockdep assertion for remaining dev->iommu_group reads

[Robin Murphy]

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.

> 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

Re: [PATCH v2 02/10] iommu: Add a lockdep assertion for remaining dev->iommu_group reads

[Jason Gunthorpe]

On Tue, Aug 08, 2023 at 05:22:55PM +0100, Robin Murphy wrote:
> 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(), 

Well, that may have been the intention, but as a side effect it turns
out that it is the only thing that locks the access to the
dev->iommu_group as well. This is some other bug that I suppose nobody
noticed.

> 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.

Regardless, it does serialize the group stuff so what I did here is
recognize that as its main purpose and made the probe serialization a
secondary thing, which is eventually entirely removed.

I could have constructed this the other way and said that the group
locking is missing and added another global lock, but that seems
equally confusing since it isn't missing, it is just mis-named :)

> 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.

That is great, but it won't address the dev->group locking.

I'm not sure there is further value in trying to remove the
device_lock() around probe, but cleaning up the iommu_configure stuff
would be nice.

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 03/10] iommu: Add generic_single_device_group()

[Jason Gunthorpe]

This implements the common pattern seen in drivers of a single iommu_group
for the entire iommu driver instance. Implement this in core code so the
drivers that want this can select it from their ops.

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommu.c | 27 +++++++++++++++++++++++++++
 include/linux/iommu.h |  3 +++
 2 files changed, 30 insertions(+)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index f1c8a333553e3b..c673d021d367e4 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -289,6 +289,10 @@ void iommu_device_unregister(struct iommu_device *iommu)
 	spin_lock(&iommu_device_lock);
 	list_del(&iommu->list);
 	spin_unlock(&iommu_device_lock);
+
+	/* Pairs with the alloc in generic_single_device_group() */
+	iommu_group_put(iommu->singleton_group);
+	iommu->singleton_group = NULL;
 }
 EXPORT_SYMBOL_GPL(iommu_device_unregister);
 
@@ -1595,6 +1599,29 @@ struct iommu_group *generic_device_group(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(generic_device_group);
 
+/*
+ * Generic device_group call-back function. It just allocates one
+ * iommu-group per iommu driver instance shared by every device
+ * probed by that iommu driver.
+ */
+struct iommu_group *generic_single_device_group(struct device *dev)
+{
+	struct iommu_device *iommu = dev->iommu->iommu_dev;
+
+	lockdep_assert_held(&dev_iommu_group_lock);
+
+	if (!iommu->singleton_group) {
+		struct iommu_group *group;
+
+		group = iommu_group_alloc();
+		if (IS_ERR(group))
+			return group;
+		iommu->singleton_group = group;
+	}
+	return iommu_group_ref_get(iommu->singleton_group);
+}
+EXPORT_SYMBOL_GPL(generic_single_device_group);
+
 /*
  * Use standard PCI bus topology, isolation features, and DMA alias quirks
  * to find or create an IOMMU group for a device.
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index b1dcb1b9b17040..f1e18e81fca78b 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -361,6 +361,7 @@ struct iommu_domain_ops {
  * @list: Used by the iommu-core to keep a list of registered iommus
  * @ops: iommu-ops for talking to this iommu
  * @dev: struct device for sysfs handling
+ * @singleton_group: Used internally for drivers that have only one group
  * @max_pasids: number of supported PASIDs
  */
 struct iommu_device {
@@ -368,6 +369,7 @@ struct iommu_device {
 	const struct iommu_ops *ops;
 	struct fwnode_handle *fwnode;
 	struct device *dev;
+	struct iommu_group *singleton_group;
 	u32 max_pasids;
 };
 
@@ -640,6 +642,7 @@ extern struct iommu_group *pci_device_group(struct device *dev);
 extern struct iommu_group *generic_device_group(struct device *dev);
 /* FSL-MC device grouping function */
 struct iommu_group *fsl_mc_device_group(struct device *dev);
+extern struct iommu_group *generic_single_device_group(struct device *dev);
 
 /**
  * struct iommu_fwspec - per-device IOMMU instance data
-- 
2.41.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v2 03/10] iommu: Add generic_single_device_group()

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Tuesday, August 1, 2023 1:50 AM
> 
> This implements the common pattern seen in drivers of a single
> iommu_group
> for the entire iommu driver instance. Implement this in core code so the
> drivers that want this can select it from their ops.
> 
> Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 04/10] iommu/sun50i: Convert to generic_single_device_group()

[Jason Gunthorpe]

Use the new helper.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/sun50i-iommu.c | 29 ++++++-----------------------
 1 file changed, 6 insertions(+), 23 deletions(-)

diff --git a/drivers/iommu/sun50i-iommu.c b/drivers/iommu/sun50i-iommu.c
index 74c5cb93e90027..b8df655185ab2a 100644
--- a/drivers/iommu/sun50i-iommu.c
+++ b/drivers/iommu/sun50i-iommu.c
@@ -107,7 +107,6 @@ struct sun50i_iommu {
 	struct clk *clk;
 
 	struct iommu_domain *domain;
-	struct iommu_group *group;
 	struct kmem_cache *pt_pool;
 };
 
@@ -808,13 +807,6 @@ static struct iommu_device *sun50i_iommu_probe_device(struct device *dev)
 	return &iommu->iommu;
 }
 
-static struct iommu_group *sun50i_iommu_device_group(struct device *dev)
-{
-	struct sun50i_iommu *iommu = sun50i_iommu_from_dev(dev);
-
-	return iommu_group_ref_get(iommu->group);
-}
-
 static int sun50i_iommu_of_xlate(struct device *dev,
 				 struct of_phandle_args *args)
 {
@@ -828,7 +820,7 @@ static int sun50i_iommu_of_xlate(struct device *dev,
 
 static const struct iommu_ops sun50i_iommu_ops = {
 	.pgsize_bitmap	= SZ_4K,
-	.device_group	= sun50i_iommu_device_group,
+	.device_group	= generic_single_device_group,
 	.domain_alloc	= sun50i_iommu_domain_alloc,
 	.of_xlate	= sun50i_iommu_of_xlate,
 	.probe_device	= sun50i_iommu_probe_device,
@@ -995,42 +987,36 @@ static int sun50i_iommu_probe(struct platform_device *pdev)
 	if (!iommu->pt_pool)
 		return -ENOMEM;
 
-	iommu->group = iommu_group_alloc();
-	if (IS_ERR(iommu->group)) {
-		ret = PTR_ERR(iommu->group);
-		goto err_free_cache;
-	}
-
 	iommu->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(iommu->base)) {
 		ret = PTR_ERR(iommu->base);
-		goto err_free_group;
+		goto err_free_cache;
 	}
 
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0) {
 		ret = irq;
-		goto err_free_group;
+		goto err_free_cache;
 	}
 
 	iommu->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(iommu->clk)) {
 		dev_err(&pdev->dev, "Couldn't get our clock.\n");
 		ret = PTR_ERR(iommu->clk);
-		goto err_free_group;
+		goto err_free_cache;
 	}
 
 	iommu->reset = devm_reset_control_get(&pdev->dev, NULL);
 	if (IS_ERR(iommu->reset)) {
 		dev_err(&pdev->dev, "Couldn't get our reset line.\n");
 		ret = PTR_ERR(iommu->reset);
-		goto err_free_group;
+		goto err_free_cache;
 	}
 
 	ret = iommu_device_sysfs_add(&iommu->iommu, &pdev->dev,
 				     NULL, dev_name(&pdev->dev));
 	if (ret)
-		goto err_free_group;
+		goto err_free_cache;
 
 	ret = iommu_device_register(&iommu->iommu, &sun50i_iommu_ops, &pdev->dev);
 	if (ret)
@@ -1049,9 +1035,6 @@ static int sun50i_iommu_probe(struct platform_device *pdev)
 err_remove_sysfs:
 	iommu_device_sysfs_remove(&iommu->iommu);
 
-err_free_group:
-	iommu_group_put(iommu->group);
-
 err_free_cache:
 	kmem_cache_destroy(iommu->pt_pool);
 
-- 
2.41.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 05/10] iommu/sprd: Convert to generic_single_device_group()

[Jason Gunthorpe]

Use the new helper.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/sprd-iommu.c | 24 ++----------------------
 1 file changed, 2 insertions(+), 22 deletions(-)

diff --git a/drivers/iommu/sprd-iommu.c b/drivers/iommu/sprd-iommu.c
index 39e34fdeccda78..9fc98fd144e445 100644
--- a/drivers/iommu/sprd-iommu.c
+++ b/drivers/iommu/sprd-iommu.c
@@ -69,7 +69,6 @@ struct sprd_iommu_device {
 	void __iomem		*base;
 	struct device		*dev;
 	struct iommu_device	iommu;
-	struct iommu_group	*group;
 	struct clk		*eb;
 };
 
@@ -397,13 +396,6 @@ static struct iommu_device *sprd_iommu_probe_device(struct device *dev)
 	return &sdev->iommu;
 }
 
-static struct iommu_group *sprd_iommu_device_group(struct device *dev)
-{
-	struct sprd_iommu_device *sdev = dev_iommu_priv_get(dev);
-
-	return iommu_group_ref_get(sdev->group);
-}
-
 static int sprd_iommu_of_xlate(struct device *dev, struct of_phandle_args *args)
 {
 	struct platform_device *pdev;
@@ -421,7 +413,7 @@ static int sprd_iommu_of_xlate(struct device *dev, struct of_phandle_args *args)
 static const struct iommu_ops sprd_iommu_ops = {
 	.domain_alloc	= sprd_iommu_domain_alloc,
 	.probe_device	= sprd_iommu_probe_device,
-	.device_group	= sprd_iommu_device_group,
+	.device_group	= generic_single_device_group,
 	.of_xlate	= sprd_iommu_of_xlate,
 	.pgsize_bitmap	= SPRD_IOMMU_PAGE_SIZE,
 	.owner		= THIS_MODULE,
@@ -494,16 +486,9 @@ static int sprd_iommu_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, sdev);
 	sdev->dev = dev;
 
-	/* All the client devices are in the same iommu-group */
-	sdev->group = iommu_group_alloc();
-	if (IS_ERR(sdev->group)) {
-		ret = PTR_ERR(sdev->group);
-		goto free_page;
-	}
-
 	ret = iommu_device_sysfs_add(&sdev->iommu, dev, NULL, dev_name(dev));
 	if (ret)
-		goto put_group;
+		goto free_page;
 
 	ret = iommu_device_register(&sdev->iommu, &sprd_iommu_ops, dev);
 	if (ret)
@@ -528,8 +513,6 @@ static int sprd_iommu_probe(struct platform_device *pdev)
 	iommu_device_unregister(&sdev->iommu);
 remove_sysfs:
 	iommu_device_sysfs_remove(&sdev->iommu);
-put_group:
-	iommu_group_put(sdev->group);
 free_page:
 	dma_free_coherent(sdev->dev, SPRD_IOMMU_PAGE_SIZE, sdev->prot_page_va, sdev->prot_page_pa);
 	return ret;
@@ -541,9 +524,6 @@ static void sprd_iommu_remove(struct platform_device *pdev)
 
 	dma_free_coherent(sdev->dev, SPRD_IOMMU_PAGE_SIZE, sdev->prot_page_va, sdev->prot_page_pa);
 
-	iommu_group_put(sdev->group);
-	sdev->group = NULL;
-
 	platform_set_drvdata(pdev, NULL);
 	iommu_device_sysfs_remove(&sdev->iommu);
 	iommu_device_unregister(&sdev->iommu);
-- 
2.41.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 06/10] iommu/rockchip: Convert to generic_single_device_group()

[Jason Gunthorpe]

Use the new helper.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/rockchip-iommu.c | 22 ++--------------------
 1 file changed, 2 insertions(+), 20 deletions(-)

diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index 8ff69fbf9f65db..91f13cc9411548 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -113,7 +113,6 @@ struct rk_iommu {
 	struct iommu_device iommu;
 	struct list_head node; /* entry in rk_iommu_domain.iommus */
 	struct iommu_domain *domain; /* domain to which iommu is attached */
-	struct iommu_group *group;
 };
 
 struct rk_iommudata {
@@ -1155,15 +1154,6 @@ static void rk_iommu_release_device(struct device *dev)
 	device_link_del(data->link);
 }
 
-static struct iommu_group *rk_iommu_device_group(struct device *dev)
-{
-	struct rk_iommu *iommu;
-
-	iommu = rk_iommu_from_dev(dev);
-
-	return iommu_group_ref_get(iommu->group);
-}
-
 static int rk_iommu_of_xlate(struct device *dev,
 			     struct of_phandle_args *args)
 {
@@ -1189,7 +1179,7 @@ static const struct iommu_ops rk_iommu_ops = {
 	.domain_alloc = rk_iommu_domain_alloc,
 	.probe_device = rk_iommu_probe_device,
 	.release_device = rk_iommu_release_device,
-	.device_group = rk_iommu_device_group,
+	.device_group = generic_single_device_group,
 #ifdef CONFIG_ARM
 	.set_platform_dma_ops = rk_iommu_set_platform_dma,
 #endif
@@ -1280,15 +1270,9 @@ static int rk_iommu_probe(struct platform_device *pdev)
 	if (err)
 		return err;
 
-	iommu->group = iommu_group_alloc();
-	if (IS_ERR(iommu->group)) {
-		err = PTR_ERR(iommu->group);
-		goto err_unprepare_clocks;
-	}
-
 	err = iommu_device_sysfs_add(&iommu->iommu, dev, NULL, dev_name(dev));
 	if (err)
-		goto err_put_group;
+		goto err_unprepare_clocks;
 
 	err = iommu_device_register(&iommu->iommu, &rk_iommu_ops, dev);
 	if (err)
@@ -1325,8 +1309,6 @@ static int rk_iommu_probe(struct platform_device *pdev)
 	pm_runtime_disable(dev);
 err_remove_sysfs:
 	iommu_device_sysfs_remove(&iommu->iommu);
-err_put_group:
-	iommu_group_put(iommu->group);
 err_unprepare_clocks:
 	clk_bulk_unprepare(iommu->num_clocks, iommu->clocks);
 	return err;
-- 
2.41.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 06/10] iommu/rockchip: Convert to generic_single_device_group()

[Marek Szyprowski]

Hi Jason,

On 31.07.2023 19:50, Jason Gunthorpe wrote:
> Use the new helper.
>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>   drivers/iommu/rockchip-iommu.c | 22 ++--------------------
>   1 file changed, 2 insertions(+), 20 deletions(-)

After applying your recent fixes from "[PATCH 0/3] Fix device_lock 
deadlock on two probe() paths" thread I've decided to run more tests on 
all boards I have. This way I found that this patch breaks booting of 
Odroid-M1 board, which is ARM64 system based on Rockchip RK3568 
(arch/arm64/boot/dts/rockchip/rk3568-odroid-m1.dts). Here is the log:

Unable to handle kernel NULL pointer dereference at virtual address 
0000000000000028
Mem abort info:
...
Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
Modules linked in: hantro_vpu(+) crct10dif_ce rockchip_saradc v4l2_vp9 
industrialio_triggered_buffer v4l2_h264 kfifo_buf display_connector 
dwmac_rk(+) v4l2_mem2mem snd_soc_simple_card stmmac_platform 
videobuf2_dma_contig snd_soc_simple_card_utils videobuf2_memops 
rockchip_thermal videobuf2_v4l2 stmmac videodev pcs_xpcs 
snd_soc_rockchip_i2s_tdm videobuf2_common rtc_rk808 mc panfrost 
rockchipdrm drm_shmem_helper gpu_sched analogix_dp dw_mipi_dsi dw_hdmi 
drm_display_helper ip_tables x_tables ipv6
CPU: 2 PID: 147 Comm: systemd-udevd Not tainted 6.5.0-rc1+ #13826
Hardware name: Hardkernel ODROID-M1 (DT)
pstate: 60400009 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : generic_single_device_group+0x24/0x88
lr : generic_single_device_group+0x5c/0x88
...
Call trace:
  generic_single_device_group+0x24/0x88
  __iommu_probe_device+0xe8/0x444
  iommu_probe_device+0x1c/0x54
  of_iommu_configure+0x10c/0x200
  of_dma_configure_id+0x1e0/0x3b4
  platform_dma_configure+0x30/0x78
  really_probe+0x70/0x2b4
  __driver_probe_device+0x78/0x12c
  driver_probe_device+0x3c/0x160
  __driver_attach+0x9c/0x1ac
  bus_for_each_dev+0x74/0xd4
  driver_attach+0x24/0x30
  bus_add_driver+0xe4/0x1e8
  driver_register+0x60/0x128
  __platform_driver_register+0x28/0x34
  hantro_driver_init+0x20/0x1000 [hantro_vpu]
  do_one_initcall+0x74/0x2f0
  do_init_module+0x58/0x1ec
  load_module+0x1a20/0x1c64
  init_module_from_file+0x84/0xc0
  idempotent_init_module+0x180/0x250
  __arm64_sys_finit_module+0x64/0xa0
  invoke_syscall+0x48/0x114
  el0_svc_common.constprop.0+0xec/0x10c
  do_el0_svc+0x38/0xa4
  el0_svc+0x40/0xac
  el0t_64_sync_handler+0xc0/0xc4
  el0t_64_sync+0x190/0x194
---[ end trace 0000000000000000 ]---

It turns out that iommu pointer gathered from dev->iommu->iommu_dev is 
NULL in generic_single_device_group() in the above call stack. Reverting 
$subject on top of linux-next+"[PATCH 0/3] Fix device_lock deadlock on 
two probe() paths" fixes booting.


> diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
> index 8ff69fbf9f65db..91f13cc9411548 100644
> --- a/drivers/iommu/rockchip-iommu.c
> +++ b/drivers/iommu/rockchip-iommu.c
> @@ -113,7 +113,6 @@ struct rk_iommu {
>   	struct iommu_device iommu;
>   	struct list_head node; /* entry in rk_iommu_domain.iommus */
>   	struct iommu_domain *domain; /* domain to which iommu is attached */
> -	struct iommu_group *group;
>   };
>   
>   struct rk_iommudata {
> @@ -1155,15 +1154,6 @@ static void rk_iommu_release_device(struct device *dev)
>   	device_link_del(data->link);
>   }
>   
> -static struct iommu_group *rk_iommu_device_group(struct device *dev)
> -{
> -	struct rk_iommu *iommu;
> -
> -	iommu = rk_iommu_from_dev(dev);
> -
> -	return iommu_group_ref_get(iommu->group);
> -}
> -
>   static int rk_iommu_of_xlate(struct device *dev,
>   			     struct of_phandle_args *args)
>   {
> @@ -1189,7 +1179,7 @@ static const struct iommu_ops rk_iommu_ops = {
>   	.domain_alloc = rk_iommu_domain_alloc,
>   	.probe_device = rk_iommu_probe_device,
>   	.release_device = rk_iommu_release_device,
> -	.device_group = rk_iommu_device_group,
> +	.device_group = generic_single_device_group,
>   #ifdef CONFIG_ARM
>   	.set_platform_dma_ops = rk_iommu_set_platform_dma,
>   #endif
> @@ -1280,15 +1270,9 @@ static int rk_iommu_probe(struct platform_device *pdev)
>   	if (err)
>   		return err;
>   
> -	iommu->group = iommu_group_alloc();
> -	if (IS_ERR(iommu->group)) {
> -		err = PTR_ERR(iommu->group);
> -		goto err_unprepare_clocks;
> -	}
> -
>   	err = iommu_device_sysfs_add(&iommu->iommu, dev, NULL, dev_name(dev));
>   	if (err)
> -		goto err_put_group;
> +		goto err_unprepare_clocks;
>   
>   	err = iommu_device_register(&iommu->iommu, &rk_iommu_ops, dev);
>   	if (err)
> @@ -1325,8 +1309,6 @@ static int rk_iommu_probe(struct platform_device *pdev)
>   	pm_runtime_disable(dev);
>   err_remove_sysfs:
>   	iommu_device_sysfs_remove(&iommu->iommu);
> -err_put_group:
> -	iommu_group_put(iommu->group);
>   err_unprepare_clocks:
>   	clk_bulk_unprepare(iommu->num_clocks, iommu->clocks);
>   	return err;

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 06/10] iommu/rockchip: Convert to generic_single_device_group()

[Jason Gunthorpe]

On Wed, Aug 09, 2023 at 03:19:34PM +0200, Marek Szyprowski wrote:
> Hi Jason,
> 
> On 31.07.2023 19:50, Jason Gunthorpe wrote:
> > Use the new helper.
> >
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> > ---
> >   drivers/iommu/rockchip-iommu.c | 22 ++--------------------
> >   1 file changed, 2 insertions(+), 20 deletions(-)
> 
> After applying your recent fixes from "[PATCH 0/3] Fix device_lock 
> deadlock on two probe() paths" thread I've decided to run more tests on 
> all boards I have. This way I found that this patch breaks booting of 
> Odroid-M1 board, which is ARM64 system based on Rockchip RK3568 
> (arch/arm64/boot/dts/rockchip/rk3568-odroid-m1.dts). Here is the log:

Is this why?

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 8842f4975ec4a8..8677d3ace47bbe 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -366,6 +366,7 @@ static int iommu_init_device(struct device *dev, const struct iommu_ops *ops)
 		ret = PTR_ERR(iommu_dev);
 		goto err_module_put;
 	}
+	dev->iommu->iommu_dev = iommu_dev;
 
 	ret = iommu_device_link(iommu_dev, dev);
 	if (ret)
@@ -383,7 +384,6 @@ static int iommu_init_device(struct device *dev, const struct iommu_ops *ops)
 	dev->iommu_group = group;
 	mutex_unlock(&dev_iommu_group_lock);
 
-	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);
@@ -397,6 +397,7 @@ static int iommu_init_device(struct device *dev, const struct iommu_ops *ops)
 err_module_put:
 	module_put(ops->owner);
 err_free:
+	dev->iommu->iommu_dev = NULL;
 	dev_iommu_free(dev);
 	return ret;
 }

Thanks,
Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 06/10] iommu/rockchip: Convert to generic_single_device_group()

[Marek Szyprowski]

Hi Jason,

On 09.08.2023 15:51, Jason Gunthorpe wrote:
> On Wed, Aug 09, 2023 at 03:19:34PM +0200, Marek Szyprowski wrote:
>> On 31.07.2023 19:50, Jason Gunthorpe wrote:
>>> Use the new helper.
>>>
>>> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
>>> ---
>>>    drivers/iommu/rockchip-iommu.c | 22 ++--------------------
>>>    1 file changed, 2 insertions(+), 20 deletions(-)
>> After applying your recent fixes from "[PATCH 0/3] Fix device_lock
>> deadlock on two probe() paths" thread I've decided to run more tests on
>> all boards I have. This way I found that this patch breaks booting of
>> Odroid-M1 board, which is ARM64 system based on Rockchip RK3568
>> (arch/arm64/boot/dts/rockchip/rk3568-odroid-m1.dts). Here is the log:
> Is this why?

Right, this fixed the issue.

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 8842f4975ec4a8..8677d3ace47bbe 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -366,6 +366,7 @@ static int iommu_init_device(struct device *dev, const struct iommu_ops *ops)
>   		ret = PTR_ERR(iommu_dev);
>   		goto err_module_put;
>   	}
> +	dev->iommu->iommu_dev = iommu_dev;
>   
>   	ret = iommu_device_link(iommu_dev, dev);
>   	if (ret)
> @@ -383,7 +384,6 @@ static int iommu_init_device(struct device *dev, const struct iommu_ops *ops)
>   	dev->iommu_group = group;
>   	mutex_unlock(&dev_iommu_group_lock);
>   
> -	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);
> @@ -397,6 +397,7 @@ static int iommu_init_device(struct device *dev, const struct iommu_ops *ops)
>   err_module_put:
>   	module_put(ops->owner);
>   err_free:
> +	dev->iommu->iommu_dev = NULL;
>   	dev_iommu_free(dev);
>   	return ret;
>   }
>
> Thanks,
> Jason
>
Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 07/10] iommu/ipmmu-vmsa: Convert to generic_single_device_group()

[Jason Gunthorpe]

Use the new helper.

This driver is kind of weird since in ARM mode it pretends it has
per-device groups, but ARM64 mode does not.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/ipmmu-vmsa.c | 22 +++++-----------------
 1 file changed, 5 insertions(+), 17 deletions(-)

diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index 9f64c5c9f5b90a..55b9b29221461c 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -63,7 +63,6 @@ struct ipmmu_vmsa_device {
 	struct ipmmu_vmsa_domain *domains[IPMMU_CTX_MAX];
 	s8 utlb_ctx[IPMMU_UTLB_MAX];
 
-	struct iommu_group *group;
 	struct dma_iommu_mapping *mapping;
 };
 
@@ -832,28 +831,17 @@ static void ipmmu_release_device(struct device *dev)
 	arm_iommu_release_mapping(mmu->mapping);
 }
 
-static struct iommu_group *ipmmu_find_group(struct device *dev)
-{
-	struct ipmmu_vmsa_device *mmu = to_ipmmu(dev);
-	struct iommu_group *group;
-
-	if (mmu->group)
-		return iommu_group_ref_get(mmu->group);
-
-	group = iommu_group_alloc();
-	if (!IS_ERR(group))
-		mmu->group = group;
-
-	return group;
-}
-
 static const struct iommu_ops ipmmu_ops = {
 	.domain_alloc = ipmmu_domain_alloc,
 	.probe_device = ipmmu_probe_device,
 	.release_device = ipmmu_release_device,
 	.probe_finalize = ipmmu_probe_finalize,
+	/*
+	 * FIXME: The device grouping is a fixed property of the hardware's
+	 * ability to isolate and control DMA, it should not depend on kconfig.
+	 */
 	.device_group = IS_ENABLED(CONFIG_ARM) && !IS_ENABLED(CONFIG_IOMMU_DMA)
-			? generic_device_group : ipmmu_find_group,
+			? generic_device_group : generic_single_device_group,
 	.pgsize_bitmap = SZ_1G | SZ_2M | SZ_4K,
 	.of_xlate = ipmmu_of_xlate,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
-- 
2.41.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 08/10] iommu/omap: Convert to generic_single_device_group()

[Jason Gunthorpe]

Use the new helper.

For some reason omap will probe its driver even if it doesn't load an
iommu driver. Keep this working by keeping a bool to track if the iommu
driver was started.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/omap-iommu.c | 30 ++++--------------------------
 drivers/iommu/omap-iommu.h |  2 +-
 2 files changed, 5 insertions(+), 27 deletions(-)

diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index 537e402f9bba97..97c45f50bf4332 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1225,18 +1225,15 @@ static int omap_iommu_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, obj);
 
 	if (omap_iommu_can_register(pdev)) {
-		obj->group = iommu_group_alloc();
-		if (IS_ERR(obj->group))
-			return PTR_ERR(obj->group);
-
 		err = iommu_device_sysfs_add(&obj->iommu, obj->dev, NULL,
 					     obj->name);
 		if (err)
-			goto out_group;
+			return err;
 
 		err = iommu_device_register(&obj->iommu, &omap_iommu_ops, &pdev->dev);
 		if (err)
 			goto out_sysfs;
+		obj->has_iommu_driver = true;
 	}
 
 	pm_runtime_enable(obj->dev);
@@ -1252,8 +1249,6 @@ static int omap_iommu_probe(struct platform_device *pdev)
 
 out_sysfs:
 	iommu_device_sysfs_remove(&obj->iommu);
-out_group:
-	iommu_group_put(obj->group);
 	return err;
 }
 
@@ -1261,10 +1256,7 @@ static void omap_iommu_remove(struct platform_device *pdev)
 {
 	struct omap_iommu *obj = platform_get_drvdata(pdev);
 
-	if (obj->group) {
-		iommu_group_put(obj->group);
-		obj->group = NULL;
-
+	if (obj->has_iommu_driver) {
 		iommu_device_sysfs_remove(&obj->iommu);
 		iommu_device_unregister(&obj->iommu);
 	}
@@ -1717,25 +1709,11 @@ static void omap_iommu_release_device(struct device *dev)
 
 }
 
-static struct iommu_group *omap_iommu_device_group(struct device *dev)
-{
-	struct omap_iommu_arch_data *arch_data = dev_iommu_priv_get(dev);
-	struct iommu_group *group = ERR_PTR(-EINVAL);
-
-	if (!arch_data)
-		return ERR_PTR(-ENODEV);
-
-	if (arch_data->iommu_dev)
-		group = iommu_group_ref_get(arch_data->iommu_dev->group);
-
-	return group;
-}
-
 static const struct iommu_ops omap_iommu_ops = {
 	.domain_alloc	= omap_iommu_domain_alloc,
 	.probe_device	= omap_iommu_probe_device,
 	.release_device	= omap_iommu_release_device,
-	.device_group	= omap_iommu_device_group,
+	.device_group	= generic_single_device_group,
 	.set_platform_dma_ops = omap_iommu_set_platform_dma,
 	.pgsize_bitmap	= OMAP_IOMMU_PGSIZES,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
diff --git a/drivers/iommu/omap-iommu.h b/drivers/iommu/omap-iommu.h
index 18ee713ede784d..27697109ec79a5 100644
--- a/drivers/iommu/omap-iommu.h
+++ b/drivers/iommu/omap-iommu.h
@@ -80,7 +80,7 @@ struct omap_iommu {
 	u32 id;
 
 	struct iommu_device iommu;
-	struct iommu_group *group;
+	bool has_iommu_driver;
 
 	u8 pwrst;
 };
-- 
2.41.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 09/10] iommu: Complete the locking for dev->iommu_group

[Jason Gunthorpe]

Revise the locking for dev->iommu_group so that it has three safe ways to
access it:

 - It is read by a probe'd device driver. So long as a device driver is
   probed the dev->iommu_group will be guaranteed stable without further
   locking.

 - Read under the device_lock(), this primarily protects against
   parallel probe of the same device, and parallel probe/remove

 - Read/Write under the global dev_iommu_group_lock. This is used during
   probe time discovery of groups. Device drivers will scan unlocked
   portions of the device tree to locate an already existing group. These
   scans can access the dev->iommu_group under the global lock to single
   thread determining and installing the group. This ensures that groups
   are reliably formed.

Narrow the scope of the global dev_iommu_group_lock to be only during the
dev->iommu_group setup, and not for the entire probing.

Prior patches removed the various races inherent to the probe process by
consolidating all the work under the group->mutex. In this configuration
it is fine if two devices race to the group_device step of a new
iommu_group, the group->mutex locking will ensure the group_device and
domain setup part remains properly ordered.

Add the missing locking on the remove paths. For iommu_deinit_device() it
is necessary to hold the dev_iommu_group_lock due to possible races during
probe error unwind.

Fully lock the iommu_group_add/remove_device() path so we can use lockdep
assertions. Other than lockdep this is redundant, VFIO no-iommu doesn't
use group clustering.

For iommu_release_device() it is redundant, as we expect no external
references to the struct device by this point, but it is harmless so
add the missing lock to allow lockdep assertions to work.

This resolves the remarks of the comment in __iommu_probe_device().

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommu.c | 54 +++++++++++++++++++++++++++++--------------
 1 file changed, 37 insertions(+), 17 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index c673d021d367e4..7dbbcffac21930 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -370,14 +370,17 @@ static int iommu_init_device(struct device *dev, const struct iommu_ops *ops)
 	if (ret)
 		goto err_release;
 
+	mutex_lock(&dev_iommu_group_lock);
 	group = ops->device_group(dev);
 	if (WARN_ON_ONCE(group == NULL))
 		group = ERR_PTR(-EINVAL);
 	if (IS_ERR(group)) {
+		mutex_unlock(&dev_iommu_group_lock);
 		ret = PTR_ERR(group);
 		goto err_unlink;
 	}
 	dev->iommu_group = group;
+	mutex_unlock(&dev_iommu_group_lock);
 
 	dev->iommu->iommu_dev = iommu_dev;
 	dev->iommu->max_pasids = dev_iommu_get_max_pasids(dev);
@@ -435,7 +438,9 @@ static void iommu_deinit_device(struct device *dev)
 	}
 
 	/* Caller must put iommu_group */
+	mutex_lock(&dev_iommu_group_lock);
 	dev->iommu_group = NULL;
+	mutex_unlock(&dev_iommu_group_lock);
 	module_put(ops->owner);
 	dev_iommu_free(dev);
 }
@@ -450,13 +455,11 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
 	if (!ops)
 		return -ENODEV;
 	/*
-	 * Serialise to avoid races between IOMMU drivers registering in
-	 * parallel and/or the "replay" calls from ACPI/OF code via client
-	 * driver probe. Once the latter have been cleaned up we should
-	 * probably be able to use device_lock() here to minimise the scope,
-	 * but for now enforcing a simple global ordering is fine.
+	 * Allow __iommu_probe_device() to be safely called in parallel,
+	 * both dev->iommu_group and the initial setup of dev->iommu are
+	 * protected this way.
 	 */
-	mutex_lock(&dev_iommu_group_lock);
+	device_lock(dev);
 
 	/* Device is probed already if in a group */
 	if (dev->iommu_group) {
@@ -502,7 +505,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(&dev_iommu_group_lock);
+	device_unlock(dev);
 
 	if (dev_is_pci(dev))
 		iommu_dma_set_pci_32bit_workaround(dev);
@@ -517,8 +520,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(&dev_iommu_group_lock);
-
+	device_unlock(dev);
 	return ret;
 }
 
@@ -567,6 +569,7 @@ static void __iommu_group_remove_device(struct device *dev)
 	struct iommu_group *group = dev->iommu_group;
 	struct group_device *device;
 
+	device_lock_assert(dev);
 	mutex_lock(&group->mutex);
 	for_each_group_device(group, device) {
 		if (device->dev != dev)
@@ -591,14 +594,23 @@ static void __iommu_group_remove_device(struct device *dev)
 
 static void iommu_release_device(struct device *dev)
 {
-	struct iommu_group *group = dev->iommu_group;
+	struct iommu_group *group;
 
+	/*
+	 * This locking for dev->iommu_group is overkill when this is called
+	 * from the BUS_NOTIFY_REMOVED_DEVICE, as we don't expect any other
+	 * threads to have a reference to the device at that point. Keep it
+	 * because this isn't a performance path and helps lockdep analysis.
+	 */
+	device_lock(dev);
+	group = dev->iommu_group;
 	if (group)
 		__iommu_group_remove_device(dev);
 
 	/* Free any fwspec if no iommu_driver was ever attached */
 	if (dev->iommu)
 		dev_iommu_free(dev);
+	device_unlock(dev);
 }
 
 static int __init iommu_set_def_domain_type(char *str)
@@ -1081,6 +1093,8 @@ static struct group_device *iommu_group_alloc_device(struct iommu_group *group,
 	int ret, i = 0;
 	struct group_device *device;
 
+	device_lock_assert(dev);
+
 	device = kzalloc(sizeof(*device), GFP_KERNEL);
 	if (!device)
 		return ERR_PTR(-ENOMEM);
@@ -1142,9 +1156,12 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)
 {
 	struct group_device *gdev;
 
+	device_lock(dev);
 	gdev = iommu_group_alloc_device(group, dev);
-	if (IS_ERR(gdev))
+	if (IS_ERR(gdev)) {
+		device_unlock(dev);
 		return PTR_ERR(gdev);
+	}
 
 	iommu_group_ref_get(group);
 	dev->iommu_group = group;
@@ -1152,6 +1169,7 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)
 	mutex_lock(&group->mutex);
 	list_add_tail(&gdev->list, &group->devices);
 	mutex_unlock(&group->mutex);
+	device_unlock(dev);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(iommu_group_add_device);
@@ -1165,14 +1183,16 @@ EXPORT_SYMBOL_GPL(iommu_group_add_device);
  */
 void iommu_group_remove_device(struct device *dev)
 {
-	struct iommu_group *group = dev->iommu_group;
+	struct iommu_group *group;
 
-	if (!group)
-		return;
+	device_lock(dev);
+	group = dev->iommu_group;
+	if (group) {
+		dev_info(dev, "Removing from iommu group %d\n", group->id);
+		__iommu_group_remove_device(dev);
+	}
+	device_unlock(dev);
 
-	dev_info(dev, "Removing from iommu group %d\n", group->id);
-
-	__iommu_group_remove_device(dev);
 }
 EXPORT_SYMBOL_GPL(iommu_group_remove_device);
 
-- 
2.41.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v2 09/10] iommu: Complete the locking for dev->iommu_group

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Tuesday, August 1, 2023 1:51 AM
> 
> Revise the locking for dev->iommu_group so that it has three safe ways to
> access it:
> 
>  - It is read by a probe'd device driver. So long as a device driver is
>    probed the dev->iommu_group will be guaranteed stable without further
>    locking.
> 
>  - Read under the device_lock(), this primarily protects against
>    parallel probe of the same device, and parallel probe/remove
> 
>  - Read/Write under the global dev_iommu_group_lock. This is used during
>    probe time discovery of groups. Device drivers will scan unlocked
>    portions of the device tree to locate an already existing group. These
>    scans can access the dev->iommu_group under the global lock to single
>    thread determining and installing the group. This ensures that groups
>    are reliably formed.
> 
> Narrow the scope of the global dev_iommu_group_lock to be only during the
> dev->iommu_group setup, and not for the entire probing.
> 
> Prior patches removed the various races inherent to the probe process by
> consolidating all the work under the group->mutex. In this configuration
> it is fine if two devices race to the group_device step of a new
> iommu_group, the group->mutex locking will ensure the group_device and
> domain setup part remains properly ordered.
> 
> Add the missing locking on the remove paths. For iommu_deinit_device() it
> is necessary to hold the dev_iommu_group_lock due to possible races during
> probe error unwind.
> 
> Fully lock the iommu_group_add/remove_device() path so we can use
> lockdep
> assertions. Other than lockdep this is redundant, VFIO no-iommu doesn't
> use group clustering.
> 
> For iommu_release_device() it is redundant, as we expect no external
> references to the struct device by this point, but it is harmless so
> add the missing lock to allow lockdep assertions to work.
> 
> This resolves the remarks of the comment in __iommu_probe_device().
> 
> Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 9/10] iommu: Complete the locking for dev->iommu_group

[Konrad Dybcio]

On 31.07.2023 19:50, Jason Gunthorpe wrote:
> Revise the locking for dev->iommu_group so that it has three safe ways to
> access it:
> 
>  - It is read by a probe'd device driver. So long as a device driver is
>    probed the dev->iommu_group will be guaranteed stable without further
>    locking.
> 
>  - Read under the device_lock(), this primarily protects against
>    parallel probe of the same device, and parallel probe/remove
> 
>  - Read/Write under the global dev_iommu_group_lock. This is used during
>    probe time discovery of groups. Device drivers will scan unlocked
>    portions of the device tree to locate an already existing group. These
>    scans can access the dev->iommu_group under the global lock to single
>    thread determining and installing the group. This ensures that groups
>    are reliably formed.
> 
> Narrow the scope of the global dev_iommu_group_lock to be only during the
> dev->iommu_group setup, and not for the entire probing.
> 
> Prior patches removed the various races inherent to the probe process by
> consolidating all the work under the group->mutex. In this configuration
> it is fine if two devices race to the group_device step of a new
> iommu_group, the group->mutex locking will ensure the group_device and
> domain setup part remains properly ordered.
> 
> Add the missing locking on the remove paths. For iommu_deinit_device() it
> is necessary to hold the dev_iommu_group_lock due to possible races during
> probe error unwind.
> 
> Fully lock the iommu_group_add/remove_device() path so we can use lockdep
> assertions. Other than lockdep this is redundant, VFIO no-iommu doesn't
> use group clustering.
> 
> For iommu_release_device() it is redundant, as we expect no external
> references to the struct device by this point, but it is harmless so
> add the missing lock to allow lockdep assertions to work.
> 
> This resolves the remarks of the comment in __iommu_probe_device().
> 
> Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> ---
Hello, this patch breaks booting on at least one Qualcomm platform using
the SMMUv2 driver w/ qcom impl. The board hangs right after SMMU probe.

Reverting it makes the platform boot again.

Konrad

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 10/10] iommu/intel: Fix missing locking for show_device_domain_translation()

[Jason Gunthorpe]

This is called from bus_for_each_dev() and must lock the device before
calling iommu_group_get().

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/intel/debugfs.c | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/drivers/iommu/intel/debugfs.c b/drivers/iommu/intel/debugfs.c
index 1f925285104eee..0255c4a2326931 100644
--- a/drivers/iommu/intel/debugfs.c
+++ b/drivers/iommu/intel/debugfs.c
@@ -365,23 +365,25 @@ static int show_device_domain_translation(struct device *dev, void *data)
 {
 	struct iommu_group *group;
 
+	device_lock(dev);
 	group = iommu_group_get(dev);
-	if (group) {
-		/*
-		 * The group->mutex is held across the callback, which will
-		 * block calls to iommu_attach/detach_group/device. Hence,
-		 * the domain of the device will not change during traversal.
-		 *
-		 * All devices in an iommu group share a single domain, hence
-		 * we only dump the domain of the first device. Even though,
-		 * this code still possibly races with the iommu_unmap()
-		 * interface. This could be solved by RCU-freeing the page
-		 * table pages in the iommu_unmap() path.
-		 */
-		iommu_group_for_each_dev(group, data,
-					 __show_device_domain_translation);
-		iommu_group_put(group);
-	}
+	device_unlock(dev);
+	if (!group)
+		return 0;
+
+	/*
+	 * The group->mutex is held across the callback, which will
+	 * block calls to iommu_attach/detach_group/device. Hence,
+	 * the domain of the device will not change during traversal.
+	 *
+	 * All devices in an iommu group share a single domain, hence
+	 * we only dump the domain of the first device. Even though,
+	 * this code still possibly races with the iommu_unmap()
+	 * interface. This could be solved by RCU-freeing the page
+	 * table pages in the iommu_unmap() path.
+	 */
+	iommu_group_for_each_dev(group, data, __show_device_domain_translation);
+	iommu_group_put(group);
 
 	return 0;
 }
-- 
2.41.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v2 10/10] iommu/intel: Fix missing locking for show_device_domain_translation()

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Tuesday, August 1, 2023 1:51 AM
> 
> This is called from bus_for_each_dev() and must lock the device before
> calling iommu_group_get().
> 
> Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 00/10] Refine the locking for dev->iommu_group

[Joerg Roedel]

On Mon, Jul 31, 2023 at 02:50:23PM -0300, Jason Gunthorpe wrote:
> Jason Gunthorpe (10):
>   iommu: Remove useless group refcounting
>   iommu: Add a lockdep assertion for remaining dev->iommu_group reads
>   iommu: Add generic_single_device_group()
>   iommu/sun50i: Convert to generic_single_device_group()
>   iommu/sprd: Convert to generic_single_device_group()
>   iommu/rockchip: Convert to generic_single_device_group()
>   iommu/ipmmu-vmsa: Convert to generic_single_device_group()
>   iommu/omap: Convert to generic_single_device_group()
>   iommu: Complete the locking for dev->iommu_group
>   iommu/intel: Fix missing locking for show_device_domain_translation()
> 
>  drivers/iommu/intel/debugfs.c  |  34 ++++----
>  drivers/iommu/iommu.c          | 155 +++++++++++++++++++++------------
>  drivers/iommu/ipmmu-vmsa.c     |  22 ++---
>  drivers/iommu/omap-iommu.c     |  30 +------
>  drivers/iommu/omap-iommu.h     |   2 +-
>  drivers/iommu/rockchip-iommu.c |  22 +----
>  drivers/iommu/sprd-iommu.c     |  24 +----
>  drivers/iommu/sun50i-iommu.c   |  29 ++----
>  include/linux/iommu.h          |   3 +
>  9 files changed, 138 insertions(+), 183 deletions(-)

Applied, thanks for the nice cleanup!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 00/10] Refine the locking for dev->iommu_group

[Chen-Yu Tsai]

Hi,

On Mon, Aug 7, 2023 at 8:54 PM Joerg Roedel <joro@8bytes.org> wrote:
>
> On Mon, Jul 31, 2023 at 02:50:23PM -0300, Jason Gunthorpe wrote:
> > Jason Gunthorpe (10):
> >   iommu: Remove useless group refcounting
> >   iommu: Add a lockdep assertion for remaining dev->iommu_group reads
> >   iommu: Add generic_single_device_group()
> >   iommu/sun50i: Convert to generic_single_device_group()
> >   iommu/sprd: Convert to generic_single_device_group()
> >   iommu/rockchip: Convert to generic_single_device_group()
> >   iommu/ipmmu-vmsa: Convert to generic_single_device_group()
> >   iommu/omap: Convert to generic_single_device_group()
> >   iommu: Complete the locking for dev->iommu_group
> >   iommu/intel: Fix missing locking for show_device_domain_translation()
> >
> >  drivers/iommu/intel/debugfs.c  |  34 ++++----
> >  drivers/iommu/iommu.c          | 155 +++++++++++++++++++++------------
> >  drivers/iommu/ipmmu-vmsa.c     |  22 ++---
> >  drivers/iommu/omap-iommu.c     |  30 +------
> >  drivers/iommu/omap-iommu.h     |   2 +-
> >  drivers/iommu/rockchip-iommu.c |  22 +----
> >  drivers/iommu/sprd-iommu.c     |  24 +----
> >  drivers/iommu/sun50i-iommu.c   |  29 ++----
> >  include/linux/iommu.h          |   3 +
> >  9 files changed, 138 insertions(+), 183 deletions(-)
>
> Applied, thanks for the nice cleanup!

This series seems to cause a hung task during boot on MediaTek platforms.
It hangs with next-20230808. Reverting the 10 commits from this series
makes the system boot up again.

ChenYu

Logs follow.

INFO: task swapper/0:1 blocked for more than 122 seconds.
      Not tainted 6.5.0-rc5-next-20230808-08004-g396bbe23dbf4 #859
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
task:swapper/0       state:D stack:0     pid:1     ppid:0      flags:0x00000008
Call trace:
 __switch_to+0x138/0x1e8
 __schedule+0x728/0x1388
 schedule+0xa8/0x170
 schedule_timeout+0x19c/0x1b8
 __wait_for_common+0x250/0x2c0
 wait_for_completion+0x28/0x40
 __flush_work+0x37c/0x6c0
 flush_work+0x1c/0x30
 deferred_probe_initcall+0x60/0xd0
 do_one_initcall+0xe0/0x4a0
 kernel_init_freeable+0x3a4/0x730
 kernel_init+0x2c/0x1f8
 ret_from_fork+0x10/0x20
INFO: task kworker/u18:1:67 blocked for more than 122 seconds.
      Not tainted 6.5.0-rc5-next-20230808-08004-g396bbe23dbf4 #859
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
task:kworker/u18:1   state:D stack:0     pid:67    ppid:2      flags:0x00000008
Workqueue: events_unbound deferred_probe_work_func
Call trace:
 __switch_to+0x138/0x1e8
 __schedule+0x728/0x1388
 schedule+0xa8/0x170
 schedule_preempt_disabled+0x44/0x80
 __mutex_lock+0x3fc/0x598
 mutex_lock_nested+0x2c/0x40
 __iommu_probe_device+0xb8/0x6e0
 probe_iommu_group+0x18/0x38
 bus_for_each_dev+0xe4/0x168
 bus_iommu_probe+0x8c/0x240
 iommu_device_register+0x120/0x1b0
 mtk_iommu_probe+0x494/0x7a0
 platform_probe+0x94/0x100
 really_probe+0x1e4/0x3e8
 __driver_probe_device+0xc0/0x1a0
 driver_probe_device+0x110/0x1f0
 __device_attach_driver+0xf0/0x1b0
 bus_for_each_drv+0xf0/0x170
 __device_attach+0x120/0x240
 device_initial_probe+0x1c/0x30
 bus_probe_device+0xdc/0xe8
 deferred_probe_work_func+0xf0/0x140
 process_one_work+0x3b0/0x910
 worker_thread+0x33c/0x610
 kthread+0x1dc/0x1f0
 ret_from_fork+0x10/0x20

Showing all locks held in the system:
4 locks held by kworker/u18:1/67:
 #0: ffffff80c001b538 ((wq_completion)events_unbound){+.+.}-{0:0}, at:
process_one_work+0x2c4/0x910
 #1: ffffffc080777d40 (deferred_probe_work){+.+.}-{0:0}, at:
process_one_work+0x2c4/0x910
 #2: ffffff80c14090f8 (&dev->mutex){....}-{3:3}, at: __device_attach+0x8c/0x240
 #3: ffffff80c14090f8 (&dev->mutex){....}-{3:3}, at:
__iommu_probe_device+0xb8/0x6e0
1 lock held by khungtaskd/70:
 #0: ffffffe379f945a0 (rcu_read_lock){....}-{1:2}, at:
debug_show_all_locks+0x24/0x220

=============================================

Kernel panic - not syncing: hung_task: blocked tasks
CPU: 4 PID: 70 Comm: khungtaskd Not tainted
6.5.0-rc5-next-20230808-08004-g396bbe23dbf4 #859
55426859c267064a381312eb869e94c28566a87f
Hardware name: Google juniper sku16 board (DT)
Call trace:
 dump_backtrace+0xa0/0x100
 show_stack+0x20/0x38
 dump_stack_lvl+0xdc/0x148
 dump_stack+0x1c/0x28
 panic+0x460/0x4d8
 watchdog+0x4a4/0x9d0
 kthread+0x1dc/0x1f0
 ret_from_fork+0x10/0x20
SMP: stopping secondary CPUs
Kernel Offset: 0x22f7000000 from 0xffffffc080000000
PHYS_OFFSET: 0x40000000
CPU features: 0x0000000c,92010000,0800421b
Memory Limit: none
Rebooting in 30 seconds..

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 00/10] Refine the locking for dev->iommu_group

[Jason Gunthorpe]

On Tue, Aug 08, 2023 at 06:31:47PM +0800, Chen-Yu Tsai wrote:
> INFO: task kworker/u18:1:67 blocked for more than 122 seconds.
>       Not tainted 6.5.0-rc5-next-20230808-08004-g396bbe23dbf4 #859
> "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> task:kworker/u18:1   state:D stack:0     pid:67    ppid:2      flags:0x00000008
> Workqueue: events_unbound deferred_probe_work_func
> Call trace:
>  __switch_to+0x138/0x1e8
>  __schedule+0x728/0x1388
>  schedule+0xa8/0x170
>  schedule_preempt_disabled+0x44/0x80
>  __mutex_lock+0x3fc/0x598
>  mutex_lock_nested+0x2c/0x40
>  __iommu_probe_device+0xb8/0x6e0
>  probe_iommu_group+0x18/0x38
>  bus_for_each_dev+0xe4/0x168
>  bus_iommu_probe+0x8c/0x240
>  iommu_device_register+0x120/0x1b0
>  mtk_iommu_probe+0x494/0x7a0
>  platform_probe+0x94/0x100
>  really_probe+0x1e4/0x3e8
>  __driver_probe_device+0xc0/0x1a0
>  driver_probe_device+0x110/0x1f0
>  __device_attach_driver+0xf0/0x1b0
>  bus_for_each_drv+0xf0/0x170
>  __device_attach+0x120/0x240
>  device_initial_probe+0x1c/0x30
>  bus_probe_device+0xdc/0xe8
>  deferred_probe_work_func+0xf0/0x140
>  process_one_work+0x3b0/0x910
>  worker_thread+0x33c/0x610
>  kthread+0x1dc/0x1f0
>  ret_from_fork+0x10/0x20

Oh weird, I wonder why this didn't show up on my testing? It is trying
to probe the iommu device itself against the iommu and deadlocks on
the device_lock (which is held during probe):

> process_one_work+0x2c4/0x910
>  #2: ffffff80c14090f8 (&dev->mutex){....}-{3:3}, at: __device_attach+0x8c/0x240
>  #3: ffffff80c14090f8 (&dev->mutex){....}-{3:3}, at:

I suppose the easy fix is to just exclude the iommu driver from
probing, it doesn't have an iommu usually anyhow.

Does this work for you?

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 7dbbcffac21930..faa0e3520f66b0 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -273,7 +273,7 @@ int iommu_device_register(struct iommu_device *iommu,
 
 	for (int i = 0; i < ARRAY_SIZE(iommu_buses) && !err; i++) {
 		iommu_buses[i]->iommu_ops = ops;
-		err = bus_iommu_probe(iommu_buses[i]);
+		err = bus_iommu_probe(iommu_buses[i], iommu);
 	}
 	if (err)
 		iommu_device_unregister(iommu);
@@ -1784,12 +1784,21 @@ struct iommu_domain *iommu_group_default_domain(struct iommu_group *group)
 	return group->default_domain;
 }
 
+struct probe_iommu_args {
+	struct list_head *group_list;
+	struct iommu_device *iommu;
+};
+
 static int probe_iommu_group(struct device *dev, void *data)
 {
-	struct list_head *group_list = data;
+	struct probe_iommu_args *args = data;
 	int ret;
 
-	ret = __iommu_probe_device(dev, group_list);
+	/* We never probe the iommu device itself */
+	if (args->iommu && args->iommu->dev == dev)
+		return 0;
+
+	ret = __iommu_probe_device(dev, args->group_list);
 	if (ret == -ENODEV)
 		ret = 0;
 
@@ -1858,13 +1867,16 @@ static void iommu_group_do_probe_finalize(struct device *dev)
 		ops->probe_finalize(dev);
 }
 
-int bus_iommu_probe(const struct bus_type *bus)
+int bus_iommu_probe(const struct bus_type *bus, struct iommu_device *iommu)
 {
 	struct iommu_group *group, *next;
+	struct probe_iommu_args args = {};
 	LIST_HEAD(group_list);
 	int ret;
 
-	ret = bus_for_each_dev(bus, NULL, &group_list, probe_iommu_group);
+	args.group_list = &group_list;
+	args.iommu = iommu;
+	ret = bus_for_each_dev(bus, NULL, &args, probe_iommu_group);
 	if (ret)
 		return ret;
 
diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index 97c45f50bf4332..e8ab7cb832ba37 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1234,6 +1234,9 @@ static int omap_iommu_probe(struct platform_device *pdev)
 		if (err)
 			goto out_sysfs;
 		obj->has_iommu_driver = true;
+	} else {
+		/* Re-probe bus to probe device attached to this IOMMU */
+		bus_iommu_probe(&platform_bus_type, NULL);
 	}
 
 	pm_runtime_enable(obj->dev);
@@ -1242,9 +1245,6 @@ static int omap_iommu_probe(struct platform_device *pdev)
 
 	dev_info(&pdev->dev, "%s registered\n", obj->name);
 
-	/* Re-probe bus to probe device attached to this IOMMU */
-	bus_iommu_probe(&platform_bus_type);
-
 	return 0;
 
 out_sysfs:
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index f1e18e81fca78b..8e10225e0d611a 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -465,7 +465,8 @@ static inline const struct iommu_ops *dev_iommu_ops(struct device *dev)
 	return dev->iommu->iommu_dev->ops;
 }
 
-extern int bus_iommu_probe(const struct bus_type *bus);
+extern int bus_iommu_probe(const struct bus_type *bus,
+			   struct iommu_device *iommu);
 extern bool iommu_present(const struct bus_type *bus);
 extern bool device_iommu_capable(struct device *dev, enum iommu_cap cap);
 extern bool iommu_group_has_isolated_msi(struct iommu_group *group);

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 00/10] Refine the locking for dev->iommu_group

[Marek Szyprowski]

Hi,

On 08.08.2023 12:31, Chen-Yu Tsai wrote:
> Hi,
>
> On Mon, Aug 7, 2023 at 8:54 PM Joerg Roedel <joro@8bytes.org> wrote:
>> On Mon, Jul 31, 2023 at 02:50:23PM -0300, Jason Gunthorpe wrote:
>>> Jason Gunthorpe (10):
>>>    iommu: Remove useless group refcounting
>>>    iommu: Add a lockdep assertion for remaining dev->iommu_group reads
>>>    iommu: Add generic_single_device_group()
>>>    iommu/sun50i: Convert to generic_single_device_group()
>>>    iommu/sprd: Convert to generic_single_device_group()
>>>    iommu/rockchip: Convert to generic_single_device_group()
>>>    iommu/ipmmu-vmsa: Convert to generic_single_device_group()
>>>    iommu/omap: Convert to generic_single_device_group()
>>>    iommu: Complete the locking for dev->iommu_group
>>>    iommu/intel: Fix missing locking for show_device_domain_translation()
>>>
>>>   drivers/iommu/intel/debugfs.c  |  34 ++++----
>>>   drivers/iommu/iommu.c          | 155 +++++++++++++++++++++------------
>>>   drivers/iommu/ipmmu-vmsa.c     |  22 ++---
>>>   drivers/iommu/omap-iommu.c     |  30 +------
>>>   drivers/iommu/omap-iommu.h     |   2 +-
>>>   drivers/iommu/rockchip-iommu.c |  22 +----
>>>   drivers/iommu/sprd-iommu.c     |  24 +----
>>>   drivers/iommu/sun50i-iommu.c   |  29 ++----
>>>   include/linux/iommu.h          |   3 +
>>>   9 files changed, 138 insertions(+), 183 deletions(-)
>> Applied, thanks for the nice cleanup!
> This series seems to cause a hung task during boot on MediaTek platforms.
> It hangs with next-20230808. Reverting the 10 commits from this series
> makes the system boot up again.

I confirm that next-20230808 is broken on ARM 32bit based Exynos boards 
too. Boards lock up very early during boot. I will try to investigate 
this soon.


Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 00/10] Refine the locking for dev->iommu_group

[Jason Gunthorpe]

On Tue, Aug 08, 2023 at 02:32:48PM +0200, Marek Szyprowski wrote:
> Hi,
> 
> On 08.08.2023 12:31, Chen-Yu Tsai wrote:
> > Hi,
> >
> > On Mon, Aug 7, 2023 at 8:54 PM Joerg Roedel <joro@8bytes.org> wrote:
> >> On Mon, Jul 31, 2023 at 02:50:23PM -0300, Jason Gunthorpe wrote:
> >>> Jason Gunthorpe (10):
> >>>    iommu: Remove useless group refcounting
> >>>    iommu: Add a lockdep assertion for remaining dev->iommu_group reads
> >>>    iommu: Add generic_single_device_group()
> >>>    iommu/sun50i: Convert to generic_single_device_group()
> >>>    iommu/sprd: Convert to generic_single_device_group()
> >>>    iommu/rockchip: Convert to generic_single_device_group()
> >>>    iommu/ipmmu-vmsa: Convert to generic_single_device_group()
> >>>    iommu/omap: Convert to generic_single_device_group()
> >>>    iommu: Complete the locking for dev->iommu_group
> >>>    iommu/intel: Fix missing locking for show_device_domain_translation()
> >>>
> >>>   drivers/iommu/intel/debugfs.c  |  34 ++++----
> >>>   drivers/iommu/iommu.c          | 155 +++++++++++++++++++++------------
> >>>   drivers/iommu/ipmmu-vmsa.c     |  22 ++---
> >>>   drivers/iommu/omap-iommu.c     |  30 +------
> >>>   drivers/iommu/omap-iommu.h     |   2 +-
> >>>   drivers/iommu/rockchip-iommu.c |  22 +----
> >>>   drivers/iommu/sprd-iommu.c     |  24 +----
> >>>   drivers/iommu/sun50i-iommu.c   |  29 ++----
> >>>   include/linux/iommu.h          |   3 +
> >>>   9 files changed, 138 insertions(+), 183 deletions(-)
> >> Applied, thanks for the nice cleanup!
> > This series seems to cause a hung task during boot on MediaTek platforms.
> > It hangs with next-20230808. Reverting the 10 commits from this series
> > makes the system boot up again.
> 
> I confirm that next-20230808 is broken on ARM 32bit based Exynos boards 
> too. Boards lock up very early during boot. I will try to investigate 
> this soon.

Any of the drivers that use platform device as the iommu_device will
have a problem, please try:

https://lore.kernel.org/linux-iommu/ZNIz%2FNVLb6WqqvQx@nvidia.com/

Jason


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 00/10] Refine the locking for dev->iommu_group

[Marek Szyprowski]

Hi Jason,

On 08.08.2023 15:00, Jason Gunthorpe wrote:
> On Tue, Aug 08, 2023 at 02:32:48PM +0200, Marek Szyprowski wrote:
>> On 08.08.2023 12:31, Chen-Yu Tsai wrote:
>>> On Mon, Aug 7, 2023 at 8:54 PM Joerg Roedel <joro@8bytes.org> wrote:
>>>> On Mon, Jul 31, 2023 at 02:50:23PM -0300, Jason Gunthorpe wrote:
>>>>> Jason Gunthorpe (10):
>>>>>     iommu: Remove useless group refcounting
>>>>>     iommu: Add a lockdep assertion for remaining dev->iommu_group reads
>>>>>     iommu: Add generic_single_device_group()
>>>>>     iommu/sun50i: Convert to generic_single_device_group()
>>>>>     iommu/sprd: Convert to generic_single_device_group()
>>>>>     iommu/rockchip: Convert to generic_single_device_group()
>>>>>     iommu/ipmmu-vmsa: Convert to generic_single_device_group()
>>>>>     iommu/omap: Convert to generic_single_device_group()
>>>>>     iommu: Complete the locking for dev->iommu_group
>>>>>     iommu/intel: Fix missing locking for show_device_domain_translation()
>>>>>
>>>>>    drivers/iommu/intel/debugfs.c  |  34 ++++----
>>>>>    drivers/iommu/iommu.c          | 155 +++++++++++++++++++++------------
>>>>>    drivers/iommu/ipmmu-vmsa.c     |  22 ++---
>>>>>    drivers/iommu/omap-iommu.c     |  30 +------
>>>>>    drivers/iommu/omap-iommu.h     |   2 +-
>>>>>    drivers/iommu/rockchip-iommu.c |  22 +----
>>>>>    drivers/iommu/sprd-iommu.c     |  24 +----
>>>>>    drivers/iommu/sun50i-iommu.c   |  29 ++----
>>>>>    include/linux/iommu.h          |   3 +
>>>>>    9 files changed, 138 insertions(+), 183 deletions(-)
>>>> Applied, thanks for the nice cleanup!
>>> This series seems to cause a hung task during boot on MediaTek platforms.
>>> It hangs with next-20230808. Reverting the 10 commits from this series
>>> makes the system boot up again.
>> I confirm that next-20230808 is broken on ARM 32bit based Exynos boards
>> too. Boards lock up very early during boot. I will try to investigate
>> this soon.
> Any of the drivers that use platform device as the iommu_device will
> have a problem, please try:
>
> https://lore.kernel.org/linux-iommu/ZNIz%2FNVLb6WqqvQx@nvidia.com/

I've checked and it doesn't help in my case. I will soon check why.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 00/10] Refine the locking for dev->iommu_group

[Jason Gunthorpe]

On Tue, Aug 08, 2023 at 03:08:30PM +0200, Marek Szyprowski wrote:
> > Any of the drivers that use platform device as the iommu_device will
> > have a problem, please try:
> >
> > https://lore.kernel.org/linux-iommu/ZNIz%2FNVLb6WqqvQx@nvidia.com/
> 
> I've checked and it doesn't help in my case. I will soon check why.

Oh, I botched it. Forgot that the iommu_device->dev is the sysfs
handle not the HW device. Maybe this:

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 7dbbcffac21930..dbaace482861da 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -264,6 +264,7 @@ int iommu_device_register(struct iommu_device *iommu,
 		return -EBUSY;
 
 	iommu->ops = ops;
+	iommu->hwdev = hwdev;
 	if (hwdev)
 		iommu->fwnode = dev_fwnode(hwdev);
 
@@ -273,7 +274,7 @@ int iommu_device_register(struct iommu_device *iommu,
 
 	for (int i = 0; i < ARRAY_SIZE(iommu_buses) && !err; i++) {
 		iommu_buses[i]->iommu_ops = ops;
-		err = bus_iommu_probe(iommu_buses[i]);
+		err = bus_iommu_probe(iommu_buses[i], iommu);
 	}
 	if (err)
 		iommu_device_unregister(iommu);
@@ -1784,12 +1785,21 @@ struct iommu_domain *iommu_group_default_domain(struct iommu_group *group)
 	return group->default_domain;
 }
 
+struct probe_iommu_args {
+	struct list_head *group_list;
+	struct iommu_device *iommu;
+};
+
 static int probe_iommu_group(struct device *dev, void *data)
 {
-	struct list_head *group_list = data;
+	struct probe_iommu_args *args = data;
 	int ret;
 
-	ret = __iommu_probe_device(dev, group_list);
+	/* We never probe the iommu device itself */
+	if (args->iommu && args->iommu->hwdev == dev)
+		return 0;
+
+	ret = __iommu_probe_device(dev, args->group_list);
 	if (ret == -ENODEV)
 		ret = 0;
 
@@ -1858,13 +1868,16 @@ static void iommu_group_do_probe_finalize(struct device *dev)
 		ops->probe_finalize(dev);
 }
 
-int bus_iommu_probe(const struct bus_type *bus)
+int bus_iommu_probe(const struct bus_type *bus, struct iommu_device *iommu)
 {
 	struct iommu_group *group, *next;
+	struct probe_iommu_args args = {};
 	LIST_HEAD(group_list);
 	int ret;
 
-	ret = bus_for_each_dev(bus, NULL, &group_list, probe_iommu_group);
+	args.group_list = &group_list;
+	args.iommu = iommu;
+	ret = bus_for_each_dev(bus, NULL, &args, probe_iommu_group);
 	if (ret)
 		return ret;
 
diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index 97c45f50bf4332..e8ab7cb832ba37 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1234,6 +1234,9 @@ static int omap_iommu_probe(struct platform_device *pdev)
 		if (err)
 			goto out_sysfs;
 		obj->has_iommu_driver = true;
+	} else {
+		/* Re-probe bus to probe device attached to this IOMMU */
+		bus_iommu_probe(&platform_bus_type, NULL);
 	}
 
 	pm_runtime_enable(obj->dev);
@@ -1242,9 +1245,6 @@ static int omap_iommu_probe(struct platform_device *pdev)
 
 	dev_info(&pdev->dev, "%s registered\n", obj->name);
 
-	/* Re-probe bus to probe device attached to this IOMMU */
-	bus_iommu_probe(&platform_bus_type);
-
 	return 0;
 
 out_sysfs:
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index f1e18e81fca78b..784e0fd91fd17e 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -361,6 +361,7 @@ struct iommu_domain_ops {
  * @list: Used by the iommu-core to keep a list of registered iommus
  * @ops: iommu-ops for talking to this iommu
  * @dev: struct device for sysfs handling
+ * @hwdev: The device HW that controls the iommu
  * @singleton_group: Used internally for drivers that have only one group
  * @max_pasids: number of supported PASIDs
  */
@@ -369,6 +370,7 @@ struct iommu_device {
 	const struct iommu_ops *ops;
 	struct fwnode_handle *fwnode;
 	struct device *dev;
+	struct device *hwdev;
 	struct iommu_group *singleton_group;
 	u32 max_pasids;
 };
@@ -465,7 +467,8 @@ static inline const struct iommu_ops *dev_iommu_ops(struct device *dev)
 	return dev->iommu->iommu_dev->ops;
 }
 
-extern int bus_iommu_probe(const struct bus_type *bus);
+extern int bus_iommu_probe(const struct bus_type *bus,
+			   struct iommu_device *iommu);
 extern bool iommu_present(const struct bus_type *bus);
 extern bool device_iommu_capable(struct device *dev, enum iommu_cap cap);
 extern bool iommu_group_has_isolated_msi(struct iommu_group *group);

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 00/10] Refine the locking for dev->iommu_group

[Marek Szyprowski]

Hi Jason,

On 08.08.2023 15:25, Jason Gunthorpe wrote:
> On Tue, Aug 08, 2023 at 03:08:30PM +0200, Marek Szyprowski wrote:
>>> Any of the drivers that use platform device as the iommu_device will
>>> have a problem, please try:
>>>
>>> https://lore.kernel.org/linux-iommu/ZNIz%2FNVLb6WqqvQx@nvidia.com/
>> I've checked and it doesn't help in my case. I will soon check why.
> Oh, I botched it. Forgot that the iommu_device->dev is the sysfs
> handle not the HW device. Maybe this:

This fixed the early lockup, but then system hangs again a bit later. It 
looks that this device lock in __iommu_probe_device() is really 
problematic, because __iommu_probe_device() is then called during the 
iommu 'client device' probe on the probed device. Here is a complete 
call stack:

  unwind_backtrace from show_stack+0x10/0x14
  show_stack from dump_stack_lvl+0x58/0x70
  dump_stack_lvl from __iommu_probe_device+0x3d8/0x4b8
  __iommu_probe_device from iommu_probe_device+0x10/0x40
  iommu_probe_device from of_iommu_configure+0xf8/0x1c8
  of_iommu_configure from of_dma_configure_id+0x188/0x450
  of_dma_configure_id from platform_dma_configure+0x24/0x60
  platform_dma_configure from really_probe+0xac/0x3d4
  really_probe from __driver_probe_device+0xa0/0x1e8
  __driver_probe_device from driver_probe_device+0x30/0xd0
  driver_probe_device from __driver_attach+0x10c/0x190
  __driver_attach from bus_for_each_dev+0x60/0xb4
  bus_for_each_dev from bus_add_driver+0xe0/0x208
  bus_add_driver from driver_register+0x7c/0x118
  driver_register from exynos_drm_init+0xe0/0x14c
  exynos_drm_init from do_one_initcall+0x6c/0x318
  do_one_initcall from kernel_init_freeable+0x1c4/0x214
  kernel_init_freeable from kernel_init+0x18/0x12c
  kernel_init from ret_from_fork+0x14/0x2c

Any ideas?

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 00/10] Refine the locking for dev->iommu_group

[Jason Gunthorpe]

On Tue, Aug 08, 2023 at 04:02:40PM +0200, Marek Szyprowski wrote:
> Hi Jason,
> 
> On 08.08.2023 15:25, Jason Gunthorpe wrote:
> > On Tue, Aug 08, 2023 at 03:08:30PM +0200, Marek Szyprowski wrote:
> >>> Any of the drivers that use platform device as the iommu_device will
> >>> have a problem, please try:
> >>>
> >>> https://lore.kernel.org/linux-iommu/ZNIz%2FNVLb6WqqvQx@nvidia.com/
> >> I've checked and it doesn't help in my case. I will soon check why.
> > Oh, I botched it. Forgot that the iommu_device->dev is the sysfs
> > handle not the HW device. Maybe this:
> 
> This fixed the early lockup, but then system hangs again a bit later. It 
> looks that this device lock in __iommu_probe_device() is really
> problematic, 

Yes, I expected we'd hit something like this - I checked alot of call
paths but missed these two. The self-probe is sneaky, but here the
device_lock is held way up the call chain, I just missed it.

The fix is to just annotate that we already hold the lock when calling
iommu_probe_device(), since we know in those cases that we must be
holding it:

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index daa64dd687524b..3fc5e12f2f1c09 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1582,7 +1582,7 @@ static const struct iommu_ops *acpi_iommu_configure_id(struct device *dev,
 	 * iommu_probe_device() call for dev, replay it to get things in order.
 	 */
 	if (!err && dev->bus)
-		err = iommu_probe_device(dev);
+		err = iommu_probe_device_locked(dev);
 
 	/* Ignore all other errors apart from EPROBE_DEFER */
 	if (err == -EPROBE_DEFER) {
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 7dbbcffac21930..b867d7f22954e9 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -264,6 +264,7 @@ int iommu_device_register(struct iommu_device *iommu,
 		return -EBUSY;
 
 	iommu->ops = ops;
+	iommu->hwdev = hwdev;
 	if (hwdev)
 		iommu->fwnode = dev_fwnode(hwdev);
 
@@ -273,7 +274,7 @@ int iommu_device_register(struct iommu_device *iommu,
 
 	for (int i = 0; i < ARRAY_SIZE(iommu_buses) && !err; i++) {
 		iommu_buses[i]->iommu_ops = ops;
-		err = bus_iommu_probe(iommu_buses[i]);
+		err = bus_iommu_probe(iommu_buses[i], iommu);
 	}
 	if (err)
 		iommu_device_unregister(iommu);
@@ -452,24 +453,23 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
 	struct group_device *gdev;
 	int ret;
 
-	if (!ops)
-		return -ENODEV;
 	/*
 	 * Allow __iommu_probe_device() to be safely called in parallel,
 	 * both dev->iommu_group and the initial setup of dev->iommu are
 	 * protected this way.
 	 */
-	device_lock(dev);
+	device_lock_assert(dev);
+
+	if (!ops)
+		return -ENODEV;
 
 	/* Device is probed already if in a group */
-	if (dev->iommu_group) {
-		ret = 0;
-		goto out_unlock;
-	}
+	if (dev->iommu_group)
+		return 0;
 
 	ret = iommu_init_device(dev, ops);
 	if (ret)
-		goto out_unlock;
+		return ret;
 
 	group = dev->iommu_group;
 	gdev = iommu_group_alloc_device(group, dev);
@@ -505,7 +505,6 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
 			list_add_tail(&group->entry, group_list);
 	}
 	mutex_unlock(&group->mutex);
-	device_unlock(dev);
 
 	if (dev_is_pci(dev))
 		iommu_dma_set_pci_32bit_workaround(dev);
@@ -519,12 +518,10 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
 	iommu_deinit_device(dev);
 	mutex_unlock(&group->mutex);
 	iommu_group_put(group);
-out_unlock:
-	device_unlock(dev);
 	return ret;
 }
 
-int iommu_probe_device(struct device *dev)
+int iommu_probe_device_locked(struct device *dev)
 {
 	const struct iommu_ops *ops;
 	int ret;
@@ -540,6 +537,16 @@ int iommu_probe_device(struct device *dev)
 	return 0;
 }
 
+int iommu_probe_device(struct device *dev)
+{
+	int ret;
+
+	device_lock(dev);
+	ret = iommu_probe_device_locked(dev);
+	device_unlock(dev);
+	return ret;
+}
+
 static void __iommu_group_free_device(struct iommu_group *group,
 				      struct group_device *grp_dev)
 {
@@ -1784,12 +1791,26 @@ struct iommu_domain *iommu_group_default_domain(struct iommu_group *group)
 	return group->default_domain;
 }
 
+struct probe_iommu_args {
+	struct list_head *group_list;
+	struct iommu_device *iommu;
+};
+
 static int probe_iommu_group(struct device *dev, void *data)
 {
-	struct list_head *group_list = data;
+	struct probe_iommu_args *args = data;
+	bool need_lock;
 	int ret;
 
-	ret = __iommu_probe_device(dev, group_list);
+	/* Probing the iommu itself is always done under the device_lock */
+	need_lock = !args->iommu || args->iommu->hwdev != dev;
+
+	if (need_lock)
+		device_lock(dev);
+	ret = __iommu_probe_device(dev, args->group_list);
+	if (need_lock)
+		device_unlock(dev);
+
 	if (ret == -ENODEV)
 		ret = 0;
 
@@ -1858,13 +1879,16 @@ static void iommu_group_do_probe_finalize(struct device *dev)
 		ops->probe_finalize(dev);
 }
 
-int bus_iommu_probe(const struct bus_type *bus)
+int bus_iommu_probe(const struct bus_type *bus, struct iommu_device *iommu)
 {
 	struct iommu_group *group, *next;
+	struct probe_iommu_args args = {};
 	LIST_HEAD(group_list);
 	int ret;
 
-	ret = bus_for_each_dev(bus, NULL, &group_list, probe_iommu_group);
+	args.group_list = &group_list;
+	args.iommu = iommu;
+	ret = bus_for_each_dev(bus, NULL, &args, probe_iommu_group);
 	if (ret)
 		return ret;
 
diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index 157b286e36bf3a..b5b7d4bd2cefb9 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -160,7 +160,7 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
 	 * probe for dev, replay it to get things in order.
 	 */
 	if (!err && dev->bus)
-		err = iommu_probe_device(dev);
+		err = iommu_probe_device_locked(dev);
 
 	/* Ignore all other errors apart from EPROBE_DEFER */
 	if (err == -EPROBE_DEFER) {
diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index 97c45f50bf4332..828679abef7503 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1234,6 +1234,10 @@ static int omap_iommu_probe(struct platform_device *pdev)
 		if (err)
 			goto out_sysfs;
 		obj->has_iommu_driver = true;
+	} else {
+		/* Re-probe bus to probe device attached to this IOMMU */
+		obj->iommu.hwdev = &pdev->dev;
+		bus_iommu_probe(&platform_bus_type, &obj->iommu);
 	}
 
 	pm_runtime_enable(obj->dev);
@@ -1242,9 +1246,6 @@ static int omap_iommu_probe(struct platform_device *pdev)
 
 	dev_info(&pdev->dev, "%s registered\n", obj->name);
 
-	/* Re-probe bus to probe device attached to this IOMMU */
-	bus_iommu_probe(&platform_bus_type);
-
 	return 0;
 
 out_sysfs:
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index f1e18e81fca78b..96782bfb384462 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -361,6 +361,7 @@ struct iommu_domain_ops {
  * @list: Used by the iommu-core to keep a list of registered iommus
  * @ops: iommu-ops for talking to this iommu
  * @dev: struct device for sysfs handling
+ * @hwdev: The device HW that controls the iommu
  * @singleton_group: Used internally for drivers that have only one group
  * @max_pasids: number of supported PASIDs
  */
@@ -369,6 +370,7 @@ struct iommu_device {
 	const struct iommu_ops *ops;
 	struct fwnode_handle *fwnode;
 	struct device *dev;
+	struct device *hwdev;
 	struct iommu_group *singleton_group;
 	u32 max_pasids;
 };
@@ -465,7 +467,8 @@ static inline const struct iommu_ops *dev_iommu_ops(struct device *dev)
 	return dev->iommu->iommu_dev->ops;
 }
 
-extern int bus_iommu_probe(const struct bus_type *bus);
+extern int bus_iommu_probe(const struct bus_type *bus,
+			   struct iommu_device *iommu);
 extern bool iommu_present(const struct bus_type *bus);
 extern bool device_iommu_capable(struct device *dev, enum iommu_cap cap);
 extern bool iommu_group_has_isolated_msi(struct iommu_group *group);
@@ -709,6 +712,7 @@ static inline void dev_iommu_priv_set(struct device *dev, void *priv)
 }
 
 int iommu_probe_device(struct device *dev);
+int iommu_probe_device_locked(struct device *dev);
 
 int iommu_dev_enable_feature(struct device *dev, enum iommu_dev_features f);
 int iommu_dev_disable_feature(struct device *dev, enum iommu_dev_features f);



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 00/10] Refine the locking for dev->iommu_group

[Marek Szyprowski]

Hi Jason,

On 08.08.2023 16:30, Jason Gunthorpe wrote:
> On Tue, Aug 08, 2023 at 04:02:40PM +0200, Marek Szyprowski wrote:
>> On 08.08.2023 15:25, Jason Gunthorpe wrote:
>>> On Tue, Aug 08, 2023 at 03:08:30PM +0200, Marek Szyprowski wrote:
>>>>> Any of the drivers that use platform device as the iommu_device will
>>>>> have a problem, please try:
>>>>>
>>>>> https://lore.kernel.org/linux-iommu/ZNIz%2FNVLb6WqqvQx@nvidia.com/
>>>> I've checked and it doesn't help in my case. I will soon check why.
>>> Oh, I botched it. Forgot that the iommu_device->dev is the sysfs
>>> handle not the HW device. Maybe this:
>> This fixed the early lockup, but then system hangs again a bit later. It
>> looks that this device lock in __iommu_probe_device() is really
>> problematic,
> Yes, I expected we'd hit something like this - I checked alot of call
> paths but missed these two. The self-probe is sneaky, but here the
> device_lock is held way up the call chain, I just missed it.
>
> The fix is to just annotate that we already hold the lock when calling
> iommu_probe_device(), since we know in those cases that we must be
> holding it:

Great, this fixed the issue. Feel free to add:

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

 > ...

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 00/10] Refine the locking for dev->iommu_group

[Chen-Yu Tsai]

On Tue, Aug 8, 2023 at 10:30 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> On Tue, Aug 08, 2023 at 04:02:40PM +0200, Marek Szyprowski wrote:
> > Hi Jason,
> >
> > On 08.08.2023 15:25, Jason Gunthorpe wrote:
> > > On Tue, Aug 08, 2023 at 03:08:30PM +0200, Marek Szyprowski wrote:
> > >>> Any of the drivers that use platform device as the iommu_device will
> > >>> have a problem, please try:
> > >>>
> > >>> https://lore.kernel.org/linux-iommu/ZNIz%2FNVLb6WqqvQx@nvidia.com/
> > >> I've checked and it doesn't help in my case. I will soon check why.
> > > Oh, I botched it. Forgot that the iommu_device->dev is the sysfs
> > > handle not the HW device. Maybe this:
> >
> > This fixed the early lockup, but then system hangs again a bit later. It
> > looks that this device lock in __iommu_probe_device() is really
> > problematic,
>
> Yes, I expected we'd hit something like this - I checked alot of call
> paths but missed these two. The self-probe is sneaky, but here the
> device_lock is held way up the call chain, I just missed it.
>
> The fix is to just annotate that we already hold the lock when calling
> iommu_probe_device(), since we know in those cases that we must be
> holding it:

This fixed things for me, so

Tested-by: Chen-Yu Tsai <wenst@chromium.org>

> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index daa64dd687524b..3fc5e12f2f1c09 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -1582,7 +1582,7 @@ static const struct iommu_ops *acpi_iommu_configure_id(struct device *dev,
>          * iommu_probe_device() call for dev, replay it to get things in order.
>          */
>         if (!err && dev->bus)
> -               err = iommu_probe_device(dev);
> +               err = iommu_probe_device_locked(dev);
>
>         /* Ignore all other errors apart from EPROBE_DEFER */
>         if (err == -EPROBE_DEFER) {
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 7dbbcffac21930..b867d7f22954e9 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -264,6 +264,7 @@ int iommu_device_register(struct iommu_device *iommu,
>                 return -EBUSY;
>
>         iommu->ops = ops;
> +       iommu->hwdev = hwdev;
>         if (hwdev)
>                 iommu->fwnode = dev_fwnode(hwdev);
>
> @@ -273,7 +274,7 @@ int iommu_device_register(struct iommu_device *iommu,
>
>         for (int i = 0; i < ARRAY_SIZE(iommu_buses) && !err; i++) {
>                 iommu_buses[i]->iommu_ops = ops;
> -               err = bus_iommu_probe(iommu_buses[i]);
> +               err = bus_iommu_probe(iommu_buses[i], iommu);
>         }
>         if (err)
>                 iommu_device_unregister(iommu);
> @@ -452,24 +453,23 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
>         struct group_device *gdev;
>         int ret;
>
> -       if (!ops)
> -               return -ENODEV;
>         /*
>          * Allow __iommu_probe_device() to be safely called in parallel,
>          * both dev->iommu_group and the initial setup of dev->iommu are
>          * protected this way.
>          */
> -       device_lock(dev);
> +       device_lock_assert(dev);
> +
> +       if (!ops)
> +               return -ENODEV;
>
>         /* Device is probed already if in a group */
> -       if (dev->iommu_group) {
> -               ret = 0;
> -               goto out_unlock;
> -       }
> +       if (dev->iommu_group)
> +               return 0;
>
>         ret = iommu_init_device(dev, ops);
>         if (ret)
> -               goto out_unlock;
> +               return ret;
>
>         group = dev->iommu_group;
>         gdev = iommu_group_alloc_device(group, dev);
> @@ -505,7 +505,6 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
>                         list_add_tail(&group->entry, group_list);
>         }
>         mutex_unlock(&group->mutex);
> -       device_unlock(dev);
>
>         if (dev_is_pci(dev))
>                 iommu_dma_set_pci_32bit_workaround(dev);
> @@ -519,12 +518,10 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
>         iommu_deinit_device(dev);
>         mutex_unlock(&group->mutex);
>         iommu_group_put(group);
> -out_unlock:
> -       device_unlock(dev);
>         return ret;
>  }
>
> -int iommu_probe_device(struct device *dev)
> +int iommu_probe_device_locked(struct device *dev)
>  {
>         const struct iommu_ops *ops;
>         int ret;
> @@ -540,6 +537,16 @@ int iommu_probe_device(struct device *dev)
>         return 0;
>  }
>
> +int iommu_probe_device(struct device *dev)
> +{
> +       int ret;
> +
> +       device_lock(dev);
> +       ret = iommu_probe_device_locked(dev);
> +       device_unlock(dev);
> +       return ret;
> +}
> +
>  static void __iommu_group_free_device(struct iommu_group *group,
>                                       struct group_device *grp_dev)
>  {
> @@ -1784,12 +1791,26 @@ struct iommu_domain *iommu_group_default_domain(struct iommu_group *group)
>         return group->default_domain;
>  }
>
> +struct probe_iommu_args {
> +       struct list_head *group_list;
> +       struct iommu_device *iommu;
> +};
> +
>  static int probe_iommu_group(struct device *dev, void *data)
>  {
> -       struct list_head *group_list = data;
> +       struct probe_iommu_args *args = data;
> +       bool need_lock;
>         int ret;
>
> -       ret = __iommu_probe_device(dev, group_list);
> +       /* Probing the iommu itself is always done under the device_lock */
> +       need_lock = !args->iommu || args->iommu->hwdev != dev;
> +
> +       if (need_lock)
> +               device_lock(dev);
> +       ret = __iommu_probe_device(dev, args->group_list);
> +       if (need_lock)
> +               device_unlock(dev);
> +
>         if (ret == -ENODEV)
>                 ret = 0;
>
> @@ -1858,13 +1879,16 @@ static void iommu_group_do_probe_finalize(struct device *dev)
>                 ops->probe_finalize(dev);
>  }
>
> -int bus_iommu_probe(const struct bus_type *bus)
> +int bus_iommu_probe(const struct bus_type *bus, struct iommu_device *iommu)
>  {
>         struct iommu_group *group, *next;
> +       struct probe_iommu_args args = {};
>         LIST_HEAD(group_list);
>         int ret;
>
> -       ret = bus_for_each_dev(bus, NULL, &group_list, probe_iommu_group);
> +       args.group_list = &group_list;
> +       args.iommu = iommu;
> +       ret = bus_for_each_dev(bus, NULL, &args, probe_iommu_group);
>         if (ret)
>                 return ret;
>
> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> index 157b286e36bf3a..b5b7d4bd2cefb9 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -160,7 +160,7 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
>          * probe for dev, replay it to get things in order.
>          */
>         if (!err && dev->bus)
> -               err = iommu_probe_device(dev);
> +               err = iommu_probe_device_locked(dev);
>
>         /* Ignore all other errors apart from EPROBE_DEFER */
>         if (err == -EPROBE_DEFER) {
> diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
> index 97c45f50bf4332..828679abef7503 100644
> --- a/drivers/iommu/omap-iommu.c
> +++ b/drivers/iommu/omap-iommu.c
> @@ -1234,6 +1234,10 @@ static int omap_iommu_probe(struct platform_device *pdev)
>                 if (err)
>                         goto out_sysfs;
>                 obj->has_iommu_driver = true;
> +       } else {
> +               /* Re-probe bus to probe device attached to this IOMMU */
> +               obj->iommu.hwdev = &pdev->dev;
> +               bus_iommu_probe(&platform_bus_type, &obj->iommu);
>         }
>
>         pm_runtime_enable(obj->dev);
> @@ -1242,9 +1246,6 @@ static int omap_iommu_probe(struct platform_device *pdev)
>
>         dev_info(&pdev->dev, "%s registered\n", obj->name);
>
> -       /* Re-probe bus to probe device attached to this IOMMU */
> -       bus_iommu_probe(&platform_bus_type);
> -
>         return 0;
>
>  out_sysfs:
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index f1e18e81fca78b..96782bfb384462 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -361,6 +361,7 @@ struct iommu_domain_ops {
>   * @list: Used by the iommu-core to keep a list of registered iommus
>   * @ops: iommu-ops for talking to this iommu
>   * @dev: struct device for sysfs handling
> + * @hwdev: The device HW that controls the iommu
>   * @singleton_group: Used internally for drivers that have only one group
>   * @max_pasids: number of supported PASIDs
>   */
> @@ -369,6 +370,7 @@ struct iommu_device {
>         const struct iommu_ops *ops;
>         struct fwnode_handle *fwnode;
>         struct device *dev;
> +       struct device *hwdev;
>         struct iommu_group *singleton_group;
>         u32 max_pasids;
>  };
> @@ -465,7 +467,8 @@ static inline const struct iommu_ops *dev_iommu_ops(struct device *dev)
>         return dev->iommu->iommu_dev->ops;
>  }
>
> -extern int bus_iommu_probe(const struct bus_type *bus);
> +extern int bus_iommu_probe(const struct bus_type *bus,
> +                          struct iommu_device *iommu);
>  extern bool iommu_present(const struct bus_type *bus);
>  extern bool device_iommu_capable(struct device *dev, enum iommu_cap cap);
>  extern bool iommu_group_has_isolated_msi(struct iommu_group *group);
> @@ -709,6 +712,7 @@ static inline void dev_iommu_priv_set(struct device *dev, void *priv)
>  }
>
>  int iommu_probe_device(struct device *dev);
> +int iommu_probe_device_locked(struct device *dev);
>
>  int iommu_dev_enable_feature(struct device *dev, enum iommu_dev_features f);
>  int iommu_dev_disable_feature(struct device *dev, enum iommu_dev_features f);
>
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 00/10] Refine the locking for dev->iommu_group

[Marek Szyprowski]

Hi All,

On 08.08.2023 14:32, Marek Szyprowski wrote:
> On 08.08.2023 12:31, Chen-Yu Tsai wrote:
>> On Mon, Aug 7, 2023 at 8:54 PM Joerg Roedel <joro@8bytes.org> wrote:
>>> On Mon, Jul 31, 2023 at 02:50:23PM -0300, Jason Gunthorpe wrote:
>>>> Jason Gunthorpe (10):
>>>>    iommu: Remove useless group refcounting
>>>>    iommu: Add a lockdep assertion for remaining dev->iommu_group reads
>>>>    iommu: Add generic_single_device_group()
>>>>    iommu/sun50i: Convert to generic_single_device_group()
>>>>    iommu/sprd: Convert to generic_single_device_group()
>>>>    iommu/rockchip: Convert to generic_single_device_group()
>>>>    iommu/ipmmu-vmsa: Convert to generic_single_device_group()
>>>>    iommu/omap: Convert to generic_single_device_group()
>>>>    iommu: Complete the locking for dev->iommu_group
>>>>    iommu/intel: Fix missing locking for 
>>>> show_device_domain_translation()
>>>>
>>>>   drivers/iommu/intel/debugfs.c  |  34 ++++----
>>>>   drivers/iommu/iommu.c          | 155 
>>>> +++++++++++++++++++++------------
>>>>   drivers/iommu/ipmmu-vmsa.c     |  22 ++---
>>>>   drivers/iommu/omap-iommu.c     |  30 +------
>>>>   drivers/iommu/omap-iommu.h     |   2 +-
>>>>   drivers/iommu/rockchip-iommu.c |  22 +----
>>>>   drivers/iommu/sprd-iommu.c     |  24 +----
>>>>   drivers/iommu/sun50i-iommu.c   |  29 ++----
>>>>   include/linux/iommu.h          |   3 +
>>>>   9 files changed, 138 insertions(+), 183 deletions(-)
>>> Applied, thanks for the nice cleanup!
>> This series seems to cause a hung task during boot on MediaTek 
>> platforms.
>> It hangs with next-20230808. Reverting the 10 commits from this series
>> makes the system boot up again.
>
> I confirm that next-20230808 is broken on ARM 32bit based Exynos 
> boards too. Boards lock up very early during boot. I will try to 
> investigate this soon.

Hmm this turned to be Exynos IOMMU specific, but the issue is probably 
somehow generic.

The deadlock happens early in __iommu_probe_device() on 
device_lock(dev). Here is a stack dump of that call:

CPU: 1 PID: 1 Comm: swapper/0 Not tainted 6.5.0-rc5-next-20230808-dirty 
#7013
Hardware name: Samsung Exynos (Flattened Device Tree)
  unwind_backtrace from show_stack+0x10/0x14
  show_stack from dump_stack_lvl+0x58/0x70
  dump_stack_lvl from __iommu_probe_device+0x3d8/0x4ac
  __iommu_probe_device from probe_iommu_group+0x8/0x14
  probe_iommu_group from bus_for_each_dev+0x60/0xb4
  bus_for_each_dev from bus_iommu_probe+0x34/0x118
  bus_iommu_probe from iommu_device_register+0x98/0x100
  iommu_device_register from exynos_sysmmu_probe+0x238/0x3c0
  exynos_sysmmu_probe from platform_probe+0x80/0xc0
  platform_probe from really_probe+0x154/0x3d4
  really_probe from __driver_probe_device+0xa0/0x1e8
  __driver_probe_device from driver_probe_device+0x30/0xd0
  driver_probe_device from __device_attach_driver+0xbc/0x11c
  __device_attach_driver from bus_for_each_drv+0x74/0xc0
  bus_for_each_drv from __device_attach+0xec/0x1b4
  __device_attach from bus_probe_device+0x8c/0x90
  bus_probe_device from device_add+0x5b8/0x78c
  device_add from of_platform_device_create_pdata+0x94/0xcc
  of_platform_device_create_pdata from of_platform_bus_create+0x1ac/0x4d8
  of_platform_bus_create from of_platform_bus_create+0x214/0x4d8
  of_platform_bus_create from of_platform_populate+0x80/0x114
  of_platform_populate from of_platform_default_populate_init+0xcc/0xe4
  of_platform_default_populate_init from do_one_initcall+0x6c/0x318
  do_one_initcall from kernel_init_freeable+0x1c4/0x214
  kernel_init_freeable from kernel_init+0x18/0x12c
  kernel_init from ret_from_fork+0x14/0x2c

The problem here is that exynos_sysmmu_probe() is by design called under 
device_lock, then it calls iommu_device_register(), which in turn 
triggers calling __iommu_probe_device() on all platform devices in the 
system, while the still probed sysmmu device is one of them.

Frankly speaking I have no idea how to defer calling 
iommu_device_register() to avoid this deadlock. Any ideas?

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel