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

[Dong Jia Shi]

* 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 ...

> +{
> +     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 <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;

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(&ltype->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 <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.
> + */
> +
> +#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 <address@hidden>

-- 
Dong Jia