Re: [PATCH v3 5/6] rpmsg: char: Introduce a rpmsg driver for the rpmsg char device

[Mathieu Poirier]

On Mon, May 17, 2021 at 12:04:29PM +0200, Arnaud POULIQUEN wrote:
> Hello Mathieu,
> 
> On 5/7/21 6:31 PM, Mathieu Poirier wrote:
> > Good morning,
> > 
> > On Fri, May 07, 2021 at 11:30:30AM +0200, Arnaud POULIQUEN wrote:
> >> Hi Mathieu,
> >>
> >> On 5/6/21 6:11 PM, Mathieu Poirier wrote:
> >>> Good day,
> >>>
> >>> On Wed, May 05, 2021 at 08:25:24PM +0200, Arnaud POULIQUEN wrote:
> >>>> Hi Mathieu,
> >>>>
> >>>> On 5/5/21 6:41 PM, Mathieu Poirier wrote:
> >>>>> Hi Arnaud,
> >>>>>
> >>>>> On Thu, Apr 29, 2021 at 03:55:06PM +0200, Arnaud Pouliquen wrote:
> 
> [snip...]
> 
> >>>>>> +};
> >>>>>
> >>>>> The sole purpose of doing this is to create instances of rpmsg_chrdevs from the
> >>>>> name service - but is it really needed?  Up to now and aside from GLINK and SMD,
> >>>>> there asn't been other users of it so I'm wondering if it is worth going through
> >>>>> all this trouble.
> >>>>
> >>>> It is a good point.
> >>>>
> >>>> Just as a reminder, the need of ST and, I assume, some other companies, is to
> >>>> have a basic/generic communication channel to control a remote processor
> >>>> application.
> >>>>
> >>>> Nothing generic exists today for a virtio transport based implementation.
> >>>> Companies have to create their own driver.
> >>>>
> >>>> The purpose of my work is to allow our customer to use RPMsg without developing
> >>>> a specific driver to control remote applications.
> >>>>
> >>>> The rpmsg_chrdev char is a good candidate for this. No protocol, just a simple
> >>>> inter-processor link to send and receive data. The rpmsg_tty is another one.
> >>>>
> >>>> Focusing on the rpmsg_chrdev:
> >>>> We did a part of the work with the first patch set that would be in 5.13.
> >>>> But is it simple to use it for virtio transport based platforms?
> >>>> If we don't implement the NS announcement support in rpmsg_chrdev, using
> >>>> rpmsg_chrdev for a user application seems rather tricky.
> >>>> How to instantiate the communication?
> >>>
> >>> Since we already have /dev/rpmsg_ctrlX user space can instantiate an 
> >>> using that interface, which is how things are done in the GLINK/SMD world.
> >>>
> >>> Wouldn't that cover the usecases you had in mind?
> >>
> >> I have in mind that to make RPMsg easy to use, we need a generic driver with a
> >> basic user interface to send end receive data, that supports the NS announcement:
> >> -  remote side could instantiate it.
> >> -  an instantiation of the device by a Linux application generates a NS
> >> announcement sent to the remote side (for instance to create a channel for debug
> >> trace).
> >>
> > 
> > The communication using a rpmsg_chrdev should be happening in two different ways,
> > i.e RPMSG_CREATE_EPT_IOCTL and RPMSG_CREATE_DEV_IOCTL (as you had in a previous
> > patchset). 
> > 
> > From user space communication using a rpmsg_chrdev should be initiated in two
> > different ways, i.e RPMSG_CREATE_EPT_IOCTL and RPMSG_CREATE_DEV_IOCTL (as you
> > had in a previous patchset). 
> > 
> > Regarding RPMSG_CREATE_EPT_IOCTL, patches 1, 2 and 3 take care of the legacy
> > compatibility and I am quite happy with that.  In this case the driver works the
> > same way regardless of the transport mechanism - virtio, GLINK or SMD.
> 
> Ok i will send a new revision including only this ones, and continue the updates
> in a new patchset.
> 
> > 
> > Then there is instantiation with RPMSG_CREATE_DEV_IOCTL.  That creates a new
> > channel (with endpoint) when coming from /dev/rpmsg_ctrlX.  When we have that
> > functionality we can make the rpmsg_chrdev available from the name service, making
> > sure the end result is the same regardless of source of the request (remote
> > processor or user space).  I was under the impression that functionality would
> > be part of an upcoming patchset.
> > 
> > Unless I'm missing parts of the story, proceeding this way should cover all the
> > requirements we talked about.
> 
> From my windows, there are 3 remaining features:
> - capability to instantiate rpmsg_chrdev from the remote side (NS announcement)

I think this should be #2.

> - capability to instantiate rpmsg_chrdev from local user application
>   (RPMSG_CREATE_DEV_IOCTL)

This should be #1.  Once this is firmly in place #2 (above) should be relatively
easy to implement.  #1 and #2 can be in the same patchset, or not, depending on
what you prefer. 

> - capability to send a NS announcement to the remote side on  rpmsg_chrdev local
> instantiation using RPMSG_CREATE_DEV_IOCTL. This one could be more tricky to
> implement as the endpoint can be created after the channel.

That should probably come after #1 and #2, and in a separate patchset.

> 
> To simplify the review while keeping the overall picture in mind (and perhaps
> prioritize based on other companies' interests), Please, just tell me what would
> be your preference in term of splitting and next step.
> 
> > 
> >> On the other side, the initial work requested by Bjorn seems to be reached:
> >> de-correlate the control part to be able to reuse it for other rpmsg devices.
> >>
> >> I just have the feeling that we are stay in the middle of the road without the
> >> patches 4,5 and 6 to have a first basic interface relying on RPMsg.
> >>
> >>>
> >>> As you pointed out above rpmsg_chrdev should be light and simple - eliminating
> >>> patches 4, 5 and 6 would yield that.
> >>>
> >>
> >> My concern here is more about the complexity of using it by application, for
> >> platforms that rely on virtio rpmsg transport. For instance applications need to
> >> know the notion of local and remote RPMsg addressing.
> >>
> >> Based on your feeling, here is my proposition for next steps:
> >>  1- resend a version a version with only patch 1,2 3 + the patch to clean-up the
> >>    #include in rpmsg_char
> >>  2- switch back to the RPMsg TTY upstream.
> >>  3- extend rpmsg_ctrl IOCTLs to allow instantiate RPMSG_TTY from Linux userland.
> >>
> > 
> > Introducing RPMSG_TTY makes sense if a serial controller is only accessible from
> > the remote processor.  On the flip side it is an overkill if we just want a raw
> > message passing mechanism.  For that the rpmsg_chrdev driver, with the above
> > extention, should be used.
> >  
> 
> Yes the rpmsg_chrdev should be the default one to use for basic communication.

Perfect, we are on the same page.

> The main purpose of the RPMSG_TTY (from ST company POW) is to easy the
> transition in term of communication between an external and an internal
> processor based on a serial link. It provides an abstraction layer that the
> application does not have to manage the transport layer.
> 

Ok

> Both seem to me interesting to implement, but let's continue to focus on
> rpmsg_chrdev first.
> 
> Thanks,
> Arnaud
> 
> >>
> >> Then, we can come back to patches 4, 5 and 6 depending on the feedback from the
> >> users.
> >>
> >> Does this proposition would be OK for you?
> >>
> >> Thanks,
> >> Arnaud
> >>
> >>
> >>>> The application will probably has to scan the /sys/bus/rpmsg/devices/ folder to
> >>>> determine the services and associated remote address.
> >>>>
> >>>> I don't think the QCOM drivers have the same problem because they seems to
> >>>> initiate the communication and work directly with the RPMsg endpoints ( new
> >>>> channel creation on endpoint creation) while Virtio works with the RPMsg channel.
> >>>>
> >>>> By introducing the ability to instantiate rpmsg_chrdevs through the NS
> >>>> announcement, we make this easy for applications to use.
> >>>>
> >>>> And without rpmsg_chrdevs instantiation, It also means that we can't create an
> >>>> RPMsg channel for the rpmsg_chrdevs using a new RPMSG_CREATE_DEV_IOCTL control,
> >>>> right?
> >>>>
> >>>> That said, If we consider that the aim was only to extract the rpmsg_ctrl part,
> >>>> I'm not against leaving the rpmsg_char in this state and switching to the
> >>>> rpmsg_tty driver upstream including the work on the rpmsg_ctrl to create rpmsg
> >>>> channels.
> >>>>
> >>>> We could come back on this if requested by someone else.
> >>>>
> >>>> Thanks,
> >>>> Arnaud
> >>>>
> >>>>>
> >>>>> As such I suggest we don't go out of our way to expose rpmsg_chrdevs to the name
> >>>>> service.  That way patches 4, 5 and 6 of this set can be dropped.
> >>>>>
> >>>>> Thanks,
> >>>>> Mathieu
> >>>>>
>