Date:   Mon, 11 Oct 2021 12:38:18 +0200
From:   Arnaud POULIQUEN <arnaud.pouliquen@...s.st.com>
To:     Bjorn Andersson <bjorn.andersson@...aro.org>
CC:     Ohad Ben-Cohen <ohad@...ery.com>,
        Mathieu Poirier <mathieu.poirier@...aro.org>,
        <linux-remoteproc@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-stm32@...md-mailman.stormreply.com>
Subject: Re: [PATCH v5 0/4] Restructure the rpmsg char to decorrelate the
 control part.



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@...aro.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).

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.

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.

> 
> 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.

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

Powered by blists - more mailing lists