> From: Liu, Yi L <yi.l.liu@xxxxxxxxx> > Sent: Tuesday, April 16, 2024 5:25 PM > > On 2024/4/16 17:01, Tian, Kevin wrote: > >> From: Liu, Yi L <yi.l.liu@xxxxxxxxx> > >> Sent: Friday, April 12, 2024 4:21 PM > >> > >> + > >> + rc = ida_get_lowest(&vdev->pasids, pasid, pasid); > >> + if (rc == pasid) > >> + return iommufd_device_pasid_replace(vdev- > >>> iommufd_device, > >> + pasid, pt_id); > >> + > >> + rc = iommufd_device_pasid_attach(vdev->iommufd_device, pasid, > >> pt_id); > >> + if (rc) > >> + return rc; > >> + > >> + rc = ida_alloc_range(&vdev->pasids, pasid, pasid, GFP_KERNEL); > >> + if (rc < 0) { > >> + iommufd_device_pasid_detach(vdev->iommufd_device, > >> pasid); > >> + return rc; > >> + } > > > > I'd do simple operation (ida_alloc_range()) first before doing attach. > > > > But that means we rely on the ida_alloc_range() to return -ENOSPC to > indicate the pasid is allocated, hence this attach is actually a > replacement. This is easy to be broken if ida_alloc_range() returns > -ENOSPC for other reasons in future. > ida_alloc_range() could fail for other reasons e.g. -ENOMEM. in case I didn't make it clear I just meant to swap the order between iommufd_device_pasid_attach() and ida_alloc_range(). replacement is still checked against ida_get_lowest().