On Tue, Feb 28, 2023 at 09:50:52AM +0800, Baolu Lu wrote: > On 2/27/23 9:58 PM, Jason Gunthorpe wrote: > > On Sun, Feb 26, 2023 at 11:01:59AM +0800, Baolu Lu wrote: > > > On 2/25/23 8:27 AM, Jason Gunthorpe wrote: > > > > @@ -437,25 +517,77 @@ int iommufd_device_attach(struct iommufd_device *idev, u32 *pt_id) > > > > struct iommufd_ioas *ioas = > > > > container_of(pt_obj, struct iommufd_ioas, obj); > > > > - rc = iommufd_device_auto_get_domain(idev, ioas, pt_id); > > > > - if (rc) > > > > + destroy_hwpt = iommufd_device_auto_get_domain(idev, ioas, pt_id, > > > > + do_attach); > > > > + if (IS_ERR(destroy_hwpt)) > > > > goto out_put_pt_obj; > > > > break; > > > > } > > > > default: > > > > - rc = -EINVAL; > > > > + destroy_hwpt = ERR_PTR(-EINVAL); > > > > goto out_put_pt_obj; > > > > } > > > > + iommufd_put_object(pt_obj); > > > > - refcount_inc(&idev->obj.users); > > > > - rc = 0; > > > > + /* This destruction has to be after we unlock everything */ > > > > + if (destroy_hwpt) > > > Should this be > > > > > > if (!IS_ERR_OR_NULL(destroy_hwpt)) > > > > > > ? > > Never use IS_ERR_OR_NULL .. > > Can you please elaborate a bit on this? I can still see a lot of use of > it in the tree. Yes, sadly. It is usually some signal of toxic confusion about what things mean. A function that returns an ERR_PTR should very rarely return NULL, and if it does return NULL then NULL wasn't an error. Further you should never store an ERR_PTR in some structure and then later try to test it, that is madness. So with properly structured code the need should not exist. https://lore.kernel.org/all/20130109150427.GL3931@xxxxxxxxxxxxxxxxxxxxxx/ Jason
