Re: [PATCH v7 1/4] vfio: Mediated device Core driver

[Kirti Wankhede <kwankhede@nvidia.com>]

On 9/8/2016 1:39 PM, Jike Song wrote:
> On 08/25/2016 11:53 AM, Kirti Wankhede wrote:

>>  +---------------+
>>  |               |
>>  | +-----------+ |  mdev_register_driver() +--------------+
>>  | |           | +<------------------------+ __init()     |
>>  | |  mdev     | |                         |              |
>>  | |  bus      | +------------------------>+              |<-> VFIO user
>>  | |  driver   | |     probe()/remove()    | vfio_mdev.ko |    APIs
>>  | |           | |                         |              |
>>  | +-----------+ |                         +--------------+
>>  |               |
> 
> This aimed to have only one single vfio bus driver for all mediated devices,
> right?
>

Yes. That's correct.


>> +
>> +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);
>> +}
> 
> These functions are not necessary. You can always specify the attribute groups
> to dev->groups before registering a new device.
> 

At the time of mdev device create, I specifically didn't used
dev->groups because we callback in vendor driver before that, see below
code snippet, and those attributes should only be added if create()
callback returns success.

        ret = parent->ops->create(mdev, mdev_params);
        if (ret)
                return ret;

        ret = mdev_add_attribute_group(&mdev->dev,
                                        parent->ops->mdev_attr_groups);
        if (ret)
                parent->ops->destroy(mdev);



>> +
>> +static struct parent_device *mdev_get_parent_from_dev(struct device *dev)
>> +{
>> +	struct parent_device *parent;
>> +
>> +	mutex_lock(&parent_list_lock);
>> +	parent = mdev_get_parent(__find_parent_device(dev));
>> +	mutex_unlock(&parent_list_lock);
>> +
>> +	return parent;
>> +}
> 
> As we have demonstrated, all these refs and locks and release workqueue are not necessary,
> as long as you have an independent device associated with the mdev host device
> ("parent" device here).
>

I don't think every lock will go away with that. This also changes how
mdev devices entries are created in sysfs. It adds an extra directory.


> PS, "parent" is somehow a name too generic?
>

This is the term used in Linux Kernel for such cases. See 'struct
device' in include/linux/device.h. I would prefer 'parent'.

>> +
>> +static int mdev_device_create_ops(struct mdev_device *mdev, char *mdev_params)
>> +{
>> +	struct parent_device *parent = mdev->parent;
>> +	int ret;
>> +
>> +	ret = parent->ops->create(mdev, mdev_params);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = mdev_add_attribute_group(&mdev->dev,
>> +					parent->ops->mdev_attr_groups);
> 
> Ditto: dev->groups.
> 

See my above response for why this is indented to be so.


>> +	ret = parent_create_sysfs_files(dev);
>> +	if (ret)
>> +		goto add_sysfs_error;
>> +
>> +	ret = mdev_add_attribute_group(dev, ops->dev_attr_groups);
> 
> parent_create_sysfs_files and mdev_add_attribute_group are kind of doing
> the same thing, do you mind to merge them into one?
> 

Ok. I'll see I can do that.


>> +int mdev_device_get_online_status(struct device *dev, bool *online)
>> +{
>> +	int ret = 0;
>> +	struct mdev_device *mdev;
>> +	struct parent_device *parent;
>> +
>> +	mdev = mdev_get_device(to_mdev_device(dev));
>> +	if (!mdev)
>> +		return -EINVAL;
>> +
>> +	parent = mdev->parent;
>> +
>> +	if (parent->ops->get_online_status)
>> +		ret = parent->ops->get_online_status(mdev, online);
>> +
>> +	mdev_put_device(mdev);
>> +
>> +	return ret;
>> +}
> 
> The driver core has a perfect 'online' file for a device, with both
> 'show' and 'store' support, you don't need to write another one.
> 
> Please have a look at online_show and online_store in drivers/base/core.c.
> 

This is going to be removed as per the latest discussion.


> +
>> +extern struct class_attribute mdev_class_attrs[];
> 
> This is useless?
>

Oh, I missed to remove it. Thanks for pointing that out.


>> +static ssize_t mdev_create_store(struct device *dev,
>> +				 struct device_attribute *attr,
>> +				 const char *buf, size_t count)
>> +{
>> +	char *str, *pstr;
>> +	char *uuid_str, *mdev_params = NULL, *params = NULL;
>> +	uuid_le uuid;
>> +	int ret;
>> +
>> +	pstr = str = kstrndup(buf, count, GFP_KERNEL);
> 
> pstr is not used.
>

It is used below to free duped memory. If you see below code, 'str'
pointer gets incremented on strsep, so that can't be used to free
memory. pstr points to start of memory which we want to free while
returning from this function.

>> +
>> +	if (!str)
>> +		return -ENOMEM;
>> +
>> +	uuid_str = strsep(&str, ":");
>> +	if (!uuid_str) {
>> +		pr_err("mdev_create: Empty UUID string %s\n", buf);
>> +		ret = -EINVAL;
>> +		goto create_error;
>> +	}
>> +
>> +	if (str)
>> +		params = mdev_params = kstrdup(str, GFP_KERNEL);
>> +
>> +	ret = uuid_le_to_bin(uuid_str, &uuid);
>> +	if (ret) {
>> +		pr_err("mdev_create: UUID parse error %s\n", buf);
>> +		goto create_error;
>> +	}
>> +
>> +	ret = mdev_device_create(dev, uuid, mdev_params);
>> +	if (ret)
>> +		pr_err("mdev_create: Failed to create mdev device\n");
>> +	else
>> +		ret = count;
>> +
>> +create_error:
>> +	kfree(params);
>> +	kfree(pstr);
>> +	return ret;
>> +}
>> +
>> +static ssize_t mdev_destroy_store(struct device *dev,
>> +				  struct device_attribute *attr,
>> +				  const char *buf, size_t count)
>> +{
>> +	char *uuid_str, *str, *pstr;
>> +	uuid_le uuid;
>> +	int ret;
>> +
>> +	str = pstr = kstrndup(buf, count, GFP_KERNEL);
> 
> Ditto.
> 

Same as above.

>> +
>> +	if (!str)
>> +		return -ENOMEM;
>> +
>> +	uuid_str = strsep(&str, ":");
>> +	if (!uuid_str) {
>> +		pr_err("mdev_destroy: Empty UUID string %s\n", buf);
>> +		ret = -EINVAL;
>> +		goto destroy_error;
>> +	}
>> +
>> +	ret = uuid_le_to_bin(uuid_str, &uuid);
>> +	if (ret) {
>> +		pr_err("mdev_destroy: UUID parse error  %s\n", buf);
>> +		goto destroy_error;
>> +	}
>> +
>> +	ret = mdev_device_destroy(dev, uuid);
>> +	if (ret == 0)
>> +		ret = count;
>> +
>> +destroy_error:
>> +	kfree(pstr);
>> +	return ret;
>> +}
>> +
>> +
>> +int parent_create_sysfs_files(struct device *dev)
>> +{
>> +	int ret;
>> +
>> +	ret = sysfs_create_file(&dev->kobj,
>> +				&dev_attr_mdev_supported_types.attr);
>> +	if (ret) {
>> +		pr_err("Failed to create mdev_supported_types sysfs entry\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = sysfs_create_file(&dev->kobj, &dev_attr_mdev_create.attr);
>> +	if (ret) {
>> +		pr_err("Failed to create mdev_create sysfs entry\n");
>> +		goto create_sysfs_failed;
>> +	}
>> +
>> +	ret = sysfs_create_file(&dev->kobj, &dev_attr_mdev_destroy.attr);
>> +	if (ret) {
>> +		pr_err("Failed to create mdev_destroy sysfs entry\n");
>> +		sysfs_remove_file(&dev->kobj, &dev_attr_mdev_create.attr);
>> +	} else
>> +		return ret;
>> +
>> +create_sysfs_failed:
>> +	sysfs_remove_file(&dev->kobj, &dev_attr_mdev_supported_types.attr);
>> +	return ret;
>> +}
>> +
>> +void parent_remove_sysfs_files(struct device *dev)
>> +{
>> +	sysfs_remove_file(&dev->kobj, &dev_attr_mdev_supported_types.attr);
>> +	sysfs_remove_file(&dev->kobj, &dev_attr_mdev_create.attr);
>> +	sysfs_remove_file(&dev->kobj, &dev_attr_mdev_destroy.attr);
>> +}
> 
> The 2 functions above are also unnecessary: you can always group it with a single
> function call of sysfs_create_files.
>

Ok. 'supported types' and 'create' are going to change as per discussion
going on for libvirt integration. These functions would get removed with
that change.


>> +
>> +static inline struct mdev_driver *to_mdev_driver(struct device_driver *drv)
>> +{
>> +	return drv ? container_of(drv, struct mdev_driver, driver) : NULL;
>> +}
>> +
>> +static inline struct mdev_device *to_mdev_device(struct device *dev)
>> +{
>> +	return dev ? container_of(dev, struct mdev_device, dev) : NULL;
>> +}
> 
> These can be macros, like pci ones.
>

These also checks for NULL of argument which macro doesn't.

Kirti.

>> +
>> +static inline void *mdev_get_drvdata(struct mdev_device *mdev)
>> +{
>> +	return mdev->driver_data;
>> +}
>> +
>> +static inline void mdev_set_drvdata(struct mdev_device *mdev, void *data)
>> +{
>> +	mdev->driver_data = data;
>> +}
>> +
>> +extern struct bus_type mdev_bus_type;
>> +
>> +#define dev_is_mdev(d) ((d)->bus == &mdev_bus_type)
>> +
>> +extern int  mdev_register_device(struct device *dev,
>> +				 const struct parent_ops *ops);
>> +extern void mdev_unregister_device(struct device *dev);
>> +
>> +extern int  mdev_register_driver(struct mdev_driver *drv, struct module *owner);
>> +extern void mdev_unregister_driver(struct mdev_driver *drv);
>> +
>> +extern struct mdev_device *mdev_get_device(struct mdev_device *mdev);
>> +extern void mdev_put_device(struct mdev_device *mdev);
>> +
>> +extern struct mdev_device *mdev_get_device_by_group(struct iommu_group *group);
>> +
>> +#endif /* MDEV_H */
>>
> 
> --
> Thanks,
> Jike
>