From mboxrd@z Thu Jan  1 00:00:00 1970
From: Jike Song <jike.song@intel.com>
Subject: Re: [PATCH 1/3] Mediated device Core driver
Date: Mon, 04 Jul 2016 10:08:01 +0800
Message-ID: <5779C501.2050702@intel.com>
References: <1466440308-4961-1-git-send-email-kwankhede@nvidia.com> <1466440308-4961-2-git-send-email-kwankhede@nvidia.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Cc: alex.williamson@redhat.com, pbonzini@redhat.com, kraxel@redhat.com,
	cjia@nvidia.com, qemu-devel@nongnu.org, kvm@vger.kernel.org,
	kevin.tian@intel.com, shuai.ruan@intel.com, zhiyuan.lv@intel.com,
	bjsdjshi@linux.vnet.ibm.com
To: Kirti Wankhede <kwankhede@nvidia.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mga09.intel.com ([134.134.136.24]:3507 "EHLO mga09.intel.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S932236AbcGDCJR (ORCPT <rfc822;kvm@vger.kernel.org>);
	Sun, 3 Jul 2016 22:09:17 -0400
In-Reply-To: <1466440308-4961-2-git-send-email-kwankhede@nvidia.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On 06/21/2016 12:31 AM, Kirti Wankhede wrote:
> +/*
> + * 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 = 0;
> +	struct parent_device *parent;
> +
> +	if (!dev || !ops)
> +		return -EINVAL;
> +
> +	mutex_lock(&parent_devices.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);
> +	list_add(&parent->next, &parent_devices.dev_list);
> +	mutex_unlock(&parent_devices.list_lock);
> +
> +	parent->dev = dev;
> +	parent->ops = ops;
> +	mutex_init(&parent->ops_lock);
> +	mutex_init(&parent->mdev_list_lock);
> +	INIT_LIST_HEAD(&parent->mdev_list);
> +	init_waitqueue_head(&parent->release_done);
> +
> +	ret = mdev_create_sysfs_files(dev);
> +	if (ret)
> +		goto add_sysfs_error;
> +
> +	ret = mdev_add_attribute_group(dev, ops->dev_attr_groups);
> +	if (ret)
> +		goto add_group_error;
> +
> +	dev_info(dev, "MDEV: Registered\n");
> +	return 0;
> +
> +add_group_error:
> +	mdev_remove_sysfs_files(dev);
> +add_sysfs_error:
> +	mutex_lock(&parent_devices.list_lock);
> +	list_del(&parent->next);
> +	mutex_unlock(&parent_devices.list_lock);
> +	mdev_put_parent(parent);
> +	return ret;
> +
> +add_dev_err:
> +	mutex_unlock(&parent_devices.list_lock);
> +	return ret;
> +}
> +EXPORT_SYMBOL(mdev_register_device);
> +
...
> +static int __init mdev_init(void)
> +{
> +	int ret;
> +
> +	mutex_init(&parent_devices.list_lock);
> +	INIT_LIST_HEAD(&parent_devices.dev_list);
> +
> +	ret = class_register(&mdev_class);
> +	if (ret) {
> +		pr_err("Failed to register mdev class\n");
> +		return ret;
> +	}
> +
> +	ret = mdev_bus_register();
> +	if (ret) {
> +		pr_err("Failed to register mdev bus\n");
> +		class_unregister(&mdev_class);
> +		return ret;
> +	}
> +
> +	return ret;
> +}
> +
> +static void __exit mdev_exit(void)
> +{
> +	mdev_bus_unregister();
> +	class_unregister(&mdev_class);
> +}
> +
> +module_init(mdev_init)
> +module_exit(mdev_exit)

Hi Kirti,

I have a question about the order of initialization,

	phy_driver calls mdev_register_device in its __init function;
	mdev_register_device accesses parent_devices.list_lock;
	parent.list_lock is initialized in __init of mdev;

The __init function of both phy driver and mdev are classified with
module_init, if they are selected to be 'Y' in .config, it's possible that in
mdev_register_device(), the mutex is still uninitialized.

The problem here I think is both mdev and phy driver are actually *drivers*,
so once they are builtin, the initialization order is hard to assume.

Do you have any idea to avoid this? Thanks!

 
--
Thanks,
Jike