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.
