[PATCH 02/13] vfio/mdev: embedd struct mdev_parent in the parent data structure
Kirti Wankhede
kwankhede at nvidia.com
Wed Jun 15 19:29:47 UTC 2022
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
More information about the intel-gvt-dev
mailing list