* Re: [PATCH][v2] iommu: move the adding group info to proper place
2025-01-07 3:35 [PATCH][v2] iommu: move the adding group info to proper place Li RongQing
@ 2025-01-07 4:00 ` Pranjal Shrivastava
2025-01-07 12:22 ` Vasant Hegde
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Pranjal Shrivastava @ 2025-01-07 4:00 UTC (permalink / raw)
To: Li RongQing; +Cc: joro, will, robin.murphy, iommu, baolu.lu, vasant.hegde
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/
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH][v2] iommu: move the adding group info to proper place
2025-01-07 3:35 [PATCH][v2] iommu: move the adding group info to proper place Li RongQing
2025-01-07 4:00 ` Pranjal Shrivastava
@ 2025-01-07 12:22 ` Vasant Hegde
2025-01-07 12:30 ` 答复: [外部邮件] " Li,Rongqing
2025-02-11 10:14 ` 答复: " Li,Rongqing
2025-02-11 17:51 ` Robin Murphy
3 siblings, 1 reply; 7+ messages in thread
From: Vasant Hegde @ 2025-01-07 12:22 UTC (permalink / raw)
To: Li RongQing, joro, will, robin.murphy, iommu, baolu.lu
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
^ permalink raw reply [flat|nested] 7+ messages in thread* 答复: [外部邮件] Re: [PATCH][v2] iommu: move the adding group info to proper place
2025-01-07 12:22 ` Vasant Hegde
@ 2025-01-07 12:30 ` Li,Rongqing
2025-01-07 13:46 ` Vasant Hegde
0 siblings, 1 reply; 7+ messages in thread
From: Li,Rongqing @ 2025-01-07 12:30 UTC (permalink / raw)
To: Vasant Hegde, joro@8bytes.org, will@kernel.org,
robin.murphy@arm.com, iommu@lists.linux.dev,
baolu.lu@linux.intel.com
> > - 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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: 答复: [外部邮件] Re: [PATCH][v2] iommu: move the adding group info to proper place
2025-01-07 12:30 ` 答复: [外部邮件] " Li,Rongqing
@ 2025-01-07 13:46 ` Vasant Hegde
0 siblings, 0 replies; 7+ messages in thread
From: Vasant Hegde @ 2025-01-07 13:46 UTC (permalink / raw)
To: Li,Rongqing, joro@8bytes.org, will@kernel.org,
robin.murphy@arm.com, iommu@lists.linux.dev,
baolu.lu@linux.intel.com
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* 答复: [PATCH][v2] iommu: move the adding group info to proper place
2025-01-07 3:35 [PATCH][v2] iommu: move the adding group info to proper place Li RongQing
2025-01-07 4:00 ` Pranjal Shrivastava
2025-01-07 12:22 ` Vasant Hegde
@ 2025-02-11 10:14 ` Li,Rongqing
2025-02-11 17:51 ` Robin Murphy
3 siblings, 0 replies; 7+ messages in thread
From: Li,Rongqing @ 2025-02-11 10:14 UTC (permalink / raw)
To: joro@8bytes.org, will@kernel.org, robin.murphy@arm.com,
iommu@lists.linux.dev, baolu.lu@linux.intel.com,
vasant.hegde@amd.com
> 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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH][v2] iommu: move the adding group info to proper place
2025-01-07 3:35 [PATCH][v2] iommu: move the adding group info to proper place Li RongQing
` (2 preceding siblings ...)
2025-02-11 10:14 ` 答复: " Li,Rongqing
@ 2025-02-11 17:51 ` Robin Murphy
3 siblings, 0 replies; 7+ messages in thread
From: Robin Murphy @ 2025-02-11 17:51 UTC (permalink / raw)
To: Li RongQing, joro, will, iommu, baolu.lu, vasant.hegde
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);
^ permalink raw reply [flat|nested] 7+ messages in thread