On Thu, Mar 02, 2023 at 08:01:00AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@xxxxxxxxxx>
> > Sent: Saturday, February 25, 2023 8:28 AM
> >
> > +struct iommufd_hw_pagetable *
> > +iommufd_hw_pagetable_detach(struct iommufd_device *idev)
> > {
> > - if (!iommufd_hw_pagetable_has_group(hwpt, idev->igroup))
> > + struct iommufd_hw_pagetable *hwpt = idev->igroup->hwpt;
> > +
> > + lockdep_assert_held(&idev->igroup->lock);
> > +
> > + idev->igroup->devices--;
> > + if (!idev->igroup->devices) {
> > iommu_detach_group(hwpt->domain, idev->igroup->group);
> > + idev->igroup->hwpt = NULL;
>
> hwpt->obj.users should be decremented here instead of leaving it
> in iommufd_device_detach().
It is like this because eventually we can't call
iommufd_object_destroy_user() while holding the locks.
So the lowest function returns the hwpt up the call chain and once
everything is unlocked then it calls iommufd_hw_pagetable_put()
> > void iommufd_device_detach(struct iommufd_device *idev)
> > {
> > - struct iommufd_hw_pagetable *hwpt = idev->hwpt;
> > + struct iommufd_hw_pagetable *hwpt;
> >
> > - mutex_lock(&hwpt->devices_lock);
> > - list_del(&idev->devices_item);
> > - idev->hwpt = NULL;
> > - iommufd_hw_pagetable_detach(hwpt, idev);
> > - mutex_unlock(&hwpt->devices_lock);
> > + mutex_lock(&idev->igroup->lock);
> > + hwpt = iommufd_hw_pagetable_detach(idev);
>
> the only parameter is idev while the name is called hw_pagetable_xxx.
>
> is it cleaner to get hwpt here and then pass into the detach function?
Not really, the function needs three members of the idev to work.
The pair'd attach function is:
int iommufd_hw_pagetable_attach(struct iommufd_hw_pagetable *hwpt,
struct iommufd_device *idev)
So I think it would be very unclear to change the name, and this is
more a hw_pagetable operation than a device operation.
Jason
