From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36357) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c6Z8v-0007il-Nb for qemu-devel@nongnu.org; Tue, 15 Nov 2016 03:31:03 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c6Z8s-0002Hx-Hx for qemu-devel@nongnu.org; Tue, 15 Nov 2016 03:31:01 -0500 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:33721 helo=mx0a-001b2d01.pphosted.com) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1c6Z8s-0002Hf-C4 for qemu-devel@nongnu.org; Tue, 15 Nov 2016 03:30:58 -0500 Received: from pps.filterd (m0098413.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.0.17/8.16.0.17) with SMTP id uAF8TJls134897 for ; Tue, 15 Nov 2016 03:30:57 -0500 Received: from e36.co.us.ibm.com (e36.co.us.ibm.com [32.97.110.154]) by mx0b-001b2d01.pphosted.com with ESMTP id 26qu8yh6a4-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Tue, 15 Nov 2016 03:30:56 -0500 Received: from localhost by e36.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 15 Nov 2016 01:30:55 -0700 Date: Tue, 15 Nov 2016 16:30:48 +0800 From: Dong Jia Shi References: <1479138156-28905-1-git-send-email-kwankhede@nvidia.com> <1479138156-28905-2-git-send-email-kwankhede@nvidia.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1479138156-28905-2-git-send-email-kwankhede@nvidia.com> Message-Id: <20161115083048.GA17048@bjsdjshi@linux.vnet.ibm.com> Subject: Re: [Qemu-devel] [PATCH v12 01/22] vfio: Mediated device Core driver List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kirti Wankhede Cc: alex.williamson@redhat.com, pbonzini@redhat.com, kraxel@redhat.com, cjia@nvidia.com, qemu-devel@nongnu.org, kvm@vger.kernel.org, kevin.tian@intel.com, jike.song@intel.com, bjsdjshi@linux.vnet.ibm.com, linux-kernel@vger.kernel.org * Kirti Wankhede [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 > + * Kirti Wankhede > + * > + * 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 > +#include > +#include > +#include > +#include > +#include > + > +#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 > + * Kirti Wankhede > + * > + * 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 > +#include > +#include > + > +#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 > + * Kirti Wankhede > + * > + * 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 -- Dong Jia