lkml.org 
[lkml]   [2021]   [May]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v7 11/12] vduse: Introduce VDUSE - vDPA Device in Userspace
    On Mon, May 17, 2021 at 05:55:12PM +0800, Xie Yongji wrote:
    > +struct vduse_dev {
    > + struct vduse_vdpa *vdev;
    > + struct device dev;
    > + struct cdev cdev;

    You now have 2 reference counted devices controling the lifespace of a
    single structure. A mess that is guaranteed to go wrong. Please never
    do this.

    > + struct vduse_virtqueue *vqs;
    > + struct vduse_iova_domain *domain;
    > + char *name;
    > + struct mutex lock;
    > + spinlock_t msg_lock;
    > + atomic64_t msg_unique;

    Why do you need an atomic and a lock?

    > + wait_queue_head_t waitq;
    > + struct list_head send_list;
    > + struct list_head recv_list;
    > + struct list_head list;
    > + struct vdpa_callback config_cb;
    > + struct work_struct inject;
    > + spinlock_t irq_lock;
    > + unsigned long api_version;
    > + bool connected;
    > + int minor;
    > + u16 vq_size_max;
    > + u32 vq_num;
    > + u32 vq_align;
    > + u32 config_size;
    > + u32 device_id;
    > + u32 vendor_id;
    > +};
    > +
    > +struct vduse_dev_msg {
    > + struct vduse_dev_request req;
    > + struct vduse_dev_response resp;
    > + struct list_head list;
    > + wait_queue_head_t waitq;
    > + bool completed;
    > +};
    > +
    > +struct vduse_control {
    > + unsigned long api_version;

    u64?

    > +};
    > +
    > +static unsigned long max_bounce_size = (64 * 1024 * 1024);
    > +module_param(max_bounce_size, ulong, 0444);
    > +MODULE_PARM_DESC(max_bounce_size, "Maximum bounce buffer size. (default: 64M)");
    > +
    > +static unsigned long max_iova_size = (128 * 1024 * 1024);
    > +module_param(max_iova_size, ulong, 0444);
    > +MODULE_PARM_DESC(max_iova_size, "Maximum iova space size (default: 128M)");
    > +
    > +static bool allow_unsafe_device_emulation;
    > +module_param(allow_unsafe_device_emulation, bool, 0444);
    > +MODULE_PARM_DESC(allow_unsafe_device_emulation, "Allow emulating unsafe device."
    > + " We must make sure the userspace device emulation process is trusted."
    > + " Otherwise, don't enable this option. (default: false)");
    > +

    This is not the 1990's anymore, please never use module parameters, make
    these per-device attributes if you really need them.

    > +static int vduse_init(void)
    > +{
    > + int ret;
    > +
    > + if (max_bounce_size >= max_iova_size)
    > + return -EINVAL;
    > +
    > + ret = misc_register(&vduse_misc);
    > + if (ret)
    > + return ret;
    > +
    > + vduse_class = class_create(THIS_MODULE, "vduse");

    If you have a misc device, you do not need to create a class at the same
    time. Why are you doing both here? Just stick with the misc device, no
    need for anything else.

    > + if (IS_ERR(vduse_class)) {
    > + ret = PTR_ERR(vduse_class);
    > + goto err_class;
    > + }
    > + vduse_class->devnode = vduse_devnode;
    > +
    > + ret = alloc_chrdev_region(&vduse_major, 0, VDUSE_DEV_MAX, "vduse");

    Wait, you want a whole major? What is the misc device for?

    > +MODULE_VERSION(DRV_VERSION);

    MODULE_VERSION() makes no sense when the code is merged into the kernel
    tree, so you can just drop that.

    thanks,

    greg k-h

    \
     
     \ /
      Last update: 2021-05-31 06:57    [W:3.552 / U:1.248 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site