diff mbox series

[v2,2/3] iommufd/device: Make hwpt_list list_add/del symmetric

Message ID 6f388b1f20622957518ec5a9ddc7f0037e7671c4.1674939002.git.nicolinc@nvidia.com
State New
Headers
Series iommufd: Remove iommufd_hw_pagetable_has_group |

Commit Message

Nicolin Chen Jan. 28, 2023, 9:18 p.m. UTC 
  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(-)

Comments

Tian, Kevin Jan. 29, 2023, 9:24 a.m. UTC | #1 
> 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>
Nicolin Chen Jan. 29, 2023, 9:31 a.m. UTC | #2 
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
Jason Gunthorpe Jan. 30, 2023, 2:59 p.m. UTC | #3 
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
Nicolin Chen Jan. 30, 2023, 7:03 p.m. UTC | #4 
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
Jason Gunthorpe Jan. 30, 2023, 7:07 p.m. UTC | #5 
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
Nicolin Chen Jan. 30, 2023, 7:38 p.m. UTC | #6 
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
diff mbox series

Patch

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;