lkml.org 
[lkml]   [2021]   [Oct]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v5 0/4] Restructure the rpmsg char to decorrelate the control part.
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
> >>

\
 
 \ /
  Last update: 2021-10-19 05:26    [W:0.080 / U:0.592 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site