Re: [PATCH v12 01/22] vfio: Mediated device Core driver

[Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>]

* Kirti Wankhede <kwankhede@nvidia.com> [2016-11-14 21:12:15 +0530]:

Hi Kirti,

[...]

> diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
> new file mode 100644
> index 000000000000..613e8a8a3b2a
> --- /dev/null
> +++ b/drivers/vfio/mdev/mdev_core.c
> @@ -0,0 +1,374 @@
> +/*
> + * Mediated device Core Driver
> + *
> + * Copyright (c) 2016, NVIDIA CORPORATION. All rights reserved.
> + *     Author: Neo Jia <cjia@nvidia.com>
> + *             Kirti Wankhede <kwankhede@nvidia.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/slab.h>
> +#include <linux/uuid.h>
> +#include <linux/sysfs.h>
> +#include <linux/mdev.h>
> +
> +#include "mdev_private.h"
> +
> +#define DRIVER_VERSION		"0.1"
> +#define DRIVER_AUTHOR		"NVIDIA Corporation"
> +#define DRIVER_DESC		"Mediated device Core Driver"
> +
> +static LIST_HEAD(parent_list);
> +static DEFINE_MUTEX(parent_list_lock);
> +static struct class_compat *mdev_bus_compat_class;
> +
> +static int _find_mdev_device(struct device *dev, void *data)
What the underscore prefix implies to me is that this should not be
called directly. While ...

> +{
> +	struct mdev_device *mdev;
> +
> +	if (!dev_is_mdev(dev))
> +		return 0;
> +
> +	mdev = to_mdev_device(dev);
> +
> +	if (uuid_le_cmp(mdev->uuid, *(uuid_le *)data) == 0)
> +		return 1;
> +
> +	return 0;
> +}
[...]

> +
> +static int mdev_device_remove_cb(struct device *dev, void *data)
> +{
> +	if (!dev_is_mdev(dev))
> +		return 0;
> +
> +	return mdev_device_remove(dev, data ? *(bool *)data : true);
*(bool *)data will always be true, correct?
If so, we chould get rid of it.

> +}
> +
[...]

> diff --git a/drivers/vfio/mdev/mdev_driver.c b/drivers/vfio/mdev/mdev_driver.c
> new file mode 100644
> index 000000000000..6c19a2f6b5a2
> --- /dev/null
> +++ b/drivers/vfio/mdev/mdev_driver.c
> @@ -0,0 +1,120 @@
> +/*
> + * MDEV driver
> + *
> + * Copyright (c) 2016, NVIDIA CORPORATION. All rights reserved.
> + *     Author: Neo Jia <cjia@nvidia.com>
> + *             Kirti Wankhede <kwankhede@nvidia.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/iommu.h>
> +#include <linux/mdev.h>
> +
> +#include "mdev_private.h"
> +
> +static int mdev_attach_iommu(struct mdev_device *mdev)
> +{
> +	int ret;
> +	struct iommu_group *group;
> +
> +	group = iommu_group_alloc();
> +	if (IS_ERR(group))
> +		return PTR_ERR(group);
> +
> +	ret = iommu_group_add_device(group, &mdev->dev);
> +	if (ret)
> +		goto attach_fail;
> +
> +	dev_info(&mdev->dev, "MDEV: group_id = %d\n", iommu_group_id(group));
> +attach_fail:
> +	iommu_group_put(group);
> +	return ret;
> +}
No need for a goto here. How about:
	ret = iommu_group_add_device(group, &mdev->dev);
	if (!ret)
		dev_info(&mdev->dev, "MDEV: group_id = %d\n",
			 iommu_group_id(group));
	iommu_group_put(group);
	return ret;

Or just remove the dev_info stuff?

[...]

> diff --git a/drivers/vfio/mdev/mdev_sysfs.c b/drivers/vfio/mdev/mdev_sysfs.c
> new file mode 100644
> index 000000000000..04f9b2925e6d
> --- /dev/null
> +++ b/drivers/vfio/mdev/mdev_sysfs.c
[...]

> +
> +static int add_mdev_supported_type_groups(struct parent_device *parent)
> +{
> +	int i;
> +
> +	for (i = 0; parent->ops->supported_type_groups[i]; i++) {
> +		struct mdev_type *type;
> +
> +		type = add_mdev_supported_type(parent,
> +					parent->ops->supported_type_groups[i]);
> +		if (IS_ERR(type)) {
> +			struct mdev_type *ltype, *tmp;
> +
> +			list_for_each_entry_safe(ltype, tmp, &parent->type_list,
> +						  next) {
> +				list_del(&ltype->next);
> +				remove_mdev_supported_type(ltype);
> +			}
> +			return PTR_ERR(type);
> +		}
> +		list_add(&type->next, &parent->type_list);
> +	}
> +	return 0;
> +}
> +
> +/* mdev sysfs Functions */
s/F/f/

[...]

> diff --git a/include/linux/mdev.h b/include/linux/mdev.h
> new file mode 100644
> index 000000000000..4900cc472364
> --- /dev/null
> +++ b/include/linux/mdev.h
> @@ -0,0 +1,168 @@
> +/*
> + * Mediated device definition
> + *
> + * Copyright (c) 2016, NVIDIA CORPORATION. All rights reserved.
> + *     Author: Neo Jia <cjia@nvidia.com>
> + *             Kirti Wankhede <kwankhede@nvidia.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#ifndef MDEV_H
> +#define MDEV_H
> +
> +/* Parent Device */
This is really a nit pick:
s/Device/device/

> +struct parent_device {
> +	struct device		*dev;
> +	const struct parent_ops	*ops;
> +
> +	/* internal */
> +	struct kref		ref;
> +	struct mutex		lock;
> +	struct list_head	next;
> +	struct kset		*mdev_types_kset;
> +	struct list_head	type_list;
> +};
> +
> +/* Mediated device */
[...]

All findings from me are nitpickings. If you like you can have my r-b:
Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>

-- 
Dong Jia