Since the list_del() of hwpt_item is done in iommufd_device_detach(), move
its list_add_tail() to a similar place in iommufd_device_do_attach().

Also move and place the mutex outside the iommufd_device_auto_get_domain()
and iommufd_device_do_attach() calls, to serialize attach/detach routines.
This adds an additional locking protection so that the following patch can
safely remove devices_lock.

Co-developed-by: Yi Liu <yi.l.liu@intel.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/iommufd/device.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index 208757c39c90..9375fcac884c 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -198,6 +198,8 @@ static int iommufd_device_do_attach(struct iommufd_device *idev,
 	phys_addr_t sw_msi_start = PHYS_ADDR_MAX;
 	int rc;
 
+	lockdep_assert_held(&hwpt->ioas->mutex);
+
 	mutex_lock(&hwpt->devices_lock);
 
 	/*
@@ -241,6 +243,7 @@ static int iommufd_device_do_attach(struct iommufd_device *idev,
 						   hwpt->domain);
 			if (rc)
 				goto out_detach;
+			list_add_tail(&hwpt->hwpt_item, &hwpt->ioas->hwpt_list);
 		}
 	}
 
@@ -271,12 +274,13 @@ static int iommufd_device_auto_get_domain(struct iommufd_device *idev,
 	struct iommufd_hw_pagetable *hwpt;
 	int rc;
 
+	lockdep_assert_held(&ioas->mutex);
+
 	/*
 	 * There is no differentiation when domains are allocated, so any domain
 	 * that is willing to attach to the device is interchangeable with any
 	 * other.
 	 */
-	mutex_lock(&ioas->mutex);
 	list_for_each_entry(hwpt, &ioas->hwpt_list, hwpt_item) {
 		if (!hwpt->auto_domain)
 			continue;
@@ -290,29 +294,23 @@ static int iommufd_device_auto_get_domain(struct iommufd_device *idev,
 		 */
 		if (rc == -EINVAL)
 			continue;
-		goto out_unlock;
+		return rc;
 	}
 
 	hwpt = iommufd_hw_pagetable_alloc(idev->ictx, ioas, idev->dev);
-	if (IS_ERR(hwpt)) {
-		rc = PTR_ERR(hwpt);
-		goto out_unlock;
-	}
+	if (IS_ERR(hwpt))
+		return PTR_ERR(hwpt);
 	hwpt->auto_domain = true;
 
 	rc = iommufd_device_do_attach(idev, hwpt);
 	if (rc)
 		goto out_abort;
-	list_add_tail(&hwpt->hwpt_item, &ioas->hwpt_list);
 
-	mutex_unlock(&ioas->mutex);
 	iommufd_object_finalize(idev->ictx, &hwpt->obj);
 	return 0;
 
 out_abort:
 	iommufd_object_abort_and_destroy(idev->ictx, &hwpt->obj);
-out_unlock:
-	mutex_unlock(&ioas->mutex);
 	return rc;
 }
 
@@ -342,20 +340,20 @@ int iommufd_device_attach(struct iommufd_device *idev, u32 *pt_id)
 		struct iommufd_hw_pagetable *hwpt =
 			container_of(pt_obj, struct iommufd_hw_pagetable, obj);
 
+		mutex_lock(&hwpt->ioas->mutex);
 		rc = iommufd_device_do_attach(idev, hwpt);
+		mutex_unlock(&hwpt->ioas->mutex);
 		if (rc)
 			goto out_put_pt_obj;
-
-		mutex_lock(&hwpt->ioas->mutex);
-		list_add_tail(&hwpt->hwpt_item, &hwpt->ioas->hwpt_list);
-		mutex_unlock(&hwpt->ioas->mutex);
 		break;
 	}
 	case IOMMUFD_OBJ_IOAS: {
 		struct iommufd_ioas *ioas =
 			container_of(pt_obj, struct iommufd_ioas, obj);
 
+		mutex_lock(&ioas->mutex);
 		rc = iommufd_device_auto_get_domain(idev, ioas);
+		mutex_unlock(&ioas->mutex);
 		if (rc)
 			goto out_put_pt_obj;
 		break;
-- 
2.39.1

On Sat, Jan 28, 2023 at 01:18:10PM -0800, Nicolin Chen wrote:
> Since the list_del() of hwpt_item is done in iommufd_device_detach(), move
> its list_add_tail() to a similar place in iommufd_device_do_attach().
> 
> Also move and place the mutex outside the iommufd_device_auto_get_domain()
> and iommufd_device_do_attach() calls, to serialize attach/detach routines.
> This adds an additional locking protection so that the following patch can
> safely remove devices_lock.

That should be two patches then, this is just moving one line of code
from what I can tell?

Jason

On Mon, Jan 30, 2023 at 10:59:32AM -0400, Jason Gunthorpe wrote:
> On Sat, Jan 28, 2023 at 01:18:10PM -0800, Nicolin Chen wrote:
> > Since the list_del() of hwpt_item is done in iommufd_device_detach(), move
> > its list_add_tail() to a similar place in iommufd_device_do_attach().
> > 
> > Also move and place the mutex outside the iommufd_device_auto_get_domain()
> > and iommufd_device_do_attach() calls, to serialize attach/detach routines.
> > This adds an additional locking protection so that the following patch can
> > safely remove devices_lock.
> 
> That should be two patches then, this is just moving one line of code
> from what I can tell?

The mutex is used to protect the list. So moving the list means
we'd need to the mutex too. What this patch does is to enlarge
the protection scope a bit to cover iommufd_device_do_attach()
and iommufd_device_auto_get_domain().

I can revise a bit of the commit message to mention this.

Thanks
Nicolin

On Mon, Jan 30, 2023 at 11:03:09AM -0800, Nicolin Chen wrote:
> On Mon, Jan 30, 2023 at 10:59:32AM -0400, Jason Gunthorpe wrote:
> > On Sat, Jan 28, 2023 at 01:18:10PM -0800, Nicolin Chen wrote:
> > > Since the list_del() of hwpt_item is done in iommufd_device_detach(), move
> > > its list_add_tail() to a similar place in iommufd_device_do_attach().
> > > 
> > > Also move and place the mutex outside the iommufd_device_auto_get_domain()
> > > and iommufd_device_do_attach() calls, to serialize attach/detach routines.
> > > This adds an additional locking protection so that the following patch can
> > > safely remove devices_lock.
> > 
> > That should be two patches then, this is just moving one line of code
> > from what I can tell?
> 
> The mutex is used to protect the list. So moving the list means
> we'd need to the mutex too. What this patch does is to enlarge
> the protection scope a bit to cover iommufd_device_do_attach()
> and iommufd_device_auto_get_domain().

That doesn't explain why iommufd_device_auto_get_domain was changed
around, it already had the lock

Jason

On Mon, Jan 30, 2023 at 03:07:50PM -0400, Jason Gunthorpe wrote:
> On Mon, Jan 30, 2023 at 11:03:09AM -0800, Nicolin Chen wrote:
> > On Mon, Jan 30, 2023 at 10:59:32AM -0400, Jason Gunthorpe wrote:
> > > On Sat, Jan 28, 2023 at 01:18:10PM -0800, Nicolin Chen wrote:
> > > > Since the list_del() of hwpt_item is done in iommufd_device_detach(), move
> > > > its list_add_tail() to a similar place in iommufd_device_do_attach().
> > > > 
> > > > Also move and place the mutex outside the iommufd_device_auto_get_domain()
> > > > and iommufd_device_do_attach() calls, to serialize attach/detach routines.
> > > > This adds an additional locking protection so that the following patch can
> > > > safely remove devices_lock.
> > > 
> > > That should be two patches then, this is just moving one line of code
> > > from what I can tell?
> > 
> > The mutex is used to protect the list. So moving the list means
> > we'd need to the mutex too. What this patch does is to enlarge
> > the protection scope a bit to cover iommufd_device_do_attach()
> > and iommufd_device_auto_get_domain().
> 
> That doesn't explain why iommufd_device_auto_get_domain was changed
> around, it already had the lock

That is trying to make the code look like this:

iommufd_device_attach {
		...
 	case IOMMUFD_OBJ_HW_PAGETABLE:
+		mutex_lock(&hwpt->ioas->mutex);
 		rc = iommufd_device_do_attach(idev, hwpt);
+		mutex_unlock(&hwpt->ioas->mutex);
		...
 	case IOMMUFD_OBJ_IOAS:
		...
+		mutex_lock(&ioas->mutex);
 		rc = iommufd_device_auto_get_domain(idev, ioas);
+		mutex_unlock(&ioas->mutex);
		...
}

If you don't think that's necessary, I can make things intact
in iommufd_device_auto_get_domain().

Thanks
Nic

> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Sunday, January 29, 2023 5:18 AM
> 
> Since the list_del() of hwpt_item is done in iommufd_device_detach(), move
> its list_add_tail() to a similar place in iommufd_device_do_attach().

It's clearer to say that because list_del() is together with
iopt_table_remove_domain() then it makes sense to have list_add_tail()
together with iopt_table_add_domain().

> 
> Also move and place the mutex outside the
> iommufd_device_auto_get_domain()
> and iommufd_device_do_attach() calls, to serialize attach/detach routines.
> This adds an additional locking protection so that the following patch can
> safely remove devices_lock.
> 
> Co-developed-by: Yi Liu <yi.l.liu@intel.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

On Sun, Jan 29, 2023 at 09:24:38AM +0000, Tian, Kevin wrote:
> External email: Use caution opening links or attachments
> 
> 
> > From: Nicolin Chen <nicolinc@nvidia.com>
> > Sent: Sunday, January 29, 2023 5:18 AM
> >
> > Since the list_del() of hwpt_item is done in iommufd_device_detach(), move
> > its list_add_tail() to a similar place in iommufd_device_do_attach().
> 
> It's clearer to say that because list_del() is together with
> iopt_table_remove_domain() then it makes sense to have list_add_tail()
> together with iopt_table_add_domain().

OK. Will change that.

> >
> > Also move and place the mutex outside the
> > iommufd_device_auto_get_domain()
> > and iommufd_device_do_attach() calls, to serialize attach/detach routines.
> > This adds an additional locking protection so that the following patch can
> > safely remove devices_lock.
> >
> > Co-developed-by: Yi Liu <yi.l.liu@intel.com>
> > Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> 
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>

And add this too. Thanks!

Nicolin