Message ID | 4-v2-3c3bb7aa6e48+1916b-iommu_probe_jgg@nvidia.com (mailing list archive) |
---|---|
State | Handled Elsewhere, archived |
Headers | show |
Series | Consolidate the probe_device path | expand |
On 5/20/23 2:42 AM, Jason Gunthorpe wrote: > Instead of returning the struct group_device and then later freeing it, do > the entire free under the group->mutex and defer only putting the > iommu_group. > > It is safe to remove the sysfs_links and free memory while holding that > mutex. > > Move the sanity assert of the group status into > __iommu_group_free_device(). > > The next patch will improve upon this and consolidate the group put and > the mutex into __iommu_group_remove_device(). > > __iommu_group_free_device() is close to being the paired undo of > iommu_group_add_device(), following patches will improve on that. > > Reviewed-by: Kevin Tian<kevin.tian@intel.com> > Signed-off-by: Jason Gunthorpe<jgg@nvidia.com> Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com> Best regards, baolu
On Fri, 2023-05-19 at 15:42 -0300, Jason Gunthorpe wrote: > Instead of returning the struct group_device and then later freeing it, do > the entire free under the group->mutex and defer only putting the > iommu_group. > > It is safe to remove the sysfs_links and free memory while holding that > mutex. > > Move the sanity assert of the group status into > __iommu_group_free_device(). > > The next patch will improve upon this and consolidate the group put and > the mutex into __iommu_group_remove_device(). > > __iommu_group_free_device() is close to being the paired undo of > iommu_group_add_device(), following patches will improve on that. > > Reviewed-by: Kevin Tian <kevin.tian@intel.com> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/iommu/iommu.c | 83 ++++++++++++++++++++----------------------- > 1 file changed, 39 insertions(+), 44 deletions(-) > ---8<--- > + > +/* > + * Remove the iommu_group from the struct device. The attached group must be put > + * by the caller after releaseing the group->mutex. > + */ > +static void __iommu_group_remove_device(struct device *dev) > +{ > + struct iommu_group *group = dev->iommu_group; > + struct group_device *device; > + > + lockdep_assert_held(&group->mutex); > + for_each_group_device(group, device) { > + if (device->dev != dev) > + continue; > + > + list_del(&device->list); for_each_group_device() uses list_for_each_entry() but here you are deleting from the list, don't we need a ..._safe() variant then? > + __iommu_group_free_device(group, device); > + /* Caller must put iommu_group */ > + return; > + } > + WARN(true, "Corrupted iommu_group device_list"); > } > ---8<---
On Mon, May 22, 2023 at 10:35:49AM +0200, Niklas Schnelle wrote: > On Fri, 2023-05-19 at 15:42 -0300, Jason Gunthorpe wrote: > > Instead of returning the struct group_device and then later freeing it, do > > the entire free under the group->mutex and defer only putting the > > iommu_group. > > > > It is safe to remove the sysfs_links and free memory while holding that > > mutex. > > > > Move the sanity assert of the group status into > > __iommu_group_free_device(). > > > > The next patch will improve upon this and consolidate the group put and > > the mutex into __iommu_group_remove_device(). > > > > __iommu_group_free_device() is close to being the paired undo of > > iommu_group_add_device(), following patches will improve on that. > > > > Reviewed-by: Kevin Tian <kevin.tian@intel.com> > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > > --- > > drivers/iommu/iommu.c | 83 ++++++++++++++++++++----------------------- > > 1 file changed, 39 insertions(+), 44 deletions(-) > > > ---8<--- > > + > > +/* > > + * Remove the iommu_group from the struct device. The attached group must be put > > + * by the caller after releaseing the group->mutex. > > + */ > > +static void __iommu_group_remove_device(struct device *dev) > > +{ > > + struct iommu_group *group = dev->iommu_group; > > + struct group_device *device; > > + > > + lockdep_assert_held(&group->mutex); > > + for_each_group_device(group, device) { > > + if (device->dev != dev) > > + continue; > > + > > + list_del(&device->list); > > for_each_group_device() uses list_for_each_entry() but here you are > deleting from the list, don't we need a ..._safe() variant then? As a general statement, yes > > + __iommu_group_free_device(group, device); > > + /* Caller must put iommu_group */ > > + return; But the loop immediately returns before going to the next iteration so this is safe. Jason
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index 6177e01ced67ab..a87e2df5ce1238 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -470,32 +470,8 @@ int iommu_probe_device(struct device *dev) } -/* - * Remove a device from a group's device list and return the group device - * if successful. - */ -static struct group_device * -__iommu_group_remove_device(struct iommu_group *group, struct device *dev) -{ - struct group_device *device; - - lockdep_assert_held(&group->mutex); - for_each_group_device(group, device) { - if (device->dev == dev) { - list_del(&device->list); - return device; - } - } - - return NULL; -} - -/* - * Release a device from its group and decrements the iommu group reference - * count. - */ -static void __iommu_group_release_device(struct iommu_group *group, - struct group_device *grp_dev) +static void __iommu_group_free_device(struct iommu_group *group, + struct group_device *grp_dev) { struct device *dev = grp_dev->dev; @@ -504,16 +480,45 @@ static void __iommu_group_release_device(struct iommu_group *group, trace_remove_device_from_group(group->id, dev); + /* + * If the group has become empty then ownership must have been + * released, and the current domain must be set back to NULL or + * the default domain. + */ + if (list_empty(&group->devices)) + WARN_ON(group->owner_cnt || + group->domain != group->default_domain); + kfree(grp_dev->name); kfree(grp_dev); dev->iommu_group = NULL; - iommu_group_put(group); +} + +/* + * Remove the iommu_group from the struct device. The attached group must be put + * by the caller after releaseing the group->mutex. + */ +static void __iommu_group_remove_device(struct device *dev) +{ + struct iommu_group *group = dev->iommu_group; + struct group_device *device; + + lockdep_assert_held(&group->mutex); + for_each_group_device(group, device) { + if (device->dev != dev) + continue; + + list_del(&device->list); + __iommu_group_free_device(group, device); + /* Caller must put iommu_group */ + return; + } + WARN(true, "Corrupted iommu_group device_list"); } static void iommu_release_device(struct device *dev) { struct iommu_group *group = dev->iommu_group; - struct group_device *device; const struct iommu_ops *ops; if (!dev->iommu || !group) @@ -522,16 +527,7 @@ static void iommu_release_device(struct device *dev) iommu_device_unlink(dev->iommu->iommu_dev, dev); mutex_lock(&group->mutex); - device = __iommu_group_remove_device(group, dev); - - /* - * If the group has become empty then ownership must have been released, - * and the current domain must be set back to NULL or the default - * domain. - */ - if (list_empty(&group->devices)) - WARN_ON(group->owner_cnt || - group->domain != group->default_domain); + __iommu_group_remove_device(dev); /* * release_device() must stop using any attached domain on the device. @@ -547,8 +543,8 @@ static void iommu_release_device(struct device *dev) ops->release_device(dev); mutex_unlock(&group->mutex); - if (device) - __iommu_group_release_device(group, device); + /* Pairs with the get in iommu_group_add_device() */ + iommu_group_put(group); module_put(ops->owner); dev_iommu_free(dev); @@ -1107,7 +1103,6 @@ EXPORT_SYMBOL_GPL(iommu_group_add_device); void iommu_group_remove_device(struct device *dev) { struct iommu_group *group = dev->iommu_group; - struct group_device *device; if (!group) return; @@ -1115,11 +1110,11 @@ void iommu_group_remove_device(struct device *dev) dev_info(dev, "Removing from iommu group %d\n", group->id); mutex_lock(&group->mutex); - device = __iommu_group_remove_device(group, dev); + __iommu_group_remove_device(dev); mutex_unlock(&group->mutex); - if (device) - __iommu_group_release_device(group, device); + /* Pairs with the get in iommu_group_add_device() */ + iommu_group_put(group); } EXPORT_SYMBOL_GPL(iommu_group_remove_device);