Re: [PATCH] rpmsg: virtio: set dst address on first message received

[Arnaud POULIQUEN]

Hello Mathieu,

On 5/6/22 19:12, Mathieu Poirier wrote:
> On Tue, Mar 15, 2022 at 04:38:56PM +0100, Arnaud Pouliquen wrote:
>> when a rpmsg channel has been locally created with a destination address
> 
> s/when/Wen
> 
> Also, please be more specific about the "locally created" part, i.e
> rpmsg_ctrldev_ioctl() -> rpmsg_create_channel().  Otherwise it is really hard to
> understand the context of this change.
> 
>> set to RPMSG_ADDR_ANY, a name service announcement message is sent to
>> the remote side. Then the destination address is never updated, making it
>> impossible to send messages to the remote.
>>
>> An example of kernel trace observed:
>> rpmsg_tty virtio0.rpmsg-tty.29.-1: invalid addr (src 0x1d, dst 0xffffffff)
>>
>> Implement same strategy than the open-amp library:
>> On the reception of the first message, if the destination address is
>> RPMSG_ADDR_ANY, then set it to address of the remote endpoint that
>> send the message.
>>
> 
> I would have expected a "Fixes:" tag.

Difficult to give a reference. For me the issue exists since the creation
of the rpmsg virtio bus. A driver can create a channel that generates a NS
announcement leading to this issue.
The issue as been highlighted by the creation of the  RPMSG_CREATE_DEV_IOCTL
control.

> 
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@xxxxxxxxxxx>
>>
>> ---
>> Remark:
>> An alternative (or a complement?) could be to add a NS bind/unbind in
>> the NS announcement channel (in rpmsg_ns.c).
>> This would allow the local and/or the remote processor to inform the
>> remote side the the service announced in bound.
>> ---
>>  drivers/rpmsg/virtio_rpmsg_bus.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
>> index 3ede25b1f2e4..99d2119cc164 100644
>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>> @@ -708,6 +708,7 @@ static ssize_t virtio_rpmsg_get_mtu(struct rpmsg_endpoint *ept)
>>  static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
>>  			     struct rpmsg_hdr *msg, unsigned int len)
>>  {
>> +	struct rpmsg_device *rpdev;
>>  	struct rpmsg_endpoint *ept;
>>  	struct scatterlist sg;
>>  	bool little_endian = virtio_is_little_endian(vrp->vdev);
>> @@ -746,6 +747,15 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
>>  	mutex_unlock(&vrp->endpoints_lock);
>>  
>>  	if (ept) {
>> +		rpdev = ept->rpdev;
>> +		if (rpdev->ept == ept && rpdev->dst == RPMSG_ADDR_ANY) {
> 
> Please add a comment to explain the first part of the if() clause.  It took me
> quite some time to understand. 
> 
>> +			/*
>> +			 * First message received from the remote side on the default endpoint,
>> +			 * update channel destination address.
>> +			 */
>> +			rpdev->dst = msg->src;
> 
> This triggers a bot warning and should be addressed.  If it can't be addressed add
> a comment that clearly explains why so that we don't end up receiving patches
> for it every 4 weeks.

Oops, I missed it, thanks for pointing it out.


Concerning the patch itself as discussed in RP open-amp meeting. I wonder if this issue
could be addressed by the flow control[1][2][3], or if needed in any case.

I propose to send a V2 when ready to propose in parallel the flow control.
So both can be addressed at same time to have a global picture of the way to address the
use case..

Thanks,
Arnaud

[1] POC Linux code:
https://github.com/arnopo/linux/commits/signalling

[2] openamp library associated code:
https://github.com/arnopo/open-amp/commits/flow_ctrl

[3] overview presentation
https://drive.google.com/file/d/1CLU3ybI3oSBGvor18AQ-HOzOJ2nOppEb/view


> 
> Thanks,
> Mathieu
> 
>> +		}
>> +
>>  		/* make sure ept->cb doesn't go away while we use it */
>>  		mutex_lock(&ept->cb_lock);
>>  
>> -- 
>> 2.25.1
>>