On Mon, Jun 16, 2025 at 07:25:57PM -0700, Nicolin Chen wrote:
> On Mon, Jun 16, 2025 at 10:37:19AM -0300, Jason Gunthorpe wrote:
> > On Sat, Jun 14, 2025 at 12:14:32AM -0700, Nicolin Chen wrote:
> > > Now, access->ops can be NULL, to support an internal use case for the new
> > > HW queue object. Since an access object in this case will be allocated by
> > > an inernal iommufd object, the refcount on the ictx should be skipped, so
> > > as not to deadlock the release of the ictx as it would otherwise wait for
> > > the release of the access first during the release of the internal object
> > > that could wait for the release of ictx:
> > > ictx --releases--> hw_queue --releases--> access
> > > ^ |
> > > |_________________releases________________v
> > >
> > > Add a set of lightweight internal APIs to unlink access and ictx:
> > > ictx --releases--> hw_queue --releases--> access
> > >
> > > Signed-off-by: Nicolin Chen <nicolinc@xxxxxxxxxx>
> > > ---
> > > drivers/iommu/iommufd/iommufd_private.h | 8 ++++
> > > drivers/iommu/iommufd/device.c | 59 +++++++++++++++++++++----
> > > 2 files changed, 58 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
> > > index 4a375a8c9216..468717d5e5bc 100644
> > > --- a/drivers/iommu/iommufd/iommufd_private.h
> > > +++ b/drivers/iommu/iommufd/iommufd_private.h
> > > @@ -484,6 +484,14 @@ void iopt_remove_access(struct io_pagetable *iopt,
> > > struct iommufd_access *access, u32 iopt_access_list_id);
> > > void iommufd_access_destroy_object(struct iommufd_object *obj);
> > >
> > > +/* iommufd_access for internal use */
> > > +struct iommufd_access *iommufd_access_create_internal(struct iommufd_ctx *ictx);
> > > +#define iommufd_access_destroy_internal(ictx, access) \
> > > + iommufd_object_destroy_user(ictx, &(access)->obj)
> >
> > Use a static inline please
> >
> > > +int iommufd_access_attach_internal(struct iommufd_access *access,
> > > + struct iommufd_ioas *ioas);
> > > +#define iommufd_access_detach_internal(access) iommufd_access_detach(access)
> >
> >
> > > struct iommufd_eventq {
> > > struct iommufd_object obj;
> > > struct iommufd_ctx *ictx;
> > > diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
> > > index 9293722b9cff..ad33f1e41a24 100644
> > > --- a/drivers/iommu/iommufd/device.c
> > > +++ b/drivers/iommu/iommufd/device.c
> > > @@ -1084,7 +1084,39 @@ void iommufd_access_destroy_object(struct iommufd_object *obj)
> > > if (access->ioas)
> > > WARN_ON(iommufd_access_change_ioas(access, NULL));
> > > mutex_unlock(&access->ioas_lock);
> > > - iommufd_ctx_put(access->ictx);
> > > + if (access->ops)
> > > + iommufd_ctx_put(access->ictx);
> >
> > I was hoping we could null the ictx to signal internal? That didn't
> > work out?
>
> access->ictx should be NULL for internal. It should have been:
> + if (access->ictx)
> + iommufd_ctx_put(access->ictx);
>
Ohh sorry, just saw this. +1, I too believe this is better than relying
on access->ops being NULL.
> > I would at least add a comment here this is filtering internal that
> > doesn't have ictx. Maybe a little inline 'iommufd_access_is_internal'
> > is appropriate. We'll be sad down the road if we need ops for
> > internal.
>
> Yea, an inline will be cleaner. Will add that.
>
Ack.
Thanks,
Praan
