Message-ID: <aCZNZPkb5oPZiz9G@p14s>
Date: Thu, 15 May 2025 14:24:04 -0600
From: Mathieu Poirier <mathieu.poirier@...aro.org>
To: Dawei Li <dawei.li@...ux.dev>
Cc: andersson@...nel.org, linux-remoteproc@...r.kernel.org,
	linux-kernel@...r.kernel.org, set_pte_at@...look.com
Subject: Re: [PATCH v2 1/3] rpmsg: char: Reuse eptdev logic for anon device

Hi,

On Fri, May 09, 2025 at 11:59:25PM +0800, Dawei Li wrote:
> Current uAPI implementation for rpmsg ctrl & char device manipulation is
> abstracted in procedures below:
> 
> 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 anon inode based approach is introduced to address the issues above.
> Rather than generating device node and opening it, rpmsg code just make
> a anon inode representing eptdev and return the fd to userspace.

Please change occurences of "anon" for "anonyous".  "Anon" was used to avoid
writing "anonymous" all the time in the code, but description should not use the
shorthand.  In the title, this patch and all other patches.

> 
> 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@...ux.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..5b2a883d6236 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_chrdev_eptdev_alloc(struct rpmsg_device *rpdev,
> +							struct device *parent, bool cdev)


I would simply rename this rpmsg_eptdev_alloc() and then use is in
rpmsg_chrdev_eptdev_alloc() and rpmsg_anonynous_eptdev_alloc()

>  {
>  	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_chrdev_eptdev_alloc(rpdev, parent, true);
> +}
> +
> +static int __rpmsg_chrdev_eptdev_add(struct rpmsg_eptdev *eptdev,
> +				     struct rpmsg_channel_info chinfo, bool cdev)

Same here, rpmsg_eptdev_add()

>  {
>  	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);
> +	}
> +
> +	/* Anon inode device still need dev 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_chrdev_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
> 

Powered by blists - more mailing lists