From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dong Jia Shi Subject: Re: [PATCH v11 01/22] vfio: Mediated device Core driver Date: Wed, 9 Nov 2016 09:09:50 +0800 Message-ID: <1554.11247765205$1505294371@news.gmane.org> References: <1478293856-8191-1-git-send-email-kwankhede@nvidia.com> <1478293856-8191-2-git-send-email-kwankhede@nvidia.com> <20161108092552.GA2090@bjsdjshi@linux.vnet.ibm.com> <8da9a274-281e-0804-0313-3d54d05ce0ad@nvidia.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kevin.tian@intel.com, cjia@nvidia.com, kvm@vger.kernel.org, qemu-devel@nongnu.org, linux-kernel@vger.kernel.org, jike.song@intel.com, alex.williamson@redhat.com, kraxel@redhat.com, pbonzini@redhat.com, bjsdjshi@linux.vnet.ibm.com To: Kirti Wankhede Return-path: Content-Disposition: inline In-Reply-To: <8da9a274-281e-0804-0313-3d54d05ce0ad@nvidia.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org Sender: "Qemu-devel" List-Id: kvm.vger.kernel.org * Kirti Wankhede [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