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

[Jacob Pan]

Hi Baolu,

On Thu, 14 May 2026 14:51:01 +0800
Baolu Lu <baolu.lu@xxxxxxxxxxxxxxx> wrote:

> 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?
> 
Because the error handling is restructured by this patch to use
iommufd_object_abort_and_destroy() on any failure, which calls
iommufd_device_destroy(). Since idev->igroup is now set inside
iommufd_bind_iommu()/iommufd_bind_noiommu(), a failure before those
succeed (or within them before the assignment) leaves idev->igroup ==
NULL. Hence the new null check applies to both iommu and noiommu but
only needed because of this patch.

Before this patch, iommufd_device_destroy was only reachable after bind
fully succeeded. i.e. no partial init idev.