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

[Bjorn Andersson]

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
>