[PATCH v2 13/14] iommufd: Introduce iommufd_object_alloc_ucmd helper

[Tian, Kevin]

> From: Xu Yilun <yilun.xu at linux.intel.com>
> Sent: Wednesday, July 9, 2025 1:32 PM
> 
> > @@ -61,6 +61,24 @@ struct iommufd_object
> *_iommufd_object_alloc(struct iommufd_ctx *ictx,
> >  	return ERR_PTR(rc);
> >  }
> >
> > +struct iommufd_object *_iommufd_object_alloc_ucmd(struct
> iommufd_ucmd *ucmd,
> > +						  size_t size,
> > +						  enum iommufd_object_type
> type)
> > +{
> > +	struct iommufd_object *new_obj;
> > +
> > +	/* Something is coded wrong if this is hit */
> > +	if (WARN_ON(ucmd->new_obj))
> > +		return ERR_PTR(-EBUSY);
> > +
> > +	new_obj = _iommufd_object_alloc(ucmd->ictx, size, type);
> > +	if (IS_ERR(new_obj))
> > +		return new_obj;
> > +
> > +	ucmd->new_obj = new_obj;
> > +	return new_obj;
> > +}
> > +
> >  /*
> >   * Allow concurrent access to the object.
> >   *
> > @@ -448,6 +466,13 @@ static long iommufd_fops_ioctl(struct file *filp,
> unsigned int cmd,
> >  	if (ret)
> >  		return ret;
> >  	ret = op->execute(&ucmd);
> > +
> > +	if (ucmd.new_obj) {
> > +		if (ret)
> > +			iommufd_object_abort_and_destroy(ictx,
> ucmd.new_obj);
> 
> Sorry I didn't follow this thread before and maybe missed something.
> 
> According to 70eadc7fc7ef, abort op is for the object that can assume
> the caller is holding the lock. But here is for no locking, so calling
> iommufd_object_abort_and_destroy() is quite confusing.
> 
> Is it better we change to:
> 
> 	if (ret) {
> 		iommufd_object_ops[obj->type].destroy(obj);
> 		iommufd_object_abort(ictx, obj);
> 	}

I'd keep the original way. The function name describes what to do,
not what to be called exactly inside. Lacking of the abort method
doesn't change the meaning of the function which is about abort
and destroy (just like how it's called before introducing @abort).

> 
> Also explicitely assert iommufd_object_alloc_ucmd() and abort can't be
> used at the same time.
> 
> in _iommufd_object_alloc_ucmd():
> 
> 	if (WARN_ON(iommufd_object_ops[type].abort))
> 		return ERR_PTR(-EFAULT);
> 

but this check sounds necessary.