Re: [PATCH v5 4/9] iommufd: Allow binding to a noiommu device

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

On 5/14/26 06:08, Jacob Pan wrote:
>>> +}
>>> +
>>>    static void iommufd_group_release(struct kref *kref)
>>>    {
>>>    	struct iommufd_group *igroup =
>>> @@ -30,9 +40,11 @@ static void iommufd_group_release(struct kref
>>> *kref)
>>>    	WARN_ON(!xa_empty(&igroup->pasid_attach));
>>>    
>>> -	xa_cmpxchg(&igroup->ictx->groups,
>>> iommu_group_id(igroup->group), igroup,
>>> -		   NULL, GFP_KERNEL);
>>> -	iommu_group_put(igroup->group);
>>> +	if (igroup->group) {
>>> +		xa_cmpxchg(&igroup->ictx->groups,
>>> iommu_group_id(igroup->group),
>>> +			   igroup, NULL, GFP_KERNEL);
>>> +		iommu_group_put(igroup->group);
>>> +	}
>>>    	mutex_destroy(&igroup->lock);
>>>    	kfree(igroup);
>>>    }
>>> @@ -204,32 +216,19 @@ void iommufd_device_destroy(struct
>>> iommufd_object *obj) struct iommufd_device *idev =
>>>    		container_of(obj, struct iommufd_device, obj);
>>>    
>>> -	iommu_device_release_dma_owner(idev->dev);
>>> +	if (!idev->igroup)
>>> +		return;
>> I don't quite follow this logic. Is this check being added
>> specifically for the new noiommu mode? Since the noiommu mode
>> introduces the convention that "igroup->group == NULL" implies
>> noiommu, there should be no cases where idev->igroup itself is NULL.
>>
> idev->igroup can be null on error path only, not limited to noiommu. It
> is the partially initialized idev case.
> /*
>   * iommufd_device_destroy() handles partially initialized idev,
>   * so iommufd_object_abort_and_destroy() is safe to call here.
>   */
> 
> How about add this comment?
>   /* igroup is NULL when destroy called during bind error cleanup */

If it's not limited to noiommu, why not making it in a separated patch?

Thanks,
baolu