From: Jason Gunthorpe jgg@nvidia.com Sent: Wednesday, March 8, 2023 8:36 AM
@@ -379,52 +388,57 @@ static int iommufd_device_auto_get_domain(struct iommufd_device *idev,
if (!iommufd_lock_obj(&hwpt->obj)) continue;
rc = iommufd_device_do_attach(idev, hwpt);iommufd_put_object(&hwpt->obj);/** -EINVAL means the domain is incompatible with the device.* Other error codes should propagate to userspace as failure.* Success means the domain is attached.*/if (rc == -EINVAL)continue;
destroy_hwpt = (*do_attach)(idev, hwpt);
only when succeed?
iommufd_put_object(&hwpt->obj);if (IS_ERR(destroy_hwpt)) {/** -EINVAL means the domain is incompatible withthe
* device. Other error codes should propagate to* userspace as failure. Success means the domain is* attached.*/if (PTR_ERR(destroy_hwpt) == -EINVAL)continue;goto out_unlock;}
two goto's can be merged, if you still want to keep pt_id assignment in original place.
}
- hwpt = iommufd_hw_pagetable_alloc(idev->ictx, ioas, idev, true);
- hwpt = iommufd_hw_pagetable_alloc(idev->ictx, ioas, idev,
immediate_attach);
rc = PTR_ERR(hwpt);
destroy_hwpt = ERR_CAST(hwpt);- if (!immediate_attach) {
destroy_hwpt = (*do_attach)(idev, hwpt);if (IS_ERR(destroy_hwpt))goto out_abort;- } else {
destroy_hwpt = NULL;- }
Above is a bit confusing.
On one hand we have immediate_attach for drivers which must complete attach before we can add the domain to iopt. From this angle it should always be set when calling iommufd_hw_pagetable_alloc() no matter it's attach or replace.
On the other hand we assume *replace* doesn't work with driver which requires immediate_attach so it's done outside of iommufd_hw_pagetable_alloc().
I'm unsure any better way of handling this transition phase, but at least some comment would be useful in this part.