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

[Yuval Shaia]

On Mon, Apr 15, 2019 at 06:07:52PM -0700, Bart Van Assche wrote:
> 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?

The device should eventually expose Ethernet interface as well as IB.

> 
> > +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()?

Agree, not sure really needed, just saw that some drivers uses this pattern.

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

All prints will be removed, this is still wip.

> 
> > +#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?

I've noticed that other drivers exposes this in sysfs.

> 
> > +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?

Right, a mistake.

> 
> Thanks,
> 
> Bart.

Thanks.

>