mbox series

[v2,00/16] introduce generic IOCTL interface for RPMsg channels management

Message ID 20201222105726.16906-1-arnaud.pouliquen@foss.st.com
Headers show
Series introduce generic IOCTL interface for RPMsg channels management | expand

Message

Arnaud POULIQUEN Dec. 22, 2020, 10:57 a.m. UTC
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

Comments

Bjorn Andersson Jan. 4, 2021, 11:03 p.m. UTC | #1
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

>
Bjorn Andersson Jan. 5, 2021, 12:38 a.m. UTC | #2
On Tue 22 Dec 04:57 CST 2020, Arnaud Pouliquen wrote:

> By default driver_override should be 0 to avoid to force

> the channel creation with a specified name.The local variable

> is not initialized.

> 


The same problem exists in qcom_glink_native, qcom_smd and rpmsg_char.

Regards,
Bjorn

> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>

> ---

>  drivers/rpmsg/rpmsg_ns.c | 1 +

>  1 file changed, 1 insertion(+)

> 

> diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c

> index 762ff1ae279f..a526bff62947 100644

> --- a/drivers/rpmsg/rpmsg_ns.c

> +++ b/drivers/rpmsg/rpmsg_ns.c

> @@ -55,6 +55,7 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len,

>  	strncpy(chinfo.name, msg->name, sizeof(chinfo.name));

>  	chinfo.src = RPMSG_ADDR_ANY;

>  	chinfo.dst = rpmsg32_to_cpu(rpdev, msg->addr);

> +	chinfo.driver_override = NULL;

>  

>  	dev_info(dev, "%sing channel %s addr 0x%x\n",

>  		 rpmsg32_to_cpu(rpdev, msg->flags) & RPMSG_NS_DESTROY ?

> -- 

> 2.17.1

>
Bjorn Andersson Jan. 5, 2021, 12:47 a.m. UTC | #3
On Tue 22 Dec 04:57 CST 2020, Arnaud Pouliquen wrote:

This patch doesn't "clean up" the class, as described in $subject. It
just removes it.

> Suppress the management of the rpmsg class as attribute. It is already

> handled in /sys/bus rpmsg as specified in

> documentation/ABI/testing/sysfs-bus-rpmsg.


Afaict it doesn't relate to /sys/bus/rpmsg, but rather what attributes
should be associated with the rpmsg_device (and thereby present in its
sysfs directory). But if these attributes are also added by the bus,
then why do we have this code? If that's the case this seems like a nice
cleanup that we should do outside/before merging the other patches.

> This patch prepares the migration of the control device in rpmsg_ctrl.

> 


It would be useful if the commit message described how it prepares for
the migration and why.

Regards,
Bjorn

> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>

> ---

>  drivers/rpmsg/rpmsg_char.c | 48 --------------------------------------

>  1 file changed, 48 deletions(-)

> 

> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c

> index 4bbbacdbf3bb..732f5caf068a 100644

> --- a/drivers/rpmsg/rpmsg_char.c

> +++ b/drivers/rpmsg/rpmsg_char.c

> @@ -27,7 +27,6 @@

>  #define RPMSG_DEV_MAX	(MINORMASK + 1)

>  

>  static dev_t rpmsg_major;

> -static struct class *rpmsg_class;

>  

>  static DEFINE_IDA(rpmsg_ctrl_ida);

>  static DEFINE_IDA(rpmsg_ept_ida);

> @@ -291,41 +290,6 @@ static const struct file_operations rpmsg_eptdev_fops = {

>  	.compat_ioctl = compat_ptr_ioctl,

>  };

>  

> -static ssize_t name_show(struct device *dev, struct device_attribute *attr,

> -			 char *buf)

> -{

> -	struct rpmsg_eptdev *eptdev = dev_get_drvdata(dev);

> -

> -	return sprintf(buf, "%s\n", eptdev->chinfo.name);

> -}

> -static DEVICE_ATTR_RO(name);

> -

> -static ssize_t src_show(struct device *dev, struct device_attribute *attr,

> -			 char *buf)

> -{

> -	struct rpmsg_eptdev *eptdev = dev_get_drvdata(dev);

> -

> -	return sprintf(buf, "%d\n", eptdev->chinfo.src);

> -}

> -static DEVICE_ATTR_RO(src);

> -

> -static ssize_t dst_show(struct device *dev, struct device_attribute *attr,

> -			 char *buf)

> -{

> -	struct rpmsg_eptdev *eptdev = dev_get_drvdata(dev);

> -

> -	return sprintf(buf, "%d\n", eptdev->chinfo.dst);

> -}

> -static DEVICE_ATTR_RO(dst);

> -

> -static struct attribute *rpmsg_eptdev_attrs[] = {

> -	&dev_attr_name.attr,

> -	&dev_attr_src.attr,

> -	&dev_attr_dst.attr,

> -	NULL

> -};

> -ATTRIBUTE_GROUPS(rpmsg_eptdev);

> -

>  static void rpmsg_eptdev_release_device(struct device *dev)

>  {

>  	struct rpmsg_eptdev *eptdev = dev_to_eptdev(dev);

> @@ -358,9 +322,7 @@ static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,

>  	init_waitqueue_head(&eptdev->readq);

>  

>  	device_initialize(dev);

> -	dev->class = rpmsg_class;

>  	dev->parent = &ctrldev->dev;

> -	dev->groups = rpmsg_eptdev_groups;

>  	dev_set_drvdata(dev, eptdev);

>  

>  	cdev_init(&eptdev->cdev, &rpmsg_eptdev_fops);

> @@ -477,7 +439,6 @@ static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)

>  	dev = &ctrldev->dev;

>  	device_initialize(dev);

>  	dev->parent = &rpdev->dev;

> -	dev->class = rpmsg_class;

>  

>  	cdev_init(&ctrldev->cdev, &rpmsg_ctrldev_fops);

>  	ctrldev->cdev.owner = THIS_MODULE;

> @@ -553,17 +514,9 @@ static int rpmsg_char_init(void)

>  		return ret;

>  	}

>  

> -	rpmsg_class = class_create(THIS_MODULE, "rpmsg");

> -	if (IS_ERR(rpmsg_class)) {

> -		pr_err("failed to create rpmsg class\n");

> -		unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);

> -		return PTR_ERR(rpmsg_class);

> -	}

> -

>  	ret = register_rpmsg_driver(&rpmsg_chrdev_driver);

>  	if (ret < 0) {

>  		pr_err("rpmsgchr: failed to register rpmsg driver\n");

> -		class_destroy(rpmsg_class);

>  		unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);

>  	}

>  

> @@ -574,7 +527,6 @@ postcore_initcall(rpmsg_char_init);

>  static void rpmsg_chrdev_exit(void)

>  {

>  	unregister_rpmsg_driver(&rpmsg_chrdev_driver);

> -	class_destroy(rpmsg_class);

>  	unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);

>  }

>  module_exit(rpmsg_chrdev_exit);

> -- 

> 2.17.1

>
Bjorn Andersson Jan. 5, 2021, 12:54 a.m. UTC | #4
On Mon 04 Jan 18:47 CST 2021, Bjorn Andersson wrote:

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

> 

> This patch doesn't "clean up" the class, as described in $subject. It

> just removes it.

> 

> > Suppress the management of the rpmsg class as attribute. It is already

> > handled in /sys/bus rpmsg as specified in

> > documentation/ABI/testing/sysfs-bus-rpmsg.

> 

> Afaict it doesn't relate to /sys/bus/rpmsg, but rather what attributes

> should be associated with the rpmsg_device (and thereby present in its

> sysfs directory). But if these attributes are also added by the bus,

> then why do we have this code? If that's the case this seems like a nice

> cleanup that we should do outside/before merging the other patches.

> 

> > This patch prepares the migration of the control device in rpmsg_ctrl.

> > 

> 

> It would be useful if the commit message described how it prepares for

> the migration and why.

> 


Now I see what this patch does, it removes the attributes from the
character device's struct device, because they are provided by the
struct rpmsg_device's bus!

I wish your commit message made this obvious.

Also, this implies that for a few patches here rpmsg_char is just
broken - which I don't like.

Regards,
Bjorn

> Regards,

> Bjorn

> 

> > Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>

> > ---

> >  drivers/rpmsg/rpmsg_char.c | 48 --------------------------------------

> >  1 file changed, 48 deletions(-)

> > 

> > diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c

> > index 4bbbacdbf3bb..732f5caf068a 100644

> > --- a/drivers/rpmsg/rpmsg_char.c

> > +++ b/drivers/rpmsg/rpmsg_char.c

> > @@ -27,7 +27,6 @@

> >  #define RPMSG_DEV_MAX	(MINORMASK + 1)

> >  

> >  static dev_t rpmsg_major;

> > -static struct class *rpmsg_class;

> >  

> >  static DEFINE_IDA(rpmsg_ctrl_ida);

> >  static DEFINE_IDA(rpmsg_ept_ida);

> > @@ -291,41 +290,6 @@ static const struct file_operations rpmsg_eptdev_fops = {

> >  	.compat_ioctl = compat_ptr_ioctl,

> >  };

> >  

> > -static ssize_t name_show(struct device *dev, struct device_attribute *attr,

> > -			 char *buf)

> > -{

> > -	struct rpmsg_eptdev *eptdev = dev_get_drvdata(dev);

> > -

> > -	return sprintf(buf, "%s\n", eptdev->chinfo.name);

> > -}

> > -static DEVICE_ATTR_RO(name);

> > -

> > -static ssize_t src_show(struct device *dev, struct device_attribute *attr,

> > -			 char *buf)

> > -{

> > -	struct rpmsg_eptdev *eptdev = dev_get_drvdata(dev);

> > -

> > -	return sprintf(buf, "%d\n", eptdev->chinfo.src);

> > -}

> > -static DEVICE_ATTR_RO(src);

> > -

> > -static ssize_t dst_show(struct device *dev, struct device_attribute *attr,

> > -			 char *buf)

> > -{

> > -	struct rpmsg_eptdev *eptdev = dev_get_drvdata(dev);

> > -

> > -	return sprintf(buf, "%d\n", eptdev->chinfo.dst);

> > -}

> > -static DEVICE_ATTR_RO(dst);

> > -

> > -static struct attribute *rpmsg_eptdev_attrs[] = {

> > -	&dev_attr_name.attr,

> > -	&dev_attr_src.attr,

> > -	&dev_attr_dst.attr,

> > -	NULL

> > -};

> > -ATTRIBUTE_GROUPS(rpmsg_eptdev);

> > -

> >  static void rpmsg_eptdev_release_device(struct device *dev)

> >  {

> >  	struct rpmsg_eptdev *eptdev = dev_to_eptdev(dev);

> > @@ -358,9 +322,7 @@ static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,

> >  	init_waitqueue_head(&eptdev->readq);

> >  

> >  	device_initialize(dev);

> > -	dev->class = rpmsg_class;

> >  	dev->parent = &ctrldev->dev;

> > -	dev->groups = rpmsg_eptdev_groups;

> >  	dev_set_drvdata(dev, eptdev);

> >  

> >  	cdev_init(&eptdev->cdev, &rpmsg_eptdev_fops);

> > @@ -477,7 +439,6 @@ static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)

> >  	dev = &ctrldev->dev;

> >  	device_initialize(dev);

> >  	dev->parent = &rpdev->dev;

> > -	dev->class = rpmsg_class;

> >  

> >  	cdev_init(&ctrldev->cdev, &rpmsg_ctrldev_fops);

> >  	ctrldev->cdev.owner = THIS_MODULE;

> > @@ -553,17 +514,9 @@ static int rpmsg_char_init(void)

> >  		return ret;

> >  	}

> >  

> > -	rpmsg_class = class_create(THIS_MODULE, "rpmsg");

> > -	if (IS_ERR(rpmsg_class)) {

> > -		pr_err("failed to create rpmsg class\n");

> > -		unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);

> > -		return PTR_ERR(rpmsg_class);

> > -	}

> > -

> >  	ret = register_rpmsg_driver(&rpmsg_chrdev_driver);

> >  	if (ret < 0) {

> >  		pr_err("rpmsgchr: failed to register rpmsg driver\n");

> > -		class_destroy(rpmsg_class);

> >  		unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);

> >  	}

> >  

> > @@ -574,7 +527,6 @@ postcore_initcall(rpmsg_char_init);

> >  static void rpmsg_chrdev_exit(void)

> >  {

> >  	unregister_rpmsg_driver(&rpmsg_chrdev_driver);

> > -	class_destroy(rpmsg_class);

> >  	unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);

> >  }

> >  module_exit(rpmsg_chrdev_exit);

> > -- 

> > 2.17.1

> >
Bjorn Andersson Jan. 5, 2021, 12:59 a.m. UTC | #5
On Tue 22 Dec 04:57 CST 2020, Arnaud Pouliquen wrote:

> Only one endpoint can be created per device, prevent from multi open.

> 


Having multiple invocations of rpmsg_create_ept() with the same chinfo
sounds like a bad idea. I think in the SMD and GLINK case the underlying
transport would complain that the related chinfo is already "busy", but
this seems like an appropriate fix regardless.

Please add a proper Fixes: tag and send this outside of this patch
series.

Thanks,
Bjorn

> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>

> ---

>  drivers/rpmsg/rpmsg_char.c | 3 +++

>  1 file changed, 3 insertions(+)

> 

> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c

> index 66e01b979e72..4b0674a2e3e9 100644

> --- a/drivers/rpmsg/rpmsg_char.c

> +++ b/drivers/rpmsg/rpmsg_char.c

> @@ -122,6 +122,9 @@ static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)

>  	struct rpmsg_device *rpdev = eptdev->rpdev;

>  	struct device *dev = &eptdev->dev;

>  

> +	if (eptdev->ept)

> +		return -EBUSY;

> +

>  	get_device(dev);

>  

>  	ept = rpmsg_create_ept(rpdev, rpmsg_ept_cb, eptdev, eptdev->chinfo);

> -- 

> 2.17.1

>
Bjorn Andersson Jan. 5, 2021, 1:03 a.m. UTC | #6
On Tue 22 Dec 04:57 CST 2020, Arnaud Pouliquen wrote:

> The name service announcement is not sent if no endpoint is created by

> default. If the destination address is not precised by the

> application when creating the device (thanks to the RPMsg CTRL interface),

> it is not possible to have a valid RPMsg channel.

> 


In the Qualcomm transports, the chinfo.name is used to identify the
channel, so there it's valid to create a endpoint without dst.

Regards,
Bjorn

> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>

> ---

>  drivers/rpmsg/rpmsg_char.c | 10 ++++++++++

>  1 file changed, 10 insertions(+)

> 

> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c

> index 4b0674a2e3e9..8b1928594d10 100644

> --- a/drivers/rpmsg/rpmsg_char.c

> +++ b/drivers/rpmsg/rpmsg_char.c

> @@ -305,6 +305,16 @@ static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)

>  	struct device *dev;

>  	int ret;

>  

> +	/* There is not ept created by default. As consequence there is not NS

> +	 * announcement and the destination address has to be set.

> +	 * this limitation could be solved in future by adding a helper in

> +	 * rpmsg_ns.

> +	 */

> +	if (rpdev->dst == RPMSG_ADDR_ANY) {

> +		dev_err(dev, "destination address invalid (%d)\n", rpdev->dst);

> +		return -EINVAL;

> +	}

> +

>  	eptdev = kzalloc(sizeof(*eptdev), GFP_KERNEL);

>  	if (!eptdev)

>  		return -ENOMEM;

> -- 

> 2.17.1

>
Bjorn Andersson Jan. 5, 2021, 1:10 a.m. UTC | #7
On Tue 22 Dec 04:57 CST 2020, Arnaud Pouliquen wrote:

> Replace rpmsg_chrdev_register_device by the new helper

> rpmsg_ctl_register_device to probe the new IOCTL interface.

> 


This again implies that rpmsg_char was broken in SMD and GLINK during a
large part of this series. Your strategy also has the side effect of
having changed the name from /dev/rpmsg_ctrlX to /dev/rpmsg_ctlX,
breaking my userspace.

Regards,
Bjorn

> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>

> ---

>  drivers/rpmsg/qcom_glink_native.c |  2 +-

>  drivers/rpmsg/qcom_smd.c          |  2 +-

>  drivers/rpmsg/rpmsg_internal.h    | 14 --------------

>  3 files changed, 2 insertions(+), 16 deletions(-)

> 

> diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c

> index d74c338de077..6c7bb84f7da9 100644

> --- a/drivers/rpmsg/qcom_glink_native.c

> +++ b/drivers/rpmsg/qcom_glink_native.c

> @@ -1681,7 +1681,7 @@ static int qcom_glink_create_chrdev(struct qcom_glink *glink)

>  	rpdev->dev.parent = glink->dev;

>  	rpdev->dev.release = qcom_glink_device_release;

>  

> -	return rpmsg_chrdev_register_device(rpdev);

> +	return rpmsg_ctl_register_device(rpdev);

>  }

>  

>  struct qcom_glink *qcom_glink_native_probe(struct device *dev,

> diff --git a/drivers/rpmsg/qcom_smd.c b/drivers/rpmsg/qcom_smd.c

> index 40853702f54f..a39457c57705 100644

> --- a/drivers/rpmsg/qcom_smd.c

> +++ b/drivers/rpmsg/qcom_smd.c

> @@ -1138,7 +1138,7 @@ static int qcom_smd_create_chrdev(struct qcom_smd_edge *edge)

>  	qsdev->rpdev.dev.parent = &edge->dev;

>  	qsdev->rpdev.dev.release = qcom_smd_release_device;

>  

> -	return rpmsg_chrdev_register_device(&qsdev->rpdev);

> +	return rpmsg_ctl_register_device(&qsdev->rpdev);

>  }

>  

>  /*

> diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h

> index a76c344253bf..c81dfb374b64 100644

> --- a/drivers/rpmsg/rpmsg_internal.h

> +++ b/drivers/rpmsg/rpmsg_internal.h

> @@ -81,19 +81,5 @@ struct rpmsg_device *rpmsg_create_channel(struct rpmsg_device *rpdev,

>  					  struct rpmsg_channel_info *chinfo);

>  int rpmsg_release_channel(struct rpmsg_device *rpdev,

>  			  struct rpmsg_channel_info *chinfo);

> -/**

> - * rpmsg_chrdev_register_device() - register chrdev device based on rpdev

> - * @rpdev:	prepared rpdev to be used for creating endpoints

> - *

> - * This function wraps rpmsg_register_device() preparing the rpdev for use as

> - * basis for the rpmsg chrdev.

> - */

> -static inline int rpmsg_chrdev_register_device(struct rpmsg_device *rpdev)

> -{

> -	strcpy(rpdev->id.name, "rpmsg_chrdev");

> -	rpdev->driver_override = "rpmsg_chrdev";

> -

> -	return rpmsg_register_device(rpdev);

> -}

>  

>  #endif

> -- 

> 2.17.1

>
Arnaud POULIQUEN Jan. 5, 2021, 4:59 p.m. UTC | #8
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

>>
Arnaud POULIQUEN Jan. 5, 2021, 5:02 p.m. UTC | #9
On 1/5/21 1:38 AM, Bjorn Andersson wrote:
> On Tue 22 Dec 04:57 CST 2020, Arnaud Pouliquen wrote:

> 

>> By default driver_override should be 0 to avoid to force

>> the channel creation with a specified name.The local variable

>> is not initialized.

>>

> 

> The same problem exists in qcom_glink_native, qcom_smd and rpmsg_char.


Right! And perhaps initializing the structure on declaration would be a better
method.

Thanks,
Arnaud

> 

> Regards,

> Bjorn

> 

>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>

>> ---

>>  drivers/rpmsg/rpmsg_ns.c | 1 +

>>  1 file changed, 1 insertion(+)

>>

>> diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c

>> index 762ff1ae279f..a526bff62947 100644

>> --- a/drivers/rpmsg/rpmsg_ns.c

>> +++ b/drivers/rpmsg/rpmsg_ns.c

>> @@ -55,6 +55,7 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len,

>>  	strncpy(chinfo.name, msg->name, sizeof(chinfo.name));

>>  	chinfo.src = RPMSG_ADDR_ANY;

>>  	chinfo.dst = rpmsg32_to_cpu(rpdev, msg->addr);

>> +	chinfo.driver_override = NULL;

>>  

>>  	dev_info(dev, "%sing channel %s addr 0x%x\n",

>>  		 rpmsg32_to_cpu(rpdev, msg->flags) & RPMSG_NS_DESTROY ?

>> -- 

>> 2.17.1

>>
Arnaud POULIQUEN Jan. 5, 2021, 5:03 p.m. UTC | #10
On 1/5/21 1:54 AM, Bjorn Andersson wrote:
> On Mon 04 Jan 18:47 CST 2021, Bjorn Andersson wrote:

> 

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

>>

>> This patch doesn't "clean up" the class, as described in $subject. It

>> just removes it.

>>

>>> Suppress the management of the rpmsg class as attribute. It is already

>>> handled in /sys/bus rpmsg as specified in

>>> documentation/ABI/testing/sysfs-bus-rpmsg.

>>

>> Afaict it doesn't relate to /sys/bus/rpmsg, but rather what attributes

>> should be associated with the rpmsg_device (and thereby present in its

>> sysfs directory). But if these attributes are also added by the bus,

>> then why do we have this code? If that's the case this seems like a nice

>> cleanup that we should do outside/before merging the other patches.

>>

>>> This patch prepares the migration of the control device in rpmsg_ctrl.

>>>

>>

>> It would be useful if the commit message described how it prepares for

>> the migration and why.

>>

> 

> Now I see what this patch does, it removes the attributes from the

> character device's struct device, because they are provided by the

> struct rpmsg_device's bus!

> 

> I wish your commit message made this obvious.

> 

> Also, this implies that for a few patches here rpmsg_char is just

> broken - which I don't like.


I will move this patch at the end of the series and clarify commit message

Thanks
Arnaud

> 

> Regards,

> Bjorn

> 

>> Regards,

>> Bjorn

>>

>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>

>>> ---

>>>  drivers/rpmsg/rpmsg_char.c | 48 --------------------------------------

>>>  1 file changed, 48 deletions(-)

>>>

>>> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c

>>> index 4bbbacdbf3bb..732f5caf068a 100644

>>> --- a/drivers/rpmsg/rpmsg_char.c

>>> +++ b/drivers/rpmsg/rpmsg_char.c

>>> @@ -27,7 +27,6 @@

>>>  #define RPMSG_DEV_MAX	(MINORMASK + 1)

>>>  

>>>  static dev_t rpmsg_major;

>>> -static struct class *rpmsg_class;

>>>  

>>>  static DEFINE_IDA(rpmsg_ctrl_ida);

>>>  static DEFINE_IDA(rpmsg_ept_ida);

>>> @@ -291,41 +290,6 @@ static const struct file_operations rpmsg_eptdev_fops = {

>>>  	.compat_ioctl = compat_ptr_ioctl,

>>>  };

>>>  

>>> -static ssize_t name_show(struct device *dev, struct device_attribute *attr,

>>> -			 char *buf)

>>> -{

>>> -	struct rpmsg_eptdev *eptdev = dev_get_drvdata(dev);

>>> -

>>> -	return sprintf(buf, "%s\n", eptdev->chinfo.name);

>>> -}

>>> -static DEVICE_ATTR_RO(name);

>>> -

>>> -static ssize_t src_show(struct device *dev, struct device_attribute *attr,

>>> -			 char *buf)

>>> -{

>>> -	struct rpmsg_eptdev *eptdev = dev_get_drvdata(dev);

>>> -

>>> -	return sprintf(buf, "%d\n", eptdev->chinfo.src);

>>> -}

>>> -static DEVICE_ATTR_RO(src);

>>> -

>>> -static ssize_t dst_show(struct device *dev, struct device_attribute *attr,

>>> -			 char *buf)

>>> -{

>>> -	struct rpmsg_eptdev *eptdev = dev_get_drvdata(dev);

>>> -

>>> -	return sprintf(buf, "%d\n", eptdev->chinfo.dst);

>>> -}

>>> -static DEVICE_ATTR_RO(dst);

>>> -

>>> -static struct attribute *rpmsg_eptdev_attrs[] = {

>>> -	&dev_attr_name.attr,

>>> -	&dev_attr_src.attr,

>>> -	&dev_attr_dst.attr,

>>> -	NULL

>>> -};

>>> -ATTRIBUTE_GROUPS(rpmsg_eptdev);

>>> -

>>>  static void rpmsg_eptdev_release_device(struct device *dev)

>>>  {

>>>  	struct rpmsg_eptdev *eptdev = dev_to_eptdev(dev);

>>> @@ -358,9 +322,7 @@ static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,

>>>  	init_waitqueue_head(&eptdev->readq);

>>>  

>>>  	device_initialize(dev);

>>> -	dev->class = rpmsg_class;

>>>  	dev->parent = &ctrldev->dev;

>>> -	dev->groups = rpmsg_eptdev_groups;

>>>  	dev_set_drvdata(dev, eptdev);

>>>  

>>>  	cdev_init(&eptdev->cdev, &rpmsg_eptdev_fops);

>>> @@ -477,7 +439,6 @@ static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)

>>>  	dev = &ctrldev->dev;

>>>  	device_initialize(dev);

>>>  	dev->parent = &rpdev->dev;

>>> -	dev->class = rpmsg_class;

>>>  

>>>  	cdev_init(&ctrldev->cdev, &rpmsg_ctrldev_fops);

>>>  	ctrldev->cdev.owner = THIS_MODULE;

>>> @@ -553,17 +514,9 @@ static int rpmsg_char_init(void)

>>>  		return ret;

>>>  	}

>>>  

>>> -	rpmsg_class = class_create(THIS_MODULE, "rpmsg");

>>> -	if (IS_ERR(rpmsg_class)) {

>>> -		pr_err("failed to create rpmsg class\n");

>>> -		unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);

>>> -		return PTR_ERR(rpmsg_class);

>>> -	}

>>> -

>>>  	ret = register_rpmsg_driver(&rpmsg_chrdev_driver);

>>>  	if (ret < 0) {

>>>  		pr_err("rpmsgchr: failed to register rpmsg driver\n");

>>> -		class_destroy(rpmsg_class);

>>>  		unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);

>>>  	}

>>>  

>>> @@ -574,7 +527,6 @@ postcore_initcall(rpmsg_char_init);

>>>  static void rpmsg_chrdev_exit(void)

>>>  {

>>>  	unregister_rpmsg_driver(&rpmsg_chrdev_driver);

>>> -	class_destroy(rpmsg_class);

>>>  	unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);

>>>  }

>>>  module_exit(rpmsg_chrdev_exit);

>>> -- 

>>> 2.17.1

>>>
Arnaud POULIQUEN Jan. 5, 2021, 5:05 p.m. UTC | #11
On 1/5/21 1:59 AM, Bjorn Andersson wrote:
> On Tue 22 Dec 04:57 CST 2020, Arnaud Pouliquen wrote:

> 

>> Only one endpoint can be created per device, prevent from multi open.

>>

> 

> Having multiple invocations of rpmsg_create_ept() with the same chinfo

> sounds like a bad idea. I think in the SMD and GLINK case the underlying

> transport would complain that the related chinfo is already "busy", but

> this seems like an appropriate fix regardless.

> 

> Please add a proper Fixes: tag and send this outside of this patch

> series.


I will send it in a separate patch.

Regards,
Arnaud

> 

> Thanks,

> Bjorn

> 

>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>

>> ---

>>  drivers/rpmsg/rpmsg_char.c | 3 +++

>>  1 file changed, 3 insertions(+)

>>

>> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c

>> index 66e01b979e72..4b0674a2e3e9 100644

>> --- a/drivers/rpmsg/rpmsg_char.c

>> +++ b/drivers/rpmsg/rpmsg_char.c

>> @@ -122,6 +122,9 @@ static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)

>>  	struct rpmsg_device *rpdev = eptdev->rpdev;

>>  	struct device *dev = &eptdev->dev;

>>  

>> +	if (eptdev->ept)

>> +		return -EBUSY;

>> +

>>  	get_device(dev);

>>  

>>  	ept = rpmsg_create_ept(rpdev, rpmsg_ept_cb, eptdev, eptdev->chinfo);

>> -- 

>> 2.17.1

>>
Arnaud POULIQUEN Jan. 5, 2021, 5:08 p.m. UTC | #12
On 1/5/21 2:03 AM, Bjorn Andersson wrote:
> On Tue 22 Dec 04:57 CST 2020, Arnaud Pouliquen wrote:

> 

>> The name service announcement is not sent if no endpoint is created by

>> default. If the destination address is not precised by the

>> application when creating the device (thanks to the RPMsg CTRL interface),

>> it is not possible to have a valid RPMsg channel.

>>

> 

> In the Qualcomm transports, the chinfo.name is used to identify the

> channel, so there it's valid to create a endpoint without dst.


So to be move in rpmsg virtio...either reporting an error or generating a NS
announcement.

Thanks,
Arnaud

> 

> Regards,

> Bjorn

> 

>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>

>> ---

>>  drivers/rpmsg/rpmsg_char.c | 10 ++++++++++

>>  1 file changed, 10 insertions(+)

>>

>> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c

>> index 4b0674a2e3e9..8b1928594d10 100644

>> --- a/drivers/rpmsg/rpmsg_char.c

>> +++ b/drivers/rpmsg/rpmsg_char.c

>> @@ -305,6 +305,16 @@ static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)

>>  	struct device *dev;

>>  	int ret;

>>  

>> +	/* There is not ept created by default. As consequence there is not NS

>> +	 * announcement and the destination address has to be set.

>> +	 * this limitation could be solved in future by adding a helper in

>> +	 * rpmsg_ns.

>> +	 */

>> +	if (rpdev->dst == RPMSG_ADDR_ANY) {

>> +		dev_err(dev, "destination address invalid (%d)\n", rpdev->dst);

>> +		return -EINVAL;

>> +	}

>> +

>>  	eptdev = kzalloc(sizeof(*eptdev), GFP_KERNEL);

>>  	if (!eptdev)

>>  		return -ENOMEM;

>> -- 

>> 2.17.1

>>
Mathieu Poirier Jan. 13, 2021, 8:31 p.m. UTC | #13
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

>
Arnaud POULIQUEN Jan. 14, 2021, 9:05 a.m. UTC | #14
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

>>