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

2023-04-20 Thread Jason Gunthorpe
On Thu, Apr 20, 2023 at 12:25:11PM +0800, Baolu Lu wrote:
> On 4/20/23 12:11 AM, Jason Gunthorpe wrote:
> > @@ -451,16 +454,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(>mutex);
> > -   if (ret)
> > +   if (IS_ERR(gdev)) {
> > +   ret = PTR_ERR(gdev);
> > goto err_put_group;
> > +   }
> > +   list_add_tail(>list, >devices);
> 
> Do we need to put
> 
>   dev->iommu_group = group;
> 
> here?

It is done in iommu_init_driver() and iommu_deinit_driver() NULL's it

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_unlink;
}
dev->iommu_group = group;

Jason


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

2023-04-19 Thread Baolu Lu

On 4/20/23 12:11 AM, Jason Gunthorpe wrote:

@@ -451,16 +454,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(>mutex);
-   if (ret)
+   if (IS_ERR(gdev)) {
+   ret = PTR_ERR(gdev);
goto err_put_group;
+   }
  
+	list_add_tail(>list, >devices);


Do we need to put

dev->iommu_group = group;

here?


if (group_list && !group->default_domain && list_empty(>entry))
list_add_tail(>entry, group_list);
mutex_unlock(>mutex);
-   iommu_group_put(group);
-
mutex_unlock(_probe_device_lock);
  
  	return 0;


Best regards,
baolu


[PATCH 10/11] iommu: Split iommu_group_add_device()

2023-04-19 Thread 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_driver(), 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 
---
 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 a82516c8ea87ad..5ebff82041f2d1 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -128,6 +128,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 =\
@@ -427,6 +429,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)
@@ -451,16 +454,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(>mutex);
-   if (ret)
+   if (IS_ERR(gdev)) {
+   ret = PTR_ERR(gdev);
goto err_put_group;
+   }
 
+   list_add_tail(>list, >devices);
if (group_list && !group->default_domain && list_empty(>entry))
list_add_tail(>entry, group_list);
mutex_unlock(>mutex);
-   iommu_group_put(group);
-
mutex_unlock(_probe_device_lock);
 
return 0;
@@ -572,7 +576,10 @@ static void __iommu_group_remove_device(struct device *dev)
 out:
mutex_unlock(>mutex);
 
-   /* Pairs with the get in iommu_group_add_device() */
+   /*
+* Pairs with the get in iommu_init_driver() or
+* iommu_group_add_device()
+*/
iommu_group_put(group);
 }
 
@@ -1061,22 +1068,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;
 
@@ -1107,17 +1108,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(>mutex);
-   list_add_tail(>list, >devices);
-   mutex_unlock(>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);
@@ -1126,7 +1121,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 =