Re: [PATCH v2 1/3] rpmsg: char: Reuse eptdev logic for anon device

[Dawei Li]

Hi Mathieu,

Thanks for reviewing.

On Thu, May 15, 2025 at 02:24:04PM -0600, Mathieu Poirier wrote:
> 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.

Acked.

> 
> > 
> > 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@xxxxxxxxx>
> > ---
> >  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()

Agreed. rpmsg_eptdev_alloc() is much better.

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

Acked.

All the issues above, including those for patch 2/3, will be fixed in
V3.

Thanks,

	Dawei

> 
> >  {
> >  	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
> >