Re: [PATCH v5 3/9] iommufd: Move igroup allocation to a function

[Baolu Lu <baolu.lu@linux.intel.com>]

On 5/12/26 02:41, Jacob Pan wrote:
> From: Jason Gunthorpe <jgg@nvidia.com>
> 
> So it can be reused in the next patch which allows binding to noiommu
> device.
> 
> Reviewed-by: Samiullah Khawaja <skhawaja@google.com>
> Reviewed-by: Yi Liu <yi.l.liu@intel.com>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Jacob Pan <jacob.pan@linux.microsoft.com>
> ---
> v5:
>     - Add NULL group to the error handling path of
>       iommufd_group_setup_msi()

This logic is not visible in the patch's diff. Perhaps anything I
overlooked?

> v3:
>     - New patch
> ---
>   drivers/iommu/iommufd/device.c | 43 +++++++++++++++++++++-------------
>   1 file changed, 27 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
> index 170a7005f0bc..d03076fcf3c2 100644
> --- a/drivers/iommu/iommufd/device.c
> +++ b/drivers/iommu/iommufd/device.c
> @@ -56,6 +56,30 @@ static bool iommufd_group_try_get(struct iommufd_group *igroup,
>   	return kref_get_unless_zero(&igroup->ref);
>   }
>   
> +static struct iommufd_group *iommufd_alloc_group(struct iommufd_ctx *ictx,
> +						 struct iommu_group *group)
> +{
> +	struct iommufd_group *new_igroup;
> +
> +	new_igroup = kzalloc(sizeof(*new_igroup), GFP_KERNEL);

kzalloc_obj() is replaced with kzalloc(). Can you please explain the
reason?

> +	if (!new_igroup)
> +		return ERR_PTR(-ENOMEM);
> +
> +	kref_init(&new_igroup->ref);
> +	mutex_init(&new_igroup->lock);
> +	xa_init(&new_igroup->pasid_attach);
> +	new_igroup->sw_msi_start = PHYS_ADDR_MAX;
> +	/* group reference moves into new_igroup */
> +	new_igroup->group = group;
> +
> +	/*
> +	 * The ictx is not additionally refcounted here because all objects using
> +	 * an igroup must put it before their destroy completes.
> +	 */
> +	new_igroup->ictx = ictx;
> +	return new_igroup;
> +}
> +
>   /*
>    * iommufd needs to store some more data for each iommu_group, we keep a
>    * parallel xarray indexed by iommu_group id to hold this instead of putting it
> @@ -87,25 +111,12 @@ static struct iommufd_group *iommufd_get_group(struct iommufd_ctx *ictx,
>   	}
>   	xa_unlock(&ictx->groups);
>   
> -	new_igroup = kzalloc_obj(*new_igroup);
> -	if (!new_igroup) {
> +	new_igroup = iommufd_alloc_group(ictx, group);
> +	if (IS_ERR(new_igroup)) {
>   		iommu_group_put(group);
> -		return ERR_PTR(-ENOMEM);
> +		return new_igroup;
>   	}
>   
> -	kref_init(&new_igroup->ref);
> -	mutex_init(&new_igroup->lock);
> -	xa_init(&new_igroup->pasid_attach);
> -	new_igroup->sw_msi_start = PHYS_ADDR_MAX;
> -	/* group reference moves into new_igroup */
> -	new_igroup->group = group;
> -
> -	/*
> -	 * The ictx is not additionally refcounted here becase all objects using
> -	 * an igroup must put it before their destroy completes.
> -	 */
> -	new_igroup->ictx = ictx;
> -
>   	/*
>   	 * We dropped the lock so igroup is invalid. NULL is a safe and likely
>   	 * value to assume for the xa_cmpxchg algorithm.

Otherwise this patch looks good to me,

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