[PATCH vringh 0/2] Introduce CAIF Virtio driver

[PATCH vringh 0/2] Introduce CAIF Virtio driver

[sjur.brandeland]

From: Sjur Brændeland <sjur.brandeland@stericsson.com>

This patch-set introduces the CAIF Virtio Link layer driver.

This driver depends on Rusty's new host virtio ring implementation,
so this patch-set is based on the vringh branch in Rusty's git.

Regards,
Sjur

cc: Rusty Russell <rusty@rustcorp.com.au>
cc: Ohad Ben-Cohen <ohad@wizery.com>
cc: David S. Miller <davem@davemloft.net>
cc: Ido Yariv <ido@wizery.com>
cc: Erwan Yvin <erwan.yvin@stericsson.com>

Sjur Brændeland (1):
  remoteproc: Add support for vringh (Host vrings)

Vikram ARV (1):
  caif_virtio: Introduce caif over virtio

 drivers/net/caif/Kconfig               |    8 +
 drivers/net/caif/Makefile              |    3 +
 drivers/net/caif/caif_virtio.c         |  568 ++++++++++++++++++++++++++++++++
 drivers/remoteproc/Kconfig             |    3 +
 drivers/remoteproc/remoteproc_virtio.c |  127 +++++++-
 include/linux/remoteproc.h             |   14 +
 include/linux/virtio_caif.h            |   24 ++
 include/uapi/linux/virtio_ids.h        |    1 +
 8 files changed, 741 insertions(+), 7 deletions(-)
 create mode 100644 drivers/net/caif/caif_virtio.c
 create mode 100644 include/linux/virtio_caif.h

-- 
1.7.5.4

[PATCH vringh 1/2] remoteproc: Add support for vringh (Host vrings)

[sjur.brandeland]

From: Sjur Brændeland <sjur.brandeland@stericsson.com>

Add functions for creating, deleting and kicking host-side virtio rings.

The host ring is not integrated with virtiqueues and cannot be managed
through virtio-config. Remoteproc must export functions for handling the
host-side virtio rings.

The functions rproc_virtio_get_vringh(), rproc_virtio_del_vringh(),
rproc_virtio_kick_vringh() are added to remoteproc_virtio.c. The
existing functions rproc_vq_interrupt() and rproc_virtio_find_vqs()
are updated to handle the new vhost rings.

Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
cc: Ohad Ben-Cohen <ohad@wizery.com>
cc: Rusty Russell <rusty@rustcorp.com.au>
cc: Ido Yariv <ido@wizery.com>
cc: Erwan Yvin <erwan.yvin@stericsson.com>

---
Hi Ohad,

I would really appreciate if you could find time to
review this patch. It will go via Rusty's vringh tree.

Feedback and review comments are welcomed.

Thanks,
Sjur


 drivers/remoteproc/Kconfig             |    3 +
 drivers/remoteproc/remoteproc_virtio.c |  127 ++++++++++++++++++++++++++++++--
 include/linux/remoteproc.h             |   14 ++++
 3 files changed, 137 insertions(+), 7 deletions(-)

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index 96ce101..c7d1d36 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -7,6 +7,9 @@ config REMOTEPROC
 	depends on HAS_DMA
 	select FW_CONFIG
 	select VIRTIO
+	select VHOST_RING
+
+source drivers/vhost/Kconfig
 
 config OMAP_REMOTEPROC
 	tristate "OMAP remoteproc support"
diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
index 9e198e5..fa7bf7b 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -23,6 +23,7 @@
 #include <linux/virtio_config.h>
 #include <linux/virtio_ids.h>
 #include <linux/virtio_ring.h>
+#include <linux/vringh.h>
 #include <linux/err.h>
 #include <linux/kref.h>
 #include <linux/slab.h>
@@ -60,10 +61,15 @@ irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int notifyid)
 	dev_dbg(&rproc->dev, "vq index %d is interrupted\n", notifyid);
 
 	rvring = idr_find(&rproc->notifyids, notifyid);
-	if (!rvring || !rvring->vq)
+	if (!rvring)
 		return IRQ_NONE;
 
-	return vring_interrupt(0, rvring->vq);
+	if (rvring->vringh && rvring->vringh_cb)
+		return rvring->vringh_cb(&rvring->rvdev->vdev, rvring->vringh);
+	else if (rvring->vq)
+		return vring_interrupt(0, rvring->vq);
+	else
+		return IRQ_NONE;
 }
 EXPORT_SYMBOL(rproc_vq_interrupt);
 
@@ -149,14 +155,21 @@ static int rproc_virtio_find_vqs(struct virtio_device *vdev, unsigned nvqs,
 		       const char *names[])
 {
 	struct rproc *rproc = vdev_to_rproc(vdev);
-	int i, ret;
+	struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
+	int rng, id, ret, nrings = ARRAY_SIZE(rvdev->vring);
+
+	for (id = 0, rng = 0; rng < nrings; ++rng) {
+		struct rproc_vring *rvring = &rvdev->vring[rng];
+		/* Skip the host side rings */
+		if (rvring->vringh)
+			continue;
 
-	for (i = 0; i < nvqs; ++i) {
-		vqs[i] = rp_find_vq(vdev, i, callbacks[i], names[i]);
-		if (IS_ERR(vqs[i])) {
-			ret = PTR_ERR(vqs[i]);
+		vqs[id] = rp_find_vq(vdev, rng, callbacks[id], names[id]);
+		if (IS_ERR(vqs[id])) {
+			ret = PTR_ERR(vqs[id]);
 			goto error;
 		}
+		++id;
 	}
 
 	/* now that the vqs are all set, boot the remote processor */
@@ -173,6 +186,106 @@ error:
 	return ret;
 }
 
+/**
+ * rproc_virtio_new_vringh() - create a reversed virtio ring.
+ * @vdev: the virtio device
+ * @index: the virtio ring index
+ * @cb: callback for the reversed virtio ring
+ *
+ * This function should be called by the virtio-driver
+ * before calling find_vqs(). It returns a struct vringh for
+ * accessing the virtio ring.
+ *
+ * Return: struct vhost, or NULL upon error.
+ */
+struct vringh *
+rproc_virtio_new_vringh(struct virtio_device *vdev, unsigned index,
+			irqreturn_t (*cb)(struct virtio_device *vdev,
+					  struct vringh *vring))
+{
+	struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
+	struct rproc_vring *rvring;
+	struct vringh *vrh;
+	int err;
+
+	if (index > ARRAY_SIZE(rvdev->vring)) {
+		dev_err(&rvdev->vdev.dev, "bad vring index: %d\n", index);
+		return NULL;
+	}
+
+	vrh = kzalloc(sizeof(*vrh), GFP_KERNEL);
+	if (!vrh)
+		return NULL;
+
+	err = rproc_alloc_vring(rvdev, index);
+	if (err)
+		goto free_vring;
+
+
+	rvring = &rvdev->vring[index];
+	/* zero vring */
+	memset(rvring->va, 0, vring_size(rvring->len, rvring->align));
+	vring_init(&vrh->vring, rvring->len, rvring->va, rvring->align);
+
+	rvring->vringh_cb = cb;
+	rvring->vringh = vrh;
+
+	err = vringh_init_kern(vrh,
+			       rvdev->dfeatures,
+			       rvring->len,
+			       false,
+			       vrh->vring.desc,
+			       vrh->vring.avail,
+			       vrh->vring.used);
+	if (!err)
+		return vrh;
+
+	dev_err(&vdev->dev, "failed to create vhost: %d\n", err);
+	rproc_free_vring(rvring);
+free_vring:
+	kfree(vrh);
+	return NULL;
+}
+EXPORT_SYMBOL(rproc_virtio_new_vringh);
+
+/**
+ * rproc_virtio_del_vringh() - release a reversed virtio ring.
+ * @vdev: the virtio device
+ * @index: the virtio ring index
+ *
+ * This function release the reversed virtio ring.
+ */
+void rproc_virtio_del_vringh(struct virtio_device *vdev, unsigned index)
+{
+	struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
+	struct rproc_vring *rvring = &rvdev->vring[index];
+	kfree(rvring->vringh);
+	rproc_free_vring(rvring);
+	rvring->vringh_cb = NULL;
+	rvring->vringh = NULL;
+}
+EXPORT_SYMBOL(rproc_virtio_del_vringh);
+
+/**
+ * rproc_virtio_kick_vringh() - kick the remote processor.
+ * @vdev: the virtio device
+ * @index: the virtio ring index
+ *
+ * kick the remote processor, and let it know which vring to poke at
+ */
+void rproc_virtio_kick_vringh(struct virtio_device *vdev, unsigned index)
+{
+	struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
+	struct rproc_vring *rvring = &rvdev->vring[index];
+	struct rproc *rproc = rvring->rvdev->rproc;
+	int notifyid = rvring->notifyid;
+
+	dev_dbg(&rproc->dev, "kicking vq index: %d\n", notifyid);
+
+	rproc->ops->kick(rproc, notifyid);
+}
+EXPORT_SYMBOL(rproc_virtio_kick_vringh);
+
 /*
  * We don't support yet real virtio status semantics.
  *
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index faf3332..414a1fd 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -39,7 +39,9 @@
 #include <linux/klist.h>
 #include <linux/mutex.h>
 #include <linux/virtio.h>
+#include <linux/vringh.h>
 #include <linux/completion.h>
+#include <linux/interrupt.h>
 #include <linux/idr.h>
 
 /**
@@ -444,6 +446,8 @@ struct rproc {
  * @notifyid: rproc-specific unique vring index
  * @rvdev: remote vdev
  * @vq: the virtqueue of this vring
+ * @vringh_cb: callback used when device has kicked
+ * @vringh: the reversed host-side vring
  */
 struct rproc_vring {
 	void *va;
@@ -454,6 +458,9 @@ struct rproc_vring {
 	int notifyid;
 	struct rproc_vdev *rvdev;
 	struct virtqueue *vq;
+	irqreturn_t (*vringh_cb)(struct virtio_device *vdev,
+				 struct vringh *vring);
+	struct vringh *vringh;
 };
 
 /**
@@ -485,6 +492,13 @@ int rproc_boot(struct rproc *rproc);
 void rproc_shutdown(struct rproc *rproc);
 void rproc_report_crash(struct rproc *rproc, enum rproc_crash_type type);
 
+struct vringh *
+rproc_virtio_new_vringh(struct virtio_device *vdev, unsigned index,
+			irqreturn_t (*cb)(struct virtio_device *vdev,
+					  struct vringh *vring));
+void rproc_virtio_del_vringh(struct virtio_device *vdev, unsigned index);
+void rproc_virtio_kick_vringh(struct virtio_device *vdev, unsigned index);
+
 static inline struct rproc_vdev *vdev_to_rvdev(struct virtio_device *vdev)
 {
 	return container_of(vdev, struct rproc_vdev, vdev);
-- 
1.7.5.4

[PATCH vringh 2/2] caif_virtio: Introduce caif over virtio

[sjur.brandeland]

From: Vikram ARV <vikram.arv@stericsson.com>

Add the the Virtio shared memory driver for STE Modems.
caif_virtio is implemented utilizing the virtio framework
for data transport and is managed with the remoteproc frameworks.

The Virtio queue is used for transmitting data to the modem, and
the new vringh implementation is receiving data over the vring.

Signed-off-by: Vikram ARV <vikram.arv@stericsson.com>
Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>

to: David S. Miller <davem@davemloft.net>
cc: Rusty Russell <rusty@rustcorp.com.au>
cc: Ohad Ben-Cohen <ohad@wizery.com>
cc: Ido Yariv <ido@wizery.com>
cc: Erwan Yvin <erwan.yvin@stericsson.com>
---
Hi Dave,

Rusty has accepted to take this patch via his tree.
Feedback and review comments are appreciated.

Thanks,
Sjur

 drivers/net/caif/Kconfig        |    8 +
 drivers/net/caif/Makefile       |    3 +
 drivers/net/caif/caif_virtio.c  |  568 +++++++++++++++++++++++++++++++++++++++
 include/linux/virtio_caif.h     |   24 ++
 include/uapi/linux/virtio_ids.h |    1 +
 5 files changed, 604 insertions(+), 0 deletions(-)
 create mode 100644 drivers/net/caif/caif_virtio.c
 create mode 100644 include/linux/virtio_caif.h

diff --git a/drivers/net/caif/Kconfig b/drivers/net/caif/Kconfig
index abf4d7a..a8b67e9 100644
--- a/drivers/net/caif/Kconfig
+++ b/drivers/net/caif/Kconfig
@@ -47,3 +47,11 @@ config CAIF_HSI
        The caif low level driver for CAIF over HSI.
        Be aware that if you enable this then you also need to
        enable a low-level HSI driver.
+
+config CAIF_VIRTIO
+	tristate "CAIF virtio transport driver"
+	depends on CAIF
+	depends on REMOTEPROC
+	default n
+	---help---
+	The caif driver for CAIF over Virtio.
diff --git a/drivers/net/caif/Makefile b/drivers/net/caif/Makefile
index 91dff86..d9ee26a 100644
--- a/drivers/net/caif/Makefile
+++ b/drivers/net/caif/Makefile
@@ -13,3 +13,6 @@ obj-$(CONFIG_CAIF_SHM) += caif_shm.o
 
 # HSI interface
 obj-$(CONFIG_CAIF_HSI) += caif_hsi.o
+
+# Virtio interface
+obj-$(CONFIG_CAIF_VIRTIO) += caif_virtio.o
diff --git a/drivers/net/caif/caif_virtio.c b/drivers/net/caif/caif_virtio.c
new file mode 100644
index 0000000..e8ea114
--- /dev/null
+++ b/drivers/net/caif/caif_virtio.c
@@ -0,0 +1,568 @@
+/*
+ * Copyright (C) ST-Ericsson AB 2012
+ * Contact: Sjur Brendeland / sjur.brandeland@stericsson.com
+ * Authors: Vicram Arv / vikram.arv@stericsson.com,
+ *	    Dmitry Tarnyagin / dmitry.tarnyagin@stericsson.com
+ *	    Sjur Brendeland / sjur.brandeland@stericsson.com
+ * License terms: GNU General Public License (GPL) version 2
+ */
+
+#include <linux/module.h>
+#include <linux/virtio.h>
+#include <linux/virtio_ids.h>
+#include <linux/virtio_config.h>
+#include <linux/dma-mapping.h>
+#include <linux/netdevice.h>
+#include <linux/if_arp.h>
+#include <linux/spinlock.h>
+#include <linux/virtio_caif.h>
+#include <linux/virtio_ring.h>
+#include <linux/vringh.h>
+#include <linux/remoteproc.h>
+#include <net/caif/caif_dev.h>
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Vicram Arv <vikram.arv@stericsson.com>");
+MODULE_AUTHOR("Sjur Brendeland <sjur.brandeland@stericsson.com");
+MODULE_DESCRIPTION("Virtio CAIF Driver");
+
+/* Virtio Ring used in receive direction */
+#define RX_RING_INDEX 0
+
+#define CFV_DEFAULT_QUOTA 32
+
+/* struct cfv_napi_contxt - NAPI context info
+ * @riov: IOV holding data read from the ring. Note that riov may
+ *	  still hold data when cfv_rx_poll() returns.
+ * @head: Last descriptor ID we received from vringh_getdesc_kern.
+ *	  We use this to put descriptor back on the used ring. USHRT_MAX is
+ *	  used to indicate invalid head-id.
+ */
+struct cfv_napi_context {
+	struct vringh_kiov riov;
+	unsigned short head;
+};
+
+/* struct cfv_info - Caif Virtio control structure
+ * @cfdev:	caif common header
+ * @vdev:	Associated virtio device
+ * @vq_rx:	rx/downlink virtqueue
+ * @vq_tx:	tx/uplink virtqueue
+ * @ndev:	associated netdevice
+ * @queued_tx:	number of buffers queued in the tx virtqueue
+ * @watermark_tx: indicates number of buffers the tx queue
+ *		should shrink to to unblock datapath
+ * @tx_lock:	protects vq_tx to allow concurrent senders
+ * @tx_hr:	transmit headroom
+ * @rx_hr:	receive headroom
+ * @tx_tr:	transmit tailroom
+ * @rx_tr:	receive tailroom
+ * @mtu:	transmit max size
+ * @mru:	receive max size
+ */
+struct cfv_info {
+	struct caif_dev_common cfdev;
+	struct virtio_device *vdev;
+	struct vringh *vr_rx;
+	struct virtqueue *vq_tx;
+	struct net_device *ndev;
+	unsigned int queued_tx;
+	unsigned int watermark_tx;
+	/* Protect access to vq_tx */
+	spinlock_t tx_lock;
+	struct tasklet_struct tx_release_tasklet;
+	struct napi_struct napi;
+	struct cfv_napi_context ctx;
+	/* Copied from Virtio config space */
+	u16 tx_hr;
+	u16 rx_hr;
+	u16 tx_tr;
+	u16 rx_tr;
+	u32 mtu;
+	u32 mru;
+};
+
+/* struct token_info - maintains Transmit buffer data handle
+ * @size:	size of transmit buffer
+ * @dma_handle: handle to allocated dma device memory area
+ * @vaddr:	virtual address mapping to allocated memory area
+ */
+struct token_info {
+	size_t size;
+	u8 *vaddr;
+	dma_addr_t dma_handle;
+};
+
+/* Default if virtio config space is unavailable */
+#define CFV_DEF_MTU_SIZE 4096
+#define CFV_DEF_HEADROOM 32
+#define CFV_DEF_TAILROOM 32
+
+/* Require IP header to be 4-byte aligned. */
+#define IP_HDR_ALIGN 4
+
+static inline void ctx_prep_iov(struct cfv_napi_context *ctx)
+{
+	if (ctx->riov.allocated) {
+		kfree(ctx->riov.iov);
+		ctx->riov.iov = NULL;
+		ctx->riov.allocated = false;
+	}
+	ctx->riov.iov = NULL;
+	ctx->riov.i = 0;
+	ctx->riov.max = 0;
+}
+
+static void cfv_release_cb(struct virtqueue *vq_tx)
+{
+	struct cfv_info *cfv = vq_tx->vdev->priv;
+	tasklet_schedule(&cfv->tx_release_tasklet);
+}
+
+/* This is invoked whenever the remote processor completed processing
+ * a TX msg we just sent it, and the buffer is put back to the used ring.
+ */
+static void cfv_release_used_buf(struct virtqueue *vq_tx)
+{
+	struct cfv_info *cfv = vq_tx->vdev->priv;
+	unsigned long flags;
+
+	BUG_ON(vq_tx != cfv->vq_tx);
+	WARN_ON_ONCE(irqs_disabled());
+
+	for (;;) {
+		unsigned int len;
+		struct token_info *buf_info;
+
+		/* Get used buffer from used ring to recycle used descriptors */
+		spin_lock_irqsave(&cfv->tx_lock, flags);
+		buf_info = virtqueue_get_buf(vq_tx, &len);
+
+		if (!buf_info)
+			goto out;
+
+		BUG_ON(!cfv->queued_tx);
+		if (--cfv->queued_tx <= cfv->watermark_tx) {
+			cfv->watermark_tx = 0;
+			netif_tx_wake_all_queues(cfv->ndev);
+		}
+		spin_unlock_irqrestore(&cfv->tx_lock, flags);
+
+		dma_free_coherent(vq_tx->vdev->dev.parent->parent,
+				  buf_info->size, buf_info->vaddr,
+				  buf_info->dma_handle);
+		kfree(buf_info);
+	}
+	return;
+out:
+	spin_unlock_irqrestore(&cfv->tx_lock, flags);
+}
+
+static struct sk_buff *cfv_alloc_and_copy_skb(int *err,
+					      struct cfv_info *cfv,
+					      u8 *frm, u32 frm_len)
+{
+	struct sk_buff *skb;
+	u32 cfpkt_len, pad_len;
+
+	*err = 0;
+	/* Verify that packet size with down-link header and mtu size */
+	if (frm_len > cfv->mru || frm_len <= cfv->rx_hr + cfv->rx_tr) {
+		netdev_err(cfv->ndev,
+			   "Invalid frmlen:%u  mtu:%u hr:%d tr:%d\n",
+			   frm_len, cfv->mru,  cfv->rx_hr,
+			   cfv->rx_tr);
+		*err = -EPROTO;
+		return NULL;
+	}
+
+	cfpkt_len = frm_len - (cfv->rx_hr + cfv->rx_tr);
+
+	pad_len = (unsigned long)(frm + cfv->rx_hr) & (IP_HDR_ALIGN - 1);
+
+	skb = netdev_alloc_skb(cfv->ndev, frm_len + pad_len);
+	if (!skb) {
+		*err = -ENOMEM;
+		return NULL;
+	}
+	/* Reserve space for headers. */
+	skb_reserve(skb, cfv->rx_hr + pad_len);
+
+	memcpy(skb_put(skb, cfpkt_len), frm + cfv->rx_hr, cfpkt_len);
+	return skb;
+}
+
+static int cfv_rx_poll(struct napi_struct *napi, int quota)
+{
+	struct cfv_info *cfv = container_of(napi, struct cfv_info, napi);
+	int rxcnt = 0;
+	int err = 0;
+	struct vringh_kiov wiov;
+	void *buf;
+	struct sk_buff *skb;
+	struct vringh_kiov *riov = &cfv->ctx.riov;
+
+	memset(&wiov, 0, sizeof(wiov));
+
+	do {
+		skb = NULL;
+		if (riov->i == riov->max) {
+			if (cfv->ctx.head != USHRT_MAX) {
+				vringh_complete_kern(cfv->vr_rx,
+						     cfv->ctx.head,
+						     0);
+				cfv->ctx.head = USHRT_MAX;
+			}
+
+			ctx_prep_iov(&cfv->ctx);
+			err = vringh_getdesc_kern(
+				cfv->vr_rx,
+				riov,
+				&wiov,
+				&cfv->ctx.head,
+				GFP_ATOMIC);
+
+			if (err <= 0)
+				goto out;
+
+			if (wiov.max != 0) {
+				/* CAIF does not use write descriptors */
+				err = -EPROTO;
+				goto out;
+			}
+		}
+
+		buf = phys_to_virt((unsigned long) riov->iov[riov->i].iov_base);
+		/* TODO: Add check on valid buffer address */
+
+		skb = cfv_alloc_and_copy_skb(&err, cfv, buf,
+					     riov->iov[riov->i].iov_len);
+		if (unlikely(err))
+			goto out;
+
+		/* Push received packet up the stack. */
+		skb->protocol = htons(ETH_P_CAIF);
+		skb_reset_mac_header(skb);
+		skb->dev = cfv->ndev;
+		err = netif_receive_skb(skb);
+		if (unlikely(err)) {
+			++cfv->ndev->stats.rx_dropped;
+		} else {
+			++cfv->ndev->stats.rx_packets;
+			cfv->ndev->stats.rx_bytes += skb->len;
+		}
+
+		++riov->i;
+		++rxcnt;
+	} while (rxcnt < quota);
+
+	return rxcnt;
+
+out:
+	switch (err) {
+	case 0:
+		/* Empty ring, enable notifications and stop NAPI polling */
+		if (!vringh_notify_enable_kern(cfv->vr_rx)) {
+			napi_complete(napi);
+			ctx_prep_iov(&cfv->ctx);
+		}
+		return rxcnt;
+		break;
+
+	case -ENOMEM:
+		dev_kfree_skb(skb);
+		/* Stop NAPI poll on OOM, we hope to be polled later */
+		napi_complete(napi);
+		vringh_notify_enable_kern(cfv->vr_rx);
+		break;
+
+	default:
+		/* We're doomed */
+		netdev_warn(cfv->ndev, "Bad ring, disable device\n");
+		cfv->ndev->stats.rx_dropped = riov->max - riov->i;
+		ctx_prep_iov(&cfv->ctx);
+		napi_complete(napi);
+		vringh_notify_disable_kern(cfv->vr_rx);
+		netif_carrier_off(cfv->ndev);
+		break;
+	}
+
+	return rxcnt;
+}
+
+static irqreturn_t cfv_recv(struct virtio_device *vdev, struct vringh *vr_rx)
+{
+	struct cfv_info *cfv = vdev->priv;
+
+	vringh_notify_disable_kern(cfv->vr_rx);
+	napi_schedule(&cfv->napi);
+	return IRQ_HANDLED;
+}
+
+static int cfv_netdev_open(struct net_device *netdev)
+{
+	struct cfv_info *cfv = netdev_priv(netdev);
+
+	netif_carrier_on(netdev);
+	napi_enable(&cfv->napi);
+	return 0;
+}
+
+static int cfv_netdev_close(struct net_device *netdev)
+{
+	struct cfv_info *cfv = netdev_priv(netdev);
+
+	netif_carrier_off(netdev);
+	napi_disable(&cfv->napi);
+	return 0;
+}
+
+static struct token_info *cfv_alloc_and_copy_to_dmabuf(struct cfv_info *cfv,
+						       struct sk_buff *skb,
+						       struct scatterlist *sg)
+{
+	struct caif_payload_info *info = (void *)&skb->cb;
+	struct token_info *buf_info = NULL;
+	u8 pad_len, hdr_ofs;
+
+	if (unlikely(cfv->tx_hr + skb->len + cfv->tx_tr > cfv->mtu)) {
+		netdev_warn(cfv->ndev, "Invalid packet len (%d > %d)\n",
+			    cfv->tx_hr + skb->len + cfv->tx_tr, cfv->mtu);
+		goto err;
+	}
+
+	buf_info = kmalloc(sizeof(struct token_info), GFP_ATOMIC);
+	if (unlikely(!buf_info))
+		goto err;
+
+	/* Make the IP header aligned in tbe buffer */
+	hdr_ofs = cfv->tx_hr + info->hdr_len;
+	pad_len = hdr_ofs & (IP_HDR_ALIGN - 1);
+	buf_info->size = cfv->tx_hr + skb->len + cfv->tx_tr + pad_len;
+
+	if (WARN_ON_ONCE(!cfv->vdev->dev.parent))
+		goto err;
+
+	/* allocate coherent memory for the buffers */
+	buf_info->vaddr =
+		dma_alloc_coherent(cfv->vdev->dev.parent->parent,
+				   buf_info->size, &buf_info->dma_handle,
+				   GFP_ATOMIC);
+	if (unlikely(!buf_info->vaddr)) {
+		netdev_warn(cfv->ndev,
+			    "Out of DMA memory (alloc %zu bytes)\n",
+			    buf_info->size);
+		goto err;
+	}
+
+	/* copy skbuf contents to send buffer */
+	skb_copy_bits(skb, 0, buf_info->vaddr + cfv->tx_hr + pad_len, skb->len);
+	sg_init_one(sg, buf_info->vaddr + pad_len,
+		    skb->len + cfv->tx_hr + cfv->rx_hr);
+
+	return buf_info;
+err:
+	kfree(buf_info);
+	return NULL;
+}
+
+/* This is invoked whenever the host processor application has sent
+ * up-link data. Send it in the TX VQ avail ring.
+ *
+ * CAIF Virtio sends does not use linked descriptors in the tx direction.
+ */
+static int cfv_netdev_tx(struct sk_buff *skb, struct net_device *netdev)
+{
+	struct cfv_info *cfv = netdev_priv(netdev);
+	struct token_info *buf_info;
+	struct scatterlist sg;
+	unsigned long flags;
+	int ret;
+
+	buf_info = cfv_alloc_and_copy_to_dmabuf(cfv, skb, &sg);
+
+	spin_lock_irqsave(&cfv->tx_lock, flags);
+	if (unlikely(!buf_info))
+		goto flow_off;
+
+	/* Add buffer to avail ring. Flow control below should ensure
+	 * that this always succeedes
+	 */
+	ret = virtqueue_add_buf(cfv->vq_tx, &sg, 1, 0,
+				buf_info, GFP_ATOMIC);
+
+	if (unlikely(WARN_ON(ret < 0))) {
+		kfree(buf_info);
+		goto flow_off;
+	}
+
+
+	/* update netdev statistics */
+	cfv->queued_tx++;
+	cfv->ndev->stats.tx_packets++;
+	cfv->ndev->stats.tx_bytes += skb->len;
+
+	/* tell the remote processor it has a pending message to read */
+	virtqueue_kick(cfv->vq_tx);
+
+	/* Flow-off check takes into account number of cpus to make sure
+	 * virtqueue will not be overfilled in any possible smp conditions.
+	 *
+	 * Flow-on is triggered when sufficient buffers are freed
+	 */
+	if (ret <= num_present_cpus()) {
+flow_off:
+		cfv->watermark_tx = cfv->queued_tx >> 1;
+		netif_tx_stop_all_queues(netdev);
+	}
+
+	spin_unlock_irqrestore(&cfv->tx_lock, flags);
+
+	dev_kfree_skb(skb);
+	tasklet_schedule(&cfv->tx_release_tasklet);
+	return NETDEV_TX_OK;
+}
+
+static void cfv_tx_release_tasklet(unsigned long drv)
+{
+	struct cfv_info *cfv = (struct cfv_info *)drv;
+	cfv_release_used_buf(cfv->vq_tx);
+}
+
+static const struct net_device_ops cfv_netdev_ops = {
+	.ndo_open = cfv_netdev_open,
+	.ndo_stop = cfv_netdev_close,
+	.ndo_start_xmit = cfv_netdev_tx,
+};
+
+static void cfv_netdev_setup(struct net_device *netdev)
+{
+	netdev->netdev_ops = &cfv_netdev_ops;
+	netdev->type = ARPHRD_CAIF;
+	netdev->tx_queue_len = 100;
+	netdev->flags = IFF_POINTOPOINT | IFF_NOARP;
+	netdev->mtu = CFV_DEF_MTU_SIZE;
+	netdev->destructor = free_netdev;
+}
+
+static int cfv_probe(struct virtio_device *vdev)
+{
+	vq_callback_t *vq_cbs = cfv_release_cb;
+	const char *names =  "output";
+	const char *cfv_netdev_name = "cfvrt";
+	struct net_device *netdev;
+	struct virtqueue *vqs;
+
+	struct cfv_info *cfv;
+	int err = 0;
+
+	netdev = alloc_netdev(sizeof(struct cfv_info), cfv_netdev_name,
+			      cfv_netdev_setup);
+	if (!netdev)
+		return -ENOMEM;
+
+	cfv = netdev_priv(netdev);
+	cfv->vdev = vdev;
+	cfv->ndev = netdev;
+
+	spin_lock_init(&cfv->tx_lock);
+
+	cfv->vr_rx = rproc_virtio_new_vringh(vdev, RX_RING_INDEX, cfv_recv);
+	if (!cfv->vr_rx)
+		goto free_cfv;
+
+	/* Get the TX (uplink) virtque */
+	err = vdev->config->find_vqs(vdev, 1, &vqs, &vq_cbs, &names);
+	if (err)
+		goto free_cfv;
+
+	cfv->vq_tx = vqs;
+
+#define GET_VIRTIO_CONFIG_OPS(_v, _var, _f) \
+	((_v)->config->get(_v, offsetof(struct virtio_caif_transf_config, _f), \
+			   &_var, \
+			   FIELD_SIZEOF(struct virtio_caif_transf_config, _f)))
+
+	if (vdev->config->get) {
+		GET_VIRTIO_CONFIG_OPS(vdev, cfv->tx_hr, headroom);
+		GET_VIRTIO_CONFIG_OPS(vdev, cfv->rx_hr, headroom);
+		GET_VIRTIO_CONFIG_OPS(vdev, cfv->tx_tr, tailroom);
+		GET_VIRTIO_CONFIG_OPS(vdev, cfv->rx_tr, tailroom);
+		GET_VIRTIO_CONFIG_OPS(vdev, cfv->mtu, mtu);
+		GET_VIRTIO_CONFIG_OPS(vdev, cfv->mru, mtu);
+	} else {
+		cfv->tx_hr = CFV_DEF_HEADROOM;
+		cfv->rx_hr = CFV_DEF_HEADROOM;
+		cfv->tx_tr = CFV_DEF_TAILROOM;
+		cfv->rx_tr = CFV_DEF_TAILROOM;
+		cfv->mtu = CFV_DEF_MTU_SIZE;
+		cfv->mru = CFV_DEF_MTU_SIZE;
+	}
+
+	netdev->needed_headroom = cfv->tx_hr;
+	netdev->needed_tailroom = cfv->tx_tr;
+
+	/* Subtract needed tailroom from MTU to ensure enough room */
+	netdev->mtu = cfv->mtu - cfv->tx_tr;
+
+	vdev->priv = cfv;
+	memset(&cfv->ctx, 0, sizeof(cfv->ctx));
+	cfv->ctx.head = USHRT_MAX;
+
+	netif_napi_add(netdev, &cfv->napi, cfv_rx_poll, CFV_DEFAULT_QUOTA);
+	tasklet_init(&cfv->tx_release_tasklet,
+		     cfv_tx_release_tasklet,
+		     (unsigned long)cfv);
+
+	netif_carrier_off(netdev);
+
+	/* register Netdev */
+	err = register_netdev(netdev);
+	if (err) {
+		dev_err(&vdev->dev, "Unable to register netdev (%d)\n", err);
+		goto vqs_del;
+	}
+
+	/* tell the remote processor it can start sending messages */
+	rproc_virtio_kick_vringh(vdev, RX_RING_INDEX);
+
+	return 0;
+
+vqs_del:
+	vdev->config->del_vqs(cfv->vdev);
+free_cfv:
+	free_netdev(netdev);
+	return err;
+}
+
+static void cfv_remove(struct virtio_device *vdev)
+{
+	struct cfv_info *cfv = vdev->priv;
+	vdev->config->reset(vdev);
+	rproc_virtio_del_vringh(vdev, RX_RING_INDEX);
+	cfv->vr_rx = NULL;
+	vdev->config->del_vqs(cfv->vdev);
+	unregister_netdev(cfv->ndev);
+}
+
+static struct virtio_device_id id_table[] = {
+	{ VIRTIO_ID_CAIF, VIRTIO_DEV_ANY_ID },
+	{ 0 },
+};
+
+static unsigned int features[] = {
+};
+
+static struct virtio_driver caif_virtio_driver = {
+	.feature_table		= features,
+	.feature_table_size	= ARRAY_SIZE(features),
+	.driver.name		= KBUILD_MODNAME,
+	.driver.owner		= THIS_MODULE,
+	.id_table		= id_table,
+	.probe			= cfv_probe,
+	.remove			= cfv_remove,
+};
+
+module_driver(caif_virtio_driver, register_virtio_driver,
+	      unregister_virtio_driver);
+MODULE_DEVICE_TABLE(virtio, id_table);
diff --git a/include/linux/virtio_caif.h b/include/linux/virtio_caif.h
new file mode 100644
index 0000000..5d2d312
--- /dev/null
+++ b/include/linux/virtio_caif.h
@@ -0,0 +1,24 @@
+/*
+ * Copyright (C) ST-Ericsson AB 2012
+ * Author: Sjur Brændeland <sjur.brandeland@stericsson.com>
+ *
+ * This header is BSD licensed so
+ * anyone can use the definitions to implement compatible remote processors
+ */
+
+#ifndef VIRTIO_CAIF_H
+#define VIRTIO_CAIF_H
+
+#include <linux/types.h>
+struct virtio_caif_transf_config {
+	u16 headroom;
+	u16 tailroom;
+	u32 mtu;
+	u8 reserved[4];
+};
+
+struct virtio_caif_config {
+	struct virtio_caif_transf_config uplink, downlink;
+	u8 reserved[8];
+};
+#endif
diff --git a/include/uapi/linux/virtio_ids.h b/include/uapi/linux/virtio_ids.h
index a7630d0..284fc3a 100644
--- a/include/uapi/linux/virtio_ids.h
+++ b/include/uapi/linux/virtio_ids.h
@@ -38,5 +38,6 @@
 #define VIRTIO_ID_SCSI		8 /* virtio scsi */
 #define VIRTIO_ID_9P		9 /* 9p virtio console */
 #define VIRTIO_ID_RPROC_SERIAL 11 /* virtio remoteproc serial link */
+#define VIRTIO_ID_CAIF	       12 /* Virtio caif */
 
 #endif /* _LINUX_VIRTIO_IDS_H */
-- 
1.7.5.4

Re: [PATCH vringh 2/2] caif_virtio: Introduce caif over virtio

[Rusty Russell]

sjur.brandeland@stericsson.com writes:
> From: Vikram ARV <vikram.arv@stericsson.com>
>
> Add the the Virtio shared memory driver for STE Modems.
> caif_virtio is implemented utilizing the virtio framework
> for data transport and is managed with the remoteproc frameworks.
>
> The Virtio queue is used for transmitting data to the modem, and
> the new vringh implementation is receiving data over the vring.

OK, let's refine vringh now we can see another user:

> + * @head: Last descriptor ID we received from vringh_getdesc_kern.
> + *	  We use this to put descriptor back on the used ring. USHRT_MAX is
> + *	  used to indicate invalid head-id.

> + */
> +struct cfv_napi_context {
> +	struct vringh_kiov riov;
> +	unsigned short head;
> +};

Usually we use an int, and -1.  I imagine it'll take no more space,
due to padding.

> +static inline void ctx_prep_iov(struct cfv_napi_context *ctx)
> +{
> +	if (ctx->riov.allocated) {
> +		kfree(ctx->riov.iov);
> +		ctx->riov.iov = NULL;
> +		ctx->riov.allocated = false;
> +	}
> +	ctx->riov.iov = NULL;
> +	ctx->riov.i = 0;
> +	ctx->riov.max = 0;
> +}

Hmm, we should probably make sure you don't have to do this: that if
allocated once, you can simply reuse it by setting i = 0.

(This requires some care in the error handling paths, so we don't
free it from under you)...

And you probably want to free this up in cfv_remove() instead?

I'll create and test a patch now.

> +		if (riov->i == riov->max) {
> +			if (cfv->ctx.head != USHRT_MAX) {
> +				vringh_complete_kern(cfv->vr_rx,
> +						     cfv->ctx.head,
> +						     0);
> +				cfv->ctx.head = USHRT_MAX;
> +			}
> +
> +			ctx_prep_iov(&cfv->ctx);
> +			err = vringh_getdesc_kern(
> +				cfv->vr_rx,
> +				riov,
> +				&wiov,
> +				&cfv->ctx.head,
> +				GFP_ATOMIC);
> +
> +			if (err <= 0)
> +				goto out;
> +
> +			if (wiov.max != 0) {
> +				/* CAIF does not use write descriptors */
> +				err = -EPROTO;
> +				goto out;
> +			}

This is probably a common case, so we should generate this error inside
vringh_getdesc (if wiov is NULL).

I'll do that too...

> +module_driver(caif_virtio_driver, register_virtio_driver,
> +	      unregister_virtio_driver);
> +MODULE_DEVICE_TABLE(virtio, id_table);

The comment on module_driver says:

 * Use this macro to construct bus specific macros for registering
 * drivers, and do not use it on its own.

Want to send me a patch to define module_virtio_driver?

Thanks!
Rusty.

RE: [PATCH vringh 2/2] caif_virtio: Introduce caif over virtio

[Sjur BRENDELAND]

Hi Rusty,

> From: Rusty Russell [mailto:rusty@rustcorp.com.au]
> sjur.brandeland@stericsson.com writes:
> > From: Vikram ARV <vikram.arv@stericsson.com>
> >
> > Add the the Virtio shared memory driver for STE Modems.
> > caif_virtio is implemented utilizing the virtio framework
> > for data transport and is managed with the remoteproc frameworks.
> >
> > The Virtio queue is used for transmitting data to the modem, and
> > the new vringh implementation is receiving data over the vring.
> 
> OK, let's refine vringh now we can see another user:
> 
> > + * @head: Last descriptor ID we received from vringh_getdesc_kern.
> > + *	  We use this to put descriptor back on the used ring. USHRT_MAX is
> > + *	  used to indicate invalid head-id.
> 
> > + */
> > +struct cfv_napi_context {
> > +	struct vringh_kiov riov;
> > +	unsigned short head;
> > +};
> 
> Usually we use an int, and -1.  I imagine it'll take no more space,
> due to padding.

I'm passing a pointer to "head" to vringh_getdesc_kern() directly, are you
suggesting to change the head argument in  vringh_getdesc_kern()
to a int pointer as well then?

> > +static inline void ctx_prep_iov(struct cfv_napi_context *ctx)
> > +{
> > +	if (ctx->riov.allocated) {
> > +		kfree(ctx->riov.iov);
> > +		ctx->riov.iov = NULL;
> > +		ctx->riov.allocated = false;
> > +	}
> > +	ctx->riov.iov = NULL;
> > +	ctx->riov.i = 0;
> > +	ctx->riov.max = 0;
> > +}
> 
> Hmm, we should probably make sure you don't have to do this: that if
> allocated once, you can simply reuse it by setting i = 0.

Yes, I had problems getting the alloc/free of iov right. This is
perhaps the least intuitively part of the API. I maybe it's just me, but
I think some more helper functions and support from vringh in this
area would be great.

> 
> (This requires some care in the error handling paths, so we don't
> free it from under you)...
> 
> And you probably want to free this up in cfv_remove() instead?

OK, I'll look into this when you have a some more code ready...

> 
> I'll create and test a patch now.
> 
> > +		if (riov->i == riov->max) {
> > +			if (cfv->ctx.head != USHRT_MAX) {
> > +				vringh_complete_kern(cfv->vr_rx,
> > +						     cfv->ctx.head,
> > +						     0);
> > +				cfv->ctx.head = USHRT_MAX;
> > +			}
> > +
> > +			ctx_prep_iov(&cfv->ctx);
> > +			err = vringh_getdesc_kern(
> > +				cfv->vr_rx,
> > +				riov,
> > +				&wiov,
> > +				&cfv->ctx.head,
> > +				GFP_ATOMIC);
> > +
> > +			if (err <= 0)
> > +				goto out;
> > +
> > +			if (wiov.max != 0) {
> > +				/* CAIF does not use write descriptors */
> > +				err = -EPROTO;
> > +				goto out;
> > +			}
> 
> This is probably a common case, so we should generate this error inside
> vringh_getdesc (if wiov is NULL).
> 
> I'll do that too...
> 
> > +module_driver(caif_virtio_driver, register_virtio_driver,
> > +	      unregister_virtio_driver);
> > +MODULE_DEVICE_TABLE(virtio, id_table);
> 
> The comment on module_driver says:
> 
>  * Use this macro to construct bus specific macros for registering
>  * drivers, and do not use it on its own.
> 
> Want to send me a patch to define module_virtio_driver?

Sure, I can do that.

Thanks a lot Rusty.

Regards,
Sjur

RE: [PATCH vringh 2/2] caif_virtio: Introduce caif over virtio

[Rusty Russell]

Sjur BRENDELAND <sjur.brandeland@stericsson.com> writes:
> Hi Rusty,
>
>> From: Rusty Russell [mailto:rusty@rustcorp.com.au]
>> sjur.brandeland@stericsson.com writes:
>> > +struct cfv_napi_context {
>> > +	struct vringh_kiov riov;
>> > +	unsigned short head;
>> > +};
>> 
>> Usually we use an int, and -1.  I imagine it'll take no more space,
>> due to padding.
>
> I'm passing a pointer to "head" to vringh_getdesc_kern() directly, are you
> suggesting to change the head argument in  vringh_getdesc_kern()
> to a int pointer as well then?

No, that's OK in that case.

>> > +static inline void ctx_prep_iov(struct cfv_napi_context *ctx)
>> > +{
>> > +	if (ctx->riov.allocated) {
>> > +		kfree(ctx->riov.iov);
>> > +		ctx->riov.iov = NULL;
>> > +		ctx->riov.allocated = false;
>> > +	}
>> > +	ctx->riov.iov = NULL;
>> > +	ctx->riov.i = 0;
>> > +	ctx->riov.max = 0;
>> > +}
>> 
>> Hmm, we should probably make sure you don't have to do this: that if
>> allocated once, you can simply reuse it by setting i = 0.
>
> Yes, I had problems getting the alloc/free of iov right. This is
> perhaps the least intuitively part of the API. I maybe it's just me, but
> I think some more helper functions and support from vringh in this
> area would be great.

Yes, I've neatened my git tree, an in particular added a commit which
covers this explicitly, and one for the -EPROTO when we get unexpected
r/w bufs.  I've appended them below.

>> (This requires some care in the error handling paths, so we don't
>> free it from under you)...
>> 
>> And you probably want to free this up in cfv_remove() instead?
>
> OK, I'll look into this when you have a some more code ready...

Pushed (rebase!) to my git tree now.  You can also see what I've done to
vhost, though there's more to come on that front.

vringh: Allow reuse of vringh_iov/vringh_kiov.

We allocate a larger iov if we need to, but it's better if the caller
can simply hand it back for reuse, rather than freeing it every time.

We also want to interate without mangling the iov (useful for vhost).

Hence we track the number allocated and the number used separately, as
well as the offset within the current iov element, and add helpers to
init/cleanup and reuse.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c
index 55f3805..eb19948 100644
--- a/drivers/vhost/vringh.c
+++ b/drivers/vhost/vringh.c
@@ -73,21 +73,22 @@ static inline ssize_t vringh_iov_xfer(struct vringh_kiov *iov,
 {
 	int err, done = 0;
 
-	while (len && iov->i < iov->max) {
+	while (len && iov->i < iov->used) {
 		size_t partlen;
 
-		partlen = min(iov->iov[iov->i].iov_len, len);
-		err = xfer(iov->iov[iov->i].iov_base, ptr, partlen);
+		partlen = min(iov->iov[iov->i].iov_len - iov->off, len);
+		err = xfer(iov->iov[iov->i].iov_base + iov->off, ptr, partlen);
 		if (err)
 			return err;
 		done += partlen;
 		len -= partlen;
 		ptr += partlen;
-		iov->iov[iov->i].iov_base += partlen;
-		iov->iov[iov->i].iov_len -= partlen;
+		iov->off += partlen;
 
-		if (iov->iov[iov->i].iov_len == 0)
+		if (iov->off == iov->iov[iov->i].iov_len) {
+			iov->off = 0;
 			iov->i++;
+		}
 	}
 	return done;
 }
@@ -167,24 +168,25 @@ static int move_to_indirect(int *up_next, u16 *i, void *addr,
 static int resize_iovec(struct vringh_kiov *iov, gfp_t gfp)
 {
 	struct kvec *new;
-	unsigned int new_num = iov->max * 2;
+	unsigned int flag, new_num = (iov->max_num & ~VRINGH_IOV_ALLOCATED) * 2;
 
 	if (new_num < 8)
 		new_num = 8;
 
-	if (iov->allocated)
+	flag = (iov->max_num & VRINGH_IOV_ALLOCATED);
+	if (flag)
 		new = krealloc(iov->iov, new_num * sizeof(struct iovec), gfp);
 	else {
 		new = kmalloc(new_num * sizeof(struct iovec), gfp);
 		if (new) {
 			memcpy(new, iov->iov, iov->i * sizeof(struct iovec));
-			iov->allocated = true;
+			flag = VRINGH_IOV_ALLOCATED;
 		}
 	}
 	if (!new)
 		return -ENOMEM;
 	iov->iov = new;
-	iov->max = new_num;
+	iov->max_num = (new_num | flag);
 	return 0;
 }
 
@@ -257,6 +259,8 @@ __vringh_iov(struct vringh *vrh, u16 i,
 	up_next = -1;
 
 	riov->i = wiov->i = 0;
+	riov->used = wiov->used = 0;
+
 	for (;;) {
 		void *addr;
 		struct vringh_kiov *iov;
@@ -319,15 +323,15 @@ __vringh_iov(struct vringh *vrh, u16 i,
 		}
 		addr = (void *)(unsigned long)(desc.addr + range.offset);
 
-		if (unlikely(iov->i == iov->max)) {
+		if (unlikely(iov->used == (iov->max_num & ~VRINGH_IOV_ALLOCATED))) {
 			err = resize_iovec(iov, gfp);
 			if (err)
 				goto fail;
 		}
 
-		iov->iov[iov->i].iov_base = addr;
-		iov->iov[iov->i].iov_len = len;
-		iov->i++;
+		iov->iov[iov->used].iov_base = addr;
+		iov->iov[iov->used].iov_len = len;
+		iov->used++;
 
 		if (unlikely(len != desc.len)) {
 			desc.len -= len;
@@ -354,17 +358,9 @@ __vringh_iov(struct vringh *vrh, u16 i,
 		}
 	}
 
-	/* Reset for fresh iteration. */
-	riov->max = riov->i;
-	wiov->max = wiov->i;
-	riov->i = wiov->i = 0;
 	return 0;
 
 fail:
-	if (riov->allocated)
-		kfree(riov->iov);
-	if (wiov->allocated)
-		kfree(wiov->iov);
 	return err;
 }
 
@@ -612,8 +608,7 @@ EXPORT_SYMBOL(vringh_init_user);
  * *head will be vrh->vring.num.  You may be able to ignore an invalid
  * descriptor, but there's not much you can do with an invalid ring.
  *
- * If it returns 1, riov->allocated and wiov->allocated indicate if you
- * have to kfree riov->iov and wiov->iov respectively.
+ * Note that you may need to clean up riov and wiov, even on error!
  */
 int vringh_getdesc_user(struct vringh *vrh,
 			struct vringh_iov *riov,
@@ -639,10 +634,10 @@ int vringh_getdesc_user(struct vringh *vrh,
 		     offsetof(struct vringh_iov, iov));
 	BUILD_BUG_ON(offsetof(struct vringh_kiov, i) !=
 		     offsetof(struct vringh_iov, i));
-	BUILD_BUG_ON(offsetof(struct vringh_kiov, max) !=
-		     offsetof(struct vringh_iov, max));
-	BUILD_BUG_ON(offsetof(struct vringh_kiov, allocated) !=
-		     offsetof(struct vringh_iov, allocated));
+	BUILD_BUG_ON(offsetof(struct vringh_kiov, used) !=
+		     offsetof(struct vringh_iov, used));
+	BUILD_BUG_ON(offsetof(struct vringh_kiov, max_num) !=
+		     offsetof(struct vringh_iov, max_num));
 	BUILD_BUG_ON(sizeof(struct iovec) != sizeof(struct kvec));
 	BUILD_BUG_ON(offsetof(struct iovec, iov_base) !=
 		     offsetof(struct kvec, iov_base));
@@ -867,8 +862,12 @@ EXPORT_SYMBOL(vringh_init_kern);
  *
  * Returns 0 if there was no descriptor, 1 if there was, or -errno.
  *
- * If it returns 1, riov->allocated and wiov->allocated indicate if you
- * have to kfree riov->iov and wiov->iov respectively.
+ * Note that on error return, you can tell the difference between an
+ * invalid ring and a single invalid descriptor: in the former case,
+ * *head will be vrh->vring.num.  You may be able to ignore an invalid
+ * descriptor, but there's not much you can do with an invalid ring.
+ *
+ * Note that you may need to clean up riov and wiov, even on error!
  */
 int vringh_getdesc_kern(struct vringh *vrh,
 			struct vringh_kiov *riov,
diff --git a/include/linux/vringh.h b/include/linux/vringh.h
index d78e89e..ab41185 100644
--- a/include/linux/vringh.h
+++ b/include/linux/vringh.h
@@ -25,6 +25,7 @@
 #define _LINUX_VRINGH_H
 #include <uapi/linux/virtio_ring.h>
 #include <linux/uio.h>
+#include <linux/slab.h>
 #include <asm/barrier.h>
 
 /* virtio_ring with information needed for host access. */
@@ -60,17 +61,20 @@ struct vringh_range {
 /* All the information about an iovec. */
 struct vringh_iov {
 	struct iovec *iov;
-	unsigned i, max;
-	bool allocated;
+	size_t off; /* Within iov[i] */
+	unsigned i, used, max_num;
 };
 
 /* All the information about a kvec. */
 struct vringh_kiov {
 	struct kvec *iov;
-	unsigned i, max;
-	bool allocated;
+	size_t off; /* Within iov[i] */
+	unsigned i, used, max_num;
 };
 
+/* Flag on max_num to indicate we're kmalloced. */
+#define VRINGH_IOV_ALLOCATED 0x8000000
+
 /* Helpers for userspace vrings. */
 int vringh_init_user(struct vringh *vrh, u32 features,
 		     unsigned int num, bool weak_barriers,
@@ -78,6 +82,29 @@ int vringh_init_user(struct vringh *vrh, u32 features,
 		     struct vring_avail __user *avail,
 		     struct vring_used __user *used);
 
+static inline void vringh_iov_init(struct vringh_iov *iov,
+				   struct iovec *iovec, unsigned num)
+{
+	iov->used = iov->i = 0;
+	iov->off = 0;
+	iov->max_num = num;
+	iov->iov = iovec;
+}
+
+static inline void vringh_iov_reset(struct vringh_iov *iov)
+{
+	iov->off = 0;
+	iov->i = 0;
+}
+
+static inline void vringh_iov_cleanup(struct vringh_iov *iov)
+{
+	if (iov->max_num & VRINGH_IOV_ALLOCATED)
+		kfree(iov->iov);
+	iov->max_num = iov->used = iov->i = iov->off = 0;
+	iov->iov = NULL;
+}
+
 /* Convert a descriptor into iovecs. */
 int vringh_getdesc_user(struct vringh *vrh,
 			struct vringh_iov *riov,
@@ -115,6 +142,29 @@ int vringh_init_kern(struct vringh *vrh, u32 features,
 		     struct vring_avail *avail,
 		     struct vring_used *used);
 
+static inline void vringh_kiov_init(struct vringh_kiov *kiov,
+				    struct kvec *kvec, unsigned num)
+{
+	kiov->used = kiov->i = 0;
+	kiov->off = 0;
+	kiov->max_num = num;
+	kiov->iov = kvec;
+}
+
+static inline void vringh_kiov_reset(struct vringh_kiov *kiov)
+{
+	kiov->off = 0;
+	kiov->i = 0;
+}
+
+static inline void vringh_kiov_cleanup(struct vringh_kiov *kiov)
+{
+	if (kiov->max_num & VRINGH_IOV_ALLOCATED)
+		kfree(kiov->iov);
+	kiov->max_num = kiov->used = kiov->i = kiov->off = 0;
+	kiov->iov = NULL;
+}
+
 int vringh_getdesc_kern(struct vringh *vrh,
 			struct vringh_kiov *riov,
 			struct vringh_kiov *wiov,

vringh: allow NULL riov and wiov to vringh_getdesc_user()

There are numerous cases where we don't expect any writable (or
readable) descriptors, so handle that in common code rather than
making the caller check.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c
index ec50cd9..6b37974 100644
--- a/drivers/vhost/vringh.c
+++ b/drivers/vhost/vringh.c
@@ -257,8 +257,13 @@ __vringh_iov(struct vringh *vrh, u16 i,
 	desc_max = vrh->vring.num;
 	up_next = -1;
 
-	riov->i = wiov->i = 0;
-	riov->used = wiov->used = 0;
+	if (riov)
+		riov->i = riov->used = 0;
+	else if (wiov)
+		wiov->i = wiov->used = 0;
+	else
+		/* You must want something! */
+		BUG();
 
 	for (;;) {
 		void *addr;
@@ -305,7 +310,7 @@ __vringh_iov(struct vringh *vrh, u16 i,
 			iov = wiov;
 		else {
 			iov = riov;
-			if (unlikely(wiov->i)) {
+			if (unlikely(wiov && wiov->i)) {
 				vringh_bad("Readable desc %p after writable",
 					   &descs[i]);
 				err = -EINVAL;
@@ -313,6 +318,13 @@ __vringh_iov(struct vringh *vrh, u16 i,
 			}
 		}
 
+		if (!iov) {
+			vringh_bad("Unexpected %s desc",
+				   !wiov ? "writable" : "readable");
+			err = -EPROTO;
+			goto fail;
+		}
+
 	again:
 		/* Make sure it's OK, and get offset. */
 		len = desc.len;
@@ -595,8 +607,8 @@ EXPORT_SYMBOL(vringh_init_user);
 /**
  * vringh_getdesc_user - get next available descriptor from userspace ring.
  * @vrh: the userspace vring.
- * @riov: where to put the readable descriptors.
- * @wiov: where to put the writable descriptors.
+ * @riov: where to put the readable descriptors (or NULL)
+ * @wiov: where to put the writable descriptors (or NULL)
  * @getrange: function to call to check ranges.
  * @head: head index we received, for passing to vringh_complete_user().
  *
@@ -854,8 +866,8 @@ EXPORT_SYMBOL(vringh_init_kern);
 /**
  * vringh_getdesc_kern - get next available descriptor from kernelspace ring.
  * @vrh: the kernelspace vring.
- * @riov: where to put the readable descriptors.
- * @wiov: where to put the writable descriptors.
+ * @riov: where to put the readable descriptors (or NULL)
+ * @wiov: where to put the writable descriptors (or NULL)
  * @head: head index we received, for passing to vringh_complete_kern().
  * @gfp: flags for allocating larger riov/wiov.
  *

Re: [PATCH vringh 2/2] caif_virtio: Introduce caif over virtio

[Sjur Brændeland]

Hi Rusty,

On Wed, Feb 13, 2013 at 11:16 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> Sjur BRENDELAND <sjur.brandeland@stericsson.com> writes:
>>> > +static inline void ctx_prep_iov(struct cfv_napi_context *ctx)
>>> > +{
>>> > +  if (ctx->riov.allocated) {
>>> > +          kfree(ctx->riov.iov);
>>> > +          ctx->riov.iov = NULL;
>>> > +          ctx->riov.allocated = false;
>>> > +  }
>>> > +  ctx->riov.iov = NULL;
>>> > +  ctx->riov.i = 0;
>>> > +  ctx->riov.max = 0;
>>> > +}
>>>
>>> Hmm, we should probably make sure you don't have to do this: that if
>>> allocated once, you can simply reuse it by setting i = 0.
>>
>> Yes, I had problems getting the alloc/free of iov right. This is
>> perhaps the least intuitively part of the API. I maybe it's just me, but
>> I think some more helper functions and support from vringh in this
>> area would be great.
>
> Yes, I've neatened my git tree, an in particular added a commit which
> covers this explicitly, and one for the -EPROTO when we get unexpected
> r/w bufs.  I've appended them below.

Great, thanks. This helps a lot. I picked up your patch-sett yesterday
so my V2 patch-set is using this already.
...
> +static inline void vringh_iov_init(struct vringh_iov *iov,
> +                                  struct iovec *iovec, unsigned num)
> +{
> +       iov->used = iov->i = 0;
> +       iov->off = 0;
> +       iov->max_num = num;
> +       iov->iov = iovec;
> +}

How about supporting struct vringh_kiov and struct kvec as well?
I currently get the following complaints with my V2 patch-set:

drivers/net/caif/caif_virtio.c:486:2: warning: passing argument 1 of
‘vringh_iov_init’ from incompatible pointer type [enabled by default]
...

Thanks,
Sjur

Re: [PATCH vringh 2/2] caif_virtio: Introduce caif over virtio

[Rusty Russell]

Sjur Brændeland <sjurbren@gmail.com> writes:
> How about supporting struct vringh_kiov and struct kvec as well?
> I currently get the following complaints with my V2 patch-set:
>
> drivers/net/caif/caif_virtio.c:486:2: warning: passing argument 1 of
> ‘vringh_iov_init’ from incompatible pointer type [enabled by default]

vringh_kiov_init()?  Did I miss something else?

Cheers,
Rusty.

Re: [PATCH vringh 2/2] caif_virtio: Introduce caif over virtio

[Sjur Brændeland]

> Sjur Brændeland <sjurbren@gmail.com> writes:
>> How about supporting struct vringh_kiov and struct kvec as well?
>> I currently get the following complaints with my V2 patch-set:
>>
>> drivers/net/caif/caif_virtio.c:486:2: warning: passing argument 1 of
>> ‘vringh_iov_init’ from incompatible pointer type [enabled by default]
>
> vringh_kiov_init()?  Did I miss something else?

I get a warning for my useof vringh_iov_cleanup() in
addition to the one above.

How about adding kiov variants of  vringh_iov_cleanup()
and vringh_iov_reset() as well?

Thanks,
Sjur

Re: [PATCH vringh 2/2] caif_virtio: Introduce caif over virtio

[Rusty Russell]

Sjur Brændeland <sjurbren@gmail.com> writes:

>> Sjur Brændeland <sjurbren@gmail.com> writes:
>>> How about supporting struct vringh_kiov and struct kvec as well?
>>> I currently get the following complaints with my V2 patch-set:
>>>
>>> drivers/net/caif/caif_virtio.c:486:2: warning: passing argument 1 of
>>> ‘vringh_iov_init’ from incompatible pointer type [enabled by default]
>>
>> vringh_kiov_init()?  Did I miss something else?
>
> I get a warning for my useof vringh_iov_cleanup() in
> addition to the one above.
>
> How about adding kiov variants of  vringh_iov_cleanup()
> and vringh_iov_reset() as well?

Hmm, I have those here too (in the header):

+static inline void vringh_kiov_reset(struct vringh_kiov *kiov)
+{
+	kiov->off = 0;
+	kiov->i = 0;
+}
+
+static inline void vringh_kiov_cleanup(struct vringh_kiov *kiov)
+{
+	if (kiov->max_num & VRINGH_IOV_ALLOCATED)
+		kfree(kiov->iov);
+	kiov->max_num = kiov->used = kiov->i = kiov->off = 0;
+	kiov->iov = NULL;
+}

I've folded the patches and put them into my pending-rebases branch,
ready for virtio-next.

I'd really like MST's ack on this, too, so I'll repost the series.

Cheers,
Rusty.