Re: [PATCH v2 00/16] introduce generic IOCTL interface for RPMsg channels management

Bjorn Andersson
2021-01-04T23:11:22Z

On Tue 22 Dec 04:57 CST 2020, Arnaud Pouliquen wrote:

> This series is a restructuring of the RPMsg char driver, to create a generic
> RPMsg ioctl interface for all rpmsg services.
>
> The RPMsg char driver provides interfaces that:
> - expose a char RPMsg device for communication with the remote processor,
> - expose controls interface for applications to create and release endpoints.
>
> The objective of this series is to decorrelate the two interfaces:
> - Provide a char device for a RPMsg raw service in the rpmsg_char that can be
> probed by a RPMsg bus on a ns announcement.
> - Generalize the use of the ioctl for all RPMsg services by creating the
> rpmsg_ctrl, but keep it compatibile with the legacy.
>
> If the V1 create a new rpmsg_raw driver in addition to the rpmsg_ctrl this
> version try to reuse the rpmsg_char driver by addapting QCOM GLINK and SMD
> drivers.
> So a goal of this version is to help to determine the best strategy to move
> forward:
> - reuse rpmsg_char.
> - introduce a new driver and keep rpmsg_char as a legacy driver for a while.
>
> Notice that SMD and GLINK patches have to be tested, only build has been tested.
>
> 1) RPMsg control driver: rpmsg_ctrl.c
> This driver is based on the control part of the RPMsg_char driver.
> On probe a /dev/rpmsg_ctrl<X> interface is created to allow to manage the
> channels.
> The principles are the following:
> - The RPMsg service driver registers it's name and the associated service
> using the rpmsg_ctrl_unregister_ctl API. The list of supported services
> is defined in include/uapi/linux/rpmsg.h and exposed to the
> application thanks to a new field in rpmsg_endpoint_info struct.
> - On the RPMsg bus probe(e.g virtio bus) an rpmsg_ctrl device is
> registered that creates the control interface.
> - The application can then create or release a channel by specifying:
> - the name service
> - the source address.
> - the destination address.

Why is this useful?

> - The rpmsg_ctrl uses the same interface than the ns announcement to
> create and release the associated channel but using the driver_override
> field to force the service name.
> The "driver_override" allows to force the name service associated to
> an RPMsg driver, bypassing the rpmsg_device_id based match check.

You mean, the chinfo driver_override allows the ioctl to specify which
driver should be bound to the device created for the newly registered
endpoint?

> - At least for virtio bus, an associated ns announcement is sent to the
> remote side.
>
> 2) rpmsg char driver: rpmsg_char.c
> - The rpmsg class has not been removed. The associated attributes
> are already available in /sys/bus/rpmsg/.

So today a rpmsg_device gets the same attributes both from the class and
the bus? So the only difference is that there will no longer be a
/sys/class/rpmsg ?

Regards,
Bjorn

> - The eptdev device is now an RPMsg device probed by a RPMsg bus driver
> (probed only by the ioctl in rpmsg_char driver).
>
> Know current Limitations:
> - Tested only with virtio RPMsg bus and for one vdev instance.
> - The glink and smd drivers adaptations have not been tested (not able to test).
> - To limit commit and not update the IOCT interface some features have been not
> implemented in this first step:
> - the NS announcement as not been updated, it is not possible to create an
> endpoint with a destibnation address set to RPMSG_ADDR_ANY (-1),
> - not possible to destroy the channel,
> - only the "rpmsg-raw" service is supported.
>
> This series can be applied in Bjorn's rpmsg-next branch on top of the
> RPMsg_ns series(4c0943255805).
>
> This series can be tested using rpmsgexport tools available here:
> https://github.com/andersson/rpmsgexport.
> ---
> new from V1[1]:
> - In V1 the rpmsg_char.c was not impacted, a rpmsg_raw.c has been created
> instead.
> - IOCTL interface as not been updated (to go by steps).
> - smd and glink drivers has been updated to support channels creation and
> release.
>
> [1] https://patchwork.kernel.org/project/linux-remoteproc/list/?series=327277
>
> Arnaud Pouliquen (16):
> rpmsg: introduce RPMsg control driver for channel creation
> rpmsg: add RPMsg control API to register service
> rpmsg: add override field in channel info
> rpmsg: ctrl: implement the ioctl function to create device
> rpmsg: ns: initialize channel info override field
> rpmsg: add helper to register the rpmsg ctrl device
> rpmsg: char: clean up rpmsg class
> rpmsg: char: make char rpmsg a rpmsg device without the control part
> rpmsg: char: register RPMsg raw service to the ioctl interface.
> rpmsg: char: allow only one endpoint per device
> rpmsg: char: check destination address is not null
> rpmsg: virtio: use the driver_override in channel creation ops
> rpmsg: virtio: probe the rpmsg_ctl device
> rpmsg: glink: add create and release rpmsg channel ops
> rpmsg: smd: add create and release rpmsg channel ops
> rpmsg: replace rpmsg_chrdev_register_device use
>
> drivers/rpmsg/Kconfig | 8 +
> drivers/rpmsg/Makefile | 1 +
> drivers/rpmsg/qcom_glink_native.c | 96 +++++++--
> drivers/rpmsg/qcom_smd.c | 59 +++++-
> drivers/rpmsg/rpmsg_char.c | 246 ++++++-----------------
> drivers/rpmsg/rpmsg_ctrl.c | 320 ++++++++++++++++++++++++++++++
> drivers/rpmsg/rpmsg_internal.h | 14 --
> drivers/rpmsg/rpmsg_ns.c | 1 +
> drivers/rpmsg/virtio_rpmsg_bus.c | 38 +++-
> include/linux/rpmsg.h | 40 ++++
> include/uapi/linux/rpmsg.h | 14 ++
> 11 files changed, 606 insertions(+), 231 deletions(-)
> create mode 100644 drivers/rpmsg/rpmsg_ctrl.c
>
> --
> 2.17.1
>