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

[Robin Murphy <robin.murphy@arm.com>]

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