Re: [PATCH v8 11/13] rpmsg: char: Introduce the "rpmsg-raw" channel

From: Arnaud POULIQUEN
Date: Tue Jan 18 2022 - 06:04:22 EST



On 1/18/22 12:03 AM, Bjorn Andersson wrote:
On Tue 07 Dec 02:08 CST 2021, Arnaud Pouliquen wrote:

Allows to probe the endpoint device on a remote name service announcement,
by registering a rpmsg_driverfor the "rpmsg-raw" channel.

With this patch the /dev/rpmsgX interface can be instantiated by the remote
firmware.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@xxxxxxxxxxx>
---
drivers/rpmsg/rpmsg_char.c | 64 ++++++++++++++++++++++++++++++++++++++
drivers/rpmsg/rpmsg_ctrl.c | 7 +++--
2 files changed, 69 insertions(+), 2 deletions(-)

diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index cf97839f5833..92b44630e03a 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -435,6 +435,58 @@ int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent
}
EXPORT_SYMBOL(rpmsg_chrdev_eptdev_create);
+static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
+{
+ struct rpmsg_channel_info chinfo;
+ struct rpmsg_eptdev *eptdev;
+ struct device *dev = &rpdev->dev;
+
+ memcpy(chinfo.name, rpdev->id.name, RPMSG_NAME_SIZE);
+ chinfo.src = rpdev->src;
+ chinfo.dst = rpdev->dst;
+
+ eptdev = rpmsg_chrdev_eptdev_alloc(rpdev, dev);
+ if (IS_ERR(eptdev))
+ return PTR_ERR(eptdev);
+
+ /*
+ * Create the default endpoint associated to the rpmsg device and provide rpmsg_eptdev
+ * structure as callback private data.

If the only this the probe function does is to create a new endpoint
with the same properties as the rpdev, why can't you just specify a
callback on the rpmsg_chrdev_driver?

As this isn't the typical way you create a default endpoint I think the
reasoning behind this warrants a proper explanation in the commit
message.


As mentioned in [PATCH v8 09/13] rpmsg: Introduce rpmsg_create_default_ept function
"This helper function allows rpmsg drivers to create a default endpoint
on runtime with an associated private context."

Here the private context is the eptdev structure. I need to create the structure first, before
the endpoint.
I will add more details in the commit message as you suggest.

An alternative could be to directly set default_ept->priv but as the rpmsg_create_default_ept
"priv" parameter is forwarded to the rpmsg backend (using create_ept ops).
This could introduces unexpected side effect.

+ * Do not allow the creation and release of an endpoint on /dev/rpmsgX open and close,
+ * reuse the default endpoint instead

This sentence doesn't tell me anything about this code snippet and
doesn't indicate that it relates to the snippet added elsewhere in this
file by the previous patch.

+ */
+ eptdev->default_ept = rpmsg_create_default_ept(rpdev, rpmsg_ept_cb, eptdev, chinfo);
+ if (!eptdev->default_ept) {
+ dev_err(&rpdev->dev, "failed to create %s\n", chinfo.name);
+ put_device(dev);
+ kfree(eptdev);
+ return -EINVAL;
+ }
+
+ return rpmsg_chrdev_eptdev_add(eptdev, chinfo);
+}
+
+static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev)
+{
+ int ret;
+
+ ret = device_for_each_child(&rpdev->dev, NULL, rpmsg_chrdev_eptdev_destroy);
+ if (ret)
+ dev_warn(&rpdev->dev, "failed to destroy endpoints: %d\n", ret);
+}
+
+static struct rpmsg_device_id rpmsg_chrdev_id_table[] = {
+ { .name = "rpmsg-raw" },
+ { },
+};
+
+static struct rpmsg_driver rpmsg_chrdev_driver = {
+ .probe = rpmsg_chrdev_probe,
+ .remove = rpmsg_chrdev_remove,
+ .id_table = rpmsg_chrdev_id_table,
+ .drv.name = "rpmsg_chrdev",
+};
+
static int rpmsg_chrdev_init(void)
{
int ret;
@@ -445,12 +497,24 @@ static int rpmsg_chrdev_init(void)
return ret;
}
+ ret = register_rpmsg_driver(&rpmsg_chrdev_driver);
+ if (ret < 0) {
+ pr_err("rpmsg: failed to register rpmsg raw driver\n");
+ goto free_region;
+ }
+
return 0;
+
+free_region:
+ unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
+
+ return ret;
}
postcore_initcall(rpmsg_chrdev_init);
static void rpmsg_chrdev_exit(void)
{
+ unregister_rpmsg_driver(&rpmsg_chrdev_driver);
unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
}
module_exit(rpmsg_chrdev_exit);
diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
index 59d2bd264fdb..298e75dc7774 100644
--- a/drivers/rpmsg/rpmsg_ctrl.c
+++ b/drivers/rpmsg/rpmsg_ctrl.c
@@ -10,6 +10,9 @@
* Based on rpmsg performance statistics driver by Michal Simek, which in turn
* was based on TI & Google OMX rpmsg driver.
*/
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

These changes seems unrelated to above.

I apparently broke something in my patchset here during a rebase. The previous
irrelevant comment you pointed out to me is also a consequence. My apologies for this dirty patch...

Thanks,
Arnaud


Regards,
Bjorn

+
#include <linux/cdev.h>
#include <linux/device.h>
#include <linux/fs.h>
@@ -193,13 +196,13 @@ static int rpmsg_ctrldev_init(void)
ret = alloc_chrdev_region(&rpmsg_major, 0, RPMSG_DEV_MAX, "rpmsg_ctrl");
if (ret < 0) {
- pr_err("rpmsg: failed to allocate char dev region\n");
+ pr_err("failed to allocate char dev region\n");
return ret;
}
ret = register_rpmsg_driver(&rpmsg_ctrldev_driver);
if (ret < 0) {
- pr_err("rpmsg ctrl: failed to register rpmsg driver\n");
+ pr_err("failed to register rpmsg driver\n");
unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
}
--
2.17.1