| Message ID | c339d6f5-8dba-f69b-b0bc-baf45f2d5ab5@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/29/2016 06:06 PM, Kirti Wankhede wrote: > > > On 10/29/2016 10:00 AM, Jike Song wrote: >> On 10/27/2016 05:29 AM, Kirti Wankhede wrote: >>> +int mdev_register_device(struct device *dev, const struct parent_ops *ops) >>> +{ >>> + int ret; >>> + struct parent_device *parent; >>> + >>> + /* check for mandatory ops */ >>> + if (!ops || !ops->create || !ops->remove || !ops->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) { >>> + ret = -EEXIST; >>> + goto add_dev_err; >>> + } >>> + >>> + parent = kzalloc(sizeof(*parent), GFP_KERNEL); >>> + if (!parent) { >>> + ret = -ENOMEM; >>> + goto add_dev_err; >>> + } >>> + >>> + kref_init(&parent->ref); >>> + mutex_init(&parent->lock); >>> + >>> + parent->dev = dev; >>> + parent->ops = ops; >>> + >>> + ret = parent_create_sysfs_files(parent); >>> + if (ret) { >>> + mutex_unlock(&parent_list_lock); >>> + mdev_put_parent(parent); >>> + 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"); >>> + >> >> Hi Kirti, >> >> Like I replied to previous version: >> >> http://www.spinics.net/lists/kvm/msg139331.html >> > > Hi Jike, > > I saw your reply but by that time v10 version of patch series was out > for review. > Ah..yes, I forgot that :) >> You can always check if mdev_bus_compat_class already registered >> here, and register it if not yet. Same logic should be adopted to >> mdev_init. >> >> Current implementation will simply panic if configured as builtin, >> which is rare but far from impossible. >> > > Can you verify attached patch with v10 patch-set whether this works for you? > I'll incorporate this change in my next version. > Seems cool. But would you please also keep the register in mdev_init(), just check the 'in case it was already registered' case? Thanks! -- Thanks, Jike
On 10/29/2016 11:41 PM, Jike Song wrote: > On 10/29/2016 06:06 PM, Kirti Wankhede wrote: >> >> >> On 10/29/2016 10:00 AM, Jike Song wrote: >>> On 10/27/2016 05:29 AM, Kirti Wankhede wrote: >>>> +int mdev_register_device(struct device *dev, const struct parent_ops *ops) >>>> +{ >>>> + int ret; >>>> + struct parent_device *parent; >>>> + >>>> + /* check for mandatory ops */ >>>> + if (!ops || !ops->create || !ops->remove || !ops->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) { >>>> + ret = -EEXIST; >>>> + goto add_dev_err; >>>> + } >>>> + >>>> + parent = kzalloc(sizeof(*parent), GFP_KERNEL); >>>> + if (!parent) { >>>> + ret = -ENOMEM; >>>> + goto add_dev_err; >>>> + } >>>> + >>>> + kref_init(&parent->ref); >>>> + mutex_init(&parent->lock); >>>> + >>>> + parent->dev = dev; >>>> + parent->ops = ops; >>>> + >>>> + ret = parent_create_sysfs_files(parent); >>>> + if (ret) { >>>> + mutex_unlock(&parent_list_lock); >>>> + mdev_put_parent(parent); >>>> + 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"); >>>> + >>> >>> Hi Kirti, >>> >>> Like I replied to previous version: >>> >>> http://www.spinics.net/lists/kvm/msg139331.html >>> >> >> Hi Jike, >> >> I saw your reply but by that time v10 version of patch series was out >> for review. >> > > Ah..yes, I forgot that :) > >>> You can always check if mdev_bus_compat_class already registered >>> here, and register it if not yet. Same logic should be adopted to >>> mdev_init. >>> >>> Current implementation will simply panic if configured as builtin, >>> which is rare but far from impossible. >>> >> >> Can you verify attached patch with v10 patch-set whether this works for you? >> I'll incorporate this change in my next version. >> > > Seems cool. But would you please also keep the register in mdev_init(), > just check the 'in case it was already registered' case? Thanks! > The class is used only to keep symbolic to the devices which are registered to mdev framework. So I think its ok to register this class when first device is registered. Thanks, Kirti
On 11/02/2016 03:59 PM, Kirti Wankhede wrote: > On 10/29/2016 11:41 PM, Jike Song wrote: >> On 10/29/2016 06:06 PM, Kirti Wankhede wrote: >>> >>> >>> On 10/29/2016 10:00 AM, Jike Song wrote: >>>> On 10/27/2016 05:29 AM, Kirti Wankhede wrote: >>>>> +int mdev_register_device(struct device *dev, const struct parent_ops *ops) >>>>> +{ >>>>> + int ret; >>>>> + struct parent_device *parent; >>>>> + >>>>> + /* check for mandatory ops */ >>>>> + if (!ops || !ops->create || !ops->remove || !ops->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) { >>>>> + ret = -EEXIST; >>>>> + goto add_dev_err; >>>>> + } >>>>> + >>>>> + parent = kzalloc(sizeof(*parent), GFP_KERNEL); >>>>> + if (!parent) { >>>>> + ret = -ENOMEM; >>>>> + goto add_dev_err; >>>>> + } >>>>> + >>>>> + kref_init(&parent->ref); >>>>> + mutex_init(&parent->lock); >>>>> + >>>>> + parent->dev = dev; >>>>> + parent->ops = ops; >>>>> + >>>>> + ret = parent_create_sysfs_files(parent); >>>>> + if (ret) { >>>>> + mutex_unlock(&parent_list_lock); >>>>> + mdev_put_parent(parent); >>>>> + 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"); >>>>> + >>>> >>>> Hi Kirti, >>>> >>>> Like I replied to previous version: >>>> >>>> http://www.spinics.net/lists/kvm/msg139331.html >>>> >>> >>> Hi Jike, >>> >>> I saw your reply but by that time v10 version of patch series was out >>> for review. >>> >> >> Ah..yes, I forgot that :) >> >>>> You can always check if mdev_bus_compat_class already registered >>>> here, and register it if not yet. Same logic should be adopted to >>>> mdev_init. >>>> >>>> Current implementation will simply panic if configured as builtin, >>>> which is rare but far from impossible. >>>> >>> >>> Can you verify attached patch with v10 patch-set whether this works for you? >>> I'll incorporate this change in my next version. >>> >> >> Seems cool. But would you please also keep the register in mdev_init(), >> just check the 'in case it was already registered' case? Thanks! >> > > The class is used only to keep symbolic to the devices which are > registered to mdev framework. So I think its ok to register this class > when first device is registered. > That's also cool :-) So if you like it: Reviewed-by: Jike Song <jike.song@intel.com> -- Thanks, Jike
diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c index 9d8fa5c91c2e..54c59f325336 100644 --- a/drivers/vfio/mdev/mdev_core.c +++ b/drivers/vfio/mdev/mdev_core.c @@ -187,13 +187,18 @@ int mdev_register_device(struct device *dev, const struct parent_ops *ops) parent->dev = dev; parent->ops = ops; - ret = parent_create_sysfs_files(parent); - if (ret) { - mutex_unlock(&parent_list_lock); - mdev_put_parent(parent); - return ret; + 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; + } } + ret = parent_create_sysfs_files(parent); + if (ret) + goto add_dev_err; + ret = class_compat_create_link(mdev_bus_compat_class, dev, NULL); if (ret) dev_warn(dev, "Failed to create compatibility class link\n"); @@ -206,7 +211,10 @@ int mdev_register_device(struct device *dev, const struct parent_ops *ops) add_dev_err: mutex_unlock(&parent_list_lock); - put_device(dev); + if (parent) + mdev_put_parent(parent); + else + put_device(dev); return ret; } EXPORT_SYMBOL(mdev_register_device); @@ -354,12 +362,6 @@ static int __init mdev_init(void) return ret; } - mdev_bus_compat_class = class_compat_register("mdev_bus"); - if (!mdev_bus_compat_class) { - mdev_bus_unregister(); - return -ENOMEM; - } - /* * Attempt to load known vfio_mdev. This gives us a working environment * without the user needing to explicitly load vfio_mdev driver. @@ -371,7 +373,9 @@ static int __init mdev_init(void) static void __exit mdev_exit(void) { - class_compat_unregister(mdev_bus_compat_class); + if (mdev_bus_compat_class) + class_compat_unregister(mdev_bus_compat_class); + mdev_bus_unregister(); }