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

[Yuval Shaia <yuval.shaia@oracle.com>]

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.

>