Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 18 Oct 2021 20:28:11 -0700
From:   Bjorn Andersson <bjorn.andersson@linaro.org>
To:     Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>
Cc:     Ohad Ben-Cohen <ohad@wizery.com>,
        Mathieu Poirier <mathieu.poirier@linaro.org>,
        linux-remoteproc@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-stm32@st-md-mailman.stormreply.com
Subject: Re: [PATCH v5 0/4] Restructure the rpmsg char to decorrelate the
 control part.
Message-ID: <YW47S1HKWKPVHqtp@ripper>
References: <20210712123752.10449-1-arnaud.pouliquen@foss.st.com>
 <YWDSXu/MDOwOLDg0@ripper>
 <8b7179ff-6d0a-8ed5-c0a3-4298fa9b9dc6@foss.st.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <8b7179ff-6d0a-8ed5-c0a3-4298fa9b9dc6@foss.st.com>
Precedence: bulk

On Mon 11 Oct 03:38 PDT 2021, Arnaud POULIQUEN wrote:

> 
> 
> On 10/9/21 1:21 AM, Bjorn Andersson wrote:
> > On Mon 12 Jul 05:37 PDT 2021, Arnaud Pouliquen wrote:
> > 
> >> Main update from V4 [1] 
> >>  - complete commit messages with Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> >>  - rebased on kernel V.14-rc1.
> >>
> >> This series can be applied and tested on "Linux 5.14-rc1"(e73f0f0ee754) branch
> >>
> >> Series description:
> >> This series is the second step in the division of the series [2]: 
> >> "Introducing a Generic IOCTL Interface for RPMsg Channel Management".
> >>
> >> The purpose of this patchset is to split the code related to the control
> >> and the endpoint. The code related to the control part is moved in the rpmsg_ctrl.c.
> > 
> > I'm not convinced about the merits for this refactoring, you're creating
> > yet another kernel module which is fairly tightly coupled with
> > the rpmsg_char kernel module and the only case I can see where this
> > would be useful is if you want to be able to create reach
> > RPMSG_CREATE_DEV_IOCTL and RPMSG_DESTROY_EPT_IOCTL without having to
> > include the rpmsg_char part in your kernel.
> 
> This is what we discussed during a meeting we had on the rpsmg_tty subject the
> July 7, 2020. [1] sump-up what you requested from me before introducing the
> rpmsg tty. But we miss-understood your requirement?
> 
> This work is the result of our discussion:
> - decorrelate the control and stream part of the rpmsg_char to be able to reuse
> the control for other rpmsg services such as the rpmsg_tty.
> - Add capability to instantiate other rpmsg service from Linux user land
> applications.
> 
> The correlation between the rpmsg_char and the rpmsg_ctrl is due to the support
> of the RPMSG_CREATE_EPT_IOCTL RPMSG_DESTROY_EPT_IOCTL legacy controls for the
> QCOM driver.
> 
> At the end I guess the rpmsg_ctrl could become, in the future, a channel for
> endpoint signaling between processors.
> 
> [1] https://lkml.org/lkml/2020/7/15/868
> 
> > 
> >> This split is an intermediate step to extend the controls to allow user applications to
> >> instantiate rpmsg devices.
> 
> >>     
> > 
> > Can you give a concrete example of when this would be used?
> 
> Similar to what it is done with the RPMSG_CREATE_EPT_IOCTL but based on the
> channel not the endpoint (as the rpmsg_bus virtio is channel based).
> 

I've always seen the rpmsg_endpoint as some form of pipe (with the
special case in virtio rpmsg of it possibly not being connected to
anything) and then the rpmsg_channel being essentially the glue between
a "primary" endpoint and an rpmsg_device.

As such I assumed that it would make sense to do NS announcements of
rpmsg_endpoints in general, not only rpmsg_channels.

> For instance we received several issue reports from customer on rpmsg
> communication. The reason was that the coprocessor creates an unidirectional
> channel to transfer data to the main processor. But nothing works because the
> coprocessor doesn't have the remote address until the main processor send a
> first message. The workaround is to send a fake message from the Linux to
> provide is ept address.
> Making this in the other direction allows the Linux application to initiate such
> link when it is ready to receive data.
> 
> Other examples of usage:
> - Create a temporary channel to get for instance logs of the remotre proc
> - destroy and re-create some channels on Linux suspend/resume.
> 

What's the context these two sets of channels live in? A separate
rpmsg_device or you're having some userspace entity invoke the
create/destroy during suspend and resume?

> As the proposal of exposing the capability to userland to initiate the link (if
> i well remember) is coming from you, don't hesitate if you have some extra
> uscase that i can add in the cover letter.
> 

Right, I remember expressing the need to extend rpmsg_char (somehow) to
make it possible for userspace to initiate the creation of a channel to
the other side.

It's the part where userspace pokes the kernel, so that the kernel goes
and create the rpmsg_device, which magically ends up probing some driver
that I'm wondering about...

> > 
> > Per our previous discussions I believe you intend to use this to bind
> > your rpmsg_tty driver to arbitrary channels in runtime, which to me
> > sounds like you're reinventing the bind/unbind sysfs attrs.
> 
> Please tell me if I wrong, but the bind /unbind allows to probe/remove an
> exiting device. the RPMSG_CREATE_DEV_IOCTL creates a new one on the rpmsg bus,
> so not exactly the same use case.
> 

You're correct, it wouldn't allow you to locally create a new
channel/endpoint and have some driver attached to that.

Regards,
Bjorn

> Regards,
> Arnaud
> 
> > 
> > Regards,
> > Bjorn
> > 
> >> Notice that this patchset does not modify the behavior for using the RPMSG_CREATE_EPT_IOCTL
> >> and RPMSG_DESTROY_EPT_IOCTL controls.
> >>   
> >> The next step should be to add the capability to:
> >> - instantiate rpmsg_chrdev from the remote side (NS announcement),
> >> - instantiate rpmsg_chrdev from local user application by introducing the
> >>   IOCTLs RPMSG_CREATE_DEV_IOCTL and RPMSG_DESTROY_DEV_IOCTL to instantiate the rpmsg devices,
> >> - send a NS announcement to the remote side on rpmsg_chrdev local instantiation.
> >>
> >> [1]: https://patchwork.kernel.org/project/linux-remoteproc/list/?series=483793
> >> [2]: https://patchwork.kernel.org/project/linux-remoteproc/list/?series=435523
> >>
> >> Arnaud Pouliquen (4):
> >>   rpmsg: char: Remove useless include
> >>   rpmsg: char: Export eptdev create an destroy functions
> >>   rpmsg: Move the rpmsg control device from rpmsg_char to rpmsg_ctrl
> >>   rpmsg: Update rpmsg_chrdev_register_device function
> >>
> >>  drivers/rpmsg/Kconfig             |   9 ++
> >>  drivers/rpmsg/Makefile            |   1 +
> >>  drivers/rpmsg/qcom_glink_native.c |   2 +-
> >>  drivers/rpmsg/qcom_smd.c          |   2 +-
> >>  drivers/rpmsg/rpmsg_char.c        | 184 ++-----------------------
> >>  drivers/rpmsg/rpmsg_char.h        |  51 +++++++
> >>  drivers/rpmsg/rpmsg_ctrl.c        | 215 ++++++++++++++++++++++++++++++
> >>  drivers/rpmsg/rpmsg_internal.h    |   8 +-
> >>  drivers/rpmsg/virtio_rpmsg_bus.c  |   2 +-
> >>  9 files changed, 293 insertions(+), 181 deletions(-)
> >>  create mode 100644 drivers/rpmsg/rpmsg_char.h
> >>  create mode 100644 drivers/rpmsg/rpmsg_ctrl.c
> >>
> >> -- 
> >> 2.17.1
> >>