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

[Arnaud Pouliquen]

Hello Alan,

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

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

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

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

Regards
Arnaud