Re: [PATCH 00/10] rpmsg: Make RPMSG name service modular

[Mathieu Poirier <mathieu.poirier@linaro.org>]

On Thu, Sep 24, 2020 at 08:53:56AM +0200, Guennadi Liakhovetski wrote:
> Hi Mathieu,
> 
> Sorry for a delayed response, after I'd sent that my message I've 
> subscribed to remoteproc and it seems during that transition some 
> messages were only delivered from the list and not directly to me 
> or something similar has happened.
>

Ok
 
> On Tue, Sep 22, 2020 at 01:12:41PM -0600, Mathieu Poirier wrote:
> > Good day Guennadi,
> > 
> > On Tue, 22 Sep 2020 at 02:09, Guennadi Liakhovetski
> > <guennadi.liakhovetski@linux.intel.com> wrote:
> > >
> > > Hi Mathieu,
> > >
> > > Thanks for the patches. I'm trying to understand the concept of
> > > this approach and I'm probably failing at that. It seems to me
> > > that this patch set is making the NS announcement service to a
> > > separate RPMsg device and I don't understand the reasoning for
> > > doing this. As far as I understand namespace announcements
> > > belong to RPMsg devices / channels, they create a dedicated
> > > endpoint on them with a fixed pre-defined address. But they
> > > don't form a separate RPMsg device. I think the current
> > > virtio_rpmsg_bus.c has that correctly: for each rpmsg device /
> > > channel multiple endpoints can be created, where the NS
> > > service is one of them. It's just an endpoing of an rpmsg
> > > device, not a complete separate device. Have I misunderstood
> > > anything?
> > 
> > This patchset does not introduce any new features - the end result in
> > terms of functionality is exactly the same.  It is also a carbon copy
> > of the work introduced by Arnaud (hence reusing his patches), with the
> > exception that the code is presented in a slightly different order to
> > allow for a complete dissociation of RPMSG name service from the
> > virtIO transport mechanic.
> > 
> > To make that happen rpmsg device specific byte conversion operations
> > had to be introduced in struct rpmsg_device_ops and the explicit
> > creation of an rpmsg_device associated with the name service (that
> > wasn't needed when name service was welded to virtIO).  But
> > associating a rpmsg_device to the name service doesn't change anything
> > - RPMSG devices are created the same way when name service messages
> > are received from the host or the remote processor.
> 
> Yes, the current rpmsg-virtio code does create *one* rpmsg device when 
> an NS announcement arrives.

Currently an rpmsg_device is created each time a NS announcement is received.  

> Whereas with this patch set the first rpmsg 
> device would be created to probe the NS service driver and the next one 
> would still be created following the code borrowed from rpmsg-virtio 
> when an NS announcement arrives. And I don't see how those two devices 
> now make sense, sorry. I understand one device per channel, but two, of 
> which one is for a certain endpoing only, whereas other endpoints don't 
> create their devices, don't seem very logical to me.

In the current implementation the NS service channel is created automatically
when instantiating an rproc_vdev.  An official rpmsg_device is not needed since
it is implicit.  With this set (and as you noted above) an rpmsg_device to
represent the NS service is registered, the same way other services such as
rpmsg_chrdev are.  After that nothing else changes and no other rpmgs_device
are created until NS request come in.  When an NS request does come in an
rpmsg_device is created, and that for each request that is received.

> 
> Thanks
> Guennadi
> 
> > To prove my theory I ran the rpmsg_client_sample.c and it just worked,
> > no changes to client code needed.
> > 
> > Let's keep talking, it's the only way we'll get through this.
> > 
> > Mathieu
> > 
> > >
> > > Thanks
> > > Guennadi
> > >
> > > On Mon, Sep 21, 2020 at 06:09:50PM -0600, Mathieu Poirier wrote:
> > > > Hi all,
> > > >
> > > > After looking at Guennadi[1] and Arnaud's patchsets[2] it became
> > > > clear that we need to go back to a generic rpmsg_ns_msg structure
> > > > if we wanted to make progress.  To do that some of the work from
> > > > Arnaud had to be modified in a way that common name service
> > > > functionality was transport agnostic.
> > > >
> > > > This patchset is based on Arnaud's work but also include a patch
> > > > from Guennadi and some input from me.  It should serve as a
> > > > foundation for the next revision of [1].
> > > >
> > > > Applies on rpmsg-next (4e3dda0bc603) and tested on stm32mp157. I
> > > > did not test the modularisation.
> > > >
> > > > Comments and feedback would be greatly appreciated.
> > > >
> > > > Thanks,
> > > > Mathieu
> > > >
> > > > [1]. https://patchwork.kernel.org/project/linux-remoteproc/list/?series=346593
> > > > [2]. https://patchwork.kernel.org/project/linux-remoteproc/list/?series=338335
> > > >
> > > > Arnaud Pouliquen (5):
> > > >   rpmsg: virtio: rename rpmsg_create_channel
> > > >   rpmsg: core: Add channel creation internal API
> > > >   rpmsg: virtio: Add rpmsg channel device ops
> > > >   rpmsg: Turn name service into a stand alone driver
> > > >   rpmsg: virtio: use rpmsg ns device for the ns announcement
> > > >
> > > > Guennadi Liakhovetski (1):
> > > >   rpmsg: Move common structures and defines to headers
> > > >
> > > > Mathieu Poirier (4):
> > > >   rpmsg: virtio: Move virtio RPMSG structures to private header
> > > >   rpmsg: core: Add RPMSG byte conversion operations
> > > >   rpmsg: virtio: Make endianness conversion virtIO specific
> > > >   rpmsg: ns: Make Name service module transport agnostic
> > > >
> > > >  drivers/rpmsg/Kconfig            |   9 +
> > > >  drivers/rpmsg/Makefile           |   1 +
> > > >  drivers/rpmsg/rpmsg_core.c       |  96 +++++++++++
> > > >  drivers/rpmsg/rpmsg_internal.h   | 102 +++++++++++
> > > >  drivers/rpmsg/rpmsg_ns.c         | 108 ++++++++++++
> > > >  drivers/rpmsg/virtio_rpmsg_bus.c | 284 +++++++++----------------------
> > > >  include/linux/rpmsg_ns.h         |  83 +++++++++
> > > >  include/uapi/linux/rpmsg.h       |   3 +
> > > >  8 files changed, 487 insertions(+), 199 deletions(-)
> > > >  create mode 100644 drivers/rpmsg/rpmsg_ns.c
> > > >  create mode 100644 include/linux/rpmsg_ns.h
> > > >
> > > > --
> > > > 2.25.1
> > > >