* Kirti Wankhede <kwankh...@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 <c...@nvidia.com> > + * Kirti Wankhede <kwankh...@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 <c...@nvidia.com> > + * Kirti Wankhede <kwankh...@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(<ype->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 <c...@nvidia.com> > + * Kirti Wankhede <kwankh...@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 <bjsdj...@linux.vnet.ibm.com> -- Dong Jia