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

[Kirti Wankhede]

On 11/15/2016 2:00 PM, Dong Jia Shi wrote:
> * Kirti Wankhede <address@hidden> [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 <address@hidden>
>> + *             Kirti Wankhede <address@hidden>
>> + *
>> + * 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 <address@hidden>
>> + *             Kirti Wankhede <address@hidden>
>> + *
>> + * 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 <address@hidden>

Thanks,
Kirti