Subject: Re: [PATCH v7 12/12] rpmsg: core: send a ns announcement when a
 default endpoint is created
To:     Bjorn Andersson <bjorn.andersson@linaro.org>
CC:     Ohad Ben-Cohen <ohad@wizery.com>,
        Mathieu Poirier <mathieu.poirier@linaro.org>,
        <linux-remoteproc@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <linux-stm32@st-md-mailman.stormreply.com>, <julien.massot@iot.bzh>
References: <20211108141937.13016-1-arnaud.pouliquen@foss.st.com>
 <20211108141937.13016-13-arnaud.pouliquen@foss.st.com>
 <Yal5tplvcqDjEeTs@builder.lan>
From:   Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>
Message-ID: <eb3cc7a1-251e-7d40-225a-a5528a22323e@foss.st.com>
Date:   Fri, 3 Dec 2021 17:56:48 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.14.0
MIME-Version: 1.0
In-Reply-To: <Yal5tplvcqDjEeTs@builder.lan>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


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@foss.st.com>
>> ---
>>  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
>>