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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization



[Index of Archives]     [KVM Development]     [Libvirt Development]     [Libvirt Users]     [CentOS Virtualization]     [Netdev]     [Ethernet Bridging]     [Linux Wireless]     [Kernel Newbies]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]

  Powered by Linux