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

[Arnaud POULIQUEN]

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,
>