Re: [PATCH v3 3/3] rpmsg: char: Rework exception handling of rpmsg_eptdev_add()

[Zhongqiu Han]

On 11/13/2025 11:39 PM, Dawei Li wrote:

Rework  error handling of rpmsg_eptdev_add() and its callers, following
rule of "release resource where it's allocated".

Fixes: 2410558f5f11 ("rpmsg: char: Implement eptdev based on anonymous inode")
Reported-by: Dan Carpenter <dan.carpen...@linaro.org>
Closes: https://lore.kernel.org/all/aPi6gPZE2_ztOjIW@stanley.mountain/

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

diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index 0919ad0a19df..92c176e9b0e4 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -460,44 +460,34 @@ static int rpmsg_eptdev_add(struct rpmsg_eptdev *eptdev,

eptdev->chinfo = chinfo; - 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;
+               return ret;
        dev->id = ret;
        dev_set_name(dev, "rpmsg%d", ret);

- ret = 0;

-
        if (cdev) {
+               ret = ida_alloc_max(&rpmsg_minor_ida, RPMSG_DEV_MAX - 1, 
GFP_KERNEL);
+               if (ret < 0) {
+                       ida_free(&rpmsg_ept_ida, dev->id);
+                       return ret;
+               }
+
+               dev->devt = MKDEV(MAJOR(rpmsg_major), ret);
+
                ret = cdev_device_add(&eptdev->cdev, &eptdev->dev);
-               if (ret)
-                       goto free_ept_ida;
+               if (ret) {
+                       ida_free(&rpmsg_ept_ida, dev->id);
+                       ida_free(&rpmsg_minor_ida, MINOR(dev->devt));
+                       return ret;
+               }
        }

/* We can now rely on the release function for cleanup */

        dev->release = rpmsg_eptdev_release_device;

- return ret;

-
-free_ept_ida:
-       ida_free(&rpmsg_ept_ida, dev->id);
-free_minor_ida:
-       if (cdev)
-               ida_free(&rpmsg_minor_ida, MINOR(dev->devt));
-free_eptdev:
-       dev_err(&eptdev->dev, "failed to add %s\n", eptdev->chinfo.name);
-       kfree(eptdev);
-
-       return ret;
+       return 0;
  }

static int rpmsg_chrdev_eptdev_add(struct rpmsg_eptdev *eptdev, struct rpmsg_channel_info chinfo)

@@ -509,12 +499,17 @@ int rpmsg_chrdev_eptdev_create(struct rpmsg_device 
*rpdev, struct device *parent
                               struct rpmsg_channel_info chinfo)
  {
        struct rpmsg_eptdev *eptdev;
+       int ret;

eptdev = rpmsg_chrdev_eptdev_alloc(rpdev, parent);

        if (IS_ERR(eptdev))
                return PTR_ERR(eptdev);

- return rpmsg_chrdev_eptdev_add(eptdev, chinfo);

+       ret = rpmsg_chrdev_eptdev_add(eptdev, chinfo);
+       if (ret)
+               kfree(eptdev);
+
+       return ret;
  }
  EXPORT_SYMBOL(rpmsg_chrdev_eptdev_create);

@@ -545,6 +540,12 @@ int rpmsg_anonymous_eptdev_create(struct rpmsg_device *rpdev, struct device *par ret = rpmsg_eptdev_add(eptdev, chinfo, false);

        if (ret) {
+               dev_err(&eptdev->dev, "failed to add %s\n", 
eptdev->chinfo.name);
+               /*
+                * Avoid put_device() or WARN() will be triggered due to 
absence of
+                * device::release(), refer to device_release().

Hi Dawei,

As I mentioned about the potential memory leak issue in patch 1/3, we
could consider still using put_device for management, as this better
aligns with the driver model standards and avoids potential issue.
However, this requires assigning the release function in advance and
also handling the special case where ida allocation fails in
rpmsg_eptdev_add (removing the manual ida release).

+                */
+               kfree(eptdev);
                return ret;
        }

@@ -572,6 +573,7 @@ static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)

        struct rpmsg_channel_info chinfo;
        struct rpmsg_eptdev *eptdev;
        struct device *dev = &rpdev->dev;
+       int ret;

memcpy(chinfo.name, rpdev->id.name, RPMSG_NAME_SIZE);

        chinfo.src = rpdev->src;
@@ -590,7 +592,11 @@ static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
         */
        eptdev->default_ept->priv = eptdev;

- return rpmsg_chrdev_eptdev_add(eptdev, chinfo);

+       ret = rpmsg_chrdev_eptdev_add(eptdev, chinfo);
+       if (ret)
+               kfree(eptdev);
+
+       return ret;
  }

static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev)

--
Thx and BRs,
Zhongqiu Han