Purpose:
Allow the remote processor to instantiate a /dev/rpmsgX interface relying on the NS announcement
of the "rpmsg-raw" service.
This patchet is extracted from the series [1] with rework to add rpmsg_create_default_ept helper.
Aim:
There is no generic sysfs interface based on RPMsg that allows a user application to communicate
with a remote processor in a simple way.
The rpmsg_char dev solves a part of this problem by allowing an endpoint to be created on the
local side. But it does not take advantage of the NS announcement mechanism implemented for some
backends such as the virtio backend. So it is not possible to probe it from a remote initiative.
Extending the char rpmsg device to support NS announcement makes the rpmsg_char more generic.
By announcing a "rpmg-raw" service, the firmware of a remote processor will be able to
instantiate a /dev/rpmsgX interface providing to the user application a basic link to communicate
with it without any knowledge of the rpmsg protocol.
Implementation details:
- Register a rpmsg driver for the rpmsg_char driver, associated to the "rpmsg-raw" channel service.
- In case of rpmsg char device instantiated by the rpmsg bus (on NS announcement) manage the
channel default endpoint to ensure a stable default endpoint address, for communication with
the remote processor.
delta vs V3:
- add Tested-by: Julien Massot <[email protected]>
- rebased on kernel V.14-rc1 +
patchset V5: Restructure the rpmsg char to decorrelate the control part [2]
How to test it:
- This series can be applied on e73f0f0ee754kernel V.14-rc1 (e73f0f0ee754)
+ the "Restructure the rpmsg char to decorrelate the control part" series[2]
[1] https://patchwork.kernel.org/project/linux-remoteproc/list/?series=475217
[2] https://patchwork.kernel.org/project/linux-remoteproc/list/?series=514017
Arnaud Pouliquen (4):
rpmsg: Introduce rpmsg_create_default_ept function
rpmsg: char: Introduce __rpmsg_chrdev_create_eptdev function
rpmsg: char: Add possibility to use default endpoint of the rpmsg
device.
rpmsg: char: Introduce the "rpmsg-raw" channel
drivers/rpmsg/rpmsg_char.c | 120 ++++++++++++++++++++++++++++++++++---
drivers/rpmsg/rpmsg_core.c | 51 ++++++++++++++++
include/linux/rpmsg.h | 13 ++++
3 files changed, 175 insertions(+), 9 deletions(-)
--
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 static_ept field in rpmsg_eptdev structure. This boolean
determines the behavior on rpmsg_eptdev_open and rpmsg_eptdev_release call.
- If static_ept == false:
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 static_ept == true:
use the rpmsg device default endpoint for the communication.
- Address the update of _rpmsg_chrdev_eptdev_create in e separate patch for readability.
Add protection in rpmsg_eptdev_ioctl to prevent to destroy a default endpoint.
Signed-off-by: Arnaud Pouliquen <[email protected]>
Reviewed-by: Mathieu Poirier <[email protected]>
Tested-by: Julien Massot <[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 50b7d4b00175..bd728d90ba4c 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -45,6 +45,8 @@ static DEFINE_IDA(rpmsg_minor_ida);
* @queue_lock: synchronization of @queue operations
* @queue: incoming message queue
* @readq: wait object for incoming queue
+ * @static_ept: specify if the endpoint has to be created at each device opening or
+ * if the default endpoint should be used.
*/
struct rpmsg_eptdev {
struct device dev;
@@ -59,6 +61,8 @@ struct rpmsg_eptdev {
spinlock_t queue_lock;
struct sk_buff_head queue;
wait_queue_head_t readq;
+
+ bool static_ept;
};
int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data)
@@ -116,7 +120,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 static_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->static_ept)
+ ept = rpdev->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);
@@ -137,7 +149,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->static_ept)
+ rpmsg_destroy_ept(eptdev->ept);
eptdev->ept = NULL;
}
mutex_unlock(&eptdev->ept_lock);
@@ -264,6 +277,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->ept == eptdev->rpdev->ept)
+ return -EPERM;
+
return rpmsg_chrdev_eptdev_destroy(&eptdev->dev, NULL);
}
--
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]>
Reviewed-by: Mathieu Poirier <[email protected]>
Tested-by: Julien Massot <[email protected]>
---
drivers/rpmsg/rpmsg_char.c | 75 +++++++++++++++++++++++++++++++++++++-
1 file changed, 73 insertions(+), 2 deletions(-)
diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index bd728d90ba4c..1b7b610e113d 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -25,6 +25,8 @@
#include "rpmsg_char.h"
+#define RPMSG_CHAR_DEVNAME "rpmsg-raw"
+
static dev_t rpmsg_major;
static struct class *rpmsg_class;
@@ -421,6 +423,61 @@ 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 rpmsg_endpoint *ept;
+
+ memcpy(chinfo.name, RPMSG_CHAR_DEVNAME, sizeof(RPMSG_CHAR_DEVNAME));
+ chinfo.src = rpdev->src;
+ chinfo.dst = rpdev->dst;
+
+ eptdev = __rpmsg_chrdev_eptdev_create(rpdev, &rpdev->dev, chinfo);
+ 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.
+ */
+ ept = rpmsg_create_default_ept(rpdev, rpmsg_ept_cb, eptdev, eptdev->chinfo);
+ if (!ept) {
+ dev_err(&rpdev->dev, "failed to create %s\n", eptdev->chinfo.name);
+ put_device(&eptdev->dev);
+ return -EINVAL;
+ }
+
+ /*
+ * Do not allow the creation and release of an endpoint on /dev/rpmsgX open and close,
+ * reuse the default endpoint instead
+ */
+ eptdev->static_ept = true;
+
+ return 0;
+}
+
+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_CHAR_DEVNAME },
+ { },
+};
+
+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;
@@ -434,16 +491,30 @@ static int rpmsg_chrdev_init(void)
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 = PTR_ERR(rpmsg_class);
+ goto free_region;
+ }
+
+ ret = register_rpmsg_driver(&rpmsg_chrdev_driver);
+ if (ret < 0) {
+ pr_err("rpmsg: failed to register rpmsg raw driver\n");
+ goto free_class;
}
return 0;
+
+free_class:
+ class_destroy(rpmsg_class);
+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);
class_destroy(rpmsg_class);
unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
}
--
2.17.1
On Mon 12 Jul 06:18 PDT 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.
>
Why do you need this endpoint associated with the channel? Afaict the
read/write operations still operate on eptdev->ept, so who does use the
default endpoint for the device?
> 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 static_ept field in rpmsg_eptdev structure. This boolean
> determines the behavior on rpmsg_eptdev_open and rpmsg_eptdev_release call.
>
> - If static_ept == false:
> 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 static_ept == true:
> use the rpmsg device default endpoint for the communication.
> - Address the update of _rpmsg_chrdev_eptdev_create in e separate patch for readability.
>
> Add protection in rpmsg_eptdev_ioctl to prevent to destroy a default endpoint.
>
> Signed-off-by: Arnaud Pouliquen <[email protected]>
> Reviewed-by: Mathieu Poirier <[email protected]>
> Tested-by: Julien Massot <[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 50b7d4b00175..bd728d90ba4c 100644
> --- a/drivers/rpmsg/rpmsg_char.c
> +++ b/drivers/rpmsg/rpmsg_char.c
> @@ -45,6 +45,8 @@ static DEFINE_IDA(rpmsg_minor_ida);
> * @queue_lock: synchronization of @queue operations
> * @queue: incoming message queue
> * @readq: wait object for incoming queue
> + * @static_ept: specify if the endpoint has to be created at each device opening or
> + * if the default endpoint should be used.
> */
> struct rpmsg_eptdev {
> struct device dev;
> @@ -59,6 +61,8 @@ struct rpmsg_eptdev {
> spinlock_t queue_lock;
> struct sk_buff_head queue;
> wait_queue_head_t readq;
> +
> + bool static_ept;
I think you can skip rpmsg_create_default_ept() if you just make this
struct rpmsg_endpoint *.
> };
>
> int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data)
> @@ -116,7 +120,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 static_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->static_ept)
> + ept = rpdev->ept;
This would be:
if (eptdev->static_ept)
ept = eptdev->static_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);
> @@ -137,7 +149,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->static_ept)
> + rpmsg_destroy_ept(eptdev->ept);
> eptdev->ept = NULL;
> }
> mutex_unlock(&eptdev->ept_lock);
> @@ -264,6 +277,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->ept == eptdev->rpdev->ept)
And this would be if:
if (eptdev->static_ept)
return -EPERM;
Wouldn't -EINVAL or something be better than -EPERM when you try to
destroy one of these endpoints?
It's not that we don't have permission, it's that it's not a valid
operation on this object.
Regards,
Bjorn
> + return -EPERM;
> +
> return rpmsg_chrdev_eptdev_destroy(&eptdev->dev, NULL);
> }
>
> --
> 2.17.1
>
On Mon 12 Jul 06:19 PDT 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]>
> Reviewed-by: Mathieu Poirier <[email protected]>
> Tested-by: Julien Massot <[email protected]>
> ---
> drivers/rpmsg/rpmsg_char.c | 75 +++++++++++++++++++++++++++++++++++++-
> 1 file changed, 73 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> index bd728d90ba4c..1b7b610e113d 100644
> --- a/drivers/rpmsg/rpmsg_char.c
> +++ b/drivers/rpmsg/rpmsg_char.c
> @@ -25,6 +25,8 @@
>
> #include "rpmsg_char.h"
>
> +#define RPMSG_CHAR_DEVNAME "rpmsg-raw"
> +
> static dev_t rpmsg_major;
> static struct class *rpmsg_class;
>
> @@ -421,6 +423,61 @@ 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 rpmsg_endpoint *ept;
> +
> + memcpy(chinfo.name, RPMSG_CHAR_DEVNAME, sizeof(RPMSG_CHAR_DEVNAME));
The length should relate to the size of the destination buffer.
This looks like an excellent job for strscpy_pad()
> + chinfo.src = rpdev->src;
> + chinfo.dst = rpdev->dst;
> +
> + eptdev = __rpmsg_chrdev_eptdev_create(rpdev, &rpdev->dev, chinfo);
Note that this creates a new endpoint device as a child of the rpdev,
while new endpoints created by RPMSG_CREATE_EPT_IOCTL are parented by
the rpmsg_ctrl device.
So it is possible to create two /dev/rpmsgN nodes for the same endpoint,
I believe with the outcome that this one will be open but
__rpmsg_create_ept() in virtio_rpmsg_bus should return NULL if the user
tries to open the other one.
> + 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.
> + */
> + ept = rpmsg_create_default_ept(rpdev, rpmsg_ept_cb, eptdev, eptdev->chinfo);
Why don't you just set rpdev->priv to eptdev and make rpmsg_ept_cb the
callback of your rpmsg_driver?
> + if (!ept) {
> + dev_err(&rpdev->dev, "failed to create %s\n", eptdev->chinfo.name);
> + put_device(&eptdev->dev);
> + return -EINVAL;
> + }
> +
> + /*
> + * Do not allow the creation and release of an endpoint on /dev/rpmsgX open and close,
> + * reuse the default endpoint instead
> + */
What happens when __rpmsg_chrdev_eptdev_create() delivers a uevent and
user space quickly calls open() on the newly created /dev/rpmsgN, before
the next line?
> + eptdev->static_ept = true;
> +
> + return 0;
> +}
> +
> +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_CHAR_DEVNAME },
I would expect that this list would grow, but you hard coded
RPMSG_CHAR_DEVNAME in probe, so that won't work.
Regards,
Bjorn
> + { },
> +};
> +
> +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;
> @@ -434,16 +491,30 @@ static int rpmsg_chrdev_init(void)
> 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 = PTR_ERR(rpmsg_class);
> + goto free_region;
> + }
> +
> + ret = register_rpmsg_driver(&rpmsg_chrdev_driver);
> + if (ret < 0) {
> + pr_err("rpmsg: failed to register rpmsg raw driver\n");
> + goto free_class;
> }
>
> return 0;
> +
> +free_class:
> + class_destroy(rpmsg_class);
> +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);
> class_destroy(rpmsg_class);
> unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
> }
> --
> 2.17.1
>
On 10/9/21 2:06 AM, Bjorn Andersson wrote:
> On Mon 12 Jul 06:19 PDT 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]>
>> Reviewed-by: Mathieu Poirier <[email protected]>
>> Tested-by: Julien Massot <[email protected]>
>> ---
>> drivers/rpmsg/rpmsg_char.c | 75 +++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 73 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
>> index bd728d90ba4c..1b7b610e113d 100644
>> --- a/drivers/rpmsg/rpmsg_char.c
>> +++ b/drivers/rpmsg/rpmsg_char.c
>> @@ -25,6 +25,8 @@
>>
>> #include "rpmsg_char.h"
>>
>> +#define RPMSG_CHAR_DEVNAME "rpmsg-raw"
>> +
>> static dev_t rpmsg_major;
>> static struct class *rpmsg_class;
>>
>> @@ -421,6 +423,61 @@ 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 rpmsg_endpoint *ept;
>> +
>> + memcpy(chinfo.name, RPMSG_CHAR_DEVNAME, sizeof(RPMSG_CHAR_DEVNAME));
>
> The length should relate to the size of the destination buffer.
> This looks like an excellent job for strscpy_pad()
Thanks for pointing it, i will have alook
>
>> + chinfo.src = rpdev->src;
>> + chinfo.dst = rpdev->dst;
>> +
>> + eptdev = __rpmsg_chrdev_eptdev_create(rpdev, &rpdev->dev, chinfo);
>
> Note that this creates a new endpoint device as a child of the rpdev,
> while new endpoints created by RPMSG_CREATE_EPT_IOCTL are parented by
> the rpmsg_ctrl device.
Right this is probed by the rpmsg bus.
>
> So it is possible to create two /dev/rpmsgN nodes for the same endpoint,
> I believe with the outcome that this one will be open but
> __rpmsg_create_ept() in virtio_rpmsg_bus should return NULL if the user
> tries to open the other one.
I do not observe this behavior on virtio backend. In my test I create 2
instances based on the ns announcement, /dev/rpmsg0 & /dev/rpmsg1 is created
Then I create a new instance using RPMSG_CREATE_EPT_IOCTL that create the
/dev/rpmsg2
The use of ida_simple_get in __rpmsg_chrdev_eptdev_create should prevent such
use case
Do you observe such behavior on your side, or only a concern?
>
>> + 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.
>> + */
>> + ept = rpmsg_create_default_ept(rpdev, rpmsg_ept_cb, eptdev, eptdev->chinfo);
>
> Why don't you just set rpdev->priv to eptdev and make rpmsg_ept_cb the
> callback of your rpmsg_driver?
you mean ept->priv i suppose.
Because the priv parameter is managed by the rpmsg backend, so I have to assume
that it is used for some other purposes in the backend.
>
>> + if (!ept) {
>> + dev_err(&rpdev->dev, "failed to create %s\n", eptdev->chinfo.name);
>> + put_device(&eptdev->dev);
>> + return -EINVAL;
>> + }
>> +
>> + /*
>> + * Do not allow the creation and release of an endpoint on /dev/rpmsgX open and close,
>> + * reuse the default endpoint instead
>> + */
>
> What happens when __rpmsg_chrdev_eptdev_create() delivers a uevent and
> user space quickly calls open() on the newly created /dev/rpmsgN, before
> the next line?
Right, here I can see 2 solutions:
- the use of a mutex to block the open
- move the rpmsg_create_default_ept in the open but in this case i need to keep
the eptdev->static_ept bool
A preference or an alternative?
>
>> + eptdev->static_ept = true;
>> +
>> + return 0;
>> +}
>> +
>> +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_CHAR_DEVNAME },
>
> I would expect that this list would grow, but you hard coded
> RPMSG_CHAR_DEVNAME in probe, so that won't work.
The point here is more the use of RPMSG_CHAR_DEVNAME in
memcpy(chinfo.name, RPMSG_CHAR_DEVNAME, sizeof(RPMSG_CHAR_DEVNAME));
right?
I can change this by rpdev->id->name and suppress RPMSG_CHAR_DEVNAME
Regards,
Arnaud
>
> Regards,
> Bjorn
>
>> + { },
>> +};
>> +
>> +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;
>> @@ -434,16 +491,30 @@ static int rpmsg_chrdev_init(void)
>> 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 = PTR_ERR(rpmsg_class);
>> + goto free_region;
>> + }
>> +
>> + ret = register_rpmsg_driver(&rpmsg_chrdev_driver);
>> + if (ret < 0) {
>> + pr_err("rpmsg: failed to register rpmsg raw driver\n");
>> + goto free_class;
>> }
>>
>> return 0;
>> +
>> +free_class:
>> + class_destroy(rpmsg_class);
>> +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);
>> class_destroy(rpmsg_class);
>> unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
>> }
>> --
>> 2.17.1
>>
On 10/9/21 1:42 AM, Bjorn Andersson wrote:
> On Mon 12 Jul 06:18 PDT 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.
>>
>
> Why do you need this endpoint associated with the channel? Afaict the
> read/write operations still operate on eptdev->ept, so who does use the
> default endpoint for the device?
>
on virtio backend each side has its endpoint address and have to know the
destination endpoint address. When you use the rpmsg_send() this send a message
to the remote side at the default dest address.
Now if we create the local endpoint at each fopen, how to ensure that the local
address is static?
elle using a dynamic address would imply that each time we open the device we
send this new address to the remote side.
Using the default endpoint of the channel make this simple to use.
>> 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 static_ept field in rpmsg_eptdev structure. This boolean
>> determines the behavior on rpmsg_eptdev_open and rpmsg_eptdev_release call.
>>
>> - If static_ept == false:
>> 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 static_ept == true:
>> use the rpmsg device default endpoint for the communication.
>> - Address the update of _rpmsg_chrdev_eptdev_create in e separate patch for readability.
>>
>> Add protection in rpmsg_eptdev_ioctl to prevent to destroy a default endpoint.
>>
>> Signed-off-by: Arnaud Pouliquen <[email protected]>
>> Reviewed-by: Mathieu Poirier <[email protected]>
>> Tested-by: Julien Massot <[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 50b7d4b00175..bd728d90ba4c 100644
>> --- a/drivers/rpmsg/rpmsg_char.c
>> +++ b/drivers/rpmsg/rpmsg_char.c
>> @@ -45,6 +45,8 @@ static DEFINE_IDA(rpmsg_minor_ida);
>> * @queue_lock: synchronization of @queue operations
>> * @queue: incoming message queue
>> * @readq: wait object for incoming queue
>> + * @static_ept: specify if the endpoint has to be created at each device opening or
>> + * if the default endpoint should be used.
>> */
>> struct rpmsg_eptdev {
>> struct device dev;
>> @@ -59,6 +61,8 @@ struct rpmsg_eptdev {
>> spinlock_t queue_lock;
>> struct sk_buff_head queue;
>> wait_queue_head_t readq;
>> +
>> + bool static_ept;
>
> I think you can skip rpmsg_create_default_ept() if you just make this
> struct rpmsg_endpoint *.
For the static_ept, your proposal seems to me cleaner, thanks!
For the rpmsg_create_default_ept. The virtio_rpmsg_announce_create and
virtio_rpmsg_announce_destroy are based on the default endpoint.
So to be able to manage ns announcement, seems better to have a default endpoint.
Today something missing in this function is the call of announce_create ops
I was waiting for the integration of the work before sending the fix but i can
include it:
@@ -143,6 +143,7 @@ struct rpmsg_endpoint *rpmsg_create_default_ept(struct
rpmsg_device *rpdev,
struct rpmsg_channel_info chinfo)
{
struct rpmsg_endpoint *ept;
+ int err;
if (WARN_ON(!rpdev))
return NULL;
@@ -162,6 +163,9 @@ struct rpmsg_endpoint *rpmsg_create_default_ept(struct
rpmsg_device *rpdev,
rpdev->ept = ept;
rpdev->src = ept->addr;
+ if (ept && rpdev->ops->announce_create)
+ err = rpdev->ops->announce_create(rpdev);
+
return ept;
}
>
>> };
>>
>> int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data)
>> @@ -116,7 +120,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 static_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->static_ept)
>> + ept = rpdev->ept;
>
> This would be:
> if (eptdev->static_ept)
> ept = eptdev->static_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);
>> @@ -137,7 +149,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->static_ept)
>> + rpmsg_destroy_ept(eptdev->ept);
>> eptdev->ept = NULL;
>> }
>> mutex_unlock(&eptdev->ept_lock);
>> @@ -264,6 +277,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->ept == eptdev->rpdev->ept)
>
> And this would be if:
> if (eptdev->static_ept)
> return -EPERM;
>
> Wouldn't -EINVAL or something be better than -EPERM when you try to
> destroy one of these endpoints?
>
> It's not that we don't have permission, it's that it's not a valid
> operation on this object.
what about -EACCES?
Thanks,
Arnaud
>
> Regards,
> Bjorn
>
>> + return -EPERM;
>> +
>> return rpmsg_chrdev_eptdev_destroy(&eptdev->dev, NULL);
>> }
>>
>> --
>> 2.17.1
>>