Update from V6 [1] based on Bjorn Andersson comments:
- rework rpmsg class: suppress API in rpmsg.h and expose directly the variable in rpmsg_internal.h
- move rpmsg_create_default_ept declaration from rpmsg.h to rpmsg_internal.h
- rework the configs dependencies between RPMSG_CTRL and RPMSG_CHAR
Remaining discussion from V6:
- use of the rpmsg_create_default_ept API (proposal is to put it in rpmsg_internal.h).
And a new patch to fix ns announcement on default endpoint creation.
Patchset description:
The current rpmsg_char module implements a /dev/rpmsg_ctrl interface that provides the ability to
instantiate char devices (/dev/rpmsgX) associated with an rpmsg endpoint for communication with the
remote processor.
This implementation fit with QCOM rpmsg backend but not with themagement by chanel implemented in
the generic rpmsg virtio backend.
This series restructures the rpmsg_char driver to decorrelate the control part from the data part
in order to improve its compatible with the rpmsg virtio backend.
Objective:
- Expose a /dev/rpmsg_ctrlX interface for the application that is no longer dedicated to the
rpmsg_char but generalized to all rpmsg services. This offers capability to create and destroy
rpmsg channels from a user's application initiative (using the new RPMSG_CREATE_DEV_IOCTL and
RPMSG_DESTROY_DEV_IOCTL controls).
An application will be able to create/establish an rpmsg communication channel to communicate
with the remote processor, and not only wait the remote processor initiative.
This is interesting for example to establish a temporary communication link for diagnosis,
calibration, debugging... or instantiate new data flows on some user actions.
- Add capability to probe the rpmsg_char device at the initiative of the remote processor
(rpmsg service announcement mechanism).
This allows platforms based on the rpmsg virtio backend to create the /dev/rpmgX interface with
a rpmsg name service announcement.
Subsets:
- Extract the control part of the char dev and create the rpmsg_ctrl.c file (patches 1 to 6)
- Introduce the "rpmsg-raw" channel in rpmsg_char(patches 7 to 10)
- Introduce the RPMSG_CREATE_DEV_IOCTL IOCTL and RPMSG_DESTROY_DEV_IOCTL to instantiate RPMsg
devices (patch 11)
The application can then create or release a channel by specifying:
- the name service of the device to instantiate.
- the source address.
- the destination address.
- Send a ns announcement to the remote processor on default endpoint creation (patche 12)
This series has be applied and tested on 5.15 branch (6ee5808de074)[2].
[1] https://lkml.org/lkml/2021/10/22/431
[2] https://github.com/torvalds/linux.git
Arnaud Pouliquen (10):
rpmsg: char: Export eptdev create an destroy functions
rpmsg: create the rpmsg class in core instead of in rpmsg char
rpmsg: Move the rpmsg control device from rpmsg_char to rpmsg_ctrl
ARM: configs: Configs that had RPMSG_CHAR now gets RPMSG_CTRL
RISCV: configs: Configs that had RPMSG_CHAR now gets RPMSG_CTRL
rpmsg: Update rpmsg_chrdev_register_device function
rpmsg: char: Refactor rpmsg_chrdev_eptdev_create function
rpmsg: Introduce rpmsg_create_default_ept function
rpmsg: char: Add possibility to use default endpoint of the rpmsg
device.
rpmsg: char: Introduce the "rpmsg-raw" channel
rpmsg: ctrl: Introduce new RPMSG_CREATE/RELEASE_DEV_IOCTL controls
rpmsg: core: send a ns announcement when a default endpoint is created
arch/arm/configs/qcom_defconfig | 1 +
arch/riscv/configs/defconfig | 1 +
arch/riscv/configs/rv32_defconfig | 1 +
drivers/rpmsg/Kconfig | 8 +
drivers/rpmsg/Makefile | 1 +
drivers/rpmsg/qcom_glink_native.c | 2 +-
drivers/rpmsg/qcom_smd.c | 2 +-
drivers/rpmsg/rpmsg_char.c | 244 +++++++++++------------------
drivers/rpmsg/rpmsg_char.h | 51 ++++++
drivers/rpmsg/rpmsg_core.c | 83 +++++++++-
drivers/rpmsg/rpmsg_ctrl.c | 250 ++++++++++++++++++++++++++++++
drivers/rpmsg/rpmsg_internal.h | 14 +-
drivers/rpmsg/virtio_rpmsg_bus.c | 2 +-
include/uapi/linux/rpmsg.h | 10 ++
14 files changed, 506 insertions(+), 164 deletions(-)
create mode 100644 drivers/rpmsg/rpmsg_char.h
create mode 100644 drivers/rpmsg/rpmsg_ctrl.c
--
2.17.1
By providing a callback in the rpmsg_driver structure, the rpmsg devices
can be probed with a default endpoint created.
In this case, it is not possible to associated to this endpoint private data
that could allow the driver to retrieve the context.
This helper function allows rpmsg drivers to create a default endpoint
on runtime with an associated private context.
For example, a driver might create a context structure on the probe and
want to provide that context as private data for the default rpmsg
callback.
Signed-off-by: Arnaud Pouliquen <[email protected]>
Reviewed-by: Mathieu Poirier <[email protected]>
Tested-by: Julien Massot <[email protected]>
---
Update vs previous revision:
- move rpmsg_create_default_ept declaration from rpmsg.h to rpmsg_internal.h
- fix rpmsg_create_default_ept function header
- Treat rpdev->ept!= 0 as invalid usage with a WARN_ON
---
drivers/rpmsg/rpmsg_core.c | 54 ++++++++++++++++++++++++++++++++++
drivers/rpmsg/rpmsg_internal.h | 4 +++
2 files changed, 58 insertions(+)
diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
index 45227c864aa2..bdcde57c22f6 100644
--- a/drivers/rpmsg/rpmsg_core.c
+++ b/drivers/rpmsg/rpmsg_core.c
@@ -133,6 +133,60 @@ void rpmsg_destroy_ept(struct rpmsg_endpoint *ept)
}
EXPORT_SYMBOL(rpmsg_destroy_ept);
+/**
+ * rpmsg_create_default_ept() - create a default rpmsg_endpoint for a rpmsg device
+ * @rpdev: rpmsg channel device
+ * @cb: rx callback handler
+ * @priv: private data for the driver's use
+ * @chinfo: channel_info with the local rpmsg address to bind with @cb
+ *
+ * On register_rpmsg_driver if no callback is provided in the rpmsg_driver structure,
+ * no endpoint is created when the device is probed by the rpmsg bus.
+ *
+ * This function returns a pointer to an endpoint created and assigned as the default
+ * endpoint of the rpmsg device.
+ *
+ * Drivers should provide their @rpdev channel (so the new endpoint would belong
+ * to the same remote processor their channel belongs to), an rx callback
+ * function, an optional private data (which is provided back when the
+ * rx callback is invoked), and an address they want to bind with the
+ * callback. If @addr is RPMSG_ADDR_ANY, then rpmsg_create_ept will
+ * dynamically assign them an available rpmsg address (drivers should have
+ * a very good reason why not to always use RPMSG_ADDR_ANY here).
+ *
+ * Return: a pointer to the endpoint on success, or NULL on error.
+ */
+struct rpmsg_endpoint *rpmsg_create_default_ept(struct rpmsg_device *rpdev,
+ rpmsg_rx_cb_t cb, void *priv,
+ struct rpmsg_channel_info chinfo)
+{
+ struct rpmsg_endpoint *ept;
+
+ if (WARN_ON(!rpdev))
+ return NULL;
+
+ if (WARN_ON(rpdev->ept))
+ return NULL;
+
+ /* It does not make sense to create a default endpoint without a callback. */
+ if (!cb)
+ return NULL;
+
+ if (rpdev->ept)
+ return rpdev->ept;
+
+ ept = rpdev->ops->create_ept(rpdev, cb, priv, chinfo);
+ if (!ept)
+ return NULL;
+
+ /* Assign the new endpoint as default endpoint */
+ rpdev->ept = ept;
+ rpdev->src = ept->addr;
+
+ return ept;
+}
+EXPORT_SYMBOL(rpmsg_create_default_ept);
+
/**
* rpmsg_send() - send a message across to the remote processor
* @ept: the rpmsg endpoint
diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
index 8bb56d347c81..4577e9381d25 100644
--- a/drivers/rpmsg/rpmsg_internal.h
+++ b/drivers/rpmsg/rpmsg_internal.h
@@ -98,4 +98,8 @@ static inline int rpmsg_ctrldev_register_device(struct rpmsg_device *rpdev)
return rpmsg_register_device(rpdev);
}
+struct rpmsg_endpoint *rpmsg_create_default_ept(struct rpmsg_device *rpdev,
+ rpmsg_rx_cb_t cb, void *priv,
+ struct rpmsg_channel_info chinfo);
+
#endif
--
2.17.1
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 <[email protected]>
---
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 6a01e8e1c111..dd754c870ba1 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -432,6 +432,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.
+ * Do not allow the creation and release of an endpoint on /dev/rpmsgX open and close,
+ * reuse the default endpoint instead
+ */
+ 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;
@@ -442,12 +494,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
+
#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
Migrate the creation of the rpmsg class from the rpmsg_char
to the core that the class is usable by the rpmsg_char and
the futur rpmsg_ctrl module.
Suggested-by: Bjorn Andersson <[email protected]>
Signed-off-by: Arnaud Pouliquen <[email protected]>
---
Update vs previous revision:
- remove rpmsg_get_class API and export the rpmsg_class in rpmsg_internal.h
---
drivers/rpmsg/rpmsg_char.c | 10 ----------
drivers/rpmsg/rpmsg_core.c | 15 +++++++++++++--
drivers/rpmsg/rpmsg_internal.h | 2 ++
3 files changed, 15 insertions(+), 12 deletions(-)
diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index 44934d7fa3c4..8ab5ac23850c 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -29,7 +29,6 @@
#define RPMSG_DEV_MAX (MINORMASK + 1)
static dev_t rpmsg_major;
-static struct class *rpmsg_class;
static DEFINE_IDA(rpmsg_ctrl_ida);
static DEFINE_IDA(rpmsg_ept_ida);
@@ -559,17 +558,9 @@ static int rpmsg_chrdev_init(void)
return ret;
}
- rpmsg_class = class_create(THIS_MODULE, "rpmsg");
- if (IS_ERR(rpmsg_class)) {
- pr_err("failed to create rpmsg class\n");
- unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
- return PTR_ERR(rpmsg_class);
- }
-
ret = register_rpmsg_driver(&rpmsg_chrdev_driver);
if (ret < 0) {
pr_err("rpmsgchr: failed to register rpmsg driver\n");
- class_destroy(rpmsg_class);
unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
}
@@ -580,7 +571,6 @@ postcore_initcall(rpmsg_chrdev_init);
static void rpmsg_chrdev_exit(void)
{
unregister_rpmsg_driver(&rpmsg_chrdev_driver);
- class_destroy(rpmsg_class);
unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
}
module_exit(rpmsg_chrdev_exit);
diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
index 9151836190ce..45227c864aa2 100644
--- a/drivers/rpmsg/rpmsg_core.c
+++ b/drivers/rpmsg/rpmsg_core.c
@@ -20,6 +20,9 @@
#include "rpmsg_internal.h"
+struct class *rpmsg_class;
+EXPORT_SYMBOL(rpmsg_class);
+
/**
* rpmsg_create_channel() - create a new rpmsg channel
* using its name and address info.
@@ -629,10 +632,17 @@ static int __init rpmsg_init(void)
{
int ret;
+ rpmsg_class = class_create(THIS_MODULE, "rpmsg");
+ if (IS_ERR(rpmsg_class)) {
+ pr_err("failed to create rpmsg class\n");
+ return PTR_ERR(rpmsg_class);
+ }
+
ret = bus_register(&rpmsg_bus);
- if (ret)
+ if (ret) {
pr_err("failed to register rpmsg bus: %d\n", ret);
-
+ class_destroy(rpmsg_class);
+ }
return ret;
}
postcore_initcall(rpmsg_init);
@@ -640,6 +650,7 @@ postcore_initcall(rpmsg_init);
static void __exit rpmsg_fini(void)
{
bus_unregister(&rpmsg_bus);
+ class_destroy(rpmsg_class);
}
module_exit(rpmsg_fini);
diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
index a76c344253bf..1b6f998e1a4a 100644
--- a/drivers/rpmsg/rpmsg_internal.h
+++ b/drivers/rpmsg/rpmsg_internal.h
@@ -18,6 +18,8 @@
#define to_rpmsg_device(d) container_of(d, struct rpmsg_device, dev)
#define to_rpmsg_driver(d) container_of(d, struct rpmsg_driver, drv)
+extern struct class *rpmsg_class;
+
/**
* struct rpmsg_device_ops - indirection table for the rpmsg_device operations
* @create_channel: create backend-specific channel, optional
--
2.17.1
Current implementation create/destroy a new endpoint on each
rpmsg_eptdev_open/rpmsg_eptdev_release calls.
For a rpmsg device created by the NS announcement mechanism we need to
use a unique static endpoint that is the default rpmsg device endpoint
associated to the channel.
This patch prepares the introduction of a rpmsg channel device for the
char device. The rpmsg channel device will require a default endpoint to
communicate to the remote processor.
Add the default_ept field in rpmsg_eptdev structure.This pointer
determines the behavior on rpmsg_eptdev_open and rpmsg_eptdev_release call.
- If default_ept == NULL:
Use the legacy behavior by creating a new endpoint each time
rpmsg_eptdev_open is called and release it when rpmsg_eptdev_release
is called on /dev/rpmsgX device open/close.
- If default_ept is set:
use the rpmsg device default endpoint for the communication.
Address the update of _rpmsg_chrdev_eptdev_create in a separate patch for readability.
Add protection in rpmsg_eptdev_ioctl to prevent to destroy a default endpoint.
Signed-off-by: Arnaud Pouliquen <[email protected]>
---
drivers/rpmsg/rpmsg_char.c | 21 +++++++++++++++++++--
1 file changed, 19 insertions(+), 2 deletions(-)
diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index 3daf62df69a0..6a01e8e1c111 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -47,6 +47,8 @@ static DEFINE_IDA(rpmsg_minor_ida);
* @queue_lock: synchronization of @queue operations
* @queue: incoming message queue
* @readq: wait object for incoming queue
+ * @default_ept: set to channel default endpoint if the default endpoint should be re-used
+ * on device open to prevent endpoint address update.
*/
struct rpmsg_eptdev {
struct device dev;
@@ -57,10 +59,12 @@ struct rpmsg_eptdev {
struct mutex ept_lock;
struct rpmsg_endpoint *ept;
+ struct rpmsg_endpoint *default_ept;
spinlock_t queue_lock;
struct sk_buff_head queue;
wait_queue_head_t readq;
+
};
int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data)
@@ -118,7 +122,15 @@ static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
get_device(dev);
- ept = rpmsg_create_ept(rpdev, rpmsg_ept_cb, eptdev, eptdev->chinfo);
+ /*
+ * If the default_ept is set to true, the rpmsg device default endpoint is used.
+ * Else a new endpoint is created on open that will be destroyed on release.
+ */
+ if (eptdev->default_ept)
+ ept = eptdev->default_ept;
+ else
+ ept = rpmsg_create_ept(rpdev, rpmsg_ept_cb, eptdev, eptdev->chinfo);
+
if (!ept) {
dev_err(dev, "failed to open %s\n", eptdev->chinfo.name);
put_device(dev);
@@ -139,7 +151,8 @@ static int rpmsg_eptdev_release(struct inode *inode, struct file *filp)
/* Close the endpoint, if it's not already destroyed by the parent */
mutex_lock(&eptdev->ept_lock);
if (eptdev->ept) {
- rpmsg_destroy_ept(eptdev->ept);
+ if (!eptdev->default_ept)
+ rpmsg_destroy_ept(eptdev->ept);
eptdev->ept = NULL;
}
mutex_unlock(&eptdev->ept_lock);
@@ -266,6 +279,10 @@ static long rpmsg_eptdev_ioctl(struct file *fp, unsigned int cmd,
if (cmd != RPMSG_DESTROY_EPT_IOCTL)
return -EINVAL;
+ /* Don't allow to destroy a default endpoint. */
+ if (eptdev->default_ept)
+ return -EINVAL;
+
return rpmsg_chrdev_eptdev_destroy(&eptdev->dev, NULL);
}
--
2.17.1
To prepare the split of the code related to the control (ctrldev)
and the endpoint (eptdev) devices in 2 separate files:
- Rename and export the functions in rpmsg_char.h.
- Suppress the dependency with the rpmsg_ctrldev struct in the
rpmsg_eptdev_create function.
Suggested-by: Mathieu Poirier <[email protected]>
Signed-off-by: Arnaud Pouliquen <[email protected]>
---
Update vs previous revision:
- change IS_REACHABLE by IS_ENABLE ( dependency will be fixed in kconfig instead
- fix licensing
---
drivers/rpmsg/rpmsg_char.c | 17 +++++++------
drivers/rpmsg/rpmsg_char.h | 51 ++++++++++++++++++++++++++++++++++++++
2 files changed, 61 insertions(+), 7 deletions(-)
create mode 100644 drivers/rpmsg/rpmsg_char.h
diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index 2bebc9b2d163..44934d7fa3c4 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0
/*
+ * Copyright (C) 2021, STMicroelectronics
* Copyright (c) 2016, Linaro Ltd.
* Copyright (c) 2012, Michal Simek <[email protected]>
* Copyright (c) 2012, PetaLogix
@@ -23,6 +24,7 @@
#include <uapi/linux/rpmsg.h>
#include "rpmsg_internal.h"
+#include "rpmsg_char.h"
#define RPMSG_DEV_MAX (MINORMASK + 1)
@@ -78,7 +80,7 @@ struct rpmsg_eptdev {
wait_queue_head_t readq;
};
-static int rpmsg_eptdev_destroy(struct device *dev, void *data)
+int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data)
{
struct rpmsg_eptdev *eptdev = dev_to_eptdev(dev);
@@ -97,6 +99,7 @@ static int rpmsg_eptdev_destroy(struct device *dev, void *data)
return 0;
}
+EXPORT_SYMBOL(rpmsg_chrdev_eptdev_destroy);
static int rpmsg_ept_cb(struct rpmsg_device *rpdev, void *buf, int len,
void *priv, u32 addr)
@@ -280,7 +283,7 @@ static long rpmsg_eptdev_ioctl(struct file *fp, unsigned int cmd,
if (cmd != RPMSG_DESTROY_EPT_IOCTL)
return -EINVAL;
- return rpmsg_eptdev_destroy(&eptdev->dev, NULL);
+ return rpmsg_chrdev_eptdev_destroy(&eptdev->dev, NULL);
}
static const struct file_operations rpmsg_eptdev_fops = {
@@ -339,10 +342,9 @@ static void rpmsg_eptdev_release_device(struct device *dev)
kfree(eptdev);
}
-static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
+int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent,
struct rpmsg_channel_info chinfo)
{
- struct rpmsg_device *rpdev = ctrldev->rpdev;
struct rpmsg_eptdev *eptdev;
struct device *dev;
int ret;
@@ -362,7 +364,7 @@ static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
device_initialize(dev);
dev->class = rpmsg_class;
- dev->parent = &ctrldev->dev;
+ dev->parent = parent;
dev->groups = rpmsg_eptdev_groups;
dev_set_drvdata(dev, eptdev);
@@ -405,6 +407,7 @@ static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
return ret;
}
+EXPORT_SYMBOL(rpmsg_chrdev_eptdev_create);
static int rpmsg_ctrldev_open(struct inode *inode, struct file *filp)
{
@@ -444,7 +447,7 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
chinfo.src = eptinfo.src;
chinfo.dst = eptinfo.dst;
- return rpmsg_eptdev_create(ctrldev, chinfo);
+ return rpmsg_chrdev_eptdev_create(ctrldev->rpdev, &ctrldev->dev, chinfo);
};
static const struct file_operations rpmsg_ctrldev_fops = {
@@ -530,7 +533,7 @@ static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev)
int ret;
/* Destroy all endpoints */
- ret = device_for_each_child(&ctrldev->dev, NULL, rpmsg_eptdev_destroy);
+ ret = device_for_each_child(&ctrldev->dev, NULL, rpmsg_chrdev_eptdev_destroy);
if (ret)
dev_warn(&rpdev->dev, "failed to nuke endpoints: %d\n", ret);
diff --git a/drivers/rpmsg/rpmsg_char.h b/drivers/rpmsg/rpmsg_char.h
new file mode 100644
index 000000000000..aa6e08a04577
--- /dev/null
+++ b/drivers/rpmsg/rpmsg_char.h
@@ -0,0 +1,51 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) STMicroelectronics 2021.
+ */
+
+#ifndef __RPMSG_CHRDEV_H__
+#define __RPMSG_CHRDEV_H__
+
+#if IS_ENABLED(CONFIG_RPMSG_CHAR)
+/**
+ * rpmsg_chrdev_eptdev_create() - register char device based on an endpoint
+ * @rpdev: prepared rpdev to be used for creating endpoints
+ * @parent: parent device
+ * @chinfo: associated endpoint channel information.
+ *
+ * This function create a new rpmsg char endpoint device to instantiate a new
+ * endpoint based on chinfo information.
+ */
+int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent,
+ struct rpmsg_channel_info chinfo);
+
+/**
+ * rpmsg_chrdev_eptdev_destroy() - destroy created char device endpoint.
+ * @data: private data associated to the endpoint device
+ *
+ * This function destroys a rpmsg char endpoint device created by the RPMSG_DESTROY_EPT_IOCTL
+ * control.
+ */
+int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data);
+
+#else /*IS_ENABLED(CONFIG_RPMSG_CHAR) */
+
+static inline int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent,
+ struct rpmsg_channel_info chinfo)
+{
+ /* This shouldn't be possible */
+ WARN_ON(1);
+ return -EINVAL;
+}
+
+static inline int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data)
+{
+ /* This shouldn't be possible */
+ WARN_ON(1);
+
+ return 0;
+}
+
+#endif /*IS_ENABLED(CONFIG_RPMSG_CHAR) */
+
+#endif /*__RPMSG_CHRDEV_H__ */
--
2.17.1
On Mon 08 Nov 08:19 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 <[email protected]>
> ---
> 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 6a01e8e1c111..dd754c870ba1 100644
> --- a/drivers/rpmsg/rpmsg_char.c
> +++ b/drivers/rpmsg/rpmsg_char.c
> @@ -432,6 +432,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.
> + * Do not allow the creation and release of an endpoint on /dev/rpmsgX open and close,
> + * reuse the default endpoint instead
> + */
> + 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);
Which get_device() does this correlate to?
> + kfree(eptdev);
After the device_initialize() in rpmsg_chrdev_eptdev_alloc() you're
supposed to put_device() &eptdev->dev, which would kfree(eptdev)...
Note though that rpmsg_eptdev_release_device() calls cdev_del(), which
you can't do. It was however recently reported that this cdev_del()
should be done in conjunction with the device_del() as the current
implementation enables a race between release and fops->open.
Regards,
Bjorn
> + 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;
> @@ -442,12 +494,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
> +
> #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
>
On Mon 08 Nov 08:19 CST 2021, Arnaud Pouliquen wrote:
> To prepare the split of the code related to the control (ctrldev)
> and the endpoint (eptdev) devices in 2 separate files:
>
> - Rename and export the functions in rpmsg_char.h.
>
> - Suppress the dependency with the rpmsg_ctrldev struct in the
> rpmsg_eptdev_create function.
>
> Suggested-by: Mathieu Poirier <[email protected]>
> Signed-off-by: Arnaud Pouliquen <[email protected]>
> ---
> Update vs previous revision:
> - change IS_REACHABLE by IS_ENABLE ( dependency will be fixed in kconfig instead
> - fix licensing
> ---
> drivers/rpmsg/rpmsg_char.c | 17 +++++++------
> drivers/rpmsg/rpmsg_char.h | 51 ++++++++++++++++++++++++++++++++++++++
> 2 files changed, 61 insertions(+), 7 deletions(-)
> create mode 100644 drivers/rpmsg/rpmsg_char.h
>
> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> index 2bebc9b2d163..44934d7fa3c4 100644
> --- a/drivers/rpmsg/rpmsg_char.c
> +++ b/drivers/rpmsg/rpmsg_char.c
> @@ -1,5 +1,6 @@
> // SPDX-License-Identifier: GPL-2.0
> /*
> + * Copyright (C) 2021, STMicroelectronics
> * Copyright (c) 2016, Linaro Ltd.
> * Copyright (c) 2012, Michal Simek <[email protected]>
> * Copyright (c) 2012, PetaLogix
> @@ -23,6 +24,7 @@
> #include <uapi/linux/rpmsg.h>
>
> #include "rpmsg_internal.h"
> +#include "rpmsg_char.h"
>
> #define RPMSG_DEV_MAX (MINORMASK + 1)
>
> @@ -78,7 +80,7 @@ struct rpmsg_eptdev {
> wait_queue_head_t readq;
> };
>
> -static int rpmsg_eptdev_destroy(struct device *dev, void *data)
> +int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data)
> {
> struct rpmsg_eptdev *eptdev = dev_to_eptdev(dev);
>
> @@ -97,6 +99,7 @@ static int rpmsg_eptdev_destroy(struct device *dev, void *data)
>
> return 0;
> }
> +EXPORT_SYMBOL(rpmsg_chrdev_eptdev_destroy);
>
> static int rpmsg_ept_cb(struct rpmsg_device *rpdev, void *buf, int len,
> void *priv, u32 addr)
> @@ -280,7 +283,7 @@ static long rpmsg_eptdev_ioctl(struct file *fp, unsigned int cmd,
> if (cmd != RPMSG_DESTROY_EPT_IOCTL)
> return -EINVAL;
>
> - return rpmsg_eptdev_destroy(&eptdev->dev, NULL);
> + return rpmsg_chrdev_eptdev_destroy(&eptdev->dev, NULL);
> }
>
> static const struct file_operations rpmsg_eptdev_fops = {
> @@ -339,10 +342,9 @@ static void rpmsg_eptdev_release_device(struct device *dev)
> kfree(eptdev);
> }
>
> -static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
> +int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent,
> struct rpmsg_channel_info chinfo)
> {
> - struct rpmsg_device *rpdev = ctrldev->rpdev;
> struct rpmsg_eptdev *eptdev;
> struct device *dev;
> int ret;
> @@ -362,7 +364,7 @@ static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
>
> device_initialize(dev);
> dev->class = rpmsg_class;
> - dev->parent = &ctrldev->dev;
> + dev->parent = parent;
> dev->groups = rpmsg_eptdev_groups;
> dev_set_drvdata(dev, eptdev);
>
> @@ -405,6 +407,7 @@ static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
>
> return ret;
> }
> +EXPORT_SYMBOL(rpmsg_chrdev_eptdev_create);
>
> static int rpmsg_ctrldev_open(struct inode *inode, struct file *filp)
> {
> @@ -444,7 +447,7 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
> chinfo.src = eptinfo.src;
> chinfo.dst = eptinfo.dst;
>
> - return rpmsg_eptdev_create(ctrldev, chinfo);
> + return rpmsg_chrdev_eptdev_create(ctrldev->rpdev, &ctrldev->dev, chinfo);
> };
>
> static const struct file_operations rpmsg_ctrldev_fops = {
> @@ -530,7 +533,7 @@ static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev)
> int ret;
>
> /* Destroy all endpoints */
> - ret = device_for_each_child(&ctrldev->dev, NULL, rpmsg_eptdev_destroy);
> + ret = device_for_each_child(&ctrldev->dev, NULL, rpmsg_chrdev_eptdev_destroy);
> if (ret)
> dev_warn(&rpdev->dev, "failed to nuke endpoints: %d\n", ret);
>
> diff --git a/drivers/rpmsg/rpmsg_char.h b/drivers/rpmsg/rpmsg_char.h
> new file mode 100644
> index 000000000000..aa6e08a04577
> --- /dev/null
> +++ b/drivers/rpmsg/rpmsg_char.h
> @@ -0,0 +1,51 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) STMicroelectronics 2021.
> + */
> +
> +#ifndef __RPMSG_CHRDEV_H__
> +#define __RPMSG_CHRDEV_H__
> +
> +#if IS_ENABLED(CONFIG_RPMSG_CHAR)
> +/**
> + * rpmsg_chrdev_eptdev_create() - register char device based on an endpoint
> + * @rpdev: prepared rpdev to be used for creating endpoints
> + * @parent: parent device
> + * @chinfo: associated endpoint channel information.
> + *
> + * This function create a new rpmsg char endpoint device to instantiate a new
> + * endpoint based on chinfo information.
> + */
> +int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent,
> + struct rpmsg_channel_info chinfo);
> +
> +/**
> + * rpmsg_chrdev_eptdev_destroy() - destroy created char device endpoint.
> + * @data: private data associated to the endpoint device
> + *
> + * This function destroys a rpmsg char endpoint device created by the RPMSG_DESTROY_EPT_IOCTL
> + * control.
> + */
> +int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data);
> +
> +#else /*IS_ENABLED(CONFIG_RPMSG_CHAR) */
> +
> +static inline int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent,
> + struct rpmsg_channel_info chinfo)
> +{
> + /* This shouldn't be possible */
But isn't it very much possible that userspace invokes this function
through the ioctl that you move to the separate rpmsg_ctrl driver?
> + WARN_ON(1);
Which would mean that this will spam the kernel with stack dumps.
Regards,
Bjorn
> + return -EINVAL;
> +}
> +
> +static inline int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data)
> +{
> + /* This shouldn't be possible */
> + WARN_ON(1);
> +
> + return 0;
> +}
> +
> +#endif /*IS_ENABLED(CONFIG_RPMSG_CHAR) */
> +
> +#endif /*__RPMSG_CHRDEV_H__ */
> --
> 2.17.1
>
On Mon 08 Nov 08:19 CST 2021, Arnaud Pouliquen wrote:
> Migrate the creation of the rpmsg class from the rpmsg_char
> to the core that the class is usable by the rpmsg_char and
> the futur rpmsg_ctrl module.
>
> Suggested-by: Bjorn Andersson <[email protected]>
> Signed-off-by: Arnaud Pouliquen <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
Regards,
Bjorn
> ---
> Update vs previous revision:
> - remove rpmsg_get_class API and export the rpmsg_class in rpmsg_internal.h
> ---
> drivers/rpmsg/rpmsg_char.c | 10 ----------
> drivers/rpmsg/rpmsg_core.c | 15 +++++++++++++--
> drivers/rpmsg/rpmsg_internal.h | 2 ++
> 3 files changed, 15 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> index 44934d7fa3c4..8ab5ac23850c 100644
> --- a/drivers/rpmsg/rpmsg_char.c
> +++ b/drivers/rpmsg/rpmsg_char.c
> @@ -29,7 +29,6 @@
> #define RPMSG_DEV_MAX (MINORMASK + 1)
>
> static dev_t rpmsg_major;
> -static struct class *rpmsg_class;
>
> static DEFINE_IDA(rpmsg_ctrl_ida);
> static DEFINE_IDA(rpmsg_ept_ida);
> @@ -559,17 +558,9 @@ static int rpmsg_chrdev_init(void)
> return ret;
> }
>
> - rpmsg_class = class_create(THIS_MODULE, "rpmsg");
> - if (IS_ERR(rpmsg_class)) {
> - pr_err("failed to create rpmsg class\n");
> - unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
> - return PTR_ERR(rpmsg_class);
> - }
> -
> ret = register_rpmsg_driver(&rpmsg_chrdev_driver);
> if (ret < 0) {
> pr_err("rpmsgchr: failed to register rpmsg driver\n");
> - class_destroy(rpmsg_class);
> unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
> }
>
> @@ -580,7 +571,6 @@ postcore_initcall(rpmsg_chrdev_init);
> static void rpmsg_chrdev_exit(void)
> {
> unregister_rpmsg_driver(&rpmsg_chrdev_driver);
> - class_destroy(rpmsg_class);
> unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
> }
> module_exit(rpmsg_chrdev_exit);
> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
> index 9151836190ce..45227c864aa2 100644
> --- a/drivers/rpmsg/rpmsg_core.c
> +++ b/drivers/rpmsg/rpmsg_core.c
> @@ -20,6 +20,9 @@
>
> #include "rpmsg_internal.h"
>
> +struct class *rpmsg_class;
> +EXPORT_SYMBOL(rpmsg_class);
> +
> /**
> * rpmsg_create_channel() - create a new rpmsg channel
> * using its name and address info.
> @@ -629,10 +632,17 @@ static int __init rpmsg_init(void)
> {
> int ret;
>
> + rpmsg_class = class_create(THIS_MODULE, "rpmsg");
> + if (IS_ERR(rpmsg_class)) {
> + pr_err("failed to create rpmsg class\n");
> + return PTR_ERR(rpmsg_class);
> + }
> +
> ret = bus_register(&rpmsg_bus);
> - if (ret)
> + if (ret) {
> pr_err("failed to register rpmsg bus: %d\n", ret);
> -
> + class_destroy(rpmsg_class);
> + }
> return ret;
> }
> postcore_initcall(rpmsg_init);
> @@ -640,6 +650,7 @@ postcore_initcall(rpmsg_init);
> static void __exit rpmsg_fini(void)
> {
> bus_unregister(&rpmsg_bus);
> + class_destroy(rpmsg_class);
> }
> module_exit(rpmsg_fini);
>
> diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
> index a76c344253bf..1b6f998e1a4a 100644
> --- a/drivers/rpmsg/rpmsg_internal.h
> +++ b/drivers/rpmsg/rpmsg_internal.h
> @@ -18,6 +18,8 @@
> #define to_rpmsg_device(d) container_of(d, struct rpmsg_device, dev)
> #define to_rpmsg_driver(d) container_of(d, struct rpmsg_driver, drv)
>
> +extern struct class *rpmsg_class;
> +
> /**
> * struct rpmsg_device_ops - indirection table for the rpmsg_device operations
> * @create_channel: create backend-specific channel, optional
> --
> 2.17.1
>
On Mon 08 Nov 08:19 CST 2021, Arnaud Pouliquen wrote:
> Current implementation create/destroy a new endpoint on each
> rpmsg_eptdev_open/rpmsg_eptdev_release calls.
>
> For a rpmsg device created by the NS announcement mechanism we need to
> use a unique static endpoint that is the default rpmsg device endpoint
> associated to the channel.
>
> This patch prepares the introduction of a rpmsg channel device for the
> char device. The rpmsg channel device will require a default endpoint to
> communicate to the remote processor.
>
> Add the default_ept field in rpmsg_eptdev structure.This pointer
> determines the behavior on rpmsg_eptdev_open and rpmsg_eptdev_release call.
>
> - If default_ept == NULL:
> Use the legacy behavior by creating a new endpoint each time
> rpmsg_eptdev_open is called and release it when rpmsg_eptdev_release
> is called on /dev/rpmsgX device open/close.
>
> - If default_ept is set:
> use the rpmsg device default endpoint for the communication.
>
> Address the update of _rpmsg_chrdev_eptdev_create in a separate patch for readability.
>
> Add protection in rpmsg_eptdev_ioctl to prevent to destroy a default endpoint.
>
> Signed-off-by: Arnaud Pouliquen <[email protected]>
> ---
> drivers/rpmsg/rpmsg_char.c | 21 +++++++++++++++++++--
> 1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> index 3daf62df69a0..6a01e8e1c111 100644
> --- a/drivers/rpmsg/rpmsg_char.c
> +++ b/drivers/rpmsg/rpmsg_char.c
> @@ -47,6 +47,8 @@ static DEFINE_IDA(rpmsg_minor_ida);
> * @queue_lock: synchronization of @queue operations
> * @queue: incoming message queue
> * @readq: wait object for incoming queue
> + * @default_ept: set to channel default endpoint if the default endpoint should be re-used
> + * on device open to prevent endpoint address update.
> */
> struct rpmsg_eptdev {
> struct device dev;
> @@ -57,10 +59,12 @@ struct rpmsg_eptdev {
>
> struct mutex ept_lock;
> struct rpmsg_endpoint *ept;
> + struct rpmsg_endpoint *default_ept;
>
> spinlock_t queue_lock;
> struct sk_buff_head queue;
> wait_queue_head_t readq;
> +
> };
>
> int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data)
> @@ -118,7 +122,15 @@ static int rpmsg_eptdev_open(struct inode *inode, struct file *filp)
>
> get_device(dev);
>
> - ept = rpmsg_create_ept(rpdev, rpmsg_ept_cb, eptdev, eptdev->chinfo);
> + /*
> + * If the default_ept is set to true, the rpmsg device default endpoint is used.
default_ept will no longer be "set to true".
Regards,
Bjorn
> + * Else a new endpoint is created on open that will be destroyed on release.
> + */
> + if (eptdev->default_ept)
> + ept = eptdev->default_ept;
> + else
> + ept = rpmsg_create_ept(rpdev, rpmsg_ept_cb, eptdev, eptdev->chinfo);
> +
> if (!ept) {
> dev_err(dev, "failed to open %s\n", eptdev->chinfo.name);
> put_device(dev);
> @@ -139,7 +151,8 @@ static int rpmsg_eptdev_release(struct inode *inode, struct file *filp)
> /* Close the endpoint, if it's not already destroyed by the parent */
> mutex_lock(&eptdev->ept_lock);
> if (eptdev->ept) {
> - rpmsg_destroy_ept(eptdev->ept);
> + if (!eptdev->default_ept)
> + rpmsg_destroy_ept(eptdev->ept);
> eptdev->ept = NULL;
> }
> mutex_unlock(&eptdev->ept_lock);
> @@ -266,6 +279,10 @@ static long rpmsg_eptdev_ioctl(struct file *fp, unsigned int cmd,
> if (cmd != RPMSG_DESTROY_EPT_IOCTL)
> return -EINVAL;
>
> + /* Don't allow to destroy a default endpoint. */
> + if (eptdev->default_ept)
> + return -EINVAL;
> +
> return rpmsg_chrdev_eptdev_destroy(&eptdev->dev, NULL);
> }
>
> --
> 2.17.1
>
Hello Bjorn,
On 12/3/21 3:17 AM, Bjorn Andersson wrote:
> On Mon 08 Nov 08:19 CST 2021, Arnaud Pouliquen wrote:
>
>> To prepare the split of the code related to the control (ctrldev)
>> and the endpoint (eptdev) devices in 2 separate files:
>>
>> - Rename and export the functions in rpmsg_char.h.
>>
>> - Suppress the dependency with the rpmsg_ctrldev struct in the
>> rpmsg_eptdev_create function.
>>
>> Suggested-by: Mathieu Poirier <[email protected]>
>> Signed-off-by: Arnaud Pouliquen <[email protected]>
>> ---
>> Update vs previous revision:
>> - change IS_REACHABLE by IS_ENABLE ( dependency will be fixed in kconfig instead
>> - fix licensing
>> ---
>> drivers/rpmsg/rpmsg_char.c | 17 +++++++------
>> drivers/rpmsg/rpmsg_char.h | 51 ++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 61 insertions(+), 7 deletions(-)
>> create mode 100644 drivers/rpmsg/rpmsg_char.h
>>
>> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
>> index 2bebc9b2d163..44934d7fa3c4 100644
>> --- a/drivers/rpmsg/rpmsg_char.c
>> +++ b/drivers/rpmsg/rpmsg_char.c
>> @@ -1,5 +1,6 @@
>> // SPDX-License-Identifier: GPL-2.0
>> /*
>> + * Copyright (C) 2021, STMicroelectronics
>> * Copyright (c) 2016, Linaro Ltd.
>> * Copyright (c) 2012, Michal Simek <[email protected]>
>> * Copyright (c) 2012, PetaLogix
>> @@ -23,6 +24,7 @@
>> #include <uapi/linux/rpmsg.h>
>>
>> #include "rpmsg_internal.h"
>> +#include "rpmsg_char.h"
>>
>> #define RPMSG_DEV_MAX (MINORMASK + 1)
>>
>> @@ -78,7 +80,7 @@ struct rpmsg_eptdev {
>> wait_queue_head_t readq;
>> };
>>
>> -static int rpmsg_eptdev_destroy(struct device *dev, void *data)
>> +int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data)
>> {
>> struct rpmsg_eptdev *eptdev = dev_to_eptdev(dev);
>>
>> @@ -97,6 +99,7 @@ static int rpmsg_eptdev_destroy(struct device *dev, void *data)
>>
>> return 0;
>> }
>> +EXPORT_SYMBOL(rpmsg_chrdev_eptdev_destroy);
>>
>> static int rpmsg_ept_cb(struct rpmsg_device *rpdev, void *buf, int len,
>> void *priv, u32 addr)
>> @@ -280,7 +283,7 @@ static long rpmsg_eptdev_ioctl(struct file *fp, unsigned int cmd,
>> if (cmd != RPMSG_DESTROY_EPT_IOCTL)
>> return -EINVAL;
>>
>> - return rpmsg_eptdev_destroy(&eptdev->dev, NULL);
>> + return rpmsg_chrdev_eptdev_destroy(&eptdev->dev, NULL);
>> }
>>
>> static const struct file_operations rpmsg_eptdev_fops = {
>> @@ -339,10 +342,9 @@ static void rpmsg_eptdev_release_device(struct device *dev)
>> kfree(eptdev);
>> }
>>
>> -static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
>> +int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent,
>> struct rpmsg_channel_info chinfo)
>> {
>> - struct rpmsg_device *rpdev = ctrldev->rpdev;
>> struct rpmsg_eptdev *eptdev;
>> struct device *dev;
>> int ret;
>> @@ -362,7 +364,7 @@ static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
>>
>> device_initialize(dev);
>> dev->class = rpmsg_class;
>> - dev->parent = &ctrldev->dev;
>> + dev->parent = parent;
>> dev->groups = rpmsg_eptdev_groups;
>> dev_set_drvdata(dev, eptdev);
>>
>> @@ -405,6 +407,7 @@ static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
>>
>> return ret;
>> }
>> +EXPORT_SYMBOL(rpmsg_chrdev_eptdev_create);
>>
>> static int rpmsg_ctrldev_open(struct inode *inode, struct file *filp)
>> {
>> @@ -444,7 +447,7 @@ static long rpmsg_ctrldev_ioctl(struct file *fp, unsigned int cmd,
>> chinfo.src = eptinfo.src;
>> chinfo.dst = eptinfo.dst;
>>
>> - return rpmsg_eptdev_create(ctrldev, chinfo);
>> + return rpmsg_chrdev_eptdev_create(ctrldev->rpdev, &ctrldev->dev, chinfo);
>> };
>>
>> static const struct file_operations rpmsg_ctrldev_fops = {
>> @@ -530,7 +533,7 @@ static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev)
>> int ret;
>>
>> /* Destroy all endpoints */
>> - ret = device_for_each_child(&ctrldev->dev, NULL, rpmsg_eptdev_destroy);
>> + ret = device_for_each_child(&ctrldev->dev, NULL, rpmsg_chrdev_eptdev_destroy);
>> if (ret)
>> dev_warn(&rpdev->dev, "failed to nuke endpoints: %d\n", ret);
>>
>> diff --git a/drivers/rpmsg/rpmsg_char.h b/drivers/rpmsg/rpmsg_char.h
>> new file mode 100644
>> index 000000000000..aa6e08a04577
>> --- /dev/null
>> +++ b/drivers/rpmsg/rpmsg_char.h
>> @@ -0,0 +1,51 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Copyright (C) STMicroelectronics 2021.
>> + */
>> +
>> +#ifndef __RPMSG_CHRDEV_H__
>> +#define __RPMSG_CHRDEV_H__
>> +
>> +#if IS_ENABLED(CONFIG_RPMSG_CHAR)
>> +/**
>> + * rpmsg_chrdev_eptdev_create() - register char device based on an endpoint
>> + * @rpdev: prepared rpdev to be used for creating endpoints
>> + * @parent: parent device
>> + * @chinfo: associated endpoint channel information.
>> + *
>> + * This function create a new rpmsg char endpoint device to instantiate a new
>> + * endpoint based on chinfo information.
>> + */
>> +int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent,
>> + struct rpmsg_channel_info chinfo);
>> +
>> +/**
>> + * rpmsg_chrdev_eptdev_destroy() - destroy created char device endpoint.
>> + * @data: private data associated to the endpoint device
>> + *
>> + * This function destroys a rpmsg char endpoint device created by the RPMSG_DESTROY_EPT_IOCTL
>> + * control.
>> + */
>> +int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data);
>> +
>> +#else /*IS_ENABLED(CONFIG_RPMSG_CHAR) */
>> +
>> +static inline int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent,
>> + struct rpmsg_channel_info chinfo)
>> +{
>> + /* This shouldn't be possible */
>
> But isn't it very much possible that userspace invokes this function
> through the ioctl that you move to the separate rpmsg_ctrl driver?
>
>> + WARN_ON(1);
>
> Which would mean that this will spam the kernel with stack dumps.
Good catch, I will suppress the WARM_ON. I propose also to return -ENXIO
instead of -EINVAL to be aligned with other functions in rpmsg.h
Thanks,
Arnaud
>
> Regards,
> Bjorn
>
>> + return -EINVAL;
>> +}
>> +
>> +static inline int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data)
>> +{
>> + /* This shouldn't be possible */
>> + WARN_ON(1);
>> +
>> + return 0;
>> +}
>> +
>> +#endif /*IS_ENABLED(CONFIG_RPMSG_CHAR) */
>> +
>> +#endif /*__RPMSG_CHRDEV_H__ */
>> --
>> 2.17.1
>>
On Fri 03 Dec 08:37 PST 2021, Arnaud POULIQUEN wrote:
> On 12/3/21 3:17 AM, Bjorn Andersson wrote:
> > On Mon 08 Nov 08:19 CST 2021, Arnaud Pouliquen wrote:
[..]
> >> +static inline int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent,
> >> + struct rpmsg_channel_info chinfo)
> >> +{
> >> + /* This shouldn't be possible */
> >
> > But isn't it very much possible that userspace invokes this function
> > through the ioctl that you move to the separate rpmsg_ctrl driver?
> >
> >> + WARN_ON(1);
> >
> > Which would mean that this will spam the kernel with stack dumps.
>
> Good catch, I will suppress the WARM_ON. I propose also to return -ENXIO
> instead of -EINVAL to be aligned with other functions in rpmsg.h
>
ENXIO sounds better than EINVAL, let's go with that.
Thanks,
Bjorn
On 12/3/21 2:52 AM, Bjorn Andersson wrote:
> On Mon 08 Nov 08:19 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 <[email protected]>
>> ---
>> 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 6a01e8e1c111..dd754c870ba1 100644
>> --- a/drivers/rpmsg/rpmsg_char.c
>> +++ b/drivers/rpmsg/rpmsg_char.c
>> @@ -432,6 +432,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.
>> + * Do not allow the creation and release of an endpoint on /dev/rpmsgX open and close,
>> + * reuse the default endpoint instead
>> + */
>> + 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);
>
> Which get_device() does this correlate to?
this is related to device_initialize [1]( and herited from the legacy
implementation of rpmsg_char)
[1]
https://elixir.bootlin.com/linux/v5.16-rc3/source/drivers/base/core.c#L2860
>
>> + kfree(eptdev);
>
> After the device_initialize() in rpmsg_chrdev_eptdev_alloc() you're
> supposed to put_device() &eptdev->dev, which would kfree(eptdev)...
dev->release is set only in rpmsg_chrdev_eptdev_add. and calling
rpmsg_chrdev_eptdev_add at this level would need to handle the free of some
uninitialized parameters.
That why I directly free it here.
>
>
> Note though that rpmsg_eptdev_release_device() calls cdev_del(), which
> you can't do. It was however recently reported that this cdev_del()
> should be done in conjunction with the device_del() as the current
> implementation enables a race between release and fops->open.
I'm not sure to understand your point here. Is it related to your previous
comment concerning the use of put_device or do you expect something from me
specific in the around device_del and cdev_del?
Thanks,
Arnaud
>
> Regards,
> Bjorn
>
>> + 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;
>> @@ -442,12 +494,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
>> +
>> #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
>>