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@xxxxxxxxxx> > >> - 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 > >>
