[PATCH v2 00/10] Consolidate the probe_device path

[PATCH v2 00/10] Consolidate the probe_device path

[Jason Gunthorpe]

Now that the domain allocation path is less duplicated we can tackle the
probe_device path. Details of this are spread across several functions,
broadly move most of the code into __iommu_probe_device() and organize it
more strictly in terms of paired do/undo functions.

Make the locking simpler by obtaining the group->mutex fewer times and
avoiding adding a half-initialized device to an initialized
group. Previously we would lock/unlock the group three times on these
paths.

This locking change is the primary point of the series, creating the
paired do/undo functions is a path to being able to organize the setup
code under a single lock and still have a logical, not duplicated, error
unwind.

The reorganizing is done with the idea that a following series will
consolidate all of the different places calling arm_iommu_create_mapping()
and iommu_setup_dma_ops() into the new consolidated probe path and get rid
of probe_finalize.

This follows the prior series:

 https://lore.kernel.org/r/0-v5-1b99ae392328+44574-iommu_err_unwind_jgg@nvidia.com

And while it doesn't really depend on, some of the explanations around the
add/remove group logic assume these two:

 https://lore.kernel.org/r/0-v1-1421774b874b+167-ppc_device_group_jgg@nvidia.com
 https://lore.kernel.org/r/0-v2-ce71068deeec+4cf6-fsl_rm_groups_jgg@nvidia.com

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

v2:
 - Rebase to v6.4-rc2
 - Move the POWER cleanup to an independent -rc fix due to the iommu driver
   being merged
 - Update commit messages
 - Rename iommu_init_driver() to iommu_init_device()
 - Simplify __iommu_group_remove_device()
v1: https://lore.kernel.org/r/0-v1-8aecc628b904+2f42-iommu_probe_jgg@nvidia.com

Jason Gunthorpe (10):
  iommu: Have __iommu_probe_device() check for already probed devices
  iommu: Use iommu_group_ref_get/put() for dev->iommu_group
  iommu: Inline iommu_group_get_for_dev() into __iommu_probe_device()
  iommu: Simplify the __iommu_group_remove_device() flow
  iommu: Add iommu_init/deinit_device() paired functions
  iommu: Move the iommu driver sysfs setup into
    iommu_init/deinit_device()
  iommu: Do not export iommu_device_link/unlink()
  iommu: Always destroy the iommu_group during iommu_release_device()
  iommu: Split iommu_group_add_device()
  iommu: Avoid locking/unlocking for iommu_probe_device()

 drivers/acpi/scan.c         |   2 +-
 drivers/iommu/intel/iommu.c |   7 -
 drivers/iommu/iommu-sysfs.c |   8 -
 drivers/iommu/iommu.c       | 462 ++++++++++++++++++------------------
 drivers/iommu/of_iommu.c    |   2 +-
 5 files changed, 239 insertions(+), 242 deletions(-)


base-commit: 8f6c3fcaa76a9222f435230972b4f0f5c3672cad
-- 
2.40.1

[PATCH v2 01/10] iommu: Have __iommu_probe_device() check for already probed devices

[Jason Gunthorpe]

This is a step toward making __iommu_probe_device() self contained.

It should, under proper locking, check if the device is already associated
with an iommu driver and resolve parallel probes. All but one of the
callers open code this test using two different means, but they all
rely on dev->iommu_group.

Currently the bus_iommu_probe()/probe_iommu_group() and
probe_acpi_namespace_devices() rejects already probed devices with an
unlocked read of dev->iommu_group. The OF and ACPI "replay" functions use
device_iommu_mapped() which is the same read without the pointless
refcount.

Move this test into __iommu_probe_device() and put it under the
iommu_probe_device_lock. The store to dev->iommu_group is in
iommu_group_add_device() which is also called under this lock for iommu
driver devices, making it properly locked.

The only path that didn't have this check is the hotplug path triggered by
BUS_NOTIFY_ADD_DEVICE. The only way to get dev->iommu_group assigned
outside the probe path is via iommu_group_add_device(). Today the only
caller is VFIO no-iommu which never associates with an iommu driver. Thus
adding this additional check is safe.

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/acpi/scan.c         |  2 +-
 drivers/iommu/intel/iommu.c |  7 -------
 drivers/iommu/iommu.c       | 19 +++++++++----------
 drivers/iommu/of_iommu.c    |  2 +-
 4 files changed, 11 insertions(+), 19 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 0c6f06abe3f47f..945866f3bd8ebd 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1579,7 +1579,7 @@ static const struct iommu_ops *acpi_iommu_configure_id(struct device *dev,
 	 * If we have reason to believe the IOMMU driver missed the initial
 	 * iommu_probe_device() call for dev, replay it to get things in order.
 	 */
-	if (!err && dev->bus && !device_iommu_mapped(dev))
+	if (!err && dev->bus)
 		err = iommu_probe_device(dev);
 
 	/* Ignore all other errors apart from EPROBE_DEFER */
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index b871a6afd80321..3c37b50c121c2d 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -3763,7 +3763,6 @@ static int __init probe_acpi_namespace_devices(void)
 		for_each_active_dev_scope(drhd->devices,
 					  drhd->devices_cnt, i, dev) {
 			struct acpi_device_physical_node *pn;
-			struct iommu_group *group;
 			struct acpi_device *adev;
 
 			if (dev->bus != &acpi_bus_type)
@@ -3773,12 +3772,6 @@ static int __init probe_acpi_namespace_devices(void)
 			mutex_lock(&adev->physical_node_lock);
 			list_for_each_entry(pn,
 					    &adev->physical_node_list, node) {
-				group = iommu_group_get(pn->dev);
-				if (group) {
-					iommu_group_put(group);
-					continue;
-				}
-
 				ret = iommu_probe_device(pn->dev);
 				if (ret)
 					break;
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 4d7010f2b260a7..6d4d6a4d692893 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -351,9 +351,16 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
 	 * but for now enforcing a simple global ordering is fine.
 	 */
 	mutex_lock(&iommu_probe_device_lock);
+
+	/* Device is probed already if in a group */
+	if (dev->iommu_group) {
+		ret = 0;
+		goto out_unlock;
+	}
+
 	if (!dev_iommu_get(dev)) {
 		ret = -ENOMEM;
-		goto err_unlock;
+		goto out_unlock;
 	}
 
 	if (!try_module_get(ops->owner)) {
@@ -399,7 +406,7 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
 err_free:
 	dev_iommu_free(dev);
 
-err_unlock:
+out_unlock:
 	mutex_unlock(&iommu_probe_device_lock);
 
 	return ret;
@@ -1711,16 +1718,8 @@ struct iommu_domain *iommu_group_default_domain(struct iommu_group *group)
 static int probe_iommu_group(struct device *dev, void *data)
 {
 	struct list_head *group_list = data;
-	struct iommu_group *group;
 	int ret;
 
-	/* Device is probed already if in a group */
-	group = iommu_group_get(dev);
-	if (group) {
-		iommu_group_put(group);
-		return 0;
-	}
-
 	ret = __iommu_probe_device(dev, group_list);
 	if (ret == -ENODEV)
 		ret = 0;
diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index 40f57d293a79d4..157b286e36bf3a 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -159,7 +159,7 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
 	 * If we have reason to believe the IOMMU driver missed the initial
 	 * probe for dev, replay it to get things in order.
 	 */
-	if (!err && dev->bus && !device_iommu_mapped(dev))
+	if (!err && dev->bus)
 		err = iommu_probe_device(dev);
 
 	/* Ignore all other errors apart from EPROBE_DEFER */
-- 
2.40.1

Re: [PATCH v2 01/10] iommu: Have __iommu_probe_device() check for already probed devices

[Baolu Lu]

On 5/20/23 2:42 AM, Jason Gunthorpe wrote:
> This is a step toward making __iommu_probe_device() self contained.
> 
> It should, under proper locking, check if the device is already associated
> with an iommu driver and resolve parallel probes. All but one of the
> callers open code this test using two different means, but they all
> rely on dev->iommu_group.
> 
> Currently the bus_iommu_probe()/probe_iommu_group() and
> probe_acpi_namespace_devices() rejects already probed devices with an
> unlocked read of dev->iommu_group. The OF and ACPI "replay" functions use
> device_iommu_mapped() which is the same read without the pointless
> refcount.
> 
> Move this test into __iommu_probe_device() and put it under the
> iommu_probe_device_lock. The store to dev->iommu_group is in
> iommu_group_add_device() which is also called under this lock for iommu
> driver devices, making it properly locked.
> 
> The only path that didn't have this check is the hotplug path triggered by
> BUS_NOTIFY_ADD_DEVICE. The only way to get dev->iommu_group assigned
> outside the probe path is via iommu_group_add_device(). Today the only
> caller is VFIO no-iommu which never associates with an iommu driver. Thus
> adding this additional check is safe.
> 
> Reviewed-by: Kevin Tian<kevin.tian@intel.com>
> Signed-off-by: Jason Gunthorpe<jgg@nvidia.com>

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu

Re: [PATCH v2 01/10] iommu: Have __iommu_probe_device() check for already probed devices

[Rafael J. Wysocki]

On Fri, May 19, 2023 at 8:42 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> This is a step toward making __iommu_probe_device() self contained.
>
> It should, under proper locking, check if the device is already associated
> with an iommu driver and resolve parallel probes. All but one of the
> callers open code this test using two different means, but they all
> rely on dev->iommu_group.
>
> Currently the bus_iommu_probe()/probe_iommu_group() and
> probe_acpi_namespace_devices() rejects already probed devices with an
> unlocked read of dev->iommu_group. The OF and ACPI "replay" functions use
> device_iommu_mapped() which is the same read without the pointless
> refcount.
>
> Move this test into __iommu_probe_device() and put it under the
> iommu_probe_device_lock. The store to dev->iommu_group is in
> iommu_group_add_device() which is also called under this lock for iommu
> driver devices, making it properly locked.
>
> The only path that didn't have this check is the hotplug path triggered by
> BUS_NOTIFY_ADD_DEVICE. The only way to get dev->iommu_group assigned
> outside the probe path is via iommu_group_add_device(). Today the only
> caller is VFIO no-iommu which never associates with an iommu driver. Thus
> adding this additional check is safe.
>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

for the ACPI change in scan.c.

> ---
>  drivers/acpi/scan.c         |  2 +-
>  drivers/iommu/intel/iommu.c |  7 -------
>  drivers/iommu/iommu.c       | 19 +++++++++----------
>  drivers/iommu/of_iommu.c    |  2 +-
>  4 files changed, 11 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 0c6f06abe3f47f..945866f3bd8ebd 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -1579,7 +1579,7 @@ static const struct iommu_ops *acpi_iommu_configure_id(struct device *dev,
>          * If we have reason to believe the IOMMU driver missed the initial
>          * iommu_probe_device() call for dev, replay it to get things in order.
>          */
> -       if (!err && dev->bus && !device_iommu_mapped(dev))
> +       if (!err && dev->bus)
>                 err = iommu_probe_device(dev);
>
>         /* Ignore all other errors apart from EPROBE_DEFER */
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index b871a6afd80321..3c37b50c121c2d 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -3763,7 +3763,6 @@ static int __init probe_acpi_namespace_devices(void)
>                 for_each_active_dev_scope(drhd->devices,
>                                           drhd->devices_cnt, i, dev) {
>                         struct acpi_device_physical_node *pn;
> -                       struct iommu_group *group;
>                         struct acpi_device *adev;
>
>                         if (dev->bus != &acpi_bus_type)
> @@ -3773,12 +3772,6 @@ static int __init probe_acpi_namespace_devices(void)
>                         mutex_lock(&adev->physical_node_lock);
>                         list_for_each_entry(pn,
>                                             &adev->physical_node_list, node) {
> -                               group = iommu_group_get(pn->dev);
> -                               if (group) {
> -                                       iommu_group_put(group);
> -                                       continue;
> -                               }
> -
>                                 ret = iommu_probe_device(pn->dev);
>                                 if (ret)
>                                         break;
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 4d7010f2b260a7..6d4d6a4d692893 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -351,9 +351,16 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
>          * but for now enforcing a simple global ordering is fine.
>          */
>         mutex_lock(&iommu_probe_device_lock);
> +
> +       /* Device is probed already if in a group */
> +       if (dev->iommu_group) {
> +               ret = 0;
> +               goto out_unlock;
> +       }
> +
>         if (!dev_iommu_get(dev)) {
>                 ret = -ENOMEM;
> -               goto err_unlock;
> +               goto out_unlock;
>         }
>
>         if (!try_module_get(ops->owner)) {
> @@ -399,7 +406,7 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
>  err_free:
>         dev_iommu_free(dev);
>
> -err_unlock:
> +out_unlock:
>         mutex_unlock(&iommu_probe_device_lock);
>
>         return ret;
> @@ -1711,16 +1718,8 @@ struct iommu_domain *iommu_group_default_domain(struct iommu_group *group)
>  static int probe_iommu_group(struct device *dev, void *data)
>  {
>         struct list_head *group_list = data;
> -       struct iommu_group *group;
>         int ret;
>
> -       /* Device is probed already if in a group */
> -       group = iommu_group_get(dev);
> -       if (group) {
> -               iommu_group_put(group);
> -               return 0;
> -       }
> -
>         ret = __iommu_probe_device(dev, group_list);
>         if (ret == -ENODEV)
>                 ret = 0;
> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> index 40f57d293a79d4..157b286e36bf3a 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -159,7 +159,7 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
>          * If we have reason to believe the IOMMU driver missed the initial
>          * probe for dev, replay it to get things in order.
>          */
> -       if (!err && dev->bus && !device_iommu_mapped(dev))
> +       if (!err && dev->bus)
>                 err = iommu_probe_device(dev);
>
>         /* Ignore all other errors apart from EPROBE_DEFER */
> --
> 2.40.1
>

[PATCH v2 02/10] iommu: Use iommu_group_ref_get/put() for dev->iommu_group

[Jason Gunthorpe]

No reason to open code this, use the proper helper functions.

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommu.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 6d4d6a4d692893..35dadcc9999f8b 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -500,7 +500,7 @@ static void __iommu_group_release_device(struct iommu_group *group,
 	kfree(grp_dev->name);
 	kfree(grp_dev);
 	dev->iommu_group = NULL;
-	kobject_put(group->devices_kobj);
+	iommu_group_put(group);
 }
 
 static void iommu_release_device(struct device *dev)
@@ -1067,8 +1067,7 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)
 		goto err_free_name;
 	}
 
-	kobject_get(group->devices_kobj);
-
+	iommu_group_ref_get(group);
 	dev->iommu_group = group;
 
 	mutex_lock(&group->mutex);
-- 
2.40.1

Re: [PATCH v2 02/10] iommu: Use iommu_group_ref_get/put() for dev->iommu_group

[Baolu Lu]

On 5/20/23 2:42 AM, Jason Gunthorpe wrote:
> No reason to open code this, use the proper helper functions.
> 
> Reviewed-by: Kevin Tian<kevin.tian@intel.com>
> Signed-off-by: Jason Gunthorpe<jgg@nvidia.com>

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu

[PATCH v2 03/10] iommu: Inline iommu_group_get_for_dev() into __iommu_probe_device()

[Jason Gunthorpe]

This is the only caller, and it doesn't need the generality of the
function. We already know there is no iommu_group, so it is simply two
function calls.

Moving it here allows the following patches to split the logic in these
functions.

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommu.c | 50 ++++++++-----------------------------------
 1 file changed, 9 insertions(+), 41 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 35dadcc9999f8b..6177e01ced67ab 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -127,7 +127,6 @@ static int iommu_setup_default_domain(struct iommu_group *group,
 				      int target_type);
 static int iommu_create_device_direct_mappings(struct iommu_domain *domain,
 					       struct device *dev);
-static struct iommu_group *iommu_group_get_for_dev(struct device *dev);
 static ssize_t iommu_group_store_type(struct iommu_group *group,
 				      const char *buf, size_t count);
 
@@ -379,12 +378,18 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
 	if (ops->is_attach_deferred)
 		dev->iommu->attach_deferred = ops->is_attach_deferred(dev);
 
-	group = iommu_group_get_for_dev(dev);
+	group = ops->device_group(dev);
+	if (WARN_ON_ONCE(group == NULL))
+		group = ERR_PTR(-EINVAL);
 	if (IS_ERR(group)) {
 		ret = PTR_ERR(group);
 		goto out_release;
 	}
 
+	ret = iommu_group_add_device(group, dev);
+	if (ret)
+		goto err_put_group;
+
 	mutex_lock(&group->mutex);
 	if (group_list && !group->default_domain && list_empty(&group->entry))
 		list_add_tail(&group->entry, group_list);
@@ -396,6 +401,8 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
 
 	return 0;
 
+err_put_group:
+	iommu_group_put(group);
 out_release:
 	if (ops->release_device)
 		ops->release_device(dev);
@@ -1670,45 +1677,6 @@ iommu_group_alloc_default_domain(struct iommu_group *group, int req_type)
 	return dom;
 }
 
-/**
- * iommu_group_get_for_dev - Find or create the IOMMU group for a device
- * @dev: target device
- *
- * This function is intended to be called by IOMMU drivers and extended to
- * support common, bus-defined algorithms when determining or creating the
- * IOMMU group for a device.  On success, the caller will hold a reference
- * to the returned IOMMU group, which will already include the provided
- * device.  The reference should be released with iommu_group_put().
- */
-static struct iommu_group *iommu_group_get_for_dev(struct device *dev)
-{
-	const struct iommu_ops *ops = dev_iommu_ops(dev);
-	struct iommu_group *group;
-	int ret;
-
-	group = iommu_group_get(dev);
-	if (group)
-		return group;
-
-	group = ops->device_group(dev);
-	if (WARN_ON_ONCE(group == NULL))
-		return ERR_PTR(-EINVAL);
-
-	if (IS_ERR(group))
-		return group;
-
-	ret = iommu_group_add_device(group, dev);
-	if (ret)
-		goto out_put_group;
-
-	return group;
-
-out_put_group:
-	iommu_group_put(group);
-
-	return ERR_PTR(ret);
-}
-
 struct iommu_domain *iommu_group_default_domain(struct iommu_group *group)
 {
 	return group->default_domain;
-- 
2.40.1

Re: [PATCH v2 03/10] iommu: Inline iommu_group_get_for_dev() into __iommu_probe_device()

[Baolu Lu]

On 5/20/23 2:42 AM, Jason Gunthorpe wrote:
> This is the only caller, and it doesn't need the generality of the
> function. We already know there is no iommu_group, so it is simply two
> function calls.
> 
> Moving it here allows the following patches to split the logic in these
> functions.
> 
> Reviewed-by: Kevin Tian<kevin.tian@intel.com>
> Signed-off-by: Jason Gunthorpe<jgg@nvidia.com>
> ---
>   drivers/iommu/iommu.c | 50 ++++++++-----------------------------------
>   1 file changed, 9 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 35dadcc9999f8b..6177e01ced67ab 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -127,7 +127,6 @@ static int iommu_setup_default_domain(struct iommu_group *group,
>   				      int target_type);
>   static int iommu_create_device_direct_mappings(struct iommu_domain *domain,
>   					       struct device *dev);
> -static struct iommu_group *iommu_group_get_for_dev(struct device *dev);
>   static ssize_t iommu_group_store_type(struct iommu_group *group,
>   				      const char *buf, size_t count);
>   
> @@ -379,12 +378,18 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
>   	if (ops->is_attach_deferred)
>   		dev->iommu->attach_deferred = ops->is_attach_deferred(dev);
>   
> -	group = iommu_group_get_for_dev(dev);
> +	group = ops->device_group(dev);
> +	if (WARN_ON_ONCE(group == NULL))
> +		group = ERR_PTR(-EINVAL);
>   	if (IS_ERR(group)) {
>   		ret = PTR_ERR(group);
>   		goto out_release;
>   	}
>   
> +	ret = iommu_group_add_device(group, dev);
> +	if (ret)
> +		goto err_put_group;
> +
>   	mutex_lock(&group->mutex);
>   	if (group_list && !group->default_domain && list_empty(&group->entry))
>   		list_add_tail(&group->entry, group_list);
> @@ -396,6 +401,8 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
>   
>   	return 0;
>   
> +err_put_group:

nit: perhaps out_put_group?

> +	iommu_group_put(group);
>   out_release:
>   	if (ops->release_device)
>   		ops->release_device(dev);
> @@ -1670,45 +1677,6 @@ iommu_group_alloc_default_domain(struct iommu_group *group, int req_type)
>   	return dom;
>   }

Others look good to me:

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu

Re: [PATCH v2 03/10] iommu: Inline iommu_group_get_for_dev() into __iommu_probe_device()

[Jason Gunthorpe]

On Sun, May 21, 2023 at 04:19:34PM +0800, Baolu Lu wrote:
> On 5/20/23 2:42 AM, Jason Gunthorpe wrote:
> > This is the only caller, and it doesn't need the generality of the
> > function. We already know there is no iommu_group, so it is simply two
> > function calls.
> > 
> > Moving it here allows the following patches to split the logic in these
> > functions.
> > 
> > Reviewed-by: Kevin Tian<kevin.tian@intel.com>
> > Signed-off-by: Jason Gunthorpe<jgg@nvidia.com>
> > ---
> >   drivers/iommu/iommu.c | 50 ++++++++-----------------------------------
> >   1 file changed, 9 insertions(+), 41 deletions(-)
> > 
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index 35dadcc9999f8b..6177e01ced67ab 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -127,7 +127,6 @@ static int iommu_setup_default_domain(struct iommu_group *group,
> >   				      int target_type);
> >   static int iommu_create_device_direct_mappings(struct iommu_domain *domain,
> >   					       struct device *dev);
> > -static struct iommu_group *iommu_group_get_for_dev(struct device *dev);
> >   static ssize_t iommu_group_store_type(struct iommu_group *group,
> >   				      const char *buf, size_t count);
> > @@ -379,12 +378,18 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
> >   	if (ops->is_attach_deferred)
> >   		dev->iommu->attach_deferred = ops->is_attach_deferred(dev);
> > -	group = iommu_group_get_for_dev(dev);
> > +	group = ops->device_group(dev);
> > +	if (WARN_ON_ONCE(group == NULL))
> > +		group = ERR_PTR(-EINVAL);
> >   	if (IS_ERR(group)) {
> >   		ret = PTR_ERR(group);
> >   		goto out_release;
> >   	}
> > +	ret = iommu_group_add_device(group, dev);
> > +	if (ret)
> > +		goto err_put_group;
> > +
> >   	mutex_lock(&group->mutex);
> >   	if (group_list && !group->default_domain && list_empty(&group->entry))
> >   		list_add_tail(&group->entry, group_list);
> > @@ -396,6 +401,8 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
> >   	return 0;
> > +err_put_group:
> 
> nit: perhaps out_put_group?

err_ is the right label, this is not a success path.. Most of the
labels are err_ except for out_unlock which is sometimes a success and
out_module_put which has always been mislabeled..

Jason

[PATCH v2 04/10] iommu: Simplify the __iommu_group_remove_device() flow

[Jason Gunthorpe]

Instead of returning the struct group_device and then later freeing it, do
the entire free under the group->mutex and defer only putting the
iommu_group.

It is safe to remove the sysfs_links and free memory while holding that
mutex.

Move the sanity assert of the group status into
__iommu_group_free_device().

The next patch will improve upon this and consolidate the group put and
the mutex into __iommu_group_remove_device().

__iommu_group_free_device() is close to being the paired undo of
iommu_group_add_device(), following patches will improve on that.

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommu.c | 83 ++++++++++++++++++++-----------------------
 1 file changed, 39 insertions(+), 44 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 6177e01ced67ab..a87e2df5ce1238 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -470,32 +470,8 @@ int iommu_probe_device(struct device *dev)
 
 }
 
-/*
- * Remove a device from a group's device list and return the group device
- * if successful.
- */
-static struct group_device *
-__iommu_group_remove_device(struct iommu_group *group, struct device *dev)
-{
-	struct group_device *device;
-
-	lockdep_assert_held(&group->mutex);
-	for_each_group_device(group, device) {
-		if (device->dev == dev) {
-			list_del(&device->list);
-			return device;
-		}
-	}
-
-	return NULL;
-}
-
-/*
- * Release a device from its group and decrements the iommu group reference
- * count.
- */
-static void __iommu_group_release_device(struct iommu_group *group,
-					 struct group_device *grp_dev)
+static void __iommu_group_free_device(struct iommu_group *group,
+				      struct group_device *grp_dev)
 {
 	struct device *dev = grp_dev->dev;
 
@@ -504,16 +480,45 @@ static void __iommu_group_release_device(struct iommu_group *group,
 
 	trace_remove_device_from_group(group->id, dev);
 
+	/*
+	 * If the group has become empty then ownership must have been
+	 * released, and the current domain must be set back to NULL or
+	 * the default domain.
+	 */
+	if (list_empty(&group->devices))
+		WARN_ON(group->owner_cnt ||
+			group->domain != group->default_domain);
+
 	kfree(grp_dev->name);
 	kfree(grp_dev);
 	dev->iommu_group = NULL;
-	iommu_group_put(group);
+}
+
+/*
+ * Remove the iommu_group from the struct device. The attached group must be put
+ * by the caller after releaseing the group->mutex.
+ */
+static void __iommu_group_remove_device(struct device *dev)
+{
+	struct iommu_group *group = dev->iommu_group;
+	struct group_device *device;
+
+	lockdep_assert_held(&group->mutex);
+	for_each_group_device(group, device) {
+		if (device->dev != dev)
+			continue;
+
+		list_del(&device->list);
+		__iommu_group_free_device(group, device);
+		/* Caller must put iommu_group */
+		return;
+	}
+	WARN(true, "Corrupted iommu_group device_list");
 }
 
 static void iommu_release_device(struct device *dev)
 {
 	struct iommu_group *group = dev->iommu_group;
-	struct group_device *device;
 	const struct iommu_ops *ops;
 
 	if (!dev->iommu || !group)
@@ -522,16 +527,7 @@ static void iommu_release_device(struct device *dev)
 	iommu_device_unlink(dev->iommu->iommu_dev, dev);
 
 	mutex_lock(&group->mutex);
-	device = __iommu_group_remove_device(group, dev);
-
-	/*
-	 * If the group has become empty then ownership must have been released,
-	 * and the current domain must be set back to NULL or the default
-	 * domain.
-	 */
-	if (list_empty(&group->devices))
-		WARN_ON(group->owner_cnt ||
-			group->domain != group->default_domain);
+	__iommu_group_remove_device(dev);
 
 	/*
 	 * release_device() must stop using any attached domain on the device.
@@ -547,8 +543,8 @@ static void iommu_release_device(struct device *dev)
 		ops->release_device(dev);
 	mutex_unlock(&group->mutex);
 
-	if (device)
-		__iommu_group_release_device(group, device);
+	/* Pairs with the get in iommu_group_add_device() */
+	iommu_group_put(group);
 
 	module_put(ops->owner);
 	dev_iommu_free(dev);
@@ -1107,7 +1103,6 @@ EXPORT_SYMBOL_GPL(iommu_group_add_device);
 void iommu_group_remove_device(struct device *dev)
 {
 	struct iommu_group *group = dev->iommu_group;
-	struct group_device *device;
 
 	if (!group)
 		return;
@@ -1115,11 +1110,11 @@ void iommu_group_remove_device(struct device *dev)
 	dev_info(dev, "Removing from iommu group %d\n", group->id);
 
 	mutex_lock(&group->mutex);
-	device = __iommu_group_remove_device(group, dev);
+	__iommu_group_remove_device(dev);
 	mutex_unlock(&group->mutex);
 
-	if (device)
-		__iommu_group_release_device(group, device);
+	/* Pairs with the get in iommu_group_add_device() */
+	iommu_group_put(group);
 }
 EXPORT_SYMBOL_GPL(iommu_group_remove_device);
 
-- 
2.40.1

Re: [PATCH v2 04/10] iommu: Simplify the __iommu_group_remove_device() flow

[Baolu Lu]

On 5/20/23 2:42 AM, Jason Gunthorpe wrote:
> Instead of returning the struct group_device and then later freeing it, do
> the entire free under the group->mutex and defer only putting the
> iommu_group.
> 
> It is safe to remove the sysfs_links and free memory while holding that
> mutex.
> 
> Move the sanity assert of the group status into
> __iommu_group_free_device().
> 
> The next patch will improve upon this and consolidate the group put and
> the mutex into __iommu_group_remove_device().
> 
> __iommu_group_free_device() is close to being the paired undo of
> iommu_group_add_device(), following patches will improve on that.
> 
> Reviewed-by: Kevin Tian<kevin.tian@intel.com>
> Signed-off-by: Jason Gunthorpe<jgg@nvidia.com>

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu

Re: [PATCH v2 04/10] iommu: Simplify the __iommu_group_remove_device() flow

[Niklas Schnelle]

On Fri, 2023-05-19 at 15:42 -0300, Jason Gunthorpe wrote:
> Instead of returning the struct group_device and then later freeing it, do
> the entire free under the group->mutex and defer only putting the
> iommu_group.
> 
> It is safe to remove the sysfs_links and free memory while holding that
> mutex.
> 
> Move the sanity assert of the group status into
> __iommu_group_free_device().
> 
> The next patch will improve upon this and consolidate the group put and
> the mutex into __iommu_group_remove_device().
> 
> __iommu_group_free_device() is close to being the paired undo of
> iommu_group_add_device(), following patches will improve on that.
> 
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/iommu/iommu.c | 83 ++++++++++++++++++++-----------------------
>  1 file changed, 39 insertions(+), 44 deletions(-)
> 
---8<---
> +
> +/*
> + * Remove the iommu_group from the struct device. The attached group must be put
> + * by the caller after releaseing the group->mutex.
> + */
> +static void __iommu_group_remove_device(struct device *dev)
> +{
> +	struct iommu_group *group = dev->iommu_group;
> +	struct group_device *device;
> +
> +	lockdep_assert_held(&group->mutex);
> +	for_each_group_device(group, device) {
> +		if (device->dev != dev)
> +			continue;
> +
> +		list_del(&device->list);

for_each_group_device() uses list_for_each_entry() but here you are
deleting from the list, don't we need a ..._safe() variant then?

> +		__iommu_group_free_device(group, device);
> +		/* Caller must put iommu_group */
> +		return;
> +	}
> +	WARN(true, "Corrupted iommu_group device_list");
>  }
> 
---8<---

Re: [PATCH v2 04/10] iommu: Simplify the __iommu_group_remove_device() flow

[Jason Gunthorpe]

On Mon, May 22, 2023 at 10:35:49AM +0200, Niklas Schnelle wrote:
> On Fri, 2023-05-19 at 15:42 -0300, Jason Gunthorpe wrote:
> > Instead of returning the struct group_device and then later freeing it, do
> > the entire free under the group->mutex and defer only putting the
> > iommu_group.
> > 
> > It is safe to remove the sysfs_links and free memory while holding that
> > mutex.
> > 
> > Move the sanity assert of the group status into
> > __iommu_group_free_device().
> > 
> > The next patch will improve upon this and consolidate the group put and
> > the mutex into __iommu_group_remove_device().
> > 
> > __iommu_group_free_device() is close to being the paired undo of
> > iommu_group_add_device(), following patches will improve on that.
> > 
> > Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> > ---
> >  drivers/iommu/iommu.c | 83 ++++++++++++++++++++-----------------------
> >  1 file changed, 39 insertions(+), 44 deletions(-)
> > 
> ---8<---
> > +
> > +/*
> > + * Remove the iommu_group from the struct device. The attached group must be put
> > + * by the caller after releaseing the group->mutex.
> > + */
> > +static void __iommu_group_remove_device(struct device *dev)
> > +{
> > +	struct iommu_group *group = dev->iommu_group;
> > +	struct group_device *device;
> > +
> > +	lockdep_assert_held(&group->mutex);
> > +	for_each_group_device(group, device) {
> > +		if (device->dev != dev)
> > +			continue;
> > +
> > +		list_del(&device->list);
> 
> for_each_group_device() uses list_for_each_entry() but here you are
> deleting from the list, don't we need a ..._safe() variant then?

As a general statement, yes

> > +		__iommu_group_free_device(group, device);
> > +		/* Caller must put iommu_group */
> > +		return;

But the loop immediately returns before going to the next iteration so
this is safe.

Jason

[PATCH v2 05/10] iommu: Add iommu_init/deinit_device() paired functions

[Jason Gunthorpe]

Move the driver init and destruction code into two logically paired
functions.

There is a subtle ordering dependency in how the group's domains are
freed, the current code does the kobject_put() on the group which will
hopefully trigger the free of the domains before the module_put() that
protects the domain->ops.

Reorganize this to be explicit and documented. The domains are cleaned up
by iommu_deinit_device() if it is the last device to be deinit'd from the
group.  This must be done in a specific order - after
ops->release_device() and before the module_put(). Make it very clear and
obvious by putting the order directly in one function.

Leave WARN_ON's in case the refcounting gets messed up somehow.

This also moves the module_put() and dev_iommu_free() under the
group->mutex to keep the code simple.

Building paired functions like this helps ensure that error cleanup flows
in __iommu_probe_device() are correct because they share the same code
that handles the normal flow. These details become relavent as following
patches add more error unwind into __iommu_probe_device(), and ultimately
a following series adds fine-grained locking to __iommu_probe_device().

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommu.c | 190 +++++++++++++++++++++++++-----------------
 1 file changed, 112 insertions(+), 78 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index a87e2df5ce1238..2031cb4782b9b5 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -332,10 +332,99 @@ static u32 dev_iommu_get_max_pasids(struct device *dev)
 	return min_t(u32, max_pasids, dev->iommu->iommu_dev->max_pasids);
 }
 
+/*
+ * Init the dev->iommu and dev->iommu_group in the struct device and get the
+ * driver probed
+ */
+static int iommu_init_device(struct device *dev, const struct iommu_ops *ops)
+{
+	struct iommu_device *iommu_dev;
+	struct iommu_group *group;
+	int ret;
+
+	if (!dev_iommu_get(dev))
+		return -ENOMEM;
+
+	if (!try_module_get(ops->owner)) {
+		ret = -EINVAL;
+		goto err_free;
+	}
+
+	iommu_dev = ops->probe_device(dev);
+	if (IS_ERR(iommu_dev)) {
+		ret = PTR_ERR(iommu_dev);
+		goto err_module_put;
+	}
+
+	group = ops->device_group(dev);
+	if (WARN_ON_ONCE(group == NULL))
+		group = ERR_PTR(-EINVAL);
+	if (IS_ERR(group)) {
+		ret = PTR_ERR(group);
+		goto err_release;
+	}
+	dev->iommu_group = group;
+
+	dev->iommu->iommu_dev = iommu_dev;
+	dev->iommu->max_pasids = dev_iommu_get_max_pasids(dev);
+	if (ops->is_attach_deferred)
+		dev->iommu->attach_deferred = ops->is_attach_deferred(dev);
+	return 0;
+
+err_release:
+	if (ops->release_device)
+		ops->release_device(dev);
+err_module_put:
+	module_put(ops->owner);
+err_free:
+	dev_iommu_free(dev);
+	return ret;
+}
+
+static void iommu_deinit_device(struct device *dev)
+{
+	struct iommu_group *group = dev->iommu_group;
+	const struct iommu_ops *ops = dev_iommu_ops(dev);
+
+	lockdep_assert_held(&group->mutex);
+
+	/*
+	 * release_device() must stop using any attached domain on the device.
+	 * If there are still other devices in the group they are not effected
+	 * by this callback.
+	 *
+	 * The IOMMU driver must set the device to either an identity or
+	 * blocking translation and stop using any domain pointer, as it is
+	 * going to be freed.
+	 */
+	if (ops->release_device)
+		ops->release_device(dev);
+
+	/*
+	 * If this is the last driver to use the group then we must free the
+	 * domains before we do the module_put().
+	 */
+	if (list_empty(&group->devices)) {
+		if (group->default_domain) {
+			iommu_domain_free(group->default_domain);
+			group->default_domain = NULL;
+		}
+		if (group->blocking_domain) {
+			iommu_domain_free(group->blocking_domain);
+			group->blocking_domain = NULL;
+		}
+		group->domain = NULL;
+	}
+
+	/* Caller must put iommu_group */
+	dev->iommu_group = NULL;
+	module_put(ops->owner);
+	dev_iommu_free(dev);
+}
+
 static int __iommu_probe_device(struct device *dev, struct list_head *group_list)
 {
 	const struct iommu_ops *ops = dev->bus->iommu_ops;
-	struct iommu_device *iommu_dev;
 	struct iommu_group *group;
 	static DEFINE_MUTEX(iommu_probe_device_lock);
 	int ret;
@@ -357,62 +446,30 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
 		goto out_unlock;
 	}
 
-	if (!dev_iommu_get(dev)) {
-		ret = -ENOMEM;
+	ret = iommu_init_device(dev, ops);
+	if (ret)
 		goto out_unlock;
-	}
-
-	if (!try_module_get(ops->owner)) {
-		ret = -EINVAL;
-		goto err_free;
-	}
-
-	iommu_dev = ops->probe_device(dev);
-	if (IS_ERR(iommu_dev)) {
-		ret = PTR_ERR(iommu_dev);
-		goto out_module_put;
-	}
-
-	dev->iommu->iommu_dev = iommu_dev;
-	dev->iommu->max_pasids = dev_iommu_get_max_pasids(dev);
-	if (ops->is_attach_deferred)
-		dev->iommu->attach_deferred = ops->is_attach_deferred(dev);
-
-	group = ops->device_group(dev);
-	if (WARN_ON_ONCE(group == NULL))
-		group = ERR_PTR(-EINVAL);
-	if (IS_ERR(group)) {
-		ret = PTR_ERR(group);
-		goto out_release;
-	}
 
+	group = dev->iommu_group;
 	ret = iommu_group_add_device(group, dev);
+	mutex_lock(&group->mutex);
 	if (ret)
 		goto err_put_group;
 
-	mutex_lock(&group->mutex);
 	if (group_list && !group->default_domain && list_empty(&group->entry))
 		list_add_tail(&group->entry, group_list);
 	mutex_unlock(&group->mutex);
 	iommu_group_put(group);
 
 	mutex_unlock(&iommu_probe_device_lock);
-	iommu_device_link(iommu_dev, dev);
+	iommu_device_link(dev->iommu->iommu_dev, dev);
 
 	return 0;
 
 err_put_group:
+	iommu_deinit_device(dev);
+	mutex_unlock(&group->mutex);
 	iommu_group_put(group);
-out_release:
-	if (ops->release_device)
-		ops->release_device(dev);
-
-out_module_put:
-	module_put(ops->owner);
-
-err_free:
-	dev_iommu_free(dev);
-
 out_unlock:
 	mutex_unlock(&iommu_probe_device_lock);
 
@@ -491,18 +548,15 @@ static void __iommu_group_free_device(struct iommu_group *group,
 
 	kfree(grp_dev->name);
 	kfree(grp_dev);
-	dev->iommu_group = NULL;
 }
 
-/*
- * Remove the iommu_group from the struct device. The attached group must be put
- * by the caller after releaseing the group->mutex.
- */
+/* Remove the iommu_group from the struct device. */
 static void __iommu_group_remove_device(struct device *dev)
 {
 	struct iommu_group *group = dev->iommu_group;
 	struct group_device *device;
 
+	mutex_lock(&group->mutex);
 	lockdep_assert_held(&group->mutex);
 	for_each_group_device(group, device) {
 		if (device->dev != dev)
@@ -510,44 +564,30 @@ static void __iommu_group_remove_device(struct device *dev)
 
 		list_del(&device->list);
 		__iommu_group_free_device(group, device);
-		/* Caller must put iommu_group */
-		return;
+		if (dev->iommu && dev->iommu->iommu_dev)
+			iommu_deinit_device(dev);
+		else
+			dev->iommu_group = NULL;
+		goto out;
 	}
 	WARN(true, "Corrupted iommu_group device_list");
+out:
+	mutex_unlock(&group->mutex);
+
+	/* Pairs with the get in iommu_group_add_device() */
+	iommu_group_put(group);
 }
 
 static void iommu_release_device(struct device *dev)
 {
 	struct iommu_group *group = dev->iommu_group;
-	const struct iommu_ops *ops;
 
 	if (!dev->iommu || !group)
 		return;
 
 	iommu_device_unlink(dev->iommu->iommu_dev, dev);
 
-	mutex_lock(&group->mutex);
 	__iommu_group_remove_device(dev);
-
-	/*
-	 * release_device() must stop using any attached domain on the device.
-	 * If there are still other devices in the group they are not effected
-	 * by this callback.
-	 *
-	 * The IOMMU driver must set the device to either an identity or
-	 * blocking translation and stop using any domain pointer, as it is
-	 * going to be freed.
-	 */
-	ops = dev_iommu_ops(dev);
-	if (ops->release_device)
-		ops->release_device(dev);
-	mutex_unlock(&group->mutex);
-
-	/* Pairs with the get in iommu_group_add_device() */
-	iommu_group_put(group);
-
-	module_put(ops->owner);
-	dev_iommu_free(dev);
 }
 
 static int __init iommu_set_def_domain_type(char *str)
@@ -808,10 +848,9 @@ static void iommu_group_release(struct kobject *kobj)
 
 	ida_free(&iommu_group_ida, group->id);
 
-	if (group->default_domain)
-		iommu_domain_free(group->default_domain);
-	if (group->blocking_domain)
-		iommu_domain_free(group->blocking_domain);
+	/* Domains are free'd by iommu_deinit_device() */
+	WARN_ON(group->default_domain);
+	WARN_ON(group->blocking_domain);
 
 	kfree(group->name);
 	kfree(group);
@@ -1109,12 +1148,7 @@ void iommu_group_remove_device(struct device *dev)
 
 	dev_info(dev, "Removing from iommu group %d\n", group->id);
 
-	mutex_lock(&group->mutex);
 	__iommu_group_remove_device(dev);
-	mutex_unlock(&group->mutex);
-
-	/* Pairs with the get in iommu_group_add_device() */
-	iommu_group_put(group);
 }
 EXPORT_SYMBOL_GPL(iommu_group_remove_device);
 
-- 
2.40.1

Re: [PATCH v2 05/10] iommu: Add iommu_init/deinit_device() paired functions

[Baolu Lu]

On 5/20/23 2:42 AM, Jason Gunthorpe wrote:
> Move the driver init and destruction code into two logically paired
> functions.
> 
> There is a subtle ordering dependency in how the group's domains are
> freed, the current code does the kobject_put() on the group which will
> hopefully trigger the free of the domains before the module_put() that
> protects the domain->ops.
> 
> Reorganize this to be explicit and documented. The domains are cleaned up
> by iommu_deinit_device() if it is the last device to be deinit'd from the
> group.  This must be done in a specific order - after
> ops->release_device() and before the module_put(). Make it very clear and
> obvious by putting the order directly in one function.
> 
> Leave WARN_ON's in case the refcounting gets messed up somehow.
> 
> This also moves the module_put() and dev_iommu_free() under the
> group->mutex to keep the code simple.
> 
> Building paired functions like this helps ensure that error cleanup flows
> in __iommu_probe_device() are correct because they share the same code
> that handles the normal flow. These details become relavent as following
> patches add more error unwind into __iommu_probe_device(), and ultimately
> a following series adds fine-grained locking to __iommu_probe_device().
> 
> Reviewed-by: Kevin Tian<kevin.tian@intel.com>
> Signed-off-by: Jason Gunthorpe<jgg@nvidia.com>

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu

Re: [PATCH v2 05/10] iommu: Add iommu_init/deinit_device() paired functions

[Baolu Lu]

I revisited this patch. And I still have some questions.

On 5/20/23 2:42 AM, Jason Gunthorpe wrote:
> -/*
> - * Remove the iommu_group from the struct device. The attached group must be put
> - * by the caller after releaseing the group->mutex.
> - */
> +/* Remove the iommu_group from the struct device. */
>   static void __iommu_group_remove_device(struct device *dev)
>   {
>   	struct iommu_group *group = dev->iommu_group;
>   	struct group_device *device;
>   
> +	mutex_lock(&group->mutex);
>   	lockdep_assert_held(&group->mutex);

By moving mutex_lock/unlock into this helper, above
lockdep_assert_held() is unnecessary.

>   	for_each_group_device(group, device) {
>   		if (device->dev != dev)
> @@ -510,44 +564,30 @@ static void __iommu_group_remove_device(struct device *dev)
>   
>   		list_del(&device->list);
>   		__iommu_group_free_device(group, device);
> -		/* Caller must put iommu_group */
> -		return;
> +		if (dev->iommu && dev->iommu->iommu_dev)
> +			iommu_deinit_device(dev);
> +		else
> +			dev->iommu_group = NULL;
> +		goto out;
>   	}
>   	WARN(true, "Corrupted iommu_group device_list");
> +out:
> +	mutex_unlock(&group->mutex);
> +
> +	/* Pairs with the get in iommu_group_add_device() */
> +	iommu_group_put(group);

The group->devices_kobj was increased on the probe device path twice:

- iommu_init_device() - allocate the group
- iommu_group_add_device() - add device to the group

But, on the release path, it seems that group->devices_kobj is only
decreased once.

Did I overlook anything? Otherwise, the group will never be released,
right?

>   }
>   
>   static void iommu_release_device(struct device *dev)
>   {
>   	struct iommu_group *group = dev->iommu_group;
> -	const struct iommu_ops *ops;
>   
>   	if (!dev->iommu || !group)
>   		return;
>   
>   	iommu_device_unlink(dev->iommu->iommu_dev, dev);
>   
> -	mutex_lock(&group->mutex);
>   	__iommu_group_remove_device(dev);
> -
> -	/*
> -	 * release_device() must stop using any attached domain on the device.
> -	 * If there are still other devices in the group they are not effected
> -	 * by this callback.
> -	 *
> -	 * The IOMMU driver must set the device to either an identity or
> -	 * blocking translation and stop using any domain pointer, as it is
> -	 * going to be freed.
> -	 */
> -	ops = dev_iommu_ops(dev);
> -	if (ops->release_device)
> -		ops->release_device(dev);
> -	mutex_unlock(&group->mutex);
> -
> -	/* Pairs with the get in iommu_group_add_device() */
> -	iommu_group_put(group);
> -
> -	module_put(ops->owner);
> -	dev_iommu_free(dev);
>   }

Best regards,
baolu

Re: [PATCH v2 05/10] iommu: Add iommu_init/deinit_device() paired functions

[Baolu Lu]

On 5/21/23 7:31 PM, Baolu Lu wrote:
>>       for_each_group_device(group, device) {
>>           if (device->dev != dev)
>> @@ -510,44 +564,30 @@ static void __iommu_group_remove_device(struct 
>> device *dev)
>>           list_del(&device->list);
>>           __iommu_group_free_device(group, device);
>> -        /* Caller must put iommu_group */
>> -        return;
>> +        if (dev->iommu && dev->iommu->iommu_dev)
>> +            iommu_deinit_device(dev);
>> +        else
>> +            dev->iommu_group = NULL;
>> +        goto out;
>>       }
>>       WARN(true, "Corrupted iommu_group device_list");
>> +out:
>> +    mutex_unlock(&group->mutex);
>> +
>> +    /* Pairs with the get in iommu_group_add_device() */
>> +    iommu_group_put(group);
> 
> The group->devices_kobj was increased on the probe device path twice:
> 
> - iommu_init_device() - allocate the group
> - iommu_group_add_device() - add device to the group
> 
> But, on the release path, it seems that group->devices_kobj is only
> decreased once.
> 
> Did I overlook anything? Otherwise, the group will never be released,
> right?

I can answer this question by myself now. The
iommu_group_add/remove_device() helpers are only for external users.
They are not on the internal probe/release paths.

The code is fine. I can see below debug message during my test:

         pr_debug("Releasing group %d\n", group->id);

Sorry for the noise.

Best regards,
baolu

Re: [PATCH v2 05/10] iommu: Add iommu_init/deinit_device() paired functions

[Jason Gunthorpe]

On Sun, May 21, 2023 at 07:31:38PM +0800, Baolu Lu wrote:
> I revisited this patch. And I still have some questions.
> 
> On 5/20/23 2:42 AM, Jason Gunthorpe wrote:
> > -/*
> > - * Remove the iommu_group from the struct device. The attached group must be put
> > - * by the caller after releaseing the group->mutex.
> > - */
> > +/* Remove the iommu_group from the struct device. */
> >   static void __iommu_group_remove_device(struct device *dev)
> >   {
> >   	struct iommu_group *group = dev->iommu_group;
> >   	struct group_device *device;
> > +	mutex_lock(&group->mutex);
> >   	lockdep_assert_held(&group->mutex);
> 
> By moving mutex_lock/unlock into this helper, above
> lockdep_assert_held() is unnecessary.

Woops, got it thanks

> The group->devices_kobj was increased on the probe device path twice:
> 
> - iommu_init_device() - allocate the group
> - iommu_group_add_device() - add device to the group
> 
> But, on the release path, it seems that group->devices_kobj is only
> decreased once.
> 
> Did I overlook anything? Otherwise, the group will never be released,
> right?

Your answer was right, when VFIO uses add/remove device it doesn't do
init_device.

Jason

[PATCH v2 06/10] iommu: Move the iommu driver sysfs setup into iommu_init/deinit_device()

[Jason Gunthorpe]

It makes logical sense that once the driver is attached to the device the
sysfs links appear, even if we haven't fully created the group_device or
attached the device to a domain.

Fix the missing error handling on sysfs creation since
iommu_init_device() can trivially handle this.

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommu-sysfs.c |  6 ------
 drivers/iommu/iommu.c       | 13 +++++++++----
 2 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/iommu/iommu-sysfs.c b/drivers/iommu/iommu-sysfs.c
index 99869217fbec7d..c8aba0e2a30d70 100644
--- a/drivers/iommu/iommu-sysfs.c
+++ b/drivers/iommu/iommu-sysfs.c
@@ -107,9 +107,6 @@ int iommu_device_link(struct iommu_device *iommu, struct device *link)
 {
 	int ret;
 
-	if (!iommu || IS_ERR(iommu))
-		return -ENODEV;
-
 	ret = sysfs_add_link_to_group(&iommu->dev->kobj, "devices",
 				      &link->kobj, dev_name(link));
 	if (ret)
@@ -126,9 +123,6 @@ EXPORT_SYMBOL_GPL(iommu_device_link);
 
 void iommu_device_unlink(struct iommu_device *iommu, struct device *link)
 {
-	if (!iommu || IS_ERR(iommu))
-		return;
-
 	sysfs_remove_link(&link->kobj, "iommu");
 	sysfs_remove_link_from_group(&iommu->dev->kobj, "devices", dev_name(link));
 }
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 2031cb4782b9b5..69e4227bb7404f 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -356,12 +356,16 @@ static int iommu_init_device(struct device *dev, const struct iommu_ops *ops)
 		goto err_module_put;
 	}
 
+	ret = iommu_device_link(iommu_dev, dev);
+	if (ret)
+		goto err_release;
+
 	group = ops->device_group(dev);
 	if (WARN_ON_ONCE(group == NULL))
 		group = ERR_PTR(-EINVAL);
 	if (IS_ERR(group)) {
 		ret = PTR_ERR(group);
-		goto err_release;
+		goto err_unlink;
 	}
 	dev->iommu_group = group;
 
@@ -371,6 +375,8 @@ static int iommu_init_device(struct device *dev, const struct iommu_ops *ops)
 		dev->iommu->attach_deferred = ops->is_attach_deferred(dev);
 	return 0;
 
+err_unlink:
+	iommu_device_unlink(iommu_dev, dev);
 err_release:
 	if (ops->release_device)
 		ops->release_device(dev);
@@ -388,6 +394,8 @@ static void iommu_deinit_device(struct device *dev)
 
 	lockdep_assert_held(&group->mutex);
 
+	iommu_device_unlink(dev->iommu->iommu_dev, dev);
+
 	/*
 	 * release_device() must stop using any attached domain on the device.
 	 * If there are still other devices in the group they are not effected
@@ -462,7 +470,6 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
 	iommu_group_put(group);
 
 	mutex_unlock(&iommu_probe_device_lock);
-	iommu_device_link(dev->iommu->iommu_dev, dev);
 
 	return 0;
 
@@ -585,8 +592,6 @@ static void iommu_release_device(struct device *dev)
 	if (!dev->iommu || !group)
 		return;
 
-	iommu_device_unlink(dev->iommu->iommu_dev, dev);
-
 	__iommu_group_remove_device(dev);
 }
 
-- 
2.40.1

Re: [PATCH v2 06/10] iommu: Move the iommu driver sysfs setup into iommu_init/deinit_device()

[Baolu Lu]

On 5/20/23 2:42 AM, Jason Gunthorpe wrote:
> It makes logical sense that once the driver is attached to the device the
> sysfs links appear, even if we haven't fully created the group_device or
> attached the device to a domain.
> 
> Fix the missing error handling on sysfs creation since
> iommu_init_device() can trivially handle this.
> 
> Reviewed-by: Kevin Tian<kevin.tian@intel.com>
> Signed-off-by: Jason Gunthorpe<jgg@nvidia.com>

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu

[PATCH v2 07/10] iommu: Do not export iommu_device_link/unlink()

[Jason Gunthorpe]

These are not used outside iommu.c, they should not be available to
modular code.

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommu-sysfs.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/iommu/iommu-sysfs.c b/drivers/iommu/iommu-sysfs.c
index c8aba0e2a30d70..cbe378c34ba3eb 100644
--- a/drivers/iommu/iommu-sysfs.c
+++ b/drivers/iommu/iommu-sysfs.c
@@ -119,11 +119,9 @@ int iommu_device_link(struct iommu_device *iommu, struct device *link)
 
 	return ret;
 }
-EXPORT_SYMBOL_GPL(iommu_device_link);
 
 void iommu_device_unlink(struct iommu_device *iommu, struct device *link)
 {
 	sysfs_remove_link(&link->kobj, "iommu");
 	sysfs_remove_link_from_group(&iommu->dev->kobj, "devices", dev_name(link));
 }
-EXPORT_SYMBOL_GPL(iommu_device_unlink);
-- 
2.40.1

Re: [PATCH v2 07/10] iommu: Do not export iommu_device_link/unlink()

[Baolu Lu]

On 5/20/23 2:42 AM, Jason Gunthorpe wrote:
> These are not used outside iommu.c, they should not be available to
> modular code.
> 
> Reviewed-by: Kevin Tian<kevin.tian@intel.com>
> Signed-off-by: Jason Gunthorpe<jgg@nvidia.com>

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu

[PATCH v2 08/10] iommu: Always destroy the iommu_group during iommu_release_device()

[Jason Gunthorpe]

Have release fully clean up the iommu related parts of the struct device,
no matter what state they are in.

Split the logic so that the three things owned by the iommu core are
always cleaned up:
 - Any attached iommu_group
 - Any allocated dev->iommu and its contents including a fwsepc
 - Any attached driver via a struct group_device

This fixes a minor bug where a fwspec created without an iommu_group being
probed would not be freed.

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommu.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 69e4227bb7404f..6a8cbfd2274770 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -575,10 +575,8 @@ static void __iommu_group_remove_device(struct device *dev)
 			iommu_deinit_device(dev);
 		else
 			dev->iommu_group = NULL;
-		goto out;
+		break;
 	}
-	WARN(true, "Corrupted iommu_group device_list");
-out:
 	mutex_unlock(&group->mutex);
 
 	/* Pairs with the get in iommu_group_add_device() */
@@ -589,10 +587,12 @@ static void iommu_release_device(struct device *dev)
 {
 	struct iommu_group *group = dev->iommu_group;
 
-	if (!dev->iommu || !group)
-		return;
+	if (group)
+		__iommu_group_remove_device(dev);
 
-	__iommu_group_remove_device(dev);
+	/* Free any fwspec if no iommu_driver was ever attached */
+	if (dev->iommu)
+		dev_iommu_free(dev);
 }
 
 static int __init iommu_set_def_domain_type(char *str)
-- 
2.40.1

Re: [PATCH v2 08/10] iommu: Always destroy the iommu_group during iommu_release_device()

[Baolu Lu]

On 5/20/23 2:42 AM, Jason Gunthorpe wrote:
> Have release fully clean up the iommu related parts of the struct device,
> no matter what state they are in.
> 
> Split the logic so that the three things owned by the iommu core are
> always cleaned up:
>   - Any attached iommu_group
>   - Any allocated dev->iommu and its contents including a fwsepc
>   - Any attached driver via a struct group_device
> 
> This fixes a minor bug where a fwspec created without an iommu_group being
> probed would not be freed.
> 
> Reviewed-by: Kevin Tian<kevin.tian@intel.com>
> Signed-off-by: Jason Gunthorpe<jgg@nvidia.com>

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu

[PATCH v2 09/10] iommu: Split iommu_group_add_device()

[Jason Gunthorpe]

Move the list_add_tail() for the group_device into the critical region
that immediately follows in __iommu_probe_device(). This avoids one case
of unlocking and immediately re-locking the group->mutex.

Consistently make the caller responsible for setting dev->iommu_group,
prior patches moved this into iommu_init_device(), make the no-driver path
do this in iommu_group_add_device().

This completes making __iommu_group_free_device() and
iommu_group_alloc_device() into pair'd functions.

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

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 6a8cbfd2274770..0d376a3dbd7a42 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -129,6 +129,8 @@ static int iommu_create_device_direct_mappings(struct iommu_domain *domain,
 					       struct device *dev);
 static ssize_t iommu_group_store_type(struct iommu_group *group,
 				      const char *buf, size_t count);
+static struct group_device *iommu_group_alloc_device(struct iommu_group *group,
+						     struct device *dev);
 
 #define IOMMU_GROUP_ATTR(_name, _mode, _show, _store)		\
 struct iommu_group_attribute iommu_group_attr_##_name =		\
@@ -435,6 +437,7 @@ 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;
 
 	if (!ops)
@@ -459,16 +462,17 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
 		goto out_unlock;
 
 	group = dev->iommu_group;
-	ret = iommu_group_add_device(group, dev);
+	gdev = iommu_group_alloc_device(group, dev);
 	mutex_lock(&group->mutex);
-	if (ret)
+	if (IS_ERR(gdev)) {
+		ret = PTR_ERR(gdev);
 		goto err_put_group;
+	}
 
+	list_add_tail(&gdev->list, &group->devices);
 	if (group_list && !group->default_domain && list_empty(&group->entry))
 		list_add_tail(&group->entry, group_list);
 	mutex_unlock(&group->mutex);
-	iommu_group_put(group);
-
 	mutex_unlock(&iommu_probe_device_lock);
 
 	return 0;
@@ -579,7 +583,10 @@ static void __iommu_group_remove_device(struct device *dev)
 	}
 	mutex_unlock(&group->mutex);
 
-	/* Pairs with the get in iommu_group_add_device() */
+	/*
+	 * Pairs with the get in iommu_init_device() or
+	 * iommu_group_add_device()
+	 */
 	iommu_group_put(group);
 }
 
@@ -1068,22 +1075,16 @@ static int iommu_create_device_direct_mappings(struct iommu_domain *domain,
 	return ret;
 }
 
-/**
- * iommu_group_add_device - add a device to an iommu group
- * @group: the group into which to add the device (reference should be held)
- * @dev: the device
- *
- * This function is called by an iommu driver to add a device into a
- * group.  Adding a device increments the group reference count.
- */
-int iommu_group_add_device(struct iommu_group *group, struct device *dev)
+/* This is undone by __iommu_group_free_device() */
+static struct group_device *iommu_group_alloc_device(struct iommu_group *group,
+						     struct device *dev)
 {
 	int ret, i = 0;
 	struct group_device *device;
 
 	device = kzalloc(sizeof(*device), GFP_KERNEL);
 	if (!device)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
 	device->dev = dev;
 
@@ -1114,17 +1115,11 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)
 		goto err_free_name;
 	}
 
-	iommu_group_ref_get(group);
-	dev->iommu_group = group;
-
-	mutex_lock(&group->mutex);
-	list_add_tail(&device->list, &group->devices);
-	mutex_unlock(&group->mutex);
 	trace_add_device_to_group(group->id, dev);
 
 	dev_info(dev, "Adding to iommu group %d\n", group->id);
 
-	return 0;
+	return device;
 
 err_free_name:
 	kfree(device->name);
@@ -1133,7 +1128,32 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)
 err_free_device:
 	kfree(device);
 	dev_err(dev, "Failed to add to iommu group %d: %d\n", group->id, ret);
-	return ret;
+	return ERR_PTR(ret);
+}
+
+/**
+ * iommu_group_add_device - add a device to an iommu group
+ * @group: the group into which to add the device (reference should be held)
+ * @dev: the device
+ *
+ * This function is called by an iommu driver to add a device into a
+ * group.  Adding a device increments the group reference count.
+ */
+int iommu_group_add_device(struct iommu_group *group, struct device *dev)
+{
+	struct group_device *gdev;
+
+	gdev = iommu_group_alloc_device(group, dev);
+	if (IS_ERR(gdev))
+		return PTR_ERR(gdev);
+
+	iommu_group_ref_get(group);
+	dev->iommu_group = group;
+
+	mutex_lock(&group->mutex);
+	list_add_tail(&gdev->list, &group->devices);
+	mutex_unlock(&group->mutex);
+	return 0;
 }
 EXPORT_SYMBOL_GPL(iommu_group_add_device);
 
-- 
2.40.1

Re: [PATCH v2 09/10] iommu: Split iommu_group_add_device()

[Baolu Lu]

On 5/20/23 2:42 AM, Jason Gunthorpe wrote:
> Move the list_add_tail() for the group_device into the critical region
> that immediately follows in __iommu_probe_device(). This avoids one case
> of unlocking and immediately re-locking the group->mutex.
> 
> Consistently make the caller responsible for setting dev->iommu_group,
> prior patches moved this into iommu_init_device(), make the no-driver path
> do this in iommu_group_add_device().
> 
> This completes making __iommu_group_free_device() and
> iommu_group_alloc_device() into pair'd functions.
> 
> Signed-off-by: Jason Gunthorpe<jgg@nvidia.com>

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu

RE: [PATCH v2 09/10] iommu: Split iommu_group_add_device()

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Saturday, May 20, 2023 2:42 AM
> 
> Move the list_add_tail() for the group_device into the critical region
> that immediately follows in __iommu_probe_device(). This avoids one case
> of unlocking and immediately re-locking the group->mutex.
> 
> Consistently make the caller responsible for setting dev->iommu_group,
> prior patches moved this into iommu_init_device(), make the no-driver path
> do this in iommu_group_add_device().
> 
> This completes making __iommu_group_free_device() and
> iommu_group_alloc_device() into pair'd functions.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

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

[PATCH v2 10/10] iommu: Avoid locking/unlocking for iommu_probe_device()

[Jason Gunthorpe]

Remove the race where a hotplug of a device into an existing group will
have the device installed in the group->devices, but not yet attached to
the group's current domain.

Move the group attachment logic from iommu_probe_device() and put it under
the same mutex that updates the group->devices list so everything is
atomic under the lock.

We retain the two step setup of the default domain for the
bus_iommu_probe() case solely so that we have a more complete view of the
group when creating the default domain for boot time devices. This is not
generally necessary with the current code structure but seems to be
supporting some odd corner cases like alias RID's and IOMMU_RESV_DIRECT or
driver bugs returning different default_domain types for the same group.

During bus_iommu_probe() the group will have a device list but both
group->default_domain and group->domain will be NULL.

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

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 0d376a3dbd7a42..1bc75774626303 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -131,6 +131,8 @@ static ssize_t iommu_group_store_type(struct iommu_group *group,
 				      const char *buf, size_t count);
 static struct group_device *iommu_group_alloc_device(struct iommu_group *group,
 						     struct device *dev);
+static void __iommu_group_free_device(struct iommu_group *group,
+				      struct group_device *grp_dev);
 
 #define IOMMU_GROUP_ATTR(_name, _mode, _show, _store)		\
 struct iommu_group_attribute iommu_group_attr_##_name =		\
@@ -469,14 +471,39 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
 		goto err_put_group;
 	}
 
+	/*
+	 * The gdev must be in the list before calling
+	 * iommu_setup_default_domain()
+	 */
 	list_add_tail(&gdev->list, &group->devices);
-	if (group_list && !group->default_domain && list_empty(&group->entry))
-		list_add_tail(&group->entry, group_list);
+	WARN_ON(group->default_domain && !group->domain);
+	if (group->default_domain)
+		iommu_create_device_direct_mappings(group->default_domain, dev);
+	if (group->domain) {
+		ret = __iommu_device_set_domain(group, dev, group->domain, 0);
+		if (ret)
+			goto err_remove_gdev;
+	} else if (!group->default_domain && !group_list) {
+		ret = iommu_setup_default_domain(group, 0);
+		if (ret)
+			goto err_remove_gdev;
+	} else if (!group->default_domain) {
+		/*
+		 * With a group_list argument we defer the default_domain setup
+		 * to the caller by providing a de-duplicated list of groups
+		 * that need further setup.
+		 */
+		if (list_empty(&group->entry))
+			list_add_tail(&group->entry, group_list);
+	}
 	mutex_unlock(&group->mutex);
 	mutex_unlock(&iommu_probe_device_lock);
 
 	return 0;
 
+err_remove_gdev:
+	list_del(&gdev->list);
+	__iommu_group_free_device(group, gdev);
 err_put_group:
 	iommu_deinit_device(dev);
 	mutex_unlock(&group->mutex);
@@ -490,52 +517,17 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
 int iommu_probe_device(struct device *dev)
 {
 	const struct iommu_ops *ops;
-	struct iommu_group *group;
 	int ret;
 
 	ret = __iommu_probe_device(dev, NULL);
 	if (ret)
-		goto err_out;
-
-	group = iommu_group_get(dev);
-	if (!group) {
-		ret = -ENODEV;
-		goto err_release;
-	}
-
-	mutex_lock(&group->mutex);
-
-	if (group->default_domain)
-		iommu_create_device_direct_mappings(group->default_domain, dev);
-
-	if (group->domain) {
-		ret = __iommu_device_set_domain(group, dev, group->domain, 0);
-		if (ret)
-			goto err_unlock;
-	} else if (!group->default_domain) {
-		ret = iommu_setup_default_domain(group, 0);
-		if (ret)
-			goto err_unlock;
-	}
-
-	mutex_unlock(&group->mutex);
-	iommu_group_put(group);
+		return ret;
 
 	ops = dev_iommu_ops(dev);
 	if (ops->probe_finalize)
 		ops->probe_finalize(dev);
 
 	return 0;
-
-err_unlock:
-	mutex_unlock(&group->mutex);
-	iommu_group_put(group);
-err_release:
-	iommu_release_device(dev);
-
-err_out:
-	return ret;
-
 }
 
 static void __iommu_group_free_device(struct iommu_group *group,
@@ -1816,11 +1808,6 @@ int bus_iommu_probe(const struct bus_type *bus)
 	LIST_HEAD(group_list);
 	int ret;
 
-	/*
-	 * This code-path does not allocate the default domain when
-	 * creating the iommu group, so do it after the groups are
-	 * created.
-	 */
 	ret = bus_for_each_dev(bus, NULL, &group_list, probe_iommu_group);
 	if (ret)
 		return ret;
@@ -1833,6 +1820,11 @@ int bus_iommu_probe(const struct bus_type *bus)
 		/* Remove item from the list */
 		list_del_init(&group->entry);
 
+		/*
+		 * We go to the trouble of deferred default domain creation so
+		 * that the cross-group default domain type and the setup of the
+		 * IOMMU_RESV_DIRECT will work correctly in non-hotpug scenarios.
+		 */
 		ret = iommu_setup_default_domain(group, 0);
 		if (ret) {
 			mutex_unlock(&group->mutex);
-- 
2.40.1

Re: [PATCH v2 10/10] iommu: Avoid locking/unlocking for iommu_probe_device()

[Baolu Lu]

On 5/20/23 2:42 AM, Jason Gunthorpe wrote:
> Remove the race where a hotplug of a device into an existing group will
> have the device installed in the group->devices, but not yet attached to
> the group's current domain.
> 
> Move the group attachment logic from iommu_probe_device() and put it under
> the same mutex that updates the group->devices list so everything is
> atomic under the lock.
> 
> We retain the two step setup of the default domain for the
> bus_iommu_probe() case solely so that we have a more complete view of the
> group when creating the default domain for boot time devices. This is not
> generally necessary with the current code structure but seems to be
> supporting some odd corner cases like alias RID's and IOMMU_RESV_DIRECT or
> driver bugs returning different default_domain types for the same group.
> 
> During bus_iommu_probe() the group will have a device list but both
> group->default_domain and group->domain will be NULL.
> 
> Signed-off-by: Jason Gunthorpe<jgg@nvidia.com>

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Best regards,
baolu

RE: [PATCH v2 10/10] iommu: Avoid locking/unlocking for iommu_probe_device()

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Saturday, May 20, 2023 2:42 AM
> 
> Remove the race where a hotplug of a device into an existing group will
> have the device installed in the group->devices, but not yet attached to
> the group's current domain.
> 
> Move the group attachment logic from iommu_probe_device() and put it
> under
> the same mutex that updates the group->devices list so everything is
> atomic under the lock.
> 
> We retain the two step setup of the default domain for the
> bus_iommu_probe() case solely so that we have a more complete view of
> the
> group when creating the default domain for boot time devices. This is not
> generally necessary with the current code structure but seems to be
> supporting some odd corner cases like alias RID's and IOMMU_RESV_DIRECT
> or
> driver bugs returning different default_domain types for the same group.
> 
> During bus_iommu_probe() the group will have a device list but both
> group->default_domain and group->domain will be NULL.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

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