Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S938597AbcKOPQl (ORCPT ); Tue, 15 Nov 2016 10:16:41 -0500 Received: from hqemgate16.nvidia.com ([216.228.121.65]:9699 "EHLO hqemgate16.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S935507AbcKOPQj (ORCPT ); Tue, 15 Nov 2016 10:16:39 -0500 X-PGP-Universal: processed; by hqnvupgp07.nvidia.com on Mon, 14 Nov 2016 19:15:40 -0800 Subject: Re: [PATCH v12 01/22] vfio: Mediated device Core driver To: , , , , , , , , , References: <1479138156-28905-1-git-send-email-kwankhede@nvidia.com> <1479138156-28905-2-git-send-email-kwankhede@nvidia.com> <20161115083048.GA17048@bjsdjshi@linux.vnet.ibm.com> X-Nvconfidentiality: public From: Kirti Wankhede Message-ID: Date: Tue, 15 Nov 2016 20:46:32 +0530 MIME-Version: 1.0 In-Reply-To: <20161115083048.GA17048@bjsdjshi@linux.vnet.ibm.com> X-Originating-IP: [10.24.216.210] X-ClientProxiedBy: BGMAIL103.nvidia.com (10.25.59.12) To bgmail102.nvidia.com (10.25.59.11) Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4571 Lines: 169 On 11/15/2016 2:00 PM, Dong Jia Shi wrote: > * 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 ... > This only called here, i.e. it is not called directly: dev = device_find_child(parent->dev, &uuid, _find_mdev_device); >> +{ >> + 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. > No, data can be true or false based in when it is called. This is passed to mdev_device_remove_ops() where I had added comment. /* * mdev_device_remove_ops gets called from sysfs's 'remove' and when parent * device is being unregistered from mdev device framework. * - 'force_remove' is set to 'false' when called from sysfs's 'remove' which * indicates that if the mdev device is active, used by VMM or userspace * application, vendor driver could return error then don't remove the device. * - 'force_remove' is set to 'true' when called from mdev_unregister_device() * which indicate that parent device is being removed from mdev device * framework so remove mdev device forcefully. */ static int mdev_device_remove_ops(struct mdev_device *mdev, bool force_remove) >> +} >> + > [...] > >> 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; > Ok. > Or just remove the dev_info stuff? > > [...] > > All findings from me are nitpickings. If you like you can have my r-b: > Reviewed-by: Dong Jia Shi Thanks, Kirti