Re: [Qemu-devel] [PATCH v11 01/22] vfio: Mediated device Core driver

[Dong Jia Shi]

* Kirti Wankhede <address@hidden> [2016-11-09 02:36:12 +0530]:

[...]
> >> +/*
> >> + * mdev_register_device : Register a device
> >> + * @dev: device structure representing parent device.
> >> + * @ops: Parent device operation structure to be registered.
> >> + *
> >> + * Add device to list of registered parent devices.
> >> + * Returns a negative value on error, otherwise 0.
> >> + */
> >> +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;
> >> +
> >> +  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");
> >> +
> >> +  list_add(&parent->next, &parent_list);
> >> +  mutex_unlock(&parent_list_lock);
> >> +
> >> +  dev_info(dev, "MDEV: Registered\n");
> >> +  return 0;
> >> +
> >> +add_dev_err:
> >> +  mutex_unlock(&parent_list_lock);
> >> +  if (parent)
> >> +          mdev_put_parent(parent);
> > Why do this? I don't find the place that you call mdev_get_parent above.
> > 
> 
>       kref_init(&parent->ref);
> Above increments the ref_count, so mdev_put_parent() should be called if
> anything fails.
> 
> >> +  else
> >> +          put_device(dev);
> > Shouldn't we always do this?
> > 
> 
> When mdev_put_parent() is called, its release function do this. So if
> mdev_put_parent() is called, we don't need this.
Sorry for missing that. Thanks for the explanation!

> 
> >> +  return ret;
> >> +}
> >> +EXPORT_SYMBOL(mdev_register_device);
> >> +

[...]

-- 
Dong Jia