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
>>
