When a channel is created by user space application with the
RPMSG_CREATE_DEV_IOCTL controls, a ns announcement has to be sent
(depending on backend) to inform the remote side that a new service
is available.
Signed-off-by: Arnaud Pouliquen <[email protected]>
---
drivers/rpmsg/rpmsg_core.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
index bdcde57c22f6..63227279397d 100644
--- a/drivers/rpmsg/rpmsg_core.c
+++ b/drivers/rpmsg/rpmsg_core.c
@@ -145,6 +145,9 @@ EXPORT_SYMBOL(rpmsg_destroy_ept);
*
* This function returns a pointer to an endpoint created and assigned as the default
* endpoint of the rpmsg device.
+ * If we need to, we also announce about this channel to the remote
+ * processor. This announcement is needed in case the driver is exposing an rpmsg service that has
+ * been created locally.
*
* Drivers should provide their @rpdev channel (so the new endpoint would belong
* to the same remote processor their channel belongs to), an rx callback
@@ -161,6 +164,7 @@ struct rpmsg_endpoint *rpmsg_create_default_ept(struct rpmsg_device *rpdev,
struct rpmsg_channel_info chinfo)
{
struct rpmsg_endpoint *ept;
+ int err = 0;
if (WARN_ON(!rpdev))
return NULL;
@@ -183,6 +187,16 @@ struct rpmsg_endpoint *rpmsg_create_default_ept(struct rpmsg_device *rpdev,
rpdev->ept = ept;
rpdev->src = ept->addr;
+ if (rpdev->ops->announce_create)
+ err = rpdev->ops->announce_create(rpdev);
+ if (err) {
+ rpmsg_destroy_ept(ept);
+ rpdev->ept = NULL;
+ rpdev->src = RPMSG_ADDR_ANY;
+
+ return NULL;
+ }
+
return ept;
}
EXPORT_SYMBOL(rpmsg_create_default_ept);
--
2.17.1
On Mon 08 Nov 08:19 CST 2021, Arnaud Pouliquen wrote:
> When a channel is created by user space application with the
> RPMSG_CREATE_DEV_IOCTL controls, a ns announcement has to be sent
> (depending on backend) to inform the remote side that a new service
> is available.
>
> Signed-off-by: Arnaud Pouliquen <[email protected]>
> ---
> drivers/rpmsg/rpmsg_core.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
> index bdcde57c22f6..63227279397d 100644
> --- a/drivers/rpmsg/rpmsg_core.c
> +++ b/drivers/rpmsg/rpmsg_core.c
> @@ -145,6 +145,9 @@ EXPORT_SYMBOL(rpmsg_destroy_ept);
> *
> * This function returns a pointer to an endpoint created and assigned as the default
> * endpoint of the rpmsg device.
> + * If we need to, we also announce about this channel to the remote
> + * processor. This announcement is needed in case the driver is exposing an rpmsg service that has
> + * been created locally.
> *
> * Drivers should provide their @rpdev channel (so the new endpoint would belong
> * to the same remote processor their channel belongs to), an rx callback
> @@ -161,6 +164,7 @@ struct rpmsg_endpoint *rpmsg_create_default_ept(struct rpmsg_device *rpdev,
> struct rpmsg_channel_info chinfo)
> {
> struct rpmsg_endpoint *ept;
> + int err = 0;
>
> if (WARN_ON(!rpdev))
> return NULL;
> @@ -183,6 +187,16 @@ struct rpmsg_endpoint *rpmsg_create_default_ept(struct rpmsg_device *rpdev,
> rpdev->ept = ept;
> rpdev->src = ept->addr;
>
> + if (rpdev->ops->announce_create)
> + err = rpdev->ops->announce_create(rpdev);
> + if (err) {
> + rpmsg_destroy_ept(ept);
> + rpdev->ept = NULL;
> + rpdev->src = RPMSG_ADDR_ANY;
> +
> + return NULL;
> + }
> +
Unless I'm missing something I think this would be cleaner as:
if (rpdev->ops->announce_create) {
err = rpdev->ops->announce_create(rpdev);
if (err) {
...;
}
}
which also implies that you don't need to zero-initialize err.
Other than that, this looks good and follows what would happen in
rpmsg_dev_probe()...
PS. In rpmsg_dev_probe(), if rpdrv->probe() succeeds but announce_create
returns a failure we will exit the function with an error, which will
just fail really_probe() and we won't ever clean up the device (i.e.
call rpdev->remove()).
Regards,
Bjorn
> return ept;
> }
> EXPORT_SYMBOL(rpmsg_create_default_ept);
> --
> 2.17.1
>
On 12/3/21 2:58 AM, Bjorn Andersson wrote:
> On Mon 08 Nov 08:19 CST 2021, Arnaud Pouliquen wrote:
>
>> When a channel is created by user space application with the
>> RPMSG_CREATE_DEV_IOCTL controls, a ns announcement has to be sent
>> (depending on backend) to inform the remote side that a new service
>> is available.
>>
>> Signed-off-by: Arnaud Pouliquen <[email protected]>
>> ---
>> drivers/rpmsg/rpmsg_core.c | 14 ++++++++++++++
>> 1 file changed, 14 insertions(+)
>>
>> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
>> index bdcde57c22f6..63227279397d 100644
>> --- a/drivers/rpmsg/rpmsg_core.c
>> +++ b/drivers/rpmsg/rpmsg_core.c
>> @@ -145,6 +145,9 @@ EXPORT_SYMBOL(rpmsg_destroy_ept);
>> *
>> * This function returns a pointer to an endpoint created and assigned as the default
>> * endpoint of the rpmsg device.
>> + * If we need to, we also announce about this channel to the remote
>> + * processor. This announcement is needed in case the driver is exposing an rpmsg service that has
>> + * been created locally.
>> *
>> * Drivers should provide their @rpdev channel (so the new endpoint would belong
>> * to the same remote processor their channel belongs to), an rx callback
>> @@ -161,6 +164,7 @@ struct rpmsg_endpoint *rpmsg_create_default_ept(struct rpmsg_device *rpdev,
>> struct rpmsg_channel_info chinfo)
>> {
>> struct rpmsg_endpoint *ept;
>> + int err = 0;
>>
>> if (WARN_ON(!rpdev))
>> return NULL;
>> @@ -183,6 +187,16 @@ struct rpmsg_endpoint *rpmsg_create_default_ept(struct rpmsg_device *rpdev,
>> rpdev->ept = ept;
>> rpdev->src = ept->addr;
>>
>> + if (rpdev->ops->announce_create)
>> + err = rpdev->ops->announce_create(rpdev);
>> + if (err) {
>> + rpmsg_destroy_ept(ept);
>> + rpdev->ept = NULL;
>> + rpdev->src = RPMSG_ADDR_ANY;
>> +
>> + return NULL;
>> + }
>> +
>
> Unless I'm missing something I think this would be cleaner as:
>
> if (rpdev->ops->announce_create) {
> err = rpdev->ops->announce_create(rpdev);
> if (err) {
> ...;
> }
> }
>
> which also implies that you don't need to zero-initialize err.
Ack, i will change this.
>
> Other than that, this looks good and follows what would happen in
> rpmsg_dev_probe()...
>
>
> PS. In rpmsg_dev_probe(), if rpdrv->probe() succeeds but announce_create
> returns a failure we will exit the function with an error, which will
> just fail really_probe() and we won't ever clean up the device (i.e.
> call rpdev->remove()).
Right the error management in rpmsg_dev_probe needs to be improved.
I will probably found time to address this when preparing the next revision
of my patchset (just need before a clarification from you about patch
v7 10/12] rpmsg: char: Introduce the "rpmsg-raw" channel).
Thanks,
Arnaud
>
> Regards,
> Bjorn
>
>> return ept;
>> }
>> EXPORT_SYMBOL(rpmsg_create_default_ept);
>> --
>> 2.17.1
>>