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