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

[Jike Song]

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