Re: [Qemu-devel] [RFC 3/3] RDMA/virtio-rdma: VirtIO rdma driver

[Bart Van Assche]

On 4/11/19 4:01 AM, Yuval Shaia wrote:
> +++ b/drivers/infiniband/hw/virtio/Kconfig
> @@ -0,0 +1,6 @@
> +config INFINIBAND_VIRTIO_RDMA
> +     tristate "VirtIO Paravirtualized RDMA Driver"
> +     depends on NETDEVICES && ETHERNET && PCI && INET
> +     ---help---
> +       This driver provides low-level support for VirtIO Paravirtual
> +       RDMA adapter.

Does this driver really depend on Ethernet, or does it also work with
Ethernet support disabled?

> +static inline struct virtio_rdma_info *to_vdev(struct ib_device *ibdev)
> +{
> +     return container_of(ibdev, struct virtio_rdma_info, ib_dev);
> +}

Is it really worth to introduce this function? Have you considered to
use container_of(ibdev, struct virtio_rdma_info, ib_dev) directly instead
of to_vdev()?

> +static void rdma_ctrl_ack(struct virtqueue *vq)
> +{
> +     struct virtio_rdma_info *dev = vq->vdev->priv;
> +
> +     wake_up(&dev->acked);
> +
> +     printk("%s\n", __func__);
> +}

Should that printk() be changed into pr_debug()? The same comment holds for
all other printk() calls.

> +#define VIRTIO_RDMA_BOARD_ID 1
> +#define VIRTIO_RDMA_HW_NAME  "virtio-rdma"
> +#define VIRTIO_RDMA_HW_REV   1
> +#define VIRTIO_RDMA_DRIVER_VER       "1.0"

Is a driver version number useful in an upstream driver?

> +struct ib_cq *virtio_rdma_create_cq(struct ib_device *ibdev,
> +                                 const struct ib_cq_init_attr *attr,
> +                                 struct ib_ucontext *context,
> +                                 struct ib_udata *udata)
> +{
> +     struct scatterlist in, out;
> +     struct virtio_rdma_ib_cq *vcq;
> +     struct cmd_create_cq *cmd;
> +     struct rsp_create_cq *rsp;
> +     struct ib_cq *cq = NULL;
> +     int rc;
> +
> +     /* TODO: Check MAX_CQ */
> +
> +     cmd = kmalloc(sizeof(*cmd), GFP_ATOMIC);
> +     if (!cmd)
> +             return ERR_PTR(-ENOMEM);
> +
> +     rsp = kmalloc(sizeof(*rsp), GFP_ATOMIC);
> +     if (!rsp) {
> +             kfree(cmd);
> +             return ERR_PTR(-ENOMEM);
> +     }
> +
> +     vcq = kzalloc(sizeof(*vcq), GFP_KERNEL);
> +     if (!vcq)
> +             goto out;

Are you sure that you want to mix GFP_ATOMIC and GFP_KERNEL in a single
function?

Thanks,

Bart.