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.
How to test it:
- This series can be applied on git/andersson/remoteproc.git for-next branch (dc0e14fa833b)
+ 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=483793
Arnaud Pouliquen (4):
rpmsg: Introduce rpmsg_create_default_ept function
rpmsg: char: Add possibility to create and reuse default endpoint
rpmsg: char: Introduce the "rpmsg-raw" channel
rpmsg: char: Return error if user tries to destroy a default endpoint.
drivers/rpmsg/rpmsg_char.c | 92 +++++++++++++++++++++++++++++++++++---
drivers/rpmsg/rpmsg_core.c | 51 +++++++++++++++++++++
include/linux/rpmsg.h | 14 ++++++
3 files changed, 151 insertions(+), 6 deletions(-)
--
2.17.1
Using the RPMSG_DESTROY_EPT_IOCTL control, user application can
destroy an endpoint. This patch prevents to destroy a default endpoint
associated to a channel.
This update is needed to manage the "rpmsg-raw" channel. In this
case a default endpoint is used, destroying it without the
channel does not make sense.
Signed-off-by: Arnaud Pouliquen <[email protected]>
---
drivers/rpmsg/rpmsg_char.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index 3b850b218eb0..8c78a5a192c1 100644
--- a/drivers/rpmsg/rpmsg_char.c
+++ b/drivers/rpmsg/rpmsg_char.c
@@ -285,6 +285,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->rpdev || 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]>
---
drivers/rpmsg/rpmsg_char.c | 54 ++++++++++++++++++++++++++++++++++++--
1 file changed, 52 insertions(+), 2 deletions(-)
diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index 4199ac1bee10..3b850b218eb0 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;
@@ -416,6 +418,40 @@ 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;
+
+ memcpy(chinfo.name, RPMSG_CHAR_DEVNAME, sizeof(RPMSG_CHAR_DEVNAME));
+ chinfo.src = rpdev->src;
+ chinfo.dst = rpdev->dst;
+
+ return __rpmsg_chrdev_eptdev_create(rpdev, &rpdev->dev, chinfo, true);
+}
+
+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;
@@ -429,16 +465,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
The rpmsg devices can be probed without default endpoint. This function
provides the capability for rpmsg drivers to create a default endpoint
on runtime.
For example, a driver might want the rpmsg core dispatcher to drop its
messages until it is ready to process them. In this case, the driver will
create the default endpoint when the conditions are met to process the
messages.
Signed-off-by: Arnaud Pouliquen <[email protected]>
---
drivers/rpmsg/rpmsg_core.c | 51 ++++++++++++++++++++++++++++++++++++++
include/linux/rpmsg.h | 14 +++++++++++
2 files changed, 65 insertions(+)
diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
index e5daee4f9373..07b680bda61f 100644
--- a/drivers/rpmsg/rpmsg_core.c
+++ b/drivers/rpmsg/rpmsg_core.c
@@ -115,6 +115,57 @@ struct rpmsg_endpoint *rpmsg_create_ept(struct rpmsg_device *rpdev,
}
EXPORT_SYMBOL(rpmsg_create_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 the default endpoint if already created or creates
+ * a endpoint and assign it 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).
+ *
+ * Returns 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;
+
+ /* 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_destroy_ept() - destroy an existing rpmsg endpoint
* @ept: endpoing to destroy
diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
index d97dcd049f18..ab034061722c 100644
--- a/include/linux/rpmsg.h
+++ b/include/linux/rpmsg.h
@@ -172,6 +172,9 @@ void rpmsg_destroy_ept(struct rpmsg_endpoint *);
struct rpmsg_endpoint *rpmsg_create_ept(struct rpmsg_device *,
rpmsg_rx_cb_t cb, void *priv,
struct rpmsg_channel_info chinfo);
+struct rpmsg_endpoint *rpmsg_create_default_ept(struct rpmsg_device *rpdev,
+ rpmsg_rx_cb_t cb, void *priv,
+ struct rpmsg_channel_info chinfo);
int rpmsg_send(struct rpmsg_endpoint *ept, void *data, int len);
int rpmsg_sendto(struct rpmsg_endpoint *ept, void *data, int len, u32 dst);
@@ -234,6 +237,17 @@ static inline struct rpmsg_endpoint *rpmsg_create_ept(struct rpmsg_device *rpdev
return ERR_PTR(-ENXIO);
}
+static inline struct rpmsg_endpoint *rpmsg_create_default_ept(struct rpmsg_device *rpdev,
+ rpmsg_rx_cb_t cb,
+ void *priv,
+ struct rpmsg_channel_info chinfo)
+{
+ /* This shouldn't be possible */
+ WARN_ON(1);
+
+ return NULL;
+}
+
static inline int rpmsg_send(struct rpmsg_endpoint *ept, void *data, int len)
{
/* This shouldn't be possible */
--
2.17.1
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 use_default_ept field in rpmsg_eptdev structure. This boolean
determines the behavior on rpmsg_eptdev_open and rpmsg_eptdev_release call.
- If use_default_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 use_default_ept == true:
create a endpoint only on first rpmsg_eptdev_open call (if no default
endpoint already exists) and associate it to the default endpoint.
The endpoint is released when the rpmsg device is removed.
Signed-off-by: Arnaud Pouliquen <[email protected]>
---
drivers/rpmsg/rpmsg_char.c | 34 ++++++++++++++++++++++++++++++----
1 file changed, 30 insertions(+), 4 deletions(-)
diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
index fbe10d527c5c..4199ac1bee10 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
+ * @use_default_ept: specify if the endpoint has to be created at each device opening or
+ * if the default endpoint should be reused.
*/
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 use_default_ept;
};
int rpmsg_chrdev_eptdev_destroy(struct device *dev, void *data)
@@ -116,7 +120,21 @@ 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 rpmsg device default endpoint is used, create it only the first time then reuse
+ * it. Else a new endpoint is created on open that will be destroyed on release.
+ */
+ if (eptdev->use_default_ept) {
+ ept = rpdev->ept;
+ if (!ept) {
+ ept = rpmsg_create_default_ept(rpdev, rpmsg_ept_cb, eptdev, eptdev->chinfo);
+ if (ept)
+ eptdev->chinfo.src = rpdev->src;
+ }
+ } 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 +155,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->use_default_ept)
+ rpmsg_destroy_ept(eptdev->ept);
eptdev->ept = NULL;
}
mutex_unlock(&eptdev->ept_lock);
@@ -323,8 +342,8 @@ static void rpmsg_eptdev_release_device(struct device *dev)
kfree(eptdev);
}
-int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent,
- struct rpmsg_channel_info chinfo)
+static int __rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent,
+ struct rpmsg_channel_info chinfo, bool use_default_ept)
{
struct rpmsg_eptdev *eptdev;
struct device *dev;
@@ -337,6 +356,7 @@ int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent
dev = &eptdev->dev;
eptdev->rpdev = rpdev;
eptdev->chinfo = chinfo;
+ eptdev->use_default_ept = use_default_ept;
mutex_init(&eptdev->ept_lock);
spin_lock_init(&eptdev->queue_lock);
@@ -388,6 +408,12 @@ int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent
return ret;
}
+
+int rpmsg_chrdev_eptdev_create(struct rpmsg_device *rpdev, struct device *parent,
+ struct rpmsg_channel_info chinfo)
+{
+ return __rpmsg_chrdev_eptdev_create(rpdev, parent, chinfo, false);
+}
EXPORT_SYMBOL(rpmsg_chrdev_eptdev_create);
static int rpmsg_chrdev_init(void)
--
2.17.1
Hi,
On 6/7/21 7:30 PM, Arnaud Pouliquen wrote:
> 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.
>
> How to test it:
> - This series can be applied on git/andersson/remoteproc.git for-next branch (dc0e14fa833b)
> + 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=483793
Just tested this whole series on remoteproc for-next branch + [2].
Works for me, /dev/rpmsg0 is created on NS announcement, and removed when stopping the remote processor.
I can do a repeated series of open /read/write/close, and got EBUSY if I try to open it more than once
at a time.
Firmware used for testing is derived from:
https://github.com/zephyrproject-rtos/zephyr/tree/main/samples/subsys/ipc/openamp_rsc_table
with:
#define RPMSG_CHAN_NAME "rpmsg-raw"
Thanks Arnaud for your work.
Tested-by: Julien Massot <[email protected]>
--
Julien Massot [IoT.bzh]
Hi Mathieu,
On 6/8/21 4:26 PM, Mathieu Poirier wrote:
> Hi Arnaud,
>
> Between remoteproc/RPMSG and CoreSight, I have 6 patchsets to review
> (including some of your work) before getting to this one. As such it
> will take a little while.
No worries, i knew you have already some patchsets in your pipe.
As i used this series to test the rpmsg_ctrl [1], both were ready at the same time.
So i decided to push it because it can also help you to perform tests on the
series [1].
[1] https://patchwork.kernel.org/project/linux-remoteproc/list/?series=494021
Thanks,
Arnaud
>
> Thanks,
> Mathieu
>
> On Mon, 7 Jun 2021 at 11:30, Arnaud Pouliquen
> <[email protected]> wrote:
>>
>> 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.
>>
>> How to test it:
>> - This series can be applied on git/andersson/remoteproc.git for-next branch (dc0e14fa833b)
>> + 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=483793
>>
>>
>>
>> Arnaud Pouliquen (4):
>> rpmsg: Introduce rpmsg_create_default_ept function
>> rpmsg: char: Add possibility to create and reuse default endpoint
>> rpmsg: char: Introduce the "rpmsg-raw" channel
>> rpmsg: char: Return error if user tries to destroy a default endpoint.
>>
>> drivers/rpmsg/rpmsg_char.c | 92 +++++++++++++++++++++++++++++++++++---
>> drivers/rpmsg/rpmsg_core.c | 51 +++++++++++++++++++++
>> include/linux/rpmsg.h | 14 ++++++
>> 3 files changed, 151 insertions(+), 6 deletions(-)
>>
>> --
>> 2.17.1
>>
Hi Arnaud,
Between remoteproc/RPMSG and CoreSight, I have 6 patchsets to review
(including some of your work) before getting to this one. As such it
will take a little while.
Thanks,
Mathieu
On Mon, 7 Jun 2021 at 11:30, Arnaud Pouliquen
<[email protected]> wrote:
>
> 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.
>
> How to test it:
> - This series can be applied on git/andersson/remoteproc.git for-next branch (dc0e14fa833b)
> + 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=483793
>
>
>
> Arnaud Pouliquen (4):
> rpmsg: Introduce rpmsg_create_default_ept function
> rpmsg: char: Add possibility to create and reuse default endpoint
> rpmsg: char: Introduce the "rpmsg-raw" channel
> rpmsg: char: Return error if user tries to destroy a default endpoint.
>
> drivers/rpmsg/rpmsg_char.c | 92 +++++++++++++++++++++++++++++++++++---
> drivers/rpmsg/rpmsg_core.c | 51 +++++++++++++++++++++
> include/linux/rpmsg.h | 14 ++++++
> 3 files changed, 151 insertions(+), 6 deletions(-)
>
> --
> 2.17.1
>
On Mon, Jun 07, 2021 at 07:30:31PM +0200, 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 | 54 ++++++++++++++++++++++++++++++++++++--
> 1 file changed, 52 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> index 4199ac1bee10..3b850b218eb0 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;
>
> @@ -416,6 +418,40 @@ 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;
> +
> + memcpy(chinfo.name, RPMSG_CHAR_DEVNAME, sizeof(RPMSG_CHAR_DEVNAME));
> + chinfo.src = rpdev->src;
> + chinfo.dst = rpdev->dst;
> +
> + return __rpmsg_chrdev_eptdev_create(rpdev, &rpdev->dev, chinfo, true);
I am a little puzzled here as to why we need different modes... Why can't we
simply call rpmsg_chrdev_eptdev_create() and let the endpoint be created on
open() and destroyed on release() as per the current implementation?
I'd rather keep things simple for the refactoring and introduce new features
later if need be.
As I said, it may be that I don't understand the usecase.
Thanks,
Mathieu
> +}
> +
> +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;
> @@ -429,16 +465,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
>
Hi Mathieu,
On 6/15/21 10:01 PM, Mathieu Poirier wrote:
> On Mon, Jun 07, 2021 at 07:30:31PM +0200, 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 | 54 ++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 52 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
>> index 4199ac1bee10..3b850b218eb0 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;
>>
>> @@ -416,6 +418,40 @@ 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;
>> +
>> + memcpy(chinfo.name, RPMSG_CHAR_DEVNAME, sizeof(RPMSG_CHAR_DEVNAME));
>> + chinfo.src = rpdev->src;
>> + chinfo.dst = rpdev->dst;
>> +
>> + return __rpmsg_chrdev_eptdev_create(rpdev, &rpdev->dev, chinfo, true);
>
> I am a little puzzled here as to why we need different modes... Why can't we
> simply call rpmsg_chrdev_eptdev_create() and let the endpoint be created on
> open() and destroyed on release() as per the current implementation?
The main reason is the support of the NS announcement
a NS announcement is received from the remote processor:
channel name: "rpmsg-raw"
remote address (dst address): 0x400
local address (scr address) : RPMSG_ADDR_ANY
=> no default endpoint, and not local address.
case 1) if we use legacy implementation ( no default endpoint)
=> create/destroy endpoint on open/stop
- on first open: created endpoint is bound to scr address 0x406
- a first message is sent to the remote side, the address 0x406 is stored as
default channel dst address on remote side.
- on close: endpoint is closed and associated address 0x406 is free.
- another driver create an enpoint the address 0x406 is reserved for this new
endpoint.
- on new open: scr address is set to next value 0x407
=> how to inform remote processor that the address has changed?
=> no reservation mechanism that ensure that you can reuse the same address
case 2) relying on use_default_ept
=> Ensure that both side have always the same addresses to communicate.
>
> I'd rather keep things simple for the refactoring and introduce new features
> later if need be.
Yes I agree with you, but here it could become a nightmare for the remote
processor if the Linux endpoint address is not stable.
Anyway we can consider this as a workaround waiting the extension of the NS
announcement to have a better management of the address exchange on channel
initialization.
Thanks
Arnaud
>
> As I said, it may be that I don't understand the usecase.
>
> Thanks,
> Mathieu
>
>> +}
>> +
>> +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;
>> @@ -429,16 +465,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 Wed, Jun 16, 2021 at 02:38:26PM +0200, Arnaud POULIQUEN wrote:
> Hi Mathieu,
>
> On 6/15/21 10:01 PM, Mathieu Poirier wrote:
> > On Mon, Jun 07, 2021 at 07:30:31PM +0200, 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 | 54 ++++++++++++++++++++++++++++++++++++--
> >> 1 file changed, 52 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> >> index 4199ac1bee10..3b850b218eb0 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;
> >>
> >> @@ -416,6 +418,40 @@ 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;
> >> +
> >> + memcpy(chinfo.name, RPMSG_CHAR_DEVNAME, sizeof(RPMSG_CHAR_DEVNAME));
> >> + chinfo.src = rpdev->src;
> >> + chinfo.dst = rpdev->dst;
> >> +
> >> + return __rpmsg_chrdev_eptdev_create(rpdev, &rpdev->dev, chinfo, true);
> >
> > I am a little puzzled here as to why we need different modes... Why can't we
> > simply call rpmsg_chrdev_eptdev_create() and let the endpoint be created on
> > open() and destroyed on release() as per the current implementation?
>
> The main reason is the support of the NS announcement
> a NS announcement is received from the remote processor:
> channel name: "rpmsg-raw"
> remote address (dst address): 0x400
> local address (scr address) : RPMSG_ADDR_ANY
> => no default endpoint, and not local address.
>
> case 1) if we use legacy implementation ( no default endpoint)
> => create/destroy endpoint on open/stop
> - on first open: created endpoint is bound to scr address 0x406
> - a first message is sent to the remote side, the address 0x406 is stored as
> default channel dst address on remote side.
> - on close: endpoint is closed and associated address 0x406 is free.
> - another driver create an enpoint the address 0x406 is reserved for this new
> endpoint.
> - on new open: scr address is set to next value 0x407
> => how to inform remote processor that the address has changed?
> => no reservation mechanism that ensure that you can reuse the same address
>
> case 2) relying on use_default_ept
> => Ensure that both side have always the same addresses to communicate.
I see the problem and your solution is adequate - I think the code simply needs
to be moved around a little. Here is what I suggest:
1) Create the endpoint in rpmsg_chrdev_probe(), just before calling
rpmsg_chrdev_eptdev_create(). That way changes to rpmsg_eptdev_open() can be
kept to a minimum. I don't think we'll be needing
__rpmsg_chrdev_eptdev_create() anymore.
2) We can get rid of use_default_ept by taking advantage of the fact that the
rpmsg_char driver does not use rpmsg_device::ept. If we create the endpoint in
rpmsg_chrdev_probe() we know that if rpdev->ept exists, we must not create
or destroy the endpoint in rpmsg_eptdev_open() and rpmsg_eptdev_release().
3) Function rpmsg_eptdev_open() doesn't change much. If rpdev->ept is NULL than
an endpoint is created as the current implementation. Otherwise we simply do:
eptdev->ept = rpdev->ept;
4) Make sure the teardown path works as well. From what I can see, it should.
5) Add a __lot__ of comments.
If the above all works this entire patchset should become really small.
>
> >
> > I'd rather keep things simple for the refactoring and introduce new features
> > later if need be.
>
> Yes I agree with you, but here it could become a nightmare for the remote
> processor if the Linux endpoint address is not stable.
>
> Anyway we can consider this as a workaround waiting the extension of the NS
> announcement to have a better management of the address exchange on channel
> initialization.
>
> Thanks
> Arnaud
>
> >
> > As I said, it may be that I don't understand the usecase.
> >
> > Thanks,
> > Mathieu
> >
> >> +}
> >> +
> >> +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;
> >> @@ -429,16 +465,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
> >>
Hi Mathieu,
On 6/17/21 11:31 PM, Mathieu Poirier wrote:
> On Wed, Jun 16, 2021 at 02:38:26PM +0200, Arnaud POULIQUEN wrote:
>> Hi Mathieu,
>>
>> On 6/15/21 10:01 PM, Mathieu Poirier wrote:
>>> On Mon, Jun 07, 2021 at 07:30:31PM +0200, 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 | 54 ++++++++++++++++++++++++++++++++++++--
>>>> 1 file changed, 52 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
>>>> index 4199ac1bee10..3b850b218eb0 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;
>>>>
>>>> @@ -416,6 +418,40 @@ 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;
>>>> +
>>>> + memcpy(chinfo.name, RPMSG_CHAR_DEVNAME, sizeof(RPMSG_CHAR_DEVNAME));
>>>> + chinfo.src = rpdev->src;
>>>> + chinfo.dst = rpdev->dst;
>>>> +
>>>> + return __rpmsg_chrdev_eptdev_create(rpdev, &rpdev->dev, chinfo, true);
>>>
>>> I am a little puzzled here as to why we need different modes... Why can't we
>>> simply call rpmsg_chrdev_eptdev_create() and let the endpoint be created on
>>> open() and destroyed on release() as per the current implementation?
>>
>> The main reason is the support of the NS announcement
>> a NS announcement is received from the remote processor:
>> channel name: "rpmsg-raw"
>> remote address (dst address): 0x400
>> local address (scr address) : RPMSG_ADDR_ANY
>> => no default endpoint, and not local address.
>>
>> case 1) if we use legacy implementation ( no default endpoint)
>> => create/destroy endpoint on open/stop
>> - on first open: created endpoint is bound to scr address 0x406
>> - a first message is sent to the remote side, the address 0x406 is stored as
>> default channel dst address on remote side.
>> - on close: endpoint is closed and associated address 0x406 is free.
>> - another driver create an enpoint the address 0x406 is reserved for this new
>> endpoint.
>> - on new open: scr address is set to next value 0x407
>> => how to inform remote processor that the address has changed?
>> => no reservation mechanism that ensure that you can reuse the same address
>>
>> case 2) relying on use_default_ept
>> => Ensure that both side have always the same addresses to communicate.
>
> I see the problem and your solution is adequate - I think the code simply needs
> to be moved around a little. Here is what I suggest:
>
> 1) Create the endpoint in rpmsg_chrdev_probe(), just before calling
> rpmsg_chrdev_eptdev_create(). That way changes to rpmsg_eptdev_open() can be
> kept to a minimum. I don't think we'll be needing
> __rpmsg_chrdev_eptdev_create() anymore.
Yes i could, but this will break a concept of the rpmsg_char that creates the
endpoint on open, meaning that application is ready to communicate.
I would rather preserve this behavior.
>
> 2) We can get rid of use_default_ept by taking advantage of the fact that the
> rpmsg_char driver does not use rpmsg_device::ept. If we create the endpoint in
> rpmsg_chrdev_probe() we know that if rpdev->ept exists, we must not create
> or destroy the endpoint in rpmsg_eptdev_open() and rpmsg_eptdev_release().
>
> 3) Function rpmsg_eptdev_open() doesn't change much. If rpdev->ept is NULL
> than
> an endpoint is created as the current implementation. Otherwise we simply do:
>
> eptdev->ept = rpdev->ept;
>
In qcom_glink_create_chrdev, a rpmsg_ctrl rpdev with a default endpoint is
created and used as parameter of the pmsg_ctrldev_register_device [1]
=> rpdev->ept is not NULL.
So the rpmsg_char has to differentiate 2 cases on rpmsg_eptdev_open:
- A enpdoint has to be created as requested by RPMSG_CREATE_EPT_IOCTL
(regardless of the rpdev->ept value)
- for a rpmsg device created by an NS announcement: A default endpoint has to be
reused (or created if rpdev->ept is null).
so the rpdev->ept test is not relevant for decision, the use_default_ept ( or
another flag) is mandatory.
> 4) Make sure the teardown path works as well. From what I can see, it should.
>
> 5) Add a __lot__ of comments.
>
> If the above all works this entire patchset should become really small.
Thanks,
Arnaud
>
>>
>>>
>>> I'd rather keep things simple for the refactoring and introduce new features
>>> later if need be.
>>
>> Yes I agree with you, but here it could become a nightmare for the remote
>> processor if the Linux endpoint address is not stable.
>>
>> Anyway we can consider this as a workaround waiting the extension of the NS
>> announcement to have a better management of the address exchange on channel
>> initialization.
>>
>> Thanks
>> Arnaud
>>
>>>
>>> As I said, it may be that I don't understand the usecase.
>>>
>>> Thanks,
>>> Mathieu
>>>
>>>> +}
>>>> +
>>>> +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;
>>>> @@ -429,16 +465,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 Fri, Jun 18, 2021 at 01:35:43PM +0200, Arnaud POULIQUEN wrote:
> Hi Mathieu,
>
> On 6/17/21 11:31 PM, Mathieu Poirier wrote:
> > On Wed, Jun 16, 2021 at 02:38:26PM +0200, Arnaud POULIQUEN wrote:
> >> Hi Mathieu,
> >>
> >> On 6/15/21 10:01 PM, Mathieu Poirier wrote:
> >>> On Mon, Jun 07, 2021 at 07:30:31PM +0200, 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 | 54 ++++++++++++++++++++++++++++++++++++--
> >>>> 1 file changed, 52 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> >>>> index 4199ac1bee10..3b850b218eb0 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;
> >>>>
> >>>> @@ -416,6 +418,40 @@ 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;
> >>>> +
> >>>> + memcpy(chinfo.name, RPMSG_CHAR_DEVNAME, sizeof(RPMSG_CHAR_DEVNAME));
> >>>> + chinfo.src = rpdev->src;
> >>>> + chinfo.dst = rpdev->dst;
> >>>> +
> >>>> + return __rpmsg_chrdev_eptdev_create(rpdev, &rpdev->dev, chinfo, true);
> >>>
> >>> I am a little puzzled here as to why we need different modes... Why can't we
> >>> simply call rpmsg_chrdev_eptdev_create() and let the endpoint be created on
> >>> open() and destroyed on release() as per the current implementation?
> >>
> >> The main reason is the support of the NS announcement
> >> a NS announcement is received from the remote processor:
> >> channel name: "rpmsg-raw"
> >> remote address (dst address): 0x400
> >> local address (scr address) : RPMSG_ADDR_ANY
> >> => no default endpoint, and not local address.
> >>
> >> case 1) if we use legacy implementation ( no default endpoint)
> >> => create/destroy endpoint on open/stop
> >> - on first open: created endpoint is bound to scr address 0x406
> >> - a first message is sent to the remote side, the address 0x406 is stored as
> >> default channel dst address on remote side.
> >> - on close: endpoint is closed and associated address 0x406 is free.
> >> - another driver create an enpoint the address 0x406 is reserved for this new
> >> endpoint.
> >> - on new open: scr address is set to next value 0x407
> >> => how to inform remote processor that the address has changed?
> >> => no reservation mechanism that ensure that you can reuse the same address
> >>
> >> case 2) relying on use_default_ept
> >> => Ensure that both side have always the same addresses to communicate.
> >
> > I see the problem and your solution is adequate - I think the code simply needs
> > to be moved around a little. Here is what I suggest:
> >
> > 1) Create the endpoint in rpmsg_chrdev_probe(), just before calling
> > rpmsg_chrdev_eptdev_create(). That way changes to rpmsg_eptdev_open() can be
> > kept to a minimum. I don't think we'll be needing
> > __rpmsg_chrdev_eptdev_create() anymore.
>
> Yes i could, but this will break a concept of the rpmsg_char that creates the
> endpoint on open, meaning that application is ready to communicate.
In my opinion creating and destorying an endpoint on open/close is something we
want to move away from.
>
> I would rather preserve this behavior.
>
> >
> > 2) We can get rid of use_default_ept by taking advantage of the fact that the
> > rpmsg_char driver does not use rpmsg_device::ept. If we create the endpoint in
> > rpmsg_chrdev_probe() we know that if rpdev->ept exists, we must not create
> > or destroy the endpoint in rpmsg_eptdev_open() and rpmsg_eptdev_release().
> >
> > 3) Function rpmsg_eptdev_open() doesn't change much. If rpdev->ept is NULL
> > than
> > an endpoint is created as the current implementation. Otherwise we simply do:
> >
> > eptdev->ept = rpdev->ept;
> >
>
> In qcom_glink_create_chrdev, a rpmsg_ctrl rpdev with a default endpoint is
> created and used as parameter of the pmsg_ctrldev_register_device [1]
> => rpdev->ept is not NULL.
>
> So the rpmsg_char has to differentiate 2 cases on rpmsg_eptdev_open:
> - A enpdoint has to be created as requested by RPMSG_CREATE_EPT_IOCTL
> (regardless of the rpdev->ept value)
> - for a rpmsg device created by an NS announcement: A default endpoint has to be
> reused (or created if rpdev->ept is null).
>
> so the rpdev->ept test is not relevant for decision, the use_default_ept ( or
> another flag) is mandatory.
Yes, we need a flag. May I suggest "fixed_ept" rather than "used_default_ept"?
>
>
> > 4) Make sure the teardown path works as well. From what I can see, it should.
> >
> > 5) Add a __lot__ of comments.
> >
> > If the above all works this entire patchset should become really small.
>
> Thanks,
> Arnaud
>
> >
> >>
> >>>
> >>> I'd rather keep things simple for the refactoring and introduce new features
> >>> later if need be.
> >>
> >> Yes I agree with you, but here it could become a nightmare for the remote
> >> processor if the Linux endpoint address is not stable.
> >>
> >> Anyway we can consider this as a workaround waiting the extension of the NS
> >> announcement to have a better management of the address exchange on channel
> >> initialization.
> >>
> >> Thanks
> >> Arnaud
> >>
> >>>
> >>> As I said, it may be that I don't understand the usecase.
> >>>
> >>> Thanks,
> >>> Mathieu
> >>>
> >>>> +}
> >>>> +
> >>>> +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;
> >>>> @@ -429,16 +465,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, Jun 07, 2021 at 07:30:29PM +0200, Arnaud Pouliquen wrote:
> The rpmsg devices can be probed without default endpoint. This function
> provides the capability for rpmsg drivers to create a default endpoint
> on runtime.
>
> For example, a driver might want the rpmsg core dispatcher to drop its
> messages until it is ready to process them. In this case, the driver will
> create the default endpoint when the conditions are met to process the
> messages.
>
> Signed-off-by: Arnaud Pouliquen <[email protected]>
> ---
> drivers/rpmsg/rpmsg_core.c | 51 ++++++++++++++++++++++++++++++++++++++
> include/linux/rpmsg.h | 14 +++++++++++
> 2 files changed, 65 insertions(+)
>
> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
> index e5daee4f9373..07b680bda61f 100644
> --- a/drivers/rpmsg/rpmsg_core.c
> +++ b/drivers/rpmsg/rpmsg_core.c
> @@ -115,6 +115,57 @@ struct rpmsg_endpoint *rpmsg_create_ept(struct rpmsg_device *rpdev,
> }
> EXPORT_SYMBOL(rpmsg_create_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 the default endpoint if already created or creates
> + * a endpoint and assign it 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).
> + *
> + * Returns 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;
> +
> + /* 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_destroy_ept() - destroy an existing rpmsg endpoint
> * @ept: endpoing to destroy
> diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
> index d97dcd049f18..ab034061722c 100644
> --- a/include/linux/rpmsg.h
> +++ b/include/linux/rpmsg.h
> @@ -172,6 +172,9 @@ void rpmsg_destroy_ept(struct rpmsg_endpoint *);
> struct rpmsg_endpoint *rpmsg_create_ept(struct rpmsg_device *,
> rpmsg_rx_cb_t cb, void *priv,
> struct rpmsg_channel_info chinfo);
> +struct rpmsg_endpoint *rpmsg_create_default_ept(struct rpmsg_device *rpdev,
> + rpmsg_rx_cb_t cb, void *priv,
> + struct rpmsg_channel_info chinfo);
>
> int rpmsg_send(struct rpmsg_endpoint *ept, void *data, int len);
> int rpmsg_sendto(struct rpmsg_endpoint *ept, void *data, int len, u32 dst);
> @@ -234,6 +237,17 @@ static inline struct rpmsg_endpoint *rpmsg_create_ept(struct rpmsg_device *rpdev
> return ERR_PTR(-ENXIO);
> }
>
> +static inline struct rpmsg_endpoint *rpmsg_create_default_ept(struct rpmsg_device *rpdev,
> + rpmsg_rx_cb_t cb,
> + void *priv,
Please move this to the previous line to match the definition in the other arm
of the #if.
> + struct rpmsg_channel_info chinfo)
> +{
> + /* This shouldn't be possible */
> + WARN_ON(1);
> +
> + return NULL;
> +}
> +
> static inline int rpmsg_send(struct rpmsg_endpoint *ept, void *data, int len)
> {
> /* This shouldn't be possible */
> --
> 2.17.1
>
On 6/22/21 12:38 AM, Mathieu Poirier wrote:
> On Fri, Jun 18, 2021 at 01:35:43PM +0200, Arnaud POULIQUEN wrote:
>> Hi Mathieu,
>>
>> On 6/17/21 11:31 PM, Mathieu Poirier wrote:
>>> On Wed, Jun 16, 2021 at 02:38:26PM +0200, Arnaud POULIQUEN wrote:
>>>> Hi Mathieu,
>>>>
>>>> On 6/15/21 10:01 PM, Mathieu Poirier wrote:
>>>>> On Mon, Jun 07, 2021 at 07:30:31PM +0200, 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 | 54 ++++++++++++++++++++++++++++++++++++--
>>>>>> 1 file changed, 52 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
>>>>>> index 4199ac1bee10..3b850b218eb0 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;
>>>>>>
>>>>>> @@ -416,6 +418,40 @@ 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;
>>>>>> +
>>>>>> + memcpy(chinfo.name, RPMSG_CHAR_DEVNAME, sizeof(RPMSG_CHAR_DEVNAME));
>>>>>> + chinfo.src = rpdev->src;
>>>>>> + chinfo.dst = rpdev->dst;
>>>>>> +
>>>>>> + return __rpmsg_chrdev_eptdev_create(rpdev, &rpdev->dev, chinfo, true);
>>>>>
>>>>> I am a little puzzled here as to why we need different modes... Why can't we
>>>>> simply call rpmsg_chrdev_eptdev_create() and let the endpoint be created on
>>>>> open() and destroyed on release() as per the current implementation?
>>>>
>>>> The main reason is the support of the NS announcement
>>>> a NS announcement is received from the remote processor:
>>>> channel name: "rpmsg-raw"
>>>> remote address (dst address): 0x400
>>>> local address (scr address) : RPMSG_ADDR_ANY
>>>> => no default endpoint, and not local address.
>>>>
>>>> case 1) if we use legacy implementation ( no default endpoint)
>>>> => create/destroy endpoint on open/stop
>>>> - on first open: created endpoint is bound to scr address 0x406
>>>> - a first message is sent to the remote side, the address 0x406 is stored as
>>>> default channel dst address on remote side.
>>>> - on close: endpoint is closed and associated address 0x406 is free.
>>>> - another driver create an enpoint the address 0x406 is reserved for this new
>>>> endpoint.
>>>> - on new open: scr address is set to next value 0x407
>>>> => how to inform remote processor that the address has changed?
>>>> => no reservation mechanism that ensure that you can reuse the same address
>>>>
>>>> case 2) relying on use_default_ept
>>>> => Ensure that both side have always the same addresses to communicate.
>>>
>>> I see the problem and your solution is adequate - I think the code simply needs
>>> to be moved around a little. Here is what I suggest:
>>>
>>> 1) Create the endpoint in rpmsg_chrdev_probe(), just before calling
>>> rpmsg_chrdev_eptdev_create(). That way changes to rpmsg_eptdev_open() can be
>>> kept to a minimum. I don't think we'll be needing
>>> __rpmsg_chrdev_eptdev_create() anymore.
>>
>> Yes i could, but this will break a concept of the rpmsg_char that creates the
>> endpoint on open, meaning that application is ready to communicate.
>
> In my opinion creating and destorying an endpoint on open/close is something we
> want to move away from.
Not simple to answer... As discussed a mechanism is requested by some developer
to be able on a ns announcement to inform the remote side that the user
application or the client driver is ready to communicate, the endpoint creation
could be the trigger.
That said, let's go by steps. For this patchset I will try to come back to my
first implementation where i created the endpoint on probe.
>
>>
>> I would rather preserve this behavior.
>>
>>>
>>> 2) We can get rid of use_default_ept by taking advantage of the fact that the
>>> rpmsg_char driver does not use rpmsg_device::ept. If we create the endpoint in
>>> rpmsg_chrdev_probe() we know that if rpdev->ept exists, we must not create
>>> or destroy the endpoint in rpmsg_eptdev_open() and rpmsg_eptdev_release().
>>>
>>> 3) Function rpmsg_eptdev_open() doesn't change much. If rpdev->ept is NULL
>>> than
>>> an endpoint is created as the current implementation. Otherwise we simply do:
>>>
>>> eptdev->ept = rpdev->ept;
>>>
>>
>> In qcom_glink_create_chrdev, a rpmsg_ctrl rpdev with a default endpoint is
>> created and used as parameter of the pmsg_ctrldev_register_device [1]
>> => rpdev->ept is not NULL.
>>
>> So the rpmsg_char has to differentiate 2 cases on rpmsg_eptdev_open:
>> - A enpdoint has to be created as requested by RPMSG_CREATE_EPT_IOCTL
>> (regardless of the rpdev->ept value)
>> - for a rpmsg device created by an NS announcement: A default endpoint has to be
>> reused (or created if rpdev->ept is null).
>>
>> so the rpdev->ept test is not relevant for decision, the use_default_ept ( or
>> another flag) is mandatory.
>
> Yes, we need a flag. May I suggest "fixed_ept" rather than "used_default_ept"?
"fixed_ept" could be miss-understood . It can be interpreted as an endpoint with
a fixed address (not set to RPMSG_ADDR_ANY).
What about "default_ept" or "static_ept"?
Thanks
Arnaud
>
>>
>>
>>> 4) Make sure the teardown path works as well. From what I can see, it should.
>>>
>>> 5) Add a __lot__ of comments.
>>>
>>> If the above all works this entire patchset should become really small.
>>
>> Thanks,
>> Arnaud
>>
>>>
>>>>
>>>>>
>>>>> I'd rather keep things simple for the refactoring and introduce new features
>>>>> later if need be.
>>>>
>>>> Yes I agree with you, but here it could become a nightmare for the remote
>>>> processor if the Linux endpoint address is not stable.
>>>>
>>>> Anyway we can consider this as a workaround waiting the extension of the NS
>>>> announcement to have a better management of the address exchange on channel
>>>> initialization.
>>>>
>>>> Thanks
>>>> Arnaud
>>>>
>>>>>
>>>>> As I said, it may be that I don't understand the usecase.
>>>>>
>>>>> Thanks,
>>>>> Mathieu
>>>>>
>>>>>> +}
>>>>>> +
>>>>>> +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;
>>>>>> @@ -429,16 +465,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 Tue, Jun 22, 2021 at 10:21:02AM +0200, Arnaud POULIQUEN wrote:
>
>
> On 6/22/21 12:38 AM, Mathieu Poirier wrote:
> > On Fri, Jun 18, 2021 at 01:35:43PM +0200, Arnaud POULIQUEN wrote:
> >> Hi Mathieu,
> >>
> >> On 6/17/21 11:31 PM, Mathieu Poirier wrote:
> >>> On Wed, Jun 16, 2021 at 02:38:26PM +0200, Arnaud POULIQUEN wrote:
> >>>> Hi Mathieu,
> >>>>
> >>>> On 6/15/21 10:01 PM, Mathieu Poirier wrote:
> >>>>> On Mon, Jun 07, 2021 at 07:30:31PM +0200, 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 | 54 ++++++++++++++++++++++++++++++++++++--
> >>>>>> 1 file changed, 52 insertions(+), 2 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> >>>>>> index 4199ac1bee10..3b850b218eb0 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;
> >>>>>>
> >>>>>> @@ -416,6 +418,40 @@ 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;
> >>>>>> +
> >>>>>> + memcpy(chinfo.name, RPMSG_CHAR_DEVNAME, sizeof(RPMSG_CHAR_DEVNAME));
> >>>>>> + chinfo.src = rpdev->src;
> >>>>>> + chinfo.dst = rpdev->dst;
> >>>>>> +
> >>>>>> + return __rpmsg_chrdev_eptdev_create(rpdev, &rpdev->dev, chinfo, true);
> >>>>>
> >>>>> I am a little puzzled here as to why we need different modes... Why can't we
> >>>>> simply call rpmsg_chrdev_eptdev_create() and let the endpoint be created on
> >>>>> open() and destroyed on release() as per the current implementation?
> >>>>
> >>>> The main reason is the support of the NS announcement
> >>>> a NS announcement is received from the remote processor:
> >>>> channel name: "rpmsg-raw"
> >>>> remote address (dst address): 0x400
> >>>> local address (scr address) : RPMSG_ADDR_ANY
> >>>> => no default endpoint, and not local address.
> >>>>
> >>>> case 1) if we use legacy implementation ( no default endpoint)
> >>>> => create/destroy endpoint on open/stop
> >>>> - on first open: created endpoint is bound to scr address 0x406
> >>>> - a first message is sent to the remote side, the address 0x406 is stored as
> >>>> default channel dst address on remote side.
> >>>> - on close: endpoint is closed and associated address 0x406 is free.
> >>>> - another driver create an enpoint the address 0x406 is reserved for this new
> >>>> endpoint.
> >>>> - on new open: scr address is set to next value 0x407
> >>>> => how to inform remote processor that the address has changed?
> >>>> => no reservation mechanism that ensure that you can reuse the same address
> >>>>
> >>>> case 2) relying on use_default_ept
> >>>> => Ensure that both side have always the same addresses to communicate.
> >>>
> >>> I see the problem and your solution is adequate - I think the code simply needs
> >>> to be moved around a little. Here is what I suggest:
> >>>
> >>> 1) Create the endpoint in rpmsg_chrdev_probe(), just before calling
> >>> rpmsg_chrdev_eptdev_create(). That way changes to rpmsg_eptdev_open() can be
> >>> kept to a minimum. I don't think we'll be needing
> >>> __rpmsg_chrdev_eptdev_create() anymore.
> >>
> >> Yes i could, but this will break a concept of the rpmsg_char that creates the
> >> endpoint on open, meaning that application is ready to communicate.
> >
> > In my opinion creating and destorying an endpoint on open/close is something we
> > want to move away from.
>
> Not simple to answer... As discussed a mechanism is requested by some developer
> to be able on a ns announcement to inform the remote side that the user
> application or the client driver is ready to communicate, the endpoint creation
> could be the trigger.
>
> That said, let's go by steps. For this patchset I will try to come back to my
> first implementation where i created the endpoint on probe.
Going back to that conversation I realise the directions I gave out on that
front was not optimal. As you mention above please go back to creating the
endpoint on probe().
>
> >
> >>
> >> I would rather preserve this behavior.
> >>
> >>>
> >>> 2) We can get rid of use_default_ept by taking advantage of the fact that the
> >>> rpmsg_char driver does not use rpmsg_device::ept. If we create the endpoint in
> >>> rpmsg_chrdev_probe() we know that if rpdev->ept exists, we must not create
> >>> or destroy the endpoint in rpmsg_eptdev_open() and rpmsg_eptdev_release().
> >>>
> >>> 3) Function rpmsg_eptdev_open() doesn't change much. If rpdev->ept is NULL
> >>> than
> >>> an endpoint is created as the current implementation. Otherwise we simply do:
> >>>
> >>> eptdev->ept = rpdev->ept;
> >>>
> >>
> >> In qcom_glink_create_chrdev, a rpmsg_ctrl rpdev with a default endpoint is
> >> created and used as parameter of the pmsg_ctrldev_register_device [1]
> >> => rpdev->ept is not NULL.
> >>
> >> So the rpmsg_char has to differentiate 2 cases on rpmsg_eptdev_open:
> >> - A enpdoint has to be created as requested by RPMSG_CREATE_EPT_IOCTL
> >> (regardless of the rpdev->ept value)
> >> - for a rpmsg device created by an NS announcement: A default endpoint has to be
> >> reused (or created if rpdev->ept is null).
> >>
> >> so the rpdev->ept test is not relevant for decision, the use_default_ept ( or
> >> another flag) is mandatory.
> >
> > Yes, we need a flag. May I suggest "fixed_ept" rather than "used_default_ept"?
>
> "fixed_ept" could be miss-understood . It can be interpreted as an endpoint with
> a fixed address (not set to RPMSG_ADDR_ANY).
> What about "default_ept" or "static_ept"?
I'll take static_ept.
>
> Thanks
> Arnaud
>
> >
> >>
> >>
> >>> 4) Make sure the teardown path works as well. From what I can see, it should.
> >>>
> >>> 5) Add a __lot__ of comments.
> >>>
> >>> If the above all works this entire patchset should become really small.
> >>
> >> Thanks,
> >> Arnaud
> >>
> >>>
> >>>>
> >>>>>
> >>>>> I'd rather keep things simple for the refactoring and introduce new features
> >>>>> later if need be.
> >>>>
> >>>> Yes I agree with you, but here it could become a nightmare for the remote
> >>>> processor if the Linux endpoint address is not stable.
> >>>>
> >>>> Anyway we can consider this as a workaround waiting the extension of the NS
> >>>> announcement to have a better management of the address exchange on channel
> >>>> initialization.
> >>>>
> >>>> Thanks
> >>>> Arnaud
> >>>>
> >>>>>
> >>>>> As I said, it may be that I don't understand the usecase.
> >>>>>
> >>>>> Thanks,
> >>>>> Mathieu
> >>>>>
> >>>>>> +}
> >>>>>> +
> >>>>>> +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;
> >>>>>> @@ -429,16 +465,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
> >>>>>>