On 6/29/2016 7:21 PM, Xiao Guangrong wrote:
>
>
> On 06/21/2016 12:31 AM, Kirti Wankhede wrote:
>> Design for Mediated Device Driver:
...
>> +static int mdev_add_attribute_group(struct device *dev,
>> + const struct attribute_group **groups)
>> +{
>> + return sysfs_create_groups(&dev->kobj, groups);
>> +}
>> +
>> +static void mdev_remove_attribute_group(struct device *dev,
>> + const struct attribute_group **groups)
>> +{
>> + sysfs_remove_groups(&dev->kobj, groups);
>> +}
>> +
>
> better use device_add_groups() / device_remove_groups() instead?
>
These are not exported from base module. They can't be used here.
>> +}
>> +
>> +static int mdev_device_create_ops(struct mdev_device *mdev, char
>> *mdev_params)
>> +{
>> + struct parent_device *parent = mdev->parent;
>> + int ret;
>> +
>> + mutex_lock(&parent->ops_lock);
>> + if (parent->ops->create) {
>> + ret = parent->ops->create(mdev->dev.parent, mdev->uuid,
>> + mdev->instance, mdev_params);
>
> I think it is better if we pass @mdev to this callback, then the parent
> driver
> can do its specified operations and associate it with the instance,
> e.g, via mdev->private.
>
Yes, actually I was also thinking of changing it to
- ret = parent->ops->create(mdev->dev.parent, mdev->uuid,
- mdev->instance, mdev_params);
+ ret = parent->ops->create(mdev, mdev_params);
>> +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);
>
> It is not safe as Alex's already pointed it out.
>
>> +
>> + 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);
>
> And no lock to protect these operations.
>
As I replied to Alex also, yes I'm fixing it.
>> +void mdev_unregister_device(struct device *dev)
>> +{
>> + struct parent_device *parent;
>> + struct mdev_device *mdev, *n;
>> + int ret;
>> +
>> + mutex_lock(&parent_devices.list_lock);
>> + parent = find_parent_device(dev);
>> +
>> + if (!parent) {
>> + mutex_unlock(&parent_devices.list_lock);
>> + return;
>> + }
>> + dev_info(dev, "MDEV: Unregistering\n");
>> +
>> + /*
>> + * Remove parent from the list and remove create and destroy sysfs
>> + * files so that no new mediated device could be created for this
>> parent
>> + */
>> + list_del(&parent->next);
>> + mdev_remove_sysfs_files(dev);
>> + mutex_unlock(&parent_devices.list_lock);
>> +
>
> find_parent_device() does not increase the refcount of the parent-device,
> after releasing the lock, is it still safe to use the device?
>
Yes. In mdev_register_device(), kref_init() initialises refcount to 1
and then when mdev child is created refcount is incremented and on child
mdev destroys refcount is decremented. So when all child mdev are
destroyed, refcount will still be 1 until mdev_unregister_device() is
called. So when no mdev device is created, mdev_register_device() hold
parent's refcount and released from mdev_unregister_device().
>> + mutex_lock(&parent->ops_lock);
>> + mdev_remove_attribute_group(dev,
>> + parent->ops->dev_attr_groups);
>
> Why mdev_remove_sysfs_files() and mdev_remove_attribute_group()
> are protected by different locks?
>
As mentioned in reply to Alex on another thread, removing these locks.
>> +
>> +int mdev_device_destroy(struct device *dev, uuid_le uuid, uint32_t
>> instance)
>> +{
>> + struct mdev_device *mdev;
>> + struct parent_device *parent;
>> + int ret;
>> +
>> + parent = mdev_get_parent_by_dev(dev);
>> + if (!parent) {
>> + ret = -EINVAL;
>> + goto destroy_err;
>> + }
>> +
>> + mdev = find_mdev_device(parent, uuid, instance);
>> + if (!mdev) {
>> + ret = -EINVAL;
>> + goto destroy_err;
>> + }
>> +
>> + ret = mdev_device_destroy_ops(mdev, false);
>> + if (ret)
>> + goto destroy_err;
>
> find_mdev_device() does not hold the refcount of mdev, is it safe?
>
Yes, this function is just to check duplicate entry or existence of mdev
device.
>> +
>> + mdev_put_parent(parent);
>> +
>
> The refcount of parent-device is released, you can not continue to
> use it.
>
Removing these locks.
>> + mutex_lock(&parent->mdev_list_lock);
>> + list_del(&mdev->next);
>> + mutex_unlock(&parent->mdev_list_lock);
>> +
>> + mdev_put_device(mdev);
>> + return ret;
>> +
>> +destroy_err:
>> + mdev_put_parent(parent);
>> + return ret;
>> +}
>> +
>> +
>> +static struct class mdev_class = {
>> + .name = MDEV_CLASS_NAME,
>> + .owner = THIS_MODULE,
>> + .class_attrs = mdev_class_attrs,
>
> These interfaces, start and shutdown, are based on UUID, how
> about if we want to operate on the specified instance?
>
Do you mean hot-plug a 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);
>> +}
>
> Hmm, how to prevent if there are parent-devices existing
> when the module is being unloaded?
>
If parent device exits that means that other module is using
mdev_register_device()/mdev_unregister_device() from their module.
'rmmod mdev' would fail until that module is unloaded.
# rmmod mdev
rmmod: ERROR: Module mdev is in use by: nvidia
>> +
>> +static int uuid_parse(const char *str, uuid_le *uuid)
>> +{
>> + int i;
>> +
>> + if (strlen(str) < UUID_CHAR_LENGTH)
>> + return -EINVAL;
>> +
>> + for (i = 0; i < UUID_BYTE_LENGTH; i++) {
>> + if (!isxdigit(str[0]) || !isxdigit(str[1])) {
>> + pr_err("%s err", __func__);
>> + return -EINVAL;
>> + }
>> +
>> + uuid->b[i] = (hex_to_bin(str[0]) << 4) | hex_to_bin(str[1]);
>> + str += 2;
>> + if (is_uuid_sep(*str))
>> + str++;
>> + }
>> +
>> + return 0;
>> +}
>> +
>
> Can we use uuid_le_to_bin()?
I couldn't find this in kernel?
Thanks,
Kirti
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
