[PATCH v5 00/16] introduce a generic IOCTL interface for RPMsg channels management

[PATCH v5 00/16] introduce a generic IOCTL interface for RPMsg channels management

[Arnaud Pouliquen]

This series restructures the RPMsg char driver to decorrelate the control part and to
create a generic RPMsg ioctl interface compatible with other RPMsg services.

The V4 and V5 fix compilation issues reported by the kernel test robot <lkp@intel.com>
and analyzed by Dan Carpenter <dan.carpenter@oracle.com>.

The V3 is based on the guideline proposed by Mathieu Poirier to keep as much as possible
the legacy implementation of the rpmsg_char used by the GLINK and SMD platforms.

Objectives of the series:
- Allow to create a service from Linux user application:
  - with a specific name
  - with or without name service announcement.
- Allow to probe the same service by receiving either a NS announcement from the remote firmware
  or a Linux user application request.
- Use these services independently of the RPMsg transport implementation (e.g be able to use
  RPMSg char with the RPMsg virtio bus).

Steps in the series:
  - Extract the control part of the char dev and create the rpmsg_ctrl.c file (patches 1 to 6)
  - Enable the use of the chardev with the virtio backend (patches 7 to 11)
  - Introduce the RPMSG_CREATE_DEV_IOCTL IOCTL to instantiate RPMsg devices (patch 12)
    The application can then create or release a channel by specifying:
       - the name service of the device to instantiate.   
       - the source address.
       - the destination address.
  - Instantiate the /dev/rpmsg interface on remote NS announcement (patches 13 to 16)

In this revision, I do not divide the series into several parts in order to show a complete
picture of the proposed evolution. To simplify the review, if requested, I can send it in
several steps listed above.

Known current Limitations:
- Tested only with virtio RPMsg bus. The glink and smd drivers adaptations have not been tested
  (not able to test it).
- For the virtio backend: No NS announcement is sent to the remote processor if the source
  address is set to RPMSG_ADDR_ANY.
- For the virtio backend: the existing RPMSG_CREATE_EPT_IOCTL is working but the endpoints are
  not attached to an exiting channel.
- to limit patches the pending RPMSG_DESTROY_DEV_IOCTL has not ben implemented. This will be
  proposed in a second step.

This series can be applied on git/andersson/remoteproc.git for-next branch (d9ff3a5789cb).

This series can be tested using rpmsgexport, rpmsgcreatedev and ping tools available here:
https://github.com/arnopo/rpmsgexport.git

Reference to the V4 discussion thread: https://lkml.org/lkml/2021/2/17/384

Arnaud Pouliquen (16):
  rpmsg: char: rename rpmsg_char_init to rpmsg_chrdev_init
  rpmsg: move RPMSG_ADDR_ANY in user API
  rpmsg: add short description of the IOCTL defined in UAPI.
  rpmsg: char: export eptdev create an destroy functions
  rpmsg: char: dissociate the control device from the rpmsg class
  rpmsg: move the rpmsg control device from rpmsg_char to rpmsg_ctrl
  rpmsg: update rpmsg_chrdev_register_device function
  rpmsg: glink: add sendto and trysendto ops
  rpmsg: smd: add sendto and trysendto ops
  rpmsg: char: use sendto to specify the message destination address
  rpmsg: virtio: register the rpmsg_ctrl device
  rpmsg: ctrl: introduce RPMSG_CREATE_DEV_IOCTL
  rpmsg: char: introduce __rpmsg_chrdev_create_eptdev function
  rpmsg: char: introduce a RPMsg driver for the RPMsg char device
  rpmsg: char: no dynamic endpoint management for the default one
  rpmsg: char: return an error if device already open

 drivers/rpmsg/Kconfig             |   9 ++
 drivers/rpmsg/Makefile            |   1 +
 drivers/rpmsg/qcom_glink_native.c |  18 ++-
 drivers/rpmsg/qcom_smd.c          |  18 ++-
 drivers/rpmsg/rpmsg_char.c        | 237 +++++++++++-------------------
 drivers/rpmsg/rpmsg_char.h        |  51 +++++++
 drivers/rpmsg/rpmsg_ctrl.c        | 229 +++++++++++++++++++++++++++++
 drivers/rpmsg/rpmsg_internal.h    |  10 +-
 drivers/rpmsg/virtio_rpmsg_bus.c  |  57 ++++++-
 include/linux/rpmsg.h             |   3 +-
 include/uapi/linux/rpmsg.h        |  18 ++-
 11 files changed, 485 insertions(+), 166 deletions(-)
 create mode 100644 drivers/rpmsg/rpmsg_char.h
 create mode 100644 drivers/rpmsg/rpmsg_ctrl.c

-- 
2.17.1

[PATCH v5 01/16] rpmsg: char: rename rpmsg_char_init to rpmsg_chrdev_init

[Arnaud Pouliquen]

To be coherent with the other functions which are prefixed by
rpmsg_chrdev, rename the rpmsg_char_init function.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
 drivers/rpmsg/rpmsg_char.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index 4bbbacdbf3bb..9e33b53bbf56 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -543,7 +543,7 @@ static struct rpmsg_driver rpmsg_chrdev_driver = {
 	},
 };
 
-static int rpmsg_char_init(void)
+static int rpmsg_chrdev_init(void)
 {
 	int ret;
 
@@ -569,7 +569,7 @@ static int rpmsg_char_init(void)
 
 	return ret;
 }
-postcore_initcall(rpmsg_char_init);
+postcore_initcall(rpmsg_chrdev_init);
 
 static void rpmsg_chrdev_exit(void)
 {
-- 
2.17.1

[PATCH v5 02/16] rpmsg: move RPMSG_ADDR_ANY in user API

[Arnaud Pouliquen]

As the RPMSG_ADDR_ANY is a valid src or dst address that can be set by
user applications,  migrate its definition in user API.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
 include/linux/rpmsg.h      | 3 +--
 include/uapi/linux/rpmsg.h | 2 ++
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
index a5db828b2420..d97dcd049f18 100644
--- a/include/linux/rpmsg.h
+++ b/include/linux/rpmsg.h
@@ -18,8 +18,7 @@
 #include <linux/mutex.h>
 #include <linux/poll.h>
 #include <linux/rpmsg/byteorder.h>
-
-#define RPMSG_ADDR_ANY		0xFFFFFFFF
+#include <uapi/linux/rpmsg.h>
 
 struct rpmsg_device;
 struct rpmsg_endpoint;
diff --git a/include/uapi/linux/rpmsg.h b/include/uapi/linux/rpmsg.h
index e14c6dab4223..5e00748da319 100644
--- a/include/uapi/linux/rpmsg.h
+++ b/include/uapi/linux/rpmsg.h
@@ -9,6 +9,8 @@
 #include <linux/ioctl.h>
 #include <linux/types.h>
 
+#define RPMSG_ADDR_ANY		0xFFFFFFFF
+
 /**
  * struct rpmsg_endpoint_info - endpoint info representation
  * @name: name of service
-- 
2.17.1

[PATCH v5 03/16] rpmsg: add short description of the IOCTL defined in UAPI.

[Arnaud Pouliquen]

Add a description of the IOCTL and provide information on the default
value of the source and destination addresses.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
 include/uapi/linux/rpmsg.h | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/rpmsg.h b/include/uapi/linux/rpmsg.h
index 5e00748da319..f5ca8740f3fb 100644
--- a/include/uapi/linux/rpmsg.h
+++ b/include/uapi/linux/rpmsg.h
@@ -14,8 +14,8 @@
 /**
  * struct rpmsg_endpoint_info - endpoint info representation
  * @name: name of service
- * @src: local address
- * @dst: destination address
+ * @src: local address. To set to RPMSG_ADDR_ANY if not used.
+ * @dst: destination address. To set to RPMSG_ADDR_ANY if not used.
  */
 struct rpmsg_endpoint_info {
 	char name[32];
@@ -23,7 +23,14 @@ struct rpmsg_endpoint_info {
 	__u32 dst;
 };
 
+/**
+ * Instantiate a new rmpsg char device endpoint.
+ */
 #define RPMSG_CREATE_EPT_IOCTL	_IOW(0xb5, 0x1, struct rpmsg_endpoint_info)
+
+/**
+ * Destroy a rpmsg char device endpoint created by the RPMSG_CREATE_EPT_IOCTL.
+ */
 #define RPMSG_DESTROY_EPT_IOCTL	_IO(0xb5, 0x2)
 
 #endif
-- 
2.17.1

[PATCH v5 04/16] rpmsg: char: export eptdev create an destroy functions

[Arnaud Pouliquen]

To prepare the split code related to the control and the endpoint
devices in separate files:
- suppress the dependency with the rpmsg_ctrldev struct,
- rename and export the functions in rpmsg_char.h.

Suggested-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
 drivers/rpmsg/rpmsg_char.c | 22 ++++++++++------
 drivers/rpmsg/rpmsg_char.h | 51 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+), 7 deletions(-)
 create mode 100644 drivers/rpmsg/rpmsg_char.h

diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index 9e33b53bbf56..78a6d19fdf82 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
+ * Copyright (C) 2021, STMicroelectronics
  * Copyright (c) 2016, Linaro Ltd.
  * Copyright (c) 2012, Michal Simek <monstr@monstr.eu>
  * Copyright (c) 2012, PetaLogix
@@ -22,6 +23,7 @@
 #include <linux/uaccess.h>
 #include <uapi/linux/rpmsg.h>
 
+#include "rpmsg_char.h"
 #include "rpmsg_internal.h"
 
 #define RPMSG_DEV_MAX	(MINORMASK + 1)
@@ -78,7 +80,7 @@ struct rpmsg_eptdev {
 	wait_queue_head_t readq;
 };
 
-static int rpmsg_eptdev_destroy(struct device *dev, void *data)
+static int rpmsg_eptdev_destroy(struct device *dev)
 {
 	struct rpmsg_eptdev *eptdev = dev_to_eptdev(dev);
 
@@ -277,7 +279,7 @@ static long rpmsg_eptdev_ioctl(struct file *fp, unsigned int cmd,
 	if (cmd != RPMSG_DESTROY_EPT_IOCTL)
 		return -EINVAL;
 
-	return rpmsg_eptdev_destroy(&eptdev->dev, NULL);
+	return rpmsg_eptdev_destroy(&eptdev->dev);
 }
 
 static const struct file_operations rpmsg_eptdev_fops = {
@@ -336,10 +338,15 @@ static void rpmsg_eptdev_release_device(struct device *dev)
 	kfree(eptdev);
 }
 
-static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
+int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data)
+{
+	return rpmsg_eptdev_destroy(dev);
+}
+EXPORT_SYMBOL(rpmsg_chrdev_eptdev_destroy);
+
+int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent,
 			       struct rpmsg_channel_info chinfo)
 {
-	struct rpmsg_device *rpdev = ctrldev->rpdev;
 	struct rpmsg_eptdev *eptdev;
 	struct device *dev;
 	int ret;
@@ -359,7 +366,7 @@ static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
 
 	device_initialize(dev);
 	dev->class = rpmsg_class;
-	dev->parent = &ctrldev->dev;
+	dev->parent = parent;
 	dev->groups = rpmsg_eptdev_groups;
 	dev_set_drvdata(dev, eptdev);
 
@@ -402,6 +409,7 @@ static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
 
 	return ret;
 }
+EXPORT_SYMBOL(rpmsg_chrdev_create_eptdev);
 
 static int rpmsg_ctrldev_open(struct inode *inode, struct file *filp)
 {
@@ -441,7 +449,7 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
 	chinfo.src = eptinfo.src;
 	chinfo.dst = eptinfo.dst;
 
-	return rpmsg_eptdev_create(ctrldev, chinfo);
+	return rpmsg_chrdev_create_eptdev(ctrldev->rpdev, &ctrldev->dev, chinfo);
 };
 
 static const struct file_operations rpmsg_ctrldev_fops = {
@@ -527,7 +535,7 @@ static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev)
 	int ret;
 
 	/* Destroy all endpoints */
-	ret = device_for_each_child(&ctrldev->dev, NULL, rpmsg_eptdev_destroy);
+	ret = device_for_each_child(&ctrldev->dev, NULL, rpmsg_chrdev_eptdev_destroy);
 	if (ret)
 		dev_warn(&rpdev->dev, "failed to nuke endpoints: %d\n", ret);
 
diff --git a/drivers/rpmsg/rpmsg_char.h b/drivers/rpmsg/rpmsg_char.h
new file mode 100644
index 000000000000..0feb3ea9445c
--- /dev/null
+++ b/drivers/rpmsg/rpmsg_char.h
@@ -0,0 +1,51 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/*
+ * Copyright (C) STMicroelectronics 2021.
+ */
+
+#ifndef __RPMSG_CHRDEV_H__
+#define __RPMSG_CHRDEV_H__
+
+#if IS_ENABLED(CONFIG_RPMSG_CHAR)
+/**
+ * rpmsg_chrdev_create_eptdev() - register char device based on an endpoint
+ * @rpdev:  prepared rpdev to be used for creating endpoints
+ * @parent: parent device
+ * @chinfo: assiated endpoint channel information.
+ *
+ * This function create a new rpmsg char endpoint device to instantiate a new
+ * endpoint based on chinfo information.
+ */
+int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent,
+			       struct rpmsg_channel_info chinfo);
+
+/**
+ * rpmsg_chrdev_eptdev_destroy() - destroy created char device
+ * @data: parent device
+ * @chinfo: assiated endpoint channel information.
+ *
+ * This function create a new rpmsg char endpoint device to instantiate a new
+ * endpoint based on chinfo information.
+ */
+int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data);
+
+#else  /*IS_ENABLED(CONFIG_RPMSG_CHAR) */
+
+static inline int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev,
+					     struct device *parent,
+					     struct rpmsg_channel_info chinfo)
+{
+	return -EINVAL;
+}
+
+static inline int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data)
+{
+	/* This shouldn't be possible */
+	WARN_ON(1);
+
+	return 0;
+}
+
+#endif /*IS_ENABLED(CONFIG_RPMSG_CHAR) */
+
+#endif /*__RPMSG_CHRDEV_H__ */
-- 
2.17.1

Re: [PATCH v5 04/16] rpmsg: char: export eptdev create an destroy functions

[Mathieu Poirier]

Good morning,

I have started to review this set - comments will be staggered over several
days.

On Fri, Feb 19, 2021 at 12:14:49PM +0100, Arnaud Pouliquen wrote:
> To prepare the split code related to the control and the endpoint
> devices in separate files:
> - suppress the dependency with the rpmsg_ctrldev struct,
> - rename and export the functions in rpmsg_char.h.
> 
> Suggested-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
>  drivers/rpmsg/rpmsg_char.c | 22 ++++++++++------
>  drivers/rpmsg/rpmsg_char.h | 51 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 66 insertions(+), 7 deletions(-)
>  create mode 100644 drivers/rpmsg/rpmsg_char.h
> 
> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> index 9e33b53bbf56..78a6d19fdf82 100644
> --- a/drivers/rpmsg/rpmsg_char.c
> +++ b/drivers/rpmsg/rpmsg_char.c
> @@ -1,5 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /*
> + * Copyright (C) 2021, STMicroelectronics
>   * Copyright (c) 2016, Linaro Ltd.
>   * Copyright (c) 2012, Michal Simek <monstr@monstr.eu>
>   * Copyright (c) 2012, PetaLogix
> @@ -22,6 +23,7 @@
>  #include <linux/uaccess.h>
>  #include <uapi/linux/rpmsg.h>
>  
> +#include "rpmsg_char.h"
>  #include "rpmsg_internal.h"
>  
>  #define RPMSG_DEV_MAX	(MINORMASK + 1)
> @@ -78,7 +80,7 @@ struct rpmsg_eptdev {
>  	wait_queue_head_t readq;
>  };
>  
> -static int rpmsg_eptdev_destroy(struct device *dev, void *data)
> +static int rpmsg_eptdev_destroy(struct device *dev)
>  {
>  	struct rpmsg_eptdev *eptdev = dev_to_eptdev(dev);
>  
> @@ -277,7 +279,7 @@ static long rpmsg_eptdev_ioctl(struct file *fp, unsigned int cmd,
>  	if (cmd != RPMSG_DESTROY_EPT_IOCTL)
>  		return -EINVAL;
>  
> -	return rpmsg_eptdev_destroy(&eptdev->dev, NULL);
> +	return rpmsg_eptdev_destroy(&eptdev->dev);
>  }
>  
>  static const struct file_operations rpmsg_eptdev_fops = {
> @@ -336,10 +338,15 @@ static void rpmsg_eptdev_release_device(struct device *dev)
>  	kfree(eptdev);
>  }
>  
> -static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
> +int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data)
> +{
> +	return rpmsg_eptdev_destroy(dev);
> +}
> +EXPORT_SYMBOL(rpmsg_chrdev_eptdev_destroy);

Below we have rpmsg_chrdev_create_eptdev() so it would make sense to have
rpmsg_chrdev_destroy_ept().

I would also rename rpmsg_eptdev_destroy() to rpmsg_chrdev_destroy_ept() and
export that symbol rather than introducing a function that only calls another
one.  You did exactly that for rpmsg_chrdev_create_eptdev().

> +
> +int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent,
>  			       struct rpmsg_channel_info chinfo)
>  {
> -	struct rpmsg_device *rpdev = ctrldev->rpdev;
>  	struct rpmsg_eptdev *eptdev;
>  	struct device *dev;
>  	int ret;
> @@ -359,7 +366,7 @@ static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
>  
>  	device_initialize(dev);
>  	dev->class = rpmsg_class;
> -	dev->parent = &ctrldev->dev;
> +	dev->parent = parent;
>  	dev->groups = rpmsg_eptdev_groups;
>  	dev_set_drvdata(dev, eptdev);
>  
> @@ -402,6 +409,7 @@ static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
>  
>  	return ret;
>  }
> +EXPORT_SYMBOL(rpmsg_chrdev_create_eptdev);
>  
>  static int rpmsg_ctrldev_open(struct inode *inode, struct file *filp)
>  {
> @@ -441,7 +449,7 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
>  	chinfo.src = eptinfo.src;
>  	chinfo.dst = eptinfo.dst;
>  
> -	return rpmsg_eptdev_create(ctrldev, chinfo);
> +	return rpmsg_chrdev_create_eptdev(ctrldev->rpdev, &ctrldev->dev, chinfo);

Not sure why we have to change the signature of rpmsg_eptdev_create() but I may
find an answer to that question later on in the patchset.

>  };
>  
>  static const struct file_operations rpmsg_ctrldev_fops = {
> @@ -527,7 +535,7 @@ static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev)
>  	int ret;
>  
>  	/* Destroy all endpoints */
> -	ret = device_for_each_child(&ctrldev->dev, NULL, rpmsg_eptdev_destroy);
> +	ret = device_for_each_child(&ctrldev->dev, NULL, rpmsg_chrdev_eptdev_destroy);
>  	if (ret)
>  		dev_warn(&rpdev->dev, "failed to nuke endpoints: %d\n", ret);
>  
> diff --git a/drivers/rpmsg/rpmsg_char.h b/drivers/rpmsg/rpmsg_char.h
> new file mode 100644
> index 000000000000..0feb3ea9445c
> --- /dev/null
> +++ b/drivers/rpmsg/rpmsg_char.h
> @@ -0,0 +1,51 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * Copyright (C) STMicroelectronics 2021.
> + */
> +
> +#ifndef __RPMSG_CHRDEV_H__
> +#define __RPMSG_CHRDEV_H__
> +
> +#if IS_ENABLED(CONFIG_RPMSG_CHAR)
> +/**
> + * rpmsg_chrdev_create_eptdev() - register char device based on an endpoint
> + * @rpdev:  prepared rpdev to be used for creating endpoints
> + * @parent: parent device
> + * @chinfo: assiated endpoint channel information.
> + *
> + * This function create a new rpmsg char endpoint device to instantiate a new
> + * endpoint based on chinfo information.
> + */
> +int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent,
> +			       struct rpmsg_channel_info chinfo);
> +
> +/**
> + * rpmsg_chrdev_eptdev_destroy() - destroy created char device
> + * @data: parent device
> + * @chinfo: assiated endpoint channel information.
> + *
> + * This function create a new rpmsg char endpoint device to instantiate a new
> + * endpoint based on chinfo information.
> + */
> +int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data);
> +
> +#else  /*IS_ENABLED(CONFIG_RPMSG_CHAR) */
> +
> +static inline int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev,
> +					     struct device *parent,
> +					     struct rpmsg_channel_info chinfo)
> +{
> +	return -EINVAL;
> +}
> +
> +static inline int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data)
> +{
> +	/* This shouldn't be possible */
> +	WARN_ON(1);
> +
> +	return 0;
> +}
> +
> +#endif /*IS_ENABLED(CONFIG_RPMSG_CHAR) */
> +
> +#endif /*__RPMSG_CHRDEV_H__ */
> -- 
> 2.17.1
> 

Re: [PATCH v5 04/16] rpmsg: char: export eptdev create an destroy functions

[Arnaud POULIQUEN]

Hi Mathieu,

On 3/2/21 6:57 PM, Mathieu Poirier wrote:
> Good morning,
> 
> I have started to review this set - comments will be staggered over several
> days.
> 
> On Fri, Feb 19, 2021 at 12:14:49PM +0100, Arnaud Pouliquen wrote:
>> To prepare the split code related to the control and the endpoint
>> devices in separate files:
>> - suppress the dependency with the rpmsg_ctrldev struct,
>> - rename and export the functions in rpmsg_char.h.
>>
>> Suggested-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>> ---
>>  drivers/rpmsg/rpmsg_char.c | 22 ++++++++++------
>>  drivers/rpmsg/rpmsg_char.h | 51 ++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 66 insertions(+), 7 deletions(-)
>>  create mode 100644 drivers/rpmsg/rpmsg_char.h
>>
>> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
>> index 9e33b53bbf56..78a6d19fdf82 100644
>> --- a/drivers/rpmsg/rpmsg_char.c
>> +++ b/drivers/rpmsg/rpmsg_char.c
>> @@ -1,5 +1,6 @@
>>  // SPDX-License-Identifier: GPL-2.0
>>  /*
>> + * Copyright (C) 2021, STMicroelectronics
>>   * Copyright (c) 2016, Linaro Ltd.
>>   * Copyright (c) 2012, Michal Simek <monstr@monstr.eu>
>>   * Copyright (c) 2012, PetaLogix
>> @@ -22,6 +23,7 @@
>>  #include <linux/uaccess.h>
>>  #include <uapi/linux/rpmsg.h>
>>  
>> +#include "rpmsg_char.h"
>>  #include "rpmsg_internal.h"
>>  
>>  #define RPMSG_DEV_MAX	(MINORMASK + 1)
>> @@ -78,7 +80,7 @@ struct rpmsg_eptdev {
>>  	wait_queue_head_t readq;
>>  };
>>  
>> -static int rpmsg_eptdev_destroy(struct device *dev, void *data)
>> +static int rpmsg_eptdev_destroy(struct device *dev)
>>  {
>>  	struct rpmsg_eptdev *eptdev = dev_to_eptdev(dev);
>>  
>> @@ -277,7 +279,7 @@ static long rpmsg_eptdev_ioctl(struct file *fp, unsigned int cmd,
>>  	if (cmd != RPMSG_DESTROY_EPT_IOCTL)
>>  		return -EINVAL;
>>  
>> -	return rpmsg_eptdev_destroy(&eptdev->dev, NULL);
>> +	return rpmsg_eptdev_destroy(&eptdev->dev);
>>  }
>>  
>>  static const struct file_operations rpmsg_eptdev_fops = {
>> @@ -336,10 +338,15 @@ static void rpmsg_eptdev_release_device(struct device *dev)
>>  	kfree(eptdev);
>>  }
>>  
>> -static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
>> +int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data)
>> +{
>> +	return rpmsg_eptdev_destroy(dev);
>> +}
>> +EXPORT_SYMBOL(rpmsg_chrdev_eptdev_destroy);
> 
> Below we have rpmsg_chrdev_create_eptdev() so it would make sense to have
> rpmsg_chrdev_destroy_ept().
> 
> I would also rename rpmsg_eptdev_destroy() to rpmsg_chrdev_destroy_ept() and
> export that symbol rather than introducing a function that only calls another
> one.  You did exactly that for rpmsg_chrdev_create_eptdev().

This function is used in rpmsg_ctrl to remove eptdev devices.
As device_for_each_child request a specific function prototype I do not only
export but change function prototype.

I can squash all in one function but that means that the "data" parameter is
just always unused.

> 
>> +
>> +int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent,
>>  			       struct rpmsg_channel_info chinfo)
>>  {
>> -	struct rpmsg_device *rpdev = ctrldev->rpdev;
>>  	struct rpmsg_eptdev *eptdev;
>>  	struct device *dev;
>>  	int ret;
>> @@ -359,7 +366,7 @@ static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
>>  
>>  	device_initialize(dev);
>>  	dev->class = rpmsg_class;
>> -	dev->parent = &ctrldev->dev;
>> +	dev->parent = parent;
>>  	dev->groups = rpmsg_eptdev_groups;
>>  	dev_set_drvdata(dev, eptdev);
>>  
>> @@ -402,6 +409,7 @@ static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
>>  
>>  	return ret;
>>  }
>> +EXPORT_SYMBOL(rpmsg_chrdev_create_eptdev);
>>  
>>  static int rpmsg_ctrldev_open(struct inode *inode, struct file *filp)
>>  {
>> @@ -441,7 +449,7 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
>>  	chinfo.src = eptinfo.src;
>>  	chinfo.dst = eptinfo.dst;
>>  
>> -	return rpmsg_eptdev_create(ctrldev, chinfo);
>> +	return rpmsg_chrdev_create_eptdev(ctrldev->rpdev, &ctrldev->dev, chinfo);
> 
> Not sure why we have to change the signature of rpmsg_eptdev_create() but I may
> find an answer to that question later on in the patchset.

Here I need to break dependency with rpmsg_ctrldev that will move to the
rpmsg_ctrl.c. rpmsg_eptdev_create doesn't need it but just parent devices to be
attached to.

I will add more explicit information about this, in the commit message.

Thanks,
Arnaud

> 
>>  };
>>  
>>  static const struct file_operations rpmsg_ctrldev_fops = {
>> @@ -527,7 +535,7 @@ static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev)
>>  	int ret;
>>  
>>  	/* Destroy all endpoints */
>> -	ret = device_for_each_child(&ctrldev->dev, NULL, rpmsg_eptdev_destroy);
>> +	ret = device_for_each_child(&ctrldev->dev, NULL, rpmsg_chrdev_eptdev_destroy);
>>  	if (ret)
>>  		dev_warn(&rpdev->dev, "failed to nuke endpoints: %d\n", ret);
>>  
>> diff --git a/drivers/rpmsg/rpmsg_char.h b/drivers/rpmsg/rpmsg_char.h
>> new file mode 100644
>> index 000000000000..0feb3ea9445c
>> --- /dev/null
>> +++ b/drivers/rpmsg/rpmsg_char.h
>> @@ -0,0 +1,51 @@
>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>> +/*
>> + * Copyright (C) STMicroelectronics 2021.
>> + */
>> +
>> +#ifndef __RPMSG_CHRDEV_H__
>> +#define __RPMSG_CHRDEV_H__
>> +
>> +#if IS_ENABLED(CONFIG_RPMSG_CHAR)
>> +/**
>> + * rpmsg_chrdev_create_eptdev() - register char device based on an endpoint
>> + * @rpdev:  prepared rpdev to be used for creating endpoints
>> + * @parent: parent device
>> + * @chinfo: assiated endpoint channel information.
>> + *
>> + * This function create a new rpmsg char endpoint device to instantiate a new
>> + * endpoint based on chinfo information.
>> + */
>> +int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent,
>> +			       struct rpmsg_channel_info chinfo);
>> +
>> +/**
>> + * rpmsg_chrdev_eptdev_destroy() - destroy created char device
>> + * @data: parent device
>> + * @chinfo: assiated endpoint channel information.
>> + *
>> + * This function create a new rpmsg char endpoint device to instantiate a new
>> + * endpoint based on chinfo information.
>> + */
>> +int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data);
>> +
>> +#else  /*IS_ENABLED(CONFIG_RPMSG_CHAR) */
>> +
>> +static inline int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev,
>> +					     struct device *parent,
>> +					     struct rpmsg_channel_info chinfo)
>> +{
>> +	return -EINVAL;
>> +}
>> +
>> +static inline int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data)
>> +{
>> +	/* This shouldn't be possible */
>> +	WARN_ON(1);
>> +
>> +	return 0;
>> +}
>> +
>> +#endif /*IS_ENABLED(CONFIG_RPMSG_CHAR) */
>> +
>> +#endif /*__RPMSG_CHRDEV_H__ */
>> -- 
>> 2.17.1
>>

Re: [PATCH v5 04/16] rpmsg: char: export eptdev create an destroy functions

[Mathieu Poirier]

On Wed, Mar 03, 2021 at 02:22:36PM +0100, Arnaud POULIQUEN wrote:
> Hi Mathieu,
> 
> On 3/2/21 6:57 PM, Mathieu Poirier wrote:
> > Good morning,
> > 
> > I have started to review this set - comments will be staggered over several
> > days.
> > 
> > On Fri, Feb 19, 2021 at 12:14:49PM +0100, Arnaud Pouliquen wrote:
> >> To prepare the split code related to the control and the endpoint
> >> devices in separate files:
> >> - suppress the dependency with the rpmsg_ctrldev struct,
> >> - rename and export the functions in rpmsg_char.h.
> >>
> >> Suggested-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> >> ---
> >>  drivers/rpmsg/rpmsg_char.c | 22 ++++++++++------
> >>  drivers/rpmsg/rpmsg_char.h | 51 ++++++++++++++++++++++++++++++++++++++
> >>  2 files changed, 66 insertions(+), 7 deletions(-)
> >>  create mode 100644 drivers/rpmsg/rpmsg_char.h
> >>
> >> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> >> index 9e33b53bbf56..78a6d19fdf82 100644
> >> --- a/drivers/rpmsg/rpmsg_char.c
> >> +++ b/drivers/rpmsg/rpmsg_char.c
> >> @@ -1,5 +1,6 @@
> >>  // SPDX-License-Identifier: GPL-2.0
> >>  /*
> >> + * Copyright (C) 2021, STMicroelectronics
> >>   * Copyright (c) 2016, Linaro Ltd.
> >>   * Copyright (c) 2012, Michal Simek <monstr@monstr.eu>
> >>   * Copyright (c) 2012, PetaLogix
> >> @@ -22,6 +23,7 @@
> >>  #include <linux/uaccess.h>
> >>  #include <uapi/linux/rpmsg.h>
> >>  
> >> +#include "rpmsg_char.h"
> >>  #include "rpmsg_internal.h"
> >>  
> >>  #define RPMSG_DEV_MAX	(MINORMASK + 1)
> >> @@ -78,7 +80,7 @@ struct rpmsg_eptdev {
> >>  	wait_queue_head_t readq;
> >>  };
> >>  
> >> -static int rpmsg_eptdev_destroy(struct device *dev, void *data)
> >> +static int rpmsg_eptdev_destroy(struct device *dev)
> >>  {
> >>  	struct rpmsg_eptdev *eptdev = dev_to_eptdev(dev);
> >>  
> >> @@ -277,7 +279,7 @@ static long rpmsg_eptdev_ioctl(struct file *fp, unsigned int cmd,
> >>  	if (cmd != RPMSG_DESTROY_EPT_IOCTL)
> >>  		return -EINVAL;
> >>  
> >> -	return rpmsg_eptdev_destroy(&eptdev->dev, NULL);
> >> +	return rpmsg_eptdev_destroy(&eptdev->dev);
> >>  }
> >>  
> >>  static const struct file_operations rpmsg_eptdev_fops = {
> >> @@ -336,10 +338,15 @@ static void rpmsg_eptdev_release_device(struct device *dev)
> >>  	kfree(eptdev);
> >>  }
> >>  
> >> -static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
> >> +int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data)
> >> +{
> >> +	return rpmsg_eptdev_destroy(dev);
> >> +}
> >> +EXPORT_SYMBOL(rpmsg_chrdev_eptdev_destroy);
> > 
> > Below we have rpmsg_chrdev_create_eptdev() so it would make sense to have
> > rpmsg_chrdev_destroy_ept().
> > 
> > I would also rename rpmsg_eptdev_destroy() to rpmsg_chrdev_destroy_ept() and
> > export that symbol rather than introducing a function that only calls another
> > one.  You did exactly that for rpmsg_chrdev_create_eptdev().
> 
> This function is used in rpmsg_ctrl to remove eptdev devices.
> As device_for_each_child request a specific function prototype I do not only
> export but change function prototype.
> 
> I can squash all in one function but that means that the "data" parameter is
> just always unused.

The @data parameter in rpmsg_eptdev_destroy() is currently unused so nothing
changes.

> 
> > 
> >> +
> >> +int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent,
> >>  			       struct rpmsg_channel_info chinfo)
> >>  {
> >> -	struct rpmsg_device *rpdev = ctrldev->rpdev;
> >>  	struct rpmsg_eptdev *eptdev;
> >>  	struct device *dev;
> >>  	int ret;
> >> @@ -359,7 +366,7 @@ static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
> >>  
> >>  	device_initialize(dev);
> >>  	dev->class = rpmsg_class;
> >> -	dev->parent = &ctrldev->dev;
> >> +	dev->parent = parent;
> >>  	dev->groups = rpmsg_eptdev_groups;
> >>  	dev_set_drvdata(dev, eptdev);
> >>  
> >> @@ -402,6 +409,7 @@ static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
> >>  
> >>  	return ret;
> >>  }
> >> +EXPORT_SYMBOL(rpmsg_chrdev_create_eptdev);
> >>  
> >>  static int rpmsg_ctrldev_open(struct inode *inode, struct file *filp)
> >>  {
> >> @@ -441,7 +449,7 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
> >>  	chinfo.src = eptinfo.src;
> >>  	chinfo.dst = eptinfo.dst;
> >>  
> >> -	return rpmsg_eptdev_create(ctrldev, chinfo);
> >> +	return rpmsg_chrdev_create_eptdev(ctrldev->rpdev, &ctrldev->dev, chinfo);
> > 
> > Not sure why we have to change the signature of rpmsg_eptdev_create() but I may
> > find an answer to that question later on in the patchset.
> 
> Here I need to break dependency with rpmsg_ctrldev that will move to the
> rpmsg_ctrl.c. rpmsg_eptdev_create doesn't need it but just parent devices to be
> attached to.

So you don't want to have instances of struct rpmsg_ctrldev in rpmsg_char.c .
Ok, that's a valid point.

> 
> I will add more explicit information about this, in the commit message.
> 
> Thanks,
> Arnaud
> 
> > 
> >>  };
> >>  
> >>  static const struct file_operations rpmsg_ctrldev_fops = {
> >> @@ -527,7 +535,7 @@ static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev)
> >>  	int ret;
> >>  
> >>  	/* Destroy all endpoints */
> >> -	ret = device_for_each_child(&ctrldev->dev, NULL, rpmsg_eptdev_destroy);
> >> +	ret = device_for_each_child(&ctrldev->dev, NULL, rpmsg_chrdev_eptdev_destroy);
> >>  	if (ret)
> >>  		dev_warn(&rpdev->dev, "failed to nuke endpoints: %d\n", ret);
> >>  
> >> diff --git a/drivers/rpmsg/rpmsg_char.h b/drivers/rpmsg/rpmsg_char.h
> >> new file mode 100644
> >> index 000000000000..0feb3ea9445c
> >> --- /dev/null
> >> +++ b/drivers/rpmsg/rpmsg_char.h
> >> @@ -0,0 +1,51 @@
> >> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> >> +/*
> >> + * Copyright (C) STMicroelectronics 2021.
> >> + */
> >> +
> >> +#ifndef __RPMSG_CHRDEV_H__
> >> +#define __RPMSG_CHRDEV_H__
> >> +
> >> +#if IS_ENABLED(CONFIG_RPMSG_CHAR)
> >> +/**
> >> + * rpmsg_chrdev_create_eptdev() - register char device based on an endpoint
> >> + * @rpdev:  prepared rpdev to be used for creating endpoints
> >> + * @parent: parent device
> >> + * @chinfo: assiated endpoint channel information.
> >> + *
> >> + * This function create a new rpmsg char endpoint device to instantiate a new
> >> + * endpoint based on chinfo information.
> >> + */
> >> +int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent,
> >> +			       struct rpmsg_channel_info chinfo);
> >> +
> >> +/**
> >> + * rpmsg_chrdev_eptdev_destroy() - destroy created char device
> >> + * @data: parent device
> >> + * @chinfo: assiated endpoint channel information.
> >> + *
> >> + * This function create a new rpmsg char endpoint device to instantiate a new
> >> + * endpoint based on chinfo information.
> >> + */
> >> +int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data);
> >> +
> >> +#else  /*IS_ENABLED(CONFIG_RPMSG_CHAR) */
> >> +
> >> +static inline int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev,
> >> +					     struct device *parent,
> >> +					     struct rpmsg_channel_info chinfo)
> >> +{
> >> +	return -EINVAL;
> >> +}
> >> +
> >> +static inline int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data)
> >> +{
> >> +	/* This shouldn't be possible */
> >> +	WARN_ON(1);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +#endif /*IS_ENABLED(CONFIG_RPMSG_CHAR) */
> >> +
> >> +#endif /*__RPMSG_CHRDEV_H__ */
> >> -- 
> >> 2.17.1
> >>

[PATCH v5 05/16] rpmsg: char: dissociate the control device from the rpmsg class

[Arnaud Pouliquen]

The RPMsg control device is a RPMsg device, it is already
referenced in the RPMsg bus. There is only an interest to
reference the ept char devices in the rpmsg class.
This patch prepares the code split of the control and end point
devices in two separate files.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
 drivers/rpmsg/rpmsg_char.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index 78a6d19fdf82..23e369a00531 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -485,7 +485,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;
-- 
2.17.1

Re: [PATCH v5 05/16] rpmsg: char: dissociate the control device from the rpmsg class

[Mathieu Poirier]

On Fri, Feb 19, 2021 at 12:14:50PM +0100, Arnaud Pouliquen wrote:
> The RPMsg control device is a RPMsg device, it is already
> referenced in the RPMsg bus. There is only an interest to
> reference the ept char devices in the rpmsg class.
> This patch prepares the code split of the control and end point
> devices in two separate files.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
>  drivers/rpmsg/rpmsg_char.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> index 78a6d19fdf82..23e369a00531 100644
> --- a/drivers/rpmsg/rpmsg_char.c
> +++ b/drivers/rpmsg/rpmsg_char.c
> @@ -485,7 +485,6 @@ static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
>  	dev = &ctrldev->dev;
>  	device_initialize(dev);
>  	dev->parent = &rpdev->dev;
> -	dev->class = rpmsg_class;

This may break user space...  It has been around for so long that even if the
information is redundant we have to keep it.

>  
>  	cdev_init(&ctrldev->cdev, &rpmsg_ctrldev_fops);
>  	ctrldev->cdev.owner = THIS_MODULE;
> -- 
> 2.17.1
> 

Re: [PATCH v5 05/16] rpmsg: char: dissociate the control device from the rpmsg class

[Arnaud POULIQUEN]

On 3/2/21 7:01 PM, Mathieu Poirier wrote:
> On Fri, Feb 19, 2021 at 12:14:50PM +0100, Arnaud Pouliquen wrote:
>> The RPMsg control device is a RPMsg device, it is already
>> referenced in the RPMsg bus. There is only an interest to
>> reference the ept char devices in the rpmsg class.
>> This patch prepares the code split of the control and end point
>> devices in two separate files.
>>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>> ---
>>  drivers/rpmsg/rpmsg_char.c | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
>> index 78a6d19fdf82..23e369a00531 100644
>> --- a/drivers/rpmsg/rpmsg_char.c
>> +++ b/drivers/rpmsg/rpmsg_char.c
>> @@ -485,7 +485,6 @@ static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
>>  	dev = &ctrldev->dev;
>>  	device_initialize(dev);
>>  	dev->parent = &rpdev->dev;
>> -	dev->class = rpmsg_class;
> 
> This may break user space...  It has been around for so long that even if the
> information is redundant we have to keep it.

Yes, this point is part of the grey space of my series...
I did it on the assumption that the "rpmsg" class interface is not used for the
control part. Indeed, the group associated to the class provides information
about the name service, the source address and the destination address of the
endpoint.These group is not defined for the control device.

That said, to preserve the interface, I can move the class creation in rpmsg
control driver, to share it between the both drivers. As consequence I will need
to manage the probe ordering of the char and control modules to ensure that the
class is created before used. This should be solved by reintroducing patch[1]
with a fix for the compilation warning.

[1]https://lkml.org/lkml/2021/2/4/197

Thanks,
Arnaud

> 
>>  
>>  	cdev_init(&ctrldev->cdev, &rpmsg_ctrldev_fops);
>>  	ctrldev->cdev.owner = THIS_MODULE;
>> -- 
>> 2.17.1
>>

[PATCH v5 06/16] rpmsg: move the rpmsg control device from rpmsg_char to rpmsg_ctrl

[Arnaud Pouliquen]

Move the code related to the rpmsg_ctrl char device to the new
rpmsg_ctrl.c module.
Manage the dependency in the kconfig.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
 drivers/rpmsg/Kconfig      |   9 ++
 drivers/rpmsg/Makefile     |   1 +
 drivers/rpmsg/rpmsg_char.c | 163 ----------------------------
 drivers/rpmsg/rpmsg_ctrl.c | 216 +++++++++++++++++++++++++++++++++++++
 4 files changed, 226 insertions(+), 163 deletions(-)
 create mode 100644 drivers/rpmsg/rpmsg_ctrl.c

diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
index 0b4407abdf13..2d0cd7fdd710 100644
--- a/drivers/rpmsg/Kconfig
+++ b/drivers/rpmsg/Kconfig
@@ -10,11 +10,20 @@ config RPMSG_CHAR
 	tristate "RPMSG device interface"
 	depends on RPMSG
 	depends on NET
+	select RPMSG_CTRL
 	help
 	  Say Y here to export rpmsg endpoints as device files, usually found
 	  in /dev. They make it possible for user-space programs to send and
 	  receive rpmsg packets.
 
+config RPMSG_CTRL
+	tristate "RPMSG control interface"
+	depends on RPMSG
+	help
+	  Say Y here to enable the support of the /dev/rpmsg_ctlX API. This API
+	  allows user-space programs to create endpoints with specific service name,
+	  source and destination addresses.
+
 config RPMSG_NS
 	tristate "RPMSG name service announcement"
 	depends on RPMSG
diff --git a/drivers/rpmsg/Makefile b/drivers/rpmsg/Makefile
index 8d452656f0ee..58e3b382e316 100644
--- a/drivers/rpmsg/Makefile
+++ b/drivers/rpmsg/Makefile
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_RPMSG)		+= rpmsg_core.o
 obj-$(CONFIG_RPMSG_CHAR)	+= rpmsg_char.o
+obj-$(CONFIG_RPMSG_CTRL)	+= rpmsg_ctrl.o
 obj-$(CONFIG_RPMSG_NS)		+= rpmsg_ns.o
 obj-$(CONFIG_RPMSG_MTK_SCP)	+= mtk_rpmsg.o
 qcom_glink-objs			:= qcom_glink_native.o qcom_glink_ssr.o
diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index 23e369a00531..83c10b39b139 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -31,28 +31,12 @@
 static dev_t rpmsg_major;
 static struct class *rpmsg_class;
 
-static DEFINE_IDA(rpmsg_ctrl_ida);
 static DEFINE_IDA(rpmsg_ept_ida);
 static DEFINE_IDA(rpmsg_minor_ida);
 
 #define dev_to_eptdev(dev) container_of(dev, struct rpmsg_eptdev, dev)
 #define cdev_to_eptdev(i_cdev) container_of(i_cdev, struct rpmsg_eptdev, cdev)
 
-#define dev_to_ctrldev(dev) container_of(dev, struct rpmsg_ctrldev, dev)
-#define cdev_to_ctrldev(i_cdev) container_of(i_cdev, struct rpmsg_ctrldev, cdev)
-
-/**
- * struct rpmsg_ctrldev - control device for instantiating endpoint devices
- * @rpdev:	underlaying rpmsg device
- * @cdev:	cdev for the ctrl device
- * @dev:	device for the ctrl device
- */
-struct rpmsg_ctrldev {
-	struct rpmsg_device *rpdev;
-	struct cdev cdev;
-	struct device dev;
-};
-
 /**
  * struct rpmsg_eptdev - endpoint device context
  * @dev:	endpoint device
@@ -411,145 +395,6 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent
 }
 EXPORT_SYMBOL(rpmsg_chrdev_create_eptdev);
 
-static int rpmsg_ctrldev_open(struct inode *inode, struct file *filp)
-{
-	struct rpmsg_ctrldev *ctrldev = cdev_to_ctrldev(inode->i_cdev);
-
-	get_device(&ctrldev->dev);
-	filp->private_data = ctrldev;
-
-	return 0;
-}
-
-static int rpmsg_ctrldev_release(struct inode *inode, struct file *filp)
-{
-	struct rpmsg_ctrldev *ctrldev = cdev_to_ctrldev(inode->i_cdev);
-
-	put_device(&ctrldev->dev);
-
-	return 0;
-}
-
-static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
-				unsigned long arg)
-{
-	struct rpmsg_ctrldev *ctrldev = fp->private_data;
-	void __user *argp = (void __user *)arg;
-	struct rpmsg_endpoint_info eptinfo;
-	struct rpmsg_channel_info chinfo;
-
-	if (cmd != RPMSG_CREATE_EPT_IOCTL)
-		return -EINVAL;
-
-	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
-		return -EFAULT;
-
-	memcpy(chinfo.name, eptinfo.name, RPMSG_NAME_SIZE);
-	chinfo.name[RPMSG_NAME_SIZE-1] = '\0';
-	chinfo.src = eptinfo.src;
-	chinfo.dst = eptinfo.dst;
-
-	return rpmsg_chrdev_create_eptdev(ctrldev->rpdev, &ctrldev->dev, chinfo);
-};
-
-static const struct file_operations rpmsg_ctrldev_fops = {
-	.owner = THIS_MODULE,
-	.open = rpmsg_ctrldev_open,
-	.release = rpmsg_ctrldev_release,
-	.unlocked_ioctl = rpmsg_ctrldev_ioctl,
-	.compat_ioctl = compat_ptr_ioctl,
-};
-
-static void rpmsg_ctrldev_release_device(struct device *dev)
-{
-	struct rpmsg_ctrldev *ctrldev = dev_to_ctrldev(dev);
-
-	ida_simple_remove(&rpmsg_ctrl_ida, dev->id);
-	ida_simple_remove(&rpmsg_minor_ida, MINOR(dev->devt));
-	cdev_del(&ctrldev->cdev);
-	kfree(ctrldev);
-}
-
-static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
-{
-	struct rpmsg_ctrldev *ctrldev;
-	struct device *dev;
-	int ret;
-
-	ctrldev = kzalloc(sizeof(*ctrldev), GFP_KERNEL);
-	if (!ctrldev)
-		return -ENOMEM;
-
-	ctrldev->rpdev = rpdev;
-
-	dev = &ctrldev->dev;
-	device_initialize(dev);
-	dev->parent = &rpdev->dev;
-
-	cdev_init(&ctrldev->cdev, &rpmsg_ctrldev_fops);
-	ctrldev->cdev.owner = THIS_MODULE;
-
-	ret = ida_simple_get(&rpmsg_minor_ida, 0, RPMSG_DEV_MAX, GFP_KERNEL);
-	if (ret < 0)
-		goto free_ctrldev;
-	dev->devt = MKDEV(MAJOR(rpmsg_major), ret);
-
-	ret = ida_simple_get(&rpmsg_ctrl_ida, 0, 0, GFP_KERNEL);
-	if (ret < 0)
-		goto free_minor_ida;
-	dev->id = ret;
-	dev_set_name(&ctrldev->dev, "rpmsg_ctrl%d", ret);
-
-	ret = cdev_add(&ctrldev->cdev, dev->devt, 1);
-	if (ret)
-		goto free_ctrl_ida;
-
-	/* We can now rely on the release function for cleanup */
-	dev->release = rpmsg_ctrldev_release_device;
-
-	ret = device_add(dev);
-	if (ret) {
-		dev_err(&rpdev->dev, "device_add failed: %d\n", ret);
-		put_device(dev);
-	}
-
-	dev_set_drvdata(&rpdev->dev, ctrldev);
-
-	return ret;
-
-free_ctrl_ida:
-	ida_simple_remove(&rpmsg_ctrl_ida, dev->id);
-free_minor_ida:
-	ida_simple_remove(&rpmsg_minor_ida, MINOR(dev->devt));
-free_ctrldev:
-	put_device(dev);
-	kfree(ctrldev);
-
-	return ret;
-}
-
-static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev)
-{
-	struct rpmsg_ctrldev *ctrldev = dev_get_drvdata(&rpdev->dev);
-	int ret;
-
-	/* Destroy all endpoints */
-	ret = device_for_each_child(&ctrldev->dev, NULL, rpmsg_chrdev_eptdev_destroy);
-	if (ret)
-		dev_warn(&rpdev->dev, "failed to nuke endpoints: %d\n", ret);
-
-	device_del(&ctrldev->dev);
-	put_device(&ctrldev->dev);
-}
-
-static struct rpmsg_driver rpmsg_chrdev_driver = {
-	.probe = rpmsg_chrdev_probe,
-	.remove = rpmsg_chrdev_remove,
-	.drv = {
-		.name = "rpmsg_chrdev",
-	},
-};
-
 static int rpmsg_chrdev_init(void)
 {
 	int ret;
@@ -567,20 +412,12 @@ static int rpmsg_chrdev_init(void)
 		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);
-	}
-
 	return ret;
 }
 postcore_initcall(rpmsg_chrdev_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);
 }
diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
new file mode 100644
index 000000000000..fa05b67d24da
--- /dev/null
+++ b/drivers/rpmsg/rpmsg_ctrl.c
@@ -0,0 +1,216 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2021, STMicroelectronics
+ * Copyright (c) 2016, Linaro Ltd.
+ * Copyright (c) 2012, Michal Simek <monstr@monstr.eu>
+ * Copyright (c) 2012, PetaLogix
+ * Copyright (c) 2011, Texas Instruments, Inc.
+ * Copyright (c) 2011, Google, Inc.
+ *
+ * Based on rpmsg performance statistics driver by Michal Simek, which in turn
+ * was based on TI & Google OMX rpmsg driver.
+ */
+#include <linux/cdev.h>
+#include <linux/device.h>
+#include <linux/fs.h>
+#include <linux/idr.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/rpmsg.h>
+#include <linux/slab.h>
+#include <uapi/linux/rpmsg.h>
+
+#include "rpmsg_char.h"
+#include "rpmsg_internal.h"
+
+#define RPMSG_DEV_MAX	(MINORMASK + 1)
+
+static dev_t rpmsg_major;
+
+static DEFINE_IDA(rpmsg_ctrl_ida);
+static DEFINE_IDA(rpmsg_minor_ida);
+
+#define dev_to_ctrldev(dev) container_of(dev, struct rpmsg_ctrl, dev)
+#define cdev_to_ctrldev(i_cdev) container_of(i_cdev, struct rpmsg_ctrl, cdev)
+
+/**
+ * struct rpmsg_ctrl - control device for instantiating endpoint devices
+ * @rpdev:	underlaying rpmsg device
+ * @cdev:	cdev for the ctrl device
+ * @dev:	device for the ctrl device
+ */
+struct rpmsg_ctrl {
+	struct rpmsg_device *rpdev;
+	struct cdev cdev;
+	struct device dev;
+};
+
+static int rpmsg_ctrl_open(struct inode *inode, struct file *filp)
+{
+	struct rpmsg_ctrl *ctrldev = cdev_to_ctrldev(inode->i_cdev);
+
+	get_device(&ctrldev->dev);
+	filp->private_data = ctrldev;
+
+	return 0;
+}
+
+static int rpmsg_ctrl_release(struct inode *inode, struct file *filp)
+{
+	struct rpmsg_ctrl *ctrldev = cdev_to_ctrldev(inode->i_cdev);
+
+	put_device(&ctrldev->dev);
+
+	return 0;
+}
+
+static long rpmsg_ctrl_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
+{
+	struct rpmsg_ctrl *ctrldev = fp->private_data;
+	void __user *argp = (void __user *)arg;
+	struct rpmsg_endpoint_info eptinfo;
+	struct rpmsg_channel_info chinfo;
+
+	if (cmd != RPMSG_CREATE_EPT_IOCTL)
+		return -EINVAL;
+
+	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
+		return -EFAULT;
+
+	memcpy(chinfo.name, eptinfo.name, RPMSG_NAME_SIZE);
+	chinfo.name[RPMSG_NAME_SIZE - 1] = '\0';
+	chinfo.src = eptinfo.src;
+	chinfo.dst = eptinfo.dst;
+
+	return rpmsg_chrdev_create_eptdev(ctrldev->rpdev, &ctrldev->dev, chinfo);
+};
+
+static const struct file_operations rpmsg_ctrl_fops = {
+	.owner = THIS_MODULE,
+	.open = rpmsg_ctrl_open,
+	.release = rpmsg_ctrl_release,
+	.unlocked_ioctl = rpmsg_ctrl_ioctl,
+	.compat_ioctl = compat_ptr_ioctl,
+};
+
+static void rpmsg_ctrl_release_device(struct device *dev)
+{
+	struct rpmsg_ctrl *ctrldev = dev_to_ctrldev(dev);
+
+	ida_simple_remove(&rpmsg_ctrl_ida, dev->id);
+	ida_simple_remove(&rpmsg_minor_ida, MINOR(dev->devt));
+	cdev_del(&ctrldev->cdev);
+	kfree(ctrldev);
+}
+
+static int rpmsg_ctrl_probe(struct rpmsg_device *rpdev)
+{
+	struct rpmsg_ctrl *ctrldev;
+	struct device *dev;
+	int ret;
+
+	ctrldev = kzalloc(sizeof(*ctrldev), GFP_KERNEL);
+	if (!ctrldev)
+		return -ENOMEM;
+
+	ctrldev->rpdev = rpdev;
+
+	dev = &ctrldev->dev;
+	device_initialize(dev);
+	dev->parent = &rpdev->dev;
+
+	cdev_init(&ctrldev->cdev, &rpmsg_ctrl_fops);
+	ctrldev->cdev.owner = THIS_MODULE;
+
+	ret = ida_simple_get(&rpmsg_minor_ida, 0, RPMSG_DEV_MAX, GFP_KERNEL);
+	if (ret < 0)
+		goto free_ctrldev;
+	dev->devt = MKDEV(MAJOR(rpmsg_major), ret);
+
+	ret = ida_simple_get(&rpmsg_ctrl_ida, 0, 0, GFP_KERNEL);
+	if (ret < 0)
+		goto free_minor_ida;
+	dev->id = ret;
+	dev_set_name(&ctrldev->dev, "rpmsg_ctrl%d", ret);
+
+	ret = cdev_add(&ctrldev->cdev, dev->devt, 1);
+	if (ret)
+		goto free_ctrl_ida;
+
+	/* We can now rely on the release function for cleanup */
+	dev->release = rpmsg_ctrl_release_device;
+
+	ret = device_add(dev);
+	if (ret) {
+		dev_err(&rpdev->dev, "device_add failed: %d\n", ret);
+		put_device(dev);
+	}
+
+	dev_set_drvdata(&rpdev->dev, ctrldev);
+
+	return ret;
+
+free_ctrl_ida:
+	ida_simple_remove(&rpmsg_ctrl_ida, dev->id);
+free_minor_ida:
+	ida_simple_remove(&rpmsg_minor_ida, MINOR(dev->devt));
+free_ctrldev:
+	put_device(dev);
+	kfree(ctrldev);
+
+	return ret;
+}
+
+static void rpmsg_ctrl_remove(struct rpmsg_device *rpdev)
+{
+	struct rpmsg_ctrl *ctrldev = dev_get_drvdata(&rpdev->dev);
+	int ret;
+
+	/* Destroy all endpoints */
+	ret = device_for_each_child(&ctrldev->dev, NULL,
+				    rpmsg_chrdev_eptdev_destroy);
+	if (ret)
+		dev_warn(&rpdev->dev, "failed to nuke endpoints: %d\n", ret);
+
+	device_del(&ctrldev->dev);
+	put_device(&ctrldev->dev);
+}
+
+static struct rpmsg_driver rpmsg_ctrl_driver = {
+	.probe = rpmsg_ctrl_probe,
+	.remove = rpmsg_ctrl_remove,
+	.drv = {
+		.name = "rpmsg_chrdev",
+	},
+};
+
+static int rpmsg_ctrl_init(void)
+{
+	int ret;
+
+	ret = alloc_chrdev_region(&rpmsg_major, 0, RPMSG_DEV_MAX, "rpmsg");
+	if (ret < 0) {
+		pr_err("rpmsg: failed to allocate char dev region\n");
+		return ret;
+	}
+
+	ret = register_rpmsg_driver(&rpmsg_ctrl_driver);
+	if (ret < 0) {
+		pr_err("rpmsg ctrl: failed to register rpmsg driver\n");
+		unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
+	}
+
+	return ret;
+}
+postcore_initcall(rpmsg_ctrl_init);
+
+static void rpmsg_ctrl_exit(void)
+{
+	unregister_rpmsg_driver(&rpmsg_ctrl_driver);
+	unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
+}
+module_exit(rpmsg_ctrl_exit);
+
+MODULE_DESCRIPTION("rpmsg control interface");
+MODULE_ALIAS("rpmsg:" KBUILD_MODNAME);
+MODULE_LICENSE("GPL v2");
-- 
2.17.1

Re: [PATCH v5 06/16] rpmsg: move the rpmsg control device from rpmsg_char to rpmsg_ctrl

[Mathieu Poirier]

On Fri, Feb 19, 2021 at 12:14:51PM +0100, Arnaud Pouliquen wrote:
> Move the code related to the rpmsg_ctrl char device to the new
> rpmsg_ctrl.c module.
> Manage the dependency in the kconfig.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
>  drivers/rpmsg/Kconfig      |   9 ++
>  drivers/rpmsg/Makefile     |   1 +
>  drivers/rpmsg/rpmsg_char.c | 163 ----------------------------
>  drivers/rpmsg/rpmsg_ctrl.c | 216 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 226 insertions(+), 163 deletions(-)
>  create mode 100644 drivers/rpmsg/rpmsg_ctrl.c
> 
> diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
> index 0b4407abdf13..2d0cd7fdd710 100644
> --- a/drivers/rpmsg/Kconfig
> +++ b/drivers/rpmsg/Kconfig
> @@ -10,11 +10,20 @@ config RPMSG_CHAR
>  	tristate "RPMSG device interface"
>  	depends on RPMSG
>  	depends on NET
> +	select RPMSG_CTRL
>  	help
>  	  Say Y here to export rpmsg endpoints as device files, usually found
>  	  in /dev. They make it possible for user-space programs to send and
>  	  receive rpmsg packets.
>  
> +config RPMSG_CTRL
> +	tristate "RPMSG control interface"
> +	depends on RPMSG
> +	help
> +	  Say Y here to enable the support of the /dev/rpmsg_ctlX API. This API

s/rpmsg_ctlX/rpmsg_ctrlX

> +	  allows user-space programs to create endpoints with specific service name,
> +	  source and destination addresses.
> +
>  config RPMSG_NS
>  	tristate "RPMSG name service announcement"
>  	depends on RPMSG
> diff --git a/drivers/rpmsg/Makefile b/drivers/rpmsg/Makefile
> index 8d452656f0ee..58e3b382e316 100644
> --- a/drivers/rpmsg/Makefile
> +++ b/drivers/rpmsg/Makefile
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-$(CONFIG_RPMSG)		+= rpmsg_core.o
>  obj-$(CONFIG_RPMSG_CHAR)	+= rpmsg_char.o
> +obj-$(CONFIG_RPMSG_CTRL)	+= rpmsg_ctrl.o
>  obj-$(CONFIG_RPMSG_NS)		+= rpmsg_ns.o
>  obj-$(CONFIG_RPMSG_MTK_SCP)	+= mtk_rpmsg.o
>  qcom_glink-objs			:= qcom_glink_native.o qcom_glink_ssr.o
> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> index 23e369a00531..83c10b39b139 100644
> --- a/drivers/rpmsg/rpmsg_char.c
> +++ b/drivers/rpmsg/rpmsg_char.c
> @@ -31,28 +31,12 @@
>  static dev_t rpmsg_major;
>  static struct class *rpmsg_class;
>  
> -static DEFINE_IDA(rpmsg_ctrl_ida);
>  static DEFINE_IDA(rpmsg_ept_ida);
>  static DEFINE_IDA(rpmsg_minor_ida);
>  
>  #define dev_to_eptdev(dev) container_of(dev, struct rpmsg_eptdev, dev)
>  #define cdev_to_eptdev(i_cdev) container_of(i_cdev, struct rpmsg_eptdev, cdev)
>  
> -#define dev_to_ctrldev(dev) container_of(dev, struct rpmsg_ctrldev, dev)
> -#define cdev_to_ctrldev(i_cdev) container_of(i_cdev, struct rpmsg_ctrldev, cdev)
> -
> -/**
> - * struct rpmsg_ctrldev - control device for instantiating endpoint devices
> - * @rpdev:	underlaying rpmsg device
> - * @cdev:	cdev for the ctrl device
> - * @dev:	device for the ctrl device
> - */
> -struct rpmsg_ctrldev {
> -	struct rpmsg_device *rpdev;
> -	struct cdev cdev;
> -	struct device dev;
> -};

This showed up in rpmsg_ctrl.c as rpmsg_ctrl.  The same goes for many functions
names - they are removed here and re-introduced under a different name, which
makes it very hard to follow.  What ends up in the new file should be a carbon
copy of what was moved.

I'm out of time for today, more comments tomorrow.

Thanks,
Mathieu

> -
>  /**
>   * struct rpmsg_eptdev - endpoint device context
>   * @dev:	endpoint device
> @@ -411,145 +395,6 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent
>  }
>  EXPORT_SYMBOL(rpmsg_chrdev_create_eptdev);
>  
> -static int rpmsg_ctrldev_open(struct inode *inode, struct file *filp)
> -{
> -	struct rpmsg_ctrldev *ctrldev = cdev_to_ctrldev(inode->i_cdev);
> -
> -	get_device(&ctrldev->dev);
> -	filp->private_data = ctrldev;
> -
> -	return 0;
> -}
> -
> -static int rpmsg_ctrldev_release(struct inode *inode, struct file *filp)
> -{
> -	struct rpmsg_ctrldev *ctrldev = cdev_to_ctrldev(inode->i_cdev);
> -
> -	put_device(&ctrldev->dev);
> -
> -	return 0;
> -}
> -
> -static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
> -				unsigned long arg)
> -{
> -	struct rpmsg_ctrldev *ctrldev = fp->private_data;
> -	void __user *argp = (void __user *)arg;
> -	struct rpmsg_endpoint_info eptinfo;
> -	struct rpmsg_channel_info chinfo;
> -
> -	if (cmd != RPMSG_CREATE_EPT_IOCTL)
> -		return -EINVAL;
> -
> -	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
> -		return -EFAULT;
> -
> -	memcpy(chinfo.name, eptinfo.name, RPMSG_NAME_SIZE);
> -	chinfo.name[RPMSG_NAME_SIZE-1] = '\0';
> -	chinfo.src = eptinfo.src;
> -	chinfo.dst = eptinfo.dst;
> -
> -	return rpmsg_chrdev_create_eptdev(ctrldev->rpdev, &ctrldev->dev, chinfo);
> -};
> -
> -static const struct file_operations rpmsg_ctrldev_fops = {
> -	.owner = THIS_MODULE,
> -	.open = rpmsg_ctrldev_open,
> -	.release = rpmsg_ctrldev_release,
> -	.unlocked_ioctl = rpmsg_ctrldev_ioctl,
> -	.compat_ioctl = compat_ptr_ioctl,
> -};
> -
> -static void rpmsg_ctrldev_release_device(struct device *dev)
> -{
> -	struct rpmsg_ctrldev *ctrldev = dev_to_ctrldev(dev);
> -
> -	ida_simple_remove(&rpmsg_ctrl_ida, dev->id);
> -	ida_simple_remove(&rpmsg_minor_ida, MINOR(dev->devt));
> -	cdev_del(&ctrldev->cdev);
> -	kfree(ctrldev);
> -}
> -
> -static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
> -{
> -	struct rpmsg_ctrldev *ctrldev;
> -	struct device *dev;
> -	int ret;
> -
> -	ctrldev = kzalloc(sizeof(*ctrldev), GFP_KERNEL);
> -	if (!ctrldev)
> -		return -ENOMEM;
> -
> -	ctrldev->rpdev = rpdev;
> -
> -	dev = &ctrldev->dev;
> -	device_initialize(dev);
> -	dev->parent = &rpdev->dev;
> -
> -	cdev_init(&ctrldev->cdev, &rpmsg_ctrldev_fops);
> -	ctrldev->cdev.owner = THIS_MODULE;
> -
> -	ret = ida_simple_get(&rpmsg_minor_ida, 0, RPMSG_DEV_MAX, GFP_KERNEL);
> -	if (ret < 0)
> -		goto free_ctrldev;
> -	dev->devt = MKDEV(MAJOR(rpmsg_major), ret);
> -
> -	ret = ida_simple_get(&rpmsg_ctrl_ida, 0, 0, GFP_KERNEL);
> -	if (ret < 0)
> -		goto free_minor_ida;
> -	dev->id = ret;
> -	dev_set_name(&ctrldev->dev, "rpmsg_ctrl%d", ret);
> -
> -	ret = cdev_add(&ctrldev->cdev, dev->devt, 1);
> -	if (ret)
> -		goto free_ctrl_ida;
> -
> -	/* We can now rely on the release function for cleanup */
> -	dev->release = rpmsg_ctrldev_release_device;
> -
> -	ret = device_add(dev);
> -	if (ret) {
> -		dev_err(&rpdev->dev, "device_add failed: %d\n", ret);
> -		put_device(dev);
> -	}
> -
> -	dev_set_drvdata(&rpdev->dev, ctrldev);
> -
> -	return ret;
> -
> -free_ctrl_ida:
> -	ida_simple_remove(&rpmsg_ctrl_ida, dev->id);
> -free_minor_ida:
> -	ida_simple_remove(&rpmsg_minor_ida, MINOR(dev->devt));
> -free_ctrldev:
> -	put_device(dev);
> -	kfree(ctrldev);
> -
> -	return ret;
> -}
> -
> -static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev)
> -{
> -	struct rpmsg_ctrldev *ctrldev = dev_get_drvdata(&rpdev->dev);
> -	int ret;
> -
> -	/* Destroy all endpoints */
> -	ret = device_for_each_child(&ctrldev->dev, NULL, rpmsg_chrdev_eptdev_destroy);
> -	if (ret)
> -		dev_warn(&rpdev->dev, "failed to nuke endpoints: %d\n", ret);
> -
> -	device_del(&ctrldev->dev);
> -	put_device(&ctrldev->dev);
> -}
> -
> -static struct rpmsg_driver rpmsg_chrdev_driver = {
> -	.probe = rpmsg_chrdev_probe,
> -	.remove = rpmsg_chrdev_remove,
> -	.drv = {
> -		.name = "rpmsg_chrdev",
> -	},
> -};
> -
>  static int rpmsg_chrdev_init(void)
>  {
>  	int ret;
> @@ -567,20 +412,12 @@ static int rpmsg_chrdev_init(void)
>  		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);
> -	}
> -
>  	return ret;
>  }
>  postcore_initcall(rpmsg_chrdev_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);
>  }
> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
> new file mode 100644
> index 000000000000..fa05b67d24da
> --- /dev/null
> +++ b/drivers/rpmsg/rpmsg_ctrl.c
> @@ -0,0 +1,216 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2021, STMicroelectronics
> + * Copyright (c) 2016, Linaro Ltd.
> + * Copyright (c) 2012, Michal Simek <monstr@monstr.eu>
> + * Copyright (c) 2012, PetaLogix
> + * Copyright (c) 2011, Texas Instruments, Inc.
> + * Copyright (c) 2011, Google, Inc.
> + *
> + * Based on rpmsg performance statistics driver by Michal Simek, which in turn
> + * was based on TI & Google OMX rpmsg driver.
> + */
> +#include <linux/cdev.h>
> +#include <linux/device.h>
> +#include <linux/fs.h>
> +#include <linux/idr.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/rpmsg.h>
> +#include <linux/slab.h>
> +#include <uapi/linux/rpmsg.h>
> +
> +#include "rpmsg_char.h"
> +#include "rpmsg_internal.h"
> +
> +#define RPMSG_DEV_MAX	(MINORMASK + 1)
> +
> +static dev_t rpmsg_major;
> +
> +static DEFINE_IDA(rpmsg_ctrl_ida);
> +static DEFINE_IDA(rpmsg_minor_ida);
> +
> +#define dev_to_ctrldev(dev) container_of(dev, struct rpmsg_ctrl, dev)
> +#define cdev_to_ctrldev(i_cdev) container_of(i_cdev, struct rpmsg_ctrl, cdev)
> +
> +/**
> + * struct rpmsg_ctrl - control device for instantiating endpoint devices
> + * @rpdev:	underlaying rpmsg device
> + * @cdev:	cdev for the ctrl device
> + * @dev:	device for the ctrl device
> + */
> +struct rpmsg_ctrl {
> +	struct rpmsg_device *rpdev;
> +	struct cdev cdev;
> +	struct device dev;
> +};
> +
> +static int rpmsg_ctrl_open(struct inode *inode, struct file *filp)
> +{
> +	struct rpmsg_ctrl *ctrldev = cdev_to_ctrldev(inode->i_cdev);
> +
> +	get_device(&ctrldev->dev);
> +	filp->private_data = ctrldev;
> +
> +	return 0;
> +}
> +
> +static int rpmsg_ctrl_release(struct inode *inode, struct file *filp)
> +{
> +	struct rpmsg_ctrl *ctrldev = cdev_to_ctrldev(inode->i_cdev);
> +
> +	put_device(&ctrldev->dev);
> +
> +	return 0;
> +}
> +
> +static long rpmsg_ctrl_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
> +{
> +	struct rpmsg_ctrl *ctrldev = fp->private_data;
> +	void __user *argp = (void __user *)arg;
> +	struct rpmsg_endpoint_info eptinfo;
> +	struct rpmsg_channel_info chinfo;
> +
> +	if (cmd != RPMSG_CREATE_EPT_IOCTL)
> +		return -EINVAL;
> +
> +	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
> +		return -EFAULT;
> +
> +	memcpy(chinfo.name, eptinfo.name, RPMSG_NAME_SIZE);
> +	chinfo.name[RPMSG_NAME_SIZE - 1] = '\0';
> +	chinfo.src = eptinfo.src;
> +	chinfo.dst = eptinfo.dst;
> +
> +	return rpmsg_chrdev_create_eptdev(ctrldev->rpdev, &ctrldev->dev, chinfo);
> +};
> +
> +static const struct file_operations rpmsg_ctrl_fops = {
> +	.owner = THIS_MODULE,
> +	.open = rpmsg_ctrl_open,
> +	.release = rpmsg_ctrl_release,
> +	.unlocked_ioctl = rpmsg_ctrl_ioctl,
> +	.compat_ioctl = compat_ptr_ioctl,
> +};
> +
> +static void rpmsg_ctrl_release_device(struct device *dev)
> +{
> +	struct rpmsg_ctrl *ctrldev = dev_to_ctrldev(dev);
> +
> +	ida_simple_remove(&rpmsg_ctrl_ida, dev->id);
> +	ida_simple_remove(&rpmsg_minor_ida, MINOR(dev->devt));
> +	cdev_del(&ctrldev->cdev);
> +	kfree(ctrldev);
> +}
> +
> +static int rpmsg_ctrl_probe(struct rpmsg_device *rpdev)
> +{
> +	struct rpmsg_ctrl *ctrldev;
> +	struct device *dev;
> +	int ret;
> +
> +	ctrldev = kzalloc(sizeof(*ctrldev), GFP_KERNEL);
> +	if (!ctrldev)
> +		return -ENOMEM;
> +
> +	ctrldev->rpdev = rpdev;
> +
> +	dev = &ctrldev->dev;
> +	device_initialize(dev);
> +	dev->parent = &rpdev->dev;
> +
> +	cdev_init(&ctrldev->cdev, &rpmsg_ctrl_fops);
> +	ctrldev->cdev.owner = THIS_MODULE;
> +
> +	ret = ida_simple_get(&rpmsg_minor_ida, 0, RPMSG_DEV_MAX, GFP_KERNEL);
> +	if (ret < 0)
> +		goto free_ctrldev;
> +	dev->devt = MKDEV(MAJOR(rpmsg_major), ret);
> +
> +	ret = ida_simple_get(&rpmsg_ctrl_ida, 0, 0, GFP_KERNEL);
> +	if (ret < 0)
> +		goto free_minor_ida;
> +	dev->id = ret;
> +	dev_set_name(&ctrldev->dev, "rpmsg_ctrl%d", ret);
> +
> +	ret = cdev_add(&ctrldev->cdev, dev->devt, 1);
> +	if (ret)
> +		goto free_ctrl_ida;
> +
> +	/* We can now rely on the release function for cleanup */
> +	dev->release = rpmsg_ctrl_release_device;
> +
> +	ret = device_add(dev);
> +	if (ret) {
> +		dev_err(&rpdev->dev, "device_add failed: %d\n", ret);
> +		put_device(dev);
> +	}
> +
> +	dev_set_drvdata(&rpdev->dev, ctrldev);
> +
> +	return ret;
> +
> +free_ctrl_ida:
> +	ida_simple_remove(&rpmsg_ctrl_ida, dev->id);
> +free_minor_ida:
> +	ida_simple_remove(&rpmsg_minor_ida, MINOR(dev->devt));
> +free_ctrldev:
> +	put_device(dev);
> +	kfree(ctrldev);
> +
> +	return ret;
> +}
> +
> +static void rpmsg_ctrl_remove(struct rpmsg_device *rpdev)
> +{
> +	struct rpmsg_ctrl *ctrldev = dev_get_drvdata(&rpdev->dev);
> +	int ret;
> +
> +	/* Destroy all endpoints */
> +	ret = device_for_each_child(&ctrldev->dev, NULL,
> +				    rpmsg_chrdev_eptdev_destroy);
> +	if (ret)
> +		dev_warn(&rpdev->dev, "failed to nuke endpoints: %d\n", ret);
> +
> +	device_del(&ctrldev->dev);
> +	put_device(&ctrldev->dev);
> +}
> +
> +static struct rpmsg_driver rpmsg_ctrl_driver = {
> +	.probe = rpmsg_ctrl_probe,
> +	.remove = rpmsg_ctrl_remove,
> +	.drv = {
> +		.name = "rpmsg_chrdev",
> +	},
> +};
> +
> +static int rpmsg_ctrl_init(void)
> +{
> +	int ret;
> +
> +	ret = alloc_chrdev_region(&rpmsg_major, 0, RPMSG_DEV_MAX, "rpmsg");
> +	if (ret < 0) {
> +		pr_err("rpmsg: failed to allocate char dev region\n");
> +		return ret;
> +	}
> +
> +	ret = register_rpmsg_driver(&rpmsg_ctrl_driver);
> +	if (ret < 0) {
> +		pr_err("rpmsg ctrl: failed to register rpmsg driver\n");
> +		unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
> +	}
> +
> +	return ret;
> +}
> +postcore_initcall(rpmsg_ctrl_init);
> +
> +static void rpmsg_ctrl_exit(void)
> +{
> +	unregister_rpmsg_driver(&rpmsg_ctrl_driver);
> +	unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
> +}
> +module_exit(rpmsg_ctrl_exit);
> +
> +MODULE_DESCRIPTION("rpmsg control interface");
> +MODULE_ALIAS("rpmsg:" KBUILD_MODNAME);
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.17.1
> 

Re: [PATCH v5 06/16] rpmsg: move the rpmsg control device from rpmsg_char to rpmsg_ctrl

[Arnaud POULIQUEN]

On 3/2/21 7:35 PM, Mathieu Poirier wrote:
> On Fri, Feb 19, 2021 at 12:14:51PM +0100, Arnaud Pouliquen wrote:
>> Move the code related to the rpmsg_ctrl char device to the new
>> rpmsg_ctrl.c module.
>> Manage the dependency in the kconfig.
>>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>> ---
>>  drivers/rpmsg/Kconfig      |   9 ++
>>  drivers/rpmsg/Makefile     |   1 +
>>  drivers/rpmsg/rpmsg_char.c | 163 ----------------------------
>>  drivers/rpmsg/rpmsg_ctrl.c | 216 +++++++++++++++++++++++++++++++++++++
>>  4 files changed, 226 insertions(+), 163 deletions(-)
>>  create mode 100644 drivers/rpmsg/rpmsg_ctrl.c
>>
>> diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
>> index 0b4407abdf13..2d0cd7fdd710 100644
>> --- a/drivers/rpmsg/Kconfig
>> +++ b/drivers/rpmsg/Kconfig
>> @@ -10,11 +10,20 @@ config RPMSG_CHAR
>>  	tristate "RPMSG device interface"
>>  	depends on RPMSG
>>  	depends on NET
>> +	select RPMSG_CTRL
>>  	help
>>  	  Say Y here to export rpmsg endpoints as device files, usually found
>>  	  in /dev. They make it possible for user-space programs to send and
>>  	  receive rpmsg packets.
>>  
>> +config RPMSG_CTRL
>> +	tristate "RPMSG control interface"
>> +	depends on RPMSG
>> +	help
>> +	  Say Y here to enable the support of the /dev/rpmsg_ctlX API. This API
> 
> s/rpmsg_ctlX/rpmsg_ctrlX

Good catch!

> 
>> +	  allows user-space programs to create endpoints with specific service name,
>> +	  source and destination addresses.
>> +
>>  config RPMSG_NS
>>  	tristate "RPMSG name service announcement"
>>  	depends on RPMSG
>> diff --git a/drivers/rpmsg/Makefile b/drivers/rpmsg/Makefile
>> index 8d452656f0ee..58e3b382e316 100644
>> --- a/drivers/rpmsg/Makefile
>> +++ b/drivers/rpmsg/Makefile
>> @@ -1,6 +1,7 @@
>>  # SPDX-License-Identifier: GPL-2.0
>>  obj-$(CONFIG_RPMSG)		+= rpmsg_core.o
>>  obj-$(CONFIG_RPMSG_CHAR)	+= rpmsg_char.o
>> +obj-$(CONFIG_RPMSG_CTRL)	+= rpmsg_ctrl.o
>>  obj-$(CONFIG_RPMSG_NS)		+= rpmsg_ns.o
>>  obj-$(CONFIG_RPMSG_MTK_SCP)	+= mtk_rpmsg.o
>>  qcom_glink-objs			:= qcom_glink_native.o qcom_glink_ssr.o
>> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
>> index 23e369a00531..83c10b39b139 100644
>> --- a/drivers/rpmsg/rpmsg_char.c
>> +++ b/drivers/rpmsg/rpmsg_char.c
>> @@ -31,28 +31,12 @@
>>  static dev_t rpmsg_major;
>>  static struct class *rpmsg_class;
>>  
>> -static DEFINE_IDA(rpmsg_ctrl_ida);
>>  static DEFINE_IDA(rpmsg_ept_ida);
>>  static DEFINE_IDA(rpmsg_minor_ida);
>>  
>>  #define dev_to_eptdev(dev) container_of(dev, struct rpmsg_eptdev, dev)
>>  #define cdev_to_eptdev(i_cdev) container_of(i_cdev, struct rpmsg_eptdev, cdev)
>>  
>> -#define dev_to_ctrldev(dev) container_of(dev, struct rpmsg_ctrldev, dev)
>> -#define cdev_to_ctrldev(i_cdev) container_of(i_cdev, struct rpmsg_ctrldev, cdev)
>> -
>> -/**
>> - * struct rpmsg_ctrldev - control device for instantiating endpoint devices
>> - * @rpdev:	underlaying rpmsg device
>> - * @cdev:	cdev for the ctrl device
>> - * @dev:	device for the ctrl device
>> - */
>> -struct rpmsg_ctrldev {
>> -	struct rpmsg_device *rpdev;
>> -	struct cdev cdev;
>> -	struct device dev;
>> -};
> 
> This showed up in rpmsg_ctrl.c as rpmsg_ctrl.  The same goes for many functions
> names - they are removed here and re-introduced under a different name, which
> makes it very hard to follow.  What ends up in the new file should be a carbon
> copy of what was moved.

Ok i will split it in 2 steps.

Thanks
Arnaud

> 
> I'm out of time for today, more comments tomorrow.
> 
> Thanks,
> Mathieu
> 
>> -
>>  /**
>>   * struct rpmsg_eptdev - endpoint device context
>>   * @dev:	endpoint device
>> @@ -411,145 +395,6 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent
>>  }
>>  EXPORT_SYMBOL(rpmsg_chrdev_create_eptdev);
>>  
>> -static int rpmsg_ctrldev_open(struct inode *inode, struct file *filp)
>> -{
>> -	struct rpmsg_ctrldev *ctrldev = cdev_to_ctrldev(inode->i_cdev);
>> -
>> -	get_device(&ctrldev->dev);
>> -	filp->private_data = ctrldev;
>> -
>> -	return 0;
>> -}
>> -
>> -static int rpmsg_ctrldev_release(struct inode *inode, struct file *filp)
>> -{
>> -	struct rpmsg_ctrldev *ctrldev = cdev_to_ctrldev(inode->i_cdev);
>> -
>> -	put_device(&ctrldev->dev);
>> -
>> -	return 0;
>> -}
>> -
>> -static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
>> -				unsigned long arg)
>> -{
>> -	struct rpmsg_ctrldev *ctrldev = fp->private_data;
>> -	void __user *argp = (void __user *)arg;
>> -	struct rpmsg_endpoint_info eptinfo;
>> -	struct rpmsg_channel_info chinfo;
>> -
>> -	if (cmd != RPMSG_CREATE_EPT_IOCTL)
>> -		return -EINVAL;
>> -
>> -	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
>> -		return -EFAULT;
>> -
>> -	memcpy(chinfo.name, eptinfo.name, RPMSG_NAME_SIZE);
>> -	chinfo.name[RPMSG_NAME_SIZE-1] = '\0';
>> -	chinfo.src = eptinfo.src;
>> -	chinfo.dst = eptinfo.dst;
>> -
>> -	return rpmsg_chrdev_create_eptdev(ctrldev->rpdev, &ctrldev->dev, chinfo);
>> -};
>> -
>> -static const struct file_operations rpmsg_ctrldev_fops = {
>> -	.owner = THIS_MODULE,
>> -	.open = rpmsg_ctrldev_open,
>> -	.release = rpmsg_ctrldev_release,
>> -	.unlocked_ioctl = rpmsg_ctrldev_ioctl,
>> -	.compat_ioctl = compat_ptr_ioctl,
>> -};
>> -
>> -static void rpmsg_ctrldev_release_device(struct device *dev)
>> -{
>> -	struct rpmsg_ctrldev *ctrldev = dev_to_ctrldev(dev);
>> -
>> -	ida_simple_remove(&rpmsg_ctrl_ida, dev->id);
>> -	ida_simple_remove(&rpmsg_minor_ida, MINOR(dev->devt));
>> -	cdev_del(&ctrldev->cdev);
>> -	kfree(ctrldev);
>> -}
>> -
>> -static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
>> -{
>> -	struct rpmsg_ctrldev *ctrldev;
>> -	struct device *dev;
>> -	int ret;
>> -
>> -	ctrldev = kzalloc(sizeof(*ctrldev), GFP_KERNEL);
>> -	if (!ctrldev)
>> -		return -ENOMEM;
>> -
>> -	ctrldev->rpdev = rpdev;
>> -
>> -	dev = &ctrldev->dev;
>> -	device_initialize(dev);
>> -	dev->parent = &rpdev->dev;
>> -
>> -	cdev_init(&ctrldev->cdev, &rpmsg_ctrldev_fops);
>> -	ctrldev->cdev.owner = THIS_MODULE;
>> -
>> -	ret = ida_simple_get(&rpmsg_minor_ida, 0, RPMSG_DEV_MAX, GFP_KERNEL);
>> -	if (ret < 0)
>> -		goto free_ctrldev;
>> -	dev->devt = MKDEV(MAJOR(rpmsg_major), ret);
>> -
>> -	ret = ida_simple_get(&rpmsg_ctrl_ida, 0, 0, GFP_KERNEL);
>> -	if (ret < 0)
>> -		goto free_minor_ida;
>> -	dev->id = ret;
>> -	dev_set_name(&ctrldev->dev, "rpmsg_ctrl%d", ret);
>> -
>> -	ret = cdev_add(&ctrldev->cdev, dev->devt, 1);
>> -	if (ret)
>> -		goto free_ctrl_ida;
>> -
>> -	/* We can now rely on the release function for cleanup */
>> -	dev->release = rpmsg_ctrldev_release_device;
>> -
>> -	ret = device_add(dev);
>> -	if (ret) {
>> -		dev_err(&rpdev->dev, "device_add failed: %d\n", ret);
>> -		put_device(dev);
>> -	}
>> -
>> -	dev_set_drvdata(&rpdev->dev, ctrldev);
>> -
>> -	return ret;
>> -
>> -free_ctrl_ida:
>> -	ida_simple_remove(&rpmsg_ctrl_ida, dev->id);
>> -free_minor_ida:
>> -	ida_simple_remove(&rpmsg_minor_ida, MINOR(dev->devt));
>> -free_ctrldev:
>> -	put_device(dev);
>> -	kfree(ctrldev);
>> -
>> -	return ret;
>> -}
>> -
>> -static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev)
>> -{
>> -	struct rpmsg_ctrldev *ctrldev = dev_get_drvdata(&rpdev->dev);
>> -	int ret;
>> -
>> -	/* Destroy all endpoints */
>> -	ret = device_for_each_child(&ctrldev->dev, NULL, rpmsg_chrdev_eptdev_destroy);
>> -	if (ret)
>> -		dev_warn(&rpdev->dev, "failed to nuke endpoints: %d\n", ret);
>> -
>> -	device_del(&ctrldev->dev);
>> -	put_device(&ctrldev->dev);
>> -}
>> -
>> -static struct rpmsg_driver rpmsg_chrdev_driver = {
>> -	.probe = rpmsg_chrdev_probe,
>> -	.remove = rpmsg_chrdev_remove,
>> -	.drv = {
>> -		.name = "rpmsg_chrdev",
>> -	},
>> -};
>> -
>>  static int rpmsg_chrdev_init(void)
>>  {
>>  	int ret;
>> @@ -567,20 +412,12 @@ static int rpmsg_chrdev_init(void)
>>  		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);
>> -	}
>> -
>>  	return ret;
>>  }
>>  postcore_initcall(rpmsg_chrdev_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);
>>  }
>> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
>> new file mode 100644
>> index 000000000000..fa05b67d24da
>> --- /dev/null
>> +++ b/drivers/rpmsg/rpmsg_ctrl.c
>> @@ -0,0 +1,216 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2021, STMicroelectronics
>> + * Copyright (c) 2016, Linaro Ltd.
>> + * Copyright (c) 2012, Michal Simek <monstr@monstr.eu>
>> + * Copyright (c) 2012, PetaLogix
>> + * Copyright (c) 2011, Texas Instruments, Inc.
>> + * Copyright (c) 2011, Google, Inc.
>> + *
>> + * Based on rpmsg performance statistics driver by Michal Simek, which in turn
>> + * was based on TI & Google OMX rpmsg driver.
>> + */
>> +#include <linux/cdev.h>
>> +#include <linux/device.h>
>> +#include <linux/fs.h>
>> +#include <linux/idr.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/rpmsg.h>
>> +#include <linux/slab.h>
>> +#include <uapi/linux/rpmsg.h>
>> +
>> +#include "rpmsg_char.h"
>> +#include "rpmsg_internal.h"
>> +
>> +#define RPMSG_DEV_MAX	(MINORMASK + 1)
>> +
>> +static dev_t rpmsg_major;
>> +
>> +static DEFINE_IDA(rpmsg_ctrl_ida);
>> +static DEFINE_IDA(rpmsg_minor_ida);
>> +
>> +#define dev_to_ctrldev(dev) container_of(dev, struct rpmsg_ctrl, dev)
>> +#define cdev_to_ctrldev(i_cdev) container_of(i_cdev, struct rpmsg_ctrl, cdev)
>> +
>> +/**
>> + * struct rpmsg_ctrl - control device for instantiating endpoint devices
>> + * @rpdev:	underlaying rpmsg device
>> + * @cdev:	cdev for the ctrl device
>> + * @dev:	device for the ctrl device
>> + */
>> +struct rpmsg_ctrl {
>> +	struct rpmsg_device *rpdev;
>> +	struct cdev cdev;
>> +	struct device dev;
>> +};
>> +
>> +static int rpmsg_ctrl_open(struct inode *inode, struct file *filp)
>> +{
>> +	struct rpmsg_ctrl *ctrldev = cdev_to_ctrldev(inode->i_cdev);
>> +
>> +	get_device(&ctrldev->dev);
>> +	filp->private_data = ctrldev;
>> +
>> +	return 0;
>> +}
>> +
>> +static int rpmsg_ctrl_release(struct inode *inode, struct file *filp)
>> +{
>> +	struct rpmsg_ctrl *ctrldev = cdev_to_ctrldev(inode->i_cdev);
>> +
>> +	put_device(&ctrldev->dev);
>> +
>> +	return 0;
>> +}
>> +
>> +static long rpmsg_ctrl_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
>> +{
>> +	struct rpmsg_ctrl *ctrldev = fp->private_data;
>> +	void __user *argp = (void __user *)arg;
>> +	struct rpmsg_endpoint_info eptinfo;
>> +	struct rpmsg_channel_info chinfo;
>> +
>> +	if (cmd != RPMSG_CREATE_EPT_IOCTL)
>> +		return -EINVAL;
>> +
>> +	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
>> +		return -EFAULT;
>> +
>> +	memcpy(chinfo.name, eptinfo.name, RPMSG_NAME_SIZE);
>> +	chinfo.name[RPMSG_NAME_SIZE - 1] = '\0';
>> +	chinfo.src = eptinfo.src;
>> +	chinfo.dst = eptinfo.dst;
>> +
>> +	return rpmsg_chrdev_create_eptdev(ctrldev->rpdev, &ctrldev->dev, chinfo);
>> +};
>> +
>> +static const struct file_operations rpmsg_ctrl_fops = {
>> +	.owner = THIS_MODULE,
>> +	.open = rpmsg_ctrl_open,
>> +	.release = rpmsg_ctrl_release,
>> +	.unlocked_ioctl = rpmsg_ctrl_ioctl,
>> +	.compat_ioctl = compat_ptr_ioctl,
>> +};
>> +
>> +static void rpmsg_ctrl_release_device(struct device *dev)
>> +{
>> +	struct rpmsg_ctrl *ctrldev = dev_to_ctrldev(dev);
>> +
>> +	ida_simple_remove(&rpmsg_ctrl_ida, dev->id);
>> +	ida_simple_remove(&rpmsg_minor_ida, MINOR(dev->devt));
>> +	cdev_del(&ctrldev->cdev);
>> +	kfree(ctrldev);
>> +}
>> +
>> +static int rpmsg_ctrl_probe(struct rpmsg_device *rpdev)
>> +{
>> +	struct rpmsg_ctrl *ctrldev;
>> +	struct device *dev;
>> +	int ret;
>> +
>> +	ctrldev = kzalloc(sizeof(*ctrldev), GFP_KERNEL);
>> +	if (!ctrldev)
>> +		return -ENOMEM;
>> +
>> +	ctrldev->rpdev = rpdev;
>> +
>> +	dev = &ctrldev->dev;
>> +	device_initialize(dev);
>> +	dev->parent = &rpdev->dev;
>> +
>> +	cdev_init(&ctrldev->cdev, &rpmsg_ctrl_fops);
>> +	ctrldev->cdev.owner = THIS_MODULE;
>> +
>> +	ret = ida_simple_get(&rpmsg_minor_ida, 0, RPMSG_DEV_MAX, GFP_KERNEL);
>> +	if (ret < 0)
>> +		goto free_ctrldev;
>> +	dev->devt = MKDEV(MAJOR(rpmsg_major), ret);
>> +
>> +	ret = ida_simple_get(&rpmsg_ctrl_ida, 0, 0, GFP_KERNEL);
>> +	if (ret < 0)
>> +		goto free_minor_ida;
>> +	dev->id = ret;
>> +	dev_set_name(&ctrldev->dev, "rpmsg_ctrl%d", ret);
>> +
>> +	ret = cdev_add(&ctrldev->cdev, dev->devt, 1);
>> +	if (ret)
>> +		goto free_ctrl_ida;
>> +
>> +	/* We can now rely on the release function for cleanup */
>> +	dev->release = rpmsg_ctrl_release_device;
>> +
>> +	ret = device_add(dev);
>> +	if (ret) {
>> +		dev_err(&rpdev->dev, "device_add failed: %d\n", ret);
>> +		put_device(dev);
>> +	}
>> +
>> +	dev_set_drvdata(&rpdev->dev, ctrldev);
>> +
>> +	return ret;
>> +
>> +free_ctrl_ida:
>> +	ida_simple_remove(&rpmsg_ctrl_ida, dev->id);
>> +free_minor_ida:
>> +	ida_simple_remove(&rpmsg_minor_ida, MINOR(dev->devt));
>> +free_ctrldev:
>> +	put_device(dev);
>> +	kfree(ctrldev);
>> +
>> +	return ret;
>> +}
>> +
>> +static void rpmsg_ctrl_remove(struct rpmsg_device *rpdev)
>> +{
>> +	struct rpmsg_ctrl *ctrldev = dev_get_drvdata(&rpdev->dev);
>> +	int ret;
>> +
>> +	/* Destroy all endpoints */
>> +	ret = device_for_each_child(&ctrldev->dev, NULL,
>> +				    rpmsg_chrdev_eptdev_destroy);
>> +	if (ret)
>> +		dev_warn(&rpdev->dev, "failed to nuke endpoints: %d\n", ret);
>> +
>> +	device_del(&ctrldev->dev);
>> +	put_device(&ctrldev->dev);
>> +}
>> +
>> +static struct rpmsg_driver rpmsg_ctrl_driver = {
>> +	.probe = rpmsg_ctrl_probe,
>> +	.remove = rpmsg_ctrl_remove,
>> +	.drv = {
>> +		.name = "rpmsg_chrdev",
>> +	},
>> +};
>> +
>> +static int rpmsg_ctrl_init(void)
>> +{
>> +	int ret;
>> +
>> +	ret = alloc_chrdev_region(&rpmsg_major, 0, RPMSG_DEV_MAX, "rpmsg");
>> +	if (ret < 0) {
>> +		pr_err("rpmsg: failed to allocate char dev region\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = register_rpmsg_driver(&rpmsg_ctrl_driver);
>> +	if (ret < 0) {
>> +		pr_err("rpmsg ctrl: failed to register rpmsg driver\n");
>> +		unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
>> +	}
>> +
>> +	return ret;
>> +}
>> +postcore_initcall(rpmsg_ctrl_init);
>> +
>> +static void rpmsg_ctrl_exit(void)
>> +{
>> +	unregister_rpmsg_driver(&rpmsg_ctrl_driver);
>> +	unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
>> +}
>> +module_exit(rpmsg_ctrl_exit);
>> +
>> +MODULE_DESCRIPTION("rpmsg control interface");
>> +MODULE_ALIAS("rpmsg:" KBUILD_MODNAME);
>> +MODULE_LICENSE("GPL v2");
>> -- 
>> 2.17.1
>>

Re: [PATCH v5 06/16] rpmsg: move the rpmsg control device from rpmsg_char to rpmsg_ctrl

[Mathieu Poirier]

On Wed, Mar 03, 2021 at 03:59:34PM +0100, Arnaud POULIQUEN wrote:
> 
> 
> On 3/2/21 7:35 PM, Mathieu Poirier wrote:
> > On Fri, Feb 19, 2021 at 12:14:51PM +0100, Arnaud Pouliquen wrote:
> >> Move the code related to the rpmsg_ctrl char device to the new
> >> rpmsg_ctrl.c module.
> >> Manage the dependency in the kconfig.
> >>
> >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> >> ---
> >>  drivers/rpmsg/Kconfig      |   9 ++
> >>  drivers/rpmsg/Makefile     |   1 +
> >>  drivers/rpmsg/rpmsg_char.c | 163 ----------------------------
> >>  drivers/rpmsg/rpmsg_ctrl.c | 216 +++++++++++++++++++++++++++++++++++++
> >>  4 files changed, 226 insertions(+), 163 deletions(-)
> >>  create mode 100644 drivers/rpmsg/rpmsg_ctrl.c
> >>
> >> diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
> >> index 0b4407abdf13..2d0cd7fdd710 100644
> >> --- a/drivers/rpmsg/Kconfig
> >> +++ b/drivers/rpmsg/Kconfig
> >> @@ -10,11 +10,20 @@ config RPMSG_CHAR
> >>  	tristate "RPMSG device interface"
> >>  	depends on RPMSG
> >>  	depends on NET
> >> +	select RPMSG_CTRL
> >>  	help
> >>  	  Say Y here to export rpmsg endpoints as device files, usually found
> >>  	  in /dev. They make it possible for user-space programs to send and
> >>  	  receive rpmsg packets.
> >>  
> >> +config RPMSG_CTRL
> >> +	tristate "RPMSG control interface"
> >> +	depends on RPMSG
> >> +	help
> >> +	  Say Y here to enable the support of the /dev/rpmsg_ctlX API. This API
> > 
> > s/rpmsg_ctlX/rpmsg_ctrlX
> 
> Good catch!
> 
> > 
> >> +	  allows user-space programs to create endpoints with specific service name,
> >> +	  source and destination addresses.
> >> +
> >>  config RPMSG_NS
> >>  	tristate "RPMSG name service announcement"
> >>  	depends on RPMSG
> >> diff --git a/drivers/rpmsg/Makefile b/drivers/rpmsg/Makefile
> >> index 8d452656f0ee..58e3b382e316 100644
> >> --- a/drivers/rpmsg/Makefile
> >> +++ b/drivers/rpmsg/Makefile
> >> @@ -1,6 +1,7 @@
> >>  # SPDX-License-Identifier: GPL-2.0
> >>  obj-$(CONFIG_RPMSG)		+= rpmsg_core.o
> >>  obj-$(CONFIG_RPMSG_CHAR)	+= rpmsg_char.o
> >> +obj-$(CONFIG_RPMSG_CTRL)	+= rpmsg_ctrl.o
> >>  obj-$(CONFIG_RPMSG_NS)		+= rpmsg_ns.o
> >>  obj-$(CONFIG_RPMSG_MTK_SCP)	+= mtk_rpmsg.o
> >>  qcom_glink-objs			:= qcom_glink_native.o qcom_glink_ssr.o
> >> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> >> index 23e369a00531..83c10b39b139 100644
> >> --- a/drivers/rpmsg/rpmsg_char.c
> >> +++ b/drivers/rpmsg/rpmsg_char.c
> >> @@ -31,28 +31,12 @@
> >>  static dev_t rpmsg_major;
> >>  static struct class *rpmsg_class;
> >>  
> >> -static DEFINE_IDA(rpmsg_ctrl_ida);
> >>  static DEFINE_IDA(rpmsg_ept_ida);
> >>  static DEFINE_IDA(rpmsg_minor_ida);
> >>  
> >>  #define dev_to_eptdev(dev) container_of(dev, struct rpmsg_eptdev, dev)
> >>  #define cdev_to_eptdev(i_cdev) container_of(i_cdev, struct rpmsg_eptdev, cdev)
> >>  
> >> -#define dev_to_ctrldev(dev) container_of(dev, struct rpmsg_ctrldev, dev)
> >> -#define cdev_to_ctrldev(i_cdev) container_of(i_cdev, struct rpmsg_ctrldev, cdev)
> >> -
> >> -/**
> >> - * struct rpmsg_ctrldev - control device for instantiating endpoint devices
> >> - * @rpdev:	underlaying rpmsg device
> >> - * @cdev:	cdev for the ctrl device
> >> - * @dev:	device for the ctrl device
> >> - */
> >> -struct rpmsg_ctrldev {
> >> -	struct rpmsg_device *rpdev;
> >> -	struct cdev cdev;
> >> -	struct device dev;
> >> -};
> > 
> > This showed up in rpmsg_ctrl.c as rpmsg_ctrl.  The same goes for many functions
> > names - they are removed here and re-introduced under a different name, which
> > makes it very hard to follow.  What ends up in the new file should be a carbon
> > copy of what was moved.
> 
> Ok i will split it in 2 steps.

In most cases I don't think there is a need for a wholesale renaming exercise.
To me having a struct rpmsg_ctrldev and functions names such as
rpmsg_ctrldev_open() in a file called rpmsg_ctrl.c is perfectly acceptable.

> 
> Thanks
> Arnaud
> 
> > 
> > I'm out of time for today, more comments tomorrow.
> > 
> > Thanks,
> > Mathieu
> > 
> >> -
> >>  /**
> >>   * struct rpmsg_eptdev - endpoint device context
> >>   * @dev:	endpoint device
> >> @@ -411,145 +395,6 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent
> >>  }
> >>  EXPORT_SYMBOL(rpmsg_chrdev_create_eptdev);
> >>  
> >> -static int rpmsg_ctrldev_open(struct inode *inode, struct file *filp)
> >> -{
> >> -	struct rpmsg_ctrldev *ctrldev = cdev_to_ctrldev(inode->i_cdev);
> >> -
> >> -	get_device(&ctrldev->dev);
> >> -	filp->private_data = ctrldev;
> >> -
> >> -	return 0;
> >> -}
> >> -
> >> -static int rpmsg_ctrldev_release(struct inode *inode, struct file *filp)
> >> -{
> >> -	struct rpmsg_ctrldev *ctrldev = cdev_to_ctrldev(inode->i_cdev);
> >> -
> >> -	put_device(&ctrldev->dev);
> >> -
> >> -	return 0;
> >> -}
> >> -
> >> -static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
> >> -				unsigned long arg)
> >> -{
> >> -	struct rpmsg_ctrldev *ctrldev = fp->private_data;
> >> -	void __user *argp = (void __user *)arg;
> >> -	struct rpmsg_endpoint_info eptinfo;
> >> -	struct rpmsg_channel_info chinfo;
> >> -
> >> -	if (cmd != RPMSG_CREATE_EPT_IOCTL)
> >> -		return -EINVAL;
> >> -
> >> -	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
> >> -		return -EFAULT;
> >> -
> >> -	memcpy(chinfo.name, eptinfo.name, RPMSG_NAME_SIZE);
> >> -	chinfo.name[RPMSG_NAME_SIZE-1] = '\0';
> >> -	chinfo.src = eptinfo.src;
> >> -	chinfo.dst = eptinfo.dst;
> >> -
> >> -	return rpmsg_chrdev_create_eptdev(ctrldev->rpdev, &ctrldev->dev, chinfo);
> >> -};
> >> -
> >> -static const struct file_operations rpmsg_ctrldev_fops = {
> >> -	.owner = THIS_MODULE,
> >> -	.open = rpmsg_ctrldev_open,
> >> -	.release = rpmsg_ctrldev_release,
> >> -	.unlocked_ioctl = rpmsg_ctrldev_ioctl,
> >> -	.compat_ioctl = compat_ptr_ioctl,
> >> -};
> >> -
> >> -static void rpmsg_ctrldev_release_device(struct device *dev)
> >> -{
> >> -	struct rpmsg_ctrldev *ctrldev = dev_to_ctrldev(dev);
> >> -
> >> -	ida_simple_remove(&rpmsg_ctrl_ida, dev->id);
> >> -	ida_simple_remove(&rpmsg_minor_ida, MINOR(dev->devt));
> >> -	cdev_del(&ctrldev->cdev);
> >> -	kfree(ctrldev);
> >> -}
> >> -
> >> -static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
> >> -{
> >> -	struct rpmsg_ctrldev *ctrldev;
> >> -	struct device *dev;
> >> -	int ret;
> >> -
> >> -	ctrldev = kzalloc(sizeof(*ctrldev), GFP_KERNEL);
> >> -	if (!ctrldev)
> >> -		return -ENOMEM;
> >> -
> >> -	ctrldev->rpdev = rpdev;
> >> -
> >> -	dev = &ctrldev->dev;
> >> -	device_initialize(dev);
> >> -	dev->parent = &rpdev->dev;
> >> -
> >> -	cdev_init(&ctrldev->cdev, &rpmsg_ctrldev_fops);
> >> -	ctrldev->cdev.owner = THIS_MODULE;
> >> -
> >> -	ret = ida_simple_get(&rpmsg_minor_ida, 0, RPMSG_DEV_MAX, GFP_KERNEL);
> >> -	if (ret < 0)
> >> -		goto free_ctrldev;
> >> -	dev->devt = MKDEV(MAJOR(rpmsg_major), ret);
> >> -
> >> -	ret = ida_simple_get(&rpmsg_ctrl_ida, 0, 0, GFP_KERNEL);
> >> -	if (ret < 0)
> >> -		goto free_minor_ida;
> >> -	dev->id = ret;
> >> -	dev_set_name(&ctrldev->dev, "rpmsg_ctrl%d", ret);
> >> -
> >> -	ret = cdev_add(&ctrldev->cdev, dev->devt, 1);
> >> -	if (ret)
> >> -		goto free_ctrl_ida;
> >> -
> >> -	/* We can now rely on the release function for cleanup */
> >> -	dev->release = rpmsg_ctrldev_release_device;
> >> -
> >> -	ret = device_add(dev);
> >> -	if (ret) {
> >> -		dev_err(&rpdev->dev, "device_add failed: %d\n", ret);
> >> -		put_device(dev);
> >> -	}
> >> -
> >> -	dev_set_drvdata(&rpdev->dev, ctrldev);
> >> -
> >> -	return ret;
> >> -
> >> -free_ctrl_ida:
> >> -	ida_simple_remove(&rpmsg_ctrl_ida, dev->id);
> >> -free_minor_ida:
> >> -	ida_simple_remove(&rpmsg_minor_ida, MINOR(dev->devt));
> >> -free_ctrldev:
> >> -	put_device(dev);
> >> -	kfree(ctrldev);
> >> -
> >> -	return ret;
> >> -}
> >> -
> >> -static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev)
> >> -{
> >> -	struct rpmsg_ctrldev *ctrldev = dev_get_drvdata(&rpdev->dev);
> >> -	int ret;
> >> -
> >> -	/* Destroy all endpoints */
> >> -	ret = device_for_each_child(&ctrldev->dev, NULL, rpmsg_chrdev_eptdev_destroy);
> >> -	if (ret)
> >> -		dev_warn(&rpdev->dev, "failed to nuke endpoints: %d\n", ret);
> >> -
> >> -	device_del(&ctrldev->dev);
> >> -	put_device(&ctrldev->dev);
> >> -}
> >> -
> >> -static struct rpmsg_driver rpmsg_chrdev_driver = {
> >> -	.probe = rpmsg_chrdev_probe,
> >> -	.remove = rpmsg_chrdev_remove,
> >> -	.drv = {
> >> -		.name = "rpmsg_chrdev",
> >> -	},
> >> -};
> >> -
> >>  static int rpmsg_chrdev_init(void)
> >>  {
> >>  	int ret;
> >> @@ -567,20 +412,12 @@ static int rpmsg_chrdev_init(void)
> >>  		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);
> >> -	}
> >> -
> >>  	return ret;
> >>  }
> >>  postcore_initcall(rpmsg_chrdev_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);
> >>  }
> >> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
> >> new file mode 100644
> >> index 000000000000..fa05b67d24da
> >> --- /dev/null
> >> +++ b/drivers/rpmsg/rpmsg_ctrl.c
> >> @@ -0,0 +1,216 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Copyright (C) 2021, STMicroelectronics
> >> + * Copyright (c) 2016, Linaro Ltd.
> >> + * Copyright (c) 2012, Michal Simek <monstr@monstr.eu>
> >> + * Copyright (c) 2012, PetaLogix
> >> + * Copyright (c) 2011, Texas Instruments, Inc.
> >> + * Copyright (c) 2011, Google, Inc.
> >> + *
> >> + * Based on rpmsg performance statistics driver by Michal Simek, which in turn
> >> + * was based on TI & Google OMX rpmsg driver.
> >> + */
> >> +#include <linux/cdev.h>
> >> +#include <linux/device.h>
> >> +#include <linux/fs.h>
> >> +#include <linux/idr.h>
> >> +#include <linux/kernel.h>
> >> +#include <linux/module.h>
> >> +#include <linux/rpmsg.h>
> >> +#include <linux/slab.h>
> >> +#include <uapi/linux/rpmsg.h>
> >> +
> >> +#include "rpmsg_char.h"
> >> +#include "rpmsg_internal.h"
> >> +
> >> +#define RPMSG_DEV_MAX	(MINORMASK + 1)
> >> +
> >> +static dev_t rpmsg_major;
> >> +
> >> +static DEFINE_IDA(rpmsg_ctrl_ida);
> >> +static DEFINE_IDA(rpmsg_minor_ida);
> >> +
> >> +#define dev_to_ctrldev(dev) container_of(dev, struct rpmsg_ctrl, dev)
> >> +#define cdev_to_ctrldev(i_cdev) container_of(i_cdev, struct rpmsg_ctrl, cdev)
> >> +
> >> +/**
> >> + * struct rpmsg_ctrl - control device for instantiating endpoint devices
> >> + * @rpdev:	underlaying rpmsg device
> >> + * @cdev:	cdev for the ctrl device
> >> + * @dev:	device for the ctrl device
> >> + */
> >> +struct rpmsg_ctrl {
> >> +	struct rpmsg_device *rpdev;
> >> +	struct cdev cdev;
> >> +	struct device dev;
> >> +};
> >> +
> >> +static int rpmsg_ctrl_open(struct inode *inode, struct file *filp)
> >> +{
> >> +	struct rpmsg_ctrl *ctrldev = cdev_to_ctrldev(inode->i_cdev);
> >> +
> >> +	get_device(&ctrldev->dev);
> >> +	filp->private_data = ctrldev;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int rpmsg_ctrl_release(struct inode *inode, struct file *filp)
> >> +{
> >> +	struct rpmsg_ctrl *ctrldev = cdev_to_ctrldev(inode->i_cdev);
> >> +
> >> +	put_device(&ctrldev->dev);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static long rpmsg_ctrl_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
> >> +{
> >> +	struct rpmsg_ctrl *ctrldev = fp->private_data;
> >> +	void __user *argp = (void __user *)arg;
> >> +	struct rpmsg_endpoint_info eptinfo;
> >> +	struct rpmsg_channel_info chinfo;
> >> +
> >> +	if (cmd != RPMSG_CREATE_EPT_IOCTL)
> >> +		return -EINVAL;
> >> +
> >> +	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
> >> +		return -EFAULT;
> >> +
> >> +	memcpy(chinfo.name, eptinfo.name, RPMSG_NAME_SIZE);
> >> +	chinfo.name[RPMSG_NAME_SIZE - 1] = '\0';
> >> +	chinfo.src = eptinfo.src;
> >> +	chinfo.dst = eptinfo.dst;
> >> +
> >> +	return rpmsg_chrdev_create_eptdev(ctrldev->rpdev, &ctrldev->dev, chinfo);
> >> +};
> >> +
> >> +static const struct file_operations rpmsg_ctrl_fops = {
> >> +	.owner = THIS_MODULE,
> >> +	.open = rpmsg_ctrl_open,
> >> +	.release = rpmsg_ctrl_release,
> >> +	.unlocked_ioctl = rpmsg_ctrl_ioctl,
> >> +	.compat_ioctl = compat_ptr_ioctl,
> >> +};
> >> +
> >> +static void rpmsg_ctrl_release_device(struct device *dev)
> >> +{
> >> +	struct rpmsg_ctrl *ctrldev = dev_to_ctrldev(dev);
> >> +
> >> +	ida_simple_remove(&rpmsg_ctrl_ida, dev->id);
> >> +	ida_simple_remove(&rpmsg_minor_ida, MINOR(dev->devt));
> >> +	cdev_del(&ctrldev->cdev);
> >> +	kfree(ctrldev);
> >> +}
> >> +
> >> +static int rpmsg_ctrl_probe(struct rpmsg_device *rpdev)
> >> +{
> >> +	struct rpmsg_ctrl *ctrldev;
> >> +	struct device *dev;
> >> +	int ret;
> >> +
> >> +	ctrldev = kzalloc(sizeof(*ctrldev), GFP_KERNEL);
> >> +	if (!ctrldev)
> >> +		return -ENOMEM;
> >> +
> >> +	ctrldev->rpdev = rpdev;
> >> +
> >> +	dev = &ctrldev->dev;
> >> +	device_initialize(dev);
> >> +	dev->parent = &rpdev->dev;
> >> +
> >> +	cdev_init(&ctrldev->cdev, &rpmsg_ctrl_fops);
> >> +	ctrldev->cdev.owner = THIS_MODULE;
> >> +
> >> +	ret = ida_simple_get(&rpmsg_minor_ida, 0, RPMSG_DEV_MAX, GFP_KERNEL);
> >> +	if (ret < 0)
> >> +		goto free_ctrldev;
> >> +	dev->devt = MKDEV(MAJOR(rpmsg_major), ret);
> >> +
> >> +	ret = ida_simple_get(&rpmsg_ctrl_ida, 0, 0, GFP_KERNEL);
> >> +	if (ret < 0)
> >> +		goto free_minor_ida;
> >> +	dev->id = ret;
> >> +	dev_set_name(&ctrldev->dev, "rpmsg_ctrl%d", ret);
> >> +
> >> +	ret = cdev_add(&ctrldev->cdev, dev->devt, 1);
> >> +	if (ret)
> >> +		goto free_ctrl_ida;
> >> +
> >> +	/* We can now rely on the release function for cleanup */
> >> +	dev->release = rpmsg_ctrl_release_device;
> >> +
> >> +	ret = device_add(dev);
> >> +	if (ret) {
> >> +		dev_err(&rpdev->dev, "device_add failed: %d\n", ret);
> >> +		put_device(dev);
> >> +	}
> >> +
> >> +	dev_set_drvdata(&rpdev->dev, ctrldev);
> >> +
> >> +	return ret;
> >> +
> >> +free_ctrl_ida:
> >> +	ida_simple_remove(&rpmsg_ctrl_ida, dev->id);
> >> +free_minor_ida:
> >> +	ida_simple_remove(&rpmsg_minor_ida, MINOR(dev->devt));
> >> +free_ctrldev:
> >> +	put_device(dev);
> >> +	kfree(ctrldev);
> >> +
> >> +	return ret;
> >> +}
> >> +
> >> +static void rpmsg_ctrl_remove(struct rpmsg_device *rpdev)
> >> +{
> >> +	struct rpmsg_ctrl *ctrldev = dev_get_drvdata(&rpdev->dev);
> >> +	int ret;
> >> +
> >> +	/* Destroy all endpoints */
> >> +	ret = device_for_each_child(&ctrldev->dev, NULL,
> >> +				    rpmsg_chrdev_eptdev_destroy);
> >> +	if (ret)
> >> +		dev_warn(&rpdev->dev, "failed to nuke endpoints: %d\n", ret);
> >> +
> >> +	device_del(&ctrldev->dev);
> >> +	put_device(&ctrldev->dev);
> >> +}
> >> +
> >> +static struct rpmsg_driver rpmsg_ctrl_driver = {
> >> +	.probe = rpmsg_ctrl_probe,
> >> +	.remove = rpmsg_ctrl_remove,
> >> +	.drv = {
> >> +		.name = "rpmsg_chrdev",
> >> +	},
> >> +};
> >> +
> >> +static int rpmsg_ctrl_init(void)
> >> +{
> >> +	int ret;
> >> +
> >> +	ret = alloc_chrdev_region(&rpmsg_major, 0, RPMSG_DEV_MAX, "rpmsg");
> >> +	if (ret < 0) {
> >> +		pr_err("rpmsg: failed to allocate char dev region\n");
> >> +		return ret;
> >> +	}
> >> +
> >> +	ret = register_rpmsg_driver(&rpmsg_ctrl_driver);
> >> +	if (ret < 0) {
> >> +		pr_err("rpmsg ctrl: failed to register rpmsg driver\n");
> >> +		unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
> >> +	}
> >> +
> >> +	return ret;
> >> +}
> >> +postcore_initcall(rpmsg_ctrl_init);
> >> +
> >> +static void rpmsg_ctrl_exit(void)
> >> +{
> >> +	unregister_rpmsg_driver(&rpmsg_ctrl_driver);
> >> +	unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
> >> +}
> >> +module_exit(rpmsg_ctrl_exit);
> >> +
> >> +MODULE_DESCRIPTION("rpmsg control interface");
> >> +MODULE_ALIAS("rpmsg:" KBUILD_MODNAME);
> >> +MODULE_LICENSE("GPL v2");
> >> -- 
> >> 2.17.1
> >>

[PATCH v5 07/16] rpmsg: update rpmsg_chrdev_register_device function

[Arnaud Pouliquen]

As driver is now the rpmsg_ioctl, rename the function.
In addition, initialize the rpdev addresses to RPMSG_ADDR_ANY as not
defined.

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_ctrl.c        |  2 +-
 drivers/rpmsg/rpmsg_internal.h    | 10 ++++++----
 4 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
index 27a05167c18c..d4e4dd482614 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -1625,7 +1625,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_ctrl_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 19903de6268d..40a1c415c775 100644
--- a/drivers/rpmsg/qcom_smd.c
+++ b/drivers/rpmsg/qcom_smd.c
@@ -1097,7 +1097,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_ctrl_register_device(&qsdev->rpdev);
 }
 
 /*
diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
index fa05b67d24da..2e43b4096aa8 100644
--- a/drivers/rpmsg/rpmsg_ctrl.c
+++ b/drivers/rpmsg/rpmsg_ctrl.c
@@ -180,7 +180,7 @@ static struct rpmsg_driver rpmsg_ctrl_driver = {
 	.probe = rpmsg_ctrl_probe,
 	.remove = rpmsg_ctrl_remove,
 	.drv = {
-		.name = "rpmsg_chrdev",
+		.name = KBUILD_MODNAME,
 	},
 };
 
diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
index a76c344253bf..7428f4465d17 100644
--- a/drivers/rpmsg/rpmsg_internal.h
+++ b/drivers/rpmsg/rpmsg_internal.h
@@ -82,16 +82,18 @@ struct rpmsg_device *rpmsg_create_channel(struct rpmsg_device *rpdev,
 int rpmsg_release_channel(struct rpmsg_device *rpdev,
 			  struct rpmsg_channel_info *chinfo);
 /**
- * rpmsg_chrdev_register_device() - register chrdev device based on rpdev
+ * rpmsg_ctrl_register_device() - register a char device for control 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)
+static inline int rpmsg_ctrl_register_device(struct rpmsg_device *rpdev)
 {
-	strcpy(rpdev->id.name, "rpmsg_chrdev");
-	rpdev->driver_override = "rpmsg_chrdev";
+	strcpy(rpdev->id.name, "rpmsg_ctrl");
+	rpdev->driver_override = "rpmsg_ctrl";
+	rpdev->src = RPMSG_ADDR_ANY;
+	rpdev->dst = RPMSG_ADDR_ANY;
 
 	return rpmsg_register_device(rpdev);
 }
-- 
2.17.1

Re: [PATCH v5 07/16] rpmsg: update rpmsg_chrdev_register_device function

[Mathieu Poirier]

On Fri, Feb 19, 2021 at 12:14:52PM +0100, Arnaud Pouliquen wrote:
> As driver is now the rpmsg_ioctl, rename the function.
> In addition, initialize the rpdev addresses to RPMSG_ADDR_ANY as not
> defined.

This patch works but the changelog needs a rework.  The title reflects the
essence of changes but the text of the changelog doesn't.

> 
> 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_ctrl.c        |  2 +-
>  drivers/rpmsg/rpmsg_internal.h    | 10 ++++++----
>  4 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
> index 27a05167c18c..d4e4dd482614 100644
> --- a/drivers/rpmsg/qcom_glink_native.c
> +++ b/drivers/rpmsg/qcom_glink_native.c
> @@ -1625,7 +1625,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_ctrl_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 19903de6268d..40a1c415c775 100644
> --- a/drivers/rpmsg/qcom_smd.c
> +++ b/drivers/rpmsg/qcom_smd.c
> @@ -1097,7 +1097,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_ctrl_register_device(&qsdev->rpdev);
>  }
>  
>  /*
> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
> index fa05b67d24da..2e43b4096aa8 100644
> --- a/drivers/rpmsg/rpmsg_ctrl.c
> +++ b/drivers/rpmsg/rpmsg_ctrl.c
> @@ -180,7 +180,7 @@ static struct rpmsg_driver rpmsg_ctrl_driver = {
>  	.probe = rpmsg_ctrl_probe,
>  	.remove = rpmsg_ctrl_remove,
>  	.drv = {
> -		.name = "rpmsg_chrdev",
> +		.name = KBUILD_MODNAME,
>  	},
>  };
>  
> diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
> index a76c344253bf..7428f4465d17 100644
> --- a/drivers/rpmsg/rpmsg_internal.h
> +++ b/drivers/rpmsg/rpmsg_internal.h
> @@ -82,16 +82,18 @@ struct rpmsg_device *rpmsg_create_channel(struct rpmsg_device *rpdev,
>  int rpmsg_release_channel(struct rpmsg_device *rpdev,
>  			  struct rpmsg_channel_info *chinfo);
>  /**
> - * rpmsg_chrdev_register_device() - register chrdev device based on rpdev
> + * rpmsg_ctrl_register_device() - register a char device for control 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)
> +static inline int rpmsg_ctrl_register_device(struct rpmsg_device *rpdev)
>  {
> -	strcpy(rpdev->id.name, "rpmsg_chrdev");
> -	rpdev->driver_override = "rpmsg_chrdev";
> +	strcpy(rpdev->id.name, "rpmsg_ctrl");
> +	rpdev->driver_override = "rpmsg_ctrl";
> +	rpdev->src = RPMSG_ADDR_ANY;
> +	rpdev->dst = RPMSG_ADDR_ANY;
>  
>  	return rpmsg_register_device(rpdev);
>  }
> -- 
> 2.17.1
> 

[PATCH v5 08/16] rpmsg: glink: add sendto and trysendto ops

[Arnaud Pouliquen]

Implement the sendto ops to support the future rpmsg_char update for the
vitio backend support.
The use of sendto in rpmsg_char is needed as a destination address is
requested at least by the virtio backend.
The glink implementation does not need a destination address so ignores it.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
 drivers/rpmsg/qcom_glink_native.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
index d4e4dd482614..ae2c03b59c55 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -1332,6 +1332,20 @@ static int qcom_glink_trysend(struct rpmsg_endpoint *ept, void *data, int len)
 	return __qcom_glink_send(channel, data, len, false);
 }
 
+static int qcom_glink_sendto(struct rpmsg_endpoint *ept, void *data, int len, u32 dst)
+{
+	struct glink_channel *channel = to_glink_channel(ept);
+
+	return __qcom_glink_send(channel, data, len, true);
+}
+
+static int qcom_glink_trysendto(struct rpmsg_endpoint *ept, void *data, int len, u32 dst)
+{
+	struct glink_channel *channel = to_glink_channel(ept);
+
+	return __qcom_glink_send(channel, data, len, false);
+}
+
 /*
  * Finds the device_node for the glink child interested in this channel.
  */
@@ -1364,7 +1378,9 @@ static const struct rpmsg_device_ops glink_device_ops = {
 static const struct rpmsg_endpoint_ops glink_endpoint_ops = {
 	.destroy_ept = qcom_glink_destroy_ept,
 	.send = qcom_glink_send,
+	.sendto = qcom_glink_sendto,
 	.trysend = qcom_glink_trysend,
+	.trysendto = qcom_glink_trysendto,
 };
 
 static void qcom_glink_rpdev_release(struct device *dev)
-- 
2.17.1

Re: [PATCH v5 08/16] rpmsg: glink: add sendto and trysendto ops

[Mathieu Poirier]

On Fri, Feb 19, 2021 at 12:14:53PM +0100, Arnaud Pouliquen wrote:
> Implement the sendto ops to support the future rpmsg_char update for the
> vitio backend support.

Add a new line, otherwise it is very easy to read.

> The use of sendto in rpmsg_char is needed as a destination address is
> requested at least by the virtio backend.

Same here and throughout the patchset.

> The glink implementation does not need a destination address so ignores it.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
>  drivers/rpmsg/qcom_glink_native.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
> index d4e4dd482614..ae2c03b59c55 100644
> --- a/drivers/rpmsg/qcom_glink_native.c
> +++ b/drivers/rpmsg/qcom_glink_native.c
> @@ -1332,6 +1332,20 @@ static int qcom_glink_trysend(struct rpmsg_endpoint *ept, void *data, int len)
>  	return __qcom_glink_send(channel, data, len, false);
>  }
>  
> +static int qcom_glink_sendto(struct rpmsg_endpoint *ept, void *data, int len, u32 dst)
> +{
> +	struct glink_channel *channel = to_glink_channel(ept);
> +
> +	return __qcom_glink_send(channel, data, len, true);
> +}
> +
> +static int qcom_glink_trysendto(struct rpmsg_endpoint *ept, void *data, int len, u32 dst)
> +{
> +	struct glink_channel *channel = to_glink_channel(ept);
> +
> +	return __qcom_glink_send(channel, data, len, false);
> +}

Just rename send() to sendto() and trysend() to trysendto() and ignore the
destination address.  The same goes for the next patch.  I would fold patch 08
and 09 into 10 to help get the big picture. 

> +
>  /*
>   * Finds the device_node for the glink child interested in this channel.
>   */
> @@ -1364,7 +1378,9 @@ static const struct rpmsg_device_ops glink_device_ops = {
>  static const struct rpmsg_endpoint_ops glink_endpoint_ops = {
>  	.destroy_ept = qcom_glink_destroy_ept,
>  	.send = qcom_glink_send,
> +	.sendto = qcom_glink_sendto,
>  	.trysend = qcom_glink_trysend,
> +	.trysendto = qcom_glink_trysendto,
>  };
>  
>  static void qcom_glink_rpdev_release(struct device *dev)
> -- 
> 2.17.1
> 

Re: [PATCH v5 08/16] rpmsg: glink: add sendto and trysendto ops

[Arnaud POULIQUEN]

Hi Mathieu,

On 3/4/21 8:11 PM, Mathieu Poirier wrote:
> On Fri, Feb 19, 2021 at 12:14:53PM +0100, Arnaud Pouliquen wrote:
>> Implement the sendto ops to support the future rpmsg_char update for the
>> vitio backend support.
> 
> Add a new line, otherwise it is very easy to read.
> 
>> The use of sendto in rpmsg_char is needed as a destination address is
>> requested at least by the virtio backend.
> 
> Same here and throughout the patchset.
> 
>> The glink implementation does not need a destination address so ignores it.
>>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>> ---
>>  drivers/rpmsg/qcom_glink_native.c | 16 ++++++++++++++++
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
>> index d4e4dd482614..ae2c03b59c55 100644
>> --- a/drivers/rpmsg/qcom_glink_native.c
>> +++ b/drivers/rpmsg/qcom_glink_native.c
>> @@ -1332,6 +1332,20 @@ static int qcom_glink_trysend(struct rpmsg_endpoint *ept, void *data, int len)
>>  	return __qcom_glink_send(channel, data, len, false);
>>  }
>>  
>> +static int qcom_glink_sendto(struct rpmsg_endpoint *ept, void *data, int len, u32 dst)
>> +{
>> +	struct glink_channel *channel = to_glink_channel(ept);
>> +
>> +	return __qcom_glink_send(channel, data, len, true);
>> +}
>> +
>> +static int qcom_glink_trysendto(struct rpmsg_endpoint *ept, void *data, int len, u32 dst)
>> +{
>> +	struct glink_channel *channel = to_glink_channel(ept);
>> +
>> +	return __qcom_glink_send(channel, data, len, false);
>> +}
> 
> Just rename send() to sendto() and trysend() to trysendto() and ignore the
> destination address.  

Function prototypes have to match with rpmsg_endpoint_ops structure defined
below. So seems to me not possible to just rename the functions.
Please could you clarify if i missed something?

> The same goes for the next patch.  I would fold patch 08
> and 09 into 10 to help get the big picture.

I'm going to squash all in one.

Thanks,
Arnaud

> 
>> +
>>  /*
>>   * Finds the device_node for the glink child interested in this channel.
>>   */
>> @@ -1364,7 +1378,9 @@ static const struct rpmsg_device_ops glink_device_ops = {
>>  static const struct rpmsg_endpoint_ops glink_endpoint_ops = {
>>  	.destroy_ept = qcom_glink_destroy_ept,
>>  	.send = qcom_glink_send,
>> +	.sendto = qcom_glink_sendto,
>>  	.trysend = qcom_glink_trysend,
>> +	.trysendto = qcom_glink_trysendto,
>>  };
>>  
>>  static void qcom_glink_rpdev_release(struct device *dev)
>> -- 
>> 2.17.1
>>

Re: [PATCH v5 08/16] rpmsg: glink: add sendto and trysendto ops

[Mathieu Poirier]

[...]

> >>  }
> >>  
> >> +static int qcom_glink_sendto(struct rpmsg_endpoint *ept, void *data, int len, u32 dst)
> >> +{
> >> +	struct glink_channel *channel = to_glink_channel(ept);
> >> +
> >> +	return __qcom_glink_send(channel, data, len, true);
> >> +}
> >> +
> >> +static int qcom_glink_trysendto(struct rpmsg_endpoint *ept, void *data, int len, u32 dst)
> >> +{
> >> +	struct glink_channel *channel = to_glink_channel(ept);
> >> +
> >> +	return __qcom_glink_send(channel, data, len, false);
> >> +}
> > 
> > Just rename send() to sendto() and trysend() to trysendto() and ignore the
> > destination address.  
> 

Apologies for not being clear.

> Function prototypes have to match with rpmsg_endpoint_ops structure defined
> below. So seems to me not possible to just rename the functions.
> Please could you clarify if i missed something?

I don't think rproc_ops::send() and rproc_ops::trysend() are used anywhere else.
So replace them with rproc_ops::sendto() and rproc_ops::trysendto() where the
destination address would be ingnored.

> 
> > The same goes for the next patch.  I would fold patch 08
> > and 09 into 10 to help get the big picture.
> 
> I'm going to squash all in one.

Perfect

> 
> Thanks,
> Arnaud
> 
> > 
> >> +
> >>  /*
> >>   * Finds the device_node for the glink child interested in this channel.
> >>   */
> >> @@ -1364,7 +1378,9 @@ static const struct rpmsg_device_ops glink_device_ops = {
> >>  static const struct rpmsg_endpoint_ops glink_endpoint_ops = {
> >>  	.destroy_ept = qcom_glink_destroy_ept,
> >>  	.send = qcom_glink_send,
> >> +	.sendto = qcom_glink_sendto,
> >>  	.trysend = qcom_glink_trysend,
> >> +	.trysendto = qcom_glink_trysendto,
> >>  };
> >>  
> >>  static void qcom_glink_rpdev_release(struct device *dev)
> >> -- 
> >> 2.17.1
> >>

[PATCH v5 09/16] rpmsg: smd: add sendto and trysendto ops

[Arnaud Pouliquen]

Implement the sendto ops to support the future rpmsg_char update for the
vitio backend support.
The use of sendto in rpmsg_char is needed as a destination address is
requested at least by the virtio backend.
The SMD implementation does not need a destination address so ignores it.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
 drivers/rpmsg/qcom_smd.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/rpmsg/qcom_smd.c b/drivers/rpmsg/qcom_smd.c
index 40a1c415c775..2d279c03a090 100644
--- a/drivers/rpmsg/qcom_smd.c
+++ b/drivers/rpmsg/qcom_smd.c
@@ -974,6 +974,20 @@ static int qcom_smd_trysend(struct rpmsg_endpoint *ept, void *data, int len)
 	return __qcom_smd_send(qsept->qsch, data, len, false);
 }
 
+static int qcom_smd_sendto(struct rpmsg_endpoint *ept, void *data, int len, u32 dst)
+{
+	struct qcom_smd_endpoint *qsept = to_smd_endpoint(ept);
+
+	return __qcom_smd_send(qsept->qsch, data, len, true);
+}
+
+static int qcom_smd_trysendto(struct rpmsg_endpoint *ept, void *data, int len, u32 dst)
+{
+	struct qcom_smd_endpoint *qsept = to_smd_endpoint(ept);
+
+	return __qcom_smd_send(qsept->qsch, data, len, false);
+}
+
 static __poll_t qcom_smd_poll(struct rpmsg_endpoint *ept,
 				  struct file *filp, poll_table *wait)
 {
@@ -1038,7 +1052,9 @@ static const struct rpmsg_device_ops qcom_smd_device_ops = {
 static const struct rpmsg_endpoint_ops qcom_smd_endpoint_ops = {
 	.destroy_ept = qcom_smd_destroy_ept,
 	.send = qcom_smd_send,
+	.sendto = qcom_smd_sendto,
 	.trysend = qcom_smd_trysend,
+	.trysendto = qcom_smd_trysendto,
 	.poll = qcom_smd_poll,
 };
 
-- 
2.17.1

[PATCH v5 10/16] rpmsg: char: use sendto to specify the message destination address

[Arnaud Pouliquen]

When the endpoint device is created by the application a destination
address as been specified in the rpmsg_channel_info structure.
Send the message to this address instead of the default one.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
 drivers/rpmsg/rpmsg_char.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index 83c10b39b139..09ae1304837c 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -225,9 +225,9 @@ static ssize_t rpmsg_eptdev_write_iter(struct kiocb *iocb,
 	}
 
 	if (filp->f_flags & O_NONBLOCK)
-		ret = rpmsg_trysend(eptdev->ept, kbuf, len);
+		ret = rpmsg_trysendto(eptdev->ept, kbuf, len, eptdev->chinfo.dst);
 	else
-		ret = rpmsg_send(eptdev->ept, kbuf, len);
+		ret = rpmsg_sendto(eptdev->ept, kbuf, len, eptdev->chinfo.dst);
 
 unlock_eptdev:
 	mutex_unlock(&eptdev->ept_lock);
-- 
2.17.1

[PATCH v5 11/16] rpmsg: virtio: register the rpmsg_ctrl device

[Arnaud Pouliquen]

Instantiate the rpmsg_ioctl device on virtio RPMsg bus creation.
This provides the possibility to expose the RPMSG_CREATE_EPT_IOCTL
to create RPMsg chdev endpoints.

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

---
V5:
Fix compilation issue
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/rpmsg/virtio_rpmsg_bus.c | 57 +++++++++++++++++++++++++++++---
 1 file changed, 52 insertions(+), 5 deletions(-)

diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index e87d4cf926eb..2e6b34084012 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -813,14 +813,52 @@ static void rpmsg_xmit_done(struct virtqueue *svq)
 	wake_up_interruptible(&vrp->sendq);
 }
 
+static struct rpmsg_device *rpmsg_virtio_add_ctrl_dev(struct virtio_device *vdev)
+{
+	struct virtproc_info *vrp = vdev->priv;
+	struct virtio_rpmsg_channel *vch;
+	struct rpmsg_device *rpdev_ctrl;
+	int err = 0;
+
+	vch = kzalloc(sizeof(*vch), GFP_KERNEL);
+	if (!vch)
+		return ERR_PTR(-ENOMEM);
+
+	/* Link the channel to the vrp */
+	vch->vrp = vrp;
+
+	/* Assign public information to the rpmsg_device */
+	rpdev_ctrl = &vch->rpdev;
+	rpdev_ctrl->ops = &virtio_rpmsg_ops;
+
+	rpdev_ctrl->dev.parent = &vrp->vdev->dev;
+	rpdev_ctrl->dev.release = virtio_rpmsg_release_device;
+	rpdev_ctrl->little_endian = virtio_is_little_endian(vrp->vdev);
+
+	err = rpmsg_ctrl_register_device(rpdev_ctrl);
+	if (err) {
+		kfree(vch);
+		return ERR_PTR(err);
+	}
+
+	return rpdev_ctrl;
+}
+
+static void rpmsg_virtio_del_ctrl_dev(struct rpmsg_device *rpdev_ctrl)
+{
+	if (!rpdev_ctrl)
+		return;
+	kfree(to_virtio_rpmsg_channel(rpdev_ctrl));
+}
+
 static int rpmsg_probe(struct virtio_device *vdev)
 {
 	vq_callback_t *vq_cbs[] = { rpmsg_recv_done, rpmsg_xmit_done };
 	static const char * const names[] = { "input", "output" };
 	struct virtqueue *vqs[2];
 	struct virtproc_info *vrp;
-	struct virtio_rpmsg_channel *vch;
-	struct rpmsg_device *rpdev_ns;
+	struct virtio_rpmsg_channel *vch = NULL;
+	struct rpmsg_device *rpdev_ns = NULL, *rpdev_ctrl;
 	void *bufs_va;
 	int err = 0, i;
 	size_t total_buf_space;
@@ -894,12 +932,18 @@ static int rpmsg_probe(struct virtio_device *vdev)
 
 	vdev->priv = vrp;
 
+	rpdev_ctrl = rpmsg_virtio_add_ctrl_dev(vdev);
+	if (IS_ERR(rpdev_ctrl)) {
+		err = PTR_ERR(rpdev_ctrl);
+		goto free_coherent;
+	}
+
 	/* if supported by the remote processor, enable the name service */
 	if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_NS)) {
 		vch = kzalloc(sizeof(*vch), GFP_KERNEL);
 		if (!vch) {
 			err = -ENOMEM;
-			goto free_coherent;
+			goto free_ctrldev;
 		}
 
 		/* Link the channel to our vrp */
@@ -915,7 +959,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
 
 		err = rpmsg_ns_register_device(rpdev_ns);
 		if (err)
-			goto free_coherent;
+			goto free_vch;
 	}
 
 	/*
@@ -939,8 +983,11 @@ static int rpmsg_probe(struct virtio_device *vdev)
 
 	return 0;
 
-free_coherent:
+free_vch:
 	kfree(vch);
+free_ctrldev:
+	rpmsg_virtio_del_ctrl_dev(rpdev_ctrl);
+free_coherent:
 	dma_free_coherent(vdev->dev.parent, total_buf_space,
 			  bufs_va, vrp->bufs_dma);
 vqs_del:
-- 
2.17.1

Re: [PATCH v5 11/16] rpmsg: virtio: register the rpmsg_ctrl device

[Mathieu Poirier]

On Fri, Feb 19, 2021 at 12:14:56PM +0100, Arnaud Pouliquen wrote:
> Instantiate the rpmsg_ioctl device on virtio RPMsg bus creation.

s/rpmsg_ioctl/rpmsg_ctrl

Now I understand what you meant in patch 05.

> This provides the possibility to expose the RPMSG_CREATE_EPT_IOCTL
> to create RPMsg chdev endpoints.

You mean RPMSG device endpoints, i.e rpmsg_eptdev?  If so I think it should be
added to the changelog.  Otherwiser someone could be tempted to look for "chdev"
and find anything but a rpmsg_eptdev.

> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> 
> ---
> V5:
> Fix compilation issue
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>  drivers/rpmsg/virtio_rpmsg_bus.c | 57 +++++++++++++++++++++++++++++---
>  1 file changed, 52 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> index e87d4cf926eb..2e6b34084012 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -813,14 +813,52 @@ static void rpmsg_xmit_done(struct virtqueue *svq)
>  	wake_up_interruptible(&vrp->sendq);
>  }
>  
> +static struct rpmsg_device *rpmsg_virtio_add_ctrl_dev(struct virtio_device *vdev)
> +{
> +	struct virtproc_info *vrp = vdev->priv;
> +	struct virtio_rpmsg_channel *vch;
> +	struct rpmsg_device *rpdev_ctrl;
> +	int err = 0;
> +
> +	vch = kzalloc(sizeof(*vch), GFP_KERNEL);
> +	if (!vch)
> +		return ERR_PTR(-ENOMEM);
> +
> +	/* Link the channel to the vrp */
> +	vch->vrp = vrp;
> +
> +	/* Assign public information to the rpmsg_device */
> +	rpdev_ctrl = &vch->rpdev;
> +	rpdev_ctrl->ops = &virtio_rpmsg_ops;
> +
> +	rpdev_ctrl->dev.parent = &vrp->vdev->dev;
> +	rpdev_ctrl->dev.release = virtio_rpmsg_release_device;
> +	rpdev_ctrl->little_endian = virtio_is_little_endian(vrp->vdev);
> +
> +	err = rpmsg_ctrl_register_device(rpdev_ctrl);
> +	if (err) {
> +		kfree(vch);
> +		return ERR_PTR(err);
> +	}
> +
> +	return rpdev_ctrl;
> +}
> +
> +static void rpmsg_virtio_del_ctrl_dev(struct rpmsg_device *rpdev_ctrl)
> +{
> +	if (!rpdev_ctrl)
> +		return;
> +	kfree(to_virtio_rpmsg_channel(rpdev_ctrl));
> +}
> +
>  static int rpmsg_probe(struct virtio_device *vdev)
>  {
>  	vq_callback_t *vq_cbs[] = { rpmsg_recv_done, rpmsg_xmit_done };
>  	static const char * const names[] = { "input", "output" };
>  	struct virtqueue *vqs[2];
>  	struct virtproc_info *vrp;
> -	struct virtio_rpmsg_channel *vch;
> -	struct rpmsg_device *rpdev_ns;
> +	struct virtio_rpmsg_channel *vch = NULL;
> +	struct rpmsg_device *rpdev_ns = NULL, *rpdev_ctrl;

As far as I can tell @rpdev_ns doesn't have to be initialized.

>  	void *bufs_va;
>  	int err = 0, i;
>  	size_t total_buf_space;
> @@ -894,12 +932,18 @@ static int rpmsg_probe(struct virtio_device *vdev)
>  
>  	vdev->priv = vrp;
>  
> +	rpdev_ctrl = rpmsg_virtio_add_ctrl_dev(vdev);
> +	if (IS_ERR(rpdev_ctrl)) {
> +		err = PTR_ERR(rpdev_ctrl);
> +		goto free_coherent;
> +	}
> +
>  	/* if supported by the remote processor, enable the name service */
>  	if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_NS)) {
>  		vch = kzalloc(sizeof(*vch), GFP_KERNEL);
>  		if (!vch) {
>  			err = -ENOMEM;
> -			goto free_coherent;
> +			goto free_ctrldev;
>  		}
>  
>  		/* Link the channel to our vrp */
> @@ -915,7 +959,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
>  
>  		err = rpmsg_ns_register_device(rpdev_ns);
>  		if (err)
> -			goto free_coherent;
> +			goto free_vch;
>  	}
>  
>  	/*
> @@ -939,8 +983,11 @@ static int rpmsg_probe(struct virtio_device *vdev)
>  
>  	return 0;
>  
> -free_coherent:
> +free_vch:
>  	kfree(vch);
> +free_ctrldev:
> +	rpmsg_virtio_del_ctrl_dev(rpdev_ctrl);
> +free_coherent:
>  	dma_free_coherent(vdev->dev.parent, total_buf_space,
>  			  bufs_va, vrp->bufs_dma);
>  vqs_del:
> -- 
> 2.17.1
> 

Re: [PATCH v5 11/16] rpmsg: virtio: register the rpmsg_ctrl device

[Arnaud POULIQUEN]

On 3/3/21 7:43 PM, Mathieu Poirier wrote:
> On Fri, Feb 19, 2021 at 12:14:56PM +0100, Arnaud Pouliquen wrote:
>> Instantiate the rpmsg_ioctl device on virtio RPMsg bus creation.
> 
> s/rpmsg_ioctl/rpmsg_ctrl
> 
> Now I understand what you meant in patch 05.
> 
>> This provides the possibility to expose the RPMSG_CREATE_EPT_IOCTL
>> to create RPMsg chdev endpoints.
> 
> You mean RPMSG device endpoints, i.e rpmsg_eptdev?  If so I think it should be
> added to the changelog.  Otherwiser someone could be tempted to look for "chdev"
> and find anything but a rpmsg_eptdev.

In fact it is RPMsg char device endpoints, I will add more explicit details in
the changelog.

> 
>>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>>
>> ---
>> V5:
>> Fix compilation issue
>> Reported-by: kernel test robot <lkp@intel.com>
>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>> ---
>>  drivers/rpmsg/virtio_rpmsg_bus.c | 57 +++++++++++++++++++++++++++++---
>>  1 file changed, 52 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
>> index e87d4cf926eb..2e6b34084012 100644
>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>> @@ -813,14 +813,52 @@ static void rpmsg_xmit_done(struct virtqueue *svq)
>>  	wake_up_interruptible(&vrp->sendq);
>>  }
>>  
>> +static struct rpmsg_device *rpmsg_virtio_add_ctrl_dev(struct virtio_device *vdev)
>> +{
>> +	struct virtproc_info *vrp = vdev->priv;
>> +	struct virtio_rpmsg_channel *vch;
>> +	struct rpmsg_device *rpdev_ctrl;
>> +	int err = 0;
>> +
>> +	vch = kzalloc(sizeof(*vch), GFP_KERNEL);
>> +	if (!vch)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	/* Link the channel to the vrp */
>> +	vch->vrp = vrp;
>> +
>> +	/* Assign public information to the rpmsg_device */
>> +	rpdev_ctrl = &vch->rpdev;
>> +	rpdev_ctrl->ops = &virtio_rpmsg_ops;
>> +
>> +	rpdev_ctrl->dev.parent = &vrp->vdev->dev;
>> +	rpdev_ctrl->dev.release = virtio_rpmsg_release_device;
>> +	rpdev_ctrl->little_endian = virtio_is_little_endian(vrp->vdev);
>> +
>> +	err = rpmsg_ctrl_register_device(rpdev_ctrl);
>> +	if (err) {
>> +		kfree(vch);
>> +		return ERR_PTR(err);
>> +	}
>> +
>> +	return rpdev_ctrl;
>> +}
>> +
>> +static void rpmsg_virtio_del_ctrl_dev(struct rpmsg_device *rpdev_ctrl)
>> +{
>> +	if (!rpdev_ctrl)
>> +		return;
>> +	kfree(to_virtio_rpmsg_channel(rpdev_ctrl));
>> +}
>> +
>>  static int rpmsg_probe(struct virtio_device *vdev)
>>  {
>>  	vq_callback_t *vq_cbs[] = { rpmsg_recv_done, rpmsg_xmit_done };
>>  	static const char * const names[] = { "input", "output" };
>>  	struct virtqueue *vqs[2];
>>  	struct virtproc_info *vrp;
>> -	struct virtio_rpmsg_channel *vch;
>> -	struct rpmsg_device *rpdev_ns;
>> +	struct virtio_rpmsg_channel *vch = NULL;
>> +	struct rpmsg_device *rpdev_ns = NULL, *rpdev_ctrl;
> 
> As far as I can tell @rpdev_ns doesn't have to be initialized.

You are right,no more needed in V5 with the error cases restructuring.

Thanks,
Arnaud

> 
>>  	void *bufs_va;
>>  	int err = 0, i;
>>  	size_t total_buf_space;
>> @@ -894,12 +932,18 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>  
>>  	vdev->priv = vrp;
>>  
>> +	rpdev_ctrl = rpmsg_virtio_add_ctrl_dev(vdev);
>> +	if (IS_ERR(rpdev_ctrl)) {
>> +		err = PTR_ERR(rpdev_ctrl);
>> +		goto free_coherent;
>> +	}
>> +
>>  	/* if supported by the remote processor, enable the name service */
>>  	if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_NS)) {
>>  		vch = kzalloc(sizeof(*vch), GFP_KERNEL);
>>  		if (!vch) {
>>  			err = -ENOMEM;
>> -			goto free_coherent;
>> +			goto free_ctrldev;
>>  		}
>>  
>>  		/* Link the channel to our vrp */
>> @@ -915,7 +959,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>  
>>  		err = rpmsg_ns_register_device(rpdev_ns);
>>  		if (err)
>> -			goto free_coherent;
>> +			goto free_vch;
>>  	}
>>  
>>  	/*
>> @@ -939,8 +983,11 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>  
>>  	return 0;
>>  
>> -free_coherent:
>> +free_vch:
>>  	kfree(vch);
>> +free_ctrldev:
>> +	rpmsg_virtio_del_ctrl_dev(rpdev_ctrl);
>> +free_coherent:
>>  	dma_free_coherent(vdev->dev.parent, total_buf_space,
>>  			  bufs_va, vrp->bufs_dma);
>>  vqs_del:
>> -- 
>> 2.17.1
>>

[PATCH v5 12/16] rpmsg: ctrl: introduce RPMSG_CREATE_DEV_IOCTL

[Arnaud Pouliquen]

Implement the RPMSG_CREATE_DEV_IOCTL to allow the user application to
initiate a communication through a new RPMsg channel.
This Ioctl can be used to instantiate a local RPMsg device.
Depending on the back-end implementation, a NS announcement can be sent
to the remote processor.

Suggested-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
 drivers/rpmsg/rpmsg_ctrl.c | 21 +++++++++++++++++----
 include/uapi/linux/rpmsg.h |  5 +++++
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
index 2e43b4096aa8..78c13816bfc6 100644
--- a/drivers/rpmsg/rpmsg_ctrl.c
+++ b/drivers/rpmsg/rpmsg_ctrl.c
@@ -70,9 +70,7 @@ static long rpmsg_ctrl_ioctl(struct file *fp, unsigned int cmd, unsigned long ar
 	void __user *argp = (void __user *)arg;
 	struct rpmsg_endpoint_info eptinfo;
 	struct rpmsg_channel_info chinfo;
-
-	if (cmd != RPMSG_CREATE_EPT_IOCTL)
-		return -EINVAL;
+	struct rpmsg_device *newch;
 
 	if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
 		return -EFAULT;
@@ -82,7 +80,22 @@ static long rpmsg_ctrl_ioctl(struct file *fp, unsigned int cmd, unsigned long ar
 	chinfo.src = eptinfo.src;
 	chinfo.dst = eptinfo.dst;
 
-	return rpmsg_chrdev_create_eptdev(ctrldev->rpdev, &ctrldev->dev, chinfo);
+	switch (cmd) {
+	case RPMSG_CREATE_EPT_IOCTL:
+		return rpmsg_chrdev_create_eptdev(ctrldev->rpdev, &ctrldev->dev, chinfo);
+
+	case RPMSG_CREATE_DEV_IOCTL:
+		newch = rpmsg_create_channel(ctrldev->rpdev, &chinfo);
+		if (!newch) {
+			dev_err(&ctrldev->dev, "rpmsg_create_channel failed\n");
+			return -ENXIO;
+		}
+		return 0;
+
+	default:
+		return -EINVAL;
+	}
+
 };
 
 static const struct file_operations rpmsg_ctrl_fops = {
diff --git a/include/uapi/linux/rpmsg.h b/include/uapi/linux/rpmsg.h
index f5ca8740f3fb..f9d5a74e7801 100644
--- a/include/uapi/linux/rpmsg.h
+++ b/include/uapi/linux/rpmsg.h
@@ -33,4 +33,9 @@ struct rpmsg_endpoint_info {
  */
 #define RPMSG_DESTROY_EPT_IOCTL	_IO(0xb5, 0x2)
 
+/**
+ * Instantiate a rpmsg service device.
+ */
+#define RPMSG_CREATE_DEV_IOCTL	_IOW(0xb5, 0x3, struct rpmsg_endpoint_info)
+
 #endif
-- 
2.17.1

[PATCH v5 13/16] rpmsg: char: introduce __rpmsg_chrdev_create_eptdev function

[Arnaud Pouliquen]

Introduce the __rpmsg_chrdev_create_eptdev internal function that returns
the rpmsg_eptdev context structure.
This patch prepares the introduction of a RPMsg device for the
char device. the RPMsg device will need a reference to the context.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
 drivers/rpmsg/rpmsg_char.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index 09ae1304837c..66dcb8845d6c 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -328,8 +328,9 @@ int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data)
 }
 EXPORT_SYMBOL(rpmsg_chrdev_eptdev_destroy);
 
-int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent,
-			       struct rpmsg_channel_info chinfo)
+static struct rpmsg_eptdev *__rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev,
+							 struct device *parent,
+							 struct rpmsg_channel_info chinfo)
 {
 	struct rpmsg_eptdev *eptdev;
 	struct device *dev;
@@ -337,7 +338,7 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent
 
 	eptdev = kzalloc(sizeof(*eptdev), GFP_KERNEL);
 	if (!eptdev)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
 	dev = &eptdev->dev;
 	eptdev->rpdev = rpdev;
@@ -381,7 +382,7 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent
 		put_device(dev);
 	}
 
-	return ret;
+	return eptdev;
 
 free_ept_ida:
 	ida_simple_remove(&rpmsg_ept_ida, dev->id);
@@ -391,7 +392,19 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent
 	put_device(dev);
 	kfree(eptdev);
 
-	return ret;
+	return ERR_PTR(ret);
+}
+
+int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent,
+			       struct rpmsg_channel_info chinfo)
+{
+	struct rpmsg_eptdev *eptdev;
+
+	eptdev = __rpmsg_chrdev_create_eptdev(rpdev, &rpdev->dev, chinfo);
+	if (IS_ERR(eptdev))
+		return PTR_ERR(eptdev);
+
+	return 0;
 }
 EXPORT_SYMBOL(rpmsg_chrdev_create_eptdev);
 
-- 
2.17.1

Re: [PATCH v5 13/16] rpmsg: char: introduce __rpmsg_chrdev_create_eptdev function

[Mathieu Poirier]

On Fri, Feb 19, 2021 at 12:14:58PM +0100, Arnaud Pouliquen wrote:
> Introduce the __rpmsg_chrdev_create_eptdev internal function that returns
> the rpmsg_eptdev context structure.
> This patch prepares the introduction of a RPMsg device for the
> char device. the RPMsg device will need a reference to the context.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
>  drivers/rpmsg/rpmsg_char.c | 23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> index 09ae1304837c..66dcb8845d6c 100644
> --- a/drivers/rpmsg/rpmsg_char.c
> +++ b/drivers/rpmsg/rpmsg_char.c
> @@ -328,8 +328,9 @@ int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data)
>  }
>  EXPORT_SYMBOL(rpmsg_chrdev_eptdev_destroy);
>  
> -int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent,
> -			       struct rpmsg_channel_info chinfo)
> +static struct rpmsg_eptdev *__rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev,
> +							 struct device *parent,
> +							 struct rpmsg_channel_info chinfo)
>  {
>  	struct rpmsg_eptdev *eptdev;
>  	struct device *dev;
> @@ -337,7 +338,7 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent
>  
>  	eptdev = kzalloc(sizeof(*eptdev), GFP_KERNEL);
>  	if (!eptdev)
> -		return -ENOMEM;
> +		return ERR_PTR(-ENOMEM);
>  
>  	dev = &eptdev->dev;
>  	eptdev->rpdev = rpdev;
> @@ -381,7 +382,7 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent
>  		put_device(dev);
>  	}
>  
> -	return ret;
> +	return eptdev;
>  
>  free_ept_ida:
>  	ida_simple_remove(&rpmsg_ept_ida, dev->id);
> @@ -391,7 +392,19 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent
>  	put_device(dev);
>  	kfree(eptdev);
>  
> -	return ret;
> +	return ERR_PTR(ret);
> +}
> +
> +int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent,
> +			       struct rpmsg_channel_info chinfo)
> +{
> +	struct rpmsg_eptdev *eptdev;
> +
> +	eptdev = __rpmsg_chrdev_create_eptdev(rpdev, &rpdev->dev, chinfo);

Shouldn't the second argument to __rpmsg_chrdev_create_eptdev() be @parent?

> +	if (IS_ERR(eptdev))
> +		return PTR_ERR(eptdev);
> +
> +	return 0;
>  }
>  EXPORT_SYMBOL(rpmsg_chrdev_create_eptdev);
>  
> -- 
> 2.17.1
> 

Re: [PATCH v5 13/16] rpmsg: char: introduce __rpmsg_chrdev_create_eptdev function

[Arnaud POULIQUEN]

On 3/4/21 7:55 PM, Mathieu Poirier wrote:
> On Fri, Feb 19, 2021 at 12:14:58PM +0100, Arnaud Pouliquen wrote:
>> Introduce the __rpmsg_chrdev_create_eptdev internal function that returns
>> the rpmsg_eptdev context structure.
>> This patch prepares the introduction of a RPMsg device for the
>> char device. the RPMsg device will need a reference to the context.
>>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>> ---
>>  drivers/rpmsg/rpmsg_char.c | 23 ++++++++++++++++++-----
>>  1 file changed, 18 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
>> index 09ae1304837c..66dcb8845d6c 100644
>> --- a/drivers/rpmsg/rpmsg_char.c
>> +++ b/drivers/rpmsg/rpmsg_char.c
>> @@ -328,8 +328,9 @@ int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data)
>>  }
>>  EXPORT_SYMBOL(rpmsg_chrdev_eptdev_destroy);
>>  
>> -int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent,
>> -			       struct rpmsg_channel_info chinfo)
>> +static struct rpmsg_eptdev *__rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev,
>> +							 struct device *parent,
>> +							 struct rpmsg_channel_info chinfo)
>>  {
>>  	struct rpmsg_eptdev *eptdev;
>>  	struct device *dev;
>> @@ -337,7 +338,7 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent
>>  
>>  	eptdev = kzalloc(sizeof(*eptdev), GFP_KERNEL);
>>  	if (!eptdev)
>> -		return -ENOMEM;
>> +		return ERR_PTR(-ENOMEM);
>>  
>>  	dev = &eptdev->dev;
>>  	eptdev->rpdev = rpdev;
>> @@ -381,7 +382,7 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent
>>  		put_device(dev);
>>  	}
>>  
>> -	return ret;
>> +	return eptdev;
>>  
>>  free_ept_ida:
>>  	ida_simple_remove(&rpmsg_ept_ida, dev->id);
>> @@ -391,7 +392,19 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent
>>  	put_device(dev);
>>  	kfree(eptdev);
>>  
>> -	return ret;
>> +	return ERR_PTR(ret);
>> +}
>> +
>> +int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent,
>> +			       struct rpmsg_channel_info chinfo)
>> +{
>> +	struct rpmsg_eptdev *eptdev;
>> +
>> +	eptdev = __rpmsg_chrdev_create_eptdev(rpdev, &rpdev->dev, chinfo);
> 
> Shouldn't the second argument to __rpmsg_chrdev_create_eptdev() be @parent?

This keep the legacy hierarchy:
https://elixir.bootlin.com/linux/latest/source/drivers/rpmsg/rpmsg_char.c#L362

Thanks,
Arnaud


> 
>> +	if (IS_ERR(eptdev))
>> +		return PTR_ERR(eptdev);
>> +
>> +	return 0;
>>  }
>>  EXPORT_SYMBOL(rpmsg_chrdev_create_eptdev);
>>  
>> -- 
>> 2.17.1
>>

Re: [PATCH v5 13/16] rpmsg: char: introduce __rpmsg_chrdev_create_eptdev function

[Mathieu Poirier]

On Fri, Mar 05, 2021 at 11:46:47AM +0100, Arnaud POULIQUEN wrote:
> 
> 
> On 3/4/21 7:55 PM, Mathieu Poirier wrote:
> > On Fri, Feb 19, 2021 at 12:14:58PM +0100, Arnaud Pouliquen wrote:
> >> Introduce the __rpmsg_chrdev_create_eptdev internal function that returns
> >> the rpmsg_eptdev context structure.
> >> This patch prepares the introduction of a RPMsg device for the
> >> char device. the RPMsg device will need a reference to the context.
> >>
> >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> >> ---
> >>  drivers/rpmsg/rpmsg_char.c | 23 ++++++++++++++++++-----
> >>  1 file changed, 18 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> >> index 09ae1304837c..66dcb8845d6c 100644
> >> --- a/drivers/rpmsg/rpmsg_char.c
> >> +++ b/drivers/rpmsg/rpmsg_char.c
> >> @@ -328,8 +328,9 @@ int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data)
> >>  }
> >>  EXPORT_SYMBOL(rpmsg_chrdev_eptdev_destroy);
> >>  
> >> -int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent,
> >> -			       struct rpmsg_channel_info chinfo)
> >> +static struct rpmsg_eptdev *__rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev,
> >> +							 struct device *parent,
> >> +							 struct rpmsg_channel_info chinfo)
> >>  {
> >>  	struct rpmsg_eptdev *eptdev;
> >>  	struct device *dev;
> >> @@ -337,7 +338,7 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent
> >>  
> >>  	eptdev = kzalloc(sizeof(*eptdev), GFP_KERNEL);
> >>  	if (!eptdev)
> >> -		return -ENOMEM;
> >> +		return ERR_PTR(-ENOMEM);
> >>  
> >>  	dev = &eptdev->dev;
> >>  	eptdev->rpdev = rpdev;
> >> @@ -381,7 +382,7 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent
> >>  		put_device(dev);
> >>  	}
> >>  
> >> -	return ret;
> >> +	return eptdev;
> >>  
> >>  free_ept_ida:
> >>  	ida_simple_remove(&rpmsg_ept_ida, dev->id);
> >> @@ -391,7 +392,19 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent
> >>  	put_device(dev);
> >>  	kfree(eptdev);
> >>  
> >> -	return ret;
> >> +	return ERR_PTR(ret);
> >> +}
> >> +
> >> +int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent,
> >> +			       struct rpmsg_channel_info chinfo)
> >> +{
> >> +	struct rpmsg_eptdev *eptdev;
> >> +
> >> +	eptdev = __rpmsg_chrdev_create_eptdev(rpdev, &rpdev->dev, chinfo);
> > 
> > Shouldn't the second argument to __rpmsg_chrdev_create_eptdev() be @parent?
> 
> This keep the legacy hierarchy:
> https://elixir.bootlin.com/linux/latest/source/drivers/rpmsg/rpmsg_char.c#L362

In patch 12 it is clear the first and second arguments are ctrldev->rpdev and
ctrldev->dev.  In this set the second arguments becomes rpdev->dev, which is
different than ctrldev->dev.  Goind back to rpmsg_ctrl_probe() we have:

        dev = &ctrldev->dev;
        device_initialize(dev);
        dev->parent = &rpdev->dev;

As such in __rpmsg_chrdev_create_eptdev(), eptdev->dev->parent becomes
ctrldev->dev->parent rather than ctrldev->dev.

> 
> Thanks,
> Arnaud
> 
> 
> > 
> >> +	if (IS_ERR(eptdev))
> >> +		return PTR_ERR(eptdev);
> >> +
> >> +	return 0;
> >>  }
> >>  EXPORT_SYMBOL(rpmsg_chrdev_create_eptdev);
> >>  
> >> -- 
> >> 2.17.1
> >>

Re: [PATCH v5 13/16] rpmsg: char: introduce __rpmsg_chrdev_create_eptdev function

[Mathieu Poirier]

On Fri, Feb 19, 2021 at 12:14:58PM +0100, Arnaud Pouliquen wrote:
> Introduce the __rpmsg_chrdev_create_eptdev internal function that returns
> the rpmsg_eptdev context structure.

Add newlines between paragraphs.

> This patch prepares the introduction of a RPMsg device for the
> char device. the RPMsg device will need a reference to the context.

s/the/The

s/RPMsg/RPMSG - throughout the patchset.

As a general note please be mindful of patch changelogs.  I often find myself
having to decipher the ideas being conveyed.

I am done reviewing this set.  There are things I will want to come back to but
the general goals behind the patchset are being achieved.

Thanks,
Mathieu

> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
>  drivers/rpmsg/rpmsg_char.c | 23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> index 09ae1304837c..66dcb8845d6c 100644
> --- a/drivers/rpmsg/rpmsg_char.c
> +++ b/drivers/rpmsg/rpmsg_char.c
> @@ -328,8 +328,9 @@ int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data)
>  }
>  EXPORT_SYMBOL(rpmsg_chrdev_eptdev_destroy);
>  
> -int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent,
> -			       struct rpmsg_channel_info chinfo)
> +static struct rpmsg_eptdev *__rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev,
> +							 struct device *parent,
> +							 struct rpmsg_channel_info chinfo)
>  {
>  	struct rpmsg_eptdev *eptdev;
>  	struct device *dev;
> @@ -337,7 +338,7 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent
>  
>  	eptdev = kzalloc(sizeof(*eptdev), GFP_KERNEL);
>  	if (!eptdev)
> -		return -ENOMEM;
> +		return ERR_PTR(-ENOMEM);
>  
>  	dev = &eptdev->dev;
>  	eptdev->rpdev = rpdev;
> @@ -381,7 +382,7 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent
>  		put_device(dev);
>  	}
>  
> -	return ret;
> +	return eptdev;
>  
>  free_ept_ida:
>  	ida_simple_remove(&rpmsg_ept_ida, dev->id);
> @@ -391,7 +392,19 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent
>  	put_device(dev);
>  	kfree(eptdev);
>  
> -	return ret;
> +	return ERR_PTR(ret);
> +}
> +
> +int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent,
> +			       struct rpmsg_channel_info chinfo)
> +{
> +	struct rpmsg_eptdev *eptdev;
> +
> +	eptdev = __rpmsg_chrdev_create_eptdev(rpdev, &rpdev->dev, chinfo);
> +	if (IS_ERR(eptdev))
> +		return PTR_ERR(eptdev);
> +
> +	return 0;
>  }
>  EXPORT_SYMBOL(rpmsg_chrdev_create_eptdev);
>  
> -- 
> 2.17.1
> 

Re: [PATCH v5 13/16] rpmsg: char: introduce __rpmsg_chrdev_create_eptdev function

[Arnaud POULIQUEN]

Hi Mathieu

On 3/4/21 8:05 PM, Mathieu Poirier wrote:
> On Fri, Feb 19, 2021 at 12:14:58PM +0100, Arnaud Pouliquen wrote:
>> Introduce the __rpmsg_chrdev_create_eptdev internal function that returns
>> the rpmsg_eptdev context structure.
> 
> Add newlines between paragraphs.
> 
>> This patch prepares the introduction of a RPMsg device for the
>> char device. the RPMsg device will need a reference to the context.
> 
> s/the/The
> 
> s/RPMsg/RPMSG - throughout the patchset.
> 
> As a general note please be mindful of patch changelogs.  I often find myself
> having to decipher the ideas being conveyed.

Sure, i will rewrite changelogs and comments to make them more explicit.

> 
> I am done reviewing this set.  There are things I will want to come back to but
> the general goals behind the patchset are being achieved.

Thanks for the review! So I'm going to move forward with this approach.

For the next revision I would propose, to simplify the review, to remove patches
related to the RPMSG_CREATE_DEV_IOCTL.

Thanks,
Arnaud

> 
> Thanks,
> Mathieu
> 
>>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>> ---
>>  drivers/rpmsg/rpmsg_char.c | 23 ++++++++++++++++++-----
>>  1 file changed, 18 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
>> index 09ae1304837c..66dcb8845d6c 100644
>> --- a/drivers/rpmsg/rpmsg_char.c
>> +++ b/drivers/rpmsg/rpmsg_char.c
>> @@ -328,8 +328,9 @@ int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data)
>>  }
>>  EXPORT_SYMBOL(rpmsg_chrdev_eptdev_destroy);
>>  
>> -int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent,
>> -			       struct rpmsg_channel_info chinfo)
>> +static struct rpmsg_eptdev *__rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev,
>> +							 struct device *parent,
>> +							 struct rpmsg_channel_info chinfo)
>>  {
>>  	struct rpmsg_eptdev *eptdev;
>>  	struct device *dev;
>> @@ -337,7 +338,7 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent
>>  
>>  	eptdev = kzalloc(sizeof(*eptdev), GFP_KERNEL);
>>  	if (!eptdev)
>> -		return -ENOMEM;
>> +		return ERR_PTR(-ENOMEM);
>>  
>>  	dev = &eptdev->dev;
>>  	eptdev->rpdev = rpdev;
>> @@ -381,7 +382,7 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent
>>  		put_device(dev);
>>  	}
>>  
>> -	return ret;
>> +	return eptdev;
>>  
>>  free_ept_ida:
>>  	ida_simple_remove(&rpmsg_ept_ida, dev->id);
>> @@ -391,7 +392,19 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent
>>  	put_device(dev);
>>  	kfree(eptdev);
>>  
>> -	return ret;
>> +	return ERR_PTR(ret);
>> +}
>> +
>> +int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent,
>> +			       struct rpmsg_channel_info chinfo)
>> +{
>> +	struct rpmsg_eptdev *eptdev;
>> +
>> +	eptdev = __rpmsg_chrdev_create_eptdev(rpdev, &rpdev->dev, chinfo);
>> +	if (IS_ERR(eptdev))
>> +		return PTR_ERR(eptdev);
>> +
>> +	return 0;
>>  }
>>  EXPORT_SYMBOL(rpmsg_chrdev_create_eptdev);
>>  
>> -- 
>> 2.17.1
>>

[PATCH v5 14/16] rpmsg: char: introduce a RPMsg driver for the RPMsg char device

[Arnaud Pouliquen]

A RPMsg char device allows to probe the endpoint device on a remote name
service announcement. With this patch the /dev/rpmsgX interface is created
either by a user application or by the remote firmware.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
 drivers/rpmsg/rpmsg_char.c | 65 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 63 insertions(+), 2 deletions(-)

diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index 66dcb8845d6c..c98b0e69679b 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -28,6 +28,8 @@
 
 #define RPMSG_DEV_MAX	(MINORMASK + 1)
 
+#define RPMSG_CHAR_DEVNAME "rpmsg-raw"
+
 static dev_t rpmsg_major;
 static struct class *rpmsg_class;
 
@@ -408,6 +410,52 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent
 }
 EXPORT_SYMBOL(rpmsg_chrdev_create_eptdev);
 
+static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
+{
+	struct rpmsg_channel_info chinfo;
+	struct rpmsg_eptdev *eptdev;
+
+	if (!rpdev->ept)
+		return -EINVAL;
+
+	memcpy(chinfo.name, RPMSG_CHAR_DEVNAME, sizeof(RPMSG_CHAR_DEVNAME));
+	chinfo.src = rpdev->src;
+	chinfo.dst = rpdev->dst;
+
+	eptdev = __rpmsg_chrdev_create_eptdev(rpdev, &rpdev->dev, chinfo);
+	if (IS_ERR(eptdev))
+		return PTR_ERR(eptdev);
+
+	/* Set the private field of the default endpoint to retrieve context on callback. */
+	rpdev->ept->priv = eptdev;
+
+	return 0;
+}
+
+static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev)
+{
+	int ret;
+
+	ret = device_for_each_child(&rpdev->dev, NULL, rpmsg_chrdev_eptdev_destroy);
+	if (ret)
+		dev_warn(&rpdev->dev, "failed to destroy endpoints: %d\n", ret);
+}
+
+static struct rpmsg_device_id rpmsg_chrdev_id_table[] = {
+	{ .name	= RPMSG_CHAR_DEVNAME },
+	{ },
+};
+
+static struct rpmsg_driver rpmsg_chrdev_driver = {
+	.probe = rpmsg_chrdev_probe,
+	.remove = rpmsg_chrdev_remove,
+	.id_table = rpmsg_chrdev_id_table,
+	.callback = rpmsg_ept_cb,
+	.drv = {
+		.name = "rpmsg_chrdev",
+	},
+};
+
 static int rpmsg_chrdev_init(void)
 {
 	int ret;
@@ -421,10 +469,23 @@ static int rpmsg_chrdev_init(void)
 	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 = PTR_ERR(rpmsg_class);
+		goto free_region;
 	}
 
+	ret = register_rpmsg_driver(&rpmsg_chrdev_driver);
+	if (ret < 0) {
+		pr_err("rpmsg: failed to register rpmsg raw driver\n");
+		goto free_class;
+	}
+
+	return 0;
+
+free_class:
+	class_destroy(rpmsg_class);
+free_region:
+	unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
+
 	return ret;
 }
 postcore_initcall(rpmsg_chrdev_init);
-- 
2.17.1

[PATCH v5 15/16] rpmsg: char: no dynamic endpoint management for the default one

[Arnaud Pouliquen]

Do not dynamically manage the default endpoint. The ept address must
not change.
This update is needed to manage the RPMSG_CREATE_DEV_IOCTL. In this
case a default endpoint is used and it's address must not change or
been reused by another service.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
 drivers/rpmsg/rpmsg_char.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index c98b0e69679b..8d3f9d6c20ad 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -114,14 +114,23 @@ static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
 	struct rpmsg_endpoint *ept;
 	struct rpmsg_device *rpdev = eptdev->rpdev;
 	struct device *dev = &eptdev->dev;
+	u32 addr = eptdev->chinfo.src;
 
 	get_device(dev);
 
-	ept = rpmsg_create_ept(rpdev, rpmsg_ept_cb, eptdev, eptdev->chinfo);
-	if (!ept) {
-		dev_err(dev, "failed to open %s\n", eptdev->chinfo.name);
-		put_device(dev);
-		return -EINVAL;
+	/*
+	 * The RPMsg device can has been created by a ns announcement. In this
+	 * case a default endpoint has been created. Reuse it to avoid to manage
+	 * a new address on each open close.
+	 */
+	ept = rpdev->ept;
+	if (!ept || addr != ept->addr) {
+		ept = rpmsg_create_ept(rpdev, rpmsg_ept_cb, eptdev, eptdev->chinfo);
+		if (!ept) {
+			dev_err(dev, "failed to open %s\n", eptdev->chinfo.name);
+			put_device(dev);
+			return -EINVAL;
+		}
 	}
 
 	eptdev->ept = ept;
@@ -133,12 +142,17 @@ static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
 static int rpmsg_eptdev_release(struct inode *inode, struct file *filp)
 {
 	struct rpmsg_eptdev *eptdev = cdev_to_eptdev(inode->i_cdev);
+	struct rpmsg_device *rpdev = eptdev->rpdev;
 	struct device *dev = &eptdev->dev;
 
-	/* Close the endpoint, if it's not already destroyed by the parent */
+	/*
+	 * Close the endpoint, if it's not already destroyed by the parent and it is not the
+	 * default one.
+	 */
 	mutex_lock(&eptdev->ept_lock);
 	if (eptdev->ept) {
-		rpmsg_destroy_ept(eptdev->ept);
+		if (eptdev->ept != rpdev->ept)
+			rpmsg_destroy_ept(eptdev->ept);
 		eptdev->ept = NULL;
 	}
 	mutex_unlock(&eptdev->ept_lock);
-- 
2.17.1

Re: [PATCH v5 15/16] rpmsg: char: no dynamic endpoint management for the default one

[Mathieu Poirier]

There has to be a capital letter at the start of the title:

rpmsg: char: No dynamic endpoint management for the default one

Please fix for all the patches.

On Fri, Feb 19, 2021 at 12:15:00PM +0100, Arnaud Pouliquen wrote:
> Do not dynamically manage the default endpoint. The ept address must
> not change.
> This update is needed to manage the RPMSG_CREATE_DEV_IOCTL. In this
> case a default endpoint is used and it's address must not change or
> been reused by another service.

The above is very difficult to understand.  I am not sure about introducing
RPMSG_CREATE_DEV_IOCTL in this patchset.  More on that in an upcoming comment.

> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
>  drivers/rpmsg/rpmsg_char.c | 28 +++++++++++++++++++++-------
>  1 file changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> index c98b0e69679b..8d3f9d6c20ad 100644
> --- a/drivers/rpmsg/rpmsg_char.c
> +++ b/drivers/rpmsg/rpmsg_char.c
> @@ -114,14 +114,23 @@ static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
>  	struct rpmsg_endpoint *ept;
>  	struct rpmsg_device *rpdev = eptdev->rpdev;
>  	struct device *dev = &eptdev->dev;
> +	u32 addr = eptdev->chinfo.src;
>  
>  	get_device(dev);
>  
> -	ept = rpmsg_create_ept(rpdev, rpmsg_ept_cb, eptdev, eptdev->chinfo);
> -	if (!ept) {
> -		dev_err(dev, "failed to open %s\n", eptdev->chinfo.name);
> -		put_device(dev);
> -		return -EINVAL;
> +	/*
> +	 * The RPMsg device can has been created by a ns announcement. In this
> +	 * case a default endpoint has been created. Reuse it to avoid to manage
> +	 * a new address on each open close.
> +	 */

Here too it is very difficult to understand because the comment
doesn't not describe what the code does.  The code creates an enpoint if it
has not been created, which means /dev/rpmsgX was created from the ioctl. 

> +	ept = rpdev->ept;
> +	if (!ept || addr != ept->addr) {
> +		ept = rpmsg_create_ept(rpdev, rpmsg_ept_cb, eptdev, eptdev->chinfo);
> +		if (!ept) {
> +			dev_err(dev, "failed to open %s\n", eptdev->chinfo.name);
> +			put_device(dev);
> +			return -EINVAL;
> +		}
>  	}
>  
>  	eptdev->ept = ept;
> @@ -133,12 +142,17 @@ static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
>  static int rpmsg_eptdev_release(struct inode *inode, struct file *filp)
>  {
>  	struct rpmsg_eptdev *eptdev = cdev_to_eptdev(inode->i_cdev);
> +	struct rpmsg_device *rpdev = eptdev->rpdev;
>  	struct device *dev = &eptdev->dev;
>  
> -	/* Close the endpoint, if it's not already destroyed by the parent */
> +	/*
> +	 * Close the endpoint, if it's not already destroyed by the parent and it is not the
> +	 * default one.
> +	 */
>  	mutex_lock(&eptdev->ept_lock);
>  	if (eptdev->ept) {
> -		rpmsg_destroy_ept(eptdev->ept);
> +		if (eptdev->ept != rpdev->ept)
> +			rpmsg_destroy_ept(eptdev->ept);
>  		eptdev->ept = NULL;
>  	}
>  	mutex_unlock(&eptdev->ept_lock);
> -- 
> 2.17.1
> 

Re: [PATCH v5 15/16] rpmsg: char: no dynamic endpoint management for the default one

[Arnaud POULIQUEN]

On 3/4/21 7:40 PM, Mathieu Poirier wrote:
> There has to be a capital letter at the start of the title:
> 
> rpmsg: char: No dynamic endpoint management for the default one
> 
> Please fix for all the patches.

Ok, I will update the subjects with capital letter in my next revision.

Just for my information, is it a new rule? kernel documentation [1] gives a
canonical subject and an example without capital letter.

[1]
https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#the-canonical-patch-format

> 
> On Fri, Feb 19, 2021 at 12:15:00PM +0100, Arnaud Pouliquen wrote:
>> Do not dynamically manage the default endpoint. The ept address must
>> not change.
>> This update is needed to manage the RPMSG_CREATE_DEV_IOCTL. In this
>> case a default endpoint is used and it's address must not change or
>> been reused by another service.
> 
> The above is very difficult to understand.  I am not sure about introducing
> RPMSG_CREATE_DEV_IOCTL in this patchset.  More on that in an upcoming comment.

The purpose of this revision was mainly to provide a view of what we could do to
provide a more generic control interface.

To simplify the review I can remove the RPMSG_CREATE_DEV_IOCTL management and
send it as a next step, in a separate patchset.

> 
>>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>> ---
>>  drivers/rpmsg/rpmsg_char.c | 28 +++++++++++++++++++++-------
>>  1 file changed, 21 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
>> index c98b0e69679b..8d3f9d6c20ad 100644
>> --- a/drivers/rpmsg/rpmsg_char.c
>> +++ b/drivers/rpmsg/rpmsg_char.c
>> @@ -114,14 +114,23 @@ static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
>>  	struct rpmsg_endpoint *ept;
>>  	struct rpmsg_device *rpdev = eptdev->rpdev;
>>  	struct device *dev = &eptdev->dev;
>> +	u32 addr = eptdev->chinfo.src;
>>  
>>  	get_device(dev);
>>  
>> -	ept = rpmsg_create_ept(rpdev, rpmsg_ept_cb, eptdev, eptdev->chinfo);
>> -	if (!ept) {
>> -		dev_err(dev, "failed to open %s\n", eptdev->chinfo.name);
>> -		put_device(dev);
>> -		return -EINVAL;
>> +	/*
>> +	 * The RPMsg device can has been created by a ns announcement. In this
>> +	 * case a default endpoint has been created. Reuse it to avoid to manage
>> +	 * a new address on each open close.
>> +	 */
> 
> Here too it is very difficult to understand because the comment
> doesn't not describe what the code does.  The code creates an enpoint if it
> has not been created, which means /dev/rpmsgX was created from the ioctl. 

Right, not enough explicit

Thanks,
Arnaud

> 
>> +	ept = rpdev->ept;
>> +	if (!ept || addr != ept->addr) {
>> +		ept = rpmsg_create_ept(rpdev, rpmsg_ept_cb, eptdev, eptdev->chinfo);
>> +		if (!ept) {
>> +			dev_err(dev, "failed to open %s\n", eptdev->chinfo.name);
>> +			put_device(dev);
>> +			return -EINVAL;
>> +		}
>>  	}
>>  
>>  	eptdev->ept = ept;
>> @@ -133,12 +142,17 @@ static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
>>  static int rpmsg_eptdev_release(struct inode *inode, struct file *filp)
>>  {
>>  	struct rpmsg_eptdev *eptdev = cdev_to_eptdev(inode->i_cdev);
>> +	struct rpmsg_device *rpdev = eptdev->rpdev;
>>  	struct device *dev = &eptdev->dev;
>>  
>> -	/* Close the endpoint, if it's not already destroyed by the parent */
>> +	/*
>> +	 * Close the endpoint, if it's not already destroyed by the parent and it is not the
>> +	 * default one.
>> +	 */
>>  	mutex_lock(&eptdev->ept_lock);
>>  	if (eptdev->ept) {
>> -		rpmsg_destroy_ept(eptdev->ept);
>> +		if (eptdev->ept != rpdev->ept)
>> +			rpmsg_destroy_ept(eptdev->ept);
>>  		eptdev->ept = NULL;
>>  	}
>>  	mutex_unlock(&eptdev->ept_lock);
>> -- 
>> 2.17.1
>>

Re: [PATCH v5 15/16] rpmsg: char: no dynamic endpoint management for the default one

[Mathieu Poirier]

On Fri, Mar 05, 2021 at 12:09:37PM +0100, Arnaud POULIQUEN wrote:
> 
> 
> On 3/4/21 7:40 PM, Mathieu Poirier wrote:
> > There has to be a capital letter at the start of the title:
> > 
> > rpmsg: char: No dynamic endpoint management for the default one
> > 
> > Please fix for all the patches.
> 
> Ok, I will update the subjects with capital letter in my next revision.
> 
> Just for my information, is it a new rule? kernel documentation [1] gives a
> canonical subject and an example without capital letter.

I don't think it is a rule but in the past few years the trend has been to
use a capital letter.  I was convinced the documentation had a capital letter
but you have proven that it doesn't so you can ignore this part if you wish.

> 
> [1]
> https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#the-canonical-patch-format
> 
> > 
> > On Fri, Feb 19, 2021 at 12:15:00PM +0100, Arnaud Pouliquen wrote:
> >> Do not dynamically manage the default endpoint. The ept address must
> >> not change.
> >> This update is needed to manage the RPMSG_CREATE_DEV_IOCTL. In this
> >> case a default endpoint is used and it's address must not change or
> >> been reused by another service.
> > 
> > The above is very difficult to understand.  I am not sure about introducing
> > RPMSG_CREATE_DEV_IOCTL in this patchset.  More on that in an upcoming comment.
> 
> The purpose of this revision was mainly to provide a view of what we could do to
> provide a more generic control interface.
> 
> To simplify the review I can remove the RPMSG_CREATE_DEV_IOCTL management and
> send it as a next step, in a separate patchset.

Yes, it would make this patchset quite simple.

> 
> > 
> >>
> >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> >> ---
> >>  drivers/rpmsg/rpmsg_char.c | 28 +++++++++++++++++++++-------
> >>  1 file changed, 21 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> >> index c98b0e69679b..8d3f9d6c20ad 100644
> >> --- a/drivers/rpmsg/rpmsg_char.c
> >> +++ b/drivers/rpmsg/rpmsg_char.c
> >> @@ -114,14 +114,23 @@ static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
> >>  	struct rpmsg_endpoint *ept;
> >>  	struct rpmsg_device *rpdev = eptdev->rpdev;
> >>  	struct device *dev = &eptdev->dev;
> >> +	u32 addr = eptdev->chinfo.src;
> >>  
> >>  	get_device(dev);
> >>  
> >> -	ept = rpmsg_create_ept(rpdev, rpmsg_ept_cb, eptdev, eptdev->chinfo);
> >> -	if (!ept) {
> >> -		dev_err(dev, "failed to open %s\n", eptdev->chinfo.name);
> >> -		put_device(dev);
> >> -		return -EINVAL;
> >> +	/*
> >> +	 * The RPMsg device can has been created by a ns announcement. In this
> >> +	 * case a default endpoint has been created. Reuse it to avoid to manage
> >> +	 * a new address on each open close.
> >> +	 */
> > 
> > Here too it is very difficult to understand because the comment
> > doesn't not describe what the code does.  The code creates an enpoint if it
> > has not been created, which means /dev/rpmsgX was created from the ioctl. 
> 
> Right, not enough explicit
> 
> Thanks,
> Arnaud
> 
> > 
> >> +	ept = rpdev->ept;
> >> +	if (!ept || addr != ept->addr) {
> >> +		ept = rpmsg_create_ept(rpdev, rpmsg_ept_cb, eptdev, eptdev->chinfo);
> >> +		if (!ept) {
> >> +			dev_err(dev, "failed to open %s\n", eptdev->chinfo.name);
> >> +			put_device(dev);
> >> +			return -EINVAL;
> >> +		}
> >>  	}
> >>  
> >>  	eptdev->ept = ept;
> >> @@ -133,12 +142,17 @@ static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
> >>  static int rpmsg_eptdev_release(struct inode *inode, struct file *filp)
> >>  {
> >>  	struct rpmsg_eptdev *eptdev = cdev_to_eptdev(inode->i_cdev);
> >> +	struct rpmsg_device *rpdev = eptdev->rpdev;
> >>  	struct device *dev = &eptdev->dev;
> >>  
> >> -	/* Close the endpoint, if it's not already destroyed by the parent */
> >> +	/*
> >> +	 * Close the endpoint, if it's not already destroyed by the parent and it is not the
> >> +	 * default one.
> >> +	 */
> >>  	mutex_lock(&eptdev->ept_lock);
> >>  	if (eptdev->ept) {
> >> -		rpmsg_destroy_ept(eptdev->ept);
> >> +		if (eptdev->ept != rpdev->ept)
> >> +			rpmsg_destroy_ept(eptdev->ept);
> >>  		eptdev->ept = NULL;
> >>  	}
> >>  	mutex_unlock(&eptdev->ept_lock);
> >> -- 
> >> 2.17.1
> >>

[PATCH v5 16/16] rpmsg: char: return an error if device already open

[Arnaud Pouliquen]

The rpmsg_create_ept function is invoked when the device is opened.
As only one endpoint must be created per device. It is not possible to
open the same device twice. But there is nothing to prevent multi open.
Return -EBUSY when device is already opened to have a generic error
instead of relying on the back-end to potentially detect the error.

Without this patch for instance the GLINK driver return -EBUSY while
the virtio bus return -ENOSPC.

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 8d3f9d6c20ad..4cd5b79559f0 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -116,6 +116,9 @@ static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
 	struct device *dev = &eptdev->dev;
 	u32 addr = eptdev->chinfo.src;
 
+	if (eptdev->ept)
+		return -EBUSY;
+
 	get_device(dev);
 
 	/*
-- 
2.17.1

Re: [PATCH v5 16/16] rpmsg: char: return an error if device already open

[Mathieu Poirier]

On Fri, Feb 19, 2021 at 12:15:01PM +0100, Arnaud Pouliquen wrote:
> The rpmsg_create_ept function is invoked when the device is opened.
> As only one endpoint must be created per device. It is not possible to
> open the same device twice. But there is nothing to prevent multi open.

s/multi/multiple

> Return -EBUSY when device is already opened to have a generic error
> instead of relying on the back-end to potentially detect the error.
> 
> Without this patch for instance the GLINK driver return -EBUSY while
> the virtio bus return -ENOSPC.
> 
> 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 8d3f9d6c20ad..4cd5b79559f0 100644
> --- a/drivers/rpmsg/rpmsg_char.c
> +++ b/drivers/rpmsg/rpmsg_char.c
> @@ -116,6 +116,9 @@ static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
>  	struct device *dev = &eptdev->dev;
>  	u32 addr = eptdev->chinfo.src;
>  
> +	if (eptdev->ept)
> +		return -EBUSY;
> +

It would be nice to return the same error code regardless of the backend but at
the same time I feel like it isn't the right place to do this.  I need to think
about this one but for now we can keep it.

>  	get_device(dev);
>  
>  	/*
> -- 
> 2.17.1
> 

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

[Arnaud POULIQUEN]

On 2/19/21 12:14 PM, Arnaud Pouliquen wrote:
> This series restructures the RPMsg char driver to decorrelate the control part and to
> create a generic RPMsg ioctl interface compatible with other RPMsg services.
> 
> The V4 and V5 fix compilation issues reported by the kernel test robot <lkp@intel.com>
> and analyzed by Dan Carpenter <dan.carpenter@oracle.com>.
> 
> The V3 is based on the guideline proposed by Mathieu Poirier to keep as much as possible
> the legacy implementation of the rpmsg_char used by the GLINK and SMD platforms.
> 
> Objectives of the series:
> - Allow to create a service from Linux user application:
>   - with a specific name
>   - with or without name service announcement.
> - Allow to probe the same service by receiving either a NS announcement from the remote firmware
>   or a Linux user application request.
> - Use these services independently of the RPMsg transport implementation (e.g be able to use
>   RPMSg char with the RPMsg virtio bus).
> 
> Steps in the series:
>   - Extract the control part of the char dev and create the rpmsg_ctrl.c file (patches 1 to 6)
>   - Enable the use of the chardev with the virtio backend (patches 7 to 11)
>   - Introduce the RPMSG_CREATE_DEV_IOCTL IOCTL to instantiate RPMsg devices (patch 12)
>     The application can then create or release a channel by specifying:
>        - the name service of the device to instantiate.   
>        - the source address.
>        - the destination address.
>   - Instantiate the /dev/rpmsg interface on remote NS announcement (patches 13 to 16)
> 
> In this revision, I do not divide the series into several parts in order to show a complete
> picture of the proposed evolution. To simplify the review, if requested, I can send it in
> several steps listed above.

No new revision of this series planned, but move forward by splitting it in 3
new series to ease the review.
The first step is addressed here:
https://patchwork.kernel.org/project/linux-remoteproc/list/?series=446305

> 
> Known current Limitations:
> - Tested only with virtio RPMsg bus. The glink and smd drivers adaptations have not been tested
>   (not able to test it).
> - For the virtio backend: No NS announcement is sent to the remote processor if the source
>   address is set to RPMSG_ADDR_ANY.
> - For the virtio backend: the existing RPMSG_CREATE_EPT_IOCTL is working but the endpoints are
>   not attached to an exiting channel.
> - to limit patches the pending RPMSG_DESTROY_DEV_IOCTL has not ben implemented. This will be
>   proposed in a second step.
> 
> This series can be applied on git/andersson/remoteproc.git for-next branch (d9ff3a5789cb).
> 
> This series can be tested using rpmsgexport, rpmsgcreatedev and ping tools available here:
> https://github.com/arnopo/rpmsgexport.git
> 
> Reference to the V4 discussion thread: https://lkml.org/lkml/2021/2/17/384
> 
> Arnaud Pouliquen (16):
>   rpmsg: char: rename rpmsg_char_init to rpmsg_chrdev_init
>   rpmsg: move RPMSG_ADDR_ANY in user API
>   rpmsg: add short description of the IOCTL defined in UAPI.
>   rpmsg: char: export eptdev create an destroy functions
>   rpmsg: char: dissociate the control device from the rpmsg class
>   rpmsg: move the rpmsg control device from rpmsg_char to rpmsg_ctrl
>   rpmsg: update rpmsg_chrdev_register_device function
>   rpmsg: glink: add sendto and trysendto ops
>   rpmsg: smd: add sendto and trysendto ops
>   rpmsg: char: use sendto to specify the message destination address
>   rpmsg: virtio: register the rpmsg_ctrl device
>   rpmsg: ctrl: introduce RPMSG_CREATE_DEV_IOCTL
>   rpmsg: char: introduce __rpmsg_chrdev_create_eptdev function
>   rpmsg: char: introduce a RPMsg driver for the RPMsg char device
>   rpmsg: char: no dynamic endpoint management for the default one
>   rpmsg: char: return an error if device already open
> 
>  drivers/rpmsg/Kconfig             |   9 ++
>  drivers/rpmsg/Makefile            |   1 +
>  drivers/rpmsg/qcom_glink_native.c |  18 ++-
>  drivers/rpmsg/qcom_smd.c          |  18 ++-
>  drivers/rpmsg/rpmsg_char.c        | 237 +++++++++++-------------------
>  drivers/rpmsg/rpmsg_char.h        |  51 +++++++
>  drivers/rpmsg/rpmsg_ctrl.c        | 229 +++++++++++++++++++++++++++++
>  drivers/rpmsg/rpmsg_internal.h    |  10 +-
>  drivers/rpmsg/virtio_rpmsg_bus.c  |  57 ++++++-
>  include/linux/rpmsg.h             |   3 +-
>  include/uapi/linux/rpmsg.h        |  18 ++-
>  11 files changed, 485 insertions(+), 166 deletions(-)
>  create mode 100644 drivers/rpmsg/rpmsg_char.h
>  create mode 100644 drivers/rpmsg/rpmsg_ctrl.c
>