| Message ID | 10-v1-7dedf20b2b75+4f785-vfio2_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Make vfio_mdev type safe | expand |
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
> From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Wednesday, March 24, 2021 1:55 AM > > mdev_device->type->parent is the same thing. > > The struct mdev_device was relying on the kref on the mdev_parent to also > indirectly hold a kref on the mdev_type pointer. Now that the type holds a > kref on the parent we can directly kref the mdev_type and remove this > implicit relationship. > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/vfio/mdev/mdev_core.c | 13 +++++-------- > drivers/vfio/mdev/vfio_mdev.c | 14 +++++++------- > include/linux/mdev.h | 1 - > 3 files changed, 12 insertions(+), 16 deletions(-) > > diff --git a/drivers/vfio/mdev/mdev_core.c > b/drivers/vfio/mdev/mdev_core.c > index 4b5e372ed58f26..493df3da451339 100644 > --- a/drivers/vfio/mdev/mdev_core.c > +++ b/drivers/vfio/mdev/mdev_core.c > @@ -29,7 +29,7 @@ static DEFINE_MUTEX(mdev_list_lock); > > struct device *mdev_parent_dev(struct mdev_device *mdev) > { > - return mdev->parent->dev; > + return mdev->type->parent->dev; > } > EXPORT_SYMBOL(mdev_parent_dev); > > @@ -58,12 +58,11 @@ void mdev_release_parent(struct kref *kref) > /* Caller must hold parent unreg_sem read or write lock */ > static void mdev_device_remove_common(struct mdev_device *mdev) > { > - struct mdev_parent *parent; > + struct mdev_parent *parent = mdev->type->parent; What about having a wrapper here, like mdev_parent_dev? For readability it's not necessary to show that the parent is indirectly retrieved through mdev_type. > int ret; > > mdev_remove_sysfs_files(mdev); > device_del(&mdev->dev); > - parent = mdev->parent; > lockdep_assert_held(&parent->unreg_sem); > ret = parent->ops->remove(mdev); > if (ret) > @@ -212,7 +211,7 @@ static void mdev_device_release(struct device *dev) > struct mdev_device *mdev = to_mdev_device(dev); > > /* Pairs with the get in mdev_device_create() */ > - mdev_put_parent(mdev->parent); > + kobject_put(&mdev->type->kobj); Maybe keep mdev_get/put_parent and change them to accept "struct mdev_device *" parameter like other places. > > mutex_lock(&mdev_list_lock); > list_del(&mdev->next); > @@ -250,9 +249,8 @@ int mdev_device_create(struct mdev_type *type, > const guid_t *uuid) > mdev->dev.release = mdev_device_release; > mdev->dev.groups = parent->ops->mdev_attr_groups; > mdev->type = type; > - mdev->parent = parent; > /* Pairs with the put in mdev_device_release() */ > - mdev_get_parent(parent); > + kobject_get(&type->kobj); > > guid_copy(&mdev->uuid, uuid); > list_add(&mdev->next, &mdev_list); > @@ -300,7 +298,7 @@ int mdev_device_create(struct mdev_type *type, > const guid_t *uuid) > int mdev_device_remove(struct mdev_device *mdev) > { > struct mdev_device *tmp; > - struct mdev_parent *parent; > + struct mdev_parent *parent = mdev->type->parent; > > mutex_lock(&mdev_list_lock); > list_for_each_entry(tmp, &mdev_list, next) { > @@ -321,7 +319,6 @@ int mdev_device_remove(struct mdev_device *mdev) > mdev->active = false; > mutex_unlock(&mdev_list_lock); > > - parent = mdev->parent; > /* Check if parent unregistration has started */ > if (!down_read_trylock(&parent->unreg_sem)) > return -ENODEV; > diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c > index cc9507ed85a181..922729071c5a8e 100644 > --- a/drivers/vfio/mdev/vfio_mdev.c > +++ b/drivers/vfio/mdev/vfio_mdev.c > @@ -24,7 +24,7 @@ > static int vfio_mdev_open(struct vfio_device *core_vdev) > { > struct mdev_device *mdev = to_mdev_device(core_vdev->dev); > - struct mdev_parent *parent = mdev->parent; > + struct mdev_parent *parent = mdev->type->parent; > > int ret; > > @@ -44,7 +44,7 @@ static int vfio_mdev_open(struct vfio_device > *core_vdev) > static void vfio_mdev_release(struct vfio_device *core_vdev) > { > struct mdev_device *mdev = to_mdev_device(core_vdev->dev); > - struct mdev_parent *parent = mdev->parent; > + struct mdev_parent *parent = mdev->type->parent; > > if (likely(parent->ops->release)) > parent->ops->release(mdev); > @@ -56,7 +56,7 @@ static long vfio_mdev_unlocked_ioctl(struct vfio_device > *core_vdev, > unsigned int cmd, unsigned long arg) > { > struct mdev_device *mdev = to_mdev_device(core_vdev->dev); > - struct mdev_parent *parent = mdev->parent; > + struct mdev_parent *parent = mdev->type->parent; > > if (unlikely(!parent->ops->ioctl)) > return -EINVAL; > @@ -68,7 +68,7 @@ static ssize_t vfio_mdev_read(struct vfio_device > *core_vdev, char __user *buf, > size_t count, loff_t *ppos) > { > struct mdev_device *mdev = to_mdev_device(core_vdev->dev); > - struct mdev_parent *parent = mdev->parent; > + struct mdev_parent *parent = mdev->type->parent; > > if (unlikely(!parent->ops->read)) > return -EINVAL; > @@ -81,7 +81,7 @@ static ssize_t vfio_mdev_write(struct vfio_device > *core_vdev, > loff_t *ppos) > { > struct mdev_device *mdev = to_mdev_device(core_vdev->dev); > - struct mdev_parent *parent = mdev->parent; > + struct mdev_parent *parent = mdev->type->parent; > > if (unlikely(!parent->ops->write)) > return -EINVAL; > @@ -93,7 +93,7 @@ static int vfio_mdev_mmap(struct vfio_device > *core_vdev, > struct vm_area_struct *vma) > { > struct mdev_device *mdev = to_mdev_device(core_vdev->dev); > - struct mdev_parent *parent = mdev->parent; > + struct mdev_parent *parent = mdev->type->parent; > > if (unlikely(!parent->ops->mmap)) > return -EINVAL; > @@ -104,7 +104,7 @@ static int vfio_mdev_mmap(struct vfio_device > *core_vdev, > static void vfio_mdev_request(struct vfio_device *core_vdev, unsigned int > count) > { > struct mdev_device *mdev = to_mdev_device(core_vdev->dev); > - struct mdev_parent *parent = mdev->parent; > + struct mdev_parent *parent = mdev->type->parent; > > if (parent->ops->request) > parent->ops->request(mdev, count); > diff --git a/include/linux/mdev.h b/include/linux/mdev.h > index 349e8ac1fe3382..fb582adda28a9b 100644 > --- a/include/linux/mdev.h > +++ b/include/linux/mdev.h > @@ -14,7 +14,6 @@ struct mdev_type; > > struct mdev_device { > struct device dev; > - struct mdev_parent *parent; > guid_t uuid; > void *driver_data; > struct list_head next; > -- > 2.31.0
On Fri, Mar 26, 2021 at 03:53:10AM +0000, Tian, Kevin wrote: > > @@ -58,12 +58,11 @@ void mdev_release_parent(struct kref *kref) > > /* Caller must hold parent unreg_sem read or write lock */ > > static void mdev_device_remove_common(struct mdev_device *mdev) > > { > > - struct mdev_parent *parent; > > + struct mdev_parent *parent = mdev->type->parent; > > What about having a wrapper here, like mdev_parent_dev? For > readability it's not necessary to show that the parent is indirectly > retrieved through mdev_type. I think that is too much wrappering, we only have three usages of the mdev->type->parent sequence and two are already single line inlines. > > int ret; > > > > mdev_remove_sysfs_files(mdev); > > device_del(&mdev->dev); > > - parent = mdev->parent; > > lockdep_assert_held(&parent->unreg_sem); > > ret = parent->ops->remove(mdev); > > if (ret) > > @@ -212,7 +211,7 @@ static void mdev_device_release(struct device *dev) > > struct mdev_device *mdev = to_mdev_device(dev); > > > > /* Pairs with the get in mdev_device_create() */ > > - mdev_put_parent(mdev->parent); > > + kobject_put(&mdev->type->kobj); > > Maybe keep mdev_get/put_parent and change them to accept "struct > mdev_device *" parameter like other places. We do keep mdev_get/put_parent() they manipulate the refcount inside the parent and this is only done in side the type logic now. This is get/put of the refcount on the type, and this is the only place that does it, so I don't see a reason for more wrappers. Jason
On Fri, 26 Mar 2021 08:53:50 -0300 Jason Gunthorpe <jgg@nvidia.com> wrote: > On Fri, Mar 26, 2021 at 03:53:10AM +0000, Tian, Kevin wrote: > > > > @@ -58,12 +58,11 @@ void mdev_release_parent(struct kref *kref) > > > /* Caller must hold parent unreg_sem read or write lock */ > > > static void mdev_device_remove_common(struct mdev_device *mdev) > > > { > > > - struct mdev_parent *parent; > > > + struct mdev_parent *parent = mdev->type->parent; > > > > What about having a wrapper here, like mdev_parent_dev? For > > readability it's not necessary to show that the parent is indirectly > > retrieved through mdev_type. > > I think that is too much wrappering, we only have three usages of the > mdev->type->parent sequence and two are already single line inlines. I'm counting more of those in this patch... or do you mean at the end of the series?
On Tue, 23 Mar 2021 14:55:27 -0300 Jason Gunthorpe <jgg@nvidia.com> wrote: > mdev_device->type->parent is the same thing. > > The struct mdev_device was relying on the kref on the mdev_parent to also > indirectly hold a kref on the mdev_type pointer. Now that the type holds a > kref on the parent we can directly kref the mdev_type and remove this > implicit relationship. > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/vfio/mdev/mdev_core.c | 13 +++++-------- > drivers/vfio/mdev/vfio_mdev.c | 14 +++++++------- > include/linux/mdev.h | 1 - > 3 files changed, 12 insertions(+), 16 deletions(-) With or without an additional wrapper: Reviewed-by: Cornelia Huck <cohuck@redhat.com>
On Tue, Mar 30, 2021 at 06:14:13PM +0200, Cornelia Huck wrote: > On Fri, 26 Mar 2021 08:53:50 -0300 > Jason Gunthorpe <jgg@nvidia.com> wrote: > > > On Fri, Mar 26, 2021 at 03:53:10AM +0000, Tian, Kevin wrote: > > > > > > @@ -58,12 +58,11 @@ void mdev_release_parent(struct kref *kref) > > > > /* Caller must hold parent unreg_sem read or write lock */ > > > > static void mdev_device_remove_common(struct mdev_device *mdev) > > > > { > > > > - struct mdev_parent *parent; > > > > + struct mdev_parent *parent = mdev->type->parent; > > > > > > What about having a wrapper here, like mdev_parent_dev? For > > > readability it's not necessary to show that the parent is indirectly > > > retrieved through mdev_type. > > > > I think that is too much wrappering, we only have three usages of the > > mdev->type->parent sequence and two are already single line inlines. > > I'm counting more of those in this patch... or do you mean at the end > of the series? I'm thinking of either moving them to inlines earlier or moving them all as a final patch. Given how far we've come review wise I'm inclined to do the latter Thanks, Jason
diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c index 4b5e372ed58f26..493df3da451339 100644 --- a/drivers/vfio/mdev/mdev_core.c +++ b/drivers/vfio/mdev/mdev_core.c @@ -29,7 +29,7 @@ static DEFINE_MUTEX(mdev_list_lock); struct device *mdev_parent_dev(struct mdev_device *mdev) { - return mdev->parent->dev; + return mdev->type->parent->dev; } EXPORT_SYMBOL(mdev_parent_dev); @@ -58,12 +58,11 @@ void mdev_release_parent(struct kref *kref) /* Caller must hold parent unreg_sem read or write lock */ static void mdev_device_remove_common(struct mdev_device *mdev) { - struct mdev_parent *parent; + struct mdev_parent *parent = mdev->type->parent; int ret; mdev_remove_sysfs_files(mdev); device_del(&mdev->dev); - parent = mdev->parent; lockdep_assert_held(&parent->unreg_sem); ret = parent->ops->remove(mdev); if (ret) @@ -212,7 +211,7 @@ static void mdev_device_release(struct device *dev) struct mdev_device *mdev = to_mdev_device(dev); /* Pairs with the get in mdev_device_create() */ - mdev_put_parent(mdev->parent); + kobject_put(&mdev->type->kobj); mutex_lock(&mdev_list_lock); list_del(&mdev->next); @@ -250,9 +249,8 @@ int mdev_device_create(struct mdev_type *type, const guid_t *uuid) mdev->dev.release = mdev_device_release; mdev->dev.groups = parent->ops->mdev_attr_groups; mdev->type = type; - mdev->parent = parent; /* Pairs with the put in mdev_device_release() */ - mdev_get_parent(parent); + kobject_get(&type->kobj); guid_copy(&mdev->uuid, uuid); list_add(&mdev->next, &mdev_list); @@ -300,7 +298,7 @@ int mdev_device_create(struct mdev_type *type, const guid_t *uuid) int mdev_device_remove(struct mdev_device *mdev) { struct mdev_device *tmp; - struct mdev_parent *parent; + struct mdev_parent *parent = mdev->type->parent; mutex_lock(&mdev_list_lock); list_for_each_entry(tmp, &mdev_list, next) { @@ -321,7 +319,6 @@ int mdev_device_remove(struct mdev_device *mdev) mdev->active = false; mutex_unlock(&mdev_list_lock); - parent = mdev->parent; /* Check if parent unregistration has started */ if (!down_read_trylock(&parent->unreg_sem)) return -ENODEV; diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c index cc9507ed85a181..922729071c5a8e 100644 --- a/drivers/vfio/mdev/vfio_mdev.c +++ b/drivers/vfio/mdev/vfio_mdev.c @@ -24,7 +24,7 @@ static int vfio_mdev_open(struct vfio_device *core_vdev) { struct mdev_device *mdev = to_mdev_device(core_vdev->dev); - struct mdev_parent *parent = mdev->parent; + struct mdev_parent *parent = mdev->type->parent; int ret; @@ -44,7 +44,7 @@ static int vfio_mdev_open(struct vfio_device *core_vdev) static void vfio_mdev_release(struct vfio_device *core_vdev) { struct mdev_device *mdev = to_mdev_device(core_vdev->dev); - struct mdev_parent *parent = mdev->parent; + struct mdev_parent *parent = mdev->type->parent; if (likely(parent->ops->release)) parent->ops->release(mdev); @@ -56,7 +56,7 @@ static long vfio_mdev_unlocked_ioctl(struct vfio_device *core_vdev, unsigned int cmd, unsigned long arg) { struct mdev_device *mdev = to_mdev_device(core_vdev->dev); - struct mdev_parent *parent = mdev->parent; + struct mdev_parent *parent = mdev->type->parent; if (unlikely(!parent->ops->ioctl)) return -EINVAL; @@ -68,7 +68,7 @@ static ssize_t vfio_mdev_read(struct vfio_device *core_vdev, char __user *buf, size_t count, loff_t *ppos) { struct mdev_device *mdev = to_mdev_device(core_vdev->dev); - struct mdev_parent *parent = mdev->parent; + struct mdev_parent *parent = mdev->type->parent; if (unlikely(!parent->ops->read)) return -EINVAL; @@ -81,7 +81,7 @@ static ssize_t vfio_mdev_write(struct vfio_device *core_vdev, loff_t *ppos) { struct mdev_device *mdev = to_mdev_device(core_vdev->dev); - struct mdev_parent *parent = mdev->parent; + struct mdev_parent *parent = mdev->type->parent; if (unlikely(!parent->ops->write)) return -EINVAL; @@ -93,7 +93,7 @@ static int vfio_mdev_mmap(struct vfio_device *core_vdev, struct vm_area_struct *vma) { struct mdev_device *mdev = to_mdev_device(core_vdev->dev); - struct mdev_parent *parent = mdev->parent; + struct mdev_parent *parent = mdev->type->parent; if (unlikely(!parent->ops->mmap)) return -EINVAL; @@ -104,7 +104,7 @@ static int vfio_mdev_mmap(struct vfio_device *core_vdev, static void vfio_mdev_request(struct vfio_device *core_vdev, unsigned int count) { struct mdev_device *mdev = to_mdev_device(core_vdev->dev); - struct mdev_parent *parent = mdev->parent; + struct mdev_parent *parent = mdev->type->parent; if (parent->ops->request) parent->ops->request(mdev, count); diff --git a/include/linux/mdev.h b/include/linux/mdev.h index 349e8ac1fe3382..fb582adda28a9b 100644 --- a/include/linux/mdev.h +++ b/include/linux/mdev.h @@ -14,7 +14,6 @@ struct mdev_type; struct mdev_device { struct device dev; - struct mdev_parent *parent; guid_t uuid; void *driver_data; struct list_head next;
mdev_device->type->parent is the same thing. The struct mdev_device was relying on the kref on the mdev_parent to also indirectly hold a kref on the mdev_type pointer. Now that the type holds a kref on the parent we can directly kref the mdev_type and remove this implicit relationship. Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> --- drivers/vfio/mdev/mdev_core.c | 13 +++++-------- drivers/vfio/mdev/vfio_mdev.c | 14 +++++++------- include/linux/mdev.h | 1 - 3 files changed, 12 insertions(+), 16 deletions(-)