[PATCH v8 0/2] Add rpmsg tty driver

[PATCH v8 0/2] Add rpmsg tty driver

[Arnaud Pouliquen]

This new revision reopens subject started a long time ago. Previous revision discussions
available here [1]. 

This patchset 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) by offering a virtual serial link.

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 max transmission unit')
and the second one is the rpmsg tty driver itself.

Update vs previous revision [1]:
  - simplify the patch by removing the control part(CTS) to keep only the stream part.
  - rebase on kernel V5.15-rc1
  - take into account v7 review comments.

Notice that I kept the Acked-by and Reviewed-by in "rpmsg: core: add API to get MTU" commit
even though I'm not sure if it's still relevant after such a long period...

[1] https://lkml.org/lkml/2020/3/24/1394 

Arnaud Pouliquen (2):
  rpmsg: core: add API to get MTU
  tty: add rpmsg driver

 Documentation/serial/tty_rpmsg.rst |  15 ++
 drivers/rpmsg/rpmsg_core.c         |  21 +++
 drivers/rpmsg/rpmsg_internal.h     |   2 +
 drivers/rpmsg/virtio_rpmsg_bus.c   |  10 ++
 drivers/tty/Kconfig                |   9 +
 drivers/tty/Makefile               |   1 +
 drivers/tty/rpmsg_tty.c            | 275 +++++++++++++++++++++++++++++
 include/linux/rpmsg.h              |  10 ++
 8 files changed, 343 insertions(+)
 create mode 100644 Documentation/serial/tty_rpmsg.rst
 create mode 100644 drivers/tty/rpmsg_tty.c

-- 
2.17.1

[PATCH v8 1/2] rpmsg: core: add API to get MTU

[Arnaud Pouliquen]

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

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
Acked-by: Suman Anna <s-anna@ti.com>
Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/rpmsg/rpmsg_core.c       | 21 +++++++++++++++++++++
 drivers/rpmsg/rpmsg_internal.h   |  2 ++
 drivers/rpmsg/virtio_rpmsg_bus.c | 10 ++++++++++
 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 9151836190ce..fb955a79462b 100644
--- a/drivers/rpmsg/rpmsg_core.c
+++ b/drivers/rpmsg/rpmsg_core.c
@@ -327,6 +327,27 @@ int rpmsg_trysend_offchannel(struct rpmsg_endpoint *ept, u32 src, u32 dst,
 }
 EXPORT_SYMBOL(rpmsg_trysend_offchannel);
 
+/**
+ * rpmsg_get_mtu() - get maximum transmission buffer size for sending message.
+ * @ept: the rpmsg endpoint
+ *
+ * This function returns maximum buffer size available for a single message.
+ *
+ * Return: the maximum transmission size on success and an appropriate error
+ * value on failure.
+ */
+
+ssize_t rpmsg_get_mtu(struct rpmsg_endpoint *ept)
+{
+	if (WARN_ON(!ept))
+		return -EINVAL;
+	if (!ept->ops->get_mtu)
+		return -ENOTSUPP;
+
+	return ept->ops->get_mtu(ept);
+}
+EXPORT_SYMBOL(rpmsg_get_mtu);
+
 /*
  * match a 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 a76c344253bf..b1245d3ed7c6 100644
--- a/drivers/rpmsg/rpmsg_internal.h
+++ b/drivers/rpmsg/rpmsg_internal.h
@@ -53,6 +53,7 @@ struct rpmsg_device_ops {
  * @trysendto:		see @rpmsg_trysendto(), optional
  * @trysend_offchannel:	see @rpmsg_trysend_offchannel(), optional
  * @poll:		see @rpmsg_poll(), optional
+ * @get_mtu:		see @rpmsg_get_mtu(), 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
@@ -72,6 +73,7 @@ struct rpmsg_endpoint_ops {
 			     void *data, int len);
 	__poll_t (*poll)(struct rpmsg_endpoint *ept, struct file *filp,
 			     poll_table *wait);
+	ssize_t (*get_mtu)(struct rpmsg_endpoint *ept);
 };
 
 struct device *rpmsg_find_device(struct device *parent,
diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 8e49a3bacfc7..05fd06fc67e9 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -149,6 +149,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 ssize_t virtio_rpmsg_get_mtu(struct rpmsg_endpoint *ept);
 static struct rpmsg_device *__rpmsg_create_channel(struct virtproc_info *vrp,
 						   struct rpmsg_channel_info *chinfo);
 
@@ -160,6 +161,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_mtu = virtio_rpmsg_get_mtu,
 };
 
 /**
@@ -696,6 +698,14 @@ static int virtio_rpmsg_trysend_offchannel(struct rpmsg_endpoint *ept, u32 src,
 	return rpmsg_send_offchannel_raw(rpdev, src, dst, data, len, false);
 }
 
+static ssize_t virtio_rpmsg_get_mtu(struct rpmsg_endpoint *ept)
+{
+	struct rpmsg_device *rpdev = ept->rpdev;
+	struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
+
+	return vch->vrp->buf_size - sizeof(struct rpmsg_hdr);
+}
+
 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 d97dcd049f18..990b80fb49ad 100644
--- a/include/linux/rpmsg.h
+++ b/include/linux/rpmsg.h
@@ -186,6 +186,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);
 
+ssize_t rpmsg_get_mtu(struct rpmsg_endpoint *ept);
+
 #else
 
 static inline int rpmsg_register_device(struct rpmsg_device *rpdev)
@@ -296,6 +298,14 @@ static inline __poll_t rpmsg_poll(struct rpmsg_endpoint *ept,
 	return 0;
 }
 
+static inline ssize_t rpmsg_get_mtu(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.17.1

Re: [PATCH v8 1/2] rpmsg: core: add API to get MTU

[Bjorn Andersson]

On Thu 30 Sep 09:05 PDT 2021, Arnaud Pouliquen wrote:

> Return the rpmsg buffer MTU for sending message, so rpmsg users
> can split a long message in several sub rpmsg buffers.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> Acked-by: Suman Anna <s-anna@ti.com>
> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  drivers/rpmsg/rpmsg_core.c       | 21 +++++++++++++++++++++
>  drivers/rpmsg/rpmsg_internal.h   |  2 ++
>  drivers/rpmsg/virtio_rpmsg_bus.c | 10 ++++++++++
>  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 9151836190ce..fb955a79462b 100644
> --- a/drivers/rpmsg/rpmsg_core.c
> +++ b/drivers/rpmsg/rpmsg_core.c
> @@ -327,6 +327,27 @@ int rpmsg_trysend_offchannel(struct rpmsg_endpoint *ept, u32 src, u32 dst,
>  }
>  EXPORT_SYMBOL(rpmsg_trysend_offchannel);
>  
> +/**
> + * rpmsg_get_mtu() - get maximum transmission buffer size for sending message.
> + * @ept: the rpmsg endpoint
> + *
> + * This function returns maximum buffer size available for a single message.

Nit. Can we add "outgoing" between "single" and "message" here?


Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> + *
> + * Return: the maximum transmission size on success and an appropriate error
> + * value on failure.
> + */
> +
> +ssize_t rpmsg_get_mtu(struct rpmsg_endpoint *ept)
> +{
> +	if (WARN_ON(!ept))
> +		return -EINVAL;
> +	if (!ept->ops->get_mtu)
> +		return -ENOTSUPP;
> +
> +	return ept->ops->get_mtu(ept);
> +}
> +EXPORT_SYMBOL(rpmsg_get_mtu);
> +
>  /*
>   * match a 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 a76c344253bf..b1245d3ed7c6 100644
> --- a/drivers/rpmsg/rpmsg_internal.h
> +++ b/drivers/rpmsg/rpmsg_internal.h
> @@ -53,6 +53,7 @@ struct rpmsg_device_ops {
>   * @trysendto:		see @rpmsg_trysendto(), optional
>   * @trysend_offchannel:	see @rpmsg_trysend_offchannel(), optional
>   * @poll:		see @rpmsg_poll(), optional
> + * @get_mtu:		see @rpmsg_get_mtu(), 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
> @@ -72,6 +73,7 @@ struct rpmsg_endpoint_ops {
>  			     void *data, int len);
>  	__poll_t (*poll)(struct rpmsg_endpoint *ept, struct file *filp,
>  			     poll_table *wait);
> +	ssize_t (*get_mtu)(struct rpmsg_endpoint *ept);
>  };
>  
>  struct device *rpmsg_find_device(struct device *parent,
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> index 8e49a3bacfc7..05fd06fc67e9 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -149,6 +149,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 ssize_t virtio_rpmsg_get_mtu(struct rpmsg_endpoint *ept);
>  static struct rpmsg_device *__rpmsg_create_channel(struct virtproc_info *vrp,
>  						   struct rpmsg_channel_info *chinfo);
>  
> @@ -160,6 +161,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_mtu = virtio_rpmsg_get_mtu,
>  };
>  
>  /**
> @@ -696,6 +698,14 @@ static int virtio_rpmsg_trysend_offchannel(struct rpmsg_endpoint *ept, u32 src,
>  	return rpmsg_send_offchannel_raw(rpdev, src, dst, data, len, false);
>  }
>  
> +static ssize_t virtio_rpmsg_get_mtu(struct rpmsg_endpoint *ept)
> +{
> +	struct rpmsg_device *rpdev = ept->rpdev;
> +	struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
> +
> +	return vch->vrp->buf_size - sizeof(struct rpmsg_hdr);
> +}
> +
>  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 d97dcd049f18..990b80fb49ad 100644
> --- a/include/linux/rpmsg.h
> +++ b/include/linux/rpmsg.h
> @@ -186,6 +186,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);
>  
> +ssize_t rpmsg_get_mtu(struct rpmsg_endpoint *ept);
> +
>  #else
>  
>  static inline int rpmsg_register_device(struct rpmsg_device *rpdev)
> @@ -296,6 +298,14 @@ static inline __poll_t rpmsg_poll(struct rpmsg_endpoint *ept,
>  	return 0;
>  }
>  
> +static inline ssize_t rpmsg_get_mtu(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.17.1
> 

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

[Arnaud Pouliquen]

This driver exposes a standard TTY interface on top of the rpmsg
framework through a rpmsg service.

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

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
 Documentation/serial/tty_rpmsg.rst |  15 ++
 drivers/tty/Kconfig                |   9 +
 drivers/tty/Makefile               |   1 +
 drivers/tty/rpmsg_tty.c            | 275 +++++++++++++++++++++++++++++
 4 files changed, 300 insertions(+)
 create mode 100644 Documentation/serial/tty_rpmsg.rst
 create mode 100644 drivers/tty/rpmsg_tty.c

diff --git a/Documentation/serial/tty_rpmsg.rst b/Documentation/serial/tty_rpmsg.rst
new file mode 100644
index 000000000000..b055107866c9
--- /dev/null
+++ b/Documentation/serial/tty_rpmsg.rst
@@ -0,0 +1,15 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+=========
+RPMsg TTY
+=========
+
+The rpmsg tty driver implements serial communication on the RPMsg bus to makes possible for
+user-space programs to send and receive rpmsg messages as a standard tty protocol.
+
+The remote processor can instantiate a new tty by requesting a "rpmsg-tty" RPMsg service.
+
+The "rpmsg-tty" service is directly used for data exchange. No flow control is implemented.
+
+Information related to the RPMsg and associated tty device is available in
+/sys/bus/rpmsg/devices/.
diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
index 23cc988c68a4..5095513029d7 100644
--- a/drivers/tty/Kconfig
+++ b/drivers/tty/Kconfig
@@ -368,6 +368,15 @@ config VCC
 
 source "drivers/tty/hvc/Kconfig"
 
+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.
+
 endif # TTY
 
 source "drivers/tty/serdev/Kconfig"
diff --git a/drivers/tty/Makefile b/drivers/tty/Makefile
index a2bd75fbaaa4..07aca5184a55 100644
--- a/drivers/tty/Makefile
+++ b/drivers/tty/Makefile
@@ -26,5 +26,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..0c99f54c2911
--- /dev/null
+++ b/drivers/tty/rpmsg_tty.c
@@ -0,0 +1,275 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) STMicroelectronics 2021 - All Rights Reserved
+ */
+
+#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;
+
+struct rpmsg_tty_port {
+	struct tty_port		port;	 /* TTY port data */
+	int			id;	 /* TTY rpmsg index */
+	struct rpmsg_device	*rpdev;	 /* rpmsg device */
+};
+
+static int rpmsg_tty_cb(struct rpmsg_device *rpdev, void *data, int len, void *priv, u32 src)
+{
+	struct rpmsg_tty_port *cport = dev_get_drvdata(&rpdev->dev);
+	int copied;
+
+	if (!len)
+		return -EINVAL;
+	copied = tty_insert_flip_string(&cport->port, data, len);
+	if (copied != len)
+		dev_dbg(&rpdev->dev, "Trunc buffer: available space is %d\n",
+			copied);
+	tty_flip_buffer_push(&cport->port);
+
+	return 0;
+}
+
+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;
+	}
+
+	tty->driver_data = cport;
+
+	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 = tty->driver_data;
+	struct rpmsg_device *rpdev;
+	int msg_max_size, msg_size;
+	int ret;
+
+	rpdev = cport->rpdev;
+
+	dev_dbg(&rpdev->dev, "Send msg from tty->index = %d, len = %d\n", tty->index, len);
+
+	msg_max_size = rpmsg_get_mtu(rpdev->ept);
+	if (msg_max_size < 0)
+		return msg_max_size;
+
+	msg_size = min(len, msg_max_size);
+
+	/*
+	 * Use rpmsg_trysend instead of rpmsg_send to send the message so the caller is not
+	 * hung until a rpmsg buffer is available. In such case rpmsg_trysend returns -ENOMEM.
+	 */
+	ret = rpmsg_trysend(rpdev->ept, (void *)buf, msg_size);
+	if (ret) {
+		dev_dbg(&rpdev->dev, "rpmsg_send failed: %d\n", ret);
+		return ret;
+	}
+
+	return msg_size;
+}
+
+static unsigned int rpmsg_tty_write_room(struct tty_struct *tty)
+{
+	struct rpmsg_tty_port *cport = tty->driver_data;
+	int size;
+
+	size = rpmsg_get_mtu(cport->rpdev->ept);
+	if (size < 0)
+		return 0;
+
+	return size;
+}
+
+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,
+};
+
+static struct rpmsg_tty_port *rpmsg_tty_alloc_cport(void)
+{
+	struct rpmsg_tty_port *cport;
+	int err;
+
+	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) {
+		err = cport->id;
+		kfree(cport);
+		return ERR_PTR(err);
+	}
+
+	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 const struct tty_port_operations rpmsg_tty_port_ops = { };
+
+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.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" },
+	{ },
+};
+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:
+	tty_driver_kref_put(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);
+	tty_driver_kref_put(rpmsg_tty_driver);
+	idr_destroy(&tty_idr);
+}
+
+module_init(rpmsg_tty_init);
+module_exit(rpmsg_tty_exit);
+
+MODULE_AUTHOR("Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>");
+MODULE_DESCRIPTION("remote processor messaging tty driver");
+MODULE_LICENSE("GPL v2");
-- 
2.17.1

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

[Bjorn Andersson]

On Thu 30 Sep 09:05 PDT 2021, Arnaud Pouliquen wrote:

> This driver exposes a standard TTY interface on top of the rpmsg
> framework through a rpmsg service.
> 
> This driver supports multi-instances, offering a /dev/ttyRPMSGx entry
> per rpmsg endpoint.
> 

I think this looks pretty good, but it's a while since we discussed
this, reading your patches again makes me wonder:

1) With all the efforts related to virtualization and adding things such
as I2C support there, what are the merits of putting a tty driver ontop
of rpmsg in comparison with directly on virtio - which would be used for
virtualization as well.

2) What prevents you from pointing your userspace tool at an rpmsg_char
endpoint?

Thanks,
Bjorn

> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
>  Documentation/serial/tty_rpmsg.rst |  15 ++
>  drivers/tty/Kconfig                |   9 +
>  drivers/tty/Makefile               |   1 +
>  drivers/tty/rpmsg_tty.c            | 275 +++++++++++++++++++++++++++++
>  4 files changed, 300 insertions(+)
>  create mode 100644 Documentation/serial/tty_rpmsg.rst
>  create mode 100644 drivers/tty/rpmsg_tty.c
> 
> diff --git a/Documentation/serial/tty_rpmsg.rst b/Documentation/serial/tty_rpmsg.rst
> new file mode 100644
> index 000000000000..b055107866c9
> --- /dev/null
> +++ b/Documentation/serial/tty_rpmsg.rst
> @@ -0,0 +1,15 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +=========
> +RPMsg TTY
> +=========
> +
> +The rpmsg tty driver implements serial communication on the RPMsg bus to makes possible for
> +user-space programs to send and receive rpmsg messages as a standard tty protocol.
> +
> +The remote processor can instantiate a new tty by requesting a "rpmsg-tty" RPMsg service.
> +
> +The "rpmsg-tty" service is directly used for data exchange. No flow control is implemented.
> +
> +Information related to the RPMsg and associated tty device is available in
> +/sys/bus/rpmsg/devices/.
> diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
> index 23cc988c68a4..5095513029d7 100644
> --- a/drivers/tty/Kconfig
> +++ b/drivers/tty/Kconfig
> @@ -368,6 +368,15 @@ config VCC
>  
>  source "drivers/tty/hvc/Kconfig"
>  
> +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.
> +
>  endif # TTY
>  
>  source "drivers/tty/serdev/Kconfig"
> diff --git a/drivers/tty/Makefile b/drivers/tty/Makefile
> index a2bd75fbaaa4..07aca5184a55 100644
> --- a/drivers/tty/Makefile
> +++ b/drivers/tty/Makefile
> @@ -26,5 +26,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..0c99f54c2911
> --- /dev/null
> +++ b/drivers/tty/rpmsg_tty.c
> @@ -0,0 +1,275 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) STMicroelectronics 2021 - All Rights Reserved
> + */
> +
> +#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;
> +
> +struct rpmsg_tty_port {
> +	struct tty_port		port;	 /* TTY port data */
> +	int			id;	 /* TTY rpmsg index */
> +	struct rpmsg_device	*rpdev;	 /* rpmsg device */
> +};
> +
> +static int rpmsg_tty_cb(struct rpmsg_device *rpdev, void *data, int len, void *priv, u32 src)
> +{
> +	struct rpmsg_tty_port *cport = dev_get_drvdata(&rpdev->dev);
> +	int copied;
> +
> +	if (!len)
> +		return -EINVAL;
> +	copied = tty_insert_flip_string(&cport->port, data, len);
> +	if (copied != len)
> +		dev_dbg(&rpdev->dev, "Trunc buffer: available space is %d\n",
> +			copied);
> +	tty_flip_buffer_push(&cport->port);
> +
> +	return 0;
> +}
> +
> +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;
> +	}
> +
> +	tty->driver_data = cport;
> +
> +	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 = tty->driver_data;
> +	struct rpmsg_device *rpdev;
> +	int msg_max_size, msg_size;
> +	int ret;
> +
> +	rpdev = cport->rpdev;
> +
> +	dev_dbg(&rpdev->dev, "Send msg from tty->index = %d, len = %d\n", tty->index, len);
> +
> +	msg_max_size = rpmsg_get_mtu(rpdev->ept);
> +	if (msg_max_size < 0)
> +		return msg_max_size;
> +
> +	msg_size = min(len, msg_max_size);
> +
> +	/*
> +	 * Use rpmsg_trysend instead of rpmsg_send to send the message so the caller is not
> +	 * hung until a rpmsg buffer is available. In such case rpmsg_trysend returns -ENOMEM.
> +	 */
> +	ret = rpmsg_trysend(rpdev->ept, (void *)buf, msg_size);
> +	if (ret) {
> +		dev_dbg(&rpdev->dev, "rpmsg_send failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return msg_size;
> +}
> +
> +static unsigned int rpmsg_tty_write_room(struct tty_struct *tty)
> +{
> +	struct rpmsg_tty_port *cport = tty->driver_data;
> +	int size;
> +
> +	size = rpmsg_get_mtu(cport->rpdev->ept);
> +	if (size < 0)
> +		return 0;
> +
> +	return size;
> +}
> +
> +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,
> +};
> +
> +static struct rpmsg_tty_port *rpmsg_tty_alloc_cport(void)
> +{
> +	struct rpmsg_tty_port *cport;
> +	int err;
> +
> +	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) {
> +		err = cport->id;
> +		kfree(cport);
> +		return ERR_PTR(err);
> +	}
> +
> +	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 const struct tty_port_operations rpmsg_tty_port_ops = { };
> +
> +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.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" },
> +	{ },
> +};
> +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:
> +	tty_driver_kref_put(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);
> +	tty_driver_kref_put(rpmsg_tty_driver);
> +	idr_destroy(&tty_idr);
> +}
> +
> +module_init(rpmsg_tty_init);
> +module_exit(rpmsg_tty_exit);
> +
> +MODULE_AUTHOR("Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>");
> +MODULE_DESCRIPTION("remote processor messaging tty driver");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.17.1
> 

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

[Arnaud POULIQUEN]

Hello Bjorn,

On 10/4/21 10:12 PM, Bjorn Andersson wrote:
> On Thu 30 Sep 09:05 PDT 2021, Arnaud Pouliquen wrote:
> 
>> This driver exposes a standard TTY interface on top of the rpmsg
>> framework through a rpmsg service.
>>
>> This driver supports multi-instances, offering a /dev/ttyRPMSGx entry
>> per rpmsg endpoint.
>>
> 
> I think this looks pretty good, but it's a while since we discussed
> this, reading your patches again makes me wonder:

That's fair, the last discussion on the topic was over a year ago.

> 
> 1) With all the efforts related to virtualization and adding things such
> as I2C support there, what are the merits of putting a tty driver ontop
> of rpmsg in comparison with directly on virtio - which would be used for
> virtualization as well.

As mentionned in the cover letter, the main advantage of the RPMsg vs virtio is
that RPMsg is able to mix the services on an unique virtio instance. Using
Virtio console implies to add a new virtio instance for each service (or
instance of a service) with associated mailbox channels.
Taking advantage of the RPMsg service mixing is important for platform (such as
stm32MP1) on which coprocessor is limited in term of memory mapping.

I also expect that this service would be available for some other backend than
the virtio one.

> 
> 2) What prevents you from pointing your userspace tool at an rpmsg_char
> endpoint?

You are right rpmsg_char could be an alternative. But advantage of the TTY is:

- It possible to reuse existing applications/tools relying on TTY devices.
An example is a TTY RPMSG device dedicated for coprocessor traces that can be
simply redirect to a log file or mixed to the kernel logs by scripts.
Another exemple is the ability to ease migration from a 2-processors system
solution (with UART-based IPC) to a system solution with an internal
coprocessor. We received such requirement from some ST customers.

- For rpmsg_char you have to share device between TX and RX (only one fopen
allowed per device), while TTY allows you to manage one device in 2 independent
threads/appliaction.

- TTY framework manages intermediate buffer that allow to free Rx RPMsg buffers.

So rpmsg_char and rpmsg_tty don't seem to me to cover exactly the same requierement.

Thanks,
Arnaud

> 
> Thanks,
> Bjorn
> 
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>> ---
>>  Documentation/serial/tty_rpmsg.rst |  15 ++
>>  drivers/tty/Kconfig                |   9 +
>>  drivers/tty/Makefile               |   1 +
>>  drivers/tty/rpmsg_tty.c            | 275 +++++++++++++++++++++++++++++
>>  4 files changed, 300 insertions(+)
>>  create mode 100644 Documentation/serial/tty_rpmsg.rst
>>  create mode 100644 drivers/tty/rpmsg_tty.c
>>
>> diff --git a/Documentation/serial/tty_rpmsg.rst b/Documentation/serial/tty_rpmsg.rst
>> new file mode 100644
>> index 000000000000..b055107866c9
>> --- /dev/null
>> +++ b/Documentation/serial/tty_rpmsg.rst
>> @@ -0,0 +1,15 @@
>> +.. SPDX-License-Identifier: GPL-2.0
>> +
>> +=========
>> +RPMsg TTY
>> +=========
>> +
>> +The rpmsg tty driver implements serial communication on the RPMsg bus to makes possible for
>> +user-space programs to send and receive rpmsg messages as a standard tty protocol.
>> +
>> +The remote processor can instantiate a new tty by requesting a "rpmsg-tty" RPMsg service.
>> +
>> +The "rpmsg-tty" service is directly used for data exchange. No flow control is implemented.
>> +
>> +Information related to the RPMsg and associated tty device is available in
>> +/sys/bus/rpmsg/devices/.
>> diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
>> index 23cc988c68a4..5095513029d7 100644
>> --- a/drivers/tty/Kconfig
>> +++ b/drivers/tty/Kconfig
>> @@ -368,6 +368,15 @@ config VCC
>>  
>>  source "drivers/tty/hvc/Kconfig"
>>  
>> +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.
>> +
>>  endif # TTY
>>  
>>  source "drivers/tty/serdev/Kconfig"
>> diff --git a/drivers/tty/Makefile b/drivers/tty/Makefile
>> index a2bd75fbaaa4..07aca5184a55 100644
>> --- a/drivers/tty/Makefile
>> +++ b/drivers/tty/Makefile
>> @@ -26,5 +26,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..0c99f54c2911
>> --- /dev/null
>> +++ b/drivers/tty/rpmsg_tty.c
>> @@ -0,0 +1,275 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) STMicroelectronics 2021 - All Rights Reserved
>> + */
>> +
>> +#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;
>> +
>> +struct rpmsg_tty_port {
>> +	struct tty_port		port;	 /* TTY port data */
>> +	int			id;	 /* TTY rpmsg index */
>> +	struct rpmsg_device	*rpdev;	 /* rpmsg device */
>> +};
>> +
>> +static int rpmsg_tty_cb(struct rpmsg_device *rpdev, void *data, int len, void *priv, u32 src)
>> +{
>> +	struct rpmsg_tty_port *cport = dev_get_drvdata(&rpdev->dev);
>> +	int copied;
>> +
>> +	if (!len)
>> +		return -EINVAL;
>> +	copied = tty_insert_flip_string(&cport->port, data, len);
>> +	if (copied != len)
>> +		dev_dbg(&rpdev->dev, "Trunc buffer: available space is %d\n",
>> +			copied);
>> +	tty_flip_buffer_push(&cport->port);
>> +
>> +	return 0;
>> +}
>> +
>> +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;
>> +	}
>> +
>> +	tty->driver_data = cport;
>> +
>> +	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 = tty->driver_data;
>> +	struct rpmsg_device *rpdev;
>> +	int msg_max_size, msg_size;
>> +	int ret;
>> +
>> +	rpdev = cport->rpdev;
>> +
>> +	dev_dbg(&rpdev->dev, "Send msg from tty->index = %d, len = %d\n", tty->index, len);
>> +
>> +	msg_max_size = rpmsg_get_mtu(rpdev->ept);
>> +	if (msg_max_size < 0)
>> +		return msg_max_size;
>> +
>> +	msg_size = min(len, msg_max_size);
>> +
>> +	/*
>> +	 * Use rpmsg_trysend instead of rpmsg_send to send the message so the caller is not
>> +	 * hung until a rpmsg buffer is available. In such case rpmsg_trysend returns -ENOMEM.
>> +	 */
>> +	ret = rpmsg_trysend(rpdev->ept, (void *)buf, msg_size);
>> +	if (ret) {
>> +		dev_dbg(&rpdev->dev, "rpmsg_send failed: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	return msg_size;
>> +}
>> +
>> +static unsigned int rpmsg_tty_write_room(struct tty_struct *tty)
>> +{
>> +	struct rpmsg_tty_port *cport = tty->driver_data;
>> +	int size;
>> +
>> +	size = rpmsg_get_mtu(cport->rpdev->ept);
>> +	if (size < 0)
>> +		return 0;
>> +
>> +	return size;
>> +}
>> +
>> +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,
>> +};
>> +
>> +static struct rpmsg_tty_port *rpmsg_tty_alloc_cport(void)
>> +{
>> +	struct rpmsg_tty_port *cport;
>> +	int err;
>> +
>> +	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) {
>> +		err = cport->id;
>> +		kfree(cport);
>> +		return ERR_PTR(err);
>> +	}
>> +
>> +	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 const struct tty_port_operations rpmsg_tty_port_ops = { };
>> +
>> +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.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" },
>> +	{ },
>> +};
>> +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:
>> +	tty_driver_kref_put(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);
>> +	tty_driver_kref_put(rpmsg_tty_driver);
>> +	idr_destroy(&tty_idr);
>> +}
>> +
>> +module_init(rpmsg_tty_init);
>> +module_exit(rpmsg_tty_exit);
>> +
>> +MODULE_AUTHOR("Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>");
>> +MODULE_DESCRIPTION("remote processor messaging tty driver");
>> +MODULE_LICENSE("GPL v2");
>> -- 
>> 2.17.1
>>

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

[Bjorn Andersson]

On Tue 05 Oct 09:00 PDT 2021, Arnaud POULIQUEN wrote:

> Hello Bjorn,
> 
> On 10/4/21 10:12 PM, Bjorn Andersson wrote:
> > On Thu 30 Sep 09:05 PDT 2021, Arnaud Pouliquen wrote:
> > 
> >> This driver exposes a standard TTY interface on top of the rpmsg
> >> framework through a rpmsg service.
> >>
> >> This driver supports multi-instances, offering a /dev/ttyRPMSGx entry
> >> per rpmsg endpoint.
> >>
> > 
> > I think this looks pretty good, but it's a while since we discussed
> > this, reading your patches again makes me wonder:
> 
> That's fair, the last discussion on the topic was over a year ago.
> 
> > 
> > 1) With all the efforts related to virtualization and adding things such
> > as I2C support there, what are the merits of putting a tty driver ontop
> > of rpmsg in comparison with directly on virtio - which would be used for
> > virtualization as well.
> 
> As mentionned in the cover letter, the main advantage of the RPMsg vs virtio is
> that RPMsg is able to mix the services on an unique virtio instance. Using
> Virtio console implies to add a new virtio instance for each service (or
> instance of a service) with associated mailbox channels.
> Taking advantage of the RPMsg service mixing is important for platform (such as
> stm32MP1) on which coprocessor is limited in term of memory mapping.
> 

The limitations related to mapping additional virtio pipes does seem
like a reasonable one. I just hope we don't end up duplicating much of
the virtualization effort, with the recently concluded I2C client
support coming to mind.

> I also expect that this service would be available for some other backend than
> the virtio one.
> 

Last time we discussed this I believe I mentioned the AT command
channels found in Qualcomm modems. Since then that got pushed into
drivers/net/wwan, for the few other cases rpmsg_char works fine.

> > 
> > 2) What prevents you from pointing your userspace tool at an rpmsg_char
> > endpoint?
> 
> You are right rpmsg_char could be an alternative. But advantage of the TTY is:
> 
> - It possible to reuse existing applications/tools relying on TTY devices.
> An example is a TTY RPMSG device dedicated for coprocessor traces that can be
> simply redirect to a log file or mixed to the kernel logs by scripts.
> Another exemple is the ability to ease migration from a 2-processors system
> solution (with UART-based IPC) to a system solution with an internal
> coprocessor. We received such requirement from some ST customers.
> 

I presume the transport itself provided by rpmsg_char would work just
fine for this, but that these applications expects something from the
tty framework, with its known ioctls etc?

> - For rpmsg_char you have to share device between TX and RX (only one fopen
> allowed per device), while TTY allows you to manage one device in 2 independent
> threads/appliaction.
> 

At least in the Qualcomm case, where the channels have a state and
clients opening and closing the rpmsg_char will affect that state it
simplifies things a lot not to support a multi-client model. So I think
there's merit to this difference.

> - TTY framework manages intermediate buffer that allow to free Rx RPMsg buffers.
> 

rpmsg_char does the same thing, by putting incoming messages on
epddev->queue.

> So rpmsg_char and rpmsg_tty don't seem to me to cover exactly the same requierement.
> 

No, you're right there's some differences here.

Regards,
Bjorn

> Thanks,
> Arnaud
> 
> > 
> > Thanks,
> > Bjorn
> > 
> >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> >> ---
> >>  Documentation/serial/tty_rpmsg.rst |  15 ++
> >>  drivers/tty/Kconfig                |   9 +
> >>  drivers/tty/Makefile               |   1 +
> >>  drivers/tty/rpmsg_tty.c            | 275 +++++++++++++++++++++++++++++
> >>  4 files changed, 300 insertions(+)
> >>  create mode 100644 Documentation/serial/tty_rpmsg.rst
> >>  create mode 100644 drivers/tty/rpmsg_tty.c
> >>
> >> diff --git a/Documentation/serial/tty_rpmsg.rst b/Documentation/serial/tty_rpmsg.rst
> >> new file mode 100644
> >> index 000000000000..b055107866c9
> >> --- /dev/null
> >> +++ b/Documentation/serial/tty_rpmsg.rst
> >> @@ -0,0 +1,15 @@
> >> +.. SPDX-License-Identifier: GPL-2.0
> >> +
> >> +=========
> >> +RPMsg TTY
> >> +=========
> >> +
> >> +The rpmsg tty driver implements serial communication on the RPMsg bus to makes possible for
> >> +user-space programs to send and receive rpmsg messages as a standard tty protocol.
> >> +
> >> +The remote processor can instantiate a new tty by requesting a "rpmsg-tty" RPMsg service.
> >> +
> >> +The "rpmsg-tty" service is directly used for data exchange. No flow control is implemented.
> >> +
> >> +Information related to the RPMsg and associated tty device is available in
> >> +/sys/bus/rpmsg/devices/.
> >> diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
> >> index 23cc988c68a4..5095513029d7 100644
> >> --- a/drivers/tty/Kconfig
> >> +++ b/drivers/tty/Kconfig
> >> @@ -368,6 +368,15 @@ config VCC
> >>  
> >>  source "drivers/tty/hvc/Kconfig"
> >>  
> >> +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.
> >> +
> >>  endif # TTY
> >>  
> >>  source "drivers/tty/serdev/Kconfig"
> >> diff --git a/drivers/tty/Makefile b/drivers/tty/Makefile
> >> index a2bd75fbaaa4..07aca5184a55 100644
> >> --- a/drivers/tty/Makefile
> >> +++ b/drivers/tty/Makefile
> >> @@ -26,5 +26,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..0c99f54c2911
> >> --- /dev/null
> >> +++ b/drivers/tty/rpmsg_tty.c
> >> @@ -0,0 +1,275 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Copyright (C) STMicroelectronics 2021 - All Rights Reserved
> >> + */
> >> +
> >> +#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;
> >> +
> >> +struct rpmsg_tty_port {
> >> +	struct tty_port		port;	 /* TTY port data */
> >> +	int			id;	 /* TTY rpmsg index */
> >> +	struct rpmsg_device	*rpdev;	 /* rpmsg device */
> >> +};
> >> +
> >> +static int rpmsg_tty_cb(struct rpmsg_device *rpdev, void *data, int len, void *priv, u32 src)
> >> +{
> >> +	struct rpmsg_tty_port *cport = dev_get_drvdata(&rpdev->dev);
> >> +	int copied;
> >> +
> >> +	if (!len)
> >> +		return -EINVAL;
> >> +	copied = tty_insert_flip_string(&cport->port, data, len);
> >> +	if (copied != len)
> >> +		dev_dbg(&rpdev->dev, "Trunc buffer: available space is %d\n",
> >> +			copied);
> >> +	tty_flip_buffer_push(&cport->port);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +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;
> >> +	}
> >> +
> >> +	tty->driver_data = cport;
> >> +
> >> +	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 = tty->driver_data;
> >> +	struct rpmsg_device *rpdev;
> >> +	int msg_max_size, msg_size;
> >> +	int ret;
> >> +
> >> +	rpdev = cport->rpdev;
> >> +
> >> +	dev_dbg(&rpdev->dev, "Send msg from tty->index = %d, len = %d\n", tty->index, len);
> >> +
> >> +	msg_max_size = rpmsg_get_mtu(rpdev->ept);
> >> +	if (msg_max_size < 0)
> >> +		return msg_max_size;
> >> +
> >> +	msg_size = min(len, msg_max_size);
> >> +
> >> +	/*
> >> +	 * Use rpmsg_trysend instead of rpmsg_send to send the message so the caller is not
> >> +	 * hung until a rpmsg buffer is available. In such case rpmsg_trysend returns -ENOMEM.
> >> +	 */
> >> +	ret = rpmsg_trysend(rpdev->ept, (void *)buf, msg_size);
> >> +	if (ret) {
> >> +		dev_dbg(&rpdev->dev, "rpmsg_send failed: %d\n", ret);
> >> +		return ret;
> >> +	}
> >> +
> >> +	return msg_size;
> >> +}
> >> +
> >> +static unsigned int rpmsg_tty_write_room(struct tty_struct *tty)
> >> +{
> >> +	struct rpmsg_tty_port *cport = tty->driver_data;
> >> +	int size;
> >> +
> >> +	size = rpmsg_get_mtu(cport->rpdev->ept);
> >> +	if (size < 0)
> >> +		return 0;
> >> +
> >> +	return size;
> >> +}
> >> +
> >> +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,
> >> +};
> >> +
> >> +static struct rpmsg_tty_port *rpmsg_tty_alloc_cport(void)
> >> +{
> >> +	struct rpmsg_tty_port *cport;
> >> +	int err;
> >> +
> >> +	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) {
> >> +		err = cport->id;
> >> +		kfree(cport);
> >> +		return ERR_PTR(err);
> >> +	}
> >> +
> >> +	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 const struct tty_port_operations rpmsg_tty_port_ops = { };
> >> +
> >> +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.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" },
> >> +	{ },
> >> +};
> >> +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:
> >> +	tty_driver_kref_put(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);
> >> +	tty_driver_kref_put(rpmsg_tty_driver);
> >> +	idr_destroy(&tty_idr);
> >> +}
> >> +
> >> +module_init(rpmsg_tty_init);
> >> +module_exit(rpmsg_tty_exit);
> >> +
> >> +MODULE_AUTHOR("Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>");
> >> +MODULE_DESCRIPTION("remote processor messaging tty driver");
> >> +MODULE_LICENSE("GPL v2");
> >> -- 
> >> 2.17.1
> >>

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

[Arnaud POULIQUEN]

Hello Bjorn,

On 10/5/21 6:24 PM, Bjorn Andersson wrote:
> On Tue 05 Oct 09:00 PDT 2021, Arnaud POULIQUEN wrote:
> 
>> Hello Bjorn,
>>
>> On 10/4/21 10:12 PM, Bjorn Andersson wrote:
>>> On Thu 30 Sep 09:05 PDT 2021, Arnaud Pouliquen wrote:
>>>
>>>> This driver exposes a standard TTY interface on top of the rpmsg
>>>> framework through a rpmsg service.
>>>>
>>>> This driver supports multi-instances, offering a /dev/ttyRPMSGx entry
>>>> per rpmsg endpoint.
>>>>
>>>
>>> I think this looks pretty good, but it's a while since we discussed
>>> this, reading your patches again makes me wonder:
>>
>> That's fair, the last discussion on the topic was over a year ago.
>>
>>>
>>> 1) With all the efforts related to virtualization and adding things such
>>> as I2C support there, what are the merits of putting a tty driver ontop
>>> of rpmsg in comparison with directly on virtio - which would be used for
>>> virtualization as well.
>>
>> As mentionned in the cover letter, the main advantage of the RPMsg vs virtio is
>> that RPMsg is able to mix the services on an unique virtio instance. Using
>> Virtio console implies to add a new virtio instance for each service (or
>> instance of a service) with associated mailbox channels.
>> Taking advantage of the RPMsg service mixing is important for platform (such as
>> stm32MP1) on which coprocessor is limited in term of memory mapping.
>>
> 
> The limitations related to mapping additional virtio pipes does seem
> like a reasonable one. I just hope we don't end up duplicating much of
> the virtualization effort, with the recently concluded I2C client
> support coming to mind.

Difficult to have a prediction on this.
This is something that should be coming with rational.The I2C management would
be a good example. If we consider a I2C bus managed by the coprocessor. A main
processor can have to access to a device on this bus.
Is limited number of access to the device justify to reserve a dedicated memory
area and a mailbox channel just for few device configurations on runtime?

Anyway supported more virtio services in remoteproc and OpenAMP library seems
something like a good middle term objective. The work I started around
remoteproc virtio, I guess, is going in this direction.

> 
>> I also expect that this service would be available for some other backend than
>> the virtio one.
>>
> 
> Last time we discussed this I believe I mentioned the AT command
> channels found in Qualcomm modems. Since then that got pushed into
> drivers/net/wwan, for the few other cases rpmsg_char works fine.
> 
>>>
>>> 2) What prevents you from pointing your userspace tool at an rpmsg_char
>>> endpoint?
>>
>> You are right rpmsg_char could be an alternative. But advantage of the TTY is:
>>
>> - It possible to reuse existing applications/tools relying on TTY devices.
>> An example is a TTY RPMSG device dedicated for coprocessor traces that can be
>> simply redirect to a log file or mixed to the kernel logs by scripts.
>> Another exemple is the ability to ease migration from a 2-processors system
>> solution (with UART-based IPC) to a system solution with an internal
>> coprocessor. We received such requirement from some ST customers.
>>
> 
> I presume the transport itself provided by rpmsg_char would work just
> fine for this, but that these applications expects something from the
> tty framework, with its known ioctls etc?

The rts mechanism i implemented in previous version is also something
interesting to add on for the flow control.

> 
>> - For rpmsg_char you have to share device between TX and RX (only one fopen
>> allowed per device), while TTY allows you to manage one device in 2 independent
>> threads/appliaction.
>>
> 
> At least in the Qualcomm case, where the channels have a state and
> clients opening and closing the rpmsg_char will affect that state it
> simplifies things a lot not to support a multi-client model. So I think
> there's merit to this difference.
> 
>> - TTY framework manages intermediate buffer that allow to free Rx RPMsg buffers.
>>
> 
> rpmsg_char does the same thing, by putting incoming messages on
> epddev->queue.

That's right, I forgot that, thanks for pointing it out.

> 
>> So rpmsg_char and rpmsg_tty don't seem to me to cover exactly the same requierement.
>>
> 
> No, you're right there's some differences here.

So can I consider that there is no blocking point on your side to move forward
with this Rpmsg service?

Thanks,
Arnaud
> 
> Regards,
> Bjorn
> 
>> Thanks,
>> Arnaud
>>
>>>
>>> Thanks,
>>> Bjorn
>>>
>>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>>>> ---
>>>>  Documentation/serial/tty_rpmsg.rst |  15 ++
>>>>  drivers/tty/Kconfig                |   9 +
>>>>  drivers/tty/Makefile               |   1 +
>>>>  drivers/tty/rpmsg_tty.c            | 275 +++++++++++++++++++++++++++++
>>>>  4 files changed, 300 insertions(+)
>>>>  create mode 100644 Documentation/serial/tty_rpmsg.rst
>>>>  create mode 100644 drivers/tty/rpmsg_tty.c
>>>>
>>>> diff --git a/Documentation/serial/tty_rpmsg.rst b/Documentation/serial/tty_rpmsg.rst
>>>> new file mode 100644
>>>> index 000000000000..b055107866c9
>>>> --- /dev/null
>>>> +++ b/Documentation/serial/tty_rpmsg.rst
>>>> @@ -0,0 +1,15 @@
>>>> +.. SPDX-License-Identifier: GPL-2.0
>>>> +
>>>> +=========
>>>> +RPMsg TTY
>>>> +=========
>>>> +
>>>> +The rpmsg tty driver implements serial communication on the RPMsg bus to makes possible for
>>>> +user-space programs to send and receive rpmsg messages as a standard tty protocol.
>>>> +
>>>> +The remote processor can instantiate a new tty by requesting a "rpmsg-tty" RPMsg service.
>>>> +
>>>> +The "rpmsg-tty" service is directly used for data exchange. No flow control is implemented.
>>>> +
>>>> +Information related to the RPMsg and associated tty device is available in
>>>> +/sys/bus/rpmsg/devices/.
>>>> diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
>>>> index 23cc988c68a4..5095513029d7 100644
>>>> --- a/drivers/tty/Kconfig
>>>> +++ b/drivers/tty/Kconfig
>>>> @@ -368,6 +368,15 @@ config VCC
>>>>  
>>>>  source "drivers/tty/hvc/Kconfig"
>>>>  
>>>> +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.
>>>> +
>>>>  endif # TTY
>>>>  
>>>>  source "drivers/tty/serdev/Kconfig"
>>>> diff --git a/drivers/tty/Makefile b/drivers/tty/Makefile
>>>> index a2bd75fbaaa4..07aca5184a55 100644
>>>> --- a/drivers/tty/Makefile
>>>> +++ b/drivers/tty/Makefile
>>>> @@ -26,5 +26,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..0c99f54c2911
>>>> --- /dev/null
>>>> +++ b/drivers/tty/rpmsg_tty.c
>>>> @@ -0,0 +1,275 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * Copyright (C) STMicroelectronics 2021 - All Rights Reserved
>>>> + */
>>>> +
>>>> +#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;
>>>> +
>>>> +struct rpmsg_tty_port {
>>>> +	struct tty_port		port;	 /* TTY port data */
>>>> +	int			id;	 /* TTY rpmsg index */
>>>> +	struct rpmsg_device	*rpdev;	 /* rpmsg device */
>>>> +};
>>>> +
>>>> +static int rpmsg_tty_cb(struct rpmsg_device *rpdev, void *data, int len, void *priv, u32 src)
>>>> +{
>>>> +	struct rpmsg_tty_port *cport = dev_get_drvdata(&rpdev->dev);
>>>> +	int copied;
>>>> +
>>>> +	if (!len)
>>>> +		return -EINVAL;
>>>> +	copied = tty_insert_flip_string(&cport->port, data, len);
>>>> +	if (copied != len)
>>>> +		dev_dbg(&rpdev->dev, "Trunc buffer: available space is %d\n",
>>>> +			copied);
>>>> +	tty_flip_buffer_push(&cport->port);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +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;
>>>> +	}
>>>> +
>>>> +	tty->driver_data = cport;
>>>> +
>>>> +	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 = tty->driver_data;
>>>> +	struct rpmsg_device *rpdev;
>>>> +	int msg_max_size, msg_size;
>>>> +	int ret;
>>>> +
>>>> +	rpdev = cport->rpdev;
>>>> +
>>>> +	dev_dbg(&rpdev->dev, "Send msg from tty->index = %d, len = %d\n", tty->index, len);
>>>> +
>>>> +	msg_max_size = rpmsg_get_mtu(rpdev->ept);
>>>> +	if (msg_max_size < 0)
>>>> +		return msg_max_size;
>>>> +
>>>> +	msg_size = min(len, msg_max_size);
>>>> +
>>>> +	/*
>>>> +	 * Use rpmsg_trysend instead of rpmsg_send to send the message so the caller is not
>>>> +	 * hung until a rpmsg buffer is available. In such case rpmsg_trysend returns -ENOMEM.
>>>> +	 */
>>>> +	ret = rpmsg_trysend(rpdev->ept, (void *)buf, msg_size);
>>>> +	if (ret) {
>>>> +		dev_dbg(&rpdev->dev, "rpmsg_send failed: %d\n", ret);
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	return msg_size;
>>>> +}
>>>> +
>>>> +static unsigned int rpmsg_tty_write_room(struct tty_struct *tty)
>>>> +{
>>>> +	struct rpmsg_tty_port *cport = tty->driver_data;
>>>> +	int size;
>>>> +
>>>> +	size = rpmsg_get_mtu(cport->rpdev->ept);
>>>> +	if (size < 0)
>>>> +		return 0;
>>>> +
>>>> +	return size;
>>>> +}
>>>> +
>>>> +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,
>>>> +};
>>>> +
>>>> +static struct rpmsg_tty_port *rpmsg_tty_alloc_cport(void)
>>>> +{
>>>> +	struct rpmsg_tty_port *cport;
>>>> +	int err;
>>>> +
>>>> +	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) {
>>>> +		err = cport->id;
>>>> +		kfree(cport);
>>>> +		return ERR_PTR(err);
>>>> +	}
>>>> +
>>>> +	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 const struct tty_port_operations rpmsg_tty_port_ops = { };
>>>> +
>>>> +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.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" },
>>>> +	{ },
>>>> +};
>>>> +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:
>>>> +	tty_driver_kref_put(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);
>>>> +	tty_driver_kref_put(rpmsg_tty_driver);
>>>> +	idr_destroy(&tty_idr);
>>>> +}
>>>> +
>>>> +module_init(rpmsg_tty_init);
>>>> +module_exit(rpmsg_tty_exit);
>>>> +
>>>> +MODULE_AUTHOR("Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>");
>>>> +MODULE_DESCRIPTION("remote processor messaging tty driver");
>>>> +MODULE_LICENSE("GPL v2");
>>>> -- 
>>>> 2.17.1
>>>>

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

[Greg Kroah-Hartman]

On Thu, Sep 30, 2021 at 06:05:20PM +0200, Arnaud Pouliquen wrote:
> This driver exposes a standard TTY interface on top of the rpmsg
> framework through a rpmsg service.
> 
> This driver supports multi-instances, offering a /dev/ttyRPMSGx entry
> per rpmsg endpoint.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
>  Documentation/serial/tty_rpmsg.rst |  15 ++
>  drivers/tty/Kconfig                |   9 +
>  drivers/tty/Makefile               |   1 +
>  drivers/tty/rpmsg_tty.c            | 275 +++++++++++++++++++++++++++++
>  4 files changed, 300 insertions(+)
>  create mode 100644 Documentation/serial/tty_rpmsg.rst
>  create mode 100644 drivers/tty/rpmsg_tty.c
> 
> diff --git a/Documentation/serial/tty_rpmsg.rst b/Documentation/serial/tty_rpmsg.rst
> new file mode 100644
> index 000000000000..b055107866c9
> --- /dev/null
> +++ b/Documentation/serial/tty_rpmsg.rst
> @@ -0,0 +1,15 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +=========
> +RPMsg TTY
> +=========
> +
> +The rpmsg tty driver implements serial communication on the RPMsg bus to makes possible for
> +user-space programs to send and receive rpmsg messages as a standard tty protocol.
> +
> +The remote processor can instantiate a new tty by requesting a "rpmsg-tty" RPMsg service.
> +
> +The "rpmsg-tty" service is directly used for data exchange. No flow control is implemented.
> +
> +Information related to the RPMsg and associated tty device is available in
> +/sys/bus/rpmsg/devices/.


Why is this file needed?  Nothing references it, and this would be the
only file in this directory.

> diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
> index 23cc988c68a4..5095513029d7 100644
> --- a/drivers/tty/Kconfig
> +++ b/drivers/tty/Kconfig
> @@ -368,6 +368,15 @@ config VCC
>  
>  source "drivers/tty/hvc/Kconfig"
>  
> +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.

What is the module name going to be?


> +
>  endif # TTY
>  
>  source "drivers/tty/serdev/Kconfig"
> diff --git a/drivers/tty/Makefile b/drivers/tty/Makefile
> index a2bd75fbaaa4..07aca5184a55 100644
> --- a/drivers/tty/Makefile
> +++ b/drivers/tty/Makefile
> @@ -26,5 +26,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..0c99f54c2911
> --- /dev/null
> +++ b/drivers/tty/rpmsg_tty.c
> @@ -0,0 +1,275 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) STMicroelectronics 2021 - All Rights Reserved

Copyright needs a year, right?

> + */
> +
> +#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

Why have a max at all?


> +
> +static DEFINE_IDR(tty_idr);	/* tty instance id */
> +static DEFINE_MUTEX(idr_lock);	/* protects tty_idr */

I didn't think an idr needed a lock anymore, are you sure this is
needed?


> +
> +static struct tty_driver *rpmsg_tty_driver;
> +
> +struct rpmsg_tty_port {
> +	struct tty_port		port;	 /* TTY port data */
> +	int			id;	 /* TTY rpmsg index */
> +	struct rpmsg_device	*rpdev;	 /* rpmsg device */
> +};
> +
> +static int rpmsg_tty_cb(struct rpmsg_device *rpdev, void *data, int len, void *priv, u32 src)
> +{
> +	struct rpmsg_tty_port *cport = dev_get_drvdata(&rpdev->dev);
> +	int copied;
> +
> +	if (!len)
> +		return -EINVAL;

How can len be 0?


> +	copied = tty_insert_flip_string(&cport->port, data, len);
> +	if (copied != len)
> +		dev_dbg(&rpdev->dev, "Trunc buffer: available space is %d\n",
> +			copied);

Is this the proper error handling?


> +	tty_flip_buffer_push(&cport->port);

Shouldn't you return the number of bytes sent?

> +
> +	return 0;
> +}
> +
> +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");

How can this happen?


> +		return -ENODEV;
> +	}
> +
> +	tty->driver_data = cport;
> +
> +	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 = tty->driver_data;
> +	struct rpmsg_device *rpdev;
> +	int msg_max_size, msg_size;
> +	int ret;
> +
> +	rpdev = cport->rpdev;
> +
> +	dev_dbg(&rpdev->dev, "Send msg from tty->index = %d, len = %d\n", tty->index, len);

ftrace is your friend, is this really still needed?

thanks,

greg k-h

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

[Arnaud POULIQUEN]

Hello Greg,

On 10/5/21 2:59 PM, Greg Kroah-Hartman wrote:
> On Thu, Sep 30, 2021 at 06:05:20PM +0200, Arnaud Pouliquen wrote:
>> This driver exposes a standard TTY interface on top of the rpmsg
>> framework through a rpmsg service.
>>
>> This driver supports multi-instances, offering a /dev/ttyRPMSGx entry
>> per rpmsg endpoint.
>>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>> ---
>>  Documentation/serial/tty_rpmsg.rst |  15 ++
>>  drivers/tty/Kconfig                |   9 +
>>  drivers/tty/Makefile               |   1 +
>>  drivers/tty/rpmsg_tty.c            | 275 +++++++++++++++++++++++++++++
>>  4 files changed, 300 insertions(+)
>>  create mode 100644 Documentation/serial/tty_rpmsg.rst
>>  create mode 100644 drivers/tty/rpmsg_tty.c
>>
>> diff --git a/Documentation/serial/tty_rpmsg.rst b/Documentation/serial/tty_rpmsg.rst
>> new file mode 100644
>> index 000000000000..b055107866c9
>> --- /dev/null
>> +++ b/Documentation/serial/tty_rpmsg.rst
>> @@ -0,0 +1,15 @@
>> +.. SPDX-License-Identifier: GPL-2.0
>> +
>> +=========
>> +RPMsg TTY
>> +=========
>> +
>> +The rpmsg tty driver implements serial communication on the RPMsg bus to makes possible for
>> +user-space programs to send and receive rpmsg messages as a standard tty protocol.
>> +
>> +The remote processor can instantiate a new tty by requesting a "rpmsg-tty" RPMsg service.
>> +
>> +The "rpmsg-tty" service is directly used for data exchange. No flow control is implemented.
>> +
>> +Information related to the RPMsg and associated tty device is available in
>> +/sys/bus/rpmsg/devices/.
> 
> 
> Why is this file needed?  Nothing references it, and this would be the
> only file in this directory.

This file is created by the RPMsg framework, it allows to have information about
RPMsg endpoint addresses associated to the rpmsg tty service instance.
I can add this additional information to clarify the sentence.

> 
>> diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
>> index 23cc988c68a4..5095513029d7 100644
>> --- a/drivers/tty/Kconfig
>> +++ b/drivers/tty/Kconfig
>> @@ -368,6 +368,15 @@ config VCC
>>  
>>  source "drivers/tty/hvc/Kconfig"
>>  
>> +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.
> 
> What is the module name going to be?

I will add information

> 
> 
>> +
>>  endif # TTY
>>  
>>  source "drivers/tty/serdev/Kconfig"
>> diff --git a/drivers/tty/Makefile b/drivers/tty/Makefile
>> index a2bd75fbaaa4..07aca5184a55 100644
>> --- a/drivers/tty/Makefile
>> +++ b/drivers/tty/Makefile
>> @@ -26,5 +26,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..0c99f54c2911
>> --- /dev/null
>> +++ b/drivers/tty/rpmsg_tty.c
>> @@ -0,0 +1,275 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) STMicroelectronics 2021 - All Rights Reserved
> 
> Copyright needs a year, right?

The year is present, but indicated after the company, to inverse

> 
>> + */
>> +
>> +#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
> 
> Why have a max at all?

This is linked to tty_alloc_driver in the module init
It is multi instance but need pre-allocation.
I did not find a proper way to do this. Any suggestion is welcome.

> 
> 
>> +
>> +static DEFINE_IDR(tty_idr);	/* tty instance id */
>> +static DEFINE_MUTEX(idr_lock);	/* protects tty_idr */
> 
> I didn't think an idr needed a lock anymore, are you sure this is
> needed?

recognized in ird_alloc header for multi instance:
https://elixir.bootlin.com/linux/v5.15-rc1/source/lib/idr.c#L60

> 
> 
>> +
>> +static struct tty_driver *rpmsg_tty_driver;
>> +
>> +struct rpmsg_tty_port {
>> +	struct tty_port		port;	 /* TTY port data */
>> +	int			id;	 /* TTY rpmsg index */
>> +	struct rpmsg_device	*rpdev;	 /* rpmsg device */
>> +};
>> +
>> +static int rpmsg_tty_cb(struct rpmsg_device *rpdev, void *data, int len, void *priv, u32 src)
>> +{
>> +	struct rpmsg_tty_port *cport = dev_get_drvdata(&rpdev->dev);
>> +	int copied;
>> +
>> +	if (!len)
>> +		return -EINVAL;
> 
> How can len be 0?

In the RPMsg framework, nothing prevents a RPMsg with len = 0 (means header with
no payload).
It should be possible that the remote processor firmware bug generates such message.

> 
> 
>> +	copied = tty_insert_flip_string(&cport->port, data, len);
>> +	if (copied != len)
>> +		dev_dbg(&rpdev->dev, "Trunc buffer: available space is %d\n",
>> +			copied);
> 
> Is this the proper error handling?

Right, as a part of the message is lost, should be an error.

> 
> 
>> +	tty_flip_buffer_push(&cport->port);
> 
> Shouldn't you return the number of bytes sent?

For the RPMsg framework you mean? No, because for another RPMsg services, it
might not make sense. Return 0 seems to me more generic.
In any case today the RPMsg framework doesn't test the callback return,
associated action would depend on the service.

> 
>> +
>> +	return 0;
>> +}
>> +
>> +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");
> 
> How can this happen?

Right over protection!

> 
> 
>> +		return -ENODEV;
>> +	}
>> +
>> +	tty->driver_data = cport;
>> +
>> +	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 = tty->driver_data;
>> +	struct rpmsg_device *rpdev;
>> +	int msg_max_size, msg_size;
>> +	int ret;
>> +
>> +	rpdev = cport->rpdev;
>> +
>> +	dev_dbg(&rpdev->dev, "Send msg from tty->index = %d, len = %d\n", tty->index, len);
> 
> ftrace is your friend, is this really still needed?
> 

Yes, but unfortunately not the friend of all our customers :)
I will clean this log. The RPMsg dynamic traces already allows to trace the
messages, which should be enough for a first level of debug.

I will send a new revision integrating your comments.

Thanks & Regards,
Arnaud

> thanks,
> 
> greg k-h
> 

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

[Greg Kroah-Hartman]

On Tue, Oct 05, 2021 at 06:03:25PM +0200, Arnaud POULIQUEN wrote:
> Hello Greg,
> 
> On 10/5/21 2:59 PM, Greg Kroah-Hartman wrote:
> > On Thu, Sep 30, 2021 at 06:05:20PM +0200, Arnaud Pouliquen wrote:
> >> This driver exposes a standard TTY interface on top of the rpmsg
> >> framework through a rpmsg service.
> >>
> >> This driver supports multi-instances, offering a /dev/ttyRPMSGx entry
> >> per rpmsg endpoint.
> >>
> >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> >> ---
> >>  Documentation/serial/tty_rpmsg.rst |  15 ++
> >>  drivers/tty/Kconfig                |   9 +
> >>  drivers/tty/Makefile               |   1 +
> >>  drivers/tty/rpmsg_tty.c            | 275 +++++++++++++++++++++++++++++
> >>  4 files changed, 300 insertions(+)
> >>  create mode 100644 Documentation/serial/tty_rpmsg.rst
> >>  create mode 100644 drivers/tty/rpmsg_tty.c
> >>
> >> diff --git a/Documentation/serial/tty_rpmsg.rst b/Documentation/serial/tty_rpmsg.rst
> >> new file mode 100644
> >> index 000000000000..b055107866c9
> >> --- /dev/null
> >> +++ b/Documentation/serial/tty_rpmsg.rst
> >> @@ -0,0 +1,15 @@
> >> +.. SPDX-License-Identifier: GPL-2.0
> >> +
> >> +=========
> >> +RPMsg TTY
> >> +=========
> >> +
> >> +The rpmsg tty driver implements serial communication on the RPMsg bus to makes possible for
> >> +user-space programs to send and receive rpmsg messages as a standard tty protocol.
> >> +
> >> +The remote processor can instantiate a new tty by requesting a "rpmsg-tty" RPMsg service.
> >> +
> >> +The "rpmsg-tty" service is directly used for data exchange. No flow control is implemented.
> >> +
> >> +Information related to the RPMsg and associated tty device is available in
> >> +/sys/bus/rpmsg/devices/.
> > 
> > 
> > Why is this file needed?  Nothing references it, and this would be the
> > only file in this directory.
> 
> This file is created by the RPMsg framework, it allows to have information about
> RPMsg endpoint addresses associated to the rpmsg tty service instance.
> I can add this additional information to clarify the sentence.

As you are not tying in this into the kernel documentation build at all,
it makes no sense to add it.

Just use normal kernel-doc comments in your .c file and tie _THAT_ into
the kernel documentation build.  No need for a .rst file at all.

thanks,

greg k-h