| Message ID | 20201222105726.16906-1-arnaud.pouliquen@foss.st.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | introduce generic IOCTL interface for RPMsg channels management | expand |
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 >
Hello Bjorn, On 1/5/21 12:03 AM, Bjorn Andersson wrote: > 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? I'm not sure to understand what is behind your question. I guess the question is why is it useful to create a channel? Mainly to use same way to probe a RPMsg service than the NS announcement. > >> - 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? Yes exactly, this is the main point of the proposal. Having the same RPMsg driver that can be probed either by the remote side using the NS announcement mechanism or by the rpmsg ctrl interface. The driver "just" has to register to the RPMsg ctrl which service it supports. > >> - 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 ? Yes, if the rpmsg_char is probed by the bus, My proposal is to suppress attributes in /sys/class/rpmsg as they are already defined in /sys/bus/rpmsg/devices/ But class attribute can be kept if needed... Thanks, Arnaud > > 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 >>
Hi Arnaud, [...] > > 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 I am finally coming around to review this set. I see that you already had an extensive conversation with Bjorn - did you want me to have a look as well or should I wait for the next revision? Thanks, Mathieu > > -- > 2.17.1 >
Hi Mathieu, On 1/13/21 9:31 PM, Mathieu Poirier wrote: > Hi Arnaud, > > [...] > >> >> 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 > > I am finally coming around to review this set. I see that you already had an > extensive conversation with Bjorn - did you want me to have a look as well or > should I wait for the next revision? Based on Bjorn first feedback, my understanding is that the management based on create/destroy channel does not match with the QCOM RPMsg backend implementation. I think this is the blocking point of my V2 implementation. Before sending a new revision i would hope that we have a roundtable discussion to clarify the direction to move forward, to avoid sending useless revisions. As discussed in [1], there are different alternatives, that probably depend on the features we expect to support. I tried to sum-up the requirement I have in mind in [1]. The 2 main directions I can see are: - rework the rpmsg_char to match with all rpmsg backend (V2 implementation) to be honest i don't know how to move forward in this direction as QCOM and virtio backends are rather different. - not modify the rpmsg_char but create the rpmsg_ctrl (and perhaps also a rpmsg_raw for a /dev/rpmsg data interface) that would use the create/destroy channel such as the rpmsg ns (V1 implementation). one advantage of this solution is that this does not impact QCOM drivers. one drawback is that we duplicate the code. [1] https://patchwork.kernel.org/project/linux-remoteproc/patch/20201222105726.16906-5-arnaud.pouliquen@foss.st.com/ [2] https://patchwork.kernel.org/project/linux-remoteproc/list/?series=327277 Thanks, Arnaud > > Thanks, > Mathieu > >> >> -- >> 2.17.1 >>
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. - 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. - 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/. - 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