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

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

[Dawei Li]

Hi,

This is V3 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 (Updated)

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 (Updated)

- 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 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 |  38 ++++++++---
 include/uapi/linux/rpmsg.h |  24 +++++++
 4 files changed, 182 insertions(+), 32 deletions(-)

---
base-commit: 92a09c47464d040866cf2b4cd052bc60555185fb

Thanks,

	Dawei
-- 
2.25.1

[PATCH v3 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 v3 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 v3 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 | 38 ++++++++++++++++++++++++++++++--------
 include/uapi/linux/rpmsg.h | 24 ++++++++++++++++++++++++
 2 files changed, 54 insertions(+), 8 deletions(-)

diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
index 28f57945ccd9..9f2f118ceb7b 100644
--- a/drivers/rpmsg/rpmsg_ctrl.c
+++ b/drivers/rpmsg/rpmsg_ctrl.c
@@ -75,19 +75,32 @@ 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;
+	int fd = -1;
+
+	if (cmd == RPMSG_CREATE_EPT_IOCTL || cmd == RPMSG_CREATE_DEV_IOCTL ||
+	    cmd == RPMSG_RELEASE_DEV_IOCTL) {
+		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;
+	} else 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;
+	}
 
 	mutex_lock(&ctrldev->ctrl_lock);
 	switch (cmd) {
@@ -110,6 +123,15 @@ 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, &fd);
+		if (!ret) {
+			ept_fd_info.fd = fd;
+			ret = copy_to_user(argp, &ept_fd_info, sizeof(ept_fd_info));
+		}
+		break;
+
 	default:
 		ret = -EINVAL;
 	}
diff --git a/include/uapi/linux/rpmsg.h b/include/uapi/linux/rpmsg.h
index f0c8da2b185b..e7057bd23577 100644
--- a/include/uapi/linux/rpmsg.h
+++ b/include/uapi/linux/rpmsg.h
@@ -53,4 +53,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 v3 3/3] rpmsg: ctrl: Introduce RPMSG_CREATE_EPT_FD_IOCTL uAPI

[Dan Carpenter]

Hi Dawei,

kernel test robot noticed the following build warnings:

url:    https://github.com/intel-lab-lkp/linux/commits/Dawei-Li/rpmsg-char-Reuse-eptdev-logic-for-anonymous-device/20250519-231006
base:   92a09c47464d040866cf2b4cd052bc60555185fb
patch link:    https://lore.kernel.org/r/20250519150823.62350-4-dawei.li%40linux.dev
patch subject: [PATCH v3 3/3] rpmsg: ctrl: Introduce RPMSG_CREATE_EPT_FD_IOCTL uAPI
config: powerpc64-randconfig-r072-20250521 (https://download.01.org/0day-ci/archive/20250521/202505211038.sqqVX8kO-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202505211038.sqqVX8kO-lkp@intel.com/

smatch warnings:
drivers/rpmsg/rpmsg_ctrl.c:140 rpmsg_ctrldev_ioctl() warn: maybe return -EFAULT instead of the bytes remaining?

vim +140 drivers/rpmsg/rpmsg_ctrl.c

617d32938d1be0 Arnaud Pouliquen 2022-01-24   74  static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
617d32938d1be0 Arnaud Pouliquen 2022-01-24   75  				unsigned long arg)
617d32938d1be0 Arnaud Pouliquen 2022-01-24   76  {
617d32938d1be0 Arnaud Pouliquen 2022-01-24   77  	struct rpmsg_ctrldev *ctrldev = fp->private_data;
74317ea5240801 Dawei Li         2025-05-19   78  	struct rpmsg_endpoint_fd_info ept_fd_info;
617d32938d1be0 Arnaud Pouliquen 2022-01-24   79  	void __user *argp = (void __user *)arg;
617d32938d1be0 Arnaud Pouliquen 2022-01-24   80  	struct rpmsg_endpoint_info eptinfo;
617d32938d1be0 Arnaud Pouliquen 2022-01-24   81  	struct rpmsg_channel_info chinfo;
8109517b394e6d Arnaud Pouliquen 2022-01-24   82  	struct rpmsg_device *rpdev;
8109517b394e6d Arnaud Pouliquen 2022-01-24   83  	int ret = 0;
74317ea5240801 Dawei Li         2025-05-19   84  	int fd = -1;
617d32938d1be0 Arnaud Pouliquen 2022-01-24   85  
74317ea5240801 Dawei Li         2025-05-19   86  	if (cmd == RPMSG_CREATE_EPT_IOCTL || cmd == RPMSG_CREATE_DEV_IOCTL ||
74317ea5240801 Dawei Li         2025-05-19   87  	    cmd == RPMSG_RELEASE_DEV_IOCTL) {
617d32938d1be0 Arnaud Pouliquen 2022-01-24   88  		if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
617d32938d1be0 Arnaud Pouliquen 2022-01-24   89  			return -EFAULT;
617d32938d1be0 Arnaud Pouliquen 2022-01-24   90  
617d32938d1be0 Arnaud Pouliquen 2022-01-24   91  		memcpy(chinfo.name, eptinfo.name, RPMSG_NAME_SIZE);
617d32938d1be0 Arnaud Pouliquen 2022-01-24   92  		chinfo.name[RPMSG_NAME_SIZE - 1] = '\0';
617d32938d1be0 Arnaud Pouliquen 2022-01-24   93  		chinfo.src = eptinfo.src;
617d32938d1be0 Arnaud Pouliquen 2022-01-24   94  		chinfo.dst = eptinfo.dst;
74317ea5240801 Dawei Li         2025-05-19   95  	} else if (cmd == RPMSG_CREATE_EPT_FD_IOCTL) {
74317ea5240801 Dawei Li         2025-05-19   96  		if (copy_from_user(&ept_fd_info, argp, sizeof(ept_fd_info)))
74317ea5240801 Dawei Li         2025-05-19   97  			return -EFAULT;
74317ea5240801 Dawei Li         2025-05-19   98  
74317ea5240801 Dawei Li         2025-05-19   99  		memcpy(chinfo.name, ept_fd_info.name, RPMSG_NAME_SIZE);
74317ea5240801 Dawei Li         2025-05-19  100  		chinfo.name[RPMSG_NAME_SIZE - 1] = '\0';
74317ea5240801 Dawei Li         2025-05-19  101  		chinfo.src = ept_fd_info.src;
74317ea5240801 Dawei Li         2025-05-19  102  		chinfo.dst = ept_fd_info.dst;
74317ea5240801 Dawei Li         2025-05-19  103  	}
617d32938d1be0 Arnaud Pouliquen 2022-01-24  104  
8109517b394e6d Arnaud Pouliquen 2022-01-24  105  	mutex_lock(&ctrldev->ctrl_lock);
8109517b394e6d Arnaud Pouliquen 2022-01-24  106  	switch (cmd) {
8109517b394e6d Arnaud Pouliquen 2022-01-24  107  	case RPMSG_CREATE_EPT_IOCTL:
8109517b394e6d Arnaud Pouliquen 2022-01-24  108  		ret = rpmsg_chrdev_eptdev_create(ctrldev->rpdev, &ctrldev->dev, chinfo);
8109517b394e6d Arnaud Pouliquen 2022-01-24  109  		break;
8109517b394e6d Arnaud Pouliquen 2022-01-24  110  
8109517b394e6d Arnaud Pouliquen 2022-01-24  111  	case RPMSG_CREATE_DEV_IOCTL:
8109517b394e6d Arnaud Pouliquen 2022-01-24  112  		rpdev = rpmsg_create_channel(ctrldev->rpdev, &chinfo);
8109517b394e6d Arnaud Pouliquen 2022-01-24  113  		if (!rpdev) {
8109517b394e6d Arnaud Pouliquen 2022-01-24  114  			dev_err(&ctrldev->dev, "failed to create %s channel\n", chinfo.name);
8109517b394e6d Arnaud Pouliquen 2022-01-24  115  			ret = -ENXIO;
8109517b394e6d Arnaud Pouliquen 2022-01-24  116  		}
8109517b394e6d Arnaud Pouliquen 2022-01-24  117  		break;
8109517b394e6d Arnaud Pouliquen 2022-01-24  118  
8109517b394e6d Arnaud Pouliquen 2022-01-24  119  	case RPMSG_RELEASE_DEV_IOCTL:
8109517b394e6d Arnaud Pouliquen 2022-01-24  120  		ret = rpmsg_release_channel(ctrldev->rpdev, &chinfo);
8109517b394e6d Arnaud Pouliquen 2022-01-24  121  		if (ret)
8109517b394e6d Arnaud Pouliquen 2022-01-24  122  			dev_err(&ctrldev->dev, "failed to release %s channel (%d)\n",
8109517b394e6d Arnaud Pouliquen 2022-01-24  123  				chinfo.name, ret);
8109517b394e6d Arnaud Pouliquen 2022-01-24  124  		break;
8109517b394e6d Arnaud Pouliquen 2022-01-24  125  
74317ea5240801 Dawei Li         2025-05-19  126  	case RPMSG_CREATE_EPT_FD_IOCTL:
74317ea5240801 Dawei Li         2025-05-19  127  		ret = rpmsg_anonymous_eptdev_create(ctrldev->rpdev, &ctrldev->dev, chinfo,
74317ea5240801 Dawei Li         2025-05-19  128  						    ept_fd_info.flags, &fd);
74317ea5240801 Dawei Li         2025-05-19  129  		if (!ret) {

You should flip this around.  if (ret)
					break;

74317ea5240801 Dawei Li         2025-05-19  130  			ept_fd_info.fd = fd;
74317ea5240801 Dawei Li         2025-05-19  131  			ret = copy_to_user(argp, &ept_fd_info, sizeof(ept_fd_info));

This should be:

	if (copy_to_user(argp, &ept_fd_info, sizeof(ept_fd_info)))
		ret = -EFAULT;


74317ea5240801 Dawei Li         2025-05-19  132  		}
74317ea5240801 Dawei Li         2025-05-19  133  		break;
74317ea5240801 Dawei Li         2025-05-19  134  
8109517b394e6d Arnaud Pouliquen 2022-01-24  135  	default:
8109517b394e6d Arnaud Pouliquen 2022-01-24  136  		ret = -EINVAL;
8109517b394e6d Arnaud Pouliquen 2022-01-24  137  	}
8109517b394e6d Arnaud Pouliquen 2022-01-24  138  	mutex_unlock(&ctrldev->ctrl_lock);
8109517b394e6d Arnaud Pouliquen 2022-01-24  139  
8109517b394e6d Arnaud Pouliquen 2022-01-24 @140  	return ret;
617d32938d1be0 Arnaud Pouliquen 2022-01-24  141  };

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v3 3/3] rpmsg: ctrl: Introduce RPMSG_CREATE_EPT_FD_IOCTL uAPI

[Beleswar Prasad Padhi]

Hi Dawei,

On 19/05/25 20:38, Dawei Li wrote:
> 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


We are returning a new fd to userspace from inside an IOCTL itself. Is this a
standard way of doing things in Kernel space? (see below related comment)

> - operations on fd_ep(write, read, poll ioctl)
> - ioctl(fd_ep, RPMSG_DESTROY_EPT_IOCTL)
> - close(fd_ep)


Can we rely on the userspace to close() the fd_ep? (if not done, could be a
memory leak..).. Opposed to fd, which we can rely on the userspace to
close() since they initiated the open() call. I am just trying to understand if
this is a standard way of doing things...

> - close(fd)
>
> Signed-off-by: Dawei Li <dawei.li@linux.dev>
> ---
>  drivers/rpmsg/rpmsg_ctrl.c | 38 ++++++++++++++++++++++++++++++--------
>  include/uapi/linux/rpmsg.h | 24 ++++++++++++++++++++++++
>  2 files changed, 54 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
> index 28f57945ccd9..9f2f118ceb7b 100644
> --- a/drivers/rpmsg/rpmsg_ctrl.c
> +++ b/drivers/rpmsg/rpmsg_ctrl.c
> @@ -75,19 +75,32 @@ 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;
> +	int fd = -1;
> +
> +	if (cmd == RPMSG_CREATE_EPT_IOCTL || cmd == RPMSG_CREATE_DEV_IOCTL ||
> +	    cmd == RPMSG_RELEASE_DEV_IOCTL) {
> +		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;
> +	} else if (cmd == RPMSG_CREATE_EPT_FD_IOCTL) {


Maybe we can put this 'else if condition' in the first 'if' and treat other
conditions under 'else', as 'RPMSG_CREATE_EPT_FD_IOCTL' is the only
ioctl with a different struct type.

Thanks,
Beleswar

> +		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;
> +	}
>  
>  	mutex_lock(&ctrldev->ctrl_lock);
>  	switch (cmd) {
> @@ -110,6 +123,15 @@ 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, &fd);
> +		if (!ret) {
> +			ept_fd_info.fd = fd;
> +			ret = copy_to_user(argp, &ept_fd_info, sizeof(ept_fd_info));
> +		}
> +		break;
> +
>  	default:
>  		ret = -EINVAL;
>  	}
> diff --git a/include/uapi/linux/rpmsg.h b/include/uapi/linux/rpmsg.h
> index f0c8da2b185b..e7057bd23577 100644
> --- a/include/uapi/linux/rpmsg.h
> +++ b/include/uapi/linux/rpmsg.h
> @@ -53,4 +53,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

Re: [PATCH v3 3/3] rpmsg: ctrl: Introduce RPMSG_CREATE_EPT_FD_IOCTL uAPI

[Dawei Li]

HI Beleswar,

Thanks for reviewing.

On Fri, May 30, 2025 at 03:15:28PM +0530, Beleswar Prasad Padhi wrote:
> Hi Dawei,
> 
> On 19/05/25 20:38, Dawei Li wrote:
> > 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
> 
> 
> We are returning a new fd to userspace from inside an IOCTL itself. Is this a
> standard way of doing things in Kernel space? (see below related comment)

Yes, anon_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
 - ...

> 
> > - operations on fd_ep(write, read, poll ioctl)
> > - ioctl(fd_ep, RPMSG_DESTROY_EPT_IOCTL)
> > - close(fd_ep)
> 
> 
> Can we rely on the userspace to close() the fd_ep? (if not done, could be a
> memory leak..).. Opposed to fd, which we can rely on the userspace to
> close() since they initiated the open() call. I am just trying to understand if
> this is a standard way of doing things...

Good question.

When userland gets a fd from kernel, it's userland's duty to manage and release
the resource when it's done with it, because kernel never knows when the fd and
its resourcen are not needed by userland except process is on exiting. The fact
remains true no matter how fd is generated from kernel:
- open()
- ioctl()
- Other syscalls(epoll_create1, e.g, as listed above)

As a result, kernel & driver provide fops->release() to achieve resource
release when fd is not needed for userland, some callers of it maybe:
- Userland call close() explicitly
- Kernel does the dirty job when user process exits(if some fds are
  still opened):
  - Userland call exit() explicitly.
  - User process was killed by some signals.

Maybe some comments/docs are needed in uAPI?

> 
> > - close(fd)
> >

[snip]

> > +
> > +	if (cmd == RPMSG_CREATE_EPT_IOCTL || cmd == RPMSG_CREATE_DEV_IOCTL ||
> > +	    cmd == RPMSG_RELEASE_DEV_IOCTL) {
> > +		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;
> > +	} else if (cmd == RPMSG_CREATE_EPT_FD_IOCTL) {
> 
> 
> Maybe we can put this 'else if condition' in the first 'if' and treat other
> conditions under 'else', as 'RPMSG_CREATE_EPT_FD_IOCTL' is the only
> ioctl with a different struct type.

Good point! I will try to address it in next respin.

> 
> Thanks,
> Beleswar
> 
> > +		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;
> > +	}
> >  

[snip]

Thanks,

	Dawei

Re: [PATCH v3 3/3] rpmsg: ctrl: Introduce RPMSG_CREATE_EPT_FD_IOCTL uAPI

[Beleswar Prasad Padhi]

Hi Dawei,

On 30/05/25 18:20, Dawei Li wrote:
> HI Beleswar,
>
> Thanks for reviewing.
>
> On Fri, May 30, 2025 at 03:15:28PM +0530, Beleswar Prasad Padhi wrote:
>> Hi Dawei,
>>
>> On 19/05/25 20:38, Dawei Li wrote:
>>> 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
>>
>> We are returning a new fd to userspace from inside an IOCTL itself. Is this a
>> standard way of doing things in Kernel space? (see below related comment)
> Yes, anon_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
>  - ...


Thank you for the extensive list of examples!

>
>>> - operations on fd_ep(write, read, poll ioctl)
>>> - ioctl(fd_ep, RPMSG_DESTROY_EPT_IOCTL)
>>> - close(fd_ep)
>>
>> Can we rely on the userspace to close() the fd_ep? (if not done, could be a
>> memory leak..).. Opposed to fd, which we can rely on the userspace to
>> close() since they initiated the open() call. I am just trying to understand if
>> this is a standard way of doing things...
> Good question.
>
> When userland gets a fd from kernel, it's userland's duty to manage and release
> the resource when it's done with it, because kernel never knows when the fd and
> its resourcen are not needed by userland except process is on exiting. The fact
> remains true no matter how fd is generated from kernel:
> - open()
> - ioctl()
> - Other syscalls(epoll_create1, e.g, as listed above)
>
> As a result, kernel & driver provide fops->release() to achieve resource
> release when fd is not needed for userland, some callers of it maybe:
> - Userland call close() explicitly
> - Kernel does the dirty job when user process exits(if some fds are
>   still opened):
>   - Userland call exit() explicitly.
>   - User process was killed by some signals.
>
> Maybe some comments/docs are needed in uAPI?


Perhaps yes. It makes sense to me now. Thanks for addressing my queries!

>
>>> - close(fd)
>>>
> [snip]
>
>>> +
>>> +	if (cmd == RPMSG_CREATE_EPT_IOCTL || cmd == RPMSG_CREATE_DEV_IOCTL ||
>>> +	    cmd == RPMSG_RELEASE_DEV_IOCTL) {
>>> +		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;
>>> +	} else if (cmd == RPMSG_CREATE_EPT_FD_IOCTL) {
>>
>> Maybe we can put this 'else if condition' in the first 'if' and treat other
>> conditions under 'else', as 'RPMSG_CREATE_EPT_FD_IOCTL' is the only
>> ioctl with a different struct type.
> Good point! I will try to address it in next respin.


Thanks,
Beleswar

>
>> Thanks,
>> Beleswar
>>
>>> +		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;
>>> +	}
>>>  
> [snip]
>
> Thanks,
>
> 	Dawei