On 11/15/2016 2:00 PM, Dong Jia Shi wrote:
> * Kirti Wankhede <kwankhede@xxxxxxxxxx> [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@xxxxxxxxxx>
>> + * Kirti Wankhede <kwankhede@xxxxxxxxxx>
>> + *
>> + * 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 ...
>
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 <cjia@xxxxxxxxxx>
>> + * Kirti Wankhede <kwankhede@xxxxxxxxxx>
>> + *
>> + * 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;
>
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 <bjsdjshi@xxxxxxxxxxxxxxxxxx>
Thanks,
Kirti
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
