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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 25 Mar 2020 17:57:26 +0100
From:   Arnaud POULIQUEN <arnaud.pouliquen@...com>
To:     Bjorn Andersson <bjorn.andersson@...aro.org>
CC:     Ohad Ben-Cohen <ohad@...ery.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jslaby@...e.com>, <linux-kernel@...r.kernel.org>,
        <linux-remoteproc@...r.kernel.org>,
        Mathieu Poirier <mathieu.poirier@...aro.org>,
        Suman Anna <s-anna@...com>,
        Fabien DESSENNE <fabien.dessenne@...com>,
        <linux-stm32@...md-mailman.stormreply.com>,
        xiang xiao <xiaoxiang781216@...il.com>
Subject: Re: [PATCH v7 2/2] tty: add rpmsg driver

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
>