[v7,2/2] tty: add rpmsg driver

Commit Message

Arnaud POULIQUEN March 24, 2020, 5:04 p.m. UTC

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 <[e-mail]>
---
 Documentation/serial/tty_rpmsg.rst |  45 ++++
 drivers/tty/Kconfig                |   9 +
 drivers/tty/Makefile               |   1 +
 drivers/tty/rpmsg_tty.c            | 417 +++++++++++++++++++++++++++++
 4 files changed, 472 insertions(+)
 create mode 100644 Documentation/serial/tty_rpmsg.rst
 create mode 100644 drivers/tty/rpmsg_tty.c

Comments

Greg Kroah-Hartman March 24, 2020, 5:18 p.m. UTC | #1

On Tue, Mar 24, 2020 at 06:04:07PM +0100, 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 <[e-mail]>
> ---
>  Documentation/serial/tty_rpmsg.rst |  45 ++++
>  drivers/tty/Kconfig                |   9 +
>  drivers/tty/Makefile               |   1 +
>  drivers/tty/rpmsg_tty.c            | 417 +++++++++++++++++++++++++++++
>  4 files changed, 472 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..fc1d3fba73c5
> --- /dev/null
> +++ b/Documentation/serial/tty_rpmsg.rst
> @@ -0,0 +1,45 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +=============
> +The 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.

Can you wrap your lines properly for this file?

thanks,

greg k-h

Joe Perches March 24, 2020, 5:23 p.m. UTC | #2

On Tue, 2020-03-24 at 18:04 +0100, 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.

trivial notes:

> diff --git a/Documentation/serial/tty_rpmsg.rst b/Documentation/serial/tty_rpmsg.rst
[]
> +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.

Very long text lines missing newlines?

[]
> +To be compliant with this driver, the remote firmware must create its data end point associated with the "rpmsg-tty-raw" service.
[]
> +To be compatible with this driver, the remote firmware must create or use its end point associated with "rpmsg-tty-ctrl" service, plus a second endpoint for the data flow.
> +On Linux rpmsg_tty probes, the data endpoint address and the CTS (set to disable)

[]

> diff --git a/drivers/tty/rpmsg_tty.c b/drivers/tty/rpmsg_tty.c
[]
> +typedef void (*rpmsg_tty_rx_cb_t)(struct rpmsg_device *, void *, int, void *,
> +				  u32);

unused typedef?

[]

> +static int __init rpmsg_tty_init(void)
> +{
[]
> +	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;
> +	}

Might use vsprintf extension %pe

		pr_err("Couldn't install rpmsg tty driver: %pe\n", ERR_PTR(err));

> +	err = register_rpmsg_driver(&rpmsg_tty_rpmsg_drv);
> +	if (err < 0) {
> +		pr_err("Couldn't register rpmsg tty driver: err %d\n", err);

etc.

Randy Dunlap March 24, 2020, 5:44 p.m. UTC | #3

Hi,

On 3/24/20 10:04 AM, 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 <[e-mail]>
> ---
>  Documentation/serial/tty_rpmsg.rst |  45 ++++
>  drivers/tty/Kconfig                |   9 +
>  drivers/tty/Makefile               |   1 +
>  drivers/tty/rpmsg_tty.c            | 417 +++++++++++++++++++++++++++++
>  4 files changed, 472 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..fc1d3fba73c5
> --- /dev/null
> +++ b/Documentation/serial/tty_rpmsg.rst
> @@ -0,0 +1,45 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +=============
> +The 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.

                                                                         to make it possible

> +
> +The remote processor can instantiate a new tty by requesting:
> +- a "rpmsg-tty-raw" RPMsg service, for TTY raw data support without flow control
> +- a "rpmsg-tty-ctrl" RPMSg service, for TTY support with flow control.
> +
> +Information related to the RPMsg and associated tty device is available in
> +/sys/bus/rpmsg/devices/.
> +
> +RPMsg TTY without control
> +---------------------

extend underline under "control"

> +
> +The default end point associated with the "rpmsg-tty-raw" service is directly
> +used for data exchange. No flow control is available.
> +
> +To be compliant with this driver, the remote firmware must create its data end point associated with the "rpmsg-tty-raw" service.
> +
> +RPMsg TTY with control
> +---------------------

extend underline length.

> +
> +The default end point associated with the "rpmsg-tty-ctrl" service is reserved for
> +the control. A second endpoint must be created for data exchange.
> +
> +The control channel is used to transmit to the remote processor the CTS status,
> +as well as the end point address for data transfer.
> +
> +To be compatible with this driver, the remote firmware must create or use its end point associated with "rpmsg-tty-ctrl" service, plus a second endpoint for the data flow.
> +On Linux rpmsg_tty probes, the data endpoint address and the CTS (set to disable)
> +is sent to the remote processor.
> +The remote processor has to respect following rules:

                               respect the following rules:

> +- It only transmits data when Linux remote cts is enable, otherwise message

                                              CTS is enabled,

> +  could be lost.
> +- It can pause/resume reception by sending a control message (rely on CTS state).
> +
> +Control message structure:
> +struct rpmsg_tty_ctrl {
> +	u8 cts;			/* remote reception status */
> +	u16 d_ept_addr;		/* data endpoint address */
> +};

Is that struct packed or padded?

Bjorn Andersson March 24, 2020, 8:52 p.m. UTC | #4

On Tue 24 Mar 10:04 PDT 2020, Arnaud Pouliquen wrote:
[..]
> diff --git a/drivers/tty/Makefile b/drivers/tty/Makefile
> index 020b1cd9294f..c2465e7ebc2a 100644
> --- a/drivers/tty/Makefile
> +++ b/drivers/tty/Makefile
> @@ -34,5 +34,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	= TTY_CH_NAME_RAW },
> +	{ .name	= TTY_CH_NAME_WITH_CTS},

I still don't like the idea that the tty devices are tied to channels by
fixed names.

This makes the driver unusable for communicating with any firmware out
there that provides tty-like data over a channel with a different name -
such as modems with channels providing an AT command interface (they are
not named "rpmsg-tty-raw").

I also fail to see how you would distinguish ttys when the firmware
provides more than a single tty - e.g. say you have a modem-like device
that provides an AT command channel and a NMEA stream.


These are the reasons why drivers/rpmsg/rpmsg_char registers a "control
device", from which you can spawn new char devices. As I've said before,
I really think the same approach should be taken for ttys - perhaps by
just extending the rpmsg_char to allow it to create tty devices in
addition to the "packet based" char device?

Regards,
Bjorn

Jiri Slaby March 25, 2020, 8:10 a.m. UTC | #5

Hi,

On 24. 03. 20, 18:44, Randy Dunlap wrote:
>> +  could be lost.
>> +- It can pause/resume reception by sending a control message (rely on CTS state).
>> +
>> +Control message structure:
>> +struct rpmsg_tty_ctrl {
>> +	u8 cts;			/* remote reception status */
>> +	u16 d_ept_addr;		/* data endpoint address */
>> +};
> 
> Is that struct packed or padded?

As it seems, this documentation is in contradiction with code, anyway:

+struct rpmsg_tty_ctrl {
+	u16 d_ept_addr;		/* data endpoint address */
+	u8 cts;			/* remote reception status */
+} __packed;

thanks,

Jiri Slaby March 25, 2020, 8:45 a.m. UTC | #6

On 24. 03. 20, 18:04, Arnaud Pouliquen wrote:
> --- /dev/null
> +++ b/drivers/tty/rpmsg_tty.c
> @@ -0,0 +1,417 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) STMicroelectronics 2020 - All Rights Reserved
> + * Authors: Arnaud Pouliquen <[e-mail]> for STMicroelectronics.
> + */
...
> +typedef void (*rpmsg_tty_rx_cb_t)(struct rpmsg_device *, void *, int, void *,
> +				  u32);

Unused, it seems?

> +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 (src == cport->data_dst) {
> +		/* data message */
> +		if (!len)
> +			return -EINVAL;
> +		copied = tty_insert_flip_string_fixed_flag(&cport->port, data,
> +							   TTY_NORMAL, len);

Provided you always pass TTY_NORMAL, why not simply call
tty_insert_flip_string instead?

> +		if (copied != len)
> +			dev_dbg(&rpdev->dev, "trunc buffer: available space is %d\n",
> +				copied);
> +		tty_flip_buffer_push(&cport->port);
> +	} else {
> +		/* control message */
> +		struct rpmsg_tty_ctrl *msg = data;
> +
> +		if (len != sizeof(*msg))
> +			return -EINVAL;
> +
> +		cport->data_dst = msg->d_ept_addr;
> +
> +		/* Update remote cts state */
> +		cport->cts = msg->cts ? 1 : 0;

Number to bool implicit conversion needs no magic, just do:
cport->cts = msg->cts;

> +		if (cport->cts)
> +			tty_port_tty_wakeup(&cport->port);
> +	}
> +
> +	return 0;
> +}
> +
> +static void rpmsg_tty_send_term_ready(struct tty_struct *tty, u8 state)

Should the state be bool? Should it be named "ready" instead?

> +{
> +	struct rpmsg_tty_port *cport = tty->driver_data;
> +	struct rpmsg_tty_ctrl m_ctrl;
> +	int ret;
> +
> +	m_ctrl.cts = state;
> +	m_ctrl.d_ept_addr = cport->d_ept->addr;
> +
> +	ret = rpmsg_trysend(cport->cs_ept, &m_ctrl, sizeof(m_ctrl));
> +	if (ret < 0)
> +		dev_dbg(tty->dev, "cannot send control (%d)\n", ret);
> +};
> +
> +static void rpmsg_tty_throttle(struct tty_struct *tty)
> +{
> +	struct rpmsg_tty_port *cport = tty->driver_data;
> +
> +	/* Disable remote transmission */
> +	if (cport->cs_ept)
> +		rpmsg_tty_send_term_ready(tty, 0);

then s/0/false/;

> +};
> +
> +static void rpmsg_tty_unthrottle(struct tty_struct *tty)
> +{
> +	struct rpmsg_tty_port *cport = tty->driver_data;
> +
> +	/* Enable remote transmission */
> +	if (cport->cs_ept)
> +		rpmsg_tty_send_term_ready(tty, 1);

and s/1/true/;

> +};
...
> +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;
> +	u8 *tmpbuf;
> +
> +	/* 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);
> +
> +	msg_max_size = rpmsg_get_mtu(rpdev->ept);
> +
> +	msg_size = min(len, msg_max_size);
> +	tmpbuf = kzalloc(msg_size, GFP_KERNEL);
> +	if (!tmpbuf)
> +		return -ENOMEM;
> +
> +	memcpy(tmpbuf, buf, msg_size);

This is kmemdup, but why do you do that in the first place?

> +	/*
> +	 * Try to send the message to remote processor, if failed return 0 as
> +	 * no data sent
> +	 */
> +	ret = rpmsg_trysendto(cport->d_ept, tmpbuf, msg_size, cport->data_dst);

data of rpmsg_trysendto is not const. OK, you seem you need to change
that first, I see no blocker for that.

> +	kfree(tmpbuf);
> +	if (ret) {
> +		dev_dbg(&rpdev->dev, "rpmsg_send failed: %d\n", ret);
> +		return 0;
> +	}
> +
> +	return msg_size;
> +}
> +
> +static int rpmsg_tty_write_room(struct tty_struct *tty)
> +{
> +	struct rpmsg_tty_port *cport = tty->driver_data;
> +
> +	return cport->cts ? rpmsg_get_mtu(cport->rpdev->ept) : 0;

With if, this would be more readable, IMO.

> +}
...> +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);

You should return ERR_PTR(cport->id) instead. It might be ENOMEM too.

> +	}
> +
> +	return cport;
> +}
...
> +static int rpmsg_tty_port_activate(struct tty_port *p, struct tty_struct *tty)
> +{
> +	p->low_latency = (p->flags & ASYNC_LOW_LATENCY) ? 1 : 0;
> +
> +	/* Allocate the buffer we use for writing data */

Where exactly -- am I missing something?

> +	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 int rpmsg_tty_probe(struct rpmsg_device *rpdev)
> +{
> +	struct rpmsg_tty_port *cport;
> +	struct device *dev = &rpdev->dev;
> +	struct rpmsg_channel_info chinfo;
> +	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);
> +	}
> +
> +	if (!strncmp(rpdev->id.name, TTY_CH_NAME_WITH_CTS,
> +		     sizeof(TTY_CH_NAME_WITH_CTS))) {

sizeof of a string feels unnatural, but will work in this case. Can a
compiler optimize strlen of a static string?

> +		/*
> +		 * the default endpoint is used for control. Create a second
> +		 * endpoint for the data that would be exchanges trough control
> +		 * endpoint. address of the data endpoint will be provided with
> +		 * the cts state
> +		 */
> +		cport->cs_ept = rpdev->ept;
> +		cport->data_dst = RPMSG_ADDR_ANY;
> +
> +		strscpy(chinfo.name, TTY_CH_NAME_WITH_CTS, sizeof(chinfo.name));
> +		chinfo.src = RPMSG_ADDR_ANY;
> +		chinfo.dst = RPMSG_ADDR_ANY;
> +
> +		cport->d_ept = rpmsg_create_ept(rpdev, rpmsg_tty_cb, cport,
> +						chinfo);
> +		if (!cport->d_ept) {
> +			dev_err(dev, "failed to create tty control channel\n");
> +			ret = -ENOMEM;
> +			goto err_r_cport;
> +		}
> +		dev_dbg(dev, "%s: creating data endpoint with address %#x\n",
> +			__func__, cport->d_ept->addr);
> +	} else {
> +		/*
> +		 * TTY over rpmsg without CTS management the default endpoint
> +		 * is use for raw data transmission.
> +		 */
> +		cport->cs_ept = NULL;
> +		cport->cts = 1;
> +		cport->d_ept = rpdev->ept;
> +		cport->data_dst = rpdev->dst;
> +	}
> +
> +	tty_port_init(&cport->port);
> +	cport->port.ops = &rpmsg_tty_port_ops;

I expected these two in rpmsg_tty_alloc_cport.

> +
> +	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);
> +	if (cport->cs_ept)
> +		rpmsg_destroy_ept(cport->d_ept);
> +err_r_cport:
> +	rpmsg_tty_release_cport(cport);
> +
> +	return ret;
> +}

thanks,

Arnaud POULIQUEN March 25, 2020, 11:34 a.m. UTC | #7

On 3/24/20 6:18 PM, Greg Kroah-Hartman wrote:
> On Tue, Mar 24, 2020 at 06:04:07PM +0100, 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 <[e-mail]>
>> ---
>>  Documentation/serial/tty_rpmsg.rst |  45 ++++
>>  drivers/tty/Kconfig                |   9 +
>>  drivers/tty/Makefile               |   1 +
>>  drivers/tty/rpmsg_tty.c            | 417 +++++++++++++++++++++++++++++
>>  4 files changed, 472 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..fc1d3fba73c5
>> --- /dev/null
>> +++ b/Documentation/serial/tty_rpmsg.rst
>> @@ -0,0 +1,45 @@
>> +.. SPDX-License-Identifier: GPL-2.0
>> +
>> +=============
>> +The 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.
> 
> Can you wrap your lines properly for this file?

My apologize for this, I've been relying a little too much on the checkpatch tool here.

Thanks for the review,
Arnaud

> 
> thanks,
> 
> greg k-h
>

Arnaud POULIQUEN March 25, 2020, 11:36 a.m. UTC | #8

On 3/24/20 6:23 PM, Joe Perches wrote:
> On Tue, 2020-03-24 at 18:04 +0100, 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.
> 
> trivial notes:
> 
>> diff --git a/Documentation/serial/tty_rpmsg.rst b/Documentation/serial/tty_rpmsg.rst
> []
>> +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.
> 
> Very long text lines missing newlines?
> 
> []
>> +To be compliant with this driver, the remote firmware must create its data end point associated with the "rpmsg-tty-raw" service.
> []
>> +To be compatible with this driver, the remote firmware must create or use its end point associated with "rpmsg-tty-ctrl" service, plus a second endpoint for the data flow.
>> +On Linux rpmsg_tty probes, the data endpoint address and the CTS (set to disable)
> 
> []
> 
>> diff --git a/drivers/tty/rpmsg_tty.c b/drivers/tty/rpmsg_tty.c
> []
>> +typedef void (*rpmsg_tty_rx_cb_t)(struct rpmsg_device *, void *, int, void *,
>> +				  u32);
> 
> unused typedef?

yes i will clean it.

> 
> []
> 
>> +static int __init rpmsg_tty_init(void)
>> +{
> []
>> +	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;
>> +	}
> 
> Might use vsprintf extension %pe
> 
> 		pr_err("Couldn't install rpmsg tty driver: %pe\n", ERR_PTR(err));

Seems not possible here as err is an integer value and not a pointer cast to an integer,
or I missed something?


Thanks for the review,
Arnaud
> 
>> +	err = register_rpmsg_driver(&rpmsg_tty_rpmsg_drv);
>> +	if (err < 0) {
>> +		pr_err("Couldn't register rpmsg tty driver: err %d\n", err);
> 
> etc.
> 
>

Arnaud POULIQUEN March 25, 2020, 11:39 a.m. UTC | #9

Hi,

On 3/24/20 6:44 PM, Randy Dunlap wrote:
> Hi,
> 
> On 3/24/20 10:04 AM, 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 <[e-mail]>
>> ---
>>  Documentation/serial/tty_rpmsg.rst |  45 ++++
>>  drivers/tty/Kconfig                |   9 +
>>  drivers/tty/Makefile               |   1 +
>>  drivers/tty/rpmsg_tty.c            | 417 +++++++++++++++++++++++++++++
>>  4 files changed, 472 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..fc1d3fba73c5
>> --- /dev/null
>> +++ b/Documentation/serial/tty_rpmsg.rst
>> @@ -0,0 +1,45 @@
>> +.. SPDX-License-Identifier: GPL-2.0
>> +
>> +=============
>> +The 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.
> 
>                                                                          to make it possible
> 
>> +
>> +The remote processor can instantiate a new tty by requesting:
>> +- a "rpmsg-tty-raw" RPMsg service, for TTY raw data support without flow control
>> +- a "rpmsg-tty-ctrl" RPMSg service, for TTY support with flow control.
>> +
>> +Information related to the RPMsg and associated tty device is available in
>> +/sys/bus/rpmsg/devices/.
>> +
>> +RPMsg TTY without control
>> +---------------------
> 
> extend underline under "control"
> 
>> +
>> +The default end point associated with the "rpmsg-tty-raw" service is directly
>> +used for data exchange. No flow control is available.
>> +
>> +To be compliant with this driver, the remote firmware must create its data end point associated with the "rpmsg-tty-raw" service.
>> +
>> +RPMsg TTY with control
>> +---------------------
> 
> extend underline length.
> 
>> +
>> +The default end point associated with the "rpmsg-tty-ctrl" service is reserved for
>> +the control. A second endpoint must be created for data exchange.
>> +
>> +The control channel is used to transmit to the remote processor the CTS status,
>> +as well as the end point address for data transfer.
>> +
>> +To be compatible with this driver, the remote firmware must create or use its end point associated with "rpmsg-tty-ctrl" service, plus a second endpoint for the data flow.
>> +On Linux rpmsg_tty probes, the data endpoint address and the CTS (set to disable)
>> +is sent to the remote processor.
>> +The remote processor has to respect following rules:
> 
>                                respect the following rules:
> 
>> +- It only transmits data when Linux remote cts is enable, otherwise message
> 
>                                               CTS is enabled,
> 
>> +  could be lost.
>> +- It can pause/resume reception by sending a control message (rely on CTS state).
>> +
>> +Control message structure:
>> +struct rpmsg_tty_ctrl {
>> +	u8 cts;			/* remote reception status */
>> +	u16 d_ept_addr;		/* data endpoint address */
>> +};
> 
> Is that struct packed or padded?

It's packed as payload of the RP message and so define is global size.
This struct is misaligned with the code.

I will take into account your comments in next version.

Thanks for the review,
Arnaud

Arnaud POULIQUEN March 25, 2020, 1:15 p.m. UTC | #10

On 3/25/20 9:45 AM, Jiri Slaby wrote:
> On 24. 03. 20, 18:04, Arnaud Pouliquen wrote:
>> --- /dev/null
>> +++ b/drivers/tty/rpmsg_tty.c
>> @@ -0,0 +1,417 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) STMicroelectronics 2020 - All Rights Reserved
>> + * Authors: Arnaud Pouliquen <[e-mail]> for STMicroelectronics.
>> + */
> ...
>> +typedef void (*rpmsg_tty_rx_cb_t)(struct rpmsg_device *, void *, int, void *,
>> +				  u32);
> 
> Unused, it seems?
> 
>> +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 (src == cport->data_dst) {
>> +		/* data message */
>> +		if (!len)
>> +			return -EINVAL;
>> +		copied = tty_insert_flip_string_fixed_flag(&cport->port, data,
>> +							   TTY_NORMAL, len);
> 
> Provided you always pass TTY_NORMAL, why not simply call
> tty_insert_flip_string instead?
> 
>> +		if (copied != len)
>> +			dev_dbg(&rpdev->dev, "trunc buffer: available space is %d\n",
>> +				copied);
>> +		tty_flip_buffer_push(&cport->port);
>> +	} else {
>> +		/* control message */
>> +		struct rpmsg_tty_ctrl *msg = data;
>> +
>> +		if (len != sizeof(*msg))
>> +			return -EINVAL;
>> +
>> +		cport->data_dst = msg->d_ept_addr;
>> +
>> +		/* Update remote cts state */
>> +		cport->cts = msg->cts ? 1 : 0;
> 
> Number to bool implicit conversion needs no magic, just do:
> cport->cts = msg->cts;

In this case i would prefer  cport->cts = (msg->cts != 1);
for the conversion

> 
>> +		if (cport->cts)
>> +			tty_port_tty_wakeup(&cport->port);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void rpmsg_tty_send_term_ready(struct tty_struct *tty, u8 state)
> 
> Should the state be bool? Should it be named "ready" instead?
> 
>> +{
>> +	struct rpmsg_tty_port *cport = tty->driver_data;
>> +	struct rpmsg_tty_ctrl m_ctrl;
>> +	int ret;
>> +
>> +	m_ctrl.cts = state;
>> +	m_ctrl.d_ept_addr = cport->d_ept->addr;
>> +
>> +	ret = rpmsg_trysend(cport->cs_ept, &m_ctrl, sizeof(m_ctrl));
>> +	if (ret < 0)
>> +		dev_dbg(tty->dev, "cannot send control (%d)\n", ret);
>> +};
>> +
>> +static void rpmsg_tty_throttle(struct tty_struct *tty)
>> +{
>> +	struct rpmsg_tty_port *cport = tty->driver_data;
>> +
>> +	/* Disable remote transmission */
>> +	if (cport->cs_ept)
>> +		rpmsg_tty_send_term_ready(tty, 0);
> 
> then s/0/false/;
> 
>> +};
>> +
>> +static void rpmsg_tty_unthrottle(struct tty_struct *tty)
>> +{
>> +	struct rpmsg_tty_port *cport = tty->driver_data;
>> +
>> +	/* Enable remote transmission */
>> +	if (cport->cs_ept)
>> +		rpmsg_tty_send_term_ready(tty, 1);
> 
> and s/1/true/> 
>> +};
> ...
>> +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;
>> +	u8 *tmpbuf;
>> +
>> +	/* 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);
>> +
>> +	msg_max_size = rpmsg_get_mtu(rpdev->ept);
>> +
>> +	msg_size = min(len, msg_max_size);
>> +	tmpbuf = kzalloc(msg_size, GFP_KERNEL);
>> +	if (!tmpbuf)
>> +		return -ENOMEM;
>> +
>> +	memcpy(tmpbuf, buf, msg_size);
> 
> This is kmemdup, but why do you do that in the first place?
> 
>> +	/*
>> +	 * Try to send the message to remote processor, if failed return 0 as
>> +	 * no data sent
>> +	 */
>> +	ret = rpmsg_trysendto(cport->d_ept, tmpbuf, msg_size, cport->data_dst);
> 
> data of rpmsg_trysendto is not const. OK, you seem you need to change
> that first, I see no blocker for that.

I created a temporary buffer to ensure that buffer to sent does not exceed the 
MTU size.
But perhaps this is an useless protection as the rpmsg_tty_write_room already
return the MTU value, and so the 'len' variable can not be higher that value
returned by the write_room?

> 
>> +	kfree(tmpbuf);
>> +	if (ret) {
>> +		dev_dbg(&rpdev->dev, "rpmsg_send failed: %d\n", ret);
>> +		return 0;
>> +	}
>> +
>> +	return msg_size;
>> +}
>> +
>> +static int rpmsg_tty_write_room(struct tty_struct *tty)
>> +{
>> +	struct rpmsg_tty_port *cport = tty->driver_data;
>> +
>> +	return cport->cts ? rpmsg_get_mtu(cport->rpdev->ept) : 0;
> 
> With if, this would be more readable, IMO.
> 
>> +}
> ...> +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);
> 
> You should return ERR_PTR(cport->id) instead. It might be ENOMEM too.
> 
>> +	}
>> +
>> +	return cport;
>> +}
> ...
>> +static int rpmsg_tty_port_activate(struct tty_port *p, struct tty_struct *tty)
>> +{
>> +	p->low_latency = (p->flags & ASYNC_LOW_LATENCY) ? 1 : 0;
>> +
>> +	/* Allocate the buffer we use for writing data */
> 
> Where exactly -- am I missing something?

in tty_port_alloc_xmit_buf. it's a copy past from mips_ejtag_fdc.c,
I will clean this line if it's confusing.
 
> 
>> +	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 int rpmsg_tty_probe(struct rpmsg_device *rpdev)
>> +{
>> +	struct rpmsg_tty_port *cport;
>> +	struct device *dev = &rpdev->dev;
>> +	struct rpmsg_channel_info chinfo;
>> +	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);
>> +	}
>> +
>> +	if (!strncmp(rpdev->id.name, TTY_CH_NAME_WITH_CTS,
>> +		     sizeof(TTY_CH_NAME_WITH_CTS))) {
> 
> sizeof of a string feels unnatural, but will work in this case. Can a
> compiler optimize strlen of a static string?

I don't know if a compiler can do this...
what about replacing sizeof by strlen function? 
i saw some code example that use strlen with static string...
(e.g  https://elixir.bootlin.com/linux/latest/source/drivers/edac/edac_mc.c#L1193)


I will take into account all your other comments in may next version.

Thanks for the review,
Arnaud

> 
>> +		/*
>> +		 * the default endpoint is used for control. Create a second
>> +		 * endpoint for the data that would be exchanges trough control
>> +		 * endpoint. address of the data endpoint will be provided with
>> +		 * the cts state
>> +		 */
>> +		cport->cs_ept = rpdev->ept;
>> +		cport->data_dst = RPMSG_ADDR_ANY;
>> +
>> +		strscpy(chinfo.name, TTY_CH_NAME_WITH_CTS, sizeof(chinfo.name));
>> +		chinfo.src = RPMSG_ADDR_ANY;
>> +		chinfo.dst = RPMSG_ADDR_ANY;
>> +
>> +		cport->d_ept = rpmsg_create_ept(rpdev, rpmsg_tty_cb, cport,
>> +						chinfo);
>> +		if (!cport->d_ept) {
>> +			dev_err(dev, "failed to create tty control channel\n");
>> +			ret = -ENOMEM;
>> +			goto err_r_cport;
>> +		}
>> +		dev_dbg(dev, "%s: creating data endpoint with address %#x\n",
>> +			__func__, cport->d_ept->addr);
>> +	} else {
>> +		/*
>> +		 * TTY over rpmsg without CTS management the default endpoint
>> +		 * is use for raw data transmission.
>> +		 */
>> +		cport->cs_ept = NULL;
>> +		cport->cts = 1;
>> +		cport->d_ept = rpdev->ept;
>> +		cport->data_dst = rpdev->dst;
>> +	}
>> +
>> +	tty_port_init(&cport->port);
>> +	cport->port.ops = &rpmsg_tty_port_ops;
> 
> I expected these two in rpmsg_tty_alloc_cport
> 
>> +
>> +	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);
>> +	if (cport->cs_ept)
>> +		rpmsg_destroy_ept(cport->d_ept);
>> +err_r_cport:
>> +	rpmsg_tty_release_cport(cport);
>> +
>> +	return ret;
>> +}
> 
> thanks,
>

Jiri Slaby March 25, 2020, 1:31 p.m. UTC | #11

On 25. 03. 20, 14:15, Arnaud POULIQUEN wrote:
>>> +		if (copied != len)
>>> +			dev_dbg(&rpdev->dev, "trunc buffer: available space is %d\n",
>>> +				copied);
>>> +		tty_flip_buffer_push(&cport->port);
>>> +	} else {
>>> +		/* control message */
>>> +		struct rpmsg_tty_ctrl *msg = data;
>>> +
>>> +		if (len != sizeof(*msg))
>>> +			return -EINVAL;
>>> +
>>> +		cport->data_dst = msg->d_ept_addr;
>>> +
>>> +		/* Update remote cts state */
>>> +		cport->cts = msg->cts ? 1 : 0;
>>
>> Number to bool implicit conversion needs no magic, just do:
>> cport->cts = msg->cts;
> 
> In this case i would prefer  cport->cts = (msg->cts != 1);
> for the conversion

That still looks confusing. In the ternary operator above, you used
msg->cts as a bool implicitly and now you are trying to artificially
create one :)?

IOW in a bool context, "msg->cts ? 1 : 0" is the same as "msg->cts".

>>> +	/*
>>> +	 * Try to send the message to remote processor, if failed return 0 as
>>> +	 * no data sent
>>> +	 */
>>> +	ret = rpmsg_trysendto(cport->d_ept, tmpbuf, msg_size, cport->data_dst);
>>
>> data of rpmsg_trysendto is not const. OK, you seem you need to change
>> that first, I see no blocker for that.
> 
> I created a temporary buffer to ensure that buffer to sent does not exceed the 
> MTU size.
> But perhaps this is an useless protection as the rpmsg_tty_write_room already
> return the MTU value, and so the 'len' variable can not be higher that value
> returned by the write_room?

You still can limit it by msg_size without cloning the buffer, right?

>>> +static int rpmsg_tty_port_activate(struct tty_port *p, struct tty_struct *tty)
>>> +{
>>> +	p->low_latency = (p->flags & ASYNC_LOW_LATENCY) ? 1 : 0;
>>> +
>>> +	/* Allocate the buffer we use for writing data */
>>
>> Where exactly -- am I missing something?
> 
> in tty_port_alloc_xmit_buf. it's a copy past from mips_ejtag_fdc.c,
> I will clean this line if it's confusing.

No, I mean where do you use the allocated buffer? mips_ejtag_fdc.c uses it.

>>> +static int rpmsg_tty_probe(struct rpmsg_device *rpdev)
>>> +{
>>> +	struct rpmsg_tty_port *cport;
>>> +	struct device *dev = &rpdev->dev;
>>> +	struct rpmsg_channel_info chinfo;
>>> +	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);
>>> +	}
>>> +
>>> +	if (!strncmp(rpdev->id.name, TTY_CH_NAME_WITH_CTS,
>>> +		     sizeof(TTY_CH_NAME_WITH_CTS))) {
>>
>> sizeof of a string feels unnatural, but will work in this case. Can a
>> compiler optimize strlen of a static string?
> 
> I don't know if a compiler can do this...
> what about replacing sizeof by strlen function? 
> i saw some code example that use strlen with static string...
> (e.g  https://elixir.bootlin.com/linux/latest/source/drivers/edac/edac_mc.c#L1193)

The question was exactly about that: can a compiler optimize it to a
bare number or will strlen call remain there?

thanks,

Arnaud POULIQUEN March 25, 2020, 4:57 p.m. UTC | #12

Hi Bjorn,

On 3/24/20 9:52 PM, Bjorn Andersson wrote:
> On Tue 24 Mar 10:04 PDT 2020, Arnaud Pouliquen wrote:
> [..]
>> diff --git a/drivers/tty/Makefile b/drivers/tty/Makefile
>> index 020b1cd9294f..c2465e7ebc2a 100644
>> --- a/drivers/tty/Makefile
>> +++ b/drivers/tty/Makefile
>> @@ -34,5 +34,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	= TTY_CH_NAME_RAW },
>> +	{ .name	= TTY_CH_NAME_WITH_CTS},
> 
> I still don't like the idea that the tty devices are tied to channels by
> fixed names.

This point has been discussed with Xiang, he has the same kind of requirement. 
My proposal here is to do this in two steps. First a fixed name, then
in a second step we can extend the naming using the implementation proposed
by Mathieu Poirier:

[1]https://lkml.org/lkml/2020/2/12/1083

Is this patch could answer to your requirement?

if requested i can I can integrate the Mathieu's patch in this patchset.
 
> 
> This makes the driver unusable for communicating with any firmware out
> there that provides tty-like data over a channel with a different name -
> such as modems with channels providing an AT command interface (they are
> not named "rpmsg-tty-raw").

I'm not fixed on the naming, any proposal is welcome.
If we use the patch [1], could be renamed 
"rpmsg-tty". then for AT command could be something like "rpmsg-tty-at"

But here seems we are speaking about service over TTY and not over RPMsg.

> 
> I also fail to see how you would distinguish ttys when the firmware
> provides more than a single tty - e.g. say you have a modem-like device
> that provides an AT command channel and a NMEA stream.

Today it is a limitation. In fact this limitation is the same for all RPMsg
devices with multi instance.
The patch [1] will allow to retrieve the instance by identifying
the service device name in /sys/class/tty/ttyRPMSG<X>/device/name

> 
> 
> These are the reasons why drivers/rpmsg/rpmsg_char registers a "control
> device", from which you can spawn new char devices. As I've said before,
> I really think the same approach should be taken for ttys - perhaps by
> just extending the rpmsg_char to allow it to create tty devices in
> addition to the "packet based" char device?
> 
I'm not very familiar with the rpmsg_char so please correct me if i'm wrong:

The rpmsg_char exposes to userland an interface to manage rpmsg channels
(relying on a char device). This interface offers the  possibility to create
new channels/endpoints and send/received related messages. 
 
Thus, the application declares the RPMsg channels which is bound if they matches
with the remote processor channel (similar behavior than a kernel rpmsg driver).
There is no constrain on the service once same service is advertised by remote
firmware.

In addition, a limitation of the rpmsg_char device is that it needs to be
associated with an existing device, as example the implementation in qcom_smd
driver.

If i try to figure out how to implement TTY using the rpmsg_char:
I should create a rpmsg_char dev in the rpmsg tty driver. Then application
will create channels related to its service. But in this case
how to ensure that channels created are related to the TTY service?  


I would also expect to manage RPMsg TTY such as a generic TTY: without
extra interface and auto mounted as an USB TTY. this means that the
/dev/ttyRMPSGx are created automatically at remote firmware startup
(without any application action). For instance a generic application 
(e.g. minicom) could control an internal remote processor such as
an external processor through a TTY link. 

Then we have also similar RPMsg driver for I2C and SPI virtual link. So extend
the rpmsg_char to support TTY seems not a good solution for long terms. 

For these reasons i would prefer to have a specific driver. And found a solution
to allow user to differentiate the TTY instances.

Anyway I am very interesting in having more details of an implementation relying
on rpmsg_char if you still thinking that is the good approach here.

Thanks for your comments, 
Arnaud

> Regards,
> Bjorn
>

Joe Perches March 26, 2020, 12:01 a.m. UTC | #13

On Wed, 2020-03-25 at 14:31 +0100, Jiri Slaby wrote:
> The question was exactly about that: can a compiler optimize it to a
> bare number or will strlen call remain there?

$ cat str.c
#include <string.h>

int foo(void)
{
	return strlen("abc");
}

$ gcc -c -O2 str.c
$ objdump -d str.o
str.o:     file format elf64-x86-64


Disassembly of section .text:

0000000000000000 <foo>:
   0:	f3 0f 1e fa          	endbr64 
   4:	b8 03 00 00 00       	mov    $0x3,%eax
   9:	c3                   	retq

Jiri Slaby March 26, 2020, 6:38 a.m. UTC | #14

On 26. 03. 20, 1:01, Joe Perches wrote:
> On Wed, 2020-03-25 at 14:31 +0100, Jiri Slaby wrote:
>> The question was exactly about that: can a compiler optimize it to a
>> bare number or will strlen call remain there?
> 
> $ cat str.c
> #include <string.h>
> 
> int foo(void)
> {
> 	return strlen("abc");
> }
> 
> $ gcc -c -O2 str.c
> $ objdump -d str.o
> str.o:     file format elf64-x86-64
> 
> 
> Disassembly of section .text:
> 
> 0000000000000000 <foo>:
>    0:	f3 0f 1e fa          	endbr64 
>    4:	b8 03 00 00 00       	mov    $0x3,%eax
>    9:	c3                   	retq   

Perfect, but:
* that is userspace (different strlen).
* what about other archs -- AFAIR, i386 implements strlen in asm in the
kernel

Maybe compilers use intrinsics and only after optimizations, they put a
call to strlen?

/me digging

Yeah, even gimple already contains:
  int D.2094;

  D.2094 = 3;
  return D.2094;


That means, even -O0 should generate 3 instead of the call:
$ echo -e '#include <string.h>\nint f() { return strlen("abc"); }' | gcc
-S -x c - -o - -O0
...
f:
.LFB0:
        .cfi_startproc
        pushq   %rbp
        .cfi_def_cfa_offset 16
        .cfi_offset 6, -16
        movq    %rsp, %rbp
        .cfi_def_cfa_register 6
        movl    $3, %eax
        popq    %rbp
        .cfi_def_cfa 7, 8
        ret
        .cfi_endproc


So fine, use strlen :).

thanks,

Arnaud POULIQUEN March 26, 2020, 10:59 a.m. UTC | #15

On 3/26/20 1:01 AM, Joe Perches wrote:
> On Wed, 2020-03-25 at 14:31 +0100, Jiri Slaby wrote:
>> The question was exactly about that: can a compiler optimize it to a
>> bare number or will strlen call remain there?
> 
> $ cat str.c
> #include <string.h>
> 
> int foo(void)
> {
> 	return strlen("abc");
> }
> 
> $ gcc -c -O2 str.c
> $ objdump -d str.o
> str.o:     file format elf64-x86-64
> 
> 
> Disassembly of section .text:
> 
> 0000000000000000 <foo>:
>    0:	f3 0f 1e fa          	endbr64 
>    4:	b8 03 00 00 00       	mov    $0x3,%eax
>    9:	c3                   	retq   
> 
> 
same result with  arm gcc using  -O1 or -Og:

str.o:     file format elf32-littlearm


Disassembly of section .text:

00000000 <foo>:
   0:	e3a00003 	mov	r0, #3
   4:	e12fff1e 	bx	lr

So in conclusion replacing sizeof by srlen even if not optimized in -o0, right?

Thanks,
Arnaud

Arnaud POULIQUEN March 26, 2020, 11:40 a.m. UTC | #16

On 3/25/20 2:31 PM, Jiri Slaby wrote:
> On 25. 03. 20, 14:15, Arnaud POULIQUEN wrote:
>>>> +		if (copied != len)
>>>> +			dev_dbg(&rpdev->dev, "trunc buffer: available space is %d\n",
>>>> +				copied);
>>>> +		tty_flip_buffer_push(&cport->port);
>>>> +	} else {
>>>> +		/* control message */
>>>> +		struct rpmsg_tty_ctrl *msg = data;
>>>> +
>>>> +		if (len != sizeof(*msg))
>>>> +			return -EINVAL;
>>>> +
>>>> +		cport->data_dst = msg->d_ept_addr;
>>>> +
>>>> +		/* Update remote cts state */
>>>> +		cport->cts = msg->cts ? 1 : 0;
>>>
>>> Number to bool implicit conversion needs no magic, just do:
>>> cport->cts = msg->cts;
>>
>> In this case i would prefer  cport->cts = (msg->cts != 1);
>> for the conversion
> 
> That still looks confusing. In the ternary operator above, you used
> msg->cts as a bool implicitly and now you are trying to artificially
> create one :)?
> 
> IOW in a bool context, "msg->cts ? 1 : 0" is the same as "msg->cts".
> Look like the better solution would be to not use bool at all here...
 
>>>> +	/*
>>>> +	 * Try to send the message to remote processor, if failed return 0 as
>>>> +	 * no data sent
>>>> +	 */
>>>> +	ret = rpmsg_trysendto(cport->d_ept, tmpbuf, msg_size, cport->data_dst);
>>>
>>> data of rpmsg_trysendto is not const. OK, you seem you need to change
>>> that first, I see no blocker for that.
>>
>> I created a temporary buffer to ensure that buffer to sent does not exceed the 
>> MTU size.
>> But perhaps this is an useless protection as the rpmsg_tty_write_room already
>> return the MTU value, and so the 'len' variable can not be higher that value
>> returned by the write_room?
> 
> You still can limit it by msg_size without cloning the buffer, right?
you are right, but in this case i need to cast the buff to suppress compilation
warning on const and I don't know if all compilers will accept this...
 
pbuf = (u8 *)buf;
ret = rpmsg_trysendto(cport->d_ept, pbuf, msg_size, cport->data_dst);

> 
>>>> +static int rpmsg_tty_port_activate(struct tty_port *p, struct tty_struct *tty)
>>>> +{
>>>> +	p->low_latency = (p->flags & ASYNC_LOW_LATENCY) ? 1 : 0;
>>>> +
>>>> +	/* Allocate the buffer we use for writing data */
>>>
>>> Where exactly -- am I missing something?
>>
>> in tty_port_alloc_xmit_buf. it's a copy past from mips_ejtag_fdc.c,
>> I will clean this line if it's confusing.
> 
> No, I mean where do you use the allocated buffer? mips_ejtag_fdc.c uses it.
Seems i misunderstood the usage of the xmit buffer, need to have look in.

> 
>>>> +static int rpmsg_tty_probe(struct rpmsg_device *rpdev)
>>>> +{
>>>> +	struct rpmsg_tty_port *cport;
>>>> +	struct device *dev = &rpdev->dev;
>>>> +	struct rpmsg_channel_info chinfo;
>>>> +	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);
>>>> +	}
>>>> +
>>>> +	if (!strncmp(rpdev->id.name, TTY_CH_NAME_WITH_CTS,
>>>> +		     sizeof(TTY_CH_NAME_WITH_CTS))) {
>>>
>>> sizeof of a string feels unnatural, but will work in this case. Can a
>>> compiler optimize strlen of a static string?
>>
>> I don't know if a compiler can do this...
>> what about replacing sizeof by strlen function? 
>> i saw some code example that use strlen with static string...
>> (e.g  https://elixir.bootlin.com/linux/latest/source/drivers/edac/edac_mc.c#L1193)
> 
> The question was exactly about that: can a compiler optimize it to a
> bare number or will strlen call remain there?
> 
i answered in Joe's mail for this point

Thanks!
Arnaud

> thanks,
>

Jiri Slaby March 26, 2020, 11:45 a.m. UTC | #17

On 26. 03. 20, 12:40, Arnaud POULIQUEN wrote:
>>>>> +	/*
>>>>> +	 * Try to send the message to remote processor, if failed return 0 as
>>>>> +	 * no data sent
>>>>> +	 */
>>>>> +	ret = rpmsg_trysendto(cport->d_ept, tmpbuf, msg_size, cport->data_dst);
>>>>
>>>> data of rpmsg_trysendto is not const. OK, you seem you need to change
>>>> that first, I see no blocker for that.
>>>
>>> I created a temporary buffer to ensure that buffer to sent does not exceed the 
>>> MTU size.
>>> But perhaps this is an useless protection as the rpmsg_tty_write_room already
>>> return the MTU value, and so the 'len' variable can not be higher that value
>>> returned by the write_room?
>>
>> You still can limit it by msg_size without cloning the buffer, right?
> you are right, but in this case i need to cast the buff to suppress compilation
> warning on const and I don't know if all compilers will accept this...
>  
> pbuf = (u8 *)buf;
> ret = rpmsg_trysendto(cport->d_ept, pbuf, msg_size, cport->data_dst);

No, don't do that. Read my first message again; in particular:

> data of rpmsg_trysendto is not const. OK, you seem you need to change
> that first, I see no blocker for that.

thanks,

Jiri Slaby March 26, 2020, 12:31 p.m. UTC | #18

On 26. 03. 20, 11:59, Arnaud POULIQUEN wrote:
> 
> 
> On 3/26/20 1:01 AM, Joe Perches wrote:
>> On Wed, 2020-03-25 at 14:31 +0100, Jiri Slaby wrote:
>>> The question was exactly about that: can a compiler optimize it to a
>>> bare number or will strlen call remain there?
>>
>> $ cat str.c
>> #include <string.h>
>>
>> int foo(void)
>> {
>> 	return strlen("abc");
>> }
>>
>> $ gcc -c -O2 str.c
>> $ objdump -d str.o
>> str.o:     file format elf64-x86-64
>>
>>
>> Disassembly of section .text:
>>
>> 0000000000000000 <foo>:
>>    0:	f3 0f 1e fa          	endbr64 
>>    4:	b8 03 00 00 00       	mov    $0x3,%eax
>>    9:	c3                   	retq   
>>
>>
> same result with  arm gcc using  -O1 or -Og:
> 
> str.o:     file format elf32-littlearm
> 
> 
> Disassembly of section .text:
> 
> 00000000 <foo>:
>    0:	e3a00003 	mov	r0, #3
>    4:	e12fff1e 	bx	lr
> 
> So in conclusion replacing sizeof by srlen even if not optimized in -o0, right?

Right, gcc guys just confirmed, that it's constant-folded during parsing
already. I asked them as I tried to dump the tree.original and the
constant was already there.

So we are safe to use strlen, at least for gcc :P. Others should adapt
if they don't follow.

thanks,

Mathieu Poirier April 1, 2020, 6:06 p.m. UTC | #19

On Tue, Mar 24, 2020 at 06:04:07PM +0100, 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 <[e-mail]>
> ---
>  Documentation/serial/tty_rpmsg.rst |  45 ++++
>  drivers/tty/Kconfig                |   9 +
>  drivers/tty/Makefile               |   1 +
>  drivers/tty/rpmsg_tty.c            | 417 +++++++++++++++++++++++++++++
>  4 files changed, 472 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..fc1d3fba73c5
> --- /dev/null
> +++ b/Documentation/serial/tty_rpmsg.rst
> @@ -0,0 +1,45 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +=============
> +The 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-raw" RPMsg service, for TTY raw data support without flow control
> +- a "rpmsg-tty-ctrl" RPMSg service, for TTY support with flow control.
> +
> +Information related to the RPMsg and associated tty device is available in
> +/sys/bus/rpmsg/devices/.
> +
> +RPMsg TTY without control
> +---------------------
> +
> +The default end point associated with the "rpmsg-tty-raw" service is directly
> +used for data exchange. No flow control is available.
> +
> +To be compliant with this driver, the remote firmware must create its data end point associated with the "rpmsg-tty-raw" service.
> +
> +RPMsg TTY with control
> +---------------------
> +
> +The default end point associated with the "rpmsg-tty-ctrl" service is reserved for
> +the control. A second endpoint must be created for data exchange.
> +
> +The control channel is used to transmit to the remote processor the CTS status,
> +as well as the end point address for data transfer.
> +
> +To be compatible with this driver, the remote firmware must create or use its end point associated with "rpmsg-tty-ctrl" service, plus a second endpoint for the data flow.
> +On Linux rpmsg_tty probes, the data endpoint address and the CTS (set to disable)
> +is sent to the remote processor.
> +The remote processor has to respect following rules:
> +- It only transmits data when Linux remote cts is enable, otherwise message
> +  could be lost.
> +- It can pause/resume reception by sending a control message (rely on CTS state).
> +
> +Control message structure:
> +struct rpmsg_tty_ctrl {
> +	u8 cts;			/* remote reception status */
> +	u16 d_ept_addr;		/* data endpoint address */
> +};
> diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
> index a312cb33a99b..9d3ff6df9f25 100644
> --- a/drivers/tty/Kconfig
> +++ b/drivers/tty/Kconfig
> @@ -454,6 +454,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 020b1cd9294f..c2465e7ebc2a 100644
> --- a/drivers/tty/Makefile
> +++ b/drivers/tty/Makefile
> @@ -34,5 +34,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..49ce3b72781a
> --- /dev/null
> +++ b/drivers/tty/rpmsg_tty.c
> @@ -0,0 +1,417 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) STMicroelectronics 2020 - All Rights Reserved
> + * Authors: Arnaud Pouliquen <[e-mail]> 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
> +
> +#define TTY_CH_NAME_RAW		"rpmsg-tty-raw"
> +#define TTY_CH_NAME_WITH_CTS	"rpmsg-tty-ctrl"
> +
> +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_ctrl {
> +	u16 d_ept_addr;		/* data endpoint address */
> +	u8 cts;			/* remote reception status */
> +} __packed;
> +
> +struct rpmsg_tty_port {
> +	struct tty_port		port;	 /* TTY port data */
> +	int			id;	 /* TTY rpmsg index */
> +	bool			cts;	 /* remote reception status */
> +	struct rpmsg_device	*rpdev;	 /* rpmsg device */
> +	struct rpmsg_endpoint   *cs_ept; /* channel control endpoint */
> +	struct rpmsg_endpoint   *d_ept;  /* data endpoint */
> +	u32 data_dst;			 /* data destination endpoint address */
> +};
> +
> +typedef void (*rpmsg_tty_rx_cb_t)(struct rpmsg_device *, void *, int, void *,
> +				  u32);
> +
> +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 (src == cport->data_dst) {
> +		/* data message */
> +		if (!len)
> +			return -EINVAL;
> +		copied = tty_insert_flip_string_fixed_flag(&cport->port, data,
> +							   TTY_NORMAL, len);
> +		if (copied != len)
> +			dev_dbg(&rpdev->dev, "trunc buffer: available space is %d\n",
> +				copied);
> +		tty_flip_buffer_push(&cport->port);
> +	} else {
> +		/* control message */
> +		struct rpmsg_tty_ctrl *msg = data;
> +
> +		if (len != sizeof(*msg))
> +			return -EINVAL;
> +
> +		cport->data_dst = msg->d_ept_addr;
> +
> +		/* Update remote cts state */
> +		cport->cts = msg->cts ? 1 : 0;
> +
> +		if (cport->cts)
> +			tty_port_tty_wakeup(&cport->port);
> +	}
> +
> +	return 0;
> +}
> +
> +static void rpmsg_tty_send_term_ready(struct tty_struct *tty, u8 state)
> +{
> +	struct rpmsg_tty_port *cport = tty->driver_data;
> +	struct rpmsg_tty_ctrl m_ctrl;
> +	int ret;
> +
> +	m_ctrl.cts = state;
> +	m_ctrl.d_ept_addr = cport->d_ept->addr;
> +
> +	ret = rpmsg_trysend(cport->cs_ept, &m_ctrl, sizeof(m_ctrl));
> +	if (ret < 0)
> +		dev_dbg(tty->dev, "cannot send control (%d)\n", ret);
> +};
> +
> +static void rpmsg_tty_throttle(struct tty_struct *tty)
> +{
> +	struct rpmsg_tty_port *cport = tty->driver_data;
> +
> +	/* Disable remote transmission */
> +	if (cport->cs_ept)
> +		rpmsg_tty_send_term_ready(tty, 0);
> +};
> +
> +static void rpmsg_tty_unthrottle(struct tty_struct *tty)
> +{
> +	struct rpmsg_tty_port *cport = tty->driver_data;
> +
> +	/* Enable remote transmission */
> +	if (cport->cs_ept)
> +		rpmsg_tty_send_term_ready(tty, 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;
> +	}
> +
> +	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;
> +	u8 *tmpbuf;
> +
> +	/* 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);
> +
> +	msg_max_size = rpmsg_get_mtu(rpdev->ept);
> +
> +	msg_size = min(len, msg_max_size);
> +	tmpbuf = kzalloc(msg_size, GFP_KERNEL);
> +	if (!tmpbuf)
> +		return -ENOMEM;
> +
> +	memcpy(tmpbuf, buf, msg_size);
> +
> +	/*
> +	 * Try to send the message to remote processor, if failed return 0 as
> +	 * no data sent
> +	 */
> +	ret = rpmsg_trysendto(cport->d_ept, tmpbuf, msg_size, cport->data_dst);
> +	kfree(tmpbuf);
> +	if (ret) {
> +		dev_dbg(&rpdev->dev, "rpmsg_send failed: %d\n", ret);
> +		return 0;
> +	}
> +
> +	return msg_size;
> +}
> +
> +static int rpmsg_tty_write_room(struct tty_struct *tty)
> +{
> +	struct rpmsg_tty_port *cport = tty->driver_data;
> +
> +	return cport->cts ? rpmsg_get_mtu(cport->rpdev->ept) : 0;
> +}
> +
> +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)
> +{
> +	p->low_latency = (p->flags & ASYNC_LOW_LATENCY) ? 1 : 0;
> +
> +	/* 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)
> +{
> +	dev_dbg(port->tty->dev, "%s: dtr_rts state %d\n", __func__, 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 rpmsg_channel_info chinfo;
> +	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);
> +	}
> +
> +	if (!strncmp(rpdev->id.name, TTY_CH_NAME_WITH_CTS,
> +		     sizeof(TTY_CH_NAME_WITH_CTS))) {
> +		/*
> +		 * the default endpoint is used for control. Create a second
> +		 * endpoint for the data that would be exchanges trough control
> +		 * endpoint. address of the data endpoint will be provided with
> +		 * the cts state
> +		 */
> +		cport->cs_ept = rpdev->ept;
> +		cport->data_dst = RPMSG_ADDR_ANY;
> +
> +		strscpy(chinfo.name, TTY_CH_NAME_WITH_CTS, sizeof(chinfo.name));

Shouldn't this be TTY_CH_NAME_RAW instead of TTY_CH_NAME_WITH_CTS?  

> +		chinfo.src = RPMSG_ADDR_ANY;
> +		chinfo.dst = RPMSG_ADDR_ANY;
> +
> +		cport->d_ept = rpmsg_create_ept(rpdev, rpmsg_tty_cb, cport,
> +						chinfo);
> +		if (!cport->d_ept) {
> +			dev_err(dev, "failed to create tty control channel\n");

Here too I don't understand why we are talking about the control channel when
the data channel is created.  Am I missing something?

Also I suggest function rpmsg_tty_cp() to be split, one for control and one for
data.  That will make it easier to follow who processes what.

> +			ret = -ENOMEM;
> +			goto err_r_cport;
> +		}
> +		dev_dbg(dev, "%s: creating data endpoint with address %#x\n",
> +			__func__, cport->d_ept->addr);
> +	} else {
> +		/*
> +		 * TTY over rpmsg without CTS management the default endpoint
> +		 * is use for raw data transmission.
> +		 */
> +		cport->cs_ept = NULL;
> +		cport->cts = 1;
> +		cport->d_ept = rpdev->ept;
> +		cport->data_dst = rpdev->dst;
> +	}
> +
> +	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);
> +	if (cport->cs_ept)
> +		rpmsg_destroy_ept(cport->d_ept);
> +err_r_cport:
> +	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);
> +	if (cport->cs_ept)
> +		rpmsg_destroy_ept(cport->d_ept);
> +	rpmsg_tty_release_cport(cport);
> +}
> +
> +static struct rpmsg_device_id rpmsg_driver_tty_id_table[] = {
> +	{ .name	= TTY_CH_NAME_RAW },
> +	{ .name	= TTY_CH_NAME_WITH_CTS},

If I'm not mistaken support for more than one tty
per remote proc can't happen because of rpmsg_find_device() in
rpmsg_create_channel() - is this correct?

Thanks,
Mathieu

> +	{ },
> +};
> +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 <[e-mail]>");
> +MODULE_DESCRIPTION("remote processor messaging tty driver");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.17.1
>

Arnaud POULIQUEN April 2, 2020, 3:25 p.m. UTC | #20

Hi Mathieu,

On 4/1/20 8:06 PM, Mathieu Poirier wrote:
> On Tue, Mar 24, 2020 at 06:04:07PM +0100, 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 <[e-mail]>
>> ---
>>  Documentation/serial/tty_rpmsg.rst |  45 ++++
>>  drivers/tty/Kconfig                |   9 +
>>  drivers/tty/Makefile               |   1 +
>>  drivers/tty/rpmsg_tty.c            | 417 +++++++++++++++++++++++++++++
>>  4 files changed, 472 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..fc1d3fba73c5
>> --- /dev/null
>> +++ b/Documentation/serial/tty_rpmsg.rst
>> @@ -0,0 +1,45 @@
>> +.. SPDX-License-Identifier: GPL-2.0
>> +
>> +=============
>> +The 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-raw" RPMsg service, for TTY raw data support without flow control
>> +- a "rpmsg-tty-ctrl" RPMSg service, for TTY support with flow control.
>> +
>> +Information related to the RPMsg and associated tty device is available in
>> +/sys/bus/rpmsg/devices/.
>> +
>> +RPMsg TTY without control
>> +---------------------
>> +
>> +The default end point associated with the "rpmsg-tty-raw" service is directly
>> +used for data exchange. No flow control is available.
>> +
>> +To be compliant with this driver, the remote firmware must create its data end point associated with the "rpmsg-tty-raw" service.
>> +
>> +RPMsg TTY with control
>> +---------------------
>> +
>> +The default end point associated with the "rpmsg-tty-ctrl" service is reserved for
>> +the control. A second endpoint must be created for data exchange.
>> +
>> +The control channel is used to transmit to the remote processor the CTS status,
>> +as well as the end point address for data transfer.
>> +
>> +To be compatible with this driver, the remote firmware must create or use its end point associated with "rpmsg-tty-ctrl" service, plus a second endpoint for the data flow.
>> +On Linux rpmsg_tty probes, the data endpoint address and the CTS (set to disable)
>> +is sent to the remote processor.
>> +The remote processor has to respect following rules:
>> +- It only transmits data when Linux remote cts is enable, otherwise message
>> +  could be lost.
>> +- It can pause/resume reception by sending a control message (rely on CTS state).
>> +
>> +Control message structure:
>> +struct rpmsg_tty_ctrl {
>> +	u8 cts;			/* remote reception status */
>> +	u16 d_ept_addr;		/* data endpoint address */
>> +};
>> diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
>> index a312cb33a99b..9d3ff6df9f25 100644
>> --- a/drivers/tty/Kconfig
>> +++ b/drivers/tty/Kconfig
>> @@ -454,6 +454,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 020b1cd9294f..c2465e7ebc2a 100644
>> --- a/drivers/tty/Makefile
>> +++ b/drivers/tty/Makefile
>> @@ -34,5 +34,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..49ce3b72781a
>> --- /dev/null
>> +++ b/drivers/tty/rpmsg_tty.c
>> @@ -0,0 +1,417 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) STMicroelectronics 2020 - All Rights Reserved
>> + * Authors: Arnaud Pouliquen <[e-mail]> 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
>> +
>> +#define TTY_CH_NAME_RAW		"rpmsg-tty-raw"
>> +#define TTY_CH_NAME_WITH_CTS	"rpmsg-tty-ctrl"
>> +
>> +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_ctrl {
>> +	u16 d_ept_addr;		/* data endpoint address */
>> +	u8 cts;			/* remote reception status */
>> +} __packed;
>> +
>> +struct rpmsg_tty_port {
>> +	struct tty_port		port;	 /* TTY port data */
>> +	int			id;	 /* TTY rpmsg index */
>> +	bool			cts;	 /* remote reception status */
>> +	struct rpmsg_device	*rpdev;	 /* rpmsg device */
>> +	struct rpmsg_endpoint   *cs_ept; /* channel control endpoint */
>> +	struct rpmsg_endpoint   *d_ept;  /* data endpoint */
>> +	u32 data_dst;			 /* data destination endpoint address */
>> +};
>> +
>> +typedef void (*rpmsg_tty_rx_cb_t)(struct rpmsg_device *, void *, int, void *,
>> +				  u32);
>> +
>> +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 (src == cport->data_dst) {
>> +		/* data message */
>> +		if (!len)
>> +			return -EINVAL;
>> +		copied = tty_insert_flip_string_fixed_flag(&cport->port, data,
>> +							   TTY_NORMAL, len);
>> +		if (copied != len)
>> +			dev_dbg(&rpdev->dev, "trunc buffer: available space is %d\n",
>> +				copied);
>> +		tty_flip_buffer_push(&cport->port);
>> +	} else {
>> +		/* control message */
>> +		struct rpmsg_tty_ctrl *msg = data;
>> +
>> +		if (len != sizeof(*msg))
>> +			return -EINVAL;
>> +
>> +		cport->data_dst = msg->d_ept_addr;
>> +
>> +		/* Update remote cts state */
>> +		cport->cts = msg->cts ? 1 : 0;
>> +
>> +		if (cport->cts)
>> +			tty_port_tty_wakeup(&cport->port);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void rpmsg_tty_send_term_ready(struct tty_struct *tty, u8 state)
>> +{
>> +	struct rpmsg_tty_port *cport = tty->driver_data;
>> +	struct rpmsg_tty_ctrl m_ctrl;
>> +	int ret;
>> +
>> +	m_ctrl.cts = state;
>> +	m_ctrl.d_ept_addr = cport->d_ept->addr;
>> +
>> +	ret = rpmsg_trysend(cport->cs_ept, &m_ctrl, sizeof(m_ctrl));
>> +	if (ret < 0)
>> +		dev_dbg(tty->dev, "cannot send control (%d)\n", ret);
>> +};
>> +
>> +static void rpmsg_tty_throttle(struct tty_struct *tty)
>> +{
>> +	struct rpmsg_tty_port *cport = tty->driver_data;
>> +
>> +	/* Disable remote transmission */
>> +	if (cport->cs_ept)
>> +		rpmsg_tty_send_term_ready(tty, 0);
>> +};
>> +
>> +static void rpmsg_tty_unthrottle(struct tty_struct *tty)
>> +{
>> +	struct rpmsg_tty_port *cport = tty->driver_data;
>> +
>> +	/* Enable remote transmission */
>> +	if (cport->cs_ept)
>> +		rpmsg_tty_send_term_ready(tty, 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;
>> +	}
>> +
>> +	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;
>> +	u8 *tmpbuf;
>> +
>> +	/* 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);
>> +
>> +	msg_max_size = rpmsg_get_mtu(rpdev->ept);
>> +
>> +	msg_size = min(len, msg_max_size);
>> +	tmpbuf = kzalloc(msg_size, GFP_KERNEL);
>> +	if (!tmpbuf)
>> +		return -ENOMEM;
>> +
>> +	memcpy(tmpbuf, buf, msg_size);
>> +
>> +	/*
>> +	 * Try to send the message to remote processor, if failed return 0 as
>> +	 * no data sent
>> +	 */
>> +	ret = rpmsg_trysendto(cport->d_ept, tmpbuf, msg_size, cport->data_dst);
>> +	kfree(tmpbuf);
>> +	if (ret) {
>> +		dev_dbg(&rpdev->dev, "rpmsg_send failed: %d\n", ret);
>> +		return 0;
>> +	}
>> +
>> +	return msg_size;
>> +}
>> +
>> +static int rpmsg_tty_write_room(struct tty_struct *tty)
>> +{
>> +	struct rpmsg_tty_port *cport = tty->driver_data;
>> +
>> +	return cport->cts ? rpmsg_get_mtu(cport->rpdev->ept) : 0;
>> +}
>> +
>> +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)
>> +{
>> +	p->low_latency = (p->flags & ASYNC_LOW_LATENCY) ? 1 : 0;
>> +
>> +	/* 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)
>> +{
>> +	dev_dbg(port->tty->dev, "%s: dtr_rts state %d\n", __func__, 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 rpmsg_channel_info chinfo;
>> +	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);
>> +	}
>> +
>> +	if (!strncmp(rpdev->id.name, TTY_CH_NAME_WITH_CTS,
>> +		     sizeof(TTY_CH_NAME_WITH_CTS))) {
>> +		/*
>> +		 * the default endpoint is used for control. Create a second
>> +		 * endpoint for the data that would be exchanges trough control
>> +		 * endpoint. address of the data endpoint will be provided with
>> +		 * the cts state
>> +		 */
>> +		cport->cs_ept = rpdev->ept;
>> +		cport->data_dst = RPMSG_ADDR_ANY;
>> +
>> +		strscpy(chinfo.name, TTY_CH_NAME_WITH_CTS, sizeof(chinfo.name));
> 
> Shouldn't this be TTY_CH_NAME_RAW instead of TTY_CH_NAME_WITH_CTS?
The TTY_CH_NAME_WITH_CTS represent here the service "TTY support with flow control"
the aim here is to define 2 endpoints on top of the service channel.
the main is used for the CTS control but also to exchange the addresses of the 
second endpoint that is used for the data flow. 
here if you associate the 2nd endpoint to a différent service name, you have no
correlation between the control ept and the data endpoint ( at least if you
want to support multi instance)

I can probaly implement it in a different way using your path [1]
by creating a correlation between the control and the data, based
on the naming.
something like:
- "tty-featureA"  /* data stream associated to featureA"
- "tty-featureB"  /* data stream associated to featureB"
- "tty-ctl-featureB"  /* control associated to featureB"

That would be probably more straight forward for users...

[1] https://lkml.org/lkml/2020/2/12/1083
> 
>> +		chinfo.src = RPMSG_ADDR_ANY;
>> +		chinfo.dst = RPMSG_ADDR_ANY;
>> +
>> +		cport->d_ept = rpmsg_create_ept(rpdev, rpmsg_tty_cb, cport,
>> +						chinfo);
>> +		if (!cport->d_ept) {
>> +			dev_err(dev, "failed to create tty control channel\n");
> 
> Here too I don't understand why we are talking about the control channel when
> the data channel is created.  Am I missing something?
> 
> Also I suggest function rpmsg_tty_cp() to be split, one for control and one for
> data.  That will make it easier to follow who processes what.

you mean rpmsg_tty_probe?  yes i will.

> 
>> +			ret = -ENOMEM;
>> +			goto err_r_cport;
>> +		}
>> +		dev_dbg(dev, "%s: creating data endpoint with address %#x\n",
>> +			__func__, cport->d_ept->addr);
>> +	} else {
>> +		/*
>> +		 * TTY over rpmsg without CTS management the default endpoint
>> +		 * is use for raw data transmission.
>> +		 */
>> +		cport->cs_ept = NULL;
>> +		cport->cts = 1;
>> +		cport->d_ept = rpdev->ept;
>> +		cport->data_dst = rpdev->dst;
>> +	}
>> +
>> +	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);
>> +	if (cport->cs_ept)
>> +		rpmsg_destroy_ept(cport->d_ept);
>> +err_r_cport:
>> +	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);
>> +	if (cport->cs_ept)
>> +		rpmsg_destroy_ept(cport->d_ept);
>> +	rpmsg_tty_release_cport(cport);
>> +}
>> +
>> +static struct rpmsg_device_id rpmsg_driver_tty_id_table[] = {
>> +	{ .name	= TTY_CH_NAME_RAW },
>> +	{ .name	= TTY_CH_NAME_WITH_CTS},
> 
> If I'm not mistaken support for more than one tty
> per remote proc can't happen because of rpmsg_find_device() in
> rpmsg_create_channel() - is this correct?

There is not bloker to instantiate the same service several times.
On remote side it is enough to create 2 endpoints with the same service
name and with the destination address set to RPMSG_ADDR_ANY. This will
trig 2 rpmsg_ns_cb, that will probe 2 times the device.

As example please have a look to the stm32MP1 sample here which creates
2 tty rpmsg channels:
https://github.com/STMicroelectronics/STM32CubeMP1/tree/master/Projects/STM32MP157C-DK2/Applications/OpenAMP/OpenAMP_TTY_echo 

Thanks,
Arnaud

> 
> Thanks,
> Mathieu
> 
>> +	{ },
>> +};
>> +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 <[e-mail]>");
>> +MODULE_DESCRIPTION("remote processor messaging tty driver");
>> +MODULE_LICENSE("GPL v2");
>> -- 
>> 2.17.1
>>

Mathieu Poirier April 3, 2020, 8:18 p.m. UTC | #21

On Thu, 2 Apr 2020 at 09:25, Arnaud POULIQUEN <[e-mail]> wrote:
>
> Hi Mathieu,
>
> On 4/1/20 8:06 PM, Mathieu Poirier wrote:
> > On Tue, Mar 24, 2020 at 06:04:07PM +0100, 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 <[e-mail]>
> >> ---
> >>  Documentation/serial/tty_rpmsg.rst |  45 ++++
> >>  drivers/tty/Kconfig                |   9 +
> >>  drivers/tty/Makefile               |   1 +
> >>  drivers/tty/rpmsg_tty.c            | 417 +++++++++++++++++++++++++++++
> >>  4 files changed, 472 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..fc1d3fba73c5
> >> --- /dev/null
> >> +++ b/Documentation/serial/tty_rpmsg.rst
> >> @@ -0,0 +1,45 @@
> >> +.. SPDX-License-Identifier: GPL-2.0
> >> +
> >> +=============
> >> +The 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-raw" RPMsg service, for TTY raw data support without flow control
> >> +- a "rpmsg-tty-ctrl" RPMSg service, for TTY support with flow control.
> >> +
> >> +Information related to the RPMsg and associated tty device is available in
> >> +/sys/bus/rpmsg/devices/.
> >> +
> >> +RPMsg TTY without control
> >> +---------------------
> >> +
> >> +The default end point associated with the "rpmsg-tty-raw" service is directly
> >> +used for data exchange. No flow control is available.
> >> +
> >> +To be compliant with this driver, the remote firmware must create its data end point associated with the "rpmsg-tty-raw" service.
> >> +
> >> +RPMsg TTY with control
> >> +---------------------
> >> +
> >> +The default end point associated with the "rpmsg-tty-ctrl" service is reserved for
> >> +the control. A second endpoint must be created for data exchange.
> >> +
> >> +The control channel is used to transmit to the remote processor the CTS status,
> >> +as well as the end point address for data transfer.
> >> +
> >> +To be compatible with this driver, the remote firmware must create or use its end point associated with "rpmsg-tty-ctrl" service, plus a second endpoint for the data flow.
> >> +On Linux rpmsg_tty probes, the data endpoint address and the CTS (set to disable)
> >> +is sent to the remote processor.
> >> +The remote processor has to respect following rules:
> >> +- It only transmits data when Linux remote cts is enable, otherwise message
> >> +  could be lost.
> >> +- It can pause/resume reception by sending a control message (rely on CTS state).
> >> +
> >> +Control message structure:
> >> +struct rpmsg_tty_ctrl {
> >> +    u8 cts;                 /* remote reception status */
> >> +    u16 d_ept_addr;         /* data endpoint address */
> >> +};
> >> diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
> >> index a312cb33a99b..9d3ff6df9f25 100644
> >> --- a/drivers/tty/Kconfig
> >> +++ b/drivers/tty/Kconfig
> >> @@ -454,6 +454,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 020b1cd9294f..c2465e7ebc2a 100644
> >> --- a/drivers/tty/Makefile
> >> +++ b/drivers/tty/Makefile
> >> @@ -34,5 +34,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..49ce3b72781a
> >> --- /dev/null
> >> +++ b/drivers/tty/rpmsg_tty.c
> >> @@ -0,0 +1,417 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Copyright (C) STMicroelectronics 2020 - All Rights Reserved
> >> + * Authors: Arnaud Pouliquen <[e-mail]> 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
> >> +
> >> +#define TTY_CH_NAME_RAW             "rpmsg-tty-raw"
> >> +#define TTY_CH_NAME_WITH_CTS        "rpmsg-tty-ctrl"
> >> +
> >> +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_ctrl {
> >> +    u16 d_ept_addr;         /* data endpoint address */
> >> +    u8 cts;                 /* remote reception status */
> >> +} __packed;
> >> +
> >> +struct rpmsg_tty_port {
> >> +    struct tty_port         port;    /* TTY port data */
> >> +    int                     id;      /* TTY rpmsg index */
> >> +    bool                    cts;     /* remote reception status */
> >> +    struct rpmsg_device     *rpdev;  /* rpmsg device */
> >> +    struct rpmsg_endpoint   *cs_ept; /* channel control endpoint */
> >> +    struct rpmsg_endpoint   *d_ept;  /* data endpoint */
> >> +    u32 data_dst;                    /* data destination endpoint address */
> >> +};
> >> +
> >> +typedef void (*rpmsg_tty_rx_cb_t)(struct rpmsg_device *, void *, int, void *,
> >> +                              u32);
> >> +
> >> +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 (src == cport->data_dst) {
> >> +            /* data message */
> >> +            if (!len)
> >> +                    return -EINVAL;
> >> +            copied = tty_insert_flip_string_fixed_flag(&cport->port, data,
> >> +                                                       TTY_NORMAL, len);
> >> +            if (copied != len)
> >> +                    dev_dbg(&rpdev->dev, "trunc buffer: available space is %d\n",
> >> +                            copied);
> >> +            tty_flip_buffer_push(&cport->port);
> >> +    } else {
> >> +            /* control message */
> >> +            struct rpmsg_tty_ctrl *msg = data;
> >> +
> >> +            if (len != sizeof(*msg))
> >> +                    return -EINVAL;
> >> +
> >> +            cport->data_dst = msg->d_ept_addr;
> >> +
> >> +            /* Update remote cts state */
> >> +            cport->cts = msg->cts ? 1 : 0;
> >> +
> >> +            if (cport->cts)
> >> +                    tty_port_tty_wakeup(&cport->port);
> >> +    }
> >> +
> >> +    return 0;
> >> +}
> >> +
> >> +static void rpmsg_tty_send_term_ready(struct tty_struct *tty, u8 state)
> >> +{
> >> +    struct rpmsg_tty_port *cport = tty->driver_data;
> >> +    struct rpmsg_tty_ctrl m_ctrl;
> >> +    int ret;
> >> +
> >> +    m_ctrl.cts = state;
> >> +    m_ctrl.d_ept_addr = cport->d_ept->addr;
> >> +
> >> +    ret = rpmsg_trysend(cport->cs_ept, &m_ctrl, sizeof(m_ctrl));
> >> +    if (ret < 0)
> >> +            dev_dbg(tty->dev, "cannot send control (%d)\n", ret);
> >> +};
> >> +
> >> +static void rpmsg_tty_throttle(struct tty_struct *tty)
> >> +{
> >> +    struct rpmsg_tty_port *cport = tty->driver_data;
> >> +
> >> +    /* Disable remote transmission */
> >> +    if (cport->cs_ept)
> >> +            rpmsg_tty_send_term_ready(tty, 0);
> >> +};
> >> +
> >> +static void rpmsg_tty_unthrottle(struct tty_struct *tty)
> >> +{
> >> +    struct rpmsg_tty_port *cport = tty->driver_data;
> >> +
> >> +    /* Enable remote transmission */
> >> +    if (cport->cs_ept)
> >> +            rpmsg_tty_send_term_ready(tty, 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;
> >> +    }
> >> +
> >> +    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;
> >> +    u8 *tmpbuf;
> >> +
> >> +    /* 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);
> >> +
> >> +    msg_max_size = rpmsg_get_mtu(rpdev->ept);
> >> +
> >> +    msg_size = min(len, msg_max_size);
> >> +    tmpbuf = kzalloc(msg_size, GFP_KERNEL);
> >> +    if (!tmpbuf)
> >> +            return -ENOMEM;
> >> +
> >> +    memcpy(tmpbuf, buf, msg_size);
> >> +
> >> +    /*
> >> +     * Try to send the message to remote processor, if failed return 0 as
> >> +     * no data sent
> >> +     */
> >> +    ret = rpmsg_trysendto(cport->d_ept, tmpbuf, msg_size, cport->data_dst);
> >> +    kfree(tmpbuf);
> >> +    if (ret) {
> >> +            dev_dbg(&rpdev->dev, "rpmsg_send failed: %d\n", ret);
> >> +            return 0;
> >> +    }
> >> +
> >> +    return msg_size;
> >> +}
> >> +
> >> +static int rpmsg_tty_write_room(struct tty_struct *tty)
> >> +{
> >> +    struct rpmsg_tty_port *cport = tty->driver_data;
> >> +
> >> +    return cport->cts ? rpmsg_get_mtu(cport->rpdev->ept) : 0;
> >> +}
> >> +
> >> +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)
> >> +{
> >> +    p->low_latency = (p->flags & ASYNC_LOW_LATENCY) ? 1 : 0;
> >> +
> >> +    /* 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)
> >> +{
> >> +    dev_dbg(port->tty->dev, "%s: dtr_rts state %d\n", __func__, 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 rpmsg_channel_info chinfo;
> >> +    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);
> >> +    }
> >> +
> >> +    if (!strncmp(rpdev->id.name, TTY_CH_NAME_WITH_CTS,
> >> +                 sizeof(TTY_CH_NAME_WITH_CTS))) {
> >> +            /*
> >> +             * the default endpoint is used for control. Create a second
> >> +             * endpoint for the data that would be exchanges trough control
> >> +             * endpoint. address of the data endpoint will be provided with
> >> +             * the cts state
> >> +             */
> >> +            cport->cs_ept = rpdev->ept;
> >> +            cport->data_dst = RPMSG_ADDR_ANY;
> >> +
> >> +            strscpy(chinfo.name, TTY_CH_NAME_WITH_CTS, sizeof(chinfo.name));
> >
> > Shouldn't this be TTY_CH_NAME_RAW instead of TTY_CH_NAME_WITH_CTS?
> The TTY_CH_NAME_WITH_CTS represent here the service "TTY support with flow control"
> the aim here is to define 2 endpoints on top of the service channel.
> the main is used for the CTS control but also to exchange the addresses of the
> second endpoint that is used for the data flow.

Right - the new endpoint, the one that is about to be created using
"chinfo.name" is a data endpoint, hence the confusion.

> here if you associate the 2nd endpoint to a différent service name, you have no
> correlation between the control ept and the data endpoint ( at least if you
> want to support multi instance)
>
> I can probaly implement it in a different way using your path [1]
> by creating a correlation between the control and the data, based
> on the naming.
> something like:
> - "tty-featureA"  /* data stream associated to featureA"
> - "tty-featureB"  /* data stream associated to featureB"
> - "tty-ctl-featureB"  /* control associated to featureB"

I would do "tty-featureB-ctrl".  That way "tty-featureB" and
"tty-featureB-ctrl" always appear one after the other when doing an
"ls" on the command line.

>
> That would be probably more straight forward for users...
>
> [1] https://lkml.org/lkml/2020/2/12/1083
> >
> >> +            chinfo.src = RPMSG_ADDR_ANY;
> >> +            chinfo.dst = RPMSG_ADDR_ANY;
> >> +
> >> +            cport->d_ept = rpmsg_create_ept(rpdev, rpmsg_tty_cb, cport,
> >> +                                            chinfo);
> >> +            if (!cport->d_ept) {
> >> +                    dev_err(dev, "failed to create tty control channel\n");
> >
> > Here too I don't understand why we are talking about the control channel when
> > the data channel is created.  Am I missing something?
> >
> > Also I suggest function rpmsg_tty_cp() to be split, one for control and one for
> > data.  That will make it easier to follow who processes what.
>
> you mean rpmsg_tty_probe?  yes i will.

Splitting rpmsg_tty_probe() would be good too, but I was referring to
rpmsg_tty_cb().

>
> >
> >> +                    ret = -ENOMEM;
> >> +                    goto err_r_cport;
> >> +            }
> >> +            dev_dbg(dev, "%s: creating data endpoint with address %#x\n",
> >> +                    __func__, cport->d_ept->addr);
> >> +    } else {
> >> +            /*
> >> +             * TTY over rpmsg without CTS management the default endpoint
> >> +             * is use for raw data transmission.
> >> +             */
> >> +            cport->cs_ept = NULL;
> >> +            cport->cts = 1;
> >> +            cport->d_ept = rpdev->ept;
> >> +            cport->data_dst = rpdev->dst;
> >> +    }
> >> +
> >> +    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);
> >> +    if (cport->cs_ept)
> >> +            rpmsg_destroy_ept(cport->d_ept);
> >> +err_r_cport:
> >> +    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);
> >> +    if (cport->cs_ept)
> >> +            rpmsg_destroy_ept(cport->d_ept);
> >> +    rpmsg_tty_release_cport(cport);
> >> +}
> >> +
> >> +static struct rpmsg_device_id rpmsg_driver_tty_id_table[] = {
> >> +    { .name = TTY_CH_NAME_RAW },
> >> +    { .name = TTY_CH_NAME_WITH_CTS},
> >
> > If I'm not mistaken support for more than one tty
> > per remote proc can't happen because of rpmsg_find_device() in
> > rpmsg_create_channel() - is this correct?
>
> There is not bloker to instantiate the same service several times.

You are correct.  I took another look at rpmsg_device_match() and the
second condition [2] allows for channels to have the same name for as
long as the destination address is different.

[2]. https://elixir.bootlin.com/linux/v5.6/source/drivers/rpmsg/rpmsg_core.c#L299

> On remote side it is enough to create 2 endpoints with the same service
> name and with the destination address set to RPMSG_ADDR_ANY. This will
> trig 2 rpmsg_ns_cb, that will probe 2 times the device.
>
> As example please have a look to the stm32MP1 sample here which creates
> 2 tty rpmsg channels:
> https://github.com/STMicroelectronics/STM32CubeMP1/tree/master/Projects/STM32MP157C-DK2/Applications/OpenAMP/OpenAMP_TTY_echo
>
> Thanks,
> Arnaud
>
> >
> > Thanks,
> > Mathieu
> >
> >> +    { },
> >> +};
> >> +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 <[e-mail]>");
> >> +MODULE_DESCRIPTION("remote processor messaging tty driver");
> >> +MODULE_LICENSE("GPL v2");
> >> --
> >> 2.17.1
> >>

Arnaud POULIQUEN April 6, 2020, 2:18 p.m. UTC | #22

Hi Bjorn,

On 3/25/20 5:57 PM, Arnaud POULIQUEN wrote:
> Hi Bjorn,
> 
> On 3/24/20 9:52 PM, Bjorn Andersson wrote:
>> On Tue 24 Mar 10:04 PDT 2020, Arnaud Pouliquen wrote:
>> [..]
>>> diff --git a/drivers/tty/Makefile b/drivers/tty/Makefile
>>> index 020b1cd9294f..c2465e7ebc2a 100644
>>> --- a/drivers/tty/Makefile
>>> +++ b/drivers/tty/Makefile
>>> @@ -34,5 +34,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	= TTY_CH_NAME_RAW },
>>> +	{ .name	= TTY_CH_NAME_WITH_CTS},
>>
>> I still don't like the idea that the tty devices are tied to channels by
>> fixed names.
> 
> This point has been discussed with Xiang, he has the same kind of requirement. 
> My proposal here is to do this in two steps. First a fixed name, then
> in a second step we can extend the naming using the implementation proposed
> by Mathieu Poirier:
> 
> [1]https://lkml.org/lkml/2020/2/12/1083
> 
> Is this patch could answer to your requirement?
> 
> if requested i can I can integrate the Mathieu's patch in this patchset.
>  
>>
>> This makes the driver unusable for communicating with any firmware out
>> there that provides tty-like data over a channel with a different name -
>> such as modems with channels providing an AT command interface (they are
>> not named "rpmsg-tty-raw").
> 
> I'm not fixed on the naming, any proposal is welcome.
> If we use the patch [1], could be renamed 
> "rpmsg-tty". then for AT command could be something like "rpmsg-tty-at"
> 
> But here seems we are speaking about service over TTY and not over RPMsg.
> 
>>
>> I also fail to see how you would distinguish ttys when the firmware
>> provides more than a single tty - e.g. say you have a modem-like device
>> that provides an AT command channel and a NMEA stream.
> 
> Today it is a limitation. In fact this limitation is the same for all RPMsg
> devices with multi instance.
> The patch [1] will allow to retrieve the instance by identifying
> the service device name in /sys/class/tty/ttyRPMSG<X>/device/name
> 
>>
>>
>> These are the reasons why drivers/rpmsg/rpmsg_char registers a "control
>> device", from which you can spawn new char devices. As I've said before,
>> I really think the same approach should be taken for ttys - perhaps by
>> just extending the rpmsg_char to allow it to create tty devices in
>> addition to the "packet based" char device?
>>
> I'm not very familiar with the rpmsg_char so please correct me if i'm wrong:
> 
> The rpmsg_char exposes to userland an interface to manage rpmsg channels
> (relying on a char device). This interface offers the  possibility to create
> new channels/endpoints and send/received related messages. 
>  
> Thus, the application declares the RPMsg channels which is bound if they matches
> with the remote processor channel (similar behavior than a kernel rpmsg driver).
> There is no constrain on the service once same service is advertised by remote
> firmware.
> 
> In addition, a limitation of the rpmsg_char device is that it needs to be
> associated with an existing device, as example the implementation in qcom_smd
> driver.
> 
> If i try to figure out how to implement TTY using the rpmsg_char:
> I should create a rpmsg_char dev in the rpmsg tty driver. Then application
> will create channels related to its service. But in this case
> how to ensure that channels created are related to the TTY service?  
> 
> 
> I would also expect to manage RPMsg TTY such as a generic TTY: without
> extra interface and auto mounted as an USB TTY. this means that the
> /dev/ttyRMPSGx are created automatically at remote firmware startup
> (without any application action). For instance a generic application 
> (e.g. minicom) could control an internal remote processor such as
> an external processor through a TTY link. 
> 
> Then we have also similar RPMsg driver for I2C and SPI virtual link. So extend
> the rpmsg_char to support TTY seems not a good solution for long terms. 
> 
> For these reasons i would prefer to have a specific driver. And found a solution
> to allow user to differentiate the TTY instances.
> 
> Anyway I am very interesting in having more details of an implementation relying
> on rpmsg_char if you still thinking that is the good approach here.

Do you think you would find time to move forward with this discussion?
I would like to prepare a v8 to fix issue reported on v7, but as your comments
challenges the driver itself, i would prefer that we first find solutions
that address your concerns.

Thanks,
Arnaud

> 
> Thanks for your comments, 
> Arnaud
> 
>> Regards,
>> Bjorn
>>

Bjorn Andersson May 6, 2020, 2:54 a.m. UTC | #23

On Wed 25 Mar 09:57 PDT 2020, Arnaud POULIQUEN wrote:

> Hi Bjorn,
> 
> On 3/24/20 9:52 PM, Bjorn Andersson wrote:
> > On Tue 24 Mar 10:04 PDT 2020, Arnaud Pouliquen wrote:
> > [..]
> >> diff --git a/drivers/tty/Makefile b/drivers/tty/Makefile
> >> index 020b1cd9294f..c2465e7ebc2a 100644
> >> --- a/drivers/tty/Makefile
> >> +++ b/drivers/tty/Makefile
> >> @@ -34,5 +34,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	= TTY_CH_NAME_RAW },
> >> +	{ .name	= TTY_CH_NAME_WITH_CTS},
> > 
> > I still don't like the idea that the tty devices are tied to channels by
> > fixed names.
> 
> This point has been discussed with Xiang, he has the same kind of requirement. 
> My proposal here is to do this in two steps. First a fixed name, then
> in a second step we can extend the naming using the implementation proposed
> by Mathieu Poirier:
> 
> [1]https://lkml.org/lkml/2020/2/12/1083
> 
> Is this patch could answer to your requirement?
> 
> if requested i can I can integrate the Mathieu's patch in this patchset.
>  
> > 
> > This makes the driver unusable for communicating with any firmware out
> > there that provides tty-like data over a channel with a different name -
> > such as modems with channels providing an AT command interface (they are
> > not named "rpmsg-tty-raw").
> 
> I'm not fixed on the naming, any proposal is welcome.
> If we use the patch [1], could be renamed 
> "rpmsg-tty". then for AT command could be something like "rpmsg-tty-at"
> 
> But here seems we are speaking about service over TTY and not over RPMsg.
> 
> > 
> > I also fail to see how you would distinguish ttys when the firmware
> > provides more than a single tty - e.g. say you have a modem-like device
> > that provides an AT command channel and a NMEA stream.
> 
> Today it is a limitation. In fact this limitation is the same for all RPMsg
> devices with multi instance.
> The patch [1] will allow to retrieve the instance by identifying
> the service device name in /sys/class/tty/ttyRPMSG<X>/device/name
> 
> > 
> > 
> > These are the reasons why drivers/rpmsg/rpmsg_char registers a "control
> > device", from which you can spawn new char devices. As I've said before,
> > I really think the same approach should be taken for ttys - perhaps by
> > just extending the rpmsg_char to allow it to create tty devices in
> > addition to the "packet based" char device?
> > 
> I'm not very familiar with the rpmsg_char so please correct me if i'm wrong:
> 
> The rpmsg_char exposes to userland an interface to manage rpmsg channels
> (relying on a char device). This interface offers the  possibility to create
> new channels/endpoints and send/received related messages. 
>  
> Thus, the application declares the RPMsg channels which is bound if they matches
> with the remote processor channel (similar behavior than a kernel rpmsg driver).
> There is no constrain on the service once same service is advertised by remote
> firmware.
> 
> In addition, a limitation of the rpmsg_char device is that it needs to be
> associated with an existing device, as example the implementation in qcom_smd
> driver.
> 

Correct, the rpmsg_char control device must be associated with a
transport instance, e.g. a virtio rpmsg instance sitting on a
remoteproc. This is necessary in order to be able to tie the dynamically
created rpmsg_char endpoints (i.e. the thing that is similar to your tty
devices) to a particular transport/remoteproc..

The reason why qcom_smd needs to be involved is because of the problem
that I want the control device to appear without depending on particular
channels being exposed by the firmware.

> If i try to figure out how to implement TTY using the rpmsg_char:
> I should create a rpmsg_char dev in the rpmsg tty driver. Then application
> will create channels related to its service. But in this case
> how to ensure that channels created are related to the TTY service?  
> 

My proposal/wish is that 1) rpmsg_char is implemented for virtio/rpmsg,
so that the control device is registered as virtio rpmsg is initiated
and 2) that rpmsg_char is extended to allow creating tty devices in
addition to the existing interface (if the existing read/write interface
isn't enough).

> 
> I would also expect to manage RPMsg TTY such as a generic TTY: without
> extra interface and auto mounted as an USB TTY. this means that the
> /dev/ttyRMPSGx are created automatically at remote firmware startup
> (without any application action). For instance a generic application 
> (e.g. minicom) could control an internal remote processor such as
> an external processor through a TTY link. 
> 

And that's not possible using the two-stage approach rpmsg_char takes,
instead I use udev rules to invoke the ioctl on the control device.

The benefit is that the design of the firmware is not tied to the design
of the Linux system.

> Then we have also similar RPMsg driver for I2C and SPI virtual link. So extend
> the rpmsg_char to support TTY seems not a good solution for long terms. 
> 

What do you mean with this? Are you saying that running tty over rpmsg
over SPI is a bad idea?

> For these reasons i would prefer to have a specific driver. And found a solution
> to allow user to differentiate the TTY instances.
> 
> Anyway I am very interesting in having more details of an implementation relying
> on rpmsg_char if you still thinking that is the good approach here.
> 

I do think it's a good idea to decouple the system design on the Linux
side from the naming of channels provided by the firmware.

Regards,
Bjorn

> Thanks for your comments, 
> Arnaud
> 
> > Regards,
> > Bjorn
> >

Arnaud POULIQUEN May 6, 2020, 10:21 a.m. UTC | #24

On 5/6/20 4:54 AM, Bjorn Andersson wrote:
> On Wed 25 Mar 09:57 PDT 2020, Arnaud POULIQUEN wrote:
> 
>> Hi Bjorn,
>>
>> On 3/24/20 9:52 PM, Bjorn Andersson wrote:
>>> On Tue 24 Mar 10:04 PDT 2020, Arnaud Pouliquen wrote:
>>> [..]
>>>> diff --git a/drivers/tty/Makefile b/drivers/tty/Makefile
>>>> index 020b1cd9294f..c2465e7ebc2a 100644
>>>> --- a/drivers/tty/Makefile
>>>> +++ b/drivers/tty/Makefile
>>>> @@ -34,5 +34,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	= TTY_CH_NAME_RAW },
>>>> +	{ .name	= TTY_CH_NAME_WITH_CTS},
>>>
>>> I still don't like the idea that the tty devices are tied to channels by
>>> fixed names.
>>
>> This point has been discussed with Xiang, he has the same kind of requirement. 
>> My proposal here is to do this in two steps. First a fixed name, then
>> in a second step we can extend the naming using the implementation proposed
>> by Mathieu Poirier:
>>
>> [1]https://lkml.org/lkml/2020/2/12/1083
>>
>> Is this patch could answer to your requirement?
>>
>> if requested i can I can integrate the Mathieu's patch in this patchset.
>>  
>>>
>>> This makes the driver unusable for communicating with any firmware out
>>> there that provides tty-like data over a channel with a different name -
>>> such as modems with channels providing an AT command interface (they are
>>> not named "rpmsg-tty-raw").
>>
>> I'm not fixed on the naming, any proposal is welcome.
>> If we use the patch [1], could be renamed 
>> "rpmsg-tty". then for AT command could be something like "rpmsg-tty-at"
>>
>> But here seems we are speaking about service over TTY and not over RPMsg.
>>
>>>
>>> I also fail to see how you would distinguish ttys when the firmware
>>> provides more than a single tty - e.g. say you have a modem-like device
>>> that provides an AT command channel and a NMEA stream.
>>
>> Today it is a limitation. In fact this limitation is the same for all RPMsg
>> devices with multi instance.
>> The patch [1] will allow to retrieve the instance by identifying
>> the service device name in /sys/class/tty/ttyRPMSG<X>/device/name
>>
>>>
>>>
>>> These are the reasons why drivers/rpmsg/rpmsg_char registers a "control
>>> device", from which you can spawn new char devices. As I've said before,
>>> I really think the same approach should be taken for ttys - perhaps by
>>> just extending the rpmsg_char to allow it to create tty devices in
>>> addition to the "packet based" char device?
>>>
>> I'm not very familiar with the rpmsg_char so please correct me if i'm wrong:
>>
>> The rpmsg_char exposes to userland an interface to manage rpmsg channels
>> (relying on a char device). This interface offers the  possibility to create
>> new channels/endpoints and send/received related messages. 
>>  
>> Thus, the application declares the RPMsg channels which is bound if they matches
>> with the remote processor channel (similar behavior than a kernel rpmsg driver).
>> There is no constrain on the service once same service is advertised by remote
>> firmware.
>>
>> In addition, a limitation of the rpmsg_char device is that it needs to be
>> associated with an existing device, as example the implementation in qcom_smd
>> driver.
>>
> 
> Correct, the rpmsg_char control device must be associated with a
> transport instance, e.g. a virtio rpmsg instance sitting on a
> remoteproc. This is necessary in order to be able to tie the dynamically
> created rpmsg_char endpoints (i.e. the thing that is similar to your tty
> devices) to a particular transport/remoteproc..
> 
> The reason why qcom_smd needs to be involved is because of the problem
> that I want the control device to appear without depending on particular
> channels being exposed by the firmware.
> 
>> If i try to figure out how to implement TTY using the rpmsg_char:
>> I should create a rpmsg_char dev in the rpmsg tty driver. Then application
>> will create channels related to its service. But in this case
>> how to ensure that channels created are related to the TTY service?  
>>
> 
> My proposal/wish is that 1) rpmsg_char is implemented for virtio/rpmsg,
> so that the control device is registered as virtio rpmsg is initiated
> and 2) that rpmsg_char is extended to allow creating tty devices in
> addition to the existing interface (if the existing read/write interface
> isn't enough).

So your proposal is to manage the tty devices throught the rpmsg_char. 

Look to me that we have two use cases here:
1) offer a standard tty device to allows application to communicate with
the remote firmware:
 - allow to use "standard application such as terminal tools,
 - keep application compatibility to communicate with an external processor
 through a serial link or with an co processor through rpmsg.
 - take benefice of the line discipline management uses by applications.
=> need an "auto" probe of the tty device

2) Expose the channels to the userland without devices. This would
be the role of the rpmsg_char probed by the virtio rpmsg.
=> application manages the channels using the char device.
=> only the application has the notion of the service associated to the channel.

Look to me that correlate both would be not flexible. 
I would keep rpmsg_char device channel service agnostic, and let application
manage the service.
In this case it would a design choice to support the service trough the TTY
interface or the rpmsg char device interface.

> 
>>
>> I would also expect to manage RPMsg TTY such as a generic TTY: without
>> extra interface and auto mounted as an USB TTY. this means that the
>> /dev/ttyRMPSGx are created automatically at remote firmware startup
>> (without any application action). For instance a generic application 
>> (e.g. minicom) could control an internal remote processor such as
>> an external processor through a TTY link. 
>>
> 
> And that's not possible using the two-stage approach rpmsg_char takes,
> instead I use udev rules to invoke the ioctl on the control device.
> 
> The benefit is that the design of the firmware is not tied to the design
> of the Linux system.
 
Agree, the rpmsg char device is very interesting for this. Today the alternative
is to implement RPMsg in userland using the OpenAMP library.

> 
>> Then we have also similar RPMsg driver for I2C and SPI virtual link. So extend
>> the rpmsg_char to support TTY seems not a good solution for long terms. 
>>
> 
> What do you mean with this? Are you saying that running tty over rpmsg
> over SPI is a bad idea?

No my point here is that we have also on shelves an rpmsg_i2c and an rpmsg_spi
drivers to manage in the same way a virtual i2c[1] and a virtual SPI with
the possibility to have Linux client drivers and/or userland interface.

So extend rpmsg_char to create a tty device in addition could be the first
step of the support of a lot of in-kernel and/or userland interfaces.

[1] https://github.com/arnopo?tab=projects

> 
>> For these reasons i would prefer to have a specific driver. And found a solution
>> to allow user to differentiate the TTY instances.
>>
>> Anyway I am very interesting in having more details of an implementation relying
>> on rpmsg_char if you still thinking that is the good approach here.
>>
> 
> I do think it's a good idea to decouple the system design on the Linux
> side from the naming of channels provided by the firmware.

IMO both have to be considered in parallel and should not be correlated as far as
possible. Extend the char device to decouple the system from the naming this seems to me a
good solution but another topic.

That said, I may not fully understand your approach. Please don't hesitate to correct me.

In any case, I cannot/will not go ahead with this implementation without your agreement.
So, please, could you tell me if an rpmsg_tty is acceptable from your point of view or if
you consider that it is mandatory to implement the tty device in rpmsg_char.

Thanks,
Arnaud

 
> 
> Regards,
> Bjorn
> 
>> Thanks for your comments, 
>> Arnaud
>>
>>> Regards,
>>> Bjorn
>>>

Patch

diff --git a/Documentation/serial/tty_rpmsg.rst b/Documentation/serial/tty_rpmsg.rst
new file mode 100644
index 000000000000..fc1d3fba73c5
--- /dev/null
+++ b/Documentation/serial/tty_rpmsg.rst
@@ -0,0 +1,45 @@ 
+.. SPDX-License-Identifier: GPL-2.0
+
+=============
+The 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-raw" RPMsg service, for TTY raw data support without flow control
+- a "rpmsg-tty-ctrl" RPMSg service, for TTY support with flow control.
+
+Information related to the RPMsg and associated tty device is available in
+/sys/bus/rpmsg/devices/.
+
+RPMsg TTY without control
+---------------------
+
+The default end point associated with the "rpmsg-tty-raw" service is directly
+used for data exchange. No flow control is available.
+
+To be compliant with this driver, the remote firmware must create its data end point associated with the "rpmsg-tty-raw" service.
+
+RPMsg TTY with control
+---------------------
+
+The default end point associated with the "rpmsg-tty-ctrl" service is reserved for
+the control. A second endpoint must be created for data exchange.
+
+The control channel is used to transmit to the remote processor the CTS status,
+as well as the end point address for data transfer.
+
+To be compatible with this driver, the remote firmware must create or use its end point associated with "rpmsg-tty-ctrl" service, plus a second endpoint for the data flow.
+On Linux rpmsg_tty probes, the data endpoint address and the CTS (set to disable)
+is sent to the remote processor.
+The remote processor has to respect following rules:
+- It only transmits data when Linux remote cts is enable, otherwise message
+  could be lost.
+- It can pause/resume reception by sending a control message (rely on CTS state).
+
+Control message structure:
+struct rpmsg_tty_ctrl {
+	u8 cts;			/* remote reception status */
+	u16 d_ept_addr;		/* data endpoint address */
+};
diff --git a/drivers/tty/Kconfig b/drivers/tty/Kconfig
index a312cb33a99b..9d3ff6df9f25 100644
--- a/drivers/tty/Kconfig
+++ b/drivers/tty/Kconfig
@@ -454,6 +454,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 020b1cd9294f..c2465e7ebc2a 100644
--- a/drivers/tty/Makefile
+++ b/drivers/tty/Makefile
@@ -34,5 +34,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..49ce3b72781a
--- /dev/null
+++ b/drivers/tty/rpmsg_tty.c
@@ -0,0 +1,417 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) STMicroelectronics 2020 - All Rights Reserved
+ * Authors: Arnaud Pouliquen <[e-mail]> 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
+
+#define TTY_CH_NAME_RAW		"rpmsg-tty-raw"
+#define TTY_CH_NAME_WITH_CTS	"rpmsg-tty-ctrl"
+
+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_ctrl {
+	u16 d_ept_addr;		/* data endpoint address */
+	u8 cts;			/* remote reception status */
+} __packed;
+
+struct rpmsg_tty_port {
+	struct tty_port		port;	 /* TTY port data */
+	int			id;	 /* TTY rpmsg index */
+	bool			cts;	 /* remote reception status */
+	struct rpmsg_device	*rpdev;	 /* rpmsg device */
+	struct rpmsg_endpoint   *cs_ept; /* channel control endpoint */
+	struct rpmsg_endpoint   *d_ept;  /* data endpoint */
+	u32 data_dst;			 /* data destination endpoint address */
+};
+
+typedef void (*rpmsg_tty_rx_cb_t)(struct rpmsg_device *, void *, int, void *,
+				  u32);
+
+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 (src == cport->data_dst) {
+		/* data message */
+		if (!len)
+			return -EINVAL;
+		copied = tty_insert_flip_string_fixed_flag(&cport->port, data,
+							   TTY_NORMAL, len);
+		if (copied != len)
+			dev_dbg(&rpdev->dev, "trunc buffer: available space is %d\n",
+				copied);
+		tty_flip_buffer_push(&cport->port);
+	} else {
+		/* control message */
+		struct rpmsg_tty_ctrl *msg = data;
+
+		if (len != sizeof(*msg))
+			return -EINVAL;
+
+		cport->data_dst = msg->d_ept_addr;
+
+		/* Update remote cts state */
+		cport->cts = msg->cts ? 1 : 0;
+
+		if (cport->cts)
+			tty_port_tty_wakeup(&cport->port);
+	}
+
+	return 0;
+}
+
+static void rpmsg_tty_send_term_ready(struct tty_struct *tty, u8 state)
+{
+	struct rpmsg_tty_port *cport = tty->driver_data;
+	struct rpmsg_tty_ctrl m_ctrl;
+	int ret;
+
+	m_ctrl.cts = state;
+	m_ctrl.d_ept_addr = cport->d_ept->addr;
+
+	ret = rpmsg_trysend(cport->cs_ept, &m_ctrl, sizeof(m_ctrl));
+	if (ret < 0)
+		dev_dbg(tty->dev, "cannot send control (%d)\n", ret);
+};
+
+static void rpmsg_tty_throttle(struct tty_struct *tty)
+{
+	struct rpmsg_tty_port *cport = tty->driver_data;
+
+	/* Disable remote transmission */
+	if (cport->cs_ept)
+		rpmsg_tty_send_term_ready(tty, 0);
+};
+
+static void rpmsg_tty_unthrottle(struct tty_struct *tty)
+{
+	struct rpmsg_tty_port *cport = tty->driver_data;
+
+	/* Enable remote transmission */
+	if (cport->cs_ept)
+		rpmsg_tty_send_term_ready(tty, 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;
+	}
+
+	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;
+	u8 *tmpbuf;
+
+	/* 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);
+
+	msg_max_size = rpmsg_get_mtu(rpdev->ept);
+
+	msg_size = min(len, msg_max_size);
+	tmpbuf = kzalloc(msg_size, GFP_KERNEL);
+	if (!tmpbuf)
+		return -ENOMEM;
+
+	memcpy(tmpbuf, buf, msg_size);
+
+	/*
+	 * Try to send the message to remote processor, if failed return 0 as
+	 * no data sent
+	 */
+	ret = rpmsg_trysendto(cport->d_ept, tmpbuf, msg_size, cport->data_dst);
+	kfree(tmpbuf);
+	if (ret) {
+		dev_dbg(&rpdev->dev, "rpmsg_send failed: %d\n", ret);
+		return 0;
+	}
+
+	return msg_size;
+}
+
+static int rpmsg_tty_write_room(struct tty_struct *tty)
+{
+	struct rpmsg_tty_port *cport = tty->driver_data;
+
+	return cport->cts ? rpmsg_get_mtu(cport->rpdev->ept) : 0;
+}
+
+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)
+{
+	p->low_latency = (p->flags & ASYNC_LOW_LATENCY) ? 1 : 0;
+
+	/* 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)
+{
+	dev_dbg(port->tty->dev, "%s: dtr_rts state %d\n", __func__, 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 rpmsg_channel_info chinfo;
+	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);
+	}
+
+	if (!strncmp(rpdev->id.name, TTY_CH_NAME_WITH_CTS,
+		     sizeof(TTY_CH_NAME_WITH_CTS))) {
+		/*
+		 * the default endpoint is used for control. Create a second
+		 * endpoint for the data that would be exchanges trough control
+		 * endpoint. address of the data endpoint will be provided with
+		 * the cts state
+		 */
+		cport->cs_ept = rpdev->ept;
+		cport->data_dst = RPMSG_ADDR_ANY;
+
+		strscpy(chinfo.name, TTY_CH_NAME_WITH_CTS, sizeof(chinfo.name));
+		chinfo.src = RPMSG_ADDR_ANY;
+		chinfo.dst = RPMSG_ADDR_ANY;
+
+		cport->d_ept = rpmsg_create_ept(rpdev, rpmsg_tty_cb, cport,
+						chinfo);
+		if (!cport->d_ept) {
+			dev_err(dev, "failed to create tty control channel\n");
+			ret = -ENOMEM;
+			goto err_r_cport;
+		}
+		dev_dbg(dev, "%s: creating data endpoint with address %#x\n",
+			__func__, cport->d_ept->addr);
+	} else {
+		/*
+		 * TTY over rpmsg without CTS management the default endpoint
+		 * is use for raw data transmission.
+		 */
+		cport->cs_ept = NULL;
+		cport->cts = 1;
+		cport->d_ept = rpdev->ept;
+		cport->data_dst = rpdev->dst;
+	}
+
+	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);
+	if (cport->cs_ept)
+		rpmsg_destroy_ept(cport->d_ept);
+err_r_cport:
+	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);
+	if (cport->cs_ept)
+		rpmsg_destroy_ept(cport->d_ept);
+	rpmsg_tty_release_cport(cport);
+}
+
+static struct rpmsg_device_id rpmsg_driver_tty_id_table[] = {
+	{ .name	= TTY_CH_NAME_RAW },
+	{ .name	= TTY_CH_NAME_WITH_CTS},
+	{ },
+};
+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 <[e-mail]>");
+MODULE_DESCRIPTION("remote processor messaging tty driver");
+MODULE_LICENSE("GPL v2");