[PATCH][v2] iommu: move the adding group info to proper place

[PATCH][v2] iommu: move the adding group info to proper place

[Li RongQing]

After commit fa0828036488 ("iommu: Split iommu_group_add_device()"), the
adding group information is left in iommu_group_alloc_device, but which
only allocate group device, not attach the device to iommu group;

so move the print to iommu probing and adding functions

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
Signed-off-by: Li RongQing <lirongqing@baidu.com>
---
Diff with v1: add log when iommu_group_alloc_device failed in iommu_group_add_device

 drivers/iommu/iommu.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 599030e..2eaa8ab 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -586,6 +586,9 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
 
 	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;
 
 err_remove_gdev:
@@ -594,6 +597,7 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
 err_put_group:
 	iommu_deinit_device(dev);
 	mutex_unlock(&group->mutex);
+	dev_err(dev, "Failed to add to iommu group %d: %d\n", group->id, ret);
 	iommu_group_put(group);
 
 	return ret;
@@ -1192,10 +1196,6 @@ static struct group_device *iommu_group_alloc_device(struct iommu_group *group,
 		goto err_free_name;
 	}
 
-	trace_add_device_to_group(group->id, dev);
-
-	dev_info(dev, "Adding to iommu group %d\n", group->id);
-
 	return device;
 
 err_free_name:
@@ -1204,7 +1204,6 @@ static struct group_device *iommu_group_alloc_device(struct iommu_group *group,
 	sysfs_remove_link(&dev->kobj, "iommu_group");
 err_free_device:
 	kfree(device);
-	dev_err(dev, "Failed to add to iommu group %d: %d\n", group->id, ret);
 	return ERR_PTR(ret);
 }
 
@@ -1219,10 +1218,14 @@ static struct group_device *iommu_group_alloc_device(struct iommu_group *group,
 int iommu_group_add_device(struct iommu_group *group, struct device *dev)
 {
 	struct group_device *gdev;
+	int ret;
 
 	gdev = iommu_group_alloc_device(group, dev);
-	if (IS_ERR(gdev))
-		return PTR_ERR(gdev);
+	if (IS_ERR(gdev)) {
+		ret = PTR_ERR(gdev);
+		dev_err(dev, "Failed to add to iommu group %d: %d\n", group->id, ret);
+		return ret;
+	}
 
 	iommu_group_ref_get(group);
 	dev->iommu_group = group;
@@ -1230,6 +1233,10 @@ 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);
+
+	trace_add_device_to_group(group->id, dev);
+	dev_info(dev, "Adding to iommu group %d\n", group->id);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(iommu_group_add_device);
-- 
2.9.4

Re: [PATCH][v2] iommu: move the adding group info to proper place

[Pranjal Shrivastava]

On Tue, Jan 07, 2025 at 11:35:58AM +0800, Li RongQing wrote:
> After commit fa0828036488 ("iommu: Split iommu_group_add_device()"), the
> adding group information is left in iommu_group_alloc_device, but which
> only allocate group device, not attach the device to iommu group;
> 
> so move the print to iommu probing and adding functions
> 
> Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
> Signed-off-by: Li RongQing <lirongqing@baidu.com>

LGTM, followed through since [1].
Acked-by: Pranjal Shrivastava <praan@google.com>

Thanks,
Praan

[1]
 https://lore.kernel.org/all/c30af43b93f04125bf9befe631922653@baidu.com/

Re: [PATCH][v2] iommu: move the adding group info to proper place

[Vasant Hegde]

Hi Li,


On 1/7/2025 9:05 AM, Li RongQing wrote:
> After commit fa0828036488 ("iommu: Split iommu_group_add_device()"), the
> adding group information is left in iommu_group_alloc_device, but which
> only allocate group device, not attach the device to iommu group;
> 
> so move the print to iommu probing and adding functions
> 
> Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> ---
> Diff with v1: add log when iommu_group_alloc_device failed in iommu_group_add_device
> 
>  drivers/iommu/iommu.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 599030e..2eaa8ab 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -586,6 +586,9 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
>  
>  	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;
>  
>  err_remove_gdev:
> @@ -594,6 +597,7 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
>  err_put_group:
>  	iommu_deinit_device(dev);
>  	mutex_unlock(&group->mutex);
> +	dev_err(dev, "Failed to add to iommu group %d: %d\n", group->id, ret);
>  	iommu_group_put(group);
>  
>  	return ret;
> @@ -1192,10 +1196,6 @@ static struct group_device *iommu_group_alloc_device(struct iommu_group *group,
>  		goto err_free_name;
>  	}
>  
> -	trace_add_device_to_group(group->id, dev);
> -
> -	dev_info(dev, "Adding to iommu group %d\n", group->id);
> -
>  	return device;
>  
>  err_free_name:
> @@ -1204,7 +1204,6 @@ static struct group_device *iommu_group_alloc_device(struct iommu_group *group,
>  	sysfs_remove_link(&dev->kobj, "iommu_group");
>  err_free_device:
>  	kfree(device);
> -	dev_err(dev, "Failed to add to iommu group %d: %d\n", group->id, ret);
>  	return ERR_PTR(ret);
>  }
>  
> @@ -1219,10 +1218,14 @@ static struct group_device *iommu_group_alloc_device(struct iommu_group *group,
>  int iommu_group_add_device(struct iommu_group *group, struct device *dev)
>  {
>  	struct group_device *gdev;
> +	int ret;
>  
>  	gdev = iommu_group_alloc_device(group, dev);
> -	if (IS_ERR(gdev))
> -		return PTR_ERR(gdev);
> +	if (IS_ERR(gdev)) {
> +		ret = PTR_ERR(gdev);
> +		dev_err(dev, "Failed to add to iommu group %d: %d\n", group->id, ret);
> +		return ret;

Why not just return PTR_ERR(gdev)?

-Vasant

答复: [外部邮件] Re: [PATCH][v2] iommu: move the adding group info to proper place

[Li,Rongqing]

> > -	if (IS_ERR(gdev))
> > -		return PTR_ERR(gdev);
> > +	if (IS_ERR(gdev)) {
> > +		ret = PTR_ERR(gdev);
> > +		dev_err(dev, "Failed to add to iommu group %d: %d\n", group->id,
> ret);
> > +		return ret;
> 
> Why not just return PTR_ERR(gdev)?
> 

Since dev_err will print PTR_ERR(gdev), so I define a local variable to conversion once


Thanks

-Li

> -Vasant

Re: 答复: [外部邮件] Re: [PATCH][v2] iommu: move the adding group info to proper place

[Vasant Hegde]

Hi Li,


On 1/7/2025 6:00 PM, Li,Rongqing wrote:
>>> -	if (IS_ERR(gdev))
>>> -		return PTR_ERR(gdev);
>>> +	if (IS_ERR(gdev)) {
>>> +		ret = PTR_ERR(gdev);
>>> +		dev_err(dev, "Failed to add to iommu group %d: %d\n", group->id,
>> ret);
>>> +		return ret;
>>
>> Why not just return PTR_ERR(gdev)?
>>
> 
> Since dev_err will print PTR_ERR(gdev), so I define a local variable to conversion once

Ok. Thanks

Reviewed-by: Vasant Hegde <vasant.hegde@amd.com>

-Vasant

答复: [PATCH][v2] iommu: move the adding group info to proper place

[Li,Rongqing]

> After commit fa0828036488 ("iommu: Split iommu_group_add_device()"), the
> adding group information is left in iommu_group_alloc_device, but which only
> allocate group device, not attach the device to iommu group;
> 
> so move the print to iommu probing and adding functions
> 
> Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
> Signed-off-by: Li RongQing <lirongqing@baidu.com>



Ping

Thanks

-LiRongQing


> ---
> Diff with v1: add log when iommu_group_alloc_device failed in
> iommu_group_add_device
> 
>  drivers/iommu/iommu.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index
> 599030e..2eaa8ab 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -586,6 +586,9 @@ static int __iommu_probe_device(struct device *dev,
> struct list_head *group_list
> 
>  	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;
> 
>  err_remove_gdev:
> @@ -594,6 +597,7 @@ static int __iommu_probe_device(struct device *dev,
> struct list_head *group_list
>  err_put_group:
>  	iommu_deinit_device(dev);
>  	mutex_unlock(&group->mutex);
> +	dev_err(dev, "Failed to add to iommu group %d: %d\n", group->id, ret);
>  	iommu_group_put(group);
> 
>  	return ret;
> @@ -1192,10 +1196,6 @@ static struct group_device
> *iommu_group_alloc_device(struct iommu_group *group,
>  		goto err_free_name;
>  	}
> 
> -	trace_add_device_to_group(group->id, dev);
> -
> -	dev_info(dev, "Adding to iommu group %d\n", group->id);
> -
>  	return device;
> 
>  err_free_name:
> @@ -1204,7 +1204,6 @@ static struct group_device
> *iommu_group_alloc_device(struct iommu_group *group,
>  	sysfs_remove_link(&dev->kobj, "iommu_group");
>  err_free_device:
>  	kfree(device);
> -	dev_err(dev, "Failed to add to iommu group %d: %d\n", group->id, ret);
>  	return ERR_PTR(ret);
>  }
> 
> @@ -1219,10 +1218,14 @@ static struct group_device
> *iommu_group_alloc_device(struct iommu_group *group,  int
> iommu_group_add_device(struct iommu_group *group, struct device *dev)  {
>  	struct group_device *gdev;
> +	int ret;
> 
>  	gdev = iommu_group_alloc_device(group, dev);
> -	if (IS_ERR(gdev))
> -		return PTR_ERR(gdev);
> +	if (IS_ERR(gdev)) {
> +		ret = PTR_ERR(gdev);
> +		dev_err(dev, "Failed to add to iommu group %d: %d\n", group->id,
> ret);
> +		return ret;
> +	}
> 
>  	iommu_group_ref_get(group);
>  	dev->iommu_group = group;
> @@ -1230,6 +1233,10 @@ 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);
> +
> +	trace_add_device_to_group(group->id, dev);
> +	dev_info(dev, "Adding to iommu group %d\n", group->id);
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(iommu_group_add_device);
> --
> 2.9.4

Re: [PATCH][v2] iommu: move the adding group info to proper place

[Robin Murphy]

On 2025-01-07 3:35 am, Li RongQing wrote:
> After commit fa0828036488 ("iommu: Split iommu_group_add_device()"), the
> adding group information is left in iommu_group_alloc_device, but which
> only allocate group device, not attach the device to iommu group;
> 
> so move the print to iommu probing and adding functions

But why? In what way is it justifiably better to have duplicate copies 
of the same code when the fact is that the only parts of the 
iommu_group_add_device() operation which can fail *are* the ones inside 
iommu_group_alloc_device()?

> Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> ---
> Diff with v1: add log when iommu_group_alloc_device failed in iommu_group_add_device
> 
>   drivers/iommu/iommu.c | 21 ++++++++++++++-------
>   1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 599030e..2eaa8ab 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -586,6 +586,9 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
>   
>   	mutex_unlock(&group->mutex);
>   
> +	trace_add_device_to_group(group->id, dev);
> +	dev_info(dev, "Adding to iommu group %d\n", group->id);

If you want to argue semantics, this is now just as much in the wrong 
place as you claim the current error message to be: by this point we are 
no longer, as implied, about to add the device to the group, we've 
already finished doing that and potentially various other subsequent 
things too.

> +
>   	return 0;
>   
>   err_remove_gdev:
> @@ -594,6 +597,7 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
>   err_put_group:
>   	iommu_deinit_device(dev);
>   	mutex_unlock(&group->mutex);
> +	dev_err(dev, "Failed to add to iommu group %d: %d\n", group->id, ret);

This is inaccurate, because we can also get here for further error 
reasons after the device technically *was* successfully added to the 
group (in terms of the nominal iommu_group_add_device() operation within 
the wider "probe device" functionality here).

>   	iommu_group_put(group);
>   
>   	return ret;
> @@ -1192,10 +1196,6 @@ static struct group_device *iommu_group_alloc_device(struct iommu_group *group,
>   		goto err_free_name;
>   	}
>   
> -	trace_add_device_to_group(group->id, dev);
> -
> -	dev_info(dev, "Adding to iommu group %d\n", group->id);
> -
>   	return device;
>   
>   err_free_name:
> @@ -1204,7 +1204,6 @@ static struct group_device *iommu_group_alloc_device(struct iommu_group *group,
>   	sysfs_remove_link(&dev->kobj, "iommu_group");
>   err_free_device:
>   	kfree(device);
> -	dev_err(dev, "Failed to add to iommu group %d: %d\n", group->id, ret);
>   	return ERR_PTR(ret);
>   }
>   
> @@ -1219,10 +1218,14 @@ static struct group_device *iommu_group_alloc_device(struct iommu_group *group,
>   int iommu_group_add_device(struct iommu_group *group, struct device *dev)
>   {
>   	struct group_device *gdev;
> +	int ret;
>   
>   	gdev = iommu_group_alloc_device(group, dev);
> -	if (IS_ERR(gdev))
> -		return PTR_ERR(gdev);
> +	if (IS_ERR(gdev)) {
> +		ret = PTR_ERR(gdev);
> +		dev_err(dev, "Failed to add to iommu group %d: %d\n", group->id, ret);
> +		return ret;
> +	}
>   
>   	iommu_group_ref_get(group);
>   	dev->iommu_group = group;
> @@ -1230,6 +1233,10 @@ 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);
> +
> +	trace_add_device_to_group(group->id, dev);
> +	dev_info(dev, "Adding to iommu group %d\n", group->id);

Again, this one is now semantically wrong too.

Thanks,
Robin.

> +
>   	return 0;
>   }
>   EXPORT_SYMBOL_GPL(iommu_group_add_device);