[PATCHv7 0/4] virtio_console: Add rproc_serial driver

[PATCHv7 0/4] virtio_console: Add rproc_serial driver

[sjur.brandeland]

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

This patch-set introduces a new virtio type "rproc_serial" for communicating
with remote processors over shared memory. The driver depends on the
the remoteproc framework. As preparation for introducing "rproc_serial"
I've done a refactoring of the transmit buffer handling.

This patch-set is a rework of the patch-set from Sept 25th, hopefully all
review comments has been addressed.

The fist patch is a bugfix and migth be applicable for 3.7.

Thanks,
Sjur

Sjur Brændeland (4):
  virtio_console: Free buffer if splice fails
  virtio_console: Use kmalloc instead of kzalloc
  virtio_console: Merge struct buffer_token into struct port_buffer
  virtio_console: Add support for remoteproc serial

 drivers/char/virtio_console.c |  328 +++++++++++++++++++++++++++++------------
 include/linux/virtio_ids.h    |    1 +
 2 files changed, 234 insertions(+), 95 deletions(-)

-- 
1.7.5.4

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCHv7 0/4] virtio_console: Add rproc_serial driver

[sjur.brandeland]

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

This patch-set introduces a new virtio type "rproc_serial" for communicating
with remote processors over shared memory. The driver depends on the
the remoteproc framework. As preparation for introducing "rproc_serial"
I've done a refactoring of the transmit buffer handling.

This patch-set is a rework of the patch-set from Sept 25th, hopefully all
review comments has been addressed.

The fist patch is a bugfix and migth be applicable for 3.7.

Thanks,
Sjur

Sjur Brændeland (4):
  virtio_console: Free buffer if splice fails
  virtio_console: Use kmalloc instead of kzalloc
  virtio_console: Merge struct buffer_token into struct port_buffer
  virtio_console: Add support for remoteproc serial

 drivers/char/virtio_console.c |  328 +++++++++++++++++++++++++++++------------
 include/linux/virtio_ids.h    |    1 +
 2 files changed, 234 insertions(+), 95 deletions(-)

-- 
1.7.5.4

[PATCHv7 1/4] virtio_console: Free buffer if splice fails

[sjur.brandeland]

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

Free the allocated scatter list if send_pages fails in function
port_splice_write.

Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
 drivers/char/virtio_console.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 8ab9c3d..c36b2f6 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -879,6 +879,8 @@ static ssize_t port_fops_splice_write(struct pipe_inode_info *pipe,
 	if (likely(ret > 0))
 		ret = send_pages(port, sgl.sg, sgl.n, sgl.len, true);
 
+	if (unlikely(ret <= 0))
+		kfree(sgl.sg);
 	return ret;
 }
 
-- 
1.7.5.4

Re: [PATCHv7 1/4] virtio_console: Free buffer if splice fails

[Rusty Russell]

sjur.brandeland@stericsson.com writes:
> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
>
> Free the allocated scatter list if send_pages fails in function
> port_splice_write.
>
> Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>

Didn't see Amit ack this, but applied anyway.

Thanks,
Rusty.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCHv7 1/4] virtio_console: Free buffer if splice fails

[Rusty Russell]

sjur.brandeland@stericsson.com writes:
> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
>
> Free the allocated scatter list if send_pages fails in function
> port_splice_write.
>
> Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>

Didn't see Amit ack this, but applied anyway.

Thanks,
Rusty.

[PATCHv7 1/4] virtio_console: Free buffer if splice fails

[sjur.brandeland]

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

Free the allocated scatter list if send_pages fails in function
port_splice_write.

Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
 drivers/char/virtio_console.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 8ab9c3d..c36b2f6 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -879,6 +879,8 @@ static ssize_t port_fops_splice_write(struct pipe_inode_info *pipe,
 	if (likely(ret > 0))
 		ret = send_pages(port, sgl.sg, sgl.n, sgl.len, true);
 
+	if (unlikely(ret <= 0))
+		kfree(sgl.sg);
 	return ret;
 }
 
-- 
1.7.5.4

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCHv7 2/4] virtio_console: Use kmalloc instead of kzalloc

[sjur.brandeland]

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

Avoid the more cpu expensive kzalloc when allocating buffers.
Originally kzalloc was intended for isolating the guest from
the host by not sending random guest data to the host. But device
isolation is not yet in place so kzalloc is not really needed.

Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
 drivers/char/virtio_console.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index c36b2f6..301d17e 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -349,7 +349,7 @@ static struct port_buffer *alloc_buf(size_t buf_size)
 	buf = kmalloc(sizeof(*buf), GFP_KERNEL);
 	if (!buf)
 		goto fail;
-	buf->buf = kzalloc(buf_size, GFP_KERNEL);
+	buf->buf = kmalloc(buf_size, GFP_KERNEL);
 	if (!buf->buf)
 		goto free_buf;
 	buf->len = 0;
-- 
1.7.5.4

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCHv7 2/4] virtio_console: Use kmalloc instead of kzalloc

[sjur.brandeland]

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

Avoid the more cpu expensive kzalloc when allocating buffers.
Originally kzalloc was intended for isolating the guest from
the host by not sending random guest data to the host. But device
isolation is not yet in place so kzalloc is not really needed.

Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
 drivers/char/virtio_console.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index c36b2f6..301d17e 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -349,7 +349,7 @@ static struct port_buffer *alloc_buf(size_t buf_size)
 	buf = kmalloc(sizeof(*buf), GFP_KERNEL);
 	if (!buf)
 		goto fail;
-	buf->buf = kzalloc(buf_size, GFP_KERNEL);
+	buf->buf = kmalloc(buf_size, GFP_KERNEL);
 	if (!buf->buf)
 		goto free_buf;
 	buf->len = 0;
-- 
1.7.5.4

Re: [PATCHv7 2/4] virtio_console: Use kmalloc instead of kzalloc

[Rusty Russell]

sjur.brandeland@stericsson.com writes:

> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
>
> Avoid the more cpu expensive kzalloc when allocating buffers.
> Originally kzalloc was intended for isolating the guest from
> the host by not sending random guest data to the host. But device
> isolation is not yet in place so kzalloc is not really needed.
>
> Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>

This looks fine to me.  This is *why* the device gives us the length
which was written; we can trust that, even if we can't trust the
writer of data.

(In theory: noone has implemented such a system, yet).

Applied.
Rusty.

> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index c36b2f6..301d17e 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -349,7 +349,7 @@ static struct port_buffer *alloc_buf(size_t buf_size)
>  	buf = kmalloc(sizeof(*buf), GFP_KERNEL);
>  	if (!buf)
>  		goto fail;
> -	buf->buf = kzalloc(buf_size, GFP_KERNEL);
> +	buf->buf = kmalloc(buf_size, GFP_KERNEL);
>  	if (!buf->buf)
>  		goto free_buf;
>  	buf->len = 0;
> -- 
> 1.7.5.4
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCHv7 2/4] virtio_console: Use kmalloc instead of kzalloc

[Rusty Russell]

sjur.brandeland@stericsson.com writes:

> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
>
> Avoid the more cpu expensive kzalloc when allocating buffers.
> Originally kzalloc was intended for isolating the guest from
> the host by not sending random guest data to the host. But device
> isolation is not yet in place so kzalloc is not really needed.
>
> Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>

This looks fine to me.  This is *why* the device gives us the length
which was written; we can trust that, even if we can't trust the
writer of data.

(In theory: noone has implemented such a system, yet).

Applied.
Rusty.

> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index c36b2f6..301d17e 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -349,7 +349,7 @@ static struct port_buffer *alloc_buf(size_t buf_size)
>  	buf = kmalloc(sizeof(*buf), GFP_KERNEL);
>  	if (!buf)
>  		goto fail;
> -	buf->buf = kzalloc(buf_size, GFP_KERNEL);
> +	buf->buf = kmalloc(buf_size, GFP_KERNEL);
>  	if (!buf->buf)
>  		goto free_buf;
>  	buf->len = 0;
> -- 
> 1.7.5.4

[PATCHv7 3/4] virtio_console: Merge struct buffer_token into struct port_buffer

[sjur.brandeland]

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

Refactoring the splice functionality by unifying the approach for
sending scatter-lists and regular buffers. This simplifies
buffer handling and reduces code size. Splice will now allocate
a port_buffer and send_buf() and free_buf() can always be used
for any buffer.

Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
 drivers/char/virtio_console.c |  131 +++++++++++++++++------------------------
 1 files changed, 55 insertions(+), 76 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 301d17e..917cc830 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -111,6 +111,12 @@ struct port_buffer {
 	size_t len;
 	/* offset in the buf from which to consume data */
 	size_t offset;
+
+	/* If sgpages == 0 then buf is used */
+	unsigned int sgpages;
+
+	/* sg is used if spages > 0. sg must be the last in is struct */
+	struct scatterlist sg[0];
 };
 
 /*
@@ -338,17 +344,39 @@ static inline bool use_multiport(struct ports_device *portdev)
 
 static void free_buf(struct port_buffer *buf)
 {
+	unsigned int i;
+
 	kfree(buf->buf);
+	for (i = 0; i < buf->sgpages; i++) {
+		struct page *page = sg_page(&buf->sg[i]);
+		if (!page)
+			break;
+		put_page(page);
+	}
+
 	kfree(buf);
 }
 
-static struct port_buffer *alloc_buf(size_t buf_size)
+static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size,
+				     int pages)
 {
 	struct port_buffer *buf;
 
-	buf = kmalloc(sizeof(*buf), GFP_KERNEL);
+	/*
+	 * Allocate buffer and the sg list. The sg list array is allocated
+	 * directly after the port_buffer struct.
+	 */
+	buf = kmalloc(sizeof(*buf) + sizeof(struct scatterlist) * pages,
+		      GFP_KERNEL);
 	if (!buf)
 		goto fail;
+
+	buf->sgpages = pages;
+	if (pages > 0) {
+		buf->buf = NULL;
+		return buf;
+	}
+
 	buf->buf = kmalloc(buf_size, GFP_KERNEL);
 	if (!buf->buf)
 		goto free_buf;
@@ -476,52 +504,26 @@ static ssize_t send_control_msg(struct port *port, unsigned int event,
 	return 0;
 }
 
-struct buffer_token {
-	union {
-		void *buf;
-		struct scatterlist *sg;
-	} u;
-	/* If sgpages == 0 then buf is used, else sg is used */
-	unsigned int sgpages;
-};
-
-static void reclaim_sg_pages(struct scatterlist *sg, unsigned int nrpages)
-{
-	int i;
-	struct page *page;
-
-	for (i = 0; i < nrpages; i++) {
-		page = sg_page(&sg[i]);
-		if (!page)
-			break;
-		put_page(page);
-	}
-	kfree(sg);
-}
 
 /* Callers must take the port->outvq_lock */
 static void reclaim_consumed_buffers(struct port *port)
 {
-	struct buffer_token *tok;
+	struct port_buffer *buf;
 	unsigned int len;
 
 	if (!port->portdev) {
 		/* Device has been unplugged.  vqs are already gone. */
 		return;
 	}
-	while ((tok = virtqueue_get_buf(port->out_vq, &len))) {
-		if (tok->sgpages)
-			reclaim_sg_pages(tok->u.sg, tok->sgpages);
-		else
-			kfree(tok->u.buf);
-		kfree(tok);
+	while ((buf = virtqueue_get_buf(port->out_vq, &len))) {
+		free_buf(buf);
 		port->outvq_full = false;
 	}
 }
 
 static ssize_t __send_to_port(struct port *port, struct scatterlist *sg,
 			      int nents, size_t in_count,
-			      struct buffer_token *tok, bool nonblock)
+			      void *data, bool nonblock)
 {
 	struct virtqueue *out_vq;
 	ssize_t ret;
@@ -534,7 +536,7 @@ static ssize_t __send_to_port(struct port *port, struct scatterlist *sg,
 
 	reclaim_consumed_buffers(port);
 
-	ret = virtqueue_add_buf(out_vq, sg, nents, 0, tok, GFP_ATOMIC);
+	ret = virtqueue_add_buf(out_vq, sg, nents, 0, data, GFP_ATOMIC);
 
 	/* Tell Host to go! */
 	virtqueue_kick(out_vq);
@@ -572,37 +574,6 @@ done:
 	return in_count;
 }
 
-static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count,
-			bool nonblock)
-{
-	struct scatterlist sg[1];
-	struct buffer_token *tok;
-
-	tok = kmalloc(sizeof(*tok), GFP_ATOMIC);
-	if (!tok)
-		return -ENOMEM;
-	tok->sgpages = 0;
-	tok->u.buf = in_buf;
-
-	sg_init_one(sg, in_buf, in_count);
-
-	return __send_to_port(port, sg, 1, in_count, tok, nonblock);
-}
-
-static ssize_t send_pages(struct port *port, struct scatterlist *sg, int nents,
-			  size_t in_count, bool nonblock)
-{
-	struct buffer_token *tok;
-
-	tok = kmalloc(sizeof(*tok), GFP_ATOMIC);
-	if (!tok)
-		return -ENOMEM;
-	tok->sgpages = nents;
-	tok->u.sg = sg;
-
-	return __send_to_port(port, sg, nents, in_count, tok, nonblock);
-}
-
 /*
  * Give out the data that's requested from the buffer that we have
  * queued up.
@@ -748,9 +719,10 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
 			       size_t count, loff_t *offp)
 {
 	struct port *port;
-	char *buf;
+	struct port_buffer *buf;
 	ssize_t ret;
 	bool nonblock;
+	struct scatterlist sg[1];
 
 	/* Userspace could be out to fool us */
 	if (!count)
@@ -766,11 +738,11 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
 
 	count = min((size_t)(32 * 1024), count);
 
-	buf = kmalloc(count, GFP_KERNEL);
+	buf = alloc_buf(port->out_vq, count, 0);
 	if (!buf)
 		return -ENOMEM;
 
-	ret = copy_from_user(buf, ubuf, count);
+	ret = copy_from_user(buf->buf, ubuf, count);
 	if (ret) {
 		ret = -EFAULT;
 		goto free_buf;
@@ -784,13 +756,14 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
 	 * through to the host.
 	 */
 	nonblock = true;
-	ret = send_buf(port, buf, count, nonblock);
+	sg_init_one(sg, buf->buf, count);
+	ret = __send_to_port(port, sg, 1, count, buf, nonblock);
 
 	if (nonblock && ret > 0)
 		goto out;
 
 free_buf:
-	kfree(buf);
+	free_buf(buf);
 out:
 	return ret;
 }
@@ -856,6 +829,7 @@ static ssize_t port_fops_splice_write(struct pipe_inode_info *pipe,
 	struct port *port = filp->private_data;
 	struct sg_list sgl;
 	ssize_t ret;
+	struct port_buffer *buf;
 	struct splice_desc sd = {
 		.total_len = len,
 		.flags = flags,
@@ -867,17 +841,18 @@ static ssize_t port_fops_splice_write(struct pipe_inode_info *pipe,
 	if (ret < 0)
 		return ret;
 
+	buf = alloc_buf(port->out_vq, 0, pipe->nrbufs);
+	if (!buf)
+		return -ENOMEM;
+
 	sgl.n = 0;
 	sgl.len = 0;
 	sgl.size = pipe->nrbufs;
-	sgl.sg = kmalloc(sizeof(struct scatterlist) * sgl.size, GFP_KERNEL);
-	if (unlikely(!sgl.sg))
-		return -ENOMEM;
-
+	sgl.sg = buf->sg;
 	sg_init_table(sgl.sg, sgl.size);
 	ret = __splice_from_pipe(pipe, &sd, pipe_to_sg);
 	if (likely(ret > 0))
-		ret = send_pages(port, sgl.sg, sgl.n, sgl.len, true);
+		ret = __send_to_port(port, buf->sg, sgl.n, sgl.len, buf, true);
 
 	if (unlikely(ret <= 0))
 		kfree(sgl.sg);
@@ -1033,6 +1008,8 @@ static const struct file_operations port_fops = {
 static int put_chars(u32 vtermno, const char *buf, int count)
 {
 	struct port *port;
+	struct scatterlist sg[1];
+
 
 	if (unlikely(early_put_chars))
 		return early_put_chars(vtermno, buf, count);
@@ -1041,7 +1018,9 @@ static int put_chars(u32 vtermno, const char *buf, int count)
 	if (!port)
 		return -EPIPE;
 
-	return send_buf(port, (void *)buf, count, false);
+	sg_init_one(sg, buf, count);
+	return __send_to_port(port, sg, 1, count, (void *)buf, false);
+
 }
 
 /*
@@ -1262,7 +1241,7 @@ static unsigned int fill_queue(struct virtqueue *vq, spinlock_t *lock)
 
 	nr_added_bufs = 0;
 	do {
-		buf = alloc_buf(PAGE_SIZE);
+		buf = alloc_buf(vq, PAGE_SIZE, 0);
 		if (!buf)
 			break;
 
-- 
1.7.5.4

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCHv7 3/4] virtio_console: Merge struct buffer_token into struct port_buffer

[sjur.brandeland]

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

Refactoring the splice functionality by unifying the approach for
sending scatter-lists and regular buffers. This simplifies
buffer handling and reduces code size. Splice will now allocate
a port_buffer and send_buf() and free_buf() can always be used
for any buffer.

Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
 drivers/char/virtio_console.c |  131 +++++++++++++++++------------------------
 1 files changed, 55 insertions(+), 76 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 301d17e..917cc830 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -111,6 +111,12 @@ struct port_buffer {
 	size_t len;
 	/* offset in the buf from which to consume data */
 	size_t offset;
+
+	/* If sgpages == 0 then buf is used */
+	unsigned int sgpages;
+
+	/* sg is used if spages > 0. sg must be the last in is struct */
+	struct scatterlist sg[0];
 };
 
 /*
@@ -338,17 +344,39 @@ static inline bool use_multiport(struct ports_device *portdev)
 
 static void free_buf(struct port_buffer *buf)
 {
+	unsigned int i;
+
 	kfree(buf->buf);
+	for (i = 0; i < buf->sgpages; i++) {
+		struct page *page = sg_page(&buf->sg[i]);
+		if (!page)
+			break;
+		put_page(page);
+	}
+
 	kfree(buf);
 }
 
-static struct port_buffer *alloc_buf(size_t buf_size)
+static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size,
+				     int pages)
 {
 	struct port_buffer *buf;
 
-	buf = kmalloc(sizeof(*buf), GFP_KERNEL);
+	/*
+	 * Allocate buffer and the sg list. The sg list array is allocated
+	 * directly after the port_buffer struct.
+	 */
+	buf = kmalloc(sizeof(*buf) + sizeof(struct scatterlist) * pages,
+		      GFP_KERNEL);
 	if (!buf)
 		goto fail;
+
+	buf->sgpages = pages;
+	if (pages > 0) {
+		buf->buf = NULL;
+		return buf;
+	}
+
 	buf->buf = kmalloc(buf_size, GFP_KERNEL);
 	if (!buf->buf)
 		goto free_buf;
@@ -476,52 +504,26 @@ static ssize_t send_control_msg(struct port *port, unsigned int event,
 	return 0;
 }
 
-struct buffer_token {
-	union {
-		void *buf;
-		struct scatterlist *sg;
-	} u;
-	/* If sgpages == 0 then buf is used, else sg is used */
-	unsigned int sgpages;
-};
-
-static void reclaim_sg_pages(struct scatterlist *sg, unsigned int nrpages)
-{
-	int i;
-	struct page *page;
-
-	for (i = 0; i < nrpages; i++) {
-		page = sg_page(&sg[i]);
-		if (!page)
-			break;
-		put_page(page);
-	}
-	kfree(sg);
-}
 
 /* Callers must take the port->outvq_lock */
 static void reclaim_consumed_buffers(struct port *port)
 {
-	struct buffer_token *tok;
+	struct port_buffer *buf;
 	unsigned int len;
 
 	if (!port->portdev) {
 		/* Device has been unplugged.  vqs are already gone. */
 		return;
 	}
-	while ((tok = virtqueue_get_buf(port->out_vq, &len))) {
-		if (tok->sgpages)
-			reclaim_sg_pages(tok->u.sg, tok->sgpages);
-		else
-			kfree(tok->u.buf);
-		kfree(tok);
+	while ((buf = virtqueue_get_buf(port->out_vq, &len))) {
+		free_buf(buf);
 		port->outvq_full = false;
 	}
 }
 
 static ssize_t __send_to_port(struct port *port, struct scatterlist *sg,
 			      int nents, size_t in_count,
-			      struct buffer_token *tok, bool nonblock)
+			      void *data, bool nonblock)
 {
 	struct virtqueue *out_vq;
 	ssize_t ret;
@@ -534,7 +536,7 @@ static ssize_t __send_to_port(struct port *port, struct scatterlist *sg,
 
 	reclaim_consumed_buffers(port);
 
-	ret = virtqueue_add_buf(out_vq, sg, nents, 0, tok, GFP_ATOMIC);
+	ret = virtqueue_add_buf(out_vq, sg, nents, 0, data, GFP_ATOMIC);
 
 	/* Tell Host to go! */
 	virtqueue_kick(out_vq);
@@ -572,37 +574,6 @@ done:
 	return in_count;
 }
 
-static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count,
-			bool nonblock)
-{
-	struct scatterlist sg[1];
-	struct buffer_token *tok;
-
-	tok = kmalloc(sizeof(*tok), GFP_ATOMIC);
-	if (!tok)
-		return -ENOMEM;
-	tok->sgpages = 0;
-	tok->u.buf = in_buf;
-
-	sg_init_one(sg, in_buf, in_count);
-
-	return __send_to_port(port, sg, 1, in_count, tok, nonblock);
-}
-
-static ssize_t send_pages(struct port *port, struct scatterlist *sg, int nents,
-			  size_t in_count, bool nonblock)
-{
-	struct buffer_token *tok;
-
-	tok = kmalloc(sizeof(*tok), GFP_ATOMIC);
-	if (!tok)
-		return -ENOMEM;
-	tok->sgpages = nents;
-	tok->u.sg = sg;
-
-	return __send_to_port(port, sg, nents, in_count, tok, nonblock);
-}
-
 /*
  * Give out the data that's requested from the buffer that we have
  * queued up.
@@ -748,9 +719,10 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
 			       size_t count, loff_t *offp)
 {
 	struct port *port;
-	char *buf;
+	struct port_buffer *buf;
 	ssize_t ret;
 	bool nonblock;
+	struct scatterlist sg[1];
 
 	/* Userspace could be out to fool us */
 	if (!count)
@@ -766,11 +738,11 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
 
 	count = min((size_t)(32 * 1024), count);
 
-	buf = kmalloc(count, GFP_KERNEL);
+	buf = alloc_buf(port->out_vq, count, 0);
 	if (!buf)
 		return -ENOMEM;
 
-	ret = copy_from_user(buf, ubuf, count);
+	ret = copy_from_user(buf->buf, ubuf, count);
 	if (ret) {
 		ret = -EFAULT;
 		goto free_buf;
@@ -784,13 +756,14 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
 	 * through to the host.
 	 */
 	nonblock = true;
-	ret = send_buf(port, buf, count, nonblock);
+	sg_init_one(sg, buf->buf, count);
+	ret = __send_to_port(port, sg, 1, count, buf, nonblock);
 
 	if (nonblock && ret > 0)
 		goto out;
 
 free_buf:
-	kfree(buf);
+	free_buf(buf);
 out:
 	return ret;
 }
@@ -856,6 +829,7 @@ static ssize_t port_fops_splice_write(struct pipe_inode_info *pipe,
 	struct port *port = filp->private_data;
 	struct sg_list sgl;
 	ssize_t ret;
+	struct port_buffer *buf;
 	struct splice_desc sd = {
 		.total_len = len,
 		.flags = flags,
@@ -867,17 +841,18 @@ static ssize_t port_fops_splice_write(struct pipe_inode_info *pipe,
 	if (ret < 0)
 		return ret;
 
+	buf = alloc_buf(port->out_vq, 0, pipe->nrbufs);
+	if (!buf)
+		return -ENOMEM;
+
 	sgl.n = 0;
 	sgl.len = 0;
 	sgl.size = pipe->nrbufs;
-	sgl.sg = kmalloc(sizeof(struct scatterlist) * sgl.size, GFP_KERNEL);
-	if (unlikely(!sgl.sg))
-		return -ENOMEM;
-
+	sgl.sg = buf->sg;
 	sg_init_table(sgl.sg, sgl.size);
 	ret = __splice_from_pipe(pipe, &sd, pipe_to_sg);
 	if (likely(ret > 0))
-		ret = send_pages(port, sgl.sg, sgl.n, sgl.len, true);
+		ret = __send_to_port(port, buf->sg, sgl.n, sgl.len, buf, true);
 
 	if (unlikely(ret <= 0))
 		kfree(sgl.sg);
@@ -1033,6 +1008,8 @@ static const struct file_operations port_fops = {
 static int put_chars(u32 vtermno, const char *buf, int count)
 {
 	struct port *port;
+	struct scatterlist sg[1];
+
 
 	if (unlikely(early_put_chars))
 		return early_put_chars(vtermno, buf, count);
@@ -1041,7 +1018,9 @@ static int put_chars(u32 vtermno, const char *buf, int count)
 	if (!port)
 		return -EPIPE;
 
-	return send_buf(port, (void *)buf, count, false);
+	sg_init_one(sg, buf, count);
+	return __send_to_port(port, sg, 1, count, (void *)buf, false);
+
 }
 
 /*
@@ -1262,7 +1241,7 @@ static unsigned int fill_queue(struct virtqueue *vq, spinlock_t *lock)
 
 	nr_added_bufs = 0;
 	do {
-		buf = alloc_buf(PAGE_SIZE);
+		buf = alloc_buf(vq, PAGE_SIZE, 0);
 		if (!buf)
 			break;
 
-- 
1.7.5.4

Re: [PATCHv7 3/4] virtio_console: Merge struct buffer_token into struct port_buffer

[Rusty Russell]

sjur.brandeland@stericsson.com writes:
> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
>
> Refactoring the splice functionality by unifying the approach for
> sending scatter-lists and regular buffers. This simplifies
> buffer handling and reduces code size. Splice will now allocate
> a port_buffer and send_buf() and free_buf() can always be used
> for any buffer.
>
> Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>

This looks sensible; a couple of extra blank lines inserted though.

Amit?

> @@ -1033,6 +1008,8 @@ static const struct file_operations port_fops = {
>  static int put_chars(u32 vtermno, const char *buf, int count)
>  {
>  	struct port *port;
> +	struct scatterlist sg[1];
> +
>  
>  	if (unlikely(early_put_chars))
>  		return early_put_chars(vtermno, buf, count);
> @@ -1041,7 +1018,9 @@ static int put_chars(u32 vtermno, const char *buf, int count)
>  	if (!port)
>  		return -EPIPE;
>  
> -	return send_buf(port, (void *)buf, count, false);
> +	sg_init_one(sg, buf, count);
> +	return __send_to_port(port, sg, 1, count, (void *)buf, false);
> +
>  }
>  
>  /*

Cheers,
Rusty.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCHv7 3/4] virtio_console: Merge struct buffer_token into struct port_buffer

[Rusty Russell]

sjur.brandeland@stericsson.com writes:
> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
>
> Refactoring the splice functionality by unifying the approach for
> sending scatter-lists and regular buffers. This simplifies
> buffer handling and reduces code size. Splice will now allocate
> a port_buffer and send_buf() and free_buf() can always be used
> for any buffer.
>
> Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>

This looks sensible; a couple of extra blank lines inserted though.

Amit?

> @@ -1033,6 +1008,8 @@ static const struct file_operations port_fops = {
>  static int put_chars(u32 vtermno, const char *buf, int count)
>  {
>  	struct port *port;
> +	struct scatterlist sg[1];
> +
>  
>  	if (unlikely(early_put_chars))
>  		return early_put_chars(vtermno, buf, count);
> @@ -1041,7 +1018,9 @@ static int put_chars(u32 vtermno, const char *buf, int count)
>  	if (!port)
>  		return -EPIPE;
>  
> -	return send_buf(port, (void *)buf, count, false);
> +	sg_init_one(sg, buf, count);
> +	return __send_to_port(port, sg, 1, count, (void *)buf, false);
> +
>  }
>  
>  /*

Cheers,
Rusty.

[PATCHv7 4/4] virtio_console: Add support for remoteproc serial

[sjur.brandeland]

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

Add a simple serial connection driver called
VIRTIO_ID_RPROC_SERIAL (11) for communicating with a
remote processor in an asymmetric multi-processing
configuration.

This implementation reuses the existing virtio_console
implementation, and adds support for DMA allocation
of data buffers and disables use of tty console and
the virtio control queue.

Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
 drivers/char/virtio_console.c |  201 ++++++++++++++++++++++++++++++++++++-----
 include/linux/virtio_ids.h    |    1 +
 2 files changed, 180 insertions(+), 22 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 917cc830..eeb9b35 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -37,8 +37,12 @@
 #include <linux/wait.h>
 #include <linux/workqueue.h>
 #include <linux/module.h>
+#include <linux/dma-mapping.h>
+#include <linux/kconfig.h>
 #include "../tty/hvc/hvc_console.h"
 
+#define is_rproc_enabled IS_ENABLED(CONFIG_REMOTEPROC)
+
 /*
  * This is a global struct for storing common data for all the devices
  * this driver handles.
@@ -112,6 +116,15 @@ struct port_buffer {
 	/* offset in the buf from which to consume data */
 	size_t offset;
 
+	/* DMA address of buffer */
+	dma_addr_t dma;
+
+	/* Device we got DMA memory from */
+	struct device *dev;
+
+	/* List of pending dma buffers to free */
+	struct list_head list;
+
 	/* If sgpages == 0 then buf is used */
 	unsigned int sgpages;
 
@@ -331,6 +344,11 @@ static bool is_console_port(struct port *port)
 	return false;
 }
 
+static bool is_rproc_serial(const struct virtio_device *vdev)
+{
+	return is_rproc_enabled && vdev->id.device == VIRTIO_ID_RPROC_SERIAL;
+}
+
 static inline bool use_multiport(struct ports_device *portdev)
 {
 	/*
@@ -342,11 +360,13 @@ static inline bool use_multiport(struct ports_device *portdev)
 	return portdev->vdev->features[0] & (1 << VIRTIO_CONSOLE_F_MULTIPORT);
 }
 
-static void free_buf(struct port_buffer *buf)
+static DEFINE_SPINLOCK(dma_bufs_lock);
+static LIST_HEAD(pending_free_dma_bufs);
+
+static void free_buf(struct port_buffer *buf, bool can_sleep)
 {
 	unsigned int i;
 
-	kfree(buf->buf);
 	for (i = 0; i < buf->sgpages; i++) {
 		struct page *page = sg_page(&buf->sg[i]);
 		if (!page)
@@ -354,14 +374,58 @@ static void free_buf(struct port_buffer *buf)
 		put_page(page);
 	}
 
+	if (!buf->dev) {
+		kfree(buf->buf);
+	} else if (is_rproc_enabled) {
+		unsigned long flags;
+
+		/* dma_free_coherent requires interrupts to be enabled. */
+		if (!can_sleep) {
+			/* queue up dma-buffers to be freed later */
+			spin_lock_irqsave(&dma_bufs_lock, flags);
+			list_add_tail(&buf->list, &pending_free_dma_bufs);
+			spin_unlock_irqrestore(&dma_bufs_lock, flags);
+			return;
+		}
+		dma_free_coherent(buf->dev, buf->size, buf->buf, buf->dma);
+
+		/* Release device refcnt and allow it to be freed */
+		put_device(buf->dev);
+	}
+
 	kfree(buf);
 }
 
+static void reclaim_dma_bufs(void)
+{
+	unsigned long flags;
+	struct port_buffer *buf, *tmp;
+	LIST_HEAD(tmp_list);
+
+	if (list_empty(&pending_free_dma_bufs))
+		return;
+
+	/* Create a copy of the pending_free_dma_bufs while holding the lock */
+	spin_lock_irqsave(&dma_bufs_lock, flags);
+	list_cut_position(&tmp_list, &pending_free_dma_bufs,
+			  pending_free_dma_bufs.prev);
+	spin_unlock_irqrestore(&dma_bufs_lock, flags);
+
+	/* Release the dma buffers, without irqs enabled */
+	list_for_each_entry_safe(buf, tmp, &tmp_list, list) {
+		list_del(&buf->list);
+		free_buf(buf, true);
+	}
+}
+
 static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size,
 				     int pages)
 {
 	struct port_buffer *buf;
 
+	if (is_rproc_serial(vq->vdev))
+		reclaim_dma_bufs();
+
 	/*
 	 * Allocate buffer and the sg list. The sg list array is allocated
 	 * directly after the port_buffer struct.
@@ -373,11 +437,34 @@ static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size,
 
 	buf->sgpages = pages;
 	if (pages > 0) {
+		buf->dev = NULL;
 		buf->buf = NULL;
 		return buf;
 	}
 
-	buf->buf = kmalloc(buf_size, GFP_KERNEL);
+	if (is_rproc_serial(vq->vdev)) {
+		/*
+		 * Allocate DMA memory from ancestor. When a virtio
+		 * device is created by remoteproc, the DMA memory is
+		 * associated with the grandparent device:
+		 * vdev => rproc => platform-dev.
+		 * The code here would have been less quirky if
+		 * DMA_MEMORY_INCLUDES_CHILDREN had been supported
+		 * in dma-coherent.c
+		 */
+		if (!vq->vdev->dev.parent || !vq->vdev->dev.parent->parent)
+			goto free_buf;
+		buf->dev = vq->vdev->dev.parent->parent;
+
+		/* Increase device refcnt to avoid freeing it */
+		get_device(buf->dev);
+		buf->buf = dma_alloc_coherent(buf->dev, buf_size, &buf->dma,
+					      GFP_KERNEL);
+	} else {
+		buf->dev = NULL;
+		buf->buf = kmalloc(buf_size, GFP_KERNEL);
+	}
+
 	if (!buf->buf)
 		goto free_buf;
 	buf->len = 0;
@@ -444,7 +531,7 @@ static void discard_port_data(struct port *port)
 		port->stats.bytes_discarded += buf->len - buf->offset;
 		if (add_inbuf(port->in_vq, buf) < 0) {
 			err++;
-			free_buf(buf);
+			free_buf(buf, false);
 		}
 		port->inbuf = NULL;
 		buf = get_inbuf(port);
@@ -516,7 +603,7 @@ static void reclaim_consumed_buffers(struct port *port)
 		return;
 	}
 	while ((buf = virtqueue_get_buf(port->out_vq, &len))) {
-		free_buf(buf);
+		free_buf(buf, false);
 		port->outvq_full = false;
 	}
 }
@@ -763,7 +850,7 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
 		goto out;
 
 free_buf:
-	free_buf(buf);
+	free_buf(buf, true);
 out:
 	return ret;
 }
@@ -837,6 +924,15 @@ static ssize_t port_fops_splice_write(struct pipe_inode_info *pipe,
 		.u.data = &sgl,
 	};
 
+	/*
+	 * Rproc_serial does not yet support splice. To support splice
+	 * pipe_to_sg() must allocate dma-buffers and copy content from
+	 * regular pages to dma pages. And alloc_buf and free_buf must
+	 * support allocating and freeing such a list of dma-buffers.
+	 */
+	if (is_rproc_serial(port->out_vq->vdev))
+		return -EINVAL;
+
 	ret = wait_port_writable(port, filp->f_flags & O_NONBLOCK);
 	if (ret < 0)
 		return ret;
@@ -855,7 +951,7 @@ static ssize_t port_fops_splice_write(struct pipe_inode_info *pipe,
 		ret = __send_to_port(port, buf->sg, sgl.n, sgl.len, buf, true);
 
 	if (unlikely(ret <= 0))
-		kfree(sgl.sg);
+		free_buf(buf, true);
 	return ret;
 }
 
@@ -904,6 +1000,8 @@ static int port_fops_release(struct inode *inode, struct file *filp)
 	reclaim_consumed_buffers(port);
 	spin_unlock_irq(&port->outvq_lock);
 
+	if (is_rproc_serial(port->portdev->vdev))
+		reclaim_dma_bufs();
 	/*
 	 * Locks aren't necessary here as a port can't be opened after
 	 * unplug, and if a port isn't unplugged, a kref would already
@@ -1057,7 +1155,10 @@ static void resize_console(struct port *port)
 		return;
 
 	vdev = port->portdev->vdev;
-	if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE))
+
+	/* Don't test F_SIZE at all if we're rproc: not a valid feature! */
+	if (!is_rproc_serial(vdev) &&
+	    virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE))
 		hvc_resize(port->cons.hvc, port->cons.ws);
 }
 
@@ -1249,7 +1350,7 @@ static unsigned int fill_queue(struct virtqueue *vq, spinlock_t *lock)
 		ret = add_inbuf(vq, buf);
 		if (ret < 0) {
 			spin_unlock_irq(lock);
-			free_buf(buf);
+			free_buf(buf, true);
 			break;
 		}
 		nr_added_bufs++;
@@ -1337,10 +1438,18 @@ static int add_port(struct ports_device *portdev, u32 id)
 		goto free_device;
 	}
 
-	/*
-	 * If we're not using multiport support, this has to be a console port
-	 */
-	if (!use_multiport(port->portdev)) {
+	if (is_rproc_serial(port->portdev->vdev))
+		/*
+		 * For rproc_serial assume remote processor is connected.
+		 * rproc_serial does not want the console port, only
+		 * the generic port implementation.
+		 */
+		port->host_connected = true;
+	else if (!use_multiport(port->portdev)) {
+		/*
+		 * If we're not using multiport support,
+		 * this has to be a console port.
+		 */
 		err = init_port_console(port);
 		if (err)
 			goto free_inbufs;
@@ -1373,7 +1482,7 @@ static int add_port(struct ports_device *portdev, u32 id)
 
 free_inbufs:
 	while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
-		free_buf(buf);
+		free_buf(buf, true);
 free_device:
 	device_destroy(pdrvdata.class, port->dev->devt);
 free_cdev:
@@ -1415,7 +1524,16 @@ static void remove_port_data(struct port *port)
 
 	/* Remove buffers we queued up for the Host to send us data in. */
 	while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
-		free_buf(buf);
+		free_buf(buf, true);
+
+	/*
+	 * Remove buffers from out queue for rproc-serial. We cannot afford
+	 * to leak any DMA mem, so reclaim this memory even if this might be
+	 * racy for the remote processor.
+	 */
+	if (is_rproc_serial(port->portdev->vdev))
+		while ((buf = virtqueue_detach_unused_buf(port->out_vq)))
+			free_buf(buf, true);
 }
 
 /*
@@ -1617,7 +1735,7 @@ static void control_work_handler(struct work_struct *work)
 		if (add_inbuf(portdev->c_ivq, buf) < 0) {
 			dev_warn(&portdev->vdev->dev,
 				 "Error adding buffer to queue\n");
-			free_buf(buf);
+			free_buf(buf, false);
 		}
 	}
 	spin_unlock(&portdev->cvq_lock);
@@ -1813,10 +1931,10 @@ static void remove_controlq_data(struct ports_device *portdev)
 		return;
 
 	while ((buf = virtqueue_get_buf(portdev->c_ivq, &len)))
-		free_buf(buf);
+		free_buf(buf, true);
 
 	while ((buf = virtqueue_detach_unused_buf(portdev->c_ivq)))
-		free_buf(buf);
+		free_buf(buf, true);
 }
 
 /*
@@ -1863,11 +1981,15 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
 
 	multiport = false;
 	portdev->config.max_nr_ports = 1;
-	if (virtio_config_val(vdev, VIRTIO_CONSOLE_F_MULTIPORT,
-			      offsetof(struct virtio_console_config,
-				       max_nr_ports),
-			      &portdev->config.max_nr_ports) == 0)
+
+	/* Don't test MULTIPORT at all if we're rproc: not a valid feature! */
+	if (!is_rproc_serial(vdev) &&
+	    virtio_config_val(vdev, VIRTIO_CONSOLE_F_MULTIPORT,
+				  offsetof(struct virtio_console_config,
+					   max_nr_ports),
+				  &portdev->config.max_nr_ports) == 0) {
 		multiport = true;
+	}
 
 	err = init_vqs(portdev);
 	if (err < 0) {
@@ -1977,6 +2099,16 @@ static unsigned int features[] = {
 	VIRTIO_CONSOLE_F_MULTIPORT,
 };
 
+static struct virtio_device_id rproc_serial_id_table[] = {
+#if IS_ENABLED(CONFIG_REMOTEPROC)
+	{ VIRTIO_ID_RPROC_SERIAL, VIRTIO_DEV_ANY_ID },
+#endif
+	{ 0 },
+};
+
+static unsigned int rproc_serial_features[] = {
+};
+
 #ifdef CONFIG_PM
 static int virtcons_freeze(struct virtio_device *vdev)
 {
@@ -2061,6 +2193,20 @@ static struct virtio_driver virtio_console = {
 #endif
 };
 
+/*
+ * virtio_rproc_serial refers to __devinit function which causes
+ * section mismatch warnings. So use __refdata to silence warnings.
+ */
+static struct virtio_driver __refdata virtio_rproc_serial = {
+	.feature_table = rproc_serial_features,
+	.feature_table_size = ARRAY_SIZE(rproc_serial_features),
+	.driver.name =	"virtio_rproc_serial",
+	.driver.owner =	THIS_MODULE,
+	.id_table =	rproc_serial_id_table,
+	.probe =	virtcons_probe,
+	.remove =	virtcons_remove,
+};
+
 static int __init init(void)
 {
 	int err;
@@ -2085,7 +2231,15 @@ static int __init init(void)
 		pr_err("Error %d registering virtio driver\n", err);
 		goto free;
 	}
+	err = register_virtio_driver(&virtio_rproc_serial);
+	if (err < 0) {
+		pr_err("Error %d registering virtio rproc serial driver\n",
+		       err);
+		goto unregister;
+	}
 	return 0;
+unregister:
+	unregister_virtio_driver(&virtio_console);
 free:
 	if (pdrvdata.debugfs_dir)
 		debugfs_remove_recursive(pdrvdata.debugfs_dir);
@@ -2095,7 +2249,10 @@ free:
 
 static void __exit fini(void)
 {
+	reclaim_dma_bufs();
+
 	unregister_virtio_driver(&virtio_console);
+	unregister_virtio_driver(&virtio_rproc_serial);
 
 	class_destroy(pdrvdata.class);
 	if (pdrvdata.debugfs_dir)
diff --git a/include/linux/virtio_ids.h b/include/linux/virtio_ids.h
index 7529b85..9740d33 100644
--- a/include/linux/virtio_ids.h
+++ b/include/linux/virtio_ids.h
@@ -37,5 +37,6 @@
 #define VIRTIO_ID_RPMSG		7 /* virtio remote processor messaging */
 #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 */
 
 #endif /* _LINUX_VIRTIO_IDS_H */
-- 
1.7.5.4

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCHv7 4/4] virtio_console: Add support for remoteproc serial

[sjur.brandeland]

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

Add a simple serial connection driver called
VIRTIO_ID_RPROC_SERIAL (11) for communicating with a
remote processor in an asymmetric multi-processing
configuration.

This implementation reuses the existing virtio_console
implementation, and adds support for DMA allocation
of data buffers and disables use of tty console and
the virtio control queue.

Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
 drivers/char/virtio_console.c |  201 ++++++++++++++++++++++++++++++++++++-----
 include/linux/virtio_ids.h    |    1 +
 2 files changed, 180 insertions(+), 22 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 917cc830..eeb9b35 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -37,8 +37,12 @@
 #include <linux/wait.h>
 #include <linux/workqueue.h>
 #include <linux/module.h>
+#include <linux/dma-mapping.h>
+#include <linux/kconfig.h>
 #include "../tty/hvc/hvc_console.h"
 
+#define is_rproc_enabled IS_ENABLED(CONFIG_REMOTEPROC)
+
 /*
  * This is a global struct for storing common data for all the devices
  * this driver handles.
@@ -112,6 +116,15 @@ struct port_buffer {
 	/* offset in the buf from which to consume data */
 	size_t offset;
 
+	/* DMA address of buffer */
+	dma_addr_t dma;
+
+	/* Device we got DMA memory from */
+	struct device *dev;
+
+	/* List of pending dma buffers to free */
+	struct list_head list;
+
 	/* If sgpages == 0 then buf is used */
 	unsigned int sgpages;
 
@@ -331,6 +344,11 @@ static bool is_console_port(struct port *port)
 	return false;
 }
 
+static bool is_rproc_serial(const struct virtio_device *vdev)
+{
+	return is_rproc_enabled && vdev->id.device == VIRTIO_ID_RPROC_SERIAL;
+}
+
 static inline bool use_multiport(struct ports_device *portdev)
 {
 	/*
@@ -342,11 +360,13 @@ static inline bool use_multiport(struct ports_device *portdev)
 	return portdev->vdev->features[0] & (1 << VIRTIO_CONSOLE_F_MULTIPORT);
 }
 
-static void free_buf(struct port_buffer *buf)
+static DEFINE_SPINLOCK(dma_bufs_lock);
+static LIST_HEAD(pending_free_dma_bufs);
+
+static void free_buf(struct port_buffer *buf, bool can_sleep)
 {
 	unsigned int i;
 
-	kfree(buf->buf);
 	for (i = 0; i < buf->sgpages; i++) {
 		struct page *page = sg_page(&buf->sg[i]);
 		if (!page)
@@ -354,14 +374,58 @@ static void free_buf(struct port_buffer *buf)
 		put_page(page);
 	}
 
+	if (!buf->dev) {
+		kfree(buf->buf);
+	} else if (is_rproc_enabled) {
+		unsigned long flags;
+
+		/* dma_free_coherent requires interrupts to be enabled. */
+		if (!can_sleep) {
+			/* queue up dma-buffers to be freed later */
+			spin_lock_irqsave(&dma_bufs_lock, flags);
+			list_add_tail(&buf->list, &pending_free_dma_bufs);
+			spin_unlock_irqrestore(&dma_bufs_lock, flags);
+			return;
+		}
+		dma_free_coherent(buf->dev, buf->size, buf->buf, buf->dma);
+
+		/* Release device refcnt and allow it to be freed */
+		put_device(buf->dev);
+	}
+
 	kfree(buf);
 }
 
+static void reclaim_dma_bufs(void)
+{
+	unsigned long flags;
+	struct port_buffer *buf, *tmp;
+	LIST_HEAD(tmp_list);
+
+	if (list_empty(&pending_free_dma_bufs))
+		return;
+
+	/* Create a copy of the pending_free_dma_bufs while holding the lock */
+	spin_lock_irqsave(&dma_bufs_lock, flags);
+	list_cut_position(&tmp_list, &pending_free_dma_bufs,
+			  pending_free_dma_bufs.prev);
+	spin_unlock_irqrestore(&dma_bufs_lock, flags);
+
+	/* Release the dma buffers, without irqs enabled */
+	list_for_each_entry_safe(buf, tmp, &tmp_list, list) {
+		list_del(&buf->list);
+		free_buf(buf, true);
+	}
+}
+
 static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size,
 				     int pages)
 {
 	struct port_buffer *buf;
 
+	if (is_rproc_serial(vq->vdev))
+		reclaim_dma_bufs();
+
 	/*
 	 * Allocate buffer and the sg list. The sg list array is allocated
 	 * directly after the port_buffer struct.
@@ -373,11 +437,34 @@ static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size,
 
 	buf->sgpages = pages;
 	if (pages > 0) {
+		buf->dev = NULL;
 		buf->buf = NULL;
 		return buf;
 	}
 
-	buf->buf = kmalloc(buf_size, GFP_KERNEL);
+	if (is_rproc_serial(vq->vdev)) {
+		/*
+		 * Allocate DMA memory from ancestor. When a virtio
+		 * device is created by remoteproc, the DMA memory is
+		 * associated with the grandparent device:
+		 * vdev => rproc => platform-dev.
+		 * The code here would have been less quirky if
+		 * DMA_MEMORY_INCLUDES_CHILDREN had been supported
+		 * in dma-coherent.c
+		 */
+		if (!vq->vdev->dev.parent || !vq->vdev->dev.parent->parent)
+			goto free_buf;
+		buf->dev = vq->vdev->dev.parent->parent;
+
+		/* Increase device refcnt to avoid freeing it */
+		get_device(buf->dev);
+		buf->buf = dma_alloc_coherent(buf->dev, buf_size, &buf->dma,
+					      GFP_KERNEL);
+	} else {
+		buf->dev = NULL;
+		buf->buf = kmalloc(buf_size, GFP_KERNEL);
+	}
+
 	if (!buf->buf)
 		goto free_buf;
 	buf->len = 0;
@@ -444,7 +531,7 @@ static void discard_port_data(struct port *port)
 		port->stats.bytes_discarded += buf->len - buf->offset;
 		if (add_inbuf(port->in_vq, buf) < 0) {
 			err++;
-			free_buf(buf);
+			free_buf(buf, false);
 		}
 		port->inbuf = NULL;
 		buf = get_inbuf(port);
@@ -516,7 +603,7 @@ static void reclaim_consumed_buffers(struct port *port)
 		return;
 	}
 	while ((buf = virtqueue_get_buf(port->out_vq, &len))) {
-		free_buf(buf);
+		free_buf(buf, false);
 		port->outvq_full = false;
 	}
 }
@@ -763,7 +850,7 @@ static ssize_t port_fops_write(struct file *filp, const char __user *ubuf,
 		goto out;
 
 free_buf:
-	free_buf(buf);
+	free_buf(buf, true);
 out:
 	return ret;
 }
@@ -837,6 +924,15 @@ static ssize_t port_fops_splice_write(struct pipe_inode_info *pipe,
 		.u.data = &sgl,
 	};
 
+	/*
+	 * Rproc_serial does not yet support splice. To support splice
+	 * pipe_to_sg() must allocate dma-buffers and copy content from
+	 * regular pages to dma pages. And alloc_buf and free_buf must
+	 * support allocating and freeing such a list of dma-buffers.
+	 */
+	if (is_rproc_serial(port->out_vq->vdev))
+		return -EINVAL;
+
 	ret = wait_port_writable(port, filp->f_flags & O_NONBLOCK);
 	if (ret < 0)
 		return ret;
@@ -855,7 +951,7 @@ static ssize_t port_fops_splice_write(struct pipe_inode_info *pipe,
 		ret = __send_to_port(port, buf->sg, sgl.n, sgl.len, buf, true);
 
 	if (unlikely(ret <= 0))
-		kfree(sgl.sg);
+		free_buf(buf, true);
 	return ret;
 }
 
@@ -904,6 +1000,8 @@ static int port_fops_release(struct inode *inode, struct file *filp)
 	reclaim_consumed_buffers(port);
 	spin_unlock_irq(&port->outvq_lock);
 
+	if (is_rproc_serial(port->portdev->vdev))
+		reclaim_dma_bufs();
 	/*
 	 * Locks aren't necessary here as a port can't be opened after
 	 * unplug, and if a port isn't unplugged, a kref would already
@@ -1057,7 +1155,10 @@ static void resize_console(struct port *port)
 		return;
 
 	vdev = port->portdev->vdev;
-	if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE))
+
+	/* Don't test F_SIZE at all if we're rproc: not a valid feature! */
+	if (!is_rproc_serial(vdev) &&
+	    virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE))
 		hvc_resize(port->cons.hvc, port->cons.ws);
 }
 
@@ -1249,7 +1350,7 @@ static unsigned int fill_queue(struct virtqueue *vq, spinlock_t *lock)
 		ret = add_inbuf(vq, buf);
 		if (ret < 0) {
 			spin_unlock_irq(lock);
-			free_buf(buf);
+			free_buf(buf, true);
 			break;
 		}
 		nr_added_bufs++;
@@ -1337,10 +1438,18 @@ static int add_port(struct ports_device *portdev, u32 id)
 		goto free_device;
 	}
 
-	/*
-	 * If we're not using multiport support, this has to be a console port
-	 */
-	if (!use_multiport(port->portdev)) {
+	if (is_rproc_serial(port->portdev->vdev))
+		/*
+		 * For rproc_serial assume remote processor is connected.
+		 * rproc_serial does not want the console port, only
+		 * the generic port implementation.
+		 */
+		port->host_connected = true;
+	else if (!use_multiport(port->portdev)) {
+		/*
+		 * If we're not using multiport support,
+		 * this has to be a console port.
+		 */
 		err = init_port_console(port);
 		if (err)
 			goto free_inbufs;
@@ -1373,7 +1482,7 @@ static int add_port(struct ports_device *portdev, u32 id)
 
 free_inbufs:
 	while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
-		free_buf(buf);
+		free_buf(buf, true);
 free_device:
 	device_destroy(pdrvdata.class, port->dev->devt);
 free_cdev:
@@ -1415,7 +1524,16 @@ static void remove_port_data(struct port *port)
 
 	/* Remove buffers we queued up for the Host to send us data in. */
 	while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
-		free_buf(buf);
+		free_buf(buf, true);
+
+	/*
+	 * Remove buffers from out queue for rproc-serial. We cannot afford
+	 * to leak any DMA mem, so reclaim this memory even if this might be
+	 * racy for the remote processor.
+	 */
+	if (is_rproc_serial(port->portdev->vdev))
+		while ((buf = virtqueue_detach_unused_buf(port->out_vq)))
+			free_buf(buf, true);
 }
 
 /*
@@ -1617,7 +1735,7 @@ static void control_work_handler(struct work_struct *work)
 		if (add_inbuf(portdev->c_ivq, buf) < 0) {
 			dev_warn(&portdev->vdev->dev,
 				 "Error adding buffer to queue\n");
-			free_buf(buf);
+			free_buf(buf, false);
 		}
 	}
 	spin_unlock(&portdev->cvq_lock);
@@ -1813,10 +1931,10 @@ static void remove_controlq_data(struct ports_device *portdev)
 		return;
 
 	while ((buf = virtqueue_get_buf(portdev->c_ivq, &len)))
-		free_buf(buf);
+		free_buf(buf, true);
 
 	while ((buf = virtqueue_detach_unused_buf(portdev->c_ivq)))
-		free_buf(buf);
+		free_buf(buf, true);
 }
 
 /*
@@ -1863,11 +1981,15 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
 
 	multiport = false;
 	portdev->config.max_nr_ports = 1;
-	if (virtio_config_val(vdev, VIRTIO_CONSOLE_F_MULTIPORT,
-			      offsetof(struct virtio_console_config,
-				       max_nr_ports),
-			      &portdev->config.max_nr_ports) == 0)
+
+	/* Don't test MULTIPORT at all if we're rproc: not a valid feature! */
+	if (!is_rproc_serial(vdev) &&
+	    virtio_config_val(vdev, VIRTIO_CONSOLE_F_MULTIPORT,
+				  offsetof(struct virtio_console_config,
+					   max_nr_ports),
+				  &portdev->config.max_nr_ports) == 0) {
 		multiport = true;
+	}
 
 	err = init_vqs(portdev);
 	if (err < 0) {
@@ -1977,6 +2099,16 @@ static unsigned int features[] = {
 	VIRTIO_CONSOLE_F_MULTIPORT,
 };
 
+static struct virtio_device_id rproc_serial_id_table[] = {
+#if IS_ENABLED(CONFIG_REMOTEPROC)
+	{ VIRTIO_ID_RPROC_SERIAL, VIRTIO_DEV_ANY_ID },
+#endif
+	{ 0 },
+};
+
+static unsigned int rproc_serial_features[] = {
+};
+
 #ifdef CONFIG_PM
 static int virtcons_freeze(struct virtio_device *vdev)
 {
@@ -2061,6 +2193,20 @@ static struct virtio_driver virtio_console = {
 #endif
 };
 
+/*
+ * virtio_rproc_serial refers to __devinit function which causes
+ * section mismatch warnings. So use __refdata to silence warnings.
+ */
+static struct virtio_driver __refdata virtio_rproc_serial = {
+	.feature_table = rproc_serial_features,
+	.feature_table_size = ARRAY_SIZE(rproc_serial_features),
+	.driver.name =	"virtio_rproc_serial",
+	.driver.owner =	THIS_MODULE,
+	.id_table =	rproc_serial_id_table,
+	.probe =	virtcons_probe,
+	.remove =	virtcons_remove,
+};
+
 static int __init init(void)
 {
 	int err;
@@ -2085,7 +2231,15 @@ static int __init init(void)
 		pr_err("Error %d registering virtio driver\n", err);
 		goto free;
 	}
+	err = register_virtio_driver(&virtio_rproc_serial);
+	if (err < 0) {
+		pr_err("Error %d registering virtio rproc serial driver\n",
+		       err);
+		goto unregister;
+	}
 	return 0;
+unregister:
+	unregister_virtio_driver(&virtio_console);
 free:
 	if (pdrvdata.debugfs_dir)
 		debugfs_remove_recursive(pdrvdata.debugfs_dir);
@@ -2095,7 +2249,10 @@ free:
 
 static void __exit fini(void)
 {
+	reclaim_dma_bufs();
+
 	unregister_virtio_driver(&virtio_console);
+	unregister_virtio_driver(&virtio_rproc_serial);
 
 	class_destroy(pdrvdata.class);
 	if (pdrvdata.debugfs_dir)
diff --git a/include/linux/virtio_ids.h b/include/linux/virtio_ids.h
index 7529b85..9740d33 100644
--- a/include/linux/virtio_ids.h
+++ b/include/linux/virtio_ids.h
@@ -37,5 +37,6 @@
 #define VIRTIO_ID_RPMSG		7 /* virtio remote processor messaging */
 #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 */
 
 #endif /* _LINUX_VIRTIO_IDS_H */
-- 
1.7.5.4

Re: [PATCHv7 4/4] virtio_console: Add support for remoteproc serial

[Rusty Russell]

sjur.brandeland@stericsson.com writes:
> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
>
> Add a simple serial connection driver called
> VIRTIO_ID_RPROC_SERIAL (11) for communicating with a
> remote processor in an asymmetric multi-processing
> configuration.
>
> This implementation reuses the existing virtio_console
> implementation, and adds support for DMA allocation
> of data buffers and disables use of tty console and
> the virtio control queue.
>
> Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>

The free-outside-interrupt issue is usually dealt with by offloading to
a wq, but your variant works (and isn't too ugly).

> +		/* dma_free_coherent requires interrupts to be enabled. */
> +		if (!can_sleep) {
> +			/* queue up dma-buffers to be freed later */
> +			spin_lock_irqsave(&dma_bufs_lock, flags);
> +			list_add_tail(&buf->list, &pending_free_dma_bufs);
> +			spin_unlock_irqrestore(&dma_bufs_lock, flags);
> +			return;
> +		}
> +		dma_free_coherent(buf->dev, buf->size, buf->buf, buf->dma);
> +
> +		/* Release device refcnt and allow it to be freed */
> +		put_device(buf->dev);

...

> +static void reclaim_dma_bufs(void)
> +{
> +	unsigned long flags;
> +	struct port_buffer *buf, *tmp;
> +	LIST_HEAD(tmp_list);
> +
> +	if (list_empty(&pending_free_dma_bufs))
> +		return;
> +
> +	/* Create a copy of the pending_free_dma_bufs while holding the lock */
> +	spin_lock_irqsave(&dma_bufs_lock, flags);
> +	list_cut_position(&tmp_list, &pending_free_dma_bufs,
> +			  pending_free_dma_bufs.prev);
> +	spin_unlock_irqrestore(&dma_bufs_lock, flags);
> +
> +	/* Release the dma buffers, without irqs enabled */
> +	list_for_each_entry_safe(buf, tmp, &tmp_list, list) {
> +		list_del(&buf->list);
> +		free_buf(buf, true);
> +	}
> +}

Looks like this should be an easy noop even if !is_rproc_serial.

> +
>  static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size,
>  				     int pages)
>  {
>  	struct port_buffer *buf;
>  
> +	if (is_rproc_serial(vq->vdev))
> +		reclaim_dma_bufs();
> +

...

> @@ -904,6 +1000,8 @@ static int port_fops_release(struct inode *inode, struct file *filp)
>  	reclaim_consumed_buffers(port);
>  	spin_unlock_irq(&port->outvq_lock);
>  
> +	if (is_rproc_serial(port->portdev->vdev))
> +		reclaim_dma_bufs();

So these are redundant.

> @@ -1415,7 +1524,16 @@ static void remove_port_data(struct port *port)
>  
>  	/* Remove buffers we queued up for the Host to send us data in. */
>  	while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
> -		free_buf(buf);
> +		free_buf(buf, true);
> +
> +	/*
> +	 * Remove buffers from out queue for rproc-serial. We cannot afford
> +	 * to leak any DMA mem, so reclaim this memory even if this might be
> +	 * racy for the remote processor.
> +	 */
> +	if (is_rproc_serial(port->portdev->vdev))
> +		while ((buf = virtqueue_detach_unused_buf(port->out_vq)))
> +			free_buf(buf, true);
>  }

This seems wrong; either this is needed even if !is_rproc_serial(), or
it's not necessary as the out_vq is empty.

Every path I can see has the device reset (in which case the queues
should not be active), or we got a VIRTIO_CONSOLE_PORT_REMOVE event (in
which case, the same).

I think we can have non-blocking writes which could leave buffers in
out_vq: Amit?
>  static void __exit fini(void)
>  {
> +	reclaim_dma_bufs();

Hmm, you didn't protect it here anyway...

Cheers,
Rusty.

Re: [PATCHv7 4/4] virtio_console: Add support for remoteproc serial

[Sjur Brændeland]

Hi Rusty,

> The free-outside-interrupt issue is usually dealt with by offloading to
> a wq, but your variant works (and isn't too ugly).

Ok, thanks.

>> +static void reclaim_dma_bufs(void)
>> +{
>> +     unsigned long flags;
>> +     struct port_buffer *buf, *tmp;
>> +     LIST_HEAD(tmp_list);
>> +
>> +     if (list_empty(&pending_free_dma_bufs))
>> +             return;
...
> Looks like this should be an easy noop even if !is_rproc_serial.

Ok,  I'll drop the superfluous check before calling reclaim_dma_bufs().

>> @@ -1415,7 +1524,16 @@ static void remove_port_data(struct port *port)
>>
>>       /* Remove buffers we queued up for the Host to send us data in. */
>>       while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
>> -             free_buf(buf);
>> +             free_buf(buf, true);
>> +
>> +     /*
>> +      * Remove buffers from out queue for rproc-serial. We cannot afford
>> +      * to leak any DMA mem, so reclaim this memory even if this might be
>> +      * racy for the remote processor.
>> +      */
>> +     if (is_rproc_serial(port->portdev->vdev))
>> +             while ((buf = virtqueue_detach_unused_buf(port->out_vq)))
>> +                     free_buf(buf, true);
>>  }
>
> This seems wrong; either this is needed even if !is_rproc_serial(), or
> it's not necessary as the out_vq is empty.
>
> Every path I can see has the device reset (in which case the queues
> should not be active), or we got a VIRTIO_CONSOLE_PORT_REMOVE event (in
> which case, the same).
>
> I think we can have non-blocking writes which could leave buffers in
> out_vq: Amit?

Hm, the remote device could potentially freeze whenever. So I think we
should handle the situation where buffer are stuck in the out-queue for
the rproc_serial device. But I'll move this piece of code into a new
follow-up patch so we can handle this issue separately.

Thanks,
Sjur

Re: [PATCHv7 4/4] virtio_console: Add support for remoteproc serial

[Sjur Brændeland]

Hi Rusty,

> The free-outside-interrupt issue is usually dealt with by offloading to
> a wq, but your variant works (and isn't too ugly).

Ok, thanks.

>> +static void reclaim_dma_bufs(void)
>> +{
>> +     unsigned long flags;
>> +     struct port_buffer *buf, *tmp;
>> +     LIST_HEAD(tmp_list);
>> +
>> +     if (list_empty(&pending_free_dma_bufs))
>> +             return;
...
> Looks like this should be an easy noop even if !is_rproc_serial.

Ok,  I'll drop the superfluous check before calling reclaim_dma_bufs().

>> @@ -1415,7 +1524,16 @@ static void remove_port_data(struct port *port)
>>
>>       /* Remove buffers we queued up for the Host to send us data in. */
>>       while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
>> -             free_buf(buf);
>> +             free_buf(buf, true);
>> +
>> +     /*
>> +      * Remove buffers from out queue for rproc-serial. We cannot afford
>> +      * to leak any DMA mem, so reclaim this memory even if this might be
>> +      * racy for the remote processor.
>> +      */
>> +     if (is_rproc_serial(port->portdev->vdev))
>> +             while ((buf = virtqueue_detach_unused_buf(port->out_vq)))
>> +                     free_buf(buf, true);
>>  }
>
> This seems wrong; either this is needed even if !is_rproc_serial(), or
> it's not necessary as the out_vq is empty.
>
> Every path I can see has the device reset (in which case the queues
> should not be active), or we got a VIRTIO_CONSOLE_PORT_REMOVE event (in
> which case, the same).
>
> I think we can have non-blocking writes which could leave buffers in
> out_vq: Amit?

Hm, the remote device could potentially freeze whenever. So I think we
should handle the situation where buffer are stuck in the out-queue for
the rproc_serial device. But I'll move this piece of code into a new
follow-up patch so we can handle this issue separately.

Thanks,
Sjur

Re: [PATCHv7 4/4] virtio_console: Add support for remoteproc serial

[Amit Shah]

On (Tue) 23 Oct 2012 [12:17:49], Rusty Russell wrote:
> sjur.brandeland@stericsson.com writes:
> > From: Sjur Brændeland <sjur.brandeland@stericsson.com>

> > @@ -1415,7 +1524,16 @@ static void remove_port_data(struct port *port)
> >  
> >  	/* Remove buffers we queued up for the Host to send us data in. */
> >  	while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
> > -		free_buf(buf);
> > +		free_buf(buf, true);
> > +
> > +	/*
> > +	 * Remove buffers from out queue for rproc-serial. We cannot afford
> > +	 * to leak any DMA mem, so reclaim this memory even if this might be
> > +	 * racy for the remote processor.
> > +	 */
> > +	if (is_rproc_serial(port->portdev->vdev))
> > +		while ((buf = virtqueue_detach_unused_buf(port->out_vq)))
> > +			free_buf(buf, true);
> >  }
> 
> This seems wrong; either this is needed even if !is_rproc_serial(), or
> it's not necessary as the out_vq is empty.
> 
> Every path I can see has the device reset (in which case the queues
> should not be active), or we got a VIRTIO_CONSOLE_PORT_REMOVE event (in
> which case, the same).
> 
> I think we can have non-blocking writes which could leave buffers in
> out_vq: Amit?

Those get 'reclaimed' just above this hunk:


static void remove_port_data(struct port *port)
{
	struct port_buffer *buf;

	/* Remove unused data this port might have received. */
	discard_port_data(port);

	reclaim_consumed_buffers(port);

	/* Remove buffers we queued up for the Host to send us data in. */
	while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
	      free_buf(buf, true);

  ...




> >  static void __exit fini(void)
> >  {
> > +	reclaim_dma_bufs();
> 
> Hmm, you didn't protect it here anyway...
> 
> Cheers,
> Rusty.

		Amit

Re: [PATCHv7 4/4] virtio_console: Add support for remoteproc serial

[Amit Shah]

On (Tue) 23 Oct 2012 [12:17:49], Rusty Russell wrote:
> sjur.brandeland@stericsson.com writes:
> > From: Sjur Brændeland <sjur.brandeland@stericsson.com>

> > @@ -1415,7 +1524,16 @@ static void remove_port_data(struct port *port)
> >  
> >  	/* Remove buffers we queued up for the Host to send us data in. */
> >  	while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
> > -		free_buf(buf);
> > +		free_buf(buf, true);
> > +
> > +	/*
> > +	 * Remove buffers from out queue for rproc-serial. We cannot afford
> > +	 * to leak any DMA mem, so reclaim this memory even if this might be
> > +	 * racy for the remote processor.
> > +	 */
> > +	if (is_rproc_serial(port->portdev->vdev))
> > +		while ((buf = virtqueue_detach_unused_buf(port->out_vq)))
> > +			free_buf(buf, true);
> >  }
> 
> This seems wrong; either this is needed even if !is_rproc_serial(), or
> it's not necessary as the out_vq is empty.
> 
> Every path I can see has the device reset (in which case the queues
> should not be active), or we got a VIRTIO_CONSOLE_PORT_REMOVE event (in
> which case, the same).
> 
> I think we can have non-blocking writes which could leave buffers in
> out_vq: Amit?

Those get 'reclaimed' just above this hunk:


static void remove_port_data(struct port *port)
{
	struct port_buffer *buf;

	/* Remove unused data this port might have received. */
	discard_port_data(port);

	reclaim_consumed_buffers(port);

	/* Remove buffers we queued up for the Host to send us data in. */
	while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
	      free_buf(buf, true);

  ...




> >  static void __exit fini(void)
> >  {
> > +	reclaim_dma_bufs();
> 
> Hmm, you didn't protect it here anyway...
> 
> Cheers,
> Rusty.

		Amit

Re: [PATCHv7 4/4] virtio_console: Add support for remoteproc serial

[Rusty Russell]

Amit Shah <amit.shah@redhat.com> writes:
> On (Tue) 23 Oct 2012 [12:17:49], Rusty Russell wrote:
>> sjur.brandeland@stericsson.com writes:
>> > From: Sjur Brændeland <sjur.brandeland@stericsson.com>
>
>> > @@ -1415,7 +1524,16 @@ static void remove_port_data(struct port *port)
>> >  
>> >  	/* Remove buffers we queued up for the Host to send us data in. */
>> >  	while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
>> > -		free_buf(buf);
>> > +		free_buf(buf, true);
>> > +
>> > +	/*
>> > +	 * Remove buffers from out queue for rproc-serial. We cannot afford
>> > +	 * to leak any DMA mem, so reclaim this memory even if this might be
>> > +	 * racy for the remote processor.
>> > +	 */
>> > +	if (is_rproc_serial(port->portdev->vdev))
>> > +		while ((buf = virtqueue_detach_unused_buf(port->out_vq)))
>> > +			free_buf(buf, true);
>> >  }
>> 
>> This seems wrong; either this is needed even if !is_rproc_serial(), or
>> it's not necessary as the out_vq is empty.
>> 
>> Every path I can see has the device reset (in which case the queues
>> should not be active), or we got a VIRTIO_CONSOLE_PORT_REMOVE event (in
>> which case, the same).
>> 
>> I think we can have non-blocking writes which could leave buffers in
>> out_vq: Amit?
>
> Those get 'reclaimed' just above this hunk:
>
>
> static void remove_port_data(struct port *port)
> {
> 	struct port_buffer *buf;
>
> 	/* Remove unused data this port might have received. */
> 	discard_port_data(port);
>
> 	reclaim_consumed_buffers(port);
>
> 	/* Remove buffers we queued up for the Host to send us data in. */
> 	while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
> 	      free_buf(buf, true);

No, that's pending input buffers, not output buffers.

Cheers,
Rusty.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCHv7 4/4] virtio_console: Add support for remoteproc serial

[Rusty Russell]

Amit Shah <amit.shah@redhat.com> writes:
> On (Tue) 23 Oct 2012 [12:17:49], Rusty Russell wrote:
>> sjur.brandeland@stericsson.com writes:
>> > From: Sjur Brændeland <sjur.brandeland@stericsson.com>
>
>> > @@ -1415,7 +1524,16 @@ static void remove_port_data(struct port *port)
>> >  
>> >  	/* Remove buffers we queued up for the Host to send us data in. */
>> >  	while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
>> > -		free_buf(buf);
>> > +		free_buf(buf, true);
>> > +
>> > +	/*
>> > +	 * Remove buffers from out queue for rproc-serial. We cannot afford
>> > +	 * to leak any DMA mem, so reclaim this memory even if this might be
>> > +	 * racy for the remote processor.
>> > +	 */
>> > +	if (is_rproc_serial(port->portdev->vdev))
>> > +		while ((buf = virtqueue_detach_unused_buf(port->out_vq)))
>> > +			free_buf(buf, true);
>> >  }
>> 
>> This seems wrong; either this is needed even if !is_rproc_serial(), or
>> it's not necessary as the out_vq is empty.
>> 
>> Every path I can see has the device reset (in which case the queues
>> should not be active), or we got a VIRTIO_CONSOLE_PORT_REMOVE event (in
>> which case, the same).
>> 
>> I think we can have non-blocking writes which could leave buffers in
>> out_vq: Amit?
>
> Those get 'reclaimed' just above this hunk:
>
>
> static void remove_port_data(struct port *port)
> {
> 	struct port_buffer *buf;
>
> 	/* Remove unused data this port might have received. */
> 	discard_port_data(port);
>
> 	reclaim_consumed_buffers(port);
>
> 	/* Remove buffers we queued up for the Host to send us data in. */
> 	while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
> 	      free_buf(buf, true);

No, that's pending input buffers, not output buffers.

Cheers,
Rusty.

Re: [PATCHv7 4/4] virtio_console: Add support for remoteproc serial

[Amit Shah]

On (Fri) 02 Nov 2012 [09:52:08], Rusty Russell wrote:
> Amit Shah <amit.shah@redhat.com> writes:
> > On (Tue) 23 Oct 2012 [12:17:49], Rusty Russell wrote:
> >> sjur.brandeland@stericsson.com writes:
> >> > From: Sjur Brændeland <sjur.brandeland@stericsson.com>
> >
> >> > @@ -1415,7 +1524,16 @@ static void remove_port_data(struct port *port)
> >> >  
> >> >  	/* Remove buffers we queued up for the Host to send us data in. */
> >> >  	while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
> >> > -		free_buf(buf);
> >> > +		free_buf(buf, true);
> >> > +
> >> > +	/*
> >> > +	 * Remove buffers from out queue for rproc-serial. We cannot afford
> >> > +	 * to leak any DMA mem, so reclaim this memory even if this might be
> >> > +	 * racy for the remote processor.
> >> > +	 */
> >> > +	if (is_rproc_serial(port->portdev->vdev))
> >> > +		while ((buf = virtqueue_detach_unused_buf(port->out_vq)))
> >> > +			free_buf(buf, true);
> >> >  }
> >> 
> >> This seems wrong; either this is needed even if !is_rproc_serial(), or
> >> it's not necessary as the out_vq is empty.
> >> 
> >> Every path I can see has the device reset (in which case the queues
> >> should not be active), or we got a VIRTIO_CONSOLE_PORT_REMOVE event (in
> >> which case, the same).
> >> 
> >> I think we can have non-blocking writes which could leave buffers in
> >> out_vq: Amit?
> >
> > Those get 'reclaimed' just above this hunk:
> >
> >
> > static void remove_port_data(struct port *port)
> > {
> > 	struct port_buffer *buf;
> >
> > 	/* Remove unused data this port might have received. */
> > 	discard_port_data(port);
> >
> > 	reclaim_consumed_buffers(port);
> >
> > 	/* Remove buffers we queued up for the Host to send us data in. */
> > 	while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
> > 	      free_buf(buf, true);
> 
> No, that's pending input buffers, not output buffers.

You're right.  Nice catch.

Sjur, can you remove the WARN_ON in your latest series, even generic
ports may have buffers in the outq.

It'll also need to be done in the port_fops_release() function.  Let
me know if you prefer I send a patch instead.

Thanks,
		Amit

Re: [PATCHv7 4/4] virtio_console: Add support for remoteproc serial

[Amit Shah]

On (Fri) 02 Nov 2012 [09:52:08], Rusty Russell wrote:
> Amit Shah <amit.shah@redhat.com> writes:
> > On (Tue) 23 Oct 2012 [12:17:49], Rusty Russell wrote:
> >> sjur.brandeland@stericsson.com writes:
> >> > From: Sjur Brændeland <sjur.brandeland@stericsson.com>
> >
> >> > @@ -1415,7 +1524,16 @@ static void remove_port_data(struct port *port)
> >> >  
> >> >  	/* Remove buffers we queued up for the Host to send us data in. */
> >> >  	while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
> >> > -		free_buf(buf);
> >> > +		free_buf(buf, true);
> >> > +
> >> > +	/*
> >> > +	 * Remove buffers from out queue for rproc-serial. We cannot afford
> >> > +	 * to leak any DMA mem, so reclaim this memory even if this might be
> >> > +	 * racy for the remote processor.
> >> > +	 */
> >> > +	if (is_rproc_serial(port->portdev->vdev))
> >> > +		while ((buf = virtqueue_detach_unused_buf(port->out_vq)))
> >> > +			free_buf(buf, true);
> >> >  }
> >> 
> >> This seems wrong; either this is needed even if !is_rproc_serial(), or
> >> it's not necessary as the out_vq is empty.
> >> 
> >> Every path I can see has the device reset (in which case the queues
> >> should not be active), or we got a VIRTIO_CONSOLE_PORT_REMOVE event (in
> >> which case, the same).
> >> 
> >> I think we can have non-blocking writes which could leave buffers in
> >> out_vq: Amit?
> >
> > Those get 'reclaimed' just above this hunk:
> >
> >
> > static void remove_port_data(struct port *port)
> > {
> > 	struct port_buffer *buf;
> >
> > 	/* Remove unused data this port might have received. */
> > 	discard_port_data(port);
> >
> > 	reclaim_consumed_buffers(port);
> >
> > 	/* Remove buffers we queued up for the Host to send us data in. */
> > 	while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
> > 	      free_buf(buf, true);
> 
> No, that's pending input buffers, not output buffers.

You're right.  Nice catch.

Sjur, can you remove the WARN_ON in your latest series, even generic
ports may have buffers in the outq.

It'll also need to be done in the port_fops_release() function.  Let
me know if you prefer I send a patch instead.

Thanks,
		Amit

RE: [PATCHv7 4/4] virtio_console: Add support for remoteproc serial

[Sjur BRENDELAND]

> On (Fri) 02 Nov 2012 [09:52:08], Rusty Russell wrote:
> > Amit Shah <amit.shah@redhat.com> writes:
> > > On (Tue) 23 Oct 2012 [12:17:49], Rusty Russell wrote:
> > >> sjur.brandeland@stericsson.com writes:
> > >> > From: Sjur Brændeland <sjur.brandeland@stericsson.com>
> > >
> > >> > @@ -1415,7 +1524,16 @@ static void remove_port_data(struct port
> *port)
> > >> >
> > >> >  	/* Remove buffers we queued up for the Host to send us data in. */
> > >> >  	while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
> > >> > -		free_buf(buf);
> > >> > +		free_buf(buf, true);
> > >> > +
> > >> > +	/*
> > >> > +	 * Remove buffers from out queue for rproc-serial. We
> cannot afford
> > >> > +	 * to leak any DMA mem, so reclaim this memory even if this
> might be
> > >> > +	 * racy for the remote processor.
> > >> > +	 */
> > >> > +	if (is_rproc_serial(port->portdev->vdev))
> > >> > +		while ((buf = virtqueue_detach_unused_buf(port-
> >out_vq)))
> > >> > +			free_buf(buf, true);
> > >> >  }
> > >>
> > >> This seems wrong; either this is needed even if !is_rproc_serial(), or
> > >> it's not necessary as the out_vq is empty.
> > >>
> > >> Every path I can see has the device reset (in which case the queues
> > >> should not be active), or we got a VIRTIO_CONSOLE_PORT_REMOVE
> event (in
> > >> which case, the same).
> > >>
> > >> I think we can have non-blocking writes which could leave buffers in
> > >> out_vq: Amit?
> > >
> > > Those get 'reclaimed' just above this hunk:
> > >
> > >
> > > static void remove_port_data(struct port *port)
> > > {
> > > 	struct port_buffer *buf;
> > >
> > > 	/* Remove unused data this port might have received. */
> > > 	discard_port_data(port);
> > >
> > > 	reclaim_consumed_buffers(port);
> > >
> > > 	/* Remove buffers we queued up for the Host to send us data in. */
> > > 	while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
> > > 	      free_buf(buf, true);
> >
> > No, that's pending input buffers, not output buffers.
> 
> You're right.  Nice catch.
> 
> Sjur, can you remove the WARN_ON in your latest series, even generic
> ports may have buffers in the outq.

Sure, I can respin the patch and remove the WARN_ON().

> It'll also need to be done in the port_fops_release() function.  Let
> me know if you prefer I send a patch instead.

I can have a go at this, as I have to respin the patch anyway.

Thanks,
Sjur

RE: [PATCHv7 4/4] virtio_console: Add support for remoteproc serial

[Sjur BRENDELAND]

> On (Fri) 02 Nov 2012 [09:52:08], Rusty Russell wrote:
> > Amit Shah <amit.shah@redhat.com> writes:
> > > On (Tue) 23 Oct 2012 [12:17:49], Rusty Russell wrote:
> > >> sjur.brandeland@stericsson.com writes:
> > >> > From: Sjur Brændeland <sjur.brandeland@stericsson.com>
> > >
> > >> > @@ -1415,7 +1524,16 @@ static void remove_port_data(struct port
> *port)
> > >> >
> > >> >  	/* Remove buffers we queued up for the Host to send us data in. */
> > >> >  	while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
> > >> > -		free_buf(buf);
> > >> > +		free_buf(buf, true);
> > >> > +
> > >> > +	/*
> > >> > +	 * Remove buffers from out queue for rproc-serial. We
> cannot afford
> > >> > +	 * to leak any DMA mem, so reclaim this memory even if this
> might be
> > >> > +	 * racy for the remote processor.
> > >> > +	 */
> > >> > +	if (is_rproc_serial(port->portdev->vdev))
> > >> > +		while ((buf = virtqueue_detach_unused_buf(port-
> >out_vq)))
> > >> > +			free_buf(buf, true);
> > >> >  }
> > >>
> > >> This seems wrong; either this is needed even if !is_rproc_serial(), or
> > >> it's not necessary as the out_vq is empty.
> > >>
> > >> Every path I can see has the device reset (in which case the queues
> > >> should not be active), or we got a VIRTIO_CONSOLE_PORT_REMOVE
> event (in
> > >> which case, the same).
> > >>
> > >> I think we can have non-blocking writes which could leave buffers in
> > >> out_vq: Amit?
> > >
> > > Those get 'reclaimed' just above this hunk:
> > >
> > >
> > > static void remove_port_data(struct port *port)
> > > {
> > > 	struct port_buffer *buf;
> > >
> > > 	/* Remove unused data this port might have received. */
> > > 	discard_port_data(port);
> > >
> > > 	reclaim_consumed_buffers(port);
> > >
> > > 	/* Remove buffers we queued up for the Host to send us data in. */
> > > 	while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
> > > 	      free_buf(buf, true);
> >
> > No, that's pending input buffers, not output buffers.
> 
> You're right.  Nice catch.
> 
> Sjur, can you remove the WARN_ON in your latest series, even generic
> ports may have buffers in the outq.

Sure, I can respin the patch and remove the WARN_ON().

> It'll also need to be done in the port_fops_release() function.  Let
> me know if you prefer I send a patch instead.

I can have a go at this, as I have to respin the patch anyway.

Thanks,
Sjur

[PATCH resend] virtio_console: Free buffers from out-queue upon close

[sjur.brandeland]

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

Free pending output buffers from the virtio out-queue when
host has acknowledged port_close. Also removed WARN_ON()
in remove_port_data().

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

Resending, this time including a proper "Subject"...
--

Hi Amit,

Note: This patch is compile tested only. I have done the removal
of buffers from out-queue in handle_control_message()
when host has acked the close request. This seems less
racy than doing it in the release function.

I you want to change this further, feel free to take over from
here and refine this.

Thanks,
Sjur

 drivers/char/virtio_console.c |   14 ++++++--------
 1 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 3fa036a..3a5831d 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1522,15 +1522,9 @@ static void remove_port_data(struct port *port)
 	while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
 		free_buf(buf, true);
 
-	/*
-	 * Check the out-queue for buffers. For VIRTIO_CONSOLE it is a
-	 * bug if this happens. But for RPROC_SERIAL the remote processor
-	 * may have crashed, leaving buffers hanging in the out-queue.
-	 */
-	while ((buf = virtqueue_detach_unused_buf(port->out_vq))) {
-		WARN_ON_ONCE(!is_rproc_serial(port->portdev->vdev));
+	/* Free pending buffers from the out-queue. */
+	while ((buf = virtqueue_detach_unused_buf(port->out_vq)))
 		free_buf(buf, true);
-	}
 }
 
 /*
@@ -1655,6 +1649,10 @@ static void handle_control_message(struct ports_device *portdev,
 		 */
 		spin_lock_irq(&port->outvq_lock);
 		reclaim_consumed_buffers(port);
+
+		/* Free pending buffers from the out-queue. */
+		while ((buf = virtqueue_detach_unused_buf(port->out_vq)))
+			free_buf(buf, true);
 		spin_unlock_irq(&port->outvq_lock);
 
 		/*
-- 
1.7.5.4

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH resend] virtio_console: Free buffers from out-queue upon close

[sjur.brandeland]

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

Free pending output buffers from the virtio out-queue when
host has acknowledged port_close. Also removed WARN_ON()
in remove_port_data().

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

Resending, this time including a proper "Subject"...
--

Hi Amit,

Note: This patch is compile tested only. I have done the removal
of buffers from out-queue in handle_control_message()
when host has acked the close request. This seems less
racy than doing it in the release function.

I you want to change this further, feel free to take over from
here and refine this.

Thanks,
Sjur

 drivers/char/virtio_console.c |   14 ++++++--------
 1 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 3fa036a..3a5831d 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1522,15 +1522,9 @@ static void remove_port_data(struct port *port)
 	while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
 		free_buf(buf, true);
 
-	/*
-	 * Check the out-queue for buffers. For VIRTIO_CONSOLE it is a
-	 * bug if this happens. But for RPROC_SERIAL the remote processor
-	 * may have crashed, leaving buffers hanging in the out-queue.
-	 */
-	while ((buf = virtqueue_detach_unused_buf(port->out_vq))) {
-		WARN_ON_ONCE(!is_rproc_serial(port->portdev->vdev));
+	/* Free pending buffers from the out-queue. */
+	while ((buf = virtqueue_detach_unused_buf(port->out_vq)))
 		free_buf(buf, true);
-	}
 }
 
 /*
@@ -1655,6 +1649,10 @@ static void handle_control_message(struct ports_device *portdev,
 		 */
 		spin_lock_irq(&port->outvq_lock);
 		reclaim_consumed_buffers(port);
+
+		/* Free pending buffers from the out-queue. */
+		while ((buf = virtqueue_detach_unused_buf(port->out_vq)))
+			free_buf(buf, true);
 		spin_unlock_irq(&port->outvq_lock);
 
 		/*
-- 
1.7.5.4

Re: [PATCH resend] virtio_console: Free buffers from out-queue upon close

[Rusty Russell]

sjur.brandeland@stericsson.com writes:

> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
>
> Free pending output buffers from the virtio out-queue when
> host has acknowledged port_close. Also removed WARN_ON()
> in remove_port_data().
>
> Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
> ---
>
> Resending, this time including a proper "Subject"...
> --
>
> Hi Amit,
>
> Note: This patch is compile tested only. I have done the removal
> of buffers from out-queue in handle_control_message()
> when host has acked the close request. This seems less
> racy than doing it in the release function.

This confuses me... why are we doing this in case
VIRTIO_CONSOLE_PORT_OPEN:?

We can't pull unconsumed buffers out of the ring when the other side may
still access it, and this seems to be doing that.

Thanks,
Rusty.

Re: [PATCH resend] virtio_console: Free buffers from out-queue upon close

[Amit Shah]

On (Thu) 08 Nov 2012 [10:28:53], Rusty Russell wrote:
> sjur.brandeland@stericsson.com writes:
> 
> > From: Sjur Brændeland <sjur.brandeland@stericsson.com>
> >
> > Free pending output buffers from the virtio out-queue when
> > host has acknowledged port_close. Also removed WARN_ON()
> > in remove_port_data().
> >
> > Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
> > ---
> >
> > Resending, this time including a proper "Subject"...
> > --
> >
> > Hi Amit,
> >
> > Note: This patch is compile tested only. I have done the removal
> > of buffers from out-queue in handle_control_message()
> > when host has acked the close request. This seems less
> > racy than doing it in the release function.
> 
> This confuses me... why are we doing this in case
> VIRTIO_CONSOLE_PORT_OPEN:?
> 
> We can't pull unconsumed buffers out of the ring when the other side may
> still access it, and this seems to be doing that.

Yes -- and it's my fault; I asked Sjur to do that in the close fops
function.

We should only do this in the port remove case (unplug or device
remove) -- so the original patch, with just the WARN_ON removed is the
right way.

I'll send the revised 3/3 patch for you.

		Amit

Re: [PATCH resend] virtio_console: Free buffers from out-queue upon close

[Amit Shah]

On (Thu) 08 Nov 2012 [10:28:53], Rusty Russell wrote:
> sjur.brandeland@stericsson.com writes:
> 
> > From: Sjur Brændeland <sjur.brandeland@stericsson.com>
> >
> > Free pending output buffers from the virtio out-queue when
> > host has acknowledged port_close. Also removed WARN_ON()
> > in remove_port_data().
> >
> > Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
> > ---
> >
> > Resending, this time including a proper "Subject"...
> > --
> >
> > Hi Amit,
> >
> > Note: This patch is compile tested only. I have done the removal
> > of buffers from out-queue in handle_control_message()
> > when host has acked the close request. This seems less
> > racy than doing it in the release function.
> 
> This confuses me... why are we doing this in case
> VIRTIO_CONSOLE_PORT_OPEN:?
> 
> We can't pull unconsumed buffers out of the ring when the other side may
> still access it, and this seems to be doing that.

Yes -- and it's my fault; I asked Sjur to do that in the close fops
function.

We should only do this in the port remove case (unplug or device
remove) -- so the original patch, with just the WARN_ON removed is the
right way.

I'll send the revised 3/3 patch for you.

		Amit

Re: [PATCH resend] virtio_console: Free buffers from out-queue upon close

[Sjur Brændeland]

>> > Note: This patch is compile tested only. I have done the removal
>> > of buffers from out-queue in handle_control_message()
>> > when host has acked the close request. This seems less
>> > racy than doing it in the release function.
>>
>> This confuses me... why are we doing this in case
>> VIRTIO_CONSOLE_PORT_OPEN:?
>>
>> We can't pull unconsumed buffers out of the ring when the other side may
>> still access it, and this seems to be doing that.
>
> Yes -- and it's my fault; I asked Sjur to do that in the close fops
> function.

Thanks Amit :-), but this was really my bad.

> We should only do this in the port remove case (unplug or device
> remove) -- so the original patch, with just the WARN_ON removed is the
> right way.
>
> I'll send the revised 3/3 patch for you.

Thank you.

Regards,
Sjur

Re: [PATCH resend] virtio_console: Free buffers from out-queue upon close

[Sjur Brændeland]

>> > Note: This patch is compile tested only. I have done the removal
>> > of buffers from out-queue in handle_control_message()
>> > when host has acked the close request. This seems less
>> > racy than doing it in the release function.
>>
>> This confuses me... why are we doing this in case
>> VIRTIO_CONSOLE_PORT_OPEN:?
>>
>> We can't pull unconsumed buffers out of the ring when the other side may
>> still access it, and this seems to be doing that.
>
> Yes -- and it's my fault; I asked Sjur to do that in the close fops
> function.

Thanks Amit :-), but this was really my bad.

> We should only do this in the port remove case (unplug or device
> remove) -- so the original patch, with just the WARN_ON removed is the
> right way.
>
> I'll send the revised 3/3 patch for you.

Thank you.

Regards,
Sjur

Re: [PATCH resend] virtio_console: Free buffers from out-queue upon close

[Rusty Russell]

sjur.brandeland@stericsson.com writes:

> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
>
> Free pending output buffers from the virtio out-queue when
> host has acknowledged port_close. Also removed WARN_ON()
> in remove_port_data().
>
> Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
> ---
>
> Resending, this time including a proper "Subject"...
> --
>
> Hi Amit,
>
> Note: This patch is compile tested only. I have done the removal
> of buffers from out-queue in handle_control_message()
> when host has acked the close request. This seems less
> racy than doing it in the release function.

This confuses me... why are we doing this in case
VIRTIO_CONSOLE_PORT_OPEN:?

We can't pull unconsumed buffers out of the ring when the other side may
still access it, and this seems to be doing that.

Thanks,
Rusty.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

(no subject)

[sjur.brandeland]

From 0ce16d6a0270daebd9972e94a834034a093228b0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Sjur=20Br=C3=A6ndeland?= <sjur.brandeland@stericsson.com>
Date: Wed, 7 Nov 2012 12:20:07 +0100
Subject: [PATCH] virtio_console:Free buffers from out-queue upon close
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Free pending output buffers from the virtio out-queue when
host has acknowledged port_close. Also removed WARN_ON()
in remove_port_data().

Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
Hi Amit,

Note: This patch is compile tested only. I have done the removal
of buffers from out-queue in handle_control_message()
when host has acked the close request. This seems less
racy than doing it in the release function.

I you want to change this further, feel free to take over from
here and refine this.

Thanks,
Sjur

 drivers/char/virtio_console.c |   14 ++++++--------
 1 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 3fa036a..3a5831d 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1522,15 +1522,9 @@ static void remove_port_data(struct port *port)
 	while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
 		free_buf(buf, true);
 
-	/*
-	 * Check the out-queue for buffers. For VIRTIO_CONSOLE it is a
-	 * bug if this happens. But for RPROC_SERIAL the remote processor
-	 * may have crashed, leaving buffers hanging in the out-queue.
-	 */
-	while ((buf = virtqueue_detach_unused_buf(port->out_vq))) {
-		WARN_ON_ONCE(!is_rproc_serial(port->portdev->vdev));
+	/* Free pending buffers from the out-queue. */
+	while ((buf = virtqueue_detach_unused_buf(port->out_vq)))
 		free_buf(buf, true);
-	}
 }
 
 /*
@@ -1655,6 +1649,10 @@ static void handle_control_message(struct ports_device *portdev,
 		 */
 		spin_lock_irq(&port->outvq_lock);
 		reclaim_consumed_buffers(port);
+
+		/* Free pending buffers from the out-queue. */
+		while ((buf = virtqueue_detach_unused_buf(port->out_vq)))
+			free_buf(buf, true);
 		spin_unlock_irq(&port->outvq_lock);
 
 		/*
-- 
1.7.5.4

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

(no subject)

[sjur.brandeland]

>From 0ce16d6a0270daebd9972e94a834034a093228b0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Sjur=20Br=C3=A6ndeland?= <sjur.brandeland@stericsson.com>
Date: Wed, 7 Nov 2012 12:20:07 +0100
Subject: [PATCH] virtio_console:Free buffers from out-queue upon close
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Free pending output buffers from the virtio out-queue when
host has acknowledged port_close. Also removed WARN_ON()
in remove_port_data().

Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
Hi Amit,

Note: This patch is compile tested only. I have done the removal
of buffers from out-queue in handle_control_message()
when host has acked the close request. This seems less
racy than doing it in the release function.

I you want to change this further, feel free to take over from
here and refine this.

Thanks,
Sjur

 drivers/char/virtio_console.c |   14 ++++++--------
 1 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 3fa036a..3a5831d 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1522,15 +1522,9 @@ static void remove_port_data(struct port *port)
 	while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
 		free_buf(buf, true);
 
-	/*
-	 * Check the out-queue for buffers. For VIRTIO_CONSOLE it is a
-	 * bug if this happens. But for RPROC_SERIAL the remote processor
-	 * may have crashed, leaving buffers hanging in the out-queue.
-	 */
-	while ((buf = virtqueue_detach_unused_buf(port->out_vq))) {
-		WARN_ON_ONCE(!is_rproc_serial(port->portdev->vdev));
+	/* Free pending buffers from the out-queue. */
+	while ((buf = virtqueue_detach_unused_buf(port->out_vq)))
 		free_buf(buf, true);
-	}
 }
 
 /*
@@ -1655,6 +1649,10 @@ static void handle_control_message(struct ports_device *portdev,
 		 */
 		spin_lock_irq(&port->outvq_lock);
 		reclaim_consumed_buffers(port);
+
+		/* Free pending buffers from the out-queue. */
+		while ((buf = virtqueue_detach_unused_buf(port->out_vq)))
+			free_buf(buf, true);
 		spin_unlock_irq(&port->outvq_lock);
 
 		/*
-- 
1.7.5.4

Re: [PATCHv7 4/4] virtio_console: Add support for remoteproc serial

[Rusty Russell]

sjur.brandeland@stericsson.com writes:
> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
>
> Add a simple serial connection driver called
> VIRTIO_ID_RPROC_SERIAL (11) for communicating with a
> remote processor in an asymmetric multi-processing
> configuration.
>
> This implementation reuses the existing virtio_console
> implementation, and adds support for DMA allocation
> of data buffers and disables use of tty console and
> the virtio control queue.
>
> Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>

The free-outside-interrupt issue is usually dealt with by offloading to
a wq, but your variant works (and isn't too ugly).

> +		/* dma_free_coherent requires interrupts to be enabled. */
> +		if (!can_sleep) {
> +			/* queue up dma-buffers to be freed later */
> +			spin_lock_irqsave(&dma_bufs_lock, flags);
> +			list_add_tail(&buf->list, &pending_free_dma_bufs);
> +			spin_unlock_irqrestore(&dma_bufs_lock, flags);
> +			return;
> +		}
> +		dma_free_coherent(buf->dev, buf->size, buf->buf, buf->dma);
> +
> +		/* Release device refcnt and allow it to be freed */
> +		put_device(buf->dev);

...

> +static void reclaim_dma_bufs(void)
> +{
> +	unsigned long flags;
> +	struct port_buffer *buf, *tmp;
> +	LIST_HEAD(tmp_list);
> +
> +	if (list_empty(&pending_free_dma_bufs))
> +		return;
> +
> +	/* Create a copy of the pending_free_dma_bufs while holding the lock */
> +	spin_lock_irqsave(&dma_bufs_lock, flags);
> +	list_cut_position(&tmp_list, &pending_free_dma_bufs,
> +			  pending_free_dma_bufs.prev);
> +	spin_unlock_irqrestore(&dma_bufs_lock, flags);
> +
> +	/* Release the dma buffers, without irqs enabled */
> +	list_for_each_entry_safe(buf, tmp, &tmp_list, list) {
> +		list_del(&buf->list);
> +		free_buf(buf, true);
> +	}
> +}

Looks like this should be an easy noop even if !is_rproc_serial.

> +
>  static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size,
>  				     int pages)
>  {
>  	struct port_buffer *buf;
>  
> +	if (is_rproc_serial(vq->vdev))
> +		reclaim_dma_bufs();
> +

...

> @@ -904,6 +1000,8 @@ static int port_fops_release(struct inode *inode, struct file *filp)
>  	reclaim_consumed_buffers(port);
>  	spin_unlock_irq(&port->outvq_lock);
>  
> +	if (is_rproc_serial(port->portdev->vdev))
> +		reclaim_dma_bufs();

So these are redundant.

> @@ -1415,7 +1524,16 @@ static void remove_port_data(struct port *port)
>  
>  	/* Remove buffers we queued up for the Host to send us data in. */
>  	while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
> -		free_buf(buf);
> +		free_buf(buf, true);
> +
> +	/*
> +	 * Remove buffers from out queue for rproc-serial. We cannot afford
> +	 * to leak any DMA mem, so reclaim this memory even if this might be
> +	 * racy for the remote processor.
> +	 */
> +	if (is_rproc_serial(port->portdev->vdev))
> +		while ((buf = virtqueue_detach_unused_buf(port->out_vq)))
> +			free_buf(buf, true);
>  }

This seems wrong; either this is needed even if !is_rproc_serial(), or
it's not necessary as the out_vq is empty.

Every path I can see has the device reset (in which case the queues
should not be active), or we got a VIRTIO_CONSOLE_PORT_REMOVE event (in
which case, the same).

I think we can have non-blocking writes which could leave buffers in
out_vq: Amit?
>  static void __exit fini(void)
>  {
> +	reclaim_dma_bufs();

Hmm, you didn't protect it here anyway...

Cheers,
Rusty.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCHv7 0/4] virtio_console: Add rproc_serial driver

[Amit Shah]

Hello Sjur,

On (Mon) 15 Oct 2012 [09:57:32], sjur.brandeland@stericsson.com wrote:
> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
> 
> This patch-set introduces a new virtio type "rproc_serial" for communicating
> with remote processors over shared memory. The driver depends on the
> the remoteproc framework. As preparation for introducing "rproc_serial"
> I've done a refactoring of the transmit buffer handling.
> 
> This patch-set is a rework of the patch-set from Sept 25th, hopefully all
> review comments has been addressed.
> 
> The fist patch is a bugfix and migth be applicable for 3.7.

Looks good,

Acked-by: Amit Shah <amit.shah@redhat.com>

include/linux/virtio_ids.h has moved to include/uapi/linux/.  Last
patch has to be re-spun for that.

Otherwise, I just checked if comments from the previous submission
were addressed.  The virtio-console test suite passes fine, so I
didn't really go through everything again.

Thanks,

		Amit

Re: [PATCHv7 0/4] virtio_console: Add rproc_serial driver

[Amit Shah]

Hello Sjur,

On (Mon) 15 Oct 2012 [09:57:32], sjur.brandeland@stericsson.com wrote:
> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
> 
> This patch-set introduces a new virtio type "rproc_serial" for communicating
> with remote processors over shared memory. The driver depends on the
> the remoteproc framework. As preparation for introducing "rproc_serial"
> I've done a refactoring of the transmit buffer handling.
> 
> This patch-set is a rework of the patch-set from Sept 25th, hopefully all
> review comments has been addressed.
> 
> The fist patch is a bugfix and migth be applicable for 3.7.

Looks good,

Acked-by: Amit Shah <amit.shah@redhat.com>

include/linux/virtio_ids.h has moved to include/uapi/linux/.  Last
patch has to be re-spun for that.

Otherwise, I just checked if comments from the previous submission
were addressed.  The virtio-console test suite passes fine, so I
didn't really go through everything again.

Thanks,

		Amit