Hi,
On Mon, Jun 09, 2025 at 11:15:29PM +0800, Dawei Li wrote:
> 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.
The above line is too long and gives me a checkpatch warning. Please refactor
and send again.
Thanks,
Mathieu
> 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 <[email protected]>
> ---
> 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
>
