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 <arnaud.pouliquen@xxxxxxxxxxx>
>> ---
>> 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
>>
