| Message ID | 20220614045428.278494-3-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/13] vfio/mdev: make mdev.h standalone includable | expand |
> From: Christoph Hellwig > Sent: Tuesday, June 14, 2022 12:54 PM > > Simplify mdev_{un}register_device by requiring the caller to pass in > a structure allocate as part of the parent device structure. This > removes the need for a list of parents and the separate mdev_parent > refcount as we can simplify rely on the reference to the parent device. > > Signed-off-by: Christoph Hellwig <hch@lst.de> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
On 6/14/2022 10:24 AM, Christoph Hellwig wrote: <snip> > /* > - * mdev_register_device : Register a device > + * mdev_register_parent: Register a device as parent for mdevs > + * @parent: parent structure registered > * @dev: device structure representing parent device. > * @mdev_driver: Device driver to bind to the newly created mdev > * > - * Add device to list of registered parent devices. > * Returns a negative value on error, otherwise 0. > */ > -int mdev_register_device(struct device *dev, struct mdev_driver *mdev_driver) > +int mdev_register_parent(struct mdev_parent *parent, struct device *dev, > + struct mdev_driver *mdev_driver) > { > - int ret; > - struct mdev_parent *parent; > char *env_string = "MDEV_STATE=registered"; > char *envp[] = { env_string, NULL }; > + int ret; > > /* check for mandatory ops */ > if (!mdev_driver->supported_type_groups) > return -EINVAL; > > - dev = get_device(dev); > - if (!dev) > - return -EINVAL; > - Hold the reference for device here once rather than helding multiple times while adding sysfs for each mdev_types below. See more below > - mutex_lock(&parent_list_lock); > - > - /* Check for duplicate */ > - parent = __find_parent_device(dev); > - if (parent) { > - parent = NULL; > - ret = -EEXIST; > - goto add_dev_err; > - } > - > - parent = kzalloc(sizeof(*parent), GFP_KERNEL); > - if (!parent) { > - ret = -ENOMEM; > - goto add_dev_err; > - } > - > - kref_init(&parent->ref); > + memset(parent, 0, sizeof(*parent)); > init_rwsem(&parent->unreg_sem); > - > parent->dev = dev; > parent->mdev_driver = mdev_driver; > > if (!mdev_bus_compat_class) { > mdev_bus_compat_class = class_compat_register("mdev_bus"); > - if (!mdev_bus_compat_class) { > - ret = -ENOMEM; > - goto add_dev_err; > - } > + if (!mdev_bus_compat_class) > + return -ENOMEM; > } > > ret = parent_create_sysfs_files(parent); > if (ret) > - goto add_dev_err; > + return ret; > > ret = class_compat_create_link(mdev_bus_compat_class, dev, NULL); > if (ret) > dev_warn(dev, "Failed to create compatibility class link\n"); > > - list_add(&parent->next, &parent_list); > - mutex_unlock(&parent_list_lock); > - > dev_info(dev, "MDEV: Registered\n"); > kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp); > - > return 0; > - > -add_dev_err: > - mutex_unlock(&parent_list_lock); > - if (parent) > - mdev_put_parent(parent); > - else > - put_device(dev); > - return ret; > } > -EXPORT_SYMBOL(mdev_register_device); > +EXPORT_SYMBOL(mdev_register_parent); > > /* > - * mdev_unregister_device : Unregister a parent device > - * @dev: device structure representing parent device. > - * > - * Remove device from list of registered parent devices. Give a chance to free > - * existing mediated devices for given device. > + * mdev_unregister_parent : Unregister a parent device > + * @parent: parent structure to unregister > */ > - > -void mdev_unregister_device(struct device *dev) > +void mdev_unregister_parent(struct mdev_parent *parent) > { > - struct mdev_parent *parent; > char *env_string = "MDEV_STATE=unregistered"; > char *envp[] = { env_string, NULL }; > > - mutex_lock(&parent_list_lock); > - parent = __find_parent_device(dev); > - > - if (!parent) { > - mutex_unlock(&parent_list_lock); > - return; > - } > - dev_info(dev, "MDEV: Unregistering\n"); > - > - list_del(&parent->next); > - mutex_unlock(&parent_list_lock); > + dev_info(parent->dev, "MDEV: Unregistering\n"); > > down_write(&parent->unreg_sem); > - > - class_compat_remove_link(mdev_bus_compat_class, dev, NULL); > - > - device_for_each_child(dev, NULL, mdev_device_remove_cb); > - > + class_compat_remove_link(mdev_bus_compat_class, parent->dev, NULL); > + device_for_each_child(parent->dev, NULL, mdev_device_remove_cb); > parent_remove_sysfs_files(parent); > up_write(&parent->unreg_sem); > > - mdev_put_parent(parent); > - > - /* We still have the caller's reference to use for the uevent */ > - kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp); > + kobject_uevent_env(&parent->dev->kobj, KOBJ_CHANGE, envp); > } > -EXPORT_SYMBOL(mdev_unregister_device); > +EXPORT_SYMBOL(mdev_unregister_parent); > > static void mdev_device_release(struct device *dev) > { > diff --git a/drivers/vfio/mdev/mdev_private.h b/drivers/vfio/mdev/mdev_private.h > index 7c9fc79f3d838..297f911fdc890 100644 > --- a/drivers/vfio/mdev/mdev_private.h > +++ b/drivers/vfio/mdev/mdev_private.h > @@ -13,17 +13,6 @@ > int mdev_bus_register(void); > void mdev_bus_unregister(void); > > -struct mdev_parent { > - struct device *dev; > - struct mdev_driver *mdev_driver; > - struct kref ref; > - struct list_head next; > - struct kset *mdev_types_kset; > - struct list_head type_list; > - /* Synchronize device creation/removal with parent unregistration */ > - struct rw_semaphore unreg_sem; > -}; > - > struct mdev_type { > struct kobject kobj; > struct kobject *devices_kobj; > @@ -48,16 +37,4 @@ void mdev_remove_sysfs_files(struct mdev_device *mdev); > int mdev_device_create(struct mdev_type *kobj, const guid_t *uuid); > int mdev_device_remove(struct mdev_device *dev); > > -void mdev_release_parent(struct kref *kref); > - > -static inline void mdev_get_parent(struct mdev_parent *parent) > -{ > - kref_get(&parent->ref); > -} > - > -static inline void mdev_put_parent(struct mdev_parent *parent) > -{ > - kref_put(&parent->ref, mdev_release_parent); > -} > - > #endif /* MDEV_PRIVATE_H */ > diff --git a/drivers/vfio/mdev/mdev_sysfs.c b/drivers/vfio/mdev/mdev_sysfs.c > index 4bfbf49aaa66a..b71ffc5594870 100644 > --- a/drivers/vfio/mdev/mdev_sysfs.c > +++ b/drivers/vfio/mdev/mdev_sysfs.c > @@ -81,7 +81,7 @@ static void mdev_type_release(struct kobject *kobj) > > pr_debug("Releasing group %s\n", kobj->name); > /* Pairs with the get in add_mdev_supported_type() */ > - mdev_put_parent(type->parent); > + put_device(type->parent->dev); > kfree(type); > } > > @@ -110,7 +110,7 @@ static struct mdev_type *add_mdev_supported_type(struct mdev_parent *parent, > type->kobj.kset = parent->mdev_types_kset; > type->parent = parent; > /* Pairs with the put in mdev_type_release() */ > - mdev_get_parent(parent); > + get_device(parent->dev); > type->type_group_id = type_group_id; > Here device reference is held and release multiple times for each mdev_type. It should be held once from mdev_register_parent() and released from mdev_unregister_parent(). Thanks, Kirti
On Thu, Jun 16, 2022 at 12:59:47AM +0530, Kirti Wankhede wrote: > > @@ -110,7 +110,7 @@ static struct mdev_type *add_mdev_supported_type(struct mdev_parent *parent, > > type->kobj.kset = parent->mdev_types_kset; > > type->parent = parent; > > /* Pairs with the put in mdev_type_release() */ > > - mdev_get_parent(parent); > > + get_device(parent->dev); > > type->type_group_id = type_group_id; > > > > Here device reference is held and release multiple times for each mdev_type. > It should be held once from mdev_register_parent() and released from > mdev_unregister_parent(). It doesn't make any sense to put a paired references in the register/unregister on the struct device - the caller is already required to hold a reference until unregister completes. The reason this is here is because the type->parent is used in a few places and is put back in release: @@ -81,7 +81,7 @@ static void mdev_type_release(struct kobject *kobj) pr_debug("Releasing group %s\n", kobj->name); /* Pairs with the get in add_mdev_supported_type() */ - mdev_put_parent(type->parent); + put_device(type->parent->dev); kfree(type); } If this was a simple sysfs kobj with only a show/store we wouldn't need to do anything as the natural kobj parentage holds a ref up to the struct device - but this kobj is used internally, ie dependent from mdev_device_create(), independently of the normal sysfs life-cycle so that doesn't protect enough either. Perhaps after this series things could be reworked to use the parent instead of the type in more places and perhaps take a device reference on the parent not the kobj in mdev_device_create() (which is screwy already), but at least this patch is correct as is. Jason
On Tue, Jun 14, 2022 at 06:54:17AM +0200, Christoph Hellwig wrote: > Simplify mdev_{un}register_device by requiring the caller to pass in > a structure allocate as part of the parent device structure. This > removes the need for a list of parents and the separate mdev_parent > refcount as we can simplify rely on the reference to the parent device. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > Reviewed-by: Kevin Tian <kevin.tian@intel.com> > --- > .../driver-api/vfio-mediated-device.rst | 12 +- > Documentation/s390/vfio-ap.rst | 2 +- > Documentation/s390/vfio-ccw.rst | 2 +- > drivers/gpu/drm/i915/gvt/gvt.h | 2 + > drivers/gpu/drm/i915/gvt/kvmgt.c | 5 +- > drivers/s390/cio/cio.h | 2 + > drivers/s390/cio/vfio_ccw_ops.c | 6 +- > drivers/s390/crypto/vfio_ap_ops.c | 5 +- > drivers/s390/crypto/vfio_ap_private.h | 1 + > drivers/vfio/mdev/mdev_core.c | 116 +++--------------- > drivers/vfio/mdev/mdev_private.h | 23 ---- > drivers/vfio/mdev/mdev_sysfs.c | 4 +- > include/linux/mdev.h | 15 ++- > samples/vfio-mdev/mbochs.c | 5 +- > samples/vfio-mdev/mdpy.c | 5 +- > samples/vfio-mdev/mtty.c | 6 +- > 16 files changed, 65 insertions(+), 146 deletions(-) Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> Jason
On 6/24/2022 1:48 AM, Jason Gunthorpe wrote: > On Thu, Jun 16, 2022 at 12:59:47AM +0530, Kirti Wankhede wrote: > >>> @@ -110,7 +110,7 @@ static struct mdev_type *add_mdev_supported_type(struct mdev_parent *parent, >>> type->kobj.kset = parent->mdev_types_kset; >>> type->parent = parent; >>> /* Pairs with the put in mdev_type_release() */ >>> - mdev_get_parent(parent); >>> + get_device(parent->dev); >>> type->type_group_id = type_group_id; >>> >> >> Here device reference is held and release multiple times for each mdev_type. >> It should be held once from mdev_register_parent() and released from >> mdev_unregister_parent(). > > It doesn't make any sense to put a paired references in the > register/unregister on the struct device - the caller is already > required to hold a reference until unregister completes. > then this need to be documented in the comment above. > The reason this is here is because the type->parent is used in a few > places and is put back in release: > > @@ -81,7 +81,7 @@ static void mdev_type_release(struct kobject *kobj) > > pr_debug("Releasing group %s\n", kobj->name); > /* Pairs with the get in add_mdev_supported_type() */ > - mdev_put_parent(type->parent); > + put_device(type->parent->dev); > kfree(type); > } > > If this was a simple sysfs kobj with only a show/store we wouldn't > need to do anything as the natural kobj parentage holds a ref up to > the struct device - but this kobj is used internally, ie dependent > from mdev_device_create(), independently of the normal sysfs > life-cycle so that doesn't protect enough either. > Life span of 'type' is from mdev_register_device to mdev_unregister_device. If device/parent is being unregistered then only types are removed, so referencing 'type' from mdev_device_create() is still safe. Therefore, parent device's reference should be held and release from register-unregister call. Thanks, Kirti
On Fri, Jun 24, 2022 at 05:59:58PM +0530, Kirti Wankhede wrote: > > The reason this is here is because the type->parent is used in a few > > places and is put back in release: > > > > @@ -81,7 +81,7 @@ static void mdev_type_release(struct kobject *kobj) > > > > pr_debug("Releasing group %s\n", kobj->name); > > /* Pairs with the get in add_mdev_supported_type() */ > > - mdev_put_parent(type->parent); > > + put_device(type->parent->dev); > > kfree(type); > > } > > > > If this was a simple sysfs kobj with only a show/store we wouldn't > > need to do anything as the natural kobj parentage holds a ref up to > > the struct device - but this kobj is used internally, ie dependent > > from mdev_device_create(), independently of the normal sysfs > > life-cycle so that doesn't protect enough either. > > > > > Life span of 'type' is from mdev_register_device to mdev_unregister_device. > If device/parent is being unregistered then only types are removed, so > referencing 'type' from mdev_device_create() is still safe. Therefore, > parent device's reference should be held and release from > register-unregister call. No, I've already explained this. Jason
On 6/24/2022 6:03 PM, Jason Gunthorpe wrote: > On Fri, Jun 24, 2022 at 05:59:58PM +0530, Kirti Wankhede wrote: > >>> The reason this is here is because the type->parent is used in a few >>> places and is put back in release: >>> >>> @@ -81,7 +81,7 @@ static void mdev_type_release(struct kobject *kobj) >>> >>> pr_debug("Releasing group %s\n", kobj->name); >>> /* Pairs with the get in add_mdev_supported_type() */ >>> - mdev_put_parent(type->parent); >>> + put_device(type->parent->dev); >>> kfree(type); >>> } >>> >>> If this was a simple sysfs kobj with only a show/store we wouldn't >>> need to do anything as the natural kobj parentage holds a ref up to >>> the struct device - but this kobj is used internally, ie dependent >>> from mdev_device_create(), independently of the normal sysfs >>> life-cycle so that doesn't protect enough either. >>> >> >> >> Life span of 'type' is from mdev_register_device to mdev_unregister_device. >> If device/parent is being unregistered then only types are removed, so >> referencing 'type' from mdev_device_create() is still safe. Therefore, >> parent device's reference should be held and release from >> register-unregister call. > > No, I've already explained this. Its not correct. kobject_init_and_add(&type->kobj, ...) which called from mdev_register_parent() -> parent_create_sysfs_files() holds reference for type->kobj This is released from mdev_unregister_parent() -> parent_remove_sysfs_files() -> kset_unregister() In the next patch [3/13] of this series, these calltraces are changed as mdev_register_parent() -> mdev_type_add() -> kobject_init_and_add(&type->kobj, ...) holds reference for type->kobj which is released from mdev_unregister_parent() -> mdev_type_remove() -> kobject_put(&type->kobj) Thanks, Kirti
On Fri, Jun 24, 2022 at 06:23:48PM +0530, Kirti Wankhede wrote: > > > On 6/24/2022 6:03 PM, Jason Gunthorpe wrote: > > On Fri, Jun 24, 2022 at 05:59:58PM +0530, Kirti Wankhede wrote: > > > > > > The reason this is here is because the type->parent is used in a few > > > > places and is put back in release: > > > > > > > > @@ -81,7 +81,7 @@ static void mdev_type_release(struct kobject *kobj) > > > > > > > > pr_debug("Releasing group %s\n", kobj->name); > > > > /* Pairs with the get in add_mdev_supported_type() */ > > > > - mdev_put_parent(type->parent); > > > > + put_device(type->parent->dev); > > > > kfree(type); > > > > } > > > > > > > > If this was a simple sysfs kobj with only a show/store we wouldn't > > > > need to do anything as the natural kobj parentage holds a ref up to > > > > the struct device - but this kobj is used internally, ie dependent > > > > from mdev_device_create(), independently of the normal sysfs > > > > life-cycle so that doesn't protect enough either. > > > > > > > > > > > > > Life span of 'type' is from mdev_register_device to mdev_unregister_device. > > > If device/parent is being unregistered then only types are removed, so > > > referencing 'type' from mdev_device_create() is still safe. Therefore, > > > parent device's reference should be held and release from > > > register-unregister call. > > > > No, I've already explained this. > > Its not correct. > > kobject_init_and_add(&type->kobj, ...) which called from > mdev_register_parent() > -> parent_create_sysfs_files() holds reference for type->kobj -> add_mdev_supported_type_groups() -> add_mdev_supported_type() -> kobject_init_and_add(&type->kobj) > This is released from > mdev_unregister_parent() > -> parent_remove_sysfs_files() > -> kset_unregister() It is not kset_unregister() that puts back. -> remove_mdev_supported_type() -> kobject_put(&type->kobj) // pairs with kobject_init_and_add So what is the issue? This is a properly paired usage of the ref. > In the next patch [3/13] of this series, these calltraces are changed as > mdev_register_parent() > -> mdev_type_add() > -> kobject_init_and_add(&type->kobj, ...) holds reference for > type->kobj > > which is released from > > mdev_unregister_parent() > -> mdev_type_remove() > -> kobject_put(&type->kobj) This is the same logic? What is the problem? Jason
On 6/24/2022 6:35 PM, Jason Gunthorpe wrote: > On Fri, Jun 24, 2022 at 06:23:48PM +0530, Kirti Wankhede wrote: >> >> >> On 6/24/2022 6:03 PM, Jason Gunthorpe wrote: >>> On Fri, Jun 24, 2022 at 05:59:58PM +0530, Kirti Wankhede wrote: >>> >>>>> The reason this is here is because the type->parent is used in a few >>>>> places and is put back in release: >>>>> >>>>> @@ -81,7 +81,7 @@ static void mdev_type_release(struct kobject *kobj) >>>>> >>>>> pr_debug("Releasing group %s\n", kobj->name); >>>>> /* Pairs with the get in add_mdev_supported_type() */ >>>>> - mdev_put_parent(type->parent); >>>>> + put_device(type->parent->dev); >>>>> kfree(type); >>>>> } >>>>> >>>>> If this was a simple sysfs kobj with only a show/store we wouldn't >>>>> need to do anything as the natural kobj parentage holds a ref up to >>>>> the struct device - but this kobj is used internally, ie dependent >>>>> from mdev_device_create(), independently of the normal sysfs >>>>> life-cycle so that doesn't protect enough either. >>>>> >>>> >>>> >>>> Life span of 'type' is from mdev_register_device to mdev_unregister_device. >>>> If device/parent is being unregistered then only types are removed, so >>>> referencing 'type' from mdev_device_create() is still safe. Therefore, >>>> parent device's reference should be held and release from >>>> register-unregister call. >>> >>> No, I've already explained this. >> >> Its not correct. >> >> kobject_init_and_add(&type->kobj, ...) which called from >> mdev_register_parent() >> -> parent_create_sysfs_files() holds reference for type->kobj > -> add_mdev_supported_type_groups() > -> add_mdev_supported_type() > -> kobject_init_and_add(&type->kobj) > >> This is released from >> mdev_unregister_parent() >> -> parent_remove_sysfs_files() >> -> kset_unregister() > > It is not kset_unregister() that puts back. > -> remove_mdev_supported_type() > -> kobject_put(&type->kobj) // pairs with kobject_init_and_add > that's correct, my bad. > So what is the issue? This is a properly paired usage of the ref. > >> In the next patch [3/13] of this series, these calltraces are changed as >> mdev_register_parent() >> -> mdev_type_add() >> -> kobject_init_and_add(&type->kobj, ...) holds reference for >> type->kobj >> >> which is released from >> >> mdev_unregister_parent() >> -> mdev_type_remove() >> -> kobject_put(&type->kobj) > > This is the same logic? What is the problem? > Pasting here your comment: >>>>> the struct device - but this kobj is used internally, ie dependent >>>>> from mdev_device_create(), independently of the normal sysfs >>>>> life-cycle so that doesn't protect enough either. Since there references are held, its safe. Thanks, Kirti
diff --git a/Documentation/driver-api/vfio-mediated-device.rst b/Documentation/driver-api/vfio-mediated-device.rst index eded8719180fb..3749f59c855fa 100644 --- a/Documentation/driver-api/vfio-mediated-device.rst +++ b/Documentation/driver-api/vfio-mediated-device.rst @@ -60,19 +60,19 @@ devices as examples, as these devices are the first devices to use this module:: | MDEV CORE | | MODULE | | mdev.ko | - | +-----------+ | mdev_register_device() +--------------+ + | +-----------+ | mdev_register_parent() +--------------+ | | | +<------------------------+ | | | | | | nvidia.ko |<-> physical | | | +------------------------>+ | device | | | | callbacks +--------------+ | | Physical | | - | | device | | mdev_register_device() +--------------+ + | | device | | mdev_register_parent() +--------------+ | | interface | |<------------------------+ | | | | | | i915.ko |<-> physical | | | +------------------------>+ | device | | | | callbacks +--------------+ | | | | - | | | | mdev_register_device() +--------------+ + | | | | mdev_register_parent() +--------------+ | | | +<------------------------+ | | | | | | ccw_device.ko|<-> physical | | | +------------------------>+ | device @@ -127,8 +127,8 @@ vfio_device_ops. When a driver wants to add the GUID creation sysfs to an existing device it has probe'd to then it should call:: - extern int mdev_register_device(struct device *dev, - struct mdev_driver *mdev_driver); + int mdev_register_parent(struct mdev_parent *parent, struct device *dev, + struct mdev_driver *mdev_driver); This will provide the 'mdev_supported_types/XX/create' files which can then be used to trigger the creation of a mdev_device. The created mdev_device will be @@ -136,7 +136,7 @@ attached to the specified driver. When the driver needs to remove itself it calls:: - extern void mdev_unregister_device(struct device *dev); + void mdev_unregister_parent(struct mdev_parent *parent); Which will unbind and destroy all the created mdevs and remove the sysfs files. diff --git a/Documentation/s390/vfio-ap.rst b/Documentation/s390/vfio-ap.rst index f57ae621f33e8..37e16158c7fbf 100644 --- a/Documentation/s390/vfio-ap.rst +++ b/Documentation/s390/vfio-ap.rst @@ -299,7 +299,7 @@ of the VFIO AP mediated matrix device driver:: | MDEV CORE | | MODULE | | mdev.ko | - | +---------+ | mdev_register_device() +--------------+ + | +---------+ | mdev_register_parent() +--------------+ | |Physical | +<-----------------------+ | | | device | | | vfio_ap.ko |<-> matrix | |interface| +----------------------->+ | device diff --git a/Documentation/s390/vfio-ccw.rst b/Documentation/s390/vfio-ccw.rst index 8aad08a8b8a50..ea928a3806f43 100644 --- a/Documentation/s390/vfio-ccw.rst +++ b/Documentation/s390/vfio-ccw.rst @@ -156,7 +156,7 @@ Below is a high Level block diagram:: | MDEV CORE | | MODULE | | mdev.ko | - | +---------+ | mdev_register_device() +--------------+ + | +---------+ | mdev_register_parent() +--------------+ | |Physical | +<-----------------------+ | | | device | | | vfio_ccw.ko |<-> subchannel | |interface| +----------------------->+ | device diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h index aee1a45da74bc..ddffd337f1c60 100644 --- a/drivers/gpu/drm/i915/gvt/gvt.h +++ b/drivers/gpu/drm/i915/gvt/gvt.h @@ -36,6 +36,7 @@ #include <uapi/linux/pci_regs.h> #include <linux/kvm_host.h> #include <linux/vfio.h> +#include <linux/mdev.h> #include "i915_drv.h" #include "intel_gvt.h" @@ -327,6 +328,7 @@ struct intel_gvt { struct intel_gvt_workload_scheduler scheduler; struct notifier_block shadow_ctx_notifier_block[I915_NUM_ENGINES]; DECLARE_HASHTABLE(cmd_table, GVT_CMD_HASH_BITS); + struct mdev_parent parent; struct intel_vgpu_type *types; unsigned int num_types; struct intel_vgpu *idle_vgpu; diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c index 50c0081c4f0d5..70401374c72bc 100644 --- a/drivers/gpu/drm/i915/gvt/kvmgt.c +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c @@ -1958,7 +1958,7 @@ static void intel_gvt_clean_device(struct drm_i915_private *i915) if (drm_WARN_ON(&i915->drm, !gvt)) return; - mdev_unregister_device(i915->drm.dev); + mdev_unregister_parent(&gvt->parent); intel_gvt_cleanup_vgpu_type_groups(gvt); intel_gvt_destroy_idle_vgpu(gvt->idle_vgpu); intel_gvt_clean_vgpu_types(gvt); @@ -2063,7 +2063,8 @@ static int intel_gvt_init_device(struct drm_i915_private *i915) if (ret) goto out_destroy_idle_vgpu; - ret = mdev_register_device(i915->drm.dev, &intel_vgpu_mdev_driver); + ret = mdev_register_parent(&gvt->parent, i915->drm.dev, + &intel_vgpu_mdev_driver); if (ret) goto out_cleanup_vgpu_type_groups; diff --git a/drivers/s390/cio/cio.h b/drivers/s390/cio/cio.h index fa8df50bb49e3..22be5ac7d23c1 100644 --- a/drivers/s390/cio/cio.h +++ b/drivers/s390/cio/cio.h @@ -5,6 +5,7 @@ #include <linux/mutex.h> #include <linux/device.h> #include <linux/mod_devicetable.h> +#include <linux/mdev.h> #include <asm/chpid.h> #include <asm/cio.h> #include <asm/fcx.h> @@ -108,6 +109,7 @@ struct subchannel { * frees it. Use driver_set_override() to set or clear it. */ const char *driver_override; + struct mdev_parent parent; } __attribute__ ((aligned(8))); DECLARE_PER_CPU_ALIGNED(struct irb, cio_irb); diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c index b49e2e9db2dc6..9192a21085ce4 100644 --- a/drivers/s390/cio/vfio_ccw_ops.c +++ b/drivers/s390/cio/vfio_ccw_ops.c @@ -11,7 +11,6 @@ */ #include <linux/vfio.h> -#include <linux/mdev.h> #include <linux/nospec.h> #include <linux/slab.h> @@ -660,10 +659,11 @@ struct mdev_driver vfio_ccw_mdev_driver = { int vfio_ccw_mdev_reg(struct subchannel *sch) { - return mdev_register_device(&sch->dev, &vfio_ccw_mdev_driver); + return mdev_register_parent(&sch->parent, &sch->dev, + &vfio_ccw_mdev_driver); } void vfio_ccw_mdev_unreg(struct subchannel *sch) { - mdev_unregister_device(&sch->dev); + mdev_unregister_parent(&sch->parent); } diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c index a7d2a95796d36..834945150dc9f 100644 --- a/drivers/s390/crypto/vfio_ap_ops.c +++ b/drivers/s390/crypto/vfio_ap_ops.c @@ -1485,7 +1485,8 @@ int vfio_ap_mdev_register(void) if (ret) return ret; - ret = mdev_register_device(&matrix_dev->device, &vfio_ap_matrix_driver); + ret = mdev_register_parent(&matrix_dev->parent, &matrix_dev->device, + &vfio_ap_matrix_driver); if (ret) goto err_driver; return 0; @@ -1497,6 +1498,6 @@ int vfio_ap_mdev_register(void) void vfio_ap_mdev_unregister(void) { - mdev_unregister_device(&matrix_dev->device); + mdev_unregister_parent(&matrix_dev->parent); mdev_unregister_driver(&vfio_ap_matrix_driver); } diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h index 6616aa83347ad..0191f6bc973a4 100644 --- a/drivers/s390/crypto/vfio_ap_private.h +++ b/drivers/s390/crypto/vfio_ap_private.h @@ -45,6 +45,7 @@ struct ap_matrix_dev { struct list_head mdev_list; struct mutex lock; struct ap_driver *vfio_ap_drv; + struct mdev_parent parent; }; extern struct ap_matrix_dev *matrix_dev; diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c index 2c32923fbad27..d38faed14c689 100644 --- a/drivers/vfio/mdev/mdev_core.c +++ b/drivers/vfio/mdev/mdev_core.c @@ -18,8 +18,6 @@ #define DRIVER_AUTHOR "NVIDIA Corporation" #define DRIVER_DESC "Mediated device Core Driver" -static LIST_HEAD(parent_list); -static DEFINE_MUTEX(parent_list_lock); static struct class_compat *mdev_bus_compat_class; static LIST_HEAD(mdev_list); @@ -61,28 +59,6 @@ struct device *mtype_get_parent_dev(struct mdev_type *mtype) } EXPORT_SYMBOL(mtype_get_parent_dev); -/* Should be called holding parent_list_lock */ -static struct mdev_parent *__find_parent_device(struct device *dev) -{ - struct mdev_parent *parent; - - list_for_each_entry(parent, &parent_list, next) { - if (parent->dev == dev) - return parent; - } - return NULL; -} - -void mdev_release_parent(struct kref *kref) -{ - struct mdev_parent *parent = container_of(kref, struct mdev_parent, - ref); - struct device *dev = parent->dev; - - kfree(parent); - put_device(dev); -} - /* Caller must hold parent unreg_sem read or write lock */ static void mdev_device_remove_common(struct mdev_device *mdev) { @@ -105,125 +81,69 @@ static int mdev_device_remove_cb(struct device *dev, void *data) } /* - * mdev_register_device : Register a device + * mdev_register_parent: Register a device as parent for mdevs + * @parent: parent structure registered * @dev: device structure representing parent device. * @mdev_driver: Device driver to bind to the newly created mdev * - * Add device to list of registered parent devices. * Returns a negative value on error, otherwise 0. */ -int mdev_register_device(struct device *dev, struct mdev_driver *mdev_driver) +int mdev_register_parent(struct mdev_parent *parent, struct device *dev, + struct mdev_driver *mdev_driver) { - int ret; - struct mdev_parent *parent; char *env_string = "MDEV_STATE=registered"; char *envp[] = { env_string, NULL }; + int ret; /* check for mandatory ops */ if (!mdev_driver->supported_type_groups) return -EINVAL; - dev = get_device(dev); - if (!dev) - return -EINVAL; - - mutex_lock(&parent_list_lock); - - /* Check for duplicate */ - parent = __find_parent_device(dev); - if (parent) { - parent = NULL; - ret = -EEXIST; - goto add_dev_err; - } - - parent = kzalloc(sizeof(*parent), GFP_KERNEL); - if (!parent) { - ret = -ENOMEM; - goto add_dev_err; - } - - kref_init(&parent->ref); + memset(parent, 0, sizeof(*parent)); init_rwsem(&parent->unreg_sem); - parent->dev = dev; parent->mdev_driver = mdev_driver; if (!mdev_bus_compat_class) { mdev_bus_compat_class = class_compat_register("mdev_bus"); - if (!mdev_bus_compat_class) { - ret = -ENOMEM; - goto add_dev_err; - } + if (!mdev_bus_compat_class) + return -ENOMEM; } ret = parent_create_sysfs_files(parent); if (ret) - goto add_dev_err; + return ret; ret = class_compat_create_link(mdev_bus_compat_class, dev, NULL); if (ret) dev_warn(dev, "Failed to create compatibility class link\n"); - list_add(&parent->next, &parent_list); - mutex_unlock(&parent_list_lock); - dev_info(dev, "MDEV: Registered\n"); kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp); - return 0; - -add_dev_err: - mutex_unlock(&parent_list_lock); - if (parent) - mdev_put_parent(parent); - else - put_device(dev); - return ret; } -EXPORT_SYMBOL(mdev_register_device); +EXPORT_SYMBOL(mdev_register_parent); /* - * mdev_unregister_device : Unregister a parent device - * @dev: device structure representing parent device. - * - * Remove device from list of registered parent devices. Give a chance to free - * existing mediated devices for given device. + * mdev_unregister_parent : Unregister a parent device + * @parent: parent structure to unregister */ - -void mdev_unregister_device(struct device *dev) +void mdev_unregister_parent(struct mdev_parent *parent) { - struct mdev_parent *parent; char *env_string = "MDEV_STATE=unregistered"; char *envp[] = { env_string, NULL }; - mutex_lock(&parent_list_lock); - parent = __find_parent_device(dev); - - if (!parent) { - mutex_unlock(&parent_list_lock); - return; - } - dev_info(dev, "MDEV: Unregistering\n"); - - list_del(&parent->next); - mutex_unlock(&parent_list_lock); + dev_info(parent->dev, "MDEV: Unregistering\n"); down_write(&parent->unreg_sem); - - class_compat_remove_link(mdev_bus_compat_class, dev, NULL); - - device_for_each_child(dev, NULL, mdev_device_remove_cb); - + class_compat_remove_link(mdev_bus_compat_class, parent->dev, NULL); + device_for_each_child(parent->dev, NULL, mdev_device_remove_cb); parent_remove_sysfs_files(parent); up_write(&parent->unreg_sem); - mdev_put_parent(parent); - - /* We still have the caller's reference to use for the uevent */ - kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp); + kobject_uevent_env(&parent->dev->kobj, KOBJ_CHANGE, envp); } -EXPORT_SYMBOL(mdev_unregister_device); +EXPORT_SYMBOL(mdev_unregister_parent); static void mdev_device_release(struct device *dev) { diff --git a/drivers/vfio/mdev/mdev_private.h b/drivers/vfio/mdev/mdev_private.h index 7c9fc79f3d838..297f911fdc890 100644 --- a/drivers/vfio/mdev/mdev_private.h +++ b/drivers/vfio/mdev/mdev_private.h @@ -13,17 +13,6 @@ int mdev_bus_register(void); void mdev_bus_unregister(void); -struct mdev_parent { - struct device *dev; - struct mdev_driver *mdev_driver; - struct kref ref; - struct list_head next; - struct kset *mdev_types_kset; - struct list_head type_list; - /* Synchronize device creation/removal with parent unregistration */ - struct rw_semaphore unreg_sem; -}; - struct mdev_type { struct kobject kobj; struct kobject *devices_kobj; @@ -48,16 +37,4 @@ void mdev_remove_sysfs_files(struct mdev_device *mdev); int mdev_device_create(struct mdev_type *kobj, const guid_t *uuid); int mdev_device_remove(struct mdev_device *dev); -void mdev_release_parent(struct kref *kref); - -static inline void mdev_get_parent(struct mdev_parent *parent) -{ - kref_get(&parent->ref); -} - -static inline void mdev_put_parent(struct mdev_parent *parent) -{ - kref_put(&parent->ref, mdev_release_parent); -} - #endif /* MDEV_PRIVATE_H */ diff --git a/drivers/vfio/mdev/mdev_sysfs.c b/drivers/vfio/mdev/mdev_sysfs.c index 4bfbf49aaa66a..b71ffc5594870 100644 --- a/drivers/vfio/mdev/mdev_sysfs.c +++ b/drivers/vfio/mdev/mdev_sysfs.c @@ -81,7 +81,7 @@ static void mdev_type_release(struct kobject *kobj) pr_debug("Releasing group %s\n", kobj->name); /* Pairs with the get in add_mdev_supported_type() */ - mdev_put_parent(type->parent); + put_device(type->parent->dev); kfree(type); } @@ -110,7 +110,7 @@ static struct mdev_type *add_mdev_supported_type(struct mdev_parent *parent, type->kobj.kset = parent->mdev_types_kset; type->parent = parent; /* Pairs with the put in mdev_type_release() */ - mdev_get_parent(parent); + get_device(parent->dev); type->type_group_id = type_group_id; ret = kobject_init_and_add(&type->kobj, &mdev_type_ktype, NULL, diff --git a/include/linux/mdev.h b/include/linux/mdev.h index 555c1d015b5f0..327ce3e5c6b5f 100644 --- a/include/linux/mdev.h +++ b/include/linux/mdev.h @@ -23,6 +23,16 @@ struct mdev_device { bool active; }; +/* embedded into the struct device that the mdev devices hang off */ +struct mdev_parent { + struct device *dev; + struct mdev_driver *mdev_driver; + struct kset *mdev_types_kset; + struct list_head type_list; + /* Synchronize device creation/removal with parent unregistration */ + struct rw_semaphore unreg_sem; +}; + static inline struct mdev_device *to_mdev_device(struct device *dev) { return container_of(dev, struct mdev_device, dev); @@ -75,8 +85,9 @@ static inline const guid_t *mdev_uuid(struct mdev_device *mdev) extern struct bus_type mdev_bus_type; -int mdev_register_device(struct device *dev, struct mdev_driver *mdev_driver); -void mdev_unregister_device(struct device *dev); +int mdev_register_parent(struct mdev_parent *parent, struct device *dev, + struct mdev_driver *mdev_driver); +void mdev_unregister_parent(struct mdev_parent *parent); int mdev_register_driver(struct mdev_driver *drv); void mdev_unregister_driver(struct mdev_driver *drv); diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-mdev/mbochs.c index d0d1bb7747240..30b3643b3b389 100644 --- a/samples/vfio-mdev/mbochs.c +++ b/samples/vfio-mdev/mbochs.c @@ -128,6 +128,7 @@ static dev_t mbochs_devt; static struct class *mbochs_class; static struct cdev mbochs_cdev; static struct device mbochs_dev; +static struct mdev_parent mbochs_parent; static atomic_t mbochs_avail_mbytes; static const struct vfio_device_ops mbochs_dev_ops; @@ -1456,7 +1457,7 @@ static int __init mbochs_dev_init(void) if (ret) goto err_class; - ret = mdev_register_device(&mbochs_dev, &mbochs_driver); + ret = mdev_register_parent(&mbochs_parent, &mbochs_dev, &mbochs_driver); if (ret) goto err_device; @@ -1477,7 +1478,7 @@ static int __init mbochs_dev_init(void) static void __exit mbochs_dev_exit(void) { mbochs_dev.bus = NULL; - mdev_unregister_device(&mbochs_dev); + mdev_unregister_parent(&mbochs_parent); device_unregister(&mbochs_dev); mdev_unregister_driver(&mbochs_driver); diff --git a/samples/vfio-mdev/mdpy.c b/samples/vfio-mdev/mdpy.c index 0c4ca1f4be7ed..132bb055628a6 100644 --- a/samples/vfio-mdev/mdpy.c +++ b/samples/vfio-mdev/mdpy.c @@ -83,6 +83,7 @@ static dev_t mdpy_devt; static struct class *mdpy_class; static struct cdev mdpy_cdev; static struct device mdpy_dev; +static struct mdev_parent mdpy_parent; static u32 mdpy_count; static const struct vfio_device_ops mdpy_dev_ops; @@ -765,7 +766,7 @@ static int __init mdpy_dev_init(void) if (ret) goto err_class; - ret = mdev_register_device(&mdpy_dev, &mdpy_driver); + ret = mdev_register_parent(&mdpy_parent, &mdpy_dev, &mdpy_driver); if (ret) goto err_device; @@ -786,7 +787,7 @@ static int __init mdpy_dev_init(void) static void __exit mdpy_dev_exit(void) { mdpy_dev.bus = NULL; - mdev_unregister_device(&mdpy_dev); + mdev_unregister_parent(&mdpy_parent); device_unregister(&mdpy_dev); mdev_unregister_driver(&mdpy_driver); diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c index 4f5a6f2d3629d..8ba5f6084a093 100644 --- a/samples/vfio-mdev/mtty.c +++ b/samples/vfio-mdev/mtty.c @@ -72,6 +72,7 @@ static struct mtty_dev { struct cdev vd_cdev; struct idr vd_idr; struct device dev; + struct mdev_parent parent; } mtty_dev; struct mdev_region_info { @@ -1350,7 +1351,8 @@ static int __init mtty_dev_init(void) if (ret) goto err_class; - ret = mdev_register_device(&mtty_dev.dev, &mtty_driver); + ret = mdev_register_parent(&mtty_dev.parent, &mtty_dev.dev, + &mtty_driver); if (ret) goto err_device; return 0; @@ -1370,7 +1372,7 @@ static int __init mtty_dev_init(void) static void __exit mtty_dev_exit(void) { mtty_dev.dev.bus = NULL; - mdev_unregister_device(&mtty_dev.dev); + mdev_unregister_parent(&mtty_dev.parent); device_unregister(&mtty_dev.dev); idr_destroy(&mtty_dev.vd_idr);
Simplify mdev_{un}register_device by requiring the caller to pass in a structure allocate as part of the parent device structure. This removes the need for a list of parents and the separate mdev_parent refcount as we can simplify rely on the reference to the parent device. Signed-off-by: Christoph Hellwig <hch@lst.de> --- .../driver-api/vfio-mediated-device.rst | 12 +- Documentation/s390/vfio-ap.rst | 2 +- Documentation/s390/vfio-ccw.rst | 2 +- drivers/gpu/drm/i915/gvt/gvt.h | 2 + drivers/gpu/drm/i915/gvt/kvmgt.c | 5 +- drivers/s390/cio/cio.h | 2 + drivers/s390/cio/vfio_ccw_ops.c | 6 +- drivers/s390/crypto/vfio_ap_ops.c | 5 +- drivers/s390/crypto/vfio_ap_private.h | 1 + drivers/vfio/mdev/mdev_core.c | 116 +++--------------- drivers/vfio/mdev/mdev_private.h | 23 ---- drivers/vfio/mdev/mdev_sysfs.c | 4 +- include/linux/mdev.h | 15 ++- samples/vfio-mdev/mbochs.c | 5 +- samples/vfio-mdev/mdpy.c | 5 +- samples/vfio-mdev/mtty.c | 6 +- 16 files changed, 65 insertions(+), 146 deletions(-)