Re: [PATCH 1/3] Mediated device Core driver

[Jike Song <jike.song@intel.com>]

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