[PATCH v2 0/2] TTY: add rpmsg tty driver

[PATCH v2 0/2] TTY: add rpmsg tty driver

[Arnaud Pouliquen]

This patch set introduces a TTY console on top of the RPMsg framework which
enables the following use cases:
- Provide a console to communicate easily with the remote processor
  application.
- Provide an interface to get the remote processor log traces without
  ring buffer limitation.
- Ease the migration from MPU + MCU processors to multi core processors
  (MPU and MCU integrated in one processor)

An alternative of this proposed solution would consist in using the virtio
console:
The drawback with that solution is that it requires a specific virtio buffer
(in addition to the one already used for RPMsg) which does not fit with remote
processors with little memory. The proposed solution allows to multiplex the
console with the other rpmsg services, optimizing the memory.

The first patch adds an API to the rpmsg framework ('get buffer size') and the
second one is the rpmsg tty driver itself.

History:
-V1 to V2:
	- modify message structure to allow to data transmission but also
	flow control
	- add documentation file to describe message structure for remote
	  implementation. 
	- add dtr/rts management
	- disable termios modes that generates non optimized behavior on RPMsg
	  transfers
	- replace rpmsg_send by rpmsg_trysend to not block the write
	- suppress useless spinlock on read.
	- miscellaneous fixes to improve robustness

Arnaud Pouliquen (2):
  rpmsg: core: add possibility to get message payload length
  tty: add rpmsg driver

 Documentation/serial/tty_rpmsg.txt |  38 +++
 drivers/rpmsg/rpmsg_core.c         |  20 ++
 drivers/rpmsg/rpmsg_internal.h     |   2 +
 drivers/rpmsg/virtio_rpmsg_bus.c   |  11 +
 drivers/tty/Kconfig                |   9 +
 drivers/tty/Makefile               |   1 +
 drivers/tty/rpmsg_tty.c            | 479 +++++++++++++++++++++++++++++++++++++
 include/linux/rpmsg.h              |  10 +
 8 files changed, 570 insertions(+)
 create mode 100644 Documentation/serial/tty_rpmsg.txt
 create mode 100644 drivers/tty/rpmsg_tty.c

-- 
2.7.4

[PATCH v2 1/2] rpmsg: core: add possibility to get message payload length

[Arnaud Pouliquen]

Return the rpmsg buffer payload size for sending message, so rpmsg users
can split a long message in several sub rpmsg buffers.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
---
 drivers/rpmsg/rpmsg_core.c       | 20 ++++++++++++++++++++
 drivers/rpmsg/rpmsg_internal.h   |  2 ++
 drivers/rpmsg/virtio_rpmsg_bus.c | 11 +++++++++++
 include/linux/rpmsg.h            | 10 ++++++++++
 4 files changed, 43 insertions(+)

diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
index 8122807db380..75c8c403ffe5 100644
--- a/drivers/rpmsg/rpmsg_core.c
+++ b/drivers/rpmsg/rpmsg_core.c
@@ -283,6 +283,26 @@ int rpmsg_trysend_offchannel(struct rpmsg_endpoint *ept, u32 src, u32 dst,
 }
 EXPORT_SYMBOL(rpmsg_trysend_offchannel);
 
+/**
+ * rpmsg_get_buf_payload_size()
+ * This function returns buffer payload size available for sending messages.
+ *
+ * @ept: the rpmsg endpoint
+ *
+ * Returns buffer payload size on success and an appropriate error value on
+ * failure.
+ */
+int rpmsg_get_buf_payload_size(struct rpmsg_endpoint *ept)
+{
+	if (WARN_ON(!ept))
+		return -EINVAL;
+	if (!ept->ops->get_buf_payload_size)
+		return -ENXIO;
+
+	return ept->ops->get_buf_payload_size(ept);
+}
+EXPORT_SYMBOL(rpmsg_get_buf_payload_size);
+
 /*
  * match an rpmsg channel with a channel info struct.
  * this is used to make sure we're not creating rpmsg devices for channels
diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
index 0d791c30b7ea..6f733a556139 100644
--- a/drivers/rpmsg/rpmsg_internal.h
+++ b/drivers/rpmsg/rpmsg_internal.h
@@ -46,6 +46,7 @@ struct rpmsg_device_ops {
  * @trysend:		see @rpmsg_trysend(), required
  * @trysendto:		see @rpmsg_trysendto(), optional
  * @trysend_offchannel:	see @rpmsg_trysend_offchannel(), optional
+ * @get_buf_payload_size: see @rpmsg_get_buf_payload_size(), optional
  *
  * Indirection table for the operations that a rpmsg backend should implement.
  * In addition to @destroy_ept, the backend must at least implement @send and
@@ -65,6 +66,7 @@ struct rpmsg_endpoint_ops {
 			     void *data, int len);
 	__poll_t (*poll)(struct rpmsg_endpoint *ept, struct file *filp,
 			     poll_table *wait);
+	int (*get_buf_payload_size)(struct rpmsg_endpoint *ept);
 };
 
 int rpmsg_register_device(struct rpmsg_device *rpdev);
diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 5d3685bd76a2..82753e76e377 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -175,6 +175,7 @@ static int virtio_rpmsg_trysendto(struct rpmsg_endpoint *ept, void *data,
 				  int len, u32 dst);
 static int virtio_rpmsg_trysend_offchannel(struct rpmsg_endpoint *ept, u32 src,
 					   u32 dst, void *data, int len);
+static int virtio_get_buf_payload_size(struct rpmsg_endpoint *ept);
 
 static const struct rpmsg_endpoint_ops virtio_endpoint_ops = {
 	.destroy_ept = virtio_rpmsg_destroy_ept,
@@ -184,6 +185,7 @@ static const struct rpmsg_endpoint_ops virtio_endpoint_ops = {
 	.trysend = virtio_rpmsg_trysend,
 	.trysendto = virtio_rpmsg_trysendto,
 	.trysend_offchannel = virtio_rpmsg_trysend_offchannel,
+	.get_buf_payload_size = virtio_get_buf_payload_size,
 };
 
 /**
@@ -699,6 +701,15 @@ static int virtio_rpmsg_trysend_offchannel(struct rpmsg_endpoint *ept, u32 src,
 	return rpmsg_send_offchannel_raw(rpdev, src, dst, data, len, false);
 }
 
+static int virtio_get_buf_payload_size(struct rpmsg_endpoint *ept)
+{
+	struct rpmsg_device *rpdev = ept->rpdev;
+	struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
+	int size = vch->vrp->buf_size - sizeof(struct rpmsg_hdr);
+
+	return size < 0 ? -EMSGSIZE : size;
+}
+
 static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
 			     struct rpmsg_hdr *msg, unsigned int len)
 {
diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
index 9fe156d1c018..250df2165086 100644
--- a/include/linux/rpmsg.h
+++ b/include/linux/rpmsg.h
@@ -135,6 +135,8 @@ int rpmsg_trysend_offchannel(struct rpmsg_endpoint *ept, u32 src, u32 dst,
 __poll_t rpmsg_poll(struct rpmsg_endpoint *ept, struct file *filp,
 			poll_table *wait);
 
+int rpmsg_get_buf_payload_size(struct rpmsg_endpoint *ept);
+
 #else
 
 static inline int register_rpmsg_device(struct rpmsg_device *dev)
@@ -242,6 +244,14 @@ static inline __poll_t rpmsg_poll(struct rpmsg_endpoint *ept,
 	return 0;
 }
 
+static int rpmsg_get_buf_payload_size(struct rpmsg_endpoint *ept)
+{
+	/* This shouldn't be possible */
+	WARN_ON(1);
+
+	return -ENXIO;
+}
+
 #endif /* IS_ENABLED(CONFIG_RPMSG) */
 
 /* use a macro to avoid include chaining to get THIS_MODULE */
-- 
2.7.4

[PATCH v2 2/2] tty: add rpmsg driver

[Arnaud Pouliquen]

This driver exposes a standard tty interface on top of the rpmsg
framework through the "rpmsg-tty-channel" rpmsg service.

This driver supports multi-instances, offering a /dev/ttyRPMSGx entry
per rpmsg endpoint.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
---
 Documentation/serial/tty_rpmsg.txt |  38 +++
 drivers/tty/Kconfig                |   9 +
 drivers/tty/Makefile               |   1 +
 drivers/tty/rpmsg_tty.c            | 479 +++++++++++++++++++++++++++++++++++++
 4 files changed, 527 insertions(+)
 create mode 100644 Documentation/serial/tty_rpmsg.txt
 create mode 100644 drivers/tty/rpmsg_tty.c

diff --git a/Documentation/serial/tty_rpmsg.txt b/Documentation/serial/tty_rpmsg.txt
new file mode 100644
index 000000000000..e069ed268a2b
--- /dev/null
+++ b/Documentation/serial/tty_rpmsg.txt
@@ -0,0 +1,38 @@
+
+			The rpmsg TTY
+
+The rpmsg tty driver implements a serial communication on the rpmsg bus,
+to communicate with a remote processor devices in asymmetric multiprocessing
+(AMP) configurations.
+
+The remote processor can instantiate a new tty by requesting a new "rpmsg-tty-channel" RPMsg service. Information related to the RPMsg and
+associated tty device is available in /sys/bus/rpmsg/devices/virtio0.rpmsg-tty-channel.-1.<X>, with
+<X> corresponding to the ttyRPMSG instance.
+
+RPMsg data/control structure
+----------------------------
+
+The RPMsg is used to send data or control messages. Differentiation between the
+stream and the control messages is done thanks to the first byte of the
+RPMsg payload:
+
+
+RPMSG_DATA	- rest of messages contains data
+
+RPMSG_CTRL 	- message contains control.
+
+
+To be compliant with this driver, the remote firmware has to respect this RPMsg
+payload structure. At least the RPMSG_DATA type has to be supported. The
+RPMSG_CTRL is optional.
+
+Flow control type
+-----------------
+
+A minimum flow control can be implemented to allow/block communication with the remote processor.
+
+DATA_TERM_READY	-	one parameter:
+			- u8 state
+				Set to indicate to remote side that terminal is
+				ready for communication.
+				Reset to block communication with remote side.
diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
index e0a04bfc873e..d7b426939f69 100644
--- a/drivers/tty/Kconfig
+++ b/drivers/tty/Kconfig
@@ -442,6 +442,15 @@ config VCC
 	help
 	  Support for Sun logical domain consoles.
 
+config RPMSG_TTY
+	tristate "RPMSG tty driver"
+	depends on RPMSG
+	help
+	  Say y here to export rpmsg endpoints as tty devices, usually found
+	  in /dev/ttyRPMSGx.
+	  This makes it possible for user-space programs to send and receive
+	  rpmsg messages as a standard tty protocol.
+
 config LDISC_AUTOLOAD
 	bool "Automatically load TTY Line Disciplines"
 	default y
diff --git a/drivers/tty/Makefile b/drivers/tty/Makefile
index c72cafdf32b4..90a98a20714d 100644
--- a/drivers/tty/Makefile
+++ b/drivers/tty/Makefile
@@ -33,5 +33,6 @@ obj-$(CONFIG_PPC_EPAPR_HV_BYTECHAN) += ehv_bytechan.o
 obj-$(CONFIG_GOLDFISH_TTY)	+= goldfish.o
 obj-$(CONFIG_MIPS_EJTAG_FDC_TTY) += mips_ejtag_fdc.o
 obj-$(CONFIG_VCC)		+= vcc.o
+obj-$(CONFIG_RPMSG_TTY)		+= rpmsg_tty.o
 
 obj-y += ipwireless/
diff --git a/drivers/tty/rpmsg_tty.c b/drivers/tty/rpmsg_tty.c
new file mode 100644
index 000000000000..bbd319bf464c
--- /dev/null
+++ b/drivers/tty/rpmsg_tty.c
@@ -0,0 +1,479 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) STMicroelectronics 2019 - All Rights Reserved
+ * Authors: Arnaud Pouliquen <arnaud.pouliquen@st.com> for STMicroelectronics.
+ *          Fabien Dessenne <fabien.dessenne@st.com> for STMicroelectronics.
+ */
+
+#include <linux/module.h>
+#include <linux/rpmsg.h>
+#include <linux/slab.h>
+#include <linux/tty.h>
+#include <linux/tty_flip.h>
+
+#define MAX_TTY_RPMSG		32
+
+static DEFINE_IDR(tty_idr);	/* tty instance id */
+static DEFINE_MUTEX(idr_lock);	/* protects tty_idr */
+
+static struct tty_driver *rpmsg_tty_driver;
+
+enum rpmsg_tty_type_t {
+	RPMSG_DATA,
+	RPMSG_CTRL,
+	NUM_RPMSG_TTY_TYPE
+};
+
+enum rpmsg_tty_ctrl_t {
+	DATA_TERM_READY,	/* ready to accept data */
+	NUM_RPMSG_TTY_CTRL_TYPE
+};
+
+struct rpmsg_tty_payload {
+	u8 cmd;
+	u8 data[0];
+};
+
+struct rpmsg_tty_ctrl {
+	u8 ctrl;
+	u8 values[0];
+};
+
+struct rpmsg_tty_port {
+	struct tty_port		port;	 /* TTY port data */
+	int			id;	 /* TTY rpmsg index */
+	struct rpmsg_device	*rpdev;	 /* rpmsg device */
+	int			cts;	 /* remote reception status */
+};
+
+static int rpmsg_tty_data_handler(struct rpmsg_device *rpdev, void *data,
+				  int len, void *priv, u32 src)
+{
+	struct rpmsg_tty_port *cport = dev_get_drvdata(&rpdev->dev);
+	u8 *cbuf;
+	int space;
+
+	dev_dbg(&rpdev->dev, "msg(<- src 0x%x) len %d\n", src, len);
+
+	if (!len)
+		return 0;
+
+	dev_dbg(&rpdev->dev, "space available: %d\n",
+		tty_buffer_space_avail(&cport->port));
+
+	space = tty_prepare_flip_string(&cport->port, &cbuf, len);
+	if (space <= 0) {
+		dev_err(&rpdev->dev, "No memory for tty_prepare_flip_string\n");
+		return -ENOMEM;
+	}
+
+	if (space != len)
+		dev_warn(&rpdev->dev, "Trunc buffer from %d to %d\n", len,
+			 space);
+
+	memcpy(cbuf, data, space);
+	tty_flip_buffer_push(&cport->port);
+
+	return 0;
+}
+
+static int rpmsg_tty_ctrl_handler(struct rpmsg_device *rpdev, void *data,
+				  int len, void *priv, u32 src)
+{
+	struct rpmsg_tty_port *cport = dev_get_drvdata(&rpdev->dev);
+	struct rpmsg_tty_ctrl *ctrl = data;
+
+	dev_dbg(&rpdev->dev, "%s: ctrl received %d\n", __func__, ctrl->ctrl);
+	print_hex_dump_debug(__func__, DUMP_PREFIX_NONE, 16, 1, data, len,
+			     true);
+
+	if (len <= sizeof(*ctrl))
+		return -EINVAL;
+
+	if (ctrl->ctrl == DATA_TERM_READY) {
+		/* Update the CTS according to remote RTS */
+		if (!ctrl->values[0]) {
+			cport->cts = 0;
+		} else {
+			cport->cts = 1;
+			tty_port_tty_wakeup(&cport->port);
+		}
+		return 0;
+	}
+
+	dev_err(&rpdev->dev, "unknown control ID %d\n", ctrl->ctrl);
+
+	return -EINVAL;
+}
+
+static const rpmsg_rx_cb_t rpmsg_tty_handler[] = {
+	[RPMSG_DATA] = rpmsg_tty_data_handler,
+	[RPMSG_CTRL] = rpmsg_tty_ctrl_handler,
+};
+
+static int rpmsg_tty_cb(struct rpmsg_device *rpdev, void *data, int len,
+			void *priv, u32 src)
+{
+	struct rpmsg_tty_payload  *rbuf = data;
+
+	if (len <= sizeof(*rbuf) || rbuf->cmd > NUM_RPMSG_TTY_TYPE) {
+		dev_err(&rpdev->dev, "Invalid message: size %d, type %d\n",
+			len, rbuf->cmd);
+		return -EINVAL;
+	}
+
+	return rpmsg_tty_handler[rbuf->cmd](rpdev, &rbuf->data,
+					    len - sizeof(rbuf->cmd), priv, src);
+}
+
+static int rpmsg_tty_write_control(struct tty_struct *tty, u8 ctrl, u8 *values,
+				   unsigned int n_value)
+{
+	struct rpmsg_tty_port *cport = idr_find(&tty_idr, tty->index);
+	struct rpmsg_tty_payload *msg;
+	struct rpmsg_tty_ctrl *m_ctrl;
+	struct rpmsg_device *rpdev;
+	unsigned int msg_size;
+	int ret;
+
+	if (!cport) {
+		dev_err(tty->dev, "cannot get cport\n");
+		return -ENODEV;
+	}
+
+	rpdev = cport->rpdev;
+
+	msg_size = sizeof(*msg) + sizeof(*m_ctrl) + n_value;
+	msg = kzalloc(msg_size, GFP_KERNEL);
+
+	msg->cmd = RPMSG_CTRL;
+	m_ctrl =  (struct rpmsg_tty_ctrl *)&msg->data[0];
+	m_ctrl->ctrl = DATA_TERM_READY;
+	memcpy(m_ctrl->values, values, n_value);
+
+	ret = rpmsg_trysend(rpdev->ept, msg, msg_size);
+	if (ret < 0) {
+		dev_err(tty->dev, "cannot send control (%d)\n", ret);
+		ret = 0;
+	}
+	kfree(msg);
+
+	return ret;
+};
+
+static void rpmsg_tty_throttle(struct tty_struct *tty)
+{
+	u8 rts = 0;
+
+	/* Disable remote transmission */
+	rpmsg_tty_write_control(tty, DATA_TERM_READY, &rts, 1);
+};
+
+static void rpmsg_tty_unthrottle(struct tty_struct *tty)
+{
+	u8 rts = 1;
+
+	/* Enable remote transmission */
+	rpmsg_tty_write_control(tty, DATA_TERM_READY, &rts, 1);
+};
+
+static int rpmsg_tty_install(struct tty_driver *driver, struct tty_struct *tty)
+{
+	struct rpmsg_tty_port *cport = idr_find(&tty_idr, tty->index);
+
+	if (!cport) {
+		dev_err(tty->dev, "cannot get cport\n");
+		return -ENODEV;
+	}
+
+	return tty_port_install(&cport->port, driver, tty);
+}
+
+static int rpmsg_tty_open(struct tty_struct *tty, struct file *filp)
+{
+	return tty_port_open(tty->port, tty, filp);
+}
+
+static void rpmsg_tty_close(struct tty_struct *tty, struct file *filp)
+{
+	return tty_port_close(tty->port, tty, filp);
+}
+
+static int rpmsg_tty_write(struct tty_struct *tty, const u8 *buf, int len)
+{
+	struct rpmsg_tty_port *cport = idr_find(&tty_idr, tty->index);
+	struct rpmsg_device *rpdev;
+	int msg_size, msg_max_size, ret = 0;
+	int cmd_sz = sizeof(struct rpmsg_tty_payload);
+	u8 *tmpbuf;
+
+	if (!cport) {
+		dev_err(tty->dev, "cannot get cport\n");
+		return -ENODEV;
+	}
+
+	/* If cts not set, the message is not sent*/
+	if (!cport->cts)
+		return 0;
+
+	rpdev = cport->rpdev;
+
+	dev_dbg(&rpdev->dev, "%s: send msg from tty->index = %d, len = %d\n",
+		__func__, tty->index, len);
+	if (!buf) {
+		dev_err(&rpdev->dev, "buf shouldn't be null.\n");
+		return -ENOMEM;
+	}
+
+	msg_max_size = rpmsg_get_buf_payload_size(rpdev->ept);
+	if (msg_max_size < 0)
+		return msg_max_size;
+
+	msg_size = min(len + cmd_sz, msg_max_size);
+	tmpbuf = kzalloc(msg_size, GFP_KERNEL);
+
+	tmpbuf[0] = RPMSG_DATA;
+	memcpy(&tmpbuf[cmd_sz], buf, msg_size - cmd_sz);
+
+	/*
+	 * Try to send the message to remote processor, if failed return 0 as
+	 * no data sent
+	 */
+	ret = rpmsg_trysend(rpdev->ept, (void *)tmpbuf, msg_size);
+	kfree(tmpbuf);
+	if (ret) {
+		dev_dbg(&rpdev->dev, "rpmsg_send failed: %d\n", ret);
+		return 0;
+	}
+
+	return msg_size - sizeof(struct rpmsg_tty_payload);
+}
+
+static int rpmsg_tty_write_room(struct tty_struct *tty)
+{
+	struct rpmsg_tty_port *cport = idr_find(&tty_idr, tty->index);
+	int space = 0;
+
+	if (!cport) {
+		dev_err(tty->dev, "cannot get cport\n");
+		return -ENODEV;
+	}
+
+	/*
+	 * Report the space in the rpmsg buffer, first byte is reserved to
+	 * define the buffer type.
+	 */
+	if (cport->cts) {
+		space = rpmsg_get_buf_payload_size(cport->rpdev->ept);
+		space -= sizeof(struct rpmsg_tty_payload);
+	}
+
+	return space;
+}
+
+static const struct tty_operations rpmsg_tty_ops = {
+	.install	= rpmsg_tty_install,
+	.open		= rpmsg_tty_open,
+	.close		= rpmsg_tty_close,
+	.write		= rpmsg_tty_write,
+	.write_room	= rpmsg_tty_write_room,
+	.throttle	= rpmsg_tty_throttle,
+	.unthrottle	= rpmsg_tty_unthrottle,
+};
+
+static struct rpmsg_tty_port *rpmsg_tty_alloc_cport(void)
+{
+	struct rpmsg_tty_port *cport;
+
+	cport = kzalloc(sizeof(*cport), GFP_KERNEL);
+	if (!cport)
+		return ERR_PTR(-ENOMEM);
+
+	mutex_lock(&idr_lock);
+	cport->id = idr_alloc(&tty_idr, cport, 0, MAX_TTY_RPMSG, GFP_KERNEL);
+	mutex_unlock(&idr_lock);
+
+	if (cport->id < 0) {
+		kfree(cport);
+		return ERR_PTR(-ENOSPC);
+	}
+
+	return cport;
+}
+
+static void rpmsg_tty_release_cport(struct rpmsg_tty_port *cport)
+{
+	mutex_lock(&idr_lock);
+	idr_remove(&tty_idr, cport->id);
+	mutex_unlock(&idr_lock);
+
+	kfree(cport);
+}
+
+static int rpmsg_tty_port_activate(struct tty_port *p, struct tty_struct *tty)
+{
+	/* Allocate the buffer we use for writing data */
+	return tty_port_alloc_xmit_buf(p);
+}
+
+static void rpmsg_tty_port_shutdown(struct tty_port *p)
+{
+	/* Free the write buffer */
+	tty_port_free_xmit_buf(p);
+}
+
+static void rpmsg_tty_dtr_rts(struct tty_port *port, int raise)
+{
+	struct rpmsg_tty_port *cport =
+				container_of(port, struct rpmsg_tty_port, port);
+
+	pr_debug("%s: dtr_rts state %d\n", __func__, raise);
+	if (!port->tty || !cport) {
+		pr_err("invalid port\n");
+		return;
+	}
+
+	cport->cts = raise;
+
+	if (raise)
+		rpmsg_tty_unthrottle(port->tty);
+	else
+		rpmsg_tty_throttle(port->tty);
+}
+
+static const struct tty_port_operations rpmsg_tty_port_ops = {
+	.activate = rpmsg_tty_port_activate,
+	.shutdown = rpmsg_tty_port_shutdown,
+	.dtr_rts  = rpmsg_tty_dtr_rts,
+};
+
+static int rpmsg_tty_probe(struct rpmsg_device *rpdev)
+{
+	struct rpmsg_tty_port *cport;
+	struct device *dev = &rpdev->dev;
+	struct device *tty_dev;
+	int ret;
+
+	cport = rpmsg_tty_alloc_cport();
+	if (IS_ERR(cport)) {
+		dev_err(dev, "failed to alloc tty port\n");
+		return PTR_ERR(cport);
+	}
+
+	tty_port_init(&cport->port);
+	cport->port.low_latency = cport->port.flags | ASYNC_LOW_LATENCY;
+	cport->port.ops = &rpmsg_tty_port_ops;
+
+	tty_dev = tty_port_register_device(&cport->port, rpmsg_tty_driver,
+					   cport->id, dev);
+	if (IS_ERR(tty_dev)) {
+		dev_err(dev, "failed to register tty port\n");
+		ret = PTR_ERR(tty_dev);
+		goto  err_destroy;
+	}
+
+	cport->rpdev = rpdev;
+
+	dev_set_drvdata(dev, cport);
+
+	dev_dbg(dev, "new channel: 0x%x -> 0x%x : ttyRPMSG%d\n",
+		rpdev->src, rpdev->dst, cport->id);
+
+	return 0;
+
+err_destroy:
+	tty_port_destroy(&cport->port);
+	rpmsg_tty_release_cport(cport);
+
+	return ret;
+}
+
+static void rpmsg_tty_remove(struct rpmsg_device *rpdev)
+{
+	struct rpmsg_tty_port *cport = dev_get_drvdata(&rpdev->dev);
+
+	dev_dbg(&rpdev->dev, "removing rpmsg tty device %d\n", cport->id);
+
+	/* User hang up to release the tty */
+	if (tty_port_initialized(&cport->port))
+		tty_port_tty_hangup(&cport->port, false);
+
+	tty_unregister_device(rpmsg_tty_driver, cport->id);
+
+	tty_port_destroy(&cport->port);
+	rpmsg_tty_release_cport(cport);
+}
+
+static struct rpmsg_device_id rpmsg_driver_tty_id_table[] = {
+	{ .name	= "rpmsg-tty-channel" },
+	{ },
+};
+MODULE_DEVICE_TABLE(rpmsg, rpmsg_driver_tty_id_table);
+
+static struct rpmsg_driver rpmsg_tty_rpmsg_drv = {
+	.drv.name	= KBUILD_MODNAME,
+	.id_table	= rpmsg_driver_tty_id_table,
+	.probe		= rpmsg_tty_probe,
+	.callback	= rpmsg_tty_cb,
+	.remove		= rpmsg_tty_remove,
+};
+
+static int __init rpmsg_tty_init(void)
+{
+	int err;
+
+	rpmsg_tty_driver = tty_alloc_driver(MAX_TTY_RPMSG, TTY_DRIVER_REAL_RAW |
+					    TTY_DRIVER_DYNAMIC_DEV);
+	if (IS_ERR(rpmsg_tty_driver))
+		return PTR_ERR(rpmsg_tty_driver);
+
+	rpmsg_tty_driver->driver_name = "rpmsg_tty";
+	rpmsg_tty_driver->name = "ttyRPMSG";
+	rpmsg_tty_driver->major = 0;
+	rpmsg_tty_driver->type = TTY_DRIVER_TYPE_CONSOLE;
+
+	/* Disable unused mode by default */
+	rpmsg_tty_driver->init_termios = tty_std_termios;
+	rpmsg_tty_driver->init_termios.c_lflag &= ~(ECHO | ICANON);
+	rpmsg_tty_driver->init_termios.c_oflag &= ~(OPOST | ONLCR);
+
+	tty_set_operations(rpmsg_tty_driver, &rpmsg_tty_ops);
+
+	err = tty_register_driver(rpmsg_tty_driver);
+	if (err < 0) {
+		pr_err("Couldn't install rpmsg tty driver: err %d\n", err);
+		goto error_put;
+	}
+
+	err = register_rpmsg_driver(&rpmsg_tty_rpmsg_drv);
+	if (err < 0) {
+		pr_err("Couldn't register rpmsg tty driver: err %d\n", err);
+		goto error_unregister;
+	}
+
+	return 0;
+
+error_unregister:
+	tty_unregister_driver(rpmsg_tty_driver);
+
+error_put:
+	put_tty_driver(rpmsg_tty_driver);
+
+	return err;
+}
+
+static void __exit rpmsg_tty_exit(void)
+{
+	unregister_rpmsg_driver(&rpmsg_tty_rpmsg_drv);
+	tty_unregister_driver(rpmsg_tty_driver);
+	put_tty_driver(rpmsg_tty_driver);
+	idr_destroy(&tty_idr);
+}
+
+module_init(rpmsg_tty_init);
+module_exit(rpmsg_tty_exit);
+
+MODULE_AUTHOR("Arnaud Pouliquen <arnaud.pouliquen@st.com>");
+MODULE_AUTHOR("Fabien Dessenne <fabien.dessenne@st.com>");
+MODULE_DESCRIPTION("virtio remote processor messaging tty driver");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

Re: [PATCH v2 2/2] tty: add rpmsg driver

[Alan Cox]

> +static int rpmsg_tty_data_handler(struct rpmsg_device *rpdev, void *data,
> +				  int len, void *priv, u32 src)
> +{
> +	struct rpmsg_tty_port *cport = dev_get_drvdata(&rpdev->dev);
> +	u8 *cbuf;
> +	int space;
> +
> +	dev_dbg(&rpdev->dev, "msg(<- src 0x%x) len %d\n", src, len);
> +
> +	if (!len)
> +		return 0;
> +
> +	dev_dbg(&rpdev->dev, "space available: %d\n",
> +		tty_buffer_space_avail(&cport->port));
> +
> +	space = tty_prepare_flip_string(&cport->port, &cbuf, len);
> +	if (space <= 0) {
> +		dev_err(&rpdev->dev, "No memory for tty_prepare_flip_string\n");
> +		return -ENOMEM;
> +	}

Really bad idea.

1. It's not an 'error'. It's normal that in the case the consumer is
blocked you can run out of processing space

2. You will trigger this when being hit by a very large fast flow of data
so responding by burning CPU spewing messages (possibly even out of this
tty) is bad.

It's not a bug - just keep statistics and throw it away. If anyone cares
they will do flow control.


> +
> +static int rpmsg_tty_write_control(struct tty_struct *tty, u8 ctrl, u8 *values,
> +				   unsigned int n_value)
> +{
> +	struct rpmsg_tty_port *cport = idr_find(&tty_idr, tty->index);
> +	struct rpmsg_tty_payload *msg;
> +	struct rpmsg_tty_ctrl *m_ctrl;
> +	struct rpmsg_device *rpdev;
> +	unsigned int msg_size;
> +	int ret;
> +
> +	if (!cport) {
> +		dev_err(tty->dev, "cannot get cport\n");
> +		return -ENODEV;
> +	}
> +
> +	rpdev = cport->rpdev;
> +
> +	msg_size = sizeof(*msg) + sizeof(*m_ctrl) + n_value;
> +	msg = kzalloc(msg_size, GFP_KERNEL);


Out of memory check ?

> +static int rpmsg_tty_write(struct tty_struct *tty, const u8 *buf, int len)
> +{
> +	struct rpmsg_tty_port *cport = idr_find(&tty_idr, tty->index);
> +	struct rpmsg_device *rpdev;
> +	int msg_size, msg_max_size, ret = 0;
> +	int cmd_sz = sizeof(struct rpmsg_tty_payload);
> +	u8 *tmpbuf;
> +
> +	if (!cport) {
> +		dev_err(tty->dev, "cannot get cport\n");
> +		return -ENODEV;
> +	}
> +
> +	/* If cts not set, the message is not sent*/
> +	if (!cport->cts)
> +		return 0;
> +
> +	rpdev = cport->rpdev;
> +
> +	dev_dbg(&rpdev->dev, "%s: send msg from tty->index = %d, len = %d\n",
> +		__func__, tty->index, len);
> +	if (!buf) {
> +		dev_err(&rpdev->dev, "buf shouldn't be null.\n");
> +		return -ENOMEM;
> +	}
> +
> +	msg_max_size = rpmsg_get_buf_payload_size(rpdev->ept);
> +	if (msg_max_size < 0)
> +		return msg_max_size;
> +
> +	msg_size = min(len + cmd_sz, msg_max_size);
> +	tmpbuf = kzalloc(msg_size, GFP_KERNEL);

Allocation failure check ?

Re: [PATCH v2 2/2] tty: add rpmsg driver

[Arnaud Pouliquen]

Hello Alan,

On 5/16/19 7:20 PM, Alan Cox wrote:
> 
>> +static int rpmsg_tty_data_handler(struct rpmsg_device *rpdev, void *data,
>> +				  int len, void *priv, u32 src)
>> +{
>> +	struct rpmsg_tty_port *cport = dev_get_drvdata(&rpdev->dev);
>> +	u8 *cbuf;
>> +	int space;
>> +
>> +	dev_dbg(&rpdev->dev, "msg(<- src 0x%x) len %d\n", src, len);
>> +
>> +	if (!len)
>> +		return 0;
>> +
>> +	dev_dbg(&rpdev->dev, "space available: %d\n",
>> +		tty_buffer_space_avail(&cport->port));
>> +
>> +	space = tty_prepare_flip_string(&cport->port, &cbuf, len);
>> +	if (space <= 0) {
>> +		dev_err(&rpdev->dev, "No memory for tty_prepare_flip_string\n");
>> +		return -ENOMEM;
>> +	}
> 
> Really bad idea.
> 
> 1. It's not an 'error'. It's normal that in the case the consumer is
> blocked you can run out of processing space
> 
> 2. You will trigger this when being hit by a very large fast flow of data
> so responding by burning CPU spewing messages (possibly even out of this
> tty) is bad.
> 
> It's not a bug - just keep statistics and throw it away. If anyone cares
> they will do flow control.

Indeed,and there is a similar issue in the rpmsg_tty_write_control (on
rpmsg_trysend failure), i will change message level to debug.
Furthermore  there is a nonsense to return error to rpmsg framework as
rpmsg reception is ok...

> 
> 
>> +
>> +static int rpmsg_tty_write_control(struct tty_struct *tty, u8 ctrl, u8 *values,
>> +				   unsigned int n_value)
>> +{
>> +	struct rpmsg_tty_port *cport = idr_find(&tty_idr, tty->index);
>> +	struct rpmsg_tty_payload *msg;
>> +	struct rpmsg_tty_ctrl *m_ctrl;
>> +	struct rpmsg_device *rpdev;
>> +	unsigned int msg_size;
>> +	int ret;
>> +
>> +	if (!cport) {
>> +		dev_err(tty->dev, "cannot get cport\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	rpdev = cport->rpdev;
>> +
>> +	msg_size = sizeof(*msg) + sizeof(*m_ctrl) + n_value;
>> +	msg = kzalloc(msg_size, GFP_KERNEL);
> 
> 
> Out of memory check ?
> 
>> +static int rpmsg_tty_write(struct tty_struct *tty, const u8 *buf, int len)
>> +{
>> +	struct rpmsg_tty_port *cport = idr_find(&tty_idr, tty->index);
>> +	struct rpmsg_device *rpdev;
>> +	int msg_size, msg_max_size, ret = 0;
>> +	int cmd_sz = sizeof(struct rpmsg_tty_payload);
>> +	u8 *tmpbuf;
>> +
>> +	if (!cport) {
>> +		dev_err(tty->dev, "cannot get cport\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	/* If cts not set, the message is not sent*/
>> +	if (!cport->cts)
>> +		return 0;
>> +
>> +	rpdev = cport->rpdev;
>> +
>> +	dev_dbg(&rpdev->dev, "%s: send msg from tty->index = %d, len = %d\n",
>> +		__func__, tty->index, len);
>> +	if (!buf) {
>> +		dev_err(&rpdev->dev, "buf shouldn't be null.\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	msg_max_size = rpmsg_get_buf_payload_size(rpdev->ept);
>> +	if (msg_max_size < 0)
>> +		return msg_max_size;
>> +
>> +	msg_size = min(len + cmd_sz, msg_max_size);
>> +	tmpbuf = kzalloc(msg_size, GFP_KERNEL);
> 
> Allocation failure check ?
> 

Thanks for your comments, i will fix in a v3.

Regards
Arnaud

Re: [PATCH v2 1/2] rpmsg: core: add possibility to get message payload length

[Bjorn Andersson]

On Fri 10 May 08:02 PDT 2019, Arnaud Pouliquen wrote:

> Return the rpmsg buffer payload size for sending message, so rpmsg users
> can split a long message in several sub rpmsg buffers.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
> Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>

This should list each person who dealt with the patch. So the first
would be the author and the last would be the one that posted it on the
list.

If you both authored the patch then you should add Co-developed-by to
denote this.

> ---
>  drivers/rpmsg/rpmsg_core.c       | 20 ++++++++++++++++++++
>  drivers/rpmsg/rpmsg_internal.h   |  2 ++
>  drivers/rpmsg/virtio_rpmsg_bus.c | 11 +++++++++++
>  include/linux/rpmsg.h            | 10 ++++++++++
>  4 files changed, 43 insertions(+)
> 
> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
> index 8122807db380..75c8c403ffe5 100644
> --- a/drivers/rpmsg/rpmsg_core.c
> +++ b/drivers/rpmsg/rpmsg_core.c
> @@ -283,6 +283,26 @@ int rpmsg_trysend_offchannel(struct rpmsg_endpoint *ept, u32 src, u32 dst,
>  }
>  EXPORT_SYMBOL(rpmsg_trysend_offchannel);
>  
> +/**
> + * rpmsg_get_buf_payload_size()
> + * This function returns buffer payload size available for sending messages.
> + *
> + * @ept: the rpmsg endpoint
> + *
> + * Returns buffer payload size on success and an appropriate error value on
> + * failure.
> + */

Please read
https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt

> +int rpmsg_get_buf_payload_size(struct rpmsg_endpoint *ept)

payload size can have many meanings and hopefully we'll end up in a
situation where its dynamic. But it could be considered useful to have a
way to query the maximum transmission size in such a setup.

So please rename this rpmsg_get_mtu() or something similar.

And I would prefer ssize_t as return type.

> +{
> +	if (WARN_ON(!ept))
> +		return -EINVAL;
> +	if (!ept->ops->get_buf_payload_size)
> +		return -ENXIO;
> +
> +	return ept->ops->get_buf_payload_size(ept);
> +}
> +EXPORT_SYMBOL(rpmsg_get_buf_payload_size);
> +
>  /*
>   * match an rpmsg channel with a channel info struct.
>   * this is used to make sure we're not creating rpmsg devices for channels
> diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
> index 0d791c30b7ea..6f733a556139 100644
> --- a/drivers/rpmsg/rpmsg_internal.h
> +++ b/drivers/rpmsg/rpmsg_internal.h
> @@ -46,6 +46,7 @@ struct rpmsg_device_ops {
>   * @trysend:		see @rpmsg_trysend(), required
>   * @trysendto:		see @rpmsg_trysendto(), optional
>   * @trysend_offchannel:	see @rpmsg_trysend_offchannel(), optional
> + * @get_buf_payload_size: see @rpmsg_get_buf_payload_size(), optional
>   *
>   * Indirection table for the operations that a rpmsg backend should implement.
>   * In addition to @destroy_ept, the backend must at least implement @send and
> @@ -65,6 +66,7 @@ struct rpmsg_endpoint_ops {
>  			     void *data, int len);
>  	__poll_t (*poll)(struct rpmsg_endpoint *ept, struct file *filp,
>  			     poll_table *wait);
> +	int (*get_buf_payload_size)(struct rpmsg_endpoint *ept);
>  };
>  
>  int rpmsg_register_device(struct rpmsg_device *rpdev);
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> index 5d3685bd76a2..82753e76e377 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -175,6 +175,7 @@ static int virtio_rpmsg_trysendto(struct rpmsg_endpoint *ept, void *data,
>  				  int len, u32 dst);
>  static int virtio_rpmsg_trysend_offchannel(struct rpmsg_endpoint *ept, u32 src,
>  					   u32 dst, void *data, int len);
> +static int virtio_get_buf_payload_size(struct rpmsg_endpoint *ept);
>  
>  static const struct rpmsg_endpoint_ops virtio_endpoint_ops = {
>  	.destroy_ept = virtio_rpmsg_destroy_ept,
> @@ -184,6 +185,7 @@ static const struct rpmsg_endpoint_ops virtio_endpoint_ops = {
>  	.trysend = virtio_rpmsg_trysend,
>  	.trysendto = virtio_rpmsg_trysendto,
>  	.trysend_offchannel = virtio_rpmsg_trysend_offchannel,
> +	.get_buf_payload_size = virtio_get_buf_payload_size,
>  };
>  
>  /**
> @@ -699,6 +701,15 @@ static int virtio_rpmsg_trysend_offchannel(struct rpmsg_endpoint *ept, u32 src,
>  	return rpmsg_send_offchannel_raw(rpdev, src, dst, data, len, false);
>  }
>  
> +static int virtio_get_buf_payload_size(struct rpmsg_endpoint *ept)
> +{
> +	struct rpmsg_device *rpdev = ept->rpdev;
> +	struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
> +	int size = vch->vrp->buf_size - sizeof(struct rpmsg_hdr);
> +
> +	return size < 0 ? -EMSGSIZE : size;

Seems that a rpmsg instance configured to not even have space for the
rpmsg_hdr in each message is rather broken. Shouldn't this be prohibited
elsewhere?

Regards,
Bjorn

> +}
> +
>  static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
>  			     struct rpmsg_hdr *msg, unsigned int len)
>  {
> diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
> index 9fe156d1c018..250df2165086 100644
> --- a/include/linux/rpmsg.h
> +++ b/include/linux/rpmsg.h
> @@ -135,6 +135,8 @@ int rpmsg_trysend_offchannel(struct rpmsg_endpoint *ept, u32 src, u32 dst,
>  __poll_t rpmsg_poll(struct rpmsg_endpoint *ept, struct file *filp,
>  			poll_table *wait);
>  
> +int rpmsg_get_buf_payload_size(struct rpmsg_endpoint *ept);
> +
>  #else
>  
>  static inline int register_rpmsg_device(struct rpmsg_device *dev)
> @@ -242,6 +244,14 @@ static inline __poll_t rpmsg_poll(struct rpmsg_endpoint *ept,
>  	return 0;
>  }
>  
> +static int rpmsg_get_buf_payload_size(struct rpmsg_endpoint *ept)
> +{
> +	/* This shouldn't be possible */
> +	WARN_ON(1);
> +
> +	return -ENXIO;
> +}
> +
>  #endif /* IS_ENABLED(CONFIG_RPMSG) */
>  
>  /* use a macro to avoid include chaining to get THIS_MODULE */
> -- 
> 2.7.4
> 

Re: [PATCH v2 2/2] tty: add rpmsg driver

[Bjorn Andersson]

On Fri 10 May 08:02 PDT 2019, Arnaud Pouliquen wrote:

> This driver exposes a standard tty interface on top of the rpmsg
> framework through the "rpmsg-tty-channel" rpmsg service.
> 
> This driver supports multi-instances, offering a /dev/ttyRPMSGx entry
> per rpmsg endpoint.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
> Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
> ---
>  Documentation/serial/tty_rpmsg.txt |  38 +++
>  drivers/tty/Kconfig                |   9 +
>  drivers/tty/Makefile               |   1 +
>  drivers/tty/rpmsg_tty.c            | 479 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 527 insertions(+)
>  create mode 100644 Documentation/serial/tty_rpmsg.txt
>  create mode 100644 drivers/tty/rpmsg_tty.c
> 
> diff --git a/Documentation/serial/tty_rpmsg.txt b/Documentation/serial/tty_rpmsg.txt
> new file mode 100644
> index 000000000000..e069ed268a2b
> --- /dev/null
> +++ b/Documentation/serial/tty_rpmsg.txt
> @@ -0,0 +1,38 @@
> +
> +			The rpmsg TTY
> +
> +The rpmsg tty driver implements a serial communication on the rpmsg bus,
> +to communicate with a remote processor devices in asymmetric multiprocessing
> +(AMP) configurations.
> +
> +The remote processor can instantiate a new tty by requesting a new "rpmsg-tty-channel" RPMsg service. Information related to the RPMsg and
> +associated tty device is available in /sys/bus/rpmsg/devices/virtio0.rpmsg-tty-channel.-1.<X>, with
> +<X> corresponding to the ttyRPMSG instance.
> +
> +RPMsg data/control structure
> +----------------------------
> +
> +The RPMsg is used to send data or control messages. Differentiation between the
> +stream and the control messages is done thanks to the first byte of the
> +RPMsg payload:
> +
> +
> +RPMSG_DATA	- rest of messages contains data
> +
> +RPMSG_CTRL 	- message contains control.
> +
> +
> +To be compliant with this driver, the remote firmware has to respect this RPMsg
> +payload structure. At least the RPMSG_DATA type has to be supported. The
> +RPMSG_CTRL is optional.
> +

This scheme prevents us from using this driver to expose any existing
tty-like channels without having to modify such firmware.

> +Flow control type
> +-----------------
> +
> +A minimum flow control can be implemented to allow/block communication with the remote processor.
> +
> +DATA_TERM_READY	-	one parameter:
> +			- u8 state
> +				Set to indicate to remote side that terminal is
> +				ready for communication.
> +				Reset to block communication with remote side.

And as shown in discussions following Qualcomm's proposed flow-control
addition to the rpmsg API the need for flow control is not limited to
this custom tty like interface. 


So I really would like to see an implementation of a side-band flow
control mechanism in the virtio rpmsg bus.

> diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
> index e0a04bfc873e..d7b426939f69 100644
> --- a/drivers/tty/Kconfig
> +++ b/drivers/tty/Kconfig
> @@ -442,6 +442,15 @@ config VCC
>  	help
>  	  Support for Sun logical domain consoles.
>  
> +config RPMSG_TTY
> +	tristate "RPMSG tty driver"
> +	depends on RPMSG
> +	help
> +	  Say y here to export rpmsg endpoints as tty devices, usually found
> +	  in /dev/ttyRPMSGx.
> +	  This makes it possible for user-space programs to send and receive
> +	  rpmsg messages as a standard tty protocol.
> +
>  config LDISC_AUTOLOAD
>  	bool "Automatically load TTY Line Disciplines"
>  	default y
> diff --git a/drivers/tty/Makefile b/drivers/tty/Makefile
> index c72cafdf32b4..90a98a20714d 100644
> --- a/drivers/tty/Makefile
> +++ b/drivers/tty/Makefile
> @@ -33,5 +33,6 @@ obj-$(CONFIG_PPC_EPAPR_HV_BYTECHAN) += ehv_bytechan.o
>  obj-$(CONFIG_GOLDFISH_TTY)	+= goldfish.o
>  obj-$(CONFIG_MIPS_EJTAG_FDC_TTY) += mips_ejtag_fdc.o
>  obj-$(CONFIG_VCC)		+= vcc.o
> +obj-$(CONFIG_RPMSG_TTY)		+= rpmsg_tty.o
>  
>  obj-y += ipwireless/
> diff --git a/drivers/tty/rpmsg_tty.c b/drivers/tty/rpmsg_tty.c
[..]
> +static struct rpmsg_device_id rpmsg_driver_tty_id_table[] = {
> +	{ .name	= "rpmsg-tty-channel" },

I really would like a mechanism that does not depend on a fixed channel
name, as this required that firmware is written specifically for being
paired with this driver.

In other words this is exactly the same problem that we worked around in
rpmsg_char.

Regards,
Bjorn

Re: [PATCH v2 1/2] rpmsg: core: add possibility to get message payload length

[Arnaud Pouliquen]

On 7/1/19 7:31 AM, Bjorn Andersson wrote:
> On Fri 10 May 08:02 PDT 2019, Arnaud Pouliquen wrote:
> 
>> Return the rpmsg buffer payload size for sending message, so rpmsg users
>> can split a long message in several sub rpmsg buffers.
>>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
>> Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
> 
> This should list each person who dealt with the patch. So the first
> would be the author and the last would be the one that posted it on the
> list.
> 
> If you both authored the patch then you should add Co-developed-by to
> denote this.
ok i will update it
> 
>> ---
>>  drivers/rpmsg/rpmsg_core.c       | 20 ++++++++++++++++++++
>>  drivers/rpmsg/rpmsg_internal.h   |  2 ++
>>  drivers/rpmsg/virtio_rpmsg_bus.c | 11 +++++++++++
>>  include/linux/rpmsg.h            | 10 ++++++++++
>>  4 files changed, 43 insertions(+)
>>
>> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
>> index 8122807db380..75c8c403ffe5 100644
>> --- a/drivers/rpmsg/rpmsg_core.c
>> +++ b/drivers/rpmsg/rpmsg_core.c
>> @@ -283,6 +283,26 @@ int rpmsg_trysend_offchannel(struct rpmsg_endpoint *ept, u32 src, u32 dst,
>>  }
>>  EXPORT_SYMBOL(rpmsg_trysend_offchannel);
>>  
>> +/**
>> + * rpmsg_get_buf_payload_size()
>> + * This function returns buffer payload size available for sending messages.
>> + *
>> + * @ept: the rpmsg endpoint
>> + *
>> + * Returns buffer payload size on success and an appropriate error value on
>> + * failure.
>> + */
> 
> Please read
> https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt
oops sorry for this dirty documentation.
> 
>> +int rpmsg_get_buf_payload_size(struct rpmsg_endpoint *ept)
> 
> payload size can have many meanings and hopefully we'll end up in a
> situation where its dynamic. But it could be considered useful to have a
> way to query the maximum transmission size in such a setup.
> 
> So please rename this rpmsg_get_mtu() or something similar.
> 
> And I would prefer ssize_t as return type.

ssize_t rpmsg_get_mtu(struct rpmsg_endpoint *ept) sounds good, ok for this
>> +{
>> +	if (WARN_ON(!ept))
>> +		return -EINVAL;
>> +	if (!ept->ops->get_buf_payload_size)
>> +		return -ENXIO;
>> +
>> +	return ept->ops->get_buf_payload_size(ept);
>> +}
>> +EXPORT_SYMBOL(rpmsg_get_buf_payload_size);
>> +
>>  /*
>>   * match an rpmsg channel with a channel info struct.
>>   * this is used to make sure we're not creating rpmsg devices for channels
>> diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
>> index 0d791c30b7ea..6f733a556139 100644
>> --- a/drivers/rpmsg/rpmsg_internal.h
>> +++ b/drivers/rpmsg/rpmsg_internal.h
>> @@ -46,6 +46,7 @@ struct rpmsg_device_ops {
>>   * @trysend:		see @rpmsg_trysend(), required
>>   * @trysendto:		see @rpmsg_trysendto(), optional
>>   * @trysend_offchannel:	see @rpmsg_trysend_offchannel(), optional
>> + * @get_buf_payload_size: see @rpmsg_get_buf_payload_size(), optional
>>   *
>>   * Indirection table for the operations that a rpmsg backend should implement.
>>   * In addition to @destroy_ept, the backend must at least implement @send and
>> @@ -65,6 +66,7 @@ struct rpmsg_endpoint_ops {
>>  			     void *data, int len);
>>  	__poll_t (*poll)(struct rpmsg_endpoint *ept, struct file *filp,
>>  			     poll_table *wait);
>> +	int (*get_buf_payload_size)(struct rpmsg_endpoint *ept);
>>  };
>>  
>>  int rpmsg_register_device(struct rpmsg_device *rpdev);
>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
>> index 5d3685bd76a2..82753e76e377 100644
>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>> @@ -175,6 +175,7 @@ static int virtio_rpmsg_trysendto(struct rpmsg_endpoint *ept, void *data,
>>  				  int len, u32 dst);
>>  static int virtio_rpmsg_trysend_offchannel(struct rpmsg_endpoint *ept, u32 src,
>>  					   u32 dst, void *data, int len);
>> +static int virtio_get_buf_payload_size(struct rpmsg_endpoint *ept);
>>  
>>  static const struct rpmsg_endpoint_ops virtio_endpoint_ops = {
>>  	.destroy_ept = virtio_rpmsg_destroy_ept,
>> @@ -184,6 +185,7 @@ static const struct rpmsg_endpoint_ops virtio_endpoint_ops = {
>>  	.trysend = virtio_rpmsg_trysend,
>>  	.trysendto = virtio_rpmsg_trysendto,
>>  	.trysend_offchannel = virtio_rpmsg_trysend_offchannel,
>> +	.get_buf_payload_size = virtio_get_buf_payload_size,
>>  };
>>  
>>  /**
>> @@ -699,6 +701,15 @@ static int virtio_rpmsg_trysend_offchannel(struct rpmsg_endpoint *ept, u32 src,
>>  	return rpmsg_send_offchannel_raw(rpdev, src, dst, data, len, false);
>>  }
>>  
>> +static int virtio_get_buf_payload_size(struct rpmsg_endpoint *ept)
>> +{
>> +	struct rpmsg_device *rpdev = ept->rpdev;
>> +	struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
>> +	int size = vch->vrp->buf_size - sizeof(struct rpmsg_hdr);
>> +
>> +	return size < 0 ? -EMSGSIZE : size;
> 
> Seems that a rpmsg instance configured to not even have space for the
> rpmsg_hdr in each message is rather broken. Shouldn't this be prohibited
> elsewhere?
yes seems that the are some useless check in code on buf_size, i will
suppress the check here.
there are some other instances in code...For time being the size is
fixed, but good place seems to be in the rpmsg_probe probe function. i
will propose a patch to clean this.

Thanks,
Arnaud

> 
> Regards,
> Bjorn
> 
>> +}
>> +
>>  static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
>>  			     struct rpmsg_hdr *msg, unsigned int len)
>>  {
>> diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
>> index 9fe156d1c018..250df2165086 100644
>> --- a/include/linux/rpmsg.h
>> +++ b/include/linux/rpmsg.h
>> @@ -135,6 +135,8 @@ int rpmsg_trysend_offchannel(struct rpmsg_endpoint *ept, u32 src, u32 dst,
>>  __poll_t rpmsg_poll(struct rpmsg_endpoint *ept, struct file *filp,
>>  			poll_table *wait);
>>  
>> +int rpmsg_get_buf_payload_size(struct rpmsg_endpoint *ept);
>> +
>>  #else
>>  
>>  static inline int register_rpmsg_device(struct rpmsg_device *dev)
>> @@ -242,6 +244,14 @@ static inline __poll_t rpmsg_poll(struct rpmsg_endpoint *ept,
>>  	return 0;
>>  }
>>  
>> +static int rpmsg_get_buf_payload_size(struct rpmsg_endpoint *ept)
>> +{
>> +	/* This shouldn't be possible */
>> +	WARN_ON(1);
>> +
>> +	return -ENXIO;
>> +}
>> +
>>  #endif /* IS_ENABLED(CONFIG_RPMSG) */
>>  
>>  /* use a macro to avoid include chaining to get THIS_MODULE */
>> -- 
>> 2.7.4
>>

Re: [PATCH v2 2/2] tty: add rpmsg driver

[Arnaud Pouliquen]

Hi Bjorn,

On 7/1/19 8:00 AM, Bjorn Andersson wrote:
> On Fri 10 May 08:02 PDT 2019, Arnaud Pouliquen wrote:
> 
>> This driver exposes a standard tty interface on top of the rpmsg
>> framework through the "rpmsg-tty-channel" rpmsg service.
>>
>> This driver supports multi-instances, offering a /dev/ttyRPMSGx entry
>> per rpmsg endpoint.
>>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
>> Signed-off-by: Fabien Dessenne <fabien.dessenne@st.com>
>> ---
>>  Documentation/serial/tty_rpmsg.txt |  38 +++
>>  drivers/tty/Kconfig                |   9 +
>>  drivers/tty/Makefile               |   1 +
>>  drivers/tty/rpmsg_tty.c            | 479 +++++++++++++++++++++++++++++++++++++
>>  4 files changed, 527 insertions(+)
>>  create mode 100644 Documentation/serial/tty_rpmsg.txt
>>  create mode 100644 drivers/tty/rpmsg_tty.c
>>
>> diff --git a/Documentation/serial/tty_rpmsg.txt b/Documentation/serial/tty_rpmsg.txt
>> new file mode 100644
>> index 000000000000..e069ed268a2b
>> --- /dev/null
>> +++ b/Documentation/serial/tty_rpmsg.txt
>> @@ -0,0 +1,38 @@
>> +
>> +			The rpmsg TTY
>> +
>> +The rpmsg tty driver implements a serial communication on the rpmsg bus,
>> +to communicate with a remote processor devices in asymmetric multiprocessing
>> +(AMP) configurations.
>> +
>> +The remote processor can instantiate a new tty by requesting a new "rpmsg-tty-channel" RPMsg service. Information related to the RPMsg and
>> +associated tty device is available in /sys/bus/rpmsg/devices/virtio0.rpmsg-tty-channel.-1.<X>, with
>> +<X> corresponding to the ttyRPMSG instance.
>> +
>> +RPMsg data/control structure
>> +----------------------------
>> +
>> +The RPMsg is used to send data or control messages. Differentiation between the
>> +stream and the control messages is done thanks to the first byte of the
>> +RPMsg payload:
>> +
>> +
>> +RPMSG_DATA	- rest of messages contains data
>> +
>> +RPMSG_CTRL 	- message contains control.
>> +
>> +
>> +To be compliant with this driver, the remote firmware has to respect this RPMsg
>> +payload structure. At least the RPMSG_DATA type has to be supported. The
>> +RPMSG_CTRL is optional.
>> +
> 
> This scheme prevents us from using this driver to expose any existing
> tty-like channels without having to modify such firmware.

An alternative could be to define a channel for the control and another
for the data. This would avoid to reserved the first bytes for message type.

> 
>> +Flow control type
>> +-----------------
>> +
>> +A minimum flow control can be implemented to allow/block communication with the remote processor.
>> +
>> +DATA_TERM_READY	-	one parameter:
>> +			- u8 state
>> +				Set to indicate to remote side that terminal is
>> +				ready for communication.
>> +				Reset to block communication with remote side.
> 
> And as shown in discussions following Qualcomm's proposed flow-control
> addition to the rpmsg API the need for flow control is not limited to
> this custom tty like interface

Yes i remember discussions around the flow control, how to set
priorities, bandwidth, etc...
Did you start something on Qualcomm side to support flow control?

RPMsg flow control would also impact the remote side (a.e. OpenAMP).
This subject can be quite extensive, so perhaps, it should be better to
treat it in a separate thread to be sure that design integrate all
requirements.

What about integrating rpmsg tty in a first step (with flow control in
rpmsg tty)? Then in a second step, adapt it on the rpmsg flow control,
and so use it to validate the feature.
This would avoid to block the rpmsg_tty integration... As you mention
seems that several companies have already their own rpmsg tty
implementation...

> 
> 
> So I really would like to see an implementation of a side-band flow
> control mechanism in the virtio rpmsg bus.
> 
>> diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
>> index e0a04bfc873e..d7b426939f69 100644
>> --- a/drivers/tty/Kconfig
>> +++ b/drivers/tty/Kconfig
>> @@ -442,6 +442,15 @@ config VCC
>>  	help
>>  	  Support for Sun logical domain consoles.
>>  
>> +config RPMSG_TTY
>> +	tristate "RPMSG tty driver"
>> +	depends on RPMSG
>> +	help
>> +	  Say y here to export rpmsg endpoints as tty devices, usually found
>> +	  in /dev/ttyRPMSGx.
>> +	  This makes it possible for user-space programs to send and receive
>> +	  rpmsg messages as a standard tty protocol.
>> +
>>  config LDISC_AUTOLOAD
>>  	bool "Automatically load TTY Line Disciplines"
>>  	default y
>> diff --git a/drivers/tty/Makefile b/drivers/tty/Makefile
>> index c72cafdf32b4..90a98a20714d 100644
>> --- a/drivers/tty/Makefile
>> +++ b/drivers/tty/Makefile
>> @@ -33,5 +33,6 @@ obj-$(CONFIG_PPC_EPAPR_HV_BYTECHAN) += ehv_bytechan.o
>>  obj-$(CONFIG_GOLDFISH_TTY)	+= goldfish.o
>>  obj-$(CONFIG_MIPS_EJTAG_FDC_TTY) += mips_ejtag_fdc.o
>>  obj-$(CONFIG_VCC)		+= vcc.o
>> +obj-$(CONFIG_RPMSG_TTY)		+= rpmsg_tty.o
>>  
>>  obj-y += ipwireless/
>> diff --git a/drivers/tty/rpmsg_tty.c b/drivers/tty/rpmsg_tty.c
> [..]
>> +static struct rpmsg_device_id rpmsg_driver_tty_id_table[] = {
>> +	{ .name	= "rpmsg-tty-channel" },
> 
> I really would like a mechanism that does not depend on a fixed channel
> name, as this required that firmware is written specifically for being
> paired with this driver.
> 
> In other words this is exactly the same problem that we worked around in
> rpmsg_charTo be sure... What you propose here is to implement an attribute for the
channel (as rpmsg_eptdevd defined in rpmsg_char), allowing application
could define the channel name?

This point is similar to the one we discussed with Xiang. What should we
consider: the tty service associated to a tty channel or the service on
top of the tty which would be in this case based on rpmsg?

I considered that the service is the tty, as we create a /dev/tty when
service is requested by the remoteproc firmware, so that the service
name is fixed in consequence.

Using attribute for channel name would mean that user registers a
service that will be on top of a tty. In this case, is the TTY is the
good communication channel? why not directly use rpmsg_char?

So perhaps could be better to extend the rpmsg char device for this kind
of requirement, allowing to probe it without associate it to an existing
rpmsg device...?

Thank,
Arnaud

> 
> Regards,
> Bjorn
>