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

[Kirti Wankhede]

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