[PATCH v4 0/3] rpmsg: Introduce RPMSG_CREATE_EPT_FD_IOCTL uAPI

[PATCH v4 0/3] rpmsg: Introduce RPMSG_CREATE_EPT_FD_IOCTL uAPI

[Dawei Li]

Hi,

This is V4 of series which introduce new uAPI(RPMSG_CREATE_EPT_FD_IOCTL)
for rpmsg subsystem.

Current uAPI implementation for rpmsg ctrl & char device manipulation is
abstracted in procedures below:
- fd = open("/dev/rpmsg_ctrlX")
- ioctl(fd, RPMSG_CREATE_EPT_IOCTL, &info); /dev/rpmsgY devnode is
  generated.
- fd_ep = open("/dev/rpmsgY", O_RDWR) 
- operations on fd_ep(write, read, poll ioctl)
- ioctl(fd_ep, RPMSG_DESTROY_EPT_IOCTL)
- close(fd_ep)
- close(fd)

This /dev/rpmsgY abstraction is less favorable for:
- Performance issue: It's time consuming for some operations are
invovled:
  - Device node creation.
    Depends on specific config, especially CONFIG_DEVTMPFS, the overall
    overhead is based on coordination between DEVTMPFS and userspace
    tools such as udev and mdev.

  - Extra kernel-space switch cost.

  - Other major costs brought by heavy-weight logic like device_add().

- /dev/rpmsgY node can be opened only once. It doesn't make much sense
    that a dynamically created device node can be opened only once.

- For some container application such as docker, a client can't access
  host's dev unless specified explicitly. But in case of /dev/rpmsgY, which
  is generated dynamically and whose existence is unknown for clients in
  advance, this uAPI based on device node doesn't fit well.

An anonymous inode based approach is introduced to address the issues above.
Rather than generating device node and opening it, rpmsg code just creates
an anonymous inode representing eptdev and return the fd to userspace.

# Performance demo

An simple C application is tested to verify performance of new uAPI.

$ cat test.c

#include <linux/rpmsg.h>

#include <sys/types.h>
#include <sys/stat.h>
#include <sys/ioctl.h>
#include <fcntl.h>
#include <string.h>
#include <stdio.h>
#include <unistd.h>
#include <stdlib.h>
#include <errno.h>
#include <sys/time.h>

#define N (1 << 20)

int main(int argc, char *argv[])
{
	int ret, fd, ep_fd, loop;
	struct rpmsg_endpoint_info info; 
	struct rpmsg_endpoint_fd_info fd_info; 
	struct timeval start, end;
	int i = 0;
	double t1, t2;

	fd = -1;
	ep_fd = -1;
	loop = N;

	if (argc == 1) {
		loop = N;
	} else if (argc > 1) {
		loop = atoi(argv[1]);
	}

	printf("loop[%d]\n", loop);

	strcpy(info.name, "epx");
	info.src = -1;
	info.dst = -1;

	strcpy(fd_info.name, "epx");
	fd_info.src = -1;
	fd_info.dst = -1;
	fd_info.fd = -1;

	while (fd < 0) {
		fd = open("/dev/rpmsg_ctrl0", O_RDWR);
		if (fd < 0) {
			printf("open rpmsg_ctrl0 failed, fd[%d]\n", fd);
		}
	}

	gettimeofday(&start, NULL);

	while (loop--) {
		ret = ioctl(fd, RPMSG_CREATE_EPT_IOCTL, &info);
		if (ret < 0) {
			printf("ioctl[RPMSG_CREATE_EPT_IOCTL] failed, ret[%d]\n", ret);
		}

		ep_fd = -1;
		i = 0;

		while (ep_fd < 0) {
			ep_fd = open("/dev/rpmsg0", O_RDWR);
			if (ep_fd < 0) {
				i++;
				printf("open rpmsg0 failed, epfd[%d]\n", ep_fd);
			}
		}

		//printf("Number of open failed[%d]\n", i);

		ret = ioctl(ep_fd, RPMSG_DESTROY_EPT_IOCTL, &info);
		if (ret < 0) {
			printf("old ioctl[RPMSG_DESTROY_EPT_IOCTL] failed, ret[%d], errno[%d]\n", ret, errno);
		}

		close(ep_fd);
	}
	
	gettimeofday(&end, NULL);

	printf("time for old way: [%ld] us\n", 1000000 * (end.tv_sec - start.tv_sec) + end.tv_usec - start.tv_usec);
	t1 = 1000000 * (end.tv_sec - start.tv_sec) + end.tv_usec - start.tv_usec;

	if (argc == 1) {
		loop = N;
	} else if (argc > 1) {
		loop = atoi(argv[1]);
	}

	printf("loop[%d]\n", loop);

	gettimeofday(&start, NULL);

	while (loop--) {
		fd_info.fd = -1;
		fd_info.flags = O_RDWR | O_CLOEXEC | O_NONBLOCK;
		ret = ioctl(fd, RPMSG_CREATE_EPT_FD_IOCTL, &fd_info);
		if (ret < 0 || fd_info.fd < 0) {
			printf("ioctl[RPMSG_CREATE_EPT_FD_IOCTL] failed, ret[%d]\n", ret);
		}

		ret = ioctl(fd_info.fd, RPMSG_DESTROY_EPT_IOCTL, &info);
		if (ret < 0) {
			printf("new ioctl[RPMSG_DESTROY_EPT_IOCTL] failed, ret[%d]\n", ret);
		}

		close(fd_info.fd);
	}
	
	gettimeofday(&end, NULL);

	printf("time for new way: [%ld] us\n", 1000000 * (end.tv_sec - start.tv_sec) + end.tv_usec - start.tv_usec);
	t2 = 1000000 * (end.tv_sec - start.tv_sec) + end.tv_usec - start.tv_usec;

	printf("t1(old) / t2(new) = %f\n", t1 / t2);

	close(fd);
}

# Performance benchmark

- Legacy means benchmark based on old uAPI
- New means benchmark based on new uAPI(the one this series introduce)
- Time are in units of us(10^-6 s)

Test	loops	Total time(legacy)	Total time(new)	legacy/new	
1	1000	218472			2445		89.354601
2	1000	223435			2419		92.366680
3	1000	224263			2487		90.174105
4	1000	218982			2465		88.836511
5	1000	209640			2574		81.445221
6	1000	203816			2509		81.233958
7	1000	203266			2458		82.695688
8	1000	222842			2835		78.603880
9	1000	209590			2719		77.083487
10	1000	194558			2621		74.230446

11	10000	2129021			31239		68.152662
12	10000	2081722			27997		74.355181
13	10000	2077086			31724		65.473648
14	10000	2073547			28290		73.296112
15	10000	2055153			26957		76.238194
16	10000	2022767			29809		67.857593
17	10000	2054562			25884		79.375753
18	10000	2036320			28511		71.422258
19	10000	2062547			28725		71.803203
20	10000	2110498			26740		78.926627

21	100000	20802565		260392		79.889417
22	100000	20373178		259836		78.407834
23	100000	20361077		256404		79.410138
24	100000	20207000		256759		78.700260
25	100000	20220358		268118		75.415892
26	100000	20711593		259130		79.927423
27	100000	20301064		258545		78.520428
28	100000	20393203		256070		79.639173
29	100000	20162830		259942		77.566649
30	100000	20471632		259291		78.952343

# Changelog:

Changes in v4:
- Build warning of copy_to_user (Dan).
- ioctl() branches reorder (Beleswar).
- Remove local variable fd and pass &ept_fd_info.fd to rpmsg_anonymous_eptdev_create().
- Link to v3: https://lore.kernel.org/all/20250519150823.62350-1-dawei.li@linux.dev/ 

Changes in v3:
- s/anon/anonymous (Mathieu)

- API naming adjustment (Mathieu)
  - __rpmsg_chrdev_eptdev_alloc ->  rpmsg_eptdev_alloc
  - __rpmsg_chrdev_eptdev_add ->  rpmsg_eptdev_add

- Add parameter 'flags' to uAPI so user can specify file flags
  explicitly on creating anonymous inode.
- Link to v2: https://lore.kernel.org/all/20250509155927.109258-1-dawei.li@linux.dev/ 

Changes in v2:
- Fix compilation error for !CONFIG_RPMSG_CHAR config(Test robot).
- Link to v1: https://lore.kernel.org/all/20250507141712.4276-1-dawei.li@linux.dev/

Dawei Li (3):
  rpmsg: char: Reuse eptdev logic for anonymous device
  rpmsg: char: Implement eptdev based on anonymous inode
  rpmsg: ctrl: Introduce RPMSG_CREATE_EPT_FD_IOCTL uAPI

 drivers/rpmsg/rpmsg_char.c | 129 ++++++++++++++++++++++++++++++-------
 drivers/rpmsg/rpmsg_char.h |  23 +++++++
 drivers/rpmsg/rpmsg_ctrl.c |  35 ++++++++--
 include/uapi/linux/rpmsg.h |  27 +++++++-
 4 files changed, 182 insertions(+), 32 deletions(-)

---
base-commit: 92a09c47464d040866cf2b4cd052bc60555185fb

Thanks,

	Dawei
-- 
2.25.1

[PATCH v4 1/3] rpmsg: char: Reuse eptdev logic for anonymous device

[Dawei Li]

Current uAPI implementation for rpmsg ctrl & char device manipulation is
abstracted in procedures below:
- fd = open("/dev/rpmsg_ctrlX")
- ioctl(fd, RPMSG_CREATE_EPT_IOCTL, &info); /dev/rpmsgY devnode is
  generated.
- fd_ep = open("/dev/rpmsgY", O_RDWR)
- operations on fd_ep(write, read, poll ioctl)
- ioctl(fd_ep, RPMSG_DESTROY_EPT_IOCTL)
- close(fd_ep)
- close(fd)

This /dev/rpmsgY abstraction is less favorable for:
- Performance issue: It's time consuming for some operations are involved:
  - Device node creation.
    Depends on specific config, especially CONFIG_DEVTMPFS, the overall
    overhead is based on coordination between DEVTMPFS and userspace
    tools such as udev and mdev.

  - Extra kernel-space switch cost.

  - Other major costs brought by heavy-weight logic like device_add().

- /dev/rpmsgY node can be opened only once. It doesn't make much sense
    that a dynamically created device node can be opened only once.

- For some container application such as docker, a client can't access
  host's device node unless specified explicitly. But in case of
  /dev/rpmsgY, which is generated dynamically and whose existence is
  unknown for clients in advance, this uAPI based on device node doesn't
  fit well.

An anonymous inode based approach is introduced to address the issues above.
Rather than generating device node and opening it, rpmsg code just creates
an anonymous inode representing eptdev and return the fd to userspace.

The legacy abstraction based on struct dev and struct cdev is honored
for:
- Avoid legacy uAPI break(RPMSG_CREATE_EPT_IOCTL)
- Reuse existing logic:
  -- dev_err() and friends.
  -- Life cycle management of struct device.

Signed-off-by: Dawei Li <dawei.li@linux.dev>
---
 drivers/rpmsg/rpmsg_char.c | 80 ++++++++++++++++++++++++++------------
 1 file changed, 56 insertions(+), 24 deletions(-)

diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index eec7642d2686..6c19f2146698 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -91,7 +91,8 @@ int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data)
 	/* wake up any blocked readers */
 	wake_up_interruptible(&eptdev->readq);
 
-	cdev_device_del(&eptdev->cdev, &eptdev->dev);
+	if (eptdev->dev.devt)
+		cdev_device_del(&eptdev->cdev, &eptdev->dev);
 	put_device(&eptdev->dev);
 
 	return 0;
@@ -132,21 +133,17 @@ static int rpmsg_ept_flow_cb(struct rpmsg_device *rpdev, void *priv, bool enable
 	return 0;
 }
 
-static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
+static int __rpmsg_eptdev_open(struct rpmsg_eptdev *eptdev)
 {
-	struct rpmsg_eptdev *eptdev = cdev_to_eptdev(inode->i_cdev);
 	struct rpmsg_endpoint *ept;
 	struct rpmsg_device *rpdev = eptdev->rpdev;
 	struct device *dev = &eptdev->dev;
 
-	mutex_lock(&eptdev->ept_lock);
 	if (eptdev->ept) {
-		mutex_unlock(&eptdev->ept_lock);
 		return -EBUSY;
 	}
 
 	if (!eptdev->rpdev) {
-		mutex_unlock(&eptdev->ept_lock);
 		return -ENETRESET;
 	}
 
@@ -164,21 +161,32 @@ static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
 	if (!ept) {
 		dev_err(dev, "failed to open %s\n", eptdev->chinfo.name);
 		put_device(dev);
-		mutex_unlock(&eptdev->ept_lock);
 		return -EINVAL;
 	}
 
 	ept->flow_cb = rpmsg_ept_flow_cb;
 	eptdev->ept = ept;
-	filp->private_data = eptdev;
-	mutex_unlock(&eptdev->ept_lock);
 
 	return 0;
 }
 
-static int rpmsg_eptdev_release(struct inode *inode, struct file *filp)
+static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
 {
 	struct rpmsg_eptdev *eptdev = cdev_to_eptdev(inode->i_cdev);
+	int ret;
+
+	mutex_lock(&eptdev->ept_lock);
+	ret = __rpmsg_eptdev_open(eptdev);
+	if (!ret)
+		filp->private_data = eptdev;
+	mutex_unlock(&eptdev->ept_lock);
+
+	return ret;
+}
+
+static int rpmsg_eptdev_release(struct inode *inode, struct file *filp)
+{
+	struct rpmsg_eptdev *eptdev = filp->private_data;
 	struct device *dev = &eptdev->dev;
 
 	/* Close the endpoint, if it's not already destroyed by the parent */
@@ -400,12 +408,13 @@ static void rpmsg_eptdev_release_device(struct device *dev)
 	struct rpmsg_eptdev *eptdev = dev_to_eptdev(dev);
 
 	ida_free(&rpmsg_ept_ida, dev->id);
-	ida_free(&rpmsg_minor_ida, MINOR(eptdev->dev.devt));
+	if (eptdev->dev.devt)
+		ida_free(&rpmsg_minor_ida, MINOR(eptdev->dev.devt));
 	kfree(eptdev);
 }
 
-static struct rpmsg_eptdev *rpmsg_chrdev_eptdev_alloc(struct rpmsg_device *rpdev,
-						      struct device *parent)
+static struct rpmsg_eptdev *rpmsg_eptdev_alloc(struct rpmsg_device *rpdev,
+					       struct device *parent, bool cdev)
 {
 	struct rpmsg_eptdev *eptdev;
 	struct device *dev;
@@ -428,33 +437,50 @@ static struct rpmsg_eptdev *rpmsg_chrdev_eptdev_alloc(struct rpmsg_device *rpdev
 	dev->groups = rpmsg_eptdev_groups;
 	dev_set_drvdata(dev, eptdev);
 
-	cdev_init(&eptdev->cdev, &rpmsg_eptdev_fops);
-	eptdev->cdev.owner = THIS_MODULE;
+	if (cdev) {
+		cdev_init(&eptdev->cdev, &rpmsg_eptdev_fops);
+		eptdev->cdev.owner = THIS_MODULE;
+	}
 
 	return eptdev;
 }
 
-static int rpmsg_chrdev_eptdev_add(struct rpmsg_eptdev *eptdev, struct rpmsg_channel_info chinfo)
+static struct rpmsg_eptdev *rpmsg_chrdev_eptdev_alloc(struct rpmsg_device *rpdev,
+						      struct device *parent)
+{
+	return rpmsg_eptdev_alloc(rpdev, parent, true);
+}
+
+static int rpmsg_eptdev_add(struct rpmsg_eptdev *eptdev,
+			    struct rpmsg_channel_info chinfo, bool cdev)
 {
 	struct device *dev = &eptdev->dev;
 	int ret;
 
 	eptdev->chinfo = chinfo;
 
-	ret = ida_alloc_max(&rpmsg_minor_ida, RPMSG_DEV_MAX - 1, GFP_KERNEL);
-	if (ret < 0)
-		goto free_eptdev;
-	dev->devt = MKDEV(MAJOR(rpmsg_major), ret);
+	if (cdev) {
+		ret = ida_alloc_max(&rpmsg_minor_ida, RPMSG_DEV_MAX - 1, GFP_KERNEL);
+		if (ret < 0)
+			goto free_eptdev;
 
+		dev->devt = MKDEV(MAJOR(rpmsg_major), ret);
+	}
+
+	/* Anonymous inode device still need device name for dev_err() and friends */
 	ret = ida_alloc(&rpmsg_ept_ida, GFP_KERNEL);
 	if (ret < 0)
 		goto free_minor_ida;
 	dev->id = ret;
 	dev_set_name(dev, "rpmsg%d", ret);
 
-	ret = cdev_device_add(&eptdev->cdev, &eptdev->dev);
-	if (ret)
-		goto free_ept_ida;
+	ret = 0;
+
+	if (cdev) {
+		ret = cdev_device_add(&eptdev->cdev, &eptdev->dev);
+		if (ret)
+			goto free_ept_ida;
+	}
 
 	/* We can now rely on the release function for cleanup */
 	dev->release = rpmsg_eptdev_release_device;
@@ -464,7 +490,8 @@ static int rpmsg_chrdev_eptdev_add(struct rpmsg_eptdev *eptdev, struct rpmsg_cha
 free_ept_ida:
 	ida_free(&rpmsg_ept_ida, dev->id);
 free_minor_ida:
-	ida_free(&rpmsg_minor_ida, MINOR(dev->devt));
+	if (cdev)
+		ida_free(&rpmsg_minor_ida, MINOR(dev->devt));
 free_eptdev:
 	put_device(dev);
 	kfree(eptdev);
@@ -472,6 +499,11 @@ static int rpmsg_chrdev_eptdev_add(struct rpmsg_eptdev *eptdev, struct rpmsg_cha
 	return ret;
 }
 
+static int rpmsg_chrdev_eptdev_add(struct rpmsg_eptdev *eptdev, struct rpmsg_channel_info chinfo)
+{
+	return rpmsg_eptdev_add(eptdev, chinfo, true);
+}
+
 int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent,
 			       struct rpmsg_channel_info chinfo)
 {
-- 
2.25.1

[PATCH v4 2/3] rpmsg: char: Implement eptdev based on anonymous inode

[Dawei Li]

Introduce new eptdev abstraction based on anonymous inode. The new API
is exactly same with legacy one except:

- It's anonymous and devnode/path free.
- Its fops->open() is empty.

Signed-off-by: Dawei Li <dawei.li@linux.dev>
---
 drivers/rpmsg/rpmsg_char.c | 49 ++++++++++++++++++++++++++++++++++++++
 drivers/rpmsg/rpmsg_char.h | 23 ++++++++++++++++++
 2 files changed, 72 insertions(+)

diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index 6c19f2146698..babc63cc4238 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -13,6 +13,7 @@
 
 #define pr_fmt(fmt)	KBUILD_MODNAME ": " fmt
 
+#include <linux/anon_inodes.h>
 #include <linux/cdev.h>
 #include <linux/device.h>
 #include <linux/fs.h>
@@ -517,6 +518,54 @@ int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent
 }
 EXPORT_SYMBOL(rpmsg_chrdev_eptdev_create);
 
+static const struct file_operations rpmsg_anonymous_eptdev_fops = {
+	.owner = THIS_MODULE,
+	.release = rpmsg_eptdev_release,
+	.read_iter = rpmsg_eptdev_read_iter,
+	.write_iter = rpmsg_eptdev_write_iter,
+	.poll = rpmsg_eptdev_poll,
+	.unlocked_ioctl = rpmsg_eptdev_ioctl,
+	.compat_ioctl = compat_ptr_ioctl,
+};
+
+int rpmsg_anonymous_eptdev_create(struct rpmsg_device *rpdev, struct device *parent,
+				  struct rpmsg_channel_info chinfo, unsigned int flags,
+				  int *pfd)
+{
+	struct rpmsg_eptdev *eptdev;
+	int ret, fd;
+
+	/* Anonymous inode only supports these file flags */
+	if (flags & ~(O_ACCMODE | O_NONBLOCK | O_CLOEXEC))
+		return -EINVAL;
+
+	eptdev = rpmsg_eptdev_alloc(rpdev, parent, false);
+	if (IS_ERR(eptdev))
+		return PTR_ERR(eptdev);
+
+	ret =  rpmsg_eptdev_add(eptdev, chinfo, false);
+	if (ret) {
+		dev_err(&eptdev->dev, "failed to add %s\n", eptdev->chinfo.name);
+		return ret;
+	}
+
+	fd = anon_inode_getfd("rpmsg-eptdev", &rpmsg_anonymous_eptdev_fops, eptdev, flags);
+	if (fd < 0) {
+		put_device(&eptdev->dev);
+		return fd;
+	}
+
+	mutex_lock(&eptdev->ept_lock);
+	ret = __rpmsg_eptdev_open(eptdev);
+	mutex_unlock(&eptdev->ept_lock);
+
+	if (!ret)
+		*pfd = fd;
+
+	return ret;
+}
+EXPORT_SYMBOL(rpmsg_anonymous_eptdev_create);
+
 static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
 {
 	struct rpmsg_channel_info chinfo;
diff --git a/drivers/rpmsg/rpmsg_char.h b/drivers/rpmsg/rpmsg_char.h
index 117d9cbc52f0..70ce2c511f13 100644
--- a/drivers/rpmsg/rpmsg_char.h
+++ b/drivers/rpmsg/rpmsg_char.h
@@ -19,6 +19,22 @@
 int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent,
 			       struct rpmsg_channel_info chinfo);
 
+/**
+ * rpmsg_anonymous_eptdev_create() - register anonymous device and its associated
+ *                                   fd based on an endpoint
+ * @rpdev:  prepared rpdev to be used for creating endpoints
+ * @parent: parent device
+ * @chinfo: associated endpoint channel information.
+ * @flag: file flag
+ * @pfd: fd in represent of endpoint device
+ *
+ * This function create a new rpmsg endpoint device and its associated fd to instantiate a new
+ * endpoint based on chinfo information.
+ */
+int rpmsg_anonymous_eptdev_create(struct rpmsg_device *rpdev, struct device *parent,
+				  struct rpmsg_channel_info chinfo, unsigned int flags,
+				  int *pfd);
+
 /**
  * rpmsg_chrdev_eptdev_destroy() - destroy created char device endpoint.
  * @data: private data associated to the endpoint device
@@ -36,6 +52,13 @@ static inline int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct
 	return -ENXIO;
 }
 
+static inline int rpmsg_anonymous_eptdev_create(struct rpmsg_device *rpdev, struct device *parent,
+						struct rpmsg_channel_info chinfo,
+						unsigned int flags, int *pfd)
+{
+	return -ENXIO;
+}
+
 static inline int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data)
 {
 	return -ENXIO;
-- 
2.25.1

[PATCH v4 3/3] rpmsg: ctrl: Introduce RPMSG_CREATE_EPT_FD_IOCTL uAPI

[Dawei Li]

Implement RPMSG_CREATE_EPT_FD_IOCTL, new uAPI for rpmsg ctrl, which
shares most of operations of RPMSG_CREATE_EPT_IOCTL except that it
returns fd representing eptdev to userspace directly.

Possible calling procedures for userspace are:
- fd = open("/dev/rpmsg_ctrlX")
- ioctl(fd, RPMSG_CREATE_EPT_FD_IOCTL, &info);
- fd_ep = info.fd
- operations on fd_ep(write, read, poll ioctl)
- ioctl(fd_ep, RPMSG_DESTROY_EPT_IOCTL)
- close(fd_ep)
- close(fd)

Signed-off-by: Dawei Li <dawei.li@linux.dev>
---
 drivers/rpmsg/rpmsg_ctrl.c | 35 ++++++++++++++++++++++++++++-------
 include/uapi/linux/rpmsg.h | 27 ++++++++++++++++++++++++++-
 2 files changed, 54 insertions(+), 8 deletions(-)

diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
index 28f57945ccd9..efb207506e5c 100644
--- a/drivers/rpmsg/rpmsg_ctrl.c
+++ b/drivers/rpmsg/rpmsg_ctrl.c
@@ -75,19 +75,30 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
 				unsigned long arg)
 {
 	struct rpmsg_ctrldev *ctrldev = fp->private_data;
+	struct rpmsg_endpoint_fd_info ept_fd_info;
 	void __user *argp = (void __user *)arg;
 	struct rpmsg_endpoint_info eptinfo;
 	struct rpmsg_channel_info chinfo;
 	struct rpmsg_device *rpdev;
 	int ret = 0;
 
-	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;
+	if (cmd == RPMSG_CREATE_EPT_FD_IOCTL) {
+		if (copy_from_user(&ept_fd_info, argp, sizeof(ept_fd_info)))
+			return -EFAULT;
+
+		memcpy(chinfo.name, ept_fd_info.name, RPMSG_NAME_SIZE);
+		chinfo.name[RPMSG_NAME_SIZE - 1] = '\0';
+		chinfo.src = ept_fd_info.src;
+		chinfo.dst = ept_fd_info.dst;
+	} else {
+		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;
+	}
 
 	mutex_lock(&ctrldev->ctrl_lock);
 	switch (cmd) {
@@ -110,6 +121,16 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
 				chinfo.name, ret);
 		break;
 
+	case RPMSG_CREATE_EPT_FD_IOCTL:
+		ret = rpmsg_anonymous_eptdev_create(ctrldev->rpdev, &ctrldev->dev, chinfo,
+						    ept_fd_info.flags, &ept_fd_info.fd);
+		if (ret)
+			break;
+
+		if (copy_to_user(argp, &ept_fd_info, sizeof(ept_fd_info)))
+			ret = -EFAULT;
+		break;
+
 	default:
 		ret = -EINVAL;
 	}
diff --git a/include/uapi/linux/rpmsg.h b/include/uapi/linux/rpmsg.h
index f0c8da2b185b..02befb298f71 100644
--- a/include/uapi/linux/rpmsg.h
+++ b/include/uapi/linux/rpmsg.h
@@ -29,7 +29,8 @@ struct rpmsg_endpoint_info {
 #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.
+ * Destroy a rpmsg char device endpoint created by the RPMSG_CREATE_EPT_IOCTL
+ * or RPMSG_CREATE_EPT_FD_IOCTL.
  */
 #define RPMSG_DESTROY_EPT_IOCTL	_IO(0xb5, 0x2)
 
@@ -53,4 +54,28 @@ struct rpmsg_endpoint_info {
  */
 #define RPMSG_SET_INCOMING_FLOWCONTROL _IOR(0xb5, 0x6, int)
 
+/**
+ * struct rpmsg_endpoint_fd_info - endpoint & fd info representation
+ * @name: name of service
+ * @src: local address. To set to RPMSG_ADDR_ANY if not used.
+ * @dst: destination address. To set to RPMSG_ADDR_ANY if not used.
+ * @flags: file flags of endpoint device, valid flags:
+ *         O_RDONLY/O_WRONLY/O_RDWR
+ *         O_NONBLOCK
+ *         O_CLOEXEC
+ * @fd: fd returned from driver
+ */
+struct rpmsg_endpoint_fd_info {
+	char name[32];
+	__u32 src;
+	__u32 dst;
+	__u32 flags;
+	__s32 fd;
+};
+
+/**
+ * Instantiate a new rmpsg endpoint which is represented by fd
+ */
+#define RPMSG_CREATE_EPT_FD_IOCTL _IOWR(0xb5, 0x7, struct rpmsg_endpoint_fd_info)
+
 #endif
-- 
2.25.1

Re: [PATCH v4 0/3] rpmsg: Introduce RPMSG_CREATE_EPT_FD_IOCTL uAPI

[Arnaud POULIQUEN]

Hello Dawei,


Please find a few comments below. It is not clear to me which parts of your
implementation are mandatory and which are optional "nice-to-have" optimizations.

Based on (potentially erroneous) hypothesis, you will find a suggestion for an
alternative to the anonymous inode approach, which does not seem to be a common
interface.


On 6/9/25 17:15, Dawei Li wrote:
> Hi,
> 
> This is V4 of series which introduce new uAPI(RPMSG_CREATE_EPT_FD_IOCTL)
> for rpmsg subsystem.
> 
> Current uAPI implementation for rpmsg ctrl & char device manipulation is
> abstracted in procedures below:
> - fd = open("/dev/rpmsg_ctrlX")
> - ioctl(fd, RPMSG_CREATE_EPT_IOCTL, &info); /dev/rpmsgY devnode is
>   generated.
> - fd_ep = open("/dev/rpmsgY", O_RDWR) 
> - operations on fd_ep(write, read, poll ioctl)
> - ioctl(fd_ep, RPMSG_DESTROY_EPT_IOCTL)
> - close(fd_ep)
> - close(fd)
> 
> This /dev/rpmsgY abstraction is less favorable for:
> - Performance issue: It's time consuming for some operations are
> invovled:
>   - Device node creation.
>     Depends on specific config, especially CONFIG_DEVTMPFS, the overall
>     overhead is based on coordination between DEVTMPFS and userspace
>     tools such as udev and mdev.
> 
>   - Extra kernel-space switch cost.
> 
>   - Other major costs brought by heavy-weight logic like device_add().

Is this a blocker of just optimization?

> 
> - /dev/rpmsgY node can be opened only once. It doesn't make much sense
>     that a dynamically created device node can be opened only once.


I assume this is blocker with the fact that you need to open the /dev/rpmsg<x>
to create the endpoint.


> 
> - For some container application such as docker, a client can't access
>   host's dev unless specified explicitly. But in case of /dev/rpmsgY, which
>   is generated dynamically and whose existence is unknown for clients in
>   advance, this uAPI based on device node doesn't fit well.

does this could be solve in userspace parsing /sys/class/rpmsg/ directory to
retreive the device?

You could face same kind of random instantiation for serial peripherals ( UART;
USb, I2C,...) based on a device tree enumeration. I suppose that user space
use to solve this.

> 
> An anonymous inode based approach is introduced to address the issues above.
> Rather than generating device node and opening it, rpmsg code just creates
> an anonymous inode representing eptdev and return the fd to userspace.

A drawback is that you need to share fb passed between processes.


> 
> # Performance demo
> 
> An simple C application is tested to verify performance of new uAPI.
> 
> $ cat test.c
> 
> #include <linux/rpmsg.h>
> 
> #include <sys/types.h>
> #include <sys/stat.h>
> #include <sys/ioctl.h>
> #include <fcntl.h>
> #include <string.h>
> #include <stdio.h>
> #include <unistd.h>
> #include <stdlib.h>
> #include <errno.h>
> #include <sys/time.h>
> 
> #define N (1 << 20)
> 
> int main(int argc, char *argv[])
> {
> 	int ret, fd, ep_fd, loop;
> 	struct rpmsg_endpoint_info info; 
> 	struct rpmsg_endpoint_fd_info fd_info; 
> 	struct timeval start, end;
> 	int i = 0;
> 	double t1, t2;
> 
> 	fd = -1;
> 	ep_fd = -1;
> 	loop = N;
> 
> 	if (argc == 1) {
> 		loop = N;
> 	} else if (argc > 1) {
> 		loop = atoi(argv[1]);
> 	}
> 
> 	printf("loop[%d]\n", loop);
> 
> 	strcpy(info.name, "epx");
> 	info.src = -1;
> 	info.dst = -1;
> 
> 	strcpy(fd_info.name, "epx");
> 	fd_info.src = -1;
> 	fd_info.dst = -1;
> 	fd_info.fd = -1;
> 
> 	while (fd < 0) {
> 		fd = open("/dev/rpmsg_ctrl0", O_RDWR);
> 		if (fd < 0) {
> 			printf("open rpmsg_ctrl0 failed, fd[%d]\n", fd);
> 		}
> 	}
> 
> 	gettimeofday(&start, NULL);
> 
> 	while (loop--) {
> 		ret = ioctl(fd, RPMSG_CREATE_EPT_IOCTL, &info);
> 		if (ret < 0) {
> 			printf("ioctl[RPMSG_CREATE_EPT_IOCTL] failed, ret[%d]\n", ret);
> 		}
> 
> 		ep_fd = -1;
> 		i = 0;
> 
> 		while (ep_fd < 0) {
> 			ep_fd = open("/dev/rpmsg0", O_RDWR);
> 			if (ep_fd < 0) {
> 				i++;
> 				printf("open rpmsg0 failed, epfd[%d]\n", ep_fd);
> 			}
> 		}
> 
> 		//printf("Number of open failed[%d]\n", i);
> 
> 		ret = ioctl(ep_fd, RPMSG_DESTROY_EPT_IOCTL, &info);
> 		if (ret < 0) {
> 			printf("old ioctl[RPMSG_DESTROY_EPT_IOCTL] failed, ret[%d], errno[%d]\n", ret, errno);
> 		}
> 
> 		close(ep_fd);
> 	}
> 	
> 	gettimeofday(&end, NULL);
> 
> 	printf("time for old way: [%ld] us\n", 1000000 * (end.tv_sec - start.tv_sec) + end.tv_usec - start.tv_usec);
> 	t1 = 1000000 * (end.tv_sec - start.tv_sec) + end.tv_usec - start.tv_usec;
> 
> 	if (argc == 1) {
> 		loop = N;
> 	} else if (argc > 1) {
> 		loop = atoi(argv[1]);
> 	}
> 
> 	printf("loop[%d]\n", loop);
> 
> 	gettimeofday(&start, NULL);
> 
> 	while (loop--) {

Do you need to create /close Endpoint sevral times in your real use case with
high timing
constraint?

> 		fd_info.fd = -1;
> 		fd_info.flags = O_RDWR | O_CLOEXEC | O_NONBLOCK;
> 		ret = ioctl(fd, RPMSG_CREATE_EPT_FD_IOCTL, &fd_info);
> 		if (ret < 0 || fd_info.fd < 0) {
> 			printf("ioctl[RPMSG_CREATE_EPT_FD_IOCTL] failed, ret[%d]\n", ret);
> 		}
> 


> 		ret = ioctl(fd_info.fd, RPMSG_DESTROY_EPT_IOCTL, &info);
> 		if (ret < 0) {
> 			printf("new ioctl[RPMSG_DESTROY_EPT_IOCTL] failed, ret[%d]\n", ret);
> 		}
> 
> 		close(fd_info.fd);

It seems strange to me to use ioctl() for opening and close() for closing, from
a symmetry point of view.

Regarding your implementation, I wonder if we could keep the /dev/rpmsg<x>
device with specific open() and close() file operations associated with your new
ioctl.

- The ioctl would create the endpoint.
- The open() and close() operations would simply manage the file descriptor and
increment/decrement a counter to prevent premature endpoint destruction.


Regards,
Arnaud

> 	}
> 	
> 	gettimeofday(&end, NULL);
> 
> 	printf("time for new way: [%ld] us\n", 1000000 * (end.tv_sec - start.tv_sec) + end.tv_usec - start.tv_usec);
> 	t2 = 1000000 * (end.tv_sec - start.tv_sec) + end.tv_usec - start.tv_usec;
> 
> 	printf("t1(old) / t2(new) = %f\n", t1 / t2);
> 
> 	close(fd);
> }
> 
> # Performance benchmark
> 
> - Legacy means benchmark based on old uAPI
> - New means benchmark based on new uAPI(the one this series introduce)
> - Time are in units of us(10^-6 s)
> 
> Test	loops	Total time(legacy)	Total time(new)	legacy/new	
> 1	1000	218472			2445		89.354601
> 2	1000	223435			2419		92.366680
> 3	1000	224263			2487		90.174105
> 4	1000	218982			2465		88.836511
> 5	1000	209640			2574		81.445221
> 6	1000	203816			2509		81.233958
> 7	1000	203266			2458		82.695688
> 8	1000	222842			2835		78.603880
> 9	1000	209590			2719		77.083487
> 10	1000	194558			2621		74.230446
> 
> 11	10000	2129021			31239		68.152662
> 12	10000	2081722			27997		74.355181
> 13	10000	2077086			31724		65.473648
> 14	10000	2073547			28290		73.296112
> 15	10000	2055153			26957		76.238194
> 16	10000	2022767			29809		67.857593
> 17	10000	2054562			25884		79.375753
> 18	10000	2036320			28511		71.422258
> 19	10000	2062547			28725		71.803203
> 20	10000	2110498			26740		78.926627
> 
> 21	100000	20802565		260392		79.889417
> 22	100000	20373178		259836		78.407834
> 23	100000	20361077		256404		79.410138
> 24	100000	20207000		256759		78.700260
> 25	100000	20220358		268118		75.415892
> 26	100000	20711593		259130		79.927423
> 27	100000	20301064		258545		78.520428
> 28	100000	20393203		256070		79.639173
> 29	100000	20162830		259942		77.566649
> 30	100000	20471632		259291		78.952343
> 
> # Changelog:
> 
> Changes in v4:
> - Build warning of copy_to_user (Dan).
> - ioctl() branches reorder (Beleswar).
> - Remove local variable fd and pass &ept_fd_info.fd to rpmsg_anonymous_eptdev_create().
> - Link to v3: https://lore.kernel.org/all/20250519150823.62350-1-dawei.li@linux.dev/ 
> 
> Changes in v3:
> - s/anon/anonymous (Mathieu)
> 
> - API naming adjustment (Mathieu)
>   - __rpmsg_chrdev_eptdev_alloc ->  rpmsg_eptdev_alloc
>   - __rpmsg_chrdev_eptdev_add ->  rpmsg_eptdev_add
> 
> - Add parameter 'flags' to uAPI so user can specify file flags
>   explicitly on creating anonymous inode.
> - Link to v2: https://lore.kernel.org/all/20250509155927.109258-1-dawei.li@linux.dev/ 
> 
> Changes in v2:
> - Fix compilation error for !CONFIG_RPMSG_CHAR config(Test robot).
> - Link to v1: https://lore.kernel.org/all/20250507141712.4276-1-dawei.li@linux.dev/
> 
> Dawei Li (3):
>   rpmsg: char: Reuse eptdev logic for anonymous device
>   rpmsg: char: Implement eptdev based on anonymous inode
>   rpmsg: ctrl: Introduce RPMSG_CREATE_EPT_FD_IOCTL uAPI
> 
>  drivers/rpmsg/rpmsg_char.c | 129 ++++++++++++++++++++++++++++++-------
>  drivers/rpmsg/rpmsg_char.h |  23 +++++++
>  drivers/rpmsg/rpmsg_ctrl.c |  35 ++++++++--
>  include/uapi/linux/rpmsg.h |  27 +++++++-
>  4 files changed, 182 insertions(+), 32 deletions(-)
> 
> ---
> base-commit: 92a09c47464d040866cf2b4cd052bc60555185fb
> 
> Thanks,
> 
> 	Dawei

Re: [PATCH v4 0/3] rpmsg: Introduce RPMSG_CREATE_EPT_FD_IOCTL uAPI

[Dawei Li]

Hi Arnaud, 
Thanks for review.

On Wed, Jun 18, 2025 at 03:07:36PM +0200, Arnaud POULIQUEN wrote:
> Hello Dawei,
> 
> 
> Please find a few comments below. It is not clear to me which parts of your
> implementation are mandatory and which are optional "nice-to-have" optimizations.

It's more like an improvement.

> 
> Based on (potentially erroneous) hypothesis, you will find a suggestion for an
> alternative to the anonymous inode approach, which does not seem to be a common
> interface.

AFAIC, annoymous inode is a common interface and used extensivly in kernel development.
Some examples below.

> 
> 
> On 6/9/25 17:15, Dawei Li wrote:
> > Hi,
> > 
> > This is V4 of series which introduce new uAPI(RPMSG_CREATE_EPT_FD_IOCTL)
> > for rpmsg subsystem.
> > 
> > Current uAPI implementation for rpmsg ctrl & char device manipulation is
> > abstracted in procedures below:
> > - fd = open("/dev/rpmsg_ctrlX")
> > - ioctl(fd, RPMSG_CREATE_EPT_IOCTL, &info); /dev/rpmsgY devnode is
> >   generated.
> > - fd_ep = open("/dev/rpmsgY", O_RDWR) 
> > - operations on fd_ep(write, read, poll ioctl)
> > - ioctl(fd_ep, RPMSG_DESTROY_EPT_IOCTL)
> > - close(fd_ep)
> > - close(fd)
> > 
> > This /dev/rpmsgY abstraction is less favorable for:
> > - Performance issue: It's time consuming for some operations are
> > invovled:
> >   - Device node creation.
> >     Depends on specific config, especially CONFIG_DEVTMPFS, the overall
> >     overhead is based on coordination between DEVTMPFS and userspace
> >     tools such as udev and mdev.
> > 
> >   - Extra kernel-space switch cost.
> > 
> >   - Other major costs brought by heavy-weight logic like device_add().
> 
> Is this a blocker of just optimization?

Yep, performance is one of motivations of this change.

> 
> > 
> > - /dev/rpmsgY node can be opened only once. It doesn't make much sense
> >     that a dynamically created device node can be opened only once.
> 
> 
> I assume this is blocker with the fact that you need to open the /dev/rpmsg<x>
> to create the endpoint.

Yes. You have to open /dev/rpmsgX which is generated by legacy ioctl to
instantiate a new endpoint.

> 
> 
> > 
> > - For some container application such as docker, a client can't access
> >   host's dev unless specified explicitly. But in case of /dev/rpmsgY, which
> >   is generated dynamically and whose existence is unknown for clients in
> >   advance, this uAPI based on device node doesn't fit well.
> 
> does this could be solve in userspace parsing /sys/class/rpmsg/ directory to
> retreive the device?

Hardly, because client still can't access /dev/rpmsgX which is generated
by host _after_ client is launched.

> 
> You could face same kind of random instantiation for serial peripherals ( UART;
> USb, I2C,...) based on a device tree enumeration. I suppose that user space
> use to solve this.
> 
> > 
> > An anonymous inode based approach is introduced to address the issues above.
> > Rather than generating device node and opening it, rpmsg code just creates
> > an anonymous inode representing eptdev and return the fd to userspace.
> 
> A drawback is that you need to share fb passed between processes.

Fd is the abstraction of an unique endpoint device, it holds true for
both legacy and new approach.

So I guess what you mean is that /dev/rpmsgX is global to all so other process
can access it?

But /dev/rpmsgX is designed to be opened only once, it's implemented as
singleton pattern.

static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
{
...
        if (eptdev->ept) {
                mutex_unlock(&eptdev->ept_lock);
                return -EBUSY;
        }
...
        eptdev->ept = ept;
...
}

[...]
 
> > 	printf("loop[%d]\n", loop);
> > 
> > 	gettimeofday(&start, NULL);
> > 
> > 	while (loop--) {
> 
> Do you need to create /close Endpoint sevral times in your real use case with
> high timing
> constraint?

No, it's just a silly benchmark demo, large sample reduces noise statistically.

> 
> > 		fd_info.fd = -1;
> > 		fd_info.flags = O_RDWR | O_CLOEXEC | O_NONBLOCK;
> > 		ret = ioctl(fd, RPMSG_CREATE_EPT_FD_IOCTL, &fd_info);
> > 		if (ret < 0 || fd_info.fd < 0) {
> > 			printf("ioctl[RPMSG_CREATE_EPT_FD_IOCTL] failed, ret[%d]\n", ret);
> > 		}
> > 
> 
> 
> > 		ret = ioctl(fd_info.fd, RPMSG_DESTROY_EPT_IOCTL, &info);
> > 		if (ret < 0) {
> > 			printf("new ioctl[RPMSG_DESTROY_EPT_IOCTL] failed, ret[%d]\n", ret);
> > 		}
> > 
> > 		close(fd_info.fd);
> 
> It seems strange to me to use ioctl() for opening and close() for closing, from
> a symmetry point of view.

Sorry to hear that. But no, it's a pretty normal skill in kernel codebase
, I had to copy some examples from reply to other reviewer[1].

anon_inode_get_{fd,file} are used extensively in kernel for returning a new
fd to userspace which is associated with an unique data structure in kernel
space, in different ways:

- via ioctl(), some examples are:

 - KVM ioctl(s)
   - KVM_CREATE_VCPU -> kvm_vm_ioctl_create_vcpu
   - KVM_GET_STATS_FD -> kvm_vcpu_ioctl_get_stats_fd
   - KVM_CREATE_DEVICE -> kvm_ioctl_create_device
   - KVM_CREATE_VM -> kvm_dev_ioctl_create_vm

 - DMA buf/fence/sync ioctls
   - DMA_BUF_IOCTL_EXPORT_SYNC_FILE -> dma_buf_export_sync_file
   - SW_SYNC_IOC_CREATE_FENCE -> sw_sync_ioctl_create_fence
   - Couples of driver implement DMA buf by using anon file _implicitly_:
     - UDMABUF_CREATE -> udmabuf_ioctl_create
     - DMA_HEAP_IOCTL_ALLOC -> dma_heap_ioctl_allocate

 - gpiolib ioctls:
   - GPIO_GET_LINEHANDLE_IOCTL -> linehandle_create
   - GPIO_V2_GET_LINE_IOCTL

 -  IOMMUFD ioctls:

 -  VFIO Ioctls:

 - ....


- via other specific syscalls:
 - epoll_create1
 - bpf
 - perf_event_open
 - inotify_init
 - ...

[1] https://lore.kernel.org/all/20250530125008.GA5355@wendao-VirtualBox/

> 
> Regarding your implementation, I wonder if we could keep the /dev/rpmsg<x>
> device with specific open() and close() file operations associated with your new
> ioctl.
> 
> - The ioctl would create the endpoint.
> - The open() and close() operations would simply manage the file descriptor and
> increment/decrement a counter to prevent premature endpoint destruction.
> 
> 
> Regards,
> Arnaud
> 

[...]

Thanks,

	Dawei

Re: [PATCH v4 0/3] rpmsg: Introduce RPMSG_CREATE_EPT_FD_IOCTL uAPI

[Arnaud POULIQUEN]

On 6/19/25 16:43, Dawei Li wrote:
> Hi Arnaud, 
> Thanks for review.
> 
> On Wed, Jun 18, 2025 at 03:07:36PM +0200, Arnaud POULIQUEN wrote:
>> Hello Dawei,
>>
>>
>> Please find a few comments below. It is not clear to me which parts of your
>> implementation are mandatory and which are optional "nice-to-have" optimizations.
> 
> It's more like an improvement.
> 
>>
>> Based on (potentially erroneous) hypothesis, you will find a suggestion for an
>> alternative to the anonymous inode approach, which does not seem to be a common
>> interface.
> 
> AFAIC, annoymous inode is a common interface and used extensivly in kernel development.
> Some examples below.
> 
>>
>>
>> On 6/9/25 17:15, Dawei Li wrote:
>>> Hi,
>>>
>>> This is V4 of series which introduce new uAPI(RPMSG_CREATE_EPT_FD_IOCTL)
>>> for rpmsg subsystem.
>>>
>>> Current uAPI implementation for rpmsg ctrl & char device manipulation is
>>> abstracted in procedures below:
>>> - fd = open("/dev/rpmsg_ctrlX")
>>> - ioctl(fd, RPMSG_CREATE_EPT_IOCTL, &info); /dev/rpmsgY devnode is
>>>   generated.
>>> - fd_ep = open("/dev/rpmsgY", O_RDWR) 
>>> - operations on fd_ep(write, read, poll ioctl)
>>> - ioctl(fd_ep, RPMSG_DESTROY_EPT_IOCTL)
>>> - close(fd_ep)
>>> - close(fd)
>>>
>>> This /dev/rpmsgY abstraction is less favorable for:
>>> - Performance issue: It's time consuming for some operations are
>>> invovled:
>>>   - Device node creation.
>>>     Depends on specific config, especially CONFIG_DEVTMPFS, the overall
>>>     overhead is based on coordination between DEVTMPFS and userspace
>>>     tools such as udev and mdev.
>>>
>>>   - Extra kernel-space switch cost.
>>>
>>>   - Other major costs brought by heavy-weight logic like device_add().
>>
>> Is this a blocker of just optimization?
> 
> Yep, performance is one of motivations of this change.
> 
>>
>>>
>>> - /dev/rpmsgY node can be opened only once. It doesn't make much sense
>>>     that a dynamically created device node can be opened only once.
>>
>>
>> I assume this is blocker with the fact that you need to open the /dev/rpmsg<x>
>> to create the endpoint.
> 
> Yes. You have to open /dev/rpmsgX which is generated by legacy ioctl to
> instantiate a new endpoint.
> 
>>
>>
>>>
>>> - For some container application such as docker, a client can't access
>>>   host's dev unless specified explicitly. But in case of /dev/rpmsgY, which
>>>   is generated dynamically and whose existence is unknown for clients in
>>>   advance, this uAPI based on device node doesn't fit well.
>>
>> does this could be solve in userspace parsing /sys/class/rpmsg/ directory to
>> retreive the device?
> 
> Hardly, because client still can't access /dev/rpmsgX which is generated
> by host _after_ client is launched.


This part is not clear to me; could you provide more details?
I cannot figure out why a client can access /dev/rpmsg_ctrlX but not /dev/rpmsgX.


> 
>>
>> You could face same kind of random instantiation for serial peripherals ( UART;
>> USb, I2C,...) based on a device tree enumeration. I suppose that user space
>> use to solve this.
>>
>>>
>>> An anonymous inode based approach is introduced to address the issues above.
>>> Rather than generating device node and opening it, rpmsg code just creates
>>> an anonymous inode representing eptdev and return the fd to userspace.
>>
>> A drawback is that you need to share fb passed between processes.
> 
> Fd is the abstraction of an unique endpoint device, it holds true for
> both legacy and new approach.
> 
> So I guess what you mean is that /dev/rpmsgX is global to all so other process
> can access it?
> 
> But /dev/rpmsgX is designed to be opened only once, it's implemented as
> singleton pattern.
> 
> static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
> {
> ...
>         if (eptdev->ept) {
>                 mutex_unlock(&eptdev->ept_lock);
>                 return -EBUSY;
>         }
> ...
>         eptdev->ept = ept;
> ...
> }
> 
> [...]
>  
>>> 	printf("loop[%d]\n", loop);
>>>
>>> 	gettimeofday(&start, NULL);
>>>
>>> 	while (loop--) {
>>
>> Do you need to create /close Endpoint sevral times in your real use case with
>> high timing
>> constraint?
> 
> No, it's just a silly benchmark demo, large sample reduces noise statistically.
> 
>>
>>> 		fd_info.fd = -1;
>>> 		fd_info.flags = O_RDWR | O_CLOEXEC | O_NONBLOCK;
>>> 		ret = ioctl(fd, RPMSG_CREATE_EPT_FD_IOCTL, &fd_info);
>>> 		if (ret < 0 || fd_info.fd < 0) {
>>> 			printf("ioctl[RPMSG_CREATE_EPT_FD_IOCTL] failed, ret[%d]\n", ret);
>>> 		}
>>>
>>
>>
>>> 		ret = ioctl(fd_info.fd, RPMSG_DESTROY_EPT_IOCTL, &info);
>>> 		if (ret < 0) {
>>> 			printf("new ioctl[RPMSG_DESTROY_EPT_IOCTL] failed, ret[%d]\n", ret);
>>> 		}
>>>
>>> 		close(fd_info.fd);
>>
>> It seems strange to me to use ioctl() for opening and close() for closing, from
>> a symmetry point of view.
> 
> Sorry to hear that. But no, it's a pretty normal skill in kernel codebase
> , I had to copy some examples from reply to other reviewer[1].

I missed this one, apologize for the duplication.

> 
> anon_inode_get_{fd,file} are used extensively in kernel for returning a new
> fd to userspace which is associated with an unique data structure in kernel
> space, in different ways:
> 
> - via ioctl(), some examples are:
> 
>  - KVM ioctl(s)
>    - KVM_CREATE_VCPU -> kvm_vm_ioctl_create_vcpu
>    - KVM_GET_STATS_FD -> kvm_vcpu_ioctl_get_stats_fd
>    - KVM_CREATE_DEVICE -> kvm_ioctl_create_device
>    - KVM_CREATE_VM -> kvm_dev_ioctl_create_vm
> 
>  - DMA buf/fence/sync ioctls
>    - DMA_BUF_IOCTL_EXPORT_SYNC_FILE -> dma_buf_export_sync_file
>    - SW_SYNC_IOC_CREATE_FENCE -> sw_sync_ioctl_create_fence
>    - Couples of driver implement DMA buf by using anon file _implicitly_:
>      - UDMABUF_CREATE -> udmabuf_ioctl_create
>      - DMA_HEAP_IOCTL_ALLOC -> dma_heap_ioctl_allocate
> 
>  - gpiolib ioctls:
>    - GPIO_GET_LINEHANDLE_IOCTL -> linehandle_create
>    - GPIO_V2_GET_LINE_IOCTL
> 
>  -  IOMMUFD ioctls:
> 
>  -  VFIO Ioctls:
> 
>  - ....
> 
> 
> - via other specific syscalls:
>  - epoll_create1
>  - bpf
>  - perf_event_open
>  - inotify_init
>  - ...

If we put the optimization aspect aside, what seems strange to me is that the
purpose of rpmsg_char was to expose a FS character device to user space. If we
need tobypass the use of /dev/rpmsgX, does it make sense to support an anonymous
inode in this driver?  I am clearly not legitimate to answer this question...


Thanks,
Arnaud

> 
> [1] https://lore.kernel.org/all/20250530125008.GA5355@wendao-VirtualBox/
> 
>>
>> Regarding your implementation, I wonder if we could keep the /dev/rpmsg<x>
>> device with specific open() and close() file operations associated with your new
>> ioctl.
>>
>> - The ioctl would create the endpoint.
>> - The open() and close() operations would simply manage the file descriptor and
>> increment/decrement a counter to prevent premature endpoint destruction.
>>
>>
>> Regards,
>> Arnaud
>>
> 
> [...]
> 
> Thanks,
> 
> 	Dawei

Re: [PATCH v4 0/3] rpmsg: Introduce RPMSG_CREATE_EPT_FD_IOCTL uAPI

[Dawei Li]

Hi Arnaud,

Thanks for the reply.

On Fri, Jun 20, 2025 at 09:52:03AM +0200, Arnaud POULIQUEN wrote:
> 
> 
> On 6/19/25 16:43, Dawei Li wrote:
> > Hi Arnaud, 
> > Thanks for review.
> > 
> > On Wed, Jun 18, 2025 at 03:07:36PM +0200, Arnaud POULIQUEN wrote:
> >> Hello Dawei,
> >>
> >>
> >> Please find a few comments below. It is not clear to me which parts of your
> >> implementation are mandatory and which are optional "nice-to-have" optimizations.
> > 
> > It's more like an improvement.
> > 
> >>
> >> Based on (potentially erroneous) hypothesis, you will find a suggestion for an
> >> alternative to the anonymous inode approach, which does not seem to be a common
> >> interface.
> > 
> > AFAIC, annoymous inode is a common interface and used extensivly in kernel development.
> > Some examples below.
> > 
> >>
> >>
> >> On 6/9/25 17:15, Dawei Li wrote:
> >>> Hi,
> >>>
> >>> This is V4 of series which introduce new uAPI(RPMSG_CREATE_EPT_FD_IOCTL)
> >>> for rpmsg subsystem.
> >>>
> >>> Current uAPI implementation for rpmsg ctrl & char device manipulation is
> >>> abstracted in procedures below:
> >>> - fd = open("/dev/rpmsg_ctrlX")
> >>> - ioctl(fd, RPMSG_CREATE_EPT_IOCTL, &info); /dev/rpmsgY devnode is
> >>>   generated.
> >>> - fd_ep = open("/dev/rpmsgY", O_RDWR) 
> >>> - operations on fd_ep(write, read, poll ioctl)
> >>> - ioctl(fd_ep, RPMSG_DESTROY_EPT_IOCTL)
> >>> - close(fd_ep)
> >>> - close(fd)
> >>>
> >>> This /dev/rpmsgY abstraction is less favorable for:
> >>> - Performance issue: It's time consuming for some operations are
> >>> invovled:
> >>>   - Device node creation.
> >>>     Depends on specific config, especially CONFIG_DEVTMPFS, the overall
> >>>     overhead is based on coordination between DEVTMPFS and userspace
> >>>     tools such as udev and mdev.
> >>>
> >>>   - Extra kernel-space switch cost.
> >>>
> >>>   - Other major costs brought by heavy-weight logic like device_add().
> >>
> >> Is this a blocker of just optimization?
> > 
> > Yep, performance is one of motivations of this change.
> > 
> >>
> >>>
> >>> - /dev/rpmsgY node can be opened only once. It doesn't make much sense
> >>>     that a dynamically created device node can be opened only once.
> >>
> >>
> >> I assume this is blocker with the fact that you need to open the /dev/rpmsg<x>
> >> to create the endpoint.
> > 
> > Yes. You have to open /dev/rpmsgX which is generated by legacy ioctl to
> > instantiate a new endpoint.
> > 
> >>
> >>
> >>>
> >>> - For some container application such as docker, a client can't access
> >>>   host's dev unless specified explicitly. But in case of /dev/rpmsgY, which
> >>>   is generated dynamically and whose existence is unknown for clients in
> >>>   advance, this uAPI based on device node doesn't fit well.
> >>
> >> does this could be solve in userspace parsing /sys/class/rpmsg/ directory to
> >> retreive the device?
> > 
> > Hardly, because client still can't access /dev/rpmsgX which is generated
> > by host _after_ client is launched.
> 
> 
> This part is not clear to me; could you provide more details?
> I cannot figure out why a client can access /dev/rpmsg_ctrlX but not /dev/rpmsgX.

Well, let's take docker as example:

For docker, when a client is launched and it wants to access host's
device, it must make explicit request when it's launched:

docker run --device=/dev/xxx

Let's presume that xxx is /dev/rpmsgX generated dynamically by _host_.
Docker command above knows nothing about these rpmsg nodes which are
generated by host _after_ client is launched. And yes, parsing
/sys/class/rpmsg may acquire info about rpmsg devices, but client still
can't access /dev/rpmsgX.

> 
> 
> > 
> >>
> >> You could face same kind of random instantiation for serial peripherals ( UART;
> >> USb, I2C,...) based on a device tree enumeration. I suppose that user space
> >> use to solve this.
> >>
> >>>
> >>> An anonymous inode based approach is introduced to address the issues above.
> >>> Rather than generating device node and opening it, rpmsg code just creates
> >>> an anonymous inode representing eptdev and return the fd to userspace.
> >>
> >> A drawback is that you need to share fb passed between processes.
> > 
> > Fd is the abstraction of an unique endpoint device, it holds true for
> > both legacy and new approach.
> > 
> > So I guess what you mean is that /dev/rpmsgX is global to all so other process
> > can access it?
> > 
> > But /dev/rpmsgX is designed to be opened only once, it's implemented as
> > singleton pattern.
> > 
> > static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
> > {
> > ...
> >         if (eptdev->ept) {
> >                 mutex_unlock(&eptdev->ept_lock);
> >                 return -EBUSY;
> >         }
> > ...
> >         eptdev->ept = ept;
> > ...
> > }
> > 
> > [...]
> >  
> >>> 	printf("loop[%d]\n", loop);
> >>>
> >>> 	gettimeofday(&start, NULL);
> >>>
> >>> 	while (loop--) {
> >>
> >> Do you need to create /close Endpoint sevral times in your real use case with
> >> high timing
> >> constraint?
> > 
> > No, it's just a silly benchmark demo, large sample reduces noise statistically.
> > 
> >>
> >>> 		fd_info.fd = -1;
> >>> 		fd_info.flags = O_RDWR | O_CLOEXEC | O_NONBLOCK;
> >>> 		ret = ioctl(fd, RPMSG_CREATE_EPT_FD_IOCTL, &fd_info);
> >>> 		if (ret < 0 || fd_info.fd < 0) {
> >>> 			printf("ioctl[RPMSG_CREATE_EPT_FD_IOCTL] failed, ret[%d]\n", ret);
> >>> 		}
> >>>
> >>
> >>
> >>> 		ret = ioctl(fd_info.fd, RPMSG_DESTROY_EPT_IOCTL, &info);
> >>> 		if (ret < 0) {
> >>> 			printf("new ioctl[RPMSG_DESTROY_EPT_IOCTL] failed, ret[%d]\n", ret);
> >>> 		}
> >>>
> >>> 		close(fd_info.fd);
> >>
> >> It seems strange to me to use ioctl() for opening and close() for closing, from
> >> a symmetry point of view.
> > 
> > Sorry to hear that. But no, it's a pretty normal skill in kernel codebase
> > , I had to copy some examples from reply to other reviewer[1].
> 
> I missed this one, apologize for the duplication.
> 
> > 
> > anon_inode_get_{fd,file} are used extensively in kernel for returning a new
> > fd to userspace which is associated with an unique data structure in kernel
> > space, in different ways:
> > 
> > - via ioctl(), some examples are:
> > 
> >  - KVM ioctl(s)
> >    - KVM_CREATE_VCPU -> kvm_vm_ioctl_create_vcpu
> >    - KVM_GET_STATS_FD -> kvm_vcpu_ioctl_get_stats_fd
> >    - KVM_CREATE_DEVICE -> kvm_ioctl_create_device
> >    - KVM_CREATE_VM -> kvm_dev_ioctl_create_vm
> > 
> >  - DMA buf/fence/sync ioctls
> >    - DMA_BUF_IOCTL_EXPORT_SYNC_FILE -> dma_buf_export_sync_file
> >    - SW_SYNC_IOC_CREATE_FENCE -> sw_sync_ioctl_create_fence
> >    - Couples of driver implement DMA buf by using anon file _implicitly_:
> >      - UDMABUF_CREATE -> udmabuf_ioctl_create
> >      - DMA_HEAP_IOCTL_ALLOC -> dma_heap_ioctl_allocate
> > 
> >  - gpiolib ioctls:
> >    - GPIO_GET_LINEHANDLE_IOCTL -> linehandle_create
> >    - GPIO_V2_GET_LINE_IOCTL
> > 
> >  -  IOMMUFD ioctls:
> > 
> >  -  VFIO Ioctls:
> > 
> >  - ....
> > 
> > 
> > - via other specific syscalls:
> >  - epoll_create1
> >  - bpf
> >  - perf_event_open
> >  - inotify_init
> >  - ...
> 
> If we put the optimization aspect aside, what seems strange to me is that the
> purpose of rpmsg_char was to expose a FS character device to user space. If we
> need tobypass the use of /dev/rpmsgX, does it make sense to support an anonymous
> inode in this driver?  I am clearly not legitimate to answer this question...

You have every right to do so, after all, it's purely a technical
discussion :).

I admit it's bit confusing to add annoymous inode logic to a file named
rpmsg_char.c which implies 'character' device. That's why I rename API
following Mathieu's comment:
  - __rpmsg_chrdev_eptdev_alloc ->  rpmsg_eptdev_alloc
  - __rpmsg_chrdev_eptdev_add ->  rpmsg_eptdev_add

As to topic how these two uAPI(s) co-exist and affect each other. This
change is based on rules:

1. Never break existing uAPI.
2. Try best to reuse existing codebase.
3. Userspace can choose whatever approach they want to.

Thanks,

	Dawei
> 
> 
> Thanks,
> Arnaud
> 
> > 
> > [1] https://lore.kernel.org/all/20250530125008.GA5355@wendao-VirtualBox/
> > 
> >>
> >> Regarding your implementation, I wonder if we could keep the /dev/rpmsg<x>
> >> device with specific open() and close() file operations associated with your new
> >> ioctl.
> >>
> >> - The ioctl would create the endpoint.
> >> - The open() and close() operations would simply manage the file descriptor and
> >> increment/decrement a counter to prevent premature endpoint destruction.
> >>
> >>
> >> Regards,
> >> Arnaud
> >>
> > 
> > [...]
> > 
> > Thanks,
> > 
> > 	Dawei

Re: [PATCH v4 0/3] rpmsg: Introduce RPMSG_CREATE_EPT_FD_IOCTL uAPI

[Arnaud POULIQUEN]

Hello Dawei,

Sorry for the late answer.

On 6/22/25 06:12, Dawei Li wrote:
> Hi Arnaud,
> 
> Thanks for the reply.
> 
> On Fri, Jun 20, 2025 at 09:52:03AM +0200, Arnaud POULIQUEN wrote:
>>
>>
>> On 6/19/25 16:43, Dawei Li wrote:
>>> Hi Arnaud, 
>>> Thanks for review.
>>>
>>> On Wed, Jun 18, 2025 at 03:07:36PM +0200, Arnaud POULIQUEN wrote:
>>>> Hello Dawei,
>>>>
>>>>
>>>> Please find a few comments below. It is not clear to me which parts of your
>>>> implementation are mandatory and which are optional "nice-to-have" optimizations.
>>>
>>> It's more like an improvement.
>>>
>>>>
>>>> Based on (potentially erroneous) hypothesis, you will find a suggestion for an
>>>> alternative to the anonymous inode approach, which does not seem to be a common
>>>> interface.
>>>
>>> AFAIC, annoymous inode is a common interface and used extensivly in kernel development.
>>> Some examples below.
>>>
>>>>
>>>>
>>>> On 6/9/25 17:15, Dawei Li wrote:
>>>>> Hi,
>>>>>
>>>>> This is V4 of series which introduce new uAPI(RPMSG_CREATE_EPT_FD_IOCTL)
>>>>> for rpmsg subsystem.
>>>>>
>>>>> Current uAPI implementation for rpmsg ctrl & char device manipulation is
>>>>> abstracted in procedures below:
>>>>> - fd = open("/dev/rpmsg_ctrlX")
>>>>> - ioctl(fd, RPMSG_CREATE_EPT_IOCTL, &info); /dev/rpmsgY devnode is
>>>>>   generated.
>>>>> - fd_ep = open("/dev/rpmsgY", O_RDWR) 
>>>>> - operations on fd_ep(write, read, poll ioctl)
>>>>> - ioctl(fd_ep, RPMSG_DESTROY_EPT_IOCTL)
>>>>> - close(fd_ep)
>>>>> - close(fd)
>>>>>
>>>>> This /dev/rpmsgY abstraction is less favorable for:
>>>>> - Performance issue: It's time consuming for some operations are
>>>>> invovled:
>>>>>   - Device node creation.
>>>>>     Depends on specific config, especially CONFIG_DEVTMPFS, the overall
>>>>>     overhead is based on coordination between DEVTMPFS and userspace
>>>>>     tools such as udev and mdev.
>>>>>
>>>>>   - Extra kernel-space switch cost.
>>>>>
>>>>>   - Other major costs brought by heavy-weight logic like device_add().
>>>>
>>>> Is this a blocker of just optimization?
>>>
>>> Yep, performance is one of motivations of this change.
>>>
>>>>
>>>>>
>>>>> - /dev/rpmsgY node can be opened only once. It doesn't make much sense
>>>>>     that a dynamically created device node can be opened only once.
>>>>
>>>>
>>>> I assume this is blocker with the fact that you need to open the /dev/rpmsg<x>
>>>> to create the endpoint.
>>>
>>> Yes. You have to open /dev/rpmsgX which is generated by legacy ioctl to
>>> instantiate a new endpoint.
>>>
>>>>
>>>>
>>>>>
>>>>> - For some container application such as docker, a client can't access
>>>>>   host's dev unless specified explicitly. But in case of /dev/rpmsgY, which
>>>>>   is generated dynamically and whose existence is unknown for clients in
>>>>>   advance, this uAPI based on device node doesn't fit well.
>>>>
>>>> does this could be solve in userspace parsing /sys/class/rpmsg/ directory to
>>>> retreive the device?
>>>
>>> Hardly, because client still can't access /dev/rpmsgX which is generated
>>> by host _after_ client is launched.
>>
>>
>> This part is not clear to me; could you provide more details?
>> I cannot figure out why a client can access /dev/rpmsg_ctrlX but not /dev/rpmsgX.
> 
> Well, let's take docker as example:

> 
> For docker, when a client is launched and it wants to access host's
> device, it must make explicit request when it's launched:
> 
> docker run --device=/dev/xxx
> 
> Let's presume that xxx is /dev/rpmsgX generated dynamically by _host_.
> Docker command above knows nothing about these rpmsg nodes which are
> generated by host _after_ client is launched. And yes, parsing> /sys/class/rpmsg may acquire info about rpmsg devices, but client still
> can't access /dev/rpmsgX.

One extra question:Are you using RPMsg over virtio?

If yes, do you test the RPMsg name service (NS) announcement, that might also
address your needs.

The principle is that the remote processor sends a name service announcement to
Linux, which probes the rpmsg character device and creates the /dev/rpmsgX
device in a predefined order known by the remote processor.
In such a case, the /dev/rpmsgX usage would be determined by the remote
processor itself.

Another advantage is that the RPMsg channel creation is not driven by either the
host or the client. In such case host does no need to define/kwnow RPMSg
endpoint addresses.

You still need to call the open() file system operation, but this should be done
one time during Docker client initialization.

Regards
Arnaud


> 
>>
>>
>>>
>>>>
>>>> You could face same kind of random instantiation for serial peripherals ( UART;
>>>> USb, I2C,...) based on a device tree enumeration. I suppose that user space
>>>> use to solve this.
>>>>
>>>>>
>>>>> An anonymous inode based approach is introduced to address the issues above.
>>>>> Rather than generating device node and opening it, rpmsg code just creates
>>>>> an anonymous inode representing eptdev and return the fd to userspace.
>>>>
>>>> A drawback is that you need to share fb passed between processes.
>>>
>>> Fd is the abstraction of an unique endpoint device, it holds true for
>>> both legacy and new approach.
>>>
>>> So I guess what you mean is that /dev/rpmsgX is global to all so other process
>>> can access it?
>>>
>>> But /dev/rpmsgX is designed to be opened only once, it's implemented as
>>> singleton pattern.
>>>
>>> static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
>>> {
>>> ...
>>>         if (eptdev->ept) {
>>>                 mutex_unlock(&eptdev->ept_lock);
>>>                 return -EBUSY;
>>>         }
>>> ...
>>>         eptdev->ept = ept;
>>> ...
>>> }
>>>
>>> [...]
>>>  
>>>>> 	printf("loop[%d]\n", loop);
>>>>>
>>>>> 	gettimeofday(&start, NULL);
>>>>>
>>>>> 	while (loop--) {
>>>>
>>>> Do you need to create /close Endpoint sevral times in your real use case with
>>>> high timing
>>>> constraint?
>>>
>>> No, it's just a silly benchmark demo, large sample reduces noise statistically.
>>>
>>>>
>>>>> 		fd_info.fd = -1;
>>>>> 		fd_info.flags = O_RDWR | O_CLOEXEC | O_NONBLOCK;
>>>>> 		ret = ioctl(fd, RPMSG_CREATE_EPT_FD_IOCTL, &fd_info);
>>>>> 		if (ret < 0 || fd_info.fd < 0) {
>>>>> 			printf("ioctl[RPMSG_CREATE_EPT_FD_IOCTL] failed, ret[%d]\n", ret);
>>>>> 		}
>>>>>
>>>>
>>>>
>>>>> 		ret = ioctl(fd_info.fd, RPMSG_DESTROY_EPT_IOCTL, &info);
>>>>> 		if (ret < 0) {
>>>>> 			printf("new ioctl[RPMSG_DESTROY_EPT_IOCTL] failed, ret[%d]\n", ret);
>>>>> 		}
>>>>>
>>>>> 		close(fd_info.fd);
>>>>
>>>> It seems strange to me to use ioctl() for opening and close() for closing, from
>>>> a symmetry point of view.
>>>
>>> Sorry to hear that. But no, it's a pretty normal skill in kernel codebase
>>> , I had to copy some examples from reply to other reviewer[1].
>>
>> I missed this one, apologize for the duplication.
>>
>>>
>>> anon_inode_get_{fd,file} are used extensively in kernel for returning a new
>>> fd to userspace which is associated with an unique data structure in kernel
>>> space, in different ways:
>>>
>>> - via ioctl(), some examples are:
>>>
>>>  - KVM ioctl(s)
>>>    - KVM_CREATE_VCPU -> kvm_vm_ioctl_create_vcpu
>>>    - KVM_GET_STATS_FD -> kvm_vcpu_ioctl_get_stats_fd
>>>    - KVM_CREATE_DEVICE -> kvm_ioctl_create_device
>>>    - KVM_CREATE_VM -> kvm_dev_ioctl_create_vm
>>>
>>>  - DMA buf/fence/sync ioctls
>>>    - DMA_BUF_IOCTL_EXPORT_SYNC_FILE -> dma_buf_export_sync_file
>>>    - SW_SYNC_IOC_CREATE_FENCE -> sw_sync_ioctl_create_fence
>>>    - Couples of driver implement DMA buf by using anon file _implicitly_:
>>>      - UDMABUF_CREATE -> udmabuf_ioctl_create
>>>      - DMA_HEAP_IOCTL_ALLOC -> dma_heap_ioctl_allocate
>>>
>>>  - gpiolib ioctls:
>>>    - GPIO_GET_LINEHANDLE_IOCTL -> linehandle_create
>>>    - GPIO_V2_GET_LINE_IOCTL
>>>
>>>  -  IOMMUFD ioctls:
>>>
>>>  -  VFIO Ioctls:
>>>
>>>  - ....
>>>
>>>
>>> - via other specific syscalls:
>>>  - epoll_create1
>>>  - bpf
>>>  - perf_event_open
>>>  - inotify_init
>>>  - ...
>>
>> If we put the optimization aspect aside, what seems strange to me is that the
>> purpose of rpmsg_char was to expose a FS character device to user space. If we
>> need tobypass the use of /dev/rpmsgX, does it make sense to support an anonymous
>> inode in this driver?  I am clearly not legitimate to answer this question...
> 
> You have every right to do so, after all, it's purely a technical
> discussion :).
> 
> I admit it's bit confusing to add annoymous inode logic to a file named
> rpmsg_char.c which implies 'character' device. That's why I rename API
> following Mathieu's comment:
>   - __rpmsg_chrdev_eptdev_alloc ->  rpmsg_eptdev_alloc
>   - __rpmsg_chrdev_eptdev_add ->  rpmsg_eptdev_add
> 
> As to topic how these two uAPI(s) co-exist and affect each other. This
> change is based on rules:
> 
> 1. Never break existing uAPI.
> 2. Try best to reuse existing codebase.
> 3. Userspace can choose whatever approach they want to.
> 
> Thanks,
> 
> 	Dawei
>>
>>
>> Thanks,
>> Arnaud
>>
>>>
>>> [1] https://lore.kernel.org/all/20250530125008.GA5355@wendao-VirtualBox/
>>>
>>>>
>>>> Regarding your implementation, I wonder if we could keep the /dev/rpmsg<x>
>>>> device with specific open() and close() file operations associated with your new
>>>> ioctl.
>>>>
>>>> - The ioctl would create the endpoint.
>>>> - The open() and close() operations would simply manage the file descriptor and
>>>> increment/decrement a counter to prevent premature endpoint destruction.
>>>>
>>>>
>>>> Regards,
>>>> Arnaud
>>>>
>>>
>>> [...]
>>>
>>> Thanks,
>>>
>>> 	Dawei

Re: [PATCH v4 0/3] rpmsg: Introduce RPMSG_CREATE_EPT_FD_IOCTL uAPI

[Dawei Li]

Hi Arnaud,

Thanks for the reply.

On Mon, Jun 30, 2025 at 09:54:40AM +0200, Arnaud POULIQUEN wrote:
> Hello Dawei,
> 
> Sorry for the late answer.
> 
> On 6/22/25 06:12, Dawei Li wrote:
> > Hi Arnaud,
> > 
> > Thanks for the reply.
> > 
> > On Fri, Jun 20, 2025 at 09:52:03AM +0200, Arnaud POULIQUEN wrote:
> >>
> >>
> >> On 6/19/25 16:43, Dawei Li wrote:
> >>> Hi Arnaud, 
> >>> Thanks for review.
> >>>
> >>> On Wed, Jun 18, 2025 at 03:07:36PM +0200, Arnaud POULIQUEN wrote:
> >>>> Hello Dawei,
> >>>>
> >>>>
> >>>> Please find a few comments below. It is not clear to me which parts of your
> >>>> implementation are mandatory and which are optional "nice-to-have" optimizations.
> >>>
> >>> It's more like an improvement.
> >>>
> >>>>
> >>>> Based on (potentially erroneous) hypothesis, you will find a suggestion for an
> >>>> alternative to the anonymous inode approach, which does not seem to be a common
> >>>> interface.
> >>>
> >>> AFAIC, annoymous inode is a common interface and used extensivly in kernel development.
> >>> Some examples below.
> >>>
> >>>>
> >>>>
> >>>> On 6/9/25 17:15, Dawei Li wrote:
> >>>>> Hi,
> >>>>>
> >>>>> This is V4 of series which introduce new uAPI(RPMSG_CREATE_EPT_FD_IOCTL)
> >>>>> for rpmsg subsystem.
> >>>>>
> >>>>> Current uAPI implementation for rpmsg ctrl & char device manipulation is
> >>>>> abstracted in procedures below:
> >>>>> - fd = open("/dev/rpmsg_ctrlX")
> >>>>> - ioctl(fd, RPMSG_CREATE_EPT_IOCTL, &info); /dev/rpmsgY devnode is
> >>>>>   generated.
> >>>>> - fd_ep = open("/dev/rpmsgY", O_RDWR) 
> >>>>> - operations on fd_ep(write, read, poll ioctl)
> >>>>> - ioctl(fd_ep, RPMSG_DESTROY_EPT_IOCTL)
> >>>>> - close(fd_ep)
> >>>>> - close(fd)
> >>>>>
> >>>>> This /dev/rpmsgY abstraction is less favorable for:
> >>>>> - Performance issue: It's time consuming for some operations are
> >>>>> invovled:
> >>>>>   - Device node creation.
> >>>>>     Depends on specific config, especially CONFIG_DEVTMPFS, the overall
> >>>>>     overhead is based on coordination between DEVTMPFS and userspace
> >>>>>     tools such as udev and mdev.
> >>>>>
> >>>>>   - Extra kernel-space switch cost.
> >>>>>
> >>>>>   - Other major costs brought by heavy-weight logic like device_add().
> >>>>
> >>>> Is this a blocker of just optimization?
> >>>
> >>> Yep, performance is one of motivations of this change.
> >>>
> >>>>
> >>>>>
> >>>>> - /dev/rpmsgY node can be opened only once. It doesn't make much sense
> >>>>>     that a dynamically created device node can be opened only once.
> >>>>
> >>>>
> >>>> I assume this is blocker with the fact that you need to open the /dev/rpmsg<x>
> >>>> to create the endpoint.
> >>>
> >>> Yes. You have to open /dev/rpmsgX which is generated by legacy ioctl to
> >>> instantiate a new endpoint.
> >>>
> >>>>
> >>>>
> >>>>>
> >>>>> - For some container application such as docker, a client can't access
> >>>>>   host's dev unless specified explicitly. But in case of /dev/rpmsgY, which
> >>>>>   is generated dynamically and whose existence is unknown for clients in
> >>>>>   advance, this uAPI based on device node doesn't fit well.
> >>>>
> >>>> does this could be solve in userspace parsing /sys/class/rpmsg/ directory to
> >>>> retreive the device?
> >>>
> >>> Hardly, because client still can't access /dev/rpmsgX which is generated
> >>> by host _after_ client is launched.
> >>
> >>
> >> This part is not clear to me; could you provide more details?
> >> I cannot figure out why a client can access /dev/rpmsg_ctrlX but not /dev/rpmsgX.
> > 
> > Well, let's take docker as example:
> 
> > 
> > For docker, when a client is launched and it wants to access host's
> > device, it must make explicit request when it's launched:
> > 
> > docker run --device=/dev/xxx
> > 
> > Let's presume that xxx is /dev/rpmsgX generated dynamically by _host_.
> > Docker command above knows nothing about these rpmsg nodes which are
> > generated by host _after_ client is launched. And yes, parsing> /sys/class/rpmsg may acquire info about rpmsg devices, but client still
> > can't access /dev/rpmsgX.
> 
> One extra question:Are you using RPMsg over virtio?
> 
> If yes, do you test the RPMsg name service (NS) announcement, that might also
> address your needs.
> 
> The principle is that the remote processor sends a name service announcement to
> Linux, which probes the rpmsg character device and creates the /dev/rpmsgX
> device in a predefined order known by the remote processor.
> In such a case, the /dev/rpmsgX usage would be determined by the remote
> processor itself.
> 
> Another advantage is that the RPMsg channel creation is not driven by either the
> host or the client. In such case host does no need to define/kwnow RPMSg
> endpoint addresses.
> 
> You still need to call the open() file system operation, but this should be done
> one time during Docker client initialization.

NS is nice, but perhaps it's not the approach for some cases.

For offloading/accelerator scenarios, ACPU is responsible for making all
the important decisions, including creations of endpoints. Because all
the user-awared software stack is running on ACPU, and if you want to
create a endpoint _dynamically_, it must be from user's command which is
from ACPU.

And this series is more about how rpmsg_char and rpmsg_ctrl coordinate
themselves about creating dynamic rpmsg endpoints in a more simple
and efficient way.

And the whole point of series is "When you want to return a fd to
userspace which represents an instance of data structure in kernel, you
don't implement it as character device". Maybe some quotes from Christian[1]
can describe it better[1]:

"I'm not sure why people are so in love with character device based apis.
It's terrible. It glues everything to devtmpfs which isn't namespacable
in any way. It's terrible to delegate and extremely restrictive in terms
of extensiblity if you need additional device entries (aka the loop
driver folly)."

[1] https://lkml.org/lkml/2025/6/24/639

Thanks,

	Dawei

> 
> Regards
> Arnaud
> 
> 
> > 
> >>
> >>
> >>>
> >>>>
> >>>> You could face same kind of random instantiation for serial peripherals ( UART;
> >>>> USb, I2C,...) based on a device tree enumeration. I suppose that user space
> >>>> use to solve this.
> >>>>
> >>>>>
> >>>>> An anonymous inode based approach is introduced to address the issues above.
> >>>>> Rather than generating device node and opening it, rpmsg code just creates
> >>>>> an anonymous inode representing eptdev and return the fd to userspace.
> >>>>
> >>>> A drawback is that you need to share fb passed between processes.
> >>>
> >>> Fd is the abstraction of an unique endpoint device, it holds true for
> >>> both legacy and new approach.
> >>>
> >>> So I guess what you mean is that /dev/rpmsgX is global to all so other process
> >>> can access it?
> >>>
> >>> But /dev/rpmsgX is designed to be opened only once, it's implemented as
> >>> singleton pattern.
> >>>
> >>> static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
> >>> {
> >>> ...
> >>>         if (eptdev->ept) {
> >>>                 mutex_unlock(&eptdev->ept_lock);
> >>>                 return -EBUSY;
> >>>         }
> >>> ...
> >>>         eptdev->ept = ept;
> >>> ...
> >>> }
> >>>
> >>> [...]
> >>>  
> >>>>> 	printf("loop[%d]\n", loop);
> >>>>>
> >>>>> 	gettimeofday(&start, NULL);
> >>>>>
> >>>>> 	while (loop--) {
> >>>>
> >>>> Do you need to create /close Endpoint sevral times in your real use case with
> >>>> high timing
> >>>> constraint?
> >>>
> >>> No, it's just a silly benchmark demo, large sample reduces noise statistically.
> >>>
> >>>>
> >>>>> 		fd_info.fd = -1;
> >>>>> 		fd_info.flags = O_RDWR | O_CLOEXEC | O_NONBLOCK;
> >>>>> 		ret = ioctl(fd, RPMSG_CREATE_EPT_FD_IOCTL, &fd_info);
> >>>>> 		if (ret < 0 || fd_info.fd < 0) {
> >>>>> 			printf("ioctl[RPMSG_CREATE_EPT_FD_IOCTL] failed, ret[%d]\n", ret);
> >>>>> 		}
> >>>>>
> >>>>
> >>>>
> >>>>> 		ret = ioctl(fd_info.fd, RPMSG_DESTROY_EPT_IOCTL, &info);
> >>>>> 		if (ret < 0) {
> >>>>> 			printf("new ioctl[RPMSG_DESTROY_EPT_IOCTL] failed, ret[%d]\n", ret);
> >>>>> 		}
> >>>>>
> >>>>> 		close(fd_info.fd);
> >>>>
> >>>> It seems strange to me to use ioctl() for opening and close() for closing, from
> >>>> a symmetry point of view.
> >>>
> >>> Sorry to hear that. But no, it's a pretty normal skill in kernel codebase
> >>> , I had to copy some examples from reply to other reviewer[1].
> >>
> >> I missed this one, apologize for the duplication.
> >>
> >>>
> >>> anon_inode_get_{fd,file} are used extensively in kernel for returning a new
> >>> fd to userspace which is associated with an unique data structure in kernel
> >>> space, in different ways:
> >>>
> >>> - via ioctl(), some examples are:
> >>>
> >>>  - KVM ioctl(s)
> >>>    - KVM_CREATE_VCPU -> kvm_vm_ioctl_create_vcpu
> >>>    - KVM_GET_STATS_FD -> kvm_vcpu_ioctl_get_stats_fd
> >>>    - KVM_CREATE_DEVICE -> kvm_ioctl_create_device
> >>>    - KVM_CREATE_VM -> kvm_dev_ioctl_create_vm
> >>>
> >>>  - DMA buf/fence/sync ioctls
> >>>    - DMA_BUF_IOCTL_EXPORT_SYNC_FILE -> dma_buf_export_sync_file
> >>>    - SW_SYNC_IOC_CREATE_FENCE -> sw_sync_ioctl_create_fence
> >>>    - Couples of driver implement DMA buf by using anon file _implicitly_:
> >>>      - UDMABUF_CREATE -> udmabuf_ioctl_create
> >>>      - DMA_HEAP_IOCTL_ALLOC -> dma_heap_ioctl_allocate
> >>>
> >>>  - gpiolib ioctls:
> >>>    - GPIO_GET_LINEHANDLE_IOCTL -> linehandle_create
> >>>    - GPIO_V2_GET_LINE_IOCTL
> >>>
> >>>  -  IOMMUFD ioctls:
> >>>
> >>>  -  VFIO Ioctls:
> >>>
> >>>  - ....
> >>>
> >>>
> >>> - via other specific syscalls:
> >>>  - epoll_create1
> >>>  - bpf
> >>>  - perf_event_open
> >>>  - inotify_init
> >>>  - ...
> >>
> >> If we put the optimization aspect aside, what seems strange to me is that the
> >> purpose of rpmsg_char was to expose a FS character device to user space. If we
> >> need tobypass the use of /dev/rpmsgX, does it make sense to support an anonymous
> >> inode in this driver?  I am clearly not legitimate to answer this question...
> > 
> > You have every right to do so, after all, it's purely a technical
> > discussion :).
> > 
> > I admit it's bit confusing to add annoymous inode logic to a file named
> > rpmsg_char.c which implies 'character' device. That's why I rename API
> > following Mathieu's comment:
> >   - __rpmsg_chrdev_eptdev_alloc ->  rpmsg_eptdev_alloc
> >   - __rpmsg_chrdev_eptdev_add ->  rpmsg_eptdev_add
> > 
> > As to topic how these two uAPI(s) co-exist and affect each other. This
> > change is based on rules:
> > 
> > 1. Never break existing uAPI.
> > 2. Try best to reuse existing codebase.
> > 3. Userspace can choose whatever approach they want to.
> > 
> > Thanks,
> > 
> > 	Dawei
> >>
> >>
> >> Thanks,
> >> Arnaud
> >>
> >>>
> >>> [1] https://lore.kernel.org/all/20250530125008.GA5355@wendao-VirtualBox/
> >>>
> >>>>
> >>>> Regarding your implementation, I wonder if we could keep the /dev/rpmsg<x>
> >>>> device with specific open() and close() file operations associated with your new
> >>>> ioctl.
> >>>>
> >>>> - The ioctl would create the endpoint.
> >>>> - The open() and close() operations would simply manage the file descriptor and
> >>>> increment/decrement a counter to prevent premature endpoint destruction.
> >>>>
> >>>>
> >>>> Regards,
> >>>> Arnaud
> >>>>
> >>>
> >>> [...]
> >>>
> >>> Thanks,
> >>>
> >>> 	Dawei

Re: [PATCH v4 0/3] rpmsg: Introduce RPMSG_CREATE_EPT_FD_IOCTL uAPI

[Arnaud POULIQUEN]

hello Dawei

On 7/1/25 16:16, Dawei Li wrote:
> Hi Arnaud,
> 
> Thanks for the reply.
> 
> On Mon, Jun 30, 2025 at 09:54:40AM +0200, Arnaud POULIQUEN wrote:
>> Hello Dawei,
>>
>> Sorry for the late answer.
>>
>> On 6/22/25 06:12, Dawei Li wrote:
>>> Hi Arnaud,
>>>
>>> Thanks for the reply.
>>>
>>> On Fri, Jun 20, 2025 at 09:52:03AM +0200, Arnaud POULIQUEN wrote:
>>>>
>>>>
>>>> On 6/19/25 16:43, Dawei Li wrote:
>>>>> Hi Arnaud, 
>>>>> Thanks for review.
>>>>>
>>>>> On Wed, Jun 18, 2025 at 03:07:36PM +0200, Arnaud POULIQUEN wrote:
>>>>>> Hello Dawei,
>>>>>>
>>>>>>
>>>>>> Please find a few comments below. It is not clear to me which parts of your
>>>>>> implementation are mandatory and which are optional "nice-to-have" optimizations.
>>>>>
>>>>> It's more like an improvement.
>>>>>
>>>>>>
>>>>>> Based on (potentially erroneous) hypothesis, you will find a suggestion for an
>>>>>> alternative to the anonymous inode approach, which does not seem to be a common
>>>>>> interface.
>>>>>
>>>>> AFAIC, annoymous inode is a common interface and used extensivly in kernel development.
>>>>> Some examples below.
>>>>>
>>>>>>
>>>>>>
>>>>>> On 6/9/25 17:15, Dawei Li wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> This is V4 of series which introduce new uAPI(RPMSG_CREATE_EPT_FD_IOCTL)
>>>>>>> for rpmsg subsystem.
>>>>>>>
>>>>>>> Current uAPI implementation for rpmsg ctrl & char device manipulation is
>>>>>>> abstracted in procedures below:
>>>>>>> - fd = open("/dev/rpmsg_ctrlX")
>>>>>>> - ioctl(fd, RPMSG_CREATE_EPT_IOCTL, &info); /dev/rpmsgY devnode is
>>>>>>>   generated.
>>>>>>> - fd_ep = open("/dev/rpmsgY", O_RDWR) 
>>>>>>> - operations on fd_ep(write, read, poll ioctl)
>>>>>>> - ioctl(fd_ep, RPMSG_DESTROY_EPT_IOCTL)
>>>>>>> - close(fd_ep)
>>>>>>> - close(fd)
>>>>>>>
>>>>>>> This /dev/rpmsgY abstraction is less favorable for:
>>>>>>> - Performance issue: It's time consuming for some operations are
>>>>>>> invovled:
>>>>>>>   - Device node creation.
>>>>>>>     Depends on specific config, especially CONFIG_DEVTMPFS, the overall
>>>>>>>     overhead is based on coordination between DEVTMPFS and userspace
>>>>>>>     tools such as udev and mdev.
>>>>>>>
>>>>>>>   - Extra kernel-space switch cost.
>>>>>>>
>>>>>>>   - Other major costs brought by heavy-weight logic like device_add().
>>>>>>
>>>>>> Is this a blocker of just optimization?
>>>>>
>>>>> Yep, performance is one of motivations of this change.
>>>>>
>>>>>>
>>>>>>>
>>>>>>> - /dev/rpmsgY node can be opened only once. It doesn't make much sense
>>>>>>>     that a dynamically created device node can be opened only once.
>>>>>>
>>>>>>
>>>>>> I assume this is blocker with the fact that you need to open the /dev/rpmsg<x>
>>>>>> to create the endpoint.
>>>>>
>>>>> Yes. You have to open /dev/rpmsgX which is generated by legacy ioctl to
>>>>> instantiate a new endpoint.
>>>>>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> - For some container application such as docker, a client can't access
>>>>>>>   host's dev unless specified explicitly. But in case of /dev/rpmsgY, which
>>>>>>>   is generated dynamically and whose existence is unknown for clients in
>>>>>>>   advance, this uAPI based on device node doesn't fit well.
>>>>>>
>>>>>> does this could be solve in userspace parsing /sys/class/rpmsg/ directory to
>>>>>> retreive the device?
>>>>>
>>>>> Hardly, because client still can't access /dev/rpmsgX which is generated
>>>>> by host _after_ client is launched.
>>>>
>>>>
>>>> This part is not clear to me; could you provide more details?
>>>> I cannot figure out why a client can access /dev/rpmsg_ctrlX but not /dev/rpmsgX.
>>>
>>> Well, let's take docker as example:
>>
>>>
>>> For docker, when a client is launched and it wants to access host's
>>> device, it must make explicit request when it's launched:
>>>
>>> docker run --device=/dev/xxx
>>>
>>> Let's presume that xxx is /dev/rpmsgX generated dynamically by _host_.
>>> Docker command above knows nothing about these rpmsg nodes which are
>>> generated by host _after_ client is launched. And yes, parsing> /sys/class/rpmsg may acquire info about rpmsg devices, but client still
>>> can't access /dev/rpmsgX.
>>
>> One extra question:Are you using RPMsg over virtio?
>>
>> If yes, do you test the RPMsg name service (NS) announcement, that might also
>> address your needs.
>>
>> The principle is that the remote processor sends a name service announcement to
>> Linux, which probes the rpmsg character device and creates the /dev/rpmsgX
>> device in a predefined order known by the remote processor.
>> In such a case, the /dev/rpmsgX usage would be determined by the remote
>> processor itself.
>>
>> Another advantage is that the RPMsg channel creation is not driven by either the
>> host or the client. In such case host does no need to define/kwnow RPMSg
>> endpoint addresses.
>>
>> You still need to call the open() file system operation, but this should be done
>> one time during Docker client initialization.
> 
> NS is nice, but perhaps it's not the approach for some cases.
> 
> For offloading/accelerator scenarios, ACPU is responsible for making all
> the important decisions, including creations of endpoints. Because all
> the user-awared software stack is running on ACPU, and if you want to
> create a endpoint _dynamically_, it must be from user's command which is
> from ACPU.
> 
> And this series is more about how rpmsg_char and rpmsg_ctrl coordinate
> themselves about creating dynamic rpmsg endpoints in a more simple
> and efficient way.
> 
> And the whole point of series is "When you want to return a fd to
> userspace which represents an instance of data structure in kernel, you
> don't implement it as character device". Maybe some quotes from Christian[1]
> can describe it better[1]:
> 
> "I'm not sure why people are so in love with character device based apis.
> It's terrible. It glues everything to devtmpfs which isn't namespacable
> in any way. It's terrible to delegate and extremely restrictive in terms
> of extensiblity if you need additional device entries (aka the loop
> driver folly)."
> 
> [1] https://lkml.org/lkml/2025/6/24/639

Thank you for all your explanations! It is always very interesting to understand
the different ways to use RPMsg.

In the end, I don't see any problem with your series. The possibility of using
an anonymous inode seems valid to me.

I am just wondering whether this should be implemented in the rpmsg_char driver
or in a new driver, addressing this question to Mathieu and Bjorn.


Regards,
Arnaud

> 
> Thanks,
> 
> 	Dawei
> 
>>
>> Regards
>> Arnaud
>>
>>
>>>
>>>>
>>>>
>>>>>
>>>>>>
>>>>>> You could face same kind of random instantiation for serial peripherals ( UART;
>>>>>> USb, I2C,...) based on a device tree enumeration. I suppose that user space
>>>>>> use to solve this.
>>>>>>
>>>>>>>
>>>>>>> An anonymous inode based approach is introduced to address the issues above.
>>>>>>> Rather than generating device node and opening it, rpmsg code just creates
>>>>>>> an anonymous inode representing eptdev and return the fd to userspace.
>>>>>>
>>>>>> A drawback is that you need to share fb passed between processes.
>>>>>
>>>>> Fd is the abstraction of an unique endpoint device, it holds true for
>>>>> both legacy and new approach.
>>>>>
>>>>> So I guess what you mean is that /dev/rpmsgX is global to all so other process
>>>>> can access it?
>>>>>
>>>>> But /dev/rpmsgX is designed to be opened only once, it's implemented as
>>>>> singleton pattern.
>>>>>
>>>>> static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
>>>>> {
>>>>> ...
>>>>>         if (eptdev->ept) {
>>>>>                 mutex_unlock(&eptdev->ept_lock);
>>>>>                 return -EBUSY;
>>>>>         }
>>>>> ...
>>>>>         eptdev->ept = ept;
>>>>> ...
>>>>> }
>>>>>
>>>>> [...]
>>>>>  
>>>>>>> 	printf("loop[%d]\n", loop);
>>>>>>>
>>>>>>> 	gettimeofday(&start, NULL);
>>>>>>>
>>>>>>> 	while (loop--) {
>>>>>>
>>>>>> Do you need to create /close Endpoint sevral times in your real use case with
>>>>>> high timing
>>>>>> constraint?
>>>>>
>>>>> No, it's just a silly benchmark demo, large sample reduces noise statistically.
>>>>>
>>>>>>
>>>>>>> 		fd_info.fd = -1;
>>>>>>> 		fd_info.flags = O_RDWR | O_CLOEXEC | O_NONBLOCK;
>>>>>>> 		ret = ioctl(fd, RPMSG_CREATE_EPT_FD_IOCTL, &fd_info);
>>>>>>> 		if (ret < 0 || fd_info.fd < 0) {
>>>>>>> 			printf("ioctl[RPMSG_CREATE_EPT_FD_IOCTL] failed, ret[%d]\n", ret);
>>>>>>> 		}
>>>>>>>
>>>>>>
>>>>>>
>>>>>>> 		ret = ioctl(fd_info.fd, RPMSG_DESTROY_EPT_IOCTL, &info);
>>>>>>> 		if (ret < 0) {
>>>>>>> 			printf("new ioctl[RPMSG_DESTROY_EPT_IOCTL] failed, ret[%d]\n", ret);
>>>>>>> 		}
>>>>>>>
>>>>>>> 		close(fd_info.fd);
>>>>>>
>>>>>> It seems strange to me to use ioctl() for opening and close() for closing, from
>>>>>> a symmetry point of view.
>>>>>
>>>>> Sorry to hear that. But no, it's a pretty normal skill in kernel codebase
>>>>> , I had to copy some examples from reply to other reviewer[1].
>>>>
>>>> I missed this one, apologize for the duplication.
>>>>
>>>>>
>>>>> anon_inode_get_{fd,file} are used extensively in kernel for returning a new
>>>>> fd to userspace which is associated with an unique data structure in kernel
>>>>> space, in different ways:
>>>>>
>>>>> - via ioctl(), some examples are:
>>>>>
>>>>>  - KVM ioctl(s)
>>>>>    - KVM_CREATE_VCPU -> kvm_vm_ioctl_create_vcpu
>>>>>    - KVM_GET_STATS_FD -> kvm_vcpu_ioctl_get_stats_fd
>>>>>    - KVM_CREATE_DEVICE -> kvm_ioctl_create_device
>>>>>    - KVM_CREATE_VM -> kvm_dev_ioctl_create_vm
>>>>>
>>>>>  - DMA buf/fence/sync ioctls
>>>>>    - DMA_BUF_IOCTL_EXPORT_SYNC_FILE -> dma_buf_export_sync_file
>>>>>    - SW_SYNC_IOC_CREATE_FENCE -> sw_sync_ioctl_create_fence
>>>>>    - Couples of driver implement DMA buf by using anon file _implicitly_:
>>>>>      - UDMABUF_CREATE -> udmabuf_ioctl_create
>>>>>      - DMA_HEAP_IOCTL_ALLOC -> dma_heap_ioctl_allocate
>>>>>
>>>>>  - gpiolib ioctls:
>>>>>    - GPIO_GET_LINEHANDLE_IOCTL -> linehandle_create
>>>>>    - GPIO_V2_GET_LINE_IOCTL
>>>>>
>>>>>  -  IOMMUFD ioctls:
>>>>>
>>>>>  -  VFIO Ioctls:
>>>>>
>>>>>  - ....
>>>>>
>>>>>
>>>>> - via other specific syscalls:
>>>>>  - epoll_create1
>>>>>  - bpf
>>>>>  - perf_event_open
>>>>>  - inotify_init
>>>>>  - ...
>>>>
>>>> If we put the optimization aspect aside, what seems strange to me is that the
>>>> purpose of rpmsg_char was to expose a FS character device to user space. If we
>>>> need tobypass the use of /dev/rpmsgX, does it make sense to support an anonymous
>>>> inode in this driver?  I am clearly not legitimate to answer this question...
>>>
>>> You have every right to do so, after all, it's purely a technical
>>> discussion :).
>>>
>>> I admit it's bit confusing to add annoymous inode logic to a file named
>>> rpmsg_char.c which implies 'character' device. That's why I rename API
>>> following Mathieu's comment:
>>>   - __rpmsg_chrdev_eptdev_alloc ->  rpmsg_eptdev_alloc
>>>   - __rpmsg_chrdev_eptdev_add ->  rpmsg_eptdev_add
>>>
>>> As to topic how these two uAPI(s) co-exist and affect each other. This
>>> change is based on rules:
>>>
>>> 1. Never break existing uAPI.
>>> 2. Try best to reuse existing codebase.
>>> 3. Userspace can choose whatever approach they want to.
>>>
>>> Thanks,
>>>
>>> 	Dawei
>>>>
>>>>
>>>> Thanks,
>>>> Arnaud
>>>>
>>>>>
>>>>> [1] https://lore.kernel.org/all/20250530125008.GA5355@wendao-VirtualBox/
>>>>>
>>>>>>
>>>>>> Regarding your implementation, I wonder if we could keep the /dev/rpmsg<x>
>>>>>> device with specific open() and close() file operations associated with your new
>>>>>> ioctl.
>>>>>>
>>>>>> - The ioctl would create the endpoint.
>>>>>> - The open() and close() operations would simply manage the file descriptor and
>>>>>> increment/decrement a counter to prevent premature endpoint destruction.
>>>>>>
>>>>>>
>>>>>> Regards,
>>>>>> Arnaud
>>>>>>
>>>>>
>>>>> [...]
>>>>>
>>>>> Thanks,
>>>>>
>>>>> 	Dawei

Re: [PATCH v4 0/3] rpmsg: Introduce RPMSG_CREATE_EPT_FD_IOCTL uAPI

[Dawei Li]

Hi Arnaud,

On Wed, Jul 02, 2025 at 09:48:59AM +0200, Arnaud POULIQUEN wrote:
> hello Dawei
> 
> On 7/1/25 16:16, Dawei Li wrote:
> > Hi Arnaud,
> > 
> > Thanks for the reply.
> > 
> > On Mon, Jun 30, 2025 at 09:54:40AM +0200, Arnaud POULIQUEN wrote:
> >> Hello Dawei,
> >>
> >> Sorry for the late answer.
> >>
> >> On 6/22/25 06:12, Dawei Li wrote:
> >>> Hi Arnaud,
> >>>
> >>> Thanks for the reply.
> >>>
> >>> On Fri, Jun 20, 2025 at 09:52:03AM +0200, Arnaud POULIQUEN wrote:
> >>>>
> >>>>
> >>>> On 6/19/25 16:43, Dawei Li wrote:
> >>>>> Hi Arnaud, 
> >>>>> Thanks for review.
> >>>>>
> >>>>> On Wed, Jun 18, 2025 at 03:07:36PM +0200, Arnaud POULIQUEN wrote:
> >>>>>> Hello Dawei,
> >>>>>>
> >>>>>>
> >>>>>> Please find a few comments below. It is not clear to me which parts of your
> >>>>>> implementation are mandatory and which are optional "nice-to-have" optimizations.
> >>>>>
> >>>>> It's more like an improvement.
> >>>>>
> >>>>>>
> >>>>>> Based on (potentially erroneous) hypothesis, you will find a suggestion for an
> >>>>>> alternative to the anonymous inode approach, which does not seem to be a common
> >>>>>> interface.
> >>>>>
> >>>>> AFAIC, annoymous inode is a common interface and used extensivly in kernel development.
> >>>>> Some examples below.
> >>>>>
> >>>>>>
> >>>>>>
> >>>>>> On 6/9/25 17:15, Dawei Li wrote:
> >>>>>>> Hi,
> >>>>>>>
> >>>>>>> This is V4 of series which introduce new uAPI(RPMSG_CREATE_EPT_FD_IOCTL)
> >>>>>>> for rpmsg subsystem.
> >>>>>>>
> >>>>>>> Current uAPI implementation for rpmsg ctrl & char device manipulation is
> >>>>>>> abstracted in procedures below:
> >>>>>>> - fd = open("/dev/rpmsg_ctrlX")
> >>>>>>> - ioctl(fd, RPMSG_CREATE_EPT_IOCTL, &info); /dev/rpmsgY devnode is
> >>>>>>>   generated.
> >>>>>>> - fd_ep = open("/dev/rpmsgY", O_RDWR) 
> >>>>>>> - operations on fd_ep(write, read, poll ioctl)
> >>>>>>> - ioctl(fd_ep, RPMSG_DESTROY_EPT_IOCTL)
> >>>>>>> - close(fd_ep)
> >>>>>>> - close(fd)
> >>>>>>>
> >>>>>>> This /dev/rpmsgY abstraction is less favorable for:
> >>>>>>> - Performance issue: It's time consuming for some operations are
> >>>>>>> invovled:
> >>>>>>>   - Device node creation.
> >>>>>>>     Depends on specific config, especially CONFIG_DEVTMPFS, the overall
> >>>>>>>     overhead is based on coordination between DEVTMPFS and userspace
> >>>>>>>     tools such as udev and mdev.
> >>>>>>>
> >>>>>>>   - Extra kernel-space switch cost.
> >>>>>>>
> >>>>>>>   - Other major costs brought by heavy-weight logic like device_add().
> >>>>>>
> >>>>>> Is this a blocker of just optimization?
> >>>>>
> >>>>> Yep, performance is one of motivations of this change.
> >>>>>
> >>>>>>
> >>>>>>>
> >>>>>>> - /dev/rpmsgY node can be opened only once. It doesn't make much sense
> >>>>>>>     that a dynamically created device node can be opened only once.
> >>>>>>
> >>>>>>
> >>>>>> I assume this is blocker with the fact that you need to open the /dev/rpmsg<x>
> >>>>>> to create the endpoint.
> >>>>>
> >>>>> Yes. You have to open /dev/rpmsgX which is generated by legacy ioctl to
> >>>>> instantiate a new endpoint.
> >>>>>
> >>>>>>
> >>>>>>
> >>>>>>>
> >>>>>>> - For some container application such as docker, a client can't access
> >>>>>>>   host's dev unless specified explicitly. But in case of /dev/rpmsgY, which
> >>>>>>>   is generated dynamically and whose existence is unknown for clients in
> >>>>>>>   advance, this uAPI based on device node doesn't fit well.
> >>>>>>
> >>>>>> does this could be solve in userspace parsing /sys/class/rpmsg/ directory to
> >>>>>> retreive the device?
> >>>>>
> >>>>> Hardly, because client still can't access /dev/rpmsgX which is generated
> >>>>> by host _after_ client is launched.
> >>>>
> >>>>
> >>>> This part is not clear to me; could you provide more details?
> >>>> I cannot figure out why a client can access /dev/rpmsg_ctrlX but not /dev/rpmsgX.
> >>>
> >>> Well, let's take docker as example:
> >>
> >>>
> >>> For docker, when a client is launched and it wants to access host's
> >>> device, it must make explicit request when it's launched:
> >>>
> >>> docker run --device=/dev/xxx
> >>>
> >>> Let's presume that xxx is /dev/rpmsgX generated dynamically by _host_.
> >>> Docker command above knows nothing about these rpmsg nodes which are
> >>> generated by host _after_ client is launched. And yes, parsing> /sys/class/rpmsg may acquire info about rpmsg devices, but client still
> >>> can't access /dev/rpmsgX.
> >>
> >> One extra question:Are you using RPMsg over virtio?
> >>
> >> If yes, do you test the RPMsg name service (NS) announcement, that might also
> >> address your needs.
> >>
> >> The principle is that the remote processor sends a name service announcement to
> >> Linux, which probes the rpmsg character device and creates the /dev/rpmsgX
> >> device in a predefined order known by the remote processor.
> >> In such a case, the /dev/rpmsgX usage would be determined by the remote
> >> processor itself.
> >>
> >> Another advantage is that the RPMsg channel creation is not driven by either the
> >> host or the client. In such case host does no need to define/kwnow RPMSg
> >> endpoint addresses.
> >>
> >> You still need to call the open() file system operation, but this should be done
> >> one time during Docker client initialization.
> > 
> > NS is nice, but perhaps it's not the approach for some cases.
> > 
> > For offloading/accelerator scenarios, ACPU is responsible for making all
> > the important decisions, including creations of endpoints. Because all
> > the user-awared software stack is running on ACPU, and if you want to
> > create a endpoint _dynamically_, it must be from user's command which is
> > from ACPU.
> > 
> > And this series is more about how rpmsg_char and rpmsg_ctrl coordinate
> > themselves about creating dynamic rpmsg endpoints in a more simple
> > and efficient way.
> > 
> > And the whole point of series is "When you want to return a fd to
> > userspace which represents an instance of data structure in kernel, you
> > don't implement it as character device". Maybe some quotes from Christian[1]
> > can describe it better[1]:
> > 
> > "I'm not sure why people are so in love with character device based apis.
> > It's terrible. It glues everything to devtmpfs which isn't namespacable
> > in any way. It's terrible to delegate and extremely restrictive in terms
> > of extensiblity if you need additional device entries (aka the loop
> > driver folly)."
> > 
> > [1] https://lkml.org/lkml/2025/6/24/639
> 
> Thank you for all your explanations! It is always very interesting to understand
> the different ways to use RPMsg.
> 
> In the end, I don't see any problem with your series. The possibility of using
> an anonymous inode seems valid to me.

Thanks for the review!

> 
> I am just wondering whether this should be implemented in the rpmsg_char driver
> or in a new driver, addressing this question to Mathieu and Bjorn.

Well, I think we are kinda in a dilemma:

- It's bit odd to add anonymous inode code in a file named rpmsg_char.c.

- On the other hand, the new anonymous code path do share couples of
  codes and abstractions of rpmsg_char.c, rpmsg_eptdev, all methods
  excluding open() in fops... . If new code is built into a standalone
  driver, how are we supposed to take care of these shared codes?

Yep, leave it to the maintainers.

Thanks,

	Dawei

> 
> 
> Regards,
> Arnaud
> 
> > 
> > Thanks,
> > 
> > 	Dawei
> > 
> >>
> >> Regards
> >> Arnaud
> >>
> >>
> >>>
> >>>>
> >>>>
> >>>>>
> >>>>>>
> >>>>>> You could face same kind of random instantiation for serial peripherals ( UART;
> >>>>>> USb, I2C,...) based on a device tree enumeration. I suppose that user space
> >>>>>> use to solve this.
> >>>>>>
> >>>>>>>
> >>>>>>> An anonymous inode based approach is introduced to address the issues above.
> >>>>>>> Rather than generating device node and opening it, rpmsg code just creates
> >>>>>>> an anonymous inode representing eptdev and return the fd to userspace.
> >>>>>>
> >>>>>> A drawback is that you need to share fb passed between processes.
> >>>>>
> >>>>> Fd is the abstraction of an unique endpoint device, it holds true for
> >>>>> both legacy and new approach.
> >>>>>
> >>>>> So I guess what you mean is that /dev/rpmsgX is global to all so other process
> >>>>> can access it?
> >>>>>
> >>>>> But /dev/rpmsgX is designed to be opened only once, it's implemented as
> >>>>> singleton pattern.
> >>>>>
> >>>>> static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
> >>>>> {
> >>>>> ...
> >>>>>         if (eptdev->ept) {
> >>>>>                 mutex_unlock(&eptdev->ept_lock);
> >>>>>                 return -EBUSY;
> >>>>>         }
> >>>>> ...
> >>>>>         eptdev->ept = ept;
> >>>>> ...
> >>>>> }
> >>>>>
> >>>>> [...]
> >>>>>  
> >>>>>>> 	printf("loop[%d]\n", loop);
> >>>>>>>
> >>>>>>> 	gettimeofday(&start, NULL);
> >>>>>>>
> >>>>>>> 	while (loop--) {
> >>>>>>
> >>>>>> Do you need to create /close Endpoint sevral times in your real use case with
> >>>>>> high timing
> >>>>>> constraint?
> >>>>>
> >>>>> No, it's just a silly benchmark demo, large sample reduces noise statistically.
> >>>>>
> >>>>>>
> >>>>>>> 		fd_info.fd = -1;
> >>>>>>> 		fd_info.flags = O_RDWR | O_CLOEXEC | O_NONBLOCK;
> >>>>>>> 		ret = ioctl(fd, RPMSG_CREATE_EPT_FD_IOCTL, &fd_info);
> >>>>>>> 		if (ret < 0 || fd_info.fd < 0) {
> >>>>>>> 			printf("ioctl[RPMSG_CREATE_EPT_FD_IOCTL] failed, ret[%d]\n", ret);
> >>>>>>> 		}
> >>>>>>>
> >>>>>>
> >>>>>>
> >>>>>>> 		ret = ioctl(fd_info.fd, RPMSG_DESTROY_EPT_IOCTL, &info);
> >>>>>>> 		if (ret < 0) {
> >>>>>>> 			printf("new ioctl[RPMSG_DESTROY_EPT_IOCTL] failed, ret[%d]\n", ret);
> >>>>>>> 		}
> >>>>>>>
> >>>>>>> 		close(fd_info.fd);
> >>>>>>
> >>>>>> It seems strange to me to use ioctl() for opening and close() for closing, from
> >>>>>> a symmetry point of view.
> >>>>>
> >>>>> Sorry to hear that. But no, it's a pretty normal skill in kernel codebase
> >>>>> , I had to copy some examples from reply to other reviewer[1].
> >>>>
> >>>> I missed this one, apologize for the duplication.
> >>>>
> >>>>>
> >>>>> anon_inode_get_{fd,file} are used extensively in kernel for returning a new
> >>>>> fd to userspace which is associated with an unique data structure in kernel
> >>>>> space, in different ways:
> >>>>>
> >>>>> - via ioctl(), some examples are:
> >>>>>
> >>>>>  - KVM ioctl(s)
> >>>>>    - KVM_CREATE_VCPU -> kvm_vm_ioctl_create_vcpu
> >>>>>    - KVM_GET_STATS_FD -> kvm_vcpu_ioctl_get_stats_fd
> >>>>>    - KVM_CREATE_DEVICE -> kvm_ioctl_create_device
> >>>>>    - KVM_CREATE_VM -> kvm_dev_ioctl_create_vm
> >>>>>
> >>>>>  - DMA buf/fence/sync ioctls
> >>>>>    - DMA_BUF_IOCTL_EXPORT_SYNC_FILE -> dma_buf_export_sync_file
> >>>>>    - SW_SYNC_IOC_CREATE_FENCE -> sw_sync_ioctl_create_fence
> >>>>>    - Couples of driver implement DMA buf by using anon file _implicitly_:
> >>>>>      - UDMABUF_CREATE -> udmabuf_ioctl_create
> >>>>>      - DMA_HEAP_IOCTL_ALLOC -> dma_heap_ioctl_allocate
> >>>>>
> >>>>>  - gpiolib ioctls:
> >>>>>    - GPIO_GET_LINEHANDLE_IOCTL -> linehandle_create
> >>>>>    - GPIO_V2_GET_LINE_IOCTL
> >>>>>
> >>>>>  -  IOMMUFD ioctls:
> >>>>>
> >>>>>  -  VFIO Ioctls:
> >>>>>
> >>>>>  - ....
> >>>>>
> >>>>>
> >>>>> - via other specific syscalls:
> >>>>>  - epoll_create1
> >>>>>  - bpf
> >>>>>  - perf_event_open
> >>>>>  - inotify_init
> >>>>>  - ...
> >>>>
> >>>> If we put the optimization aspect aside, what seems strange to me is that the
> >>>> purpose of rpmsg_char was to expose a FS character device to user space. If we
> >>>> need tobypass the use of /dev/rpmsgX, does it make sense to support an anonymous
> >>>> inode in this driver?  I am clearly not legitimate to answer this question...
> >>>
> >>> You have every right to do so, after all, it's purely a technical
> >>> discussion :).
> >>>
> >>> I admit it's bit confusing to add annoymous inode logic to a file named
> >>> rpmsg_char.c which implies 'character' device. That's why I rename API
> >>> following Mathieu's comment:
> >>>   - __rpmsg_chrdev_eptdev_alloc ->  rpmsg_eptdev_alloc
> >>>   - __rpmsg_chrdev_eptdev_add ->  rpmsg_eptdev_add
> >>>
> >>> As to topic how these two uAPI(s) co-exist and affect each other. This
> >>> change is based on rules:
> >>>
> >>> 1. Never break existing uAPI.
> >>> 2. Try best to reuse existing codebase.
> >>> 3. Userspace can choose whatever approach they want to.
> >>>
> >>> Thanks,
> >>>
> >>> 	Dawei
> >>>>
> >>>>
> >>>> Thanks,
> >>>> Arnaud
> >>>>
> >>>>>
> >>>>> [1] https://lore.kernel.org/all/20250530125008.GA5355@wendao-VirtualBox/
> >>>>>
> >>>>>>
> >>>>>> Regarding your implementation, I wonder if we could keep the /dev/rpmsg<x>
> >>>>>> device with specific open() and close() file operations associated with your new
> >>>>>> ioctl.
> >>>>>>
> >>>>>> - The ioctl would create the endpoint.
> >>>>>> - The open() and close() operations would simply manage the file descriptor and
> >>>>>> increment/decrement a counter to prevent premature endpoint destruction.
> >>>>>>
> >>>>>>
> >>>>>> Regards,
> >>>>>> Arnaud
> >>>>>>
> >>>>>
> >>>>> [...]
> >>>>>
> >>>>> Thanks,
> >>>>>
> >>>>> 	Dawei