Support the two phase creation handshake

[xiaoxiang781216]

Guard by a new feature bit to keep the backward compatiblity, @wjliang please review.

[arnopo]

I'm not a fan of this patch. The "NS service ack" has to be service dependent, not rdev dependent. The expected behavior depends on the implementation of the service itself.
From my POV, this has to be managed by the application not at the RPMsg level.

[xiaoxiang781216]

I'm not a fan of this patch. The "NS service ack" has to be service dependent, not rdev dependent. The expected behavior depends on the implementation of the service itself.

@arnop2 I can't understand why the acknowledge is service dependent? could you give some example the ack may generate some bad side efffect?
Without this ack, client doesn't have any method to get the server port, then:
1.The first message must send from server or
2.The server has to send a dummy message

From my POV, this has to be managed by the application not at the RPMsg level.

As I learn from IPC mechanism(e.g. pipe, unix socket, udp, tcp and rpc), once all necessary information pass to the library, either side(client or server) could send the message first.
Actually, many service has a infinite and passive loop:
1.receive client request, process it and send response
2.but never send ipc without receive something first
It is natural that client send the first message once the channel/socket setup, but it's very strange that rpmsg can't do this.

[wjliang]

@arnop2 I can't understand why the acknowledge is service dependent? could you give some example the ack may generate some bad side efffect?
Without this ack, client doesn't have any method to get the server port, then:
1.The first message must send from server or
2.The server has to send a dummy message

The name "client" and "server" is confusing. i think one side has "service name" is because this site is to provide service, and it doesn't need to send the first message. instead, the side which wants service will send out the name resource announcement requests to enquire the available services.
And thus, if just use the name service to know each sides' address, i think both sides can raise name service announcement requests.
The issue you are describing sounds tight to Linux kernel implementation to me, as Linux side doesn't service name service announcement request at all.

[arnopo]

I'm not a fan of this patch. The "NS service ack" has to be service dependent, not rdev dependent. The expected behavior depends on the implementation of the service itself.

@arnop2 I can't understand why the acknowledge is service dependent? could you give some example the ack may generate some bad side effect?
One side effect with your patch is in rpmsg_create_ept ( but this probably can be fix).
=>The following condition can be true for an endpoint that provides as parameter the remote destination address and doesn't use NS announcement, while another service does.
if (rdev->support_ack && ept->dest_addr != RPMSG_ADDR_ANY)

Another side effect is that this implementation adds a constraint on service that must be ready for reception as soon as the endpoint is created.
On ST platform we also added this kind of patch in Linux rpmsg, before removing it, as we consider (as explained Wendy) that we manage a "service".

Last point is that this can generates compatibility issue with some other rpmsg implementation like Linux or rpmsg-lite.
Now, to be clear, i'm not against this extension, just not fan. Like the other protocol extension(#155), i would suggest to push the extension in Linux side also. I think it is important to have adhesion of the different maintainers to keep an homogeneity in the rpmsg protocol.

Without this ack, client doesn't have any method to get the server port, then:
1.The first message must send from server or
2.The server has to send a dummy message

From my POV, this has to be managed by the application not at the RPMsg level.

As I learn from IPC mechanism(e.g. pipe, unix socket, udp, tcp and rpc), once all necessary information pass to the library, either side(client or server) could send the message first.
Actually, many service has a infinite and passive loop:
1.receive client request, process it and send response
2.but never send ipc without receive something first
It is natural that client send the first message once the channel/socket setup, but it's very strange that rpmsg can't do this.

[xiaoxiang781216]

@arnop2 I can't understand why the acknowledge is service dependent? could you give some example the ack may generate some bad side efffect?
Without this ack, client doesn't have any method to get the server port, then:
1.The first message must send from server or
2.The server has to send a dummy message

The name "client" and "server" is confusing.

Yes, let me clarify a little bit:
1.Client initiatively create the named channel and then trigger RPMSG_NS_CREATE.
2.Server passively create the same name channel in rpmsg_device->ns_bind_cb.
Server pass the received dest address to rpmsg_create_ept, so rpmsg_create_ept doesn't trigger any ns message before my patch.

i think one side has "service name" is because this site is to provide service, and it doesn't need to send the first message. instead, the side which wants service will send out the name resource announcement requests to enquire the available services.
And thus, if just use the name service to know each sides' address, i think both sides can raise name service announcement requests.

Yes, this is one approach to fix the problem: both side call rpmsg_create_ept with dest == RPMSG_ADDR_ANY and then trigger RPMSG_NS_CREATE twice to exchange the port information.
But, this approach can't handle the complex usage very well. For example, in my SoC:
1.Each MCU(total three) run a independent RTOS(NuttX here)
2.Main MCU could talk with other two satellite MCU to form a star network
3.Main MCU will provide some common service(e.g. log redirection)
In this case, the rpmsg log service on main MCU shouldn't create the channel proactively for each satellite MCU blindly, since the satellite MCU may temporarily switch to UART for debuging early crash issue.
Actually, I think both programming model is useful:
1.Peer<->Peer: both side create the channel proactively
2.Client<->Server: client create the channel proactively, server create the channel passively
The benefit for the second is:
1.It is similar to TCP/IP programming model and reduce the learning curve for the network guy.
2.It handle one server provide the service to multiple clients naturally.
this patch keep peer<->peer work as before, but make client<->server possible now.

The issue you are describing sounds tight to Linux kernel implementation to me, as Linux side doesn't service name service announcement request at all.

Yes, Linux kernel never create the name channel proactively. but as describe before, client<->server model is also useful for OpenAMP.

[xiaoxiang781216]

I'm not a fan of this patch. The "NS service ack" has to be service dependent, not rdev dependent. The expected behavior depends on the implementation of the service itself.

@arnop2 I can't understand why the acknowledge is service dependent? could you give some example the ack may generate some bad side effect?
One side effect with your patch is in rpmsg_create_ept ( but this probably can be fix).
=>The following condition can be true for an endpoint that provides as parameter the remote destination address and doesn't use NS announcement, while another service does.
if (rdev->support_ack && ept->dest_addr != RPMSG_ADDR_ANY)

Yes, It's a problem. After some thought, we should guard ALL NS annoucement(both RPMSG_NS_CREATE and RPMSG_NS_CREATE_ACK) when name equal NULL. So the caller could bypass all NS by the empty channel name.

Another side effect is that this implementation adds a constraint on service that must be ready for reception as soon as the endpoint is created.
On ST platform we also added this kind of patch in Linux rpmsg, before removing it, as we consider (as explained Wendy) that we manage a "service".

This patch just make the client could send the first message possibly, If the server in upper layer need take more time to do the preparation, it could still enforce the client don't send any message before the server send some special message first.

From my point of view, both side should be enabled to send the first message as needed once the channel setup. It's the responsibility and rights for the application protocol desginer to define the initial sequence(who send the first message), but not the transport layer enforce that only server can send the first message.

Last point is that this can generates compatibility issue with some other rpmsg implementation like Linux or rpmsg-lite.

Yes, the new RPMSG_NS_CREATE_ACK may confuse old implementation. That's why I add a feature bit VIRTIO_RPMSG_F_ACK to guard the new behaviour.

Now, to be clear, i'm not against this extension, just not fan. Like the other protocol extension(#155), i would suggest to push the extension in Linux side also. I think it is important to have adhesion of the different maintainers to keep an homogeneity in the rpmsg protocol.

Yes, I will send the similar patch to kernel side once the patch pass OpenAMP community review.

Without this ack, client doesn't have any method to get the server port, then:
1.The first message must send from server or
2.The server has to send a dummy message

From my POV, this has to be managed by the application not at the RPMsg level.

As I learn from IPC mechanism(e.g. pipe, unix socket, udp, tcp and rpc), once all necessary information pass to the library, either side(client or server) could send the message first.
Actually, many service has a infinite and passive loop:
1.receive client request, process it and send response
2.but never send ipc without receive something first
It is natural that client send the first message once the channel/socket setup, but it's very strange that rpmsg can't do this.

[xiaoxiang781216]

@arnop2 the third patch resolve your issue about the side effect of rpmsg_create_ept, please take a look.

[arnopo]

@arnop2 the third patch resolve your issue about the side effect of rpmsg_create_ept, please take a look.

@xiaoxiang-xiaomi: i think this patch break the rpmsg protocol, a rpmsg channel is a service name + one or several associated endpoints.
When you create an endpoint you have to provide a service name. 2 strategies:

• dest_addr == RPMSG_ADDR_ANY: you don't know the remote address that support this service. You have wait answer from remote side to get the dest_addr.
• dest_addr != RPMSG_ADDR_ANY: you want to address a specific endpoint, and you know that this endpoint support the service. This is used mainly when the NS_annoucement is disable, but can be used for any other reason even if NS announcement is enabled.

[wjliang]

Yes, It's a problem. After some thought, we should guard ALL NS annoucement(both RPMSG_NS_CREATE and RPMSG_NS_CREATE_ACK) when name equal NULL. So the caller could bypass all NS by the empty channel name.

This sounds like another version of RPMsg, instead of use NS_ACK feature bit, better to make it RPMsg v2 feature bit. Go back to the issue this patch is trying to solve.
It tries to solve two issues:

• Checking if NS is supported before sending NS announcement, this is better to be a separate patch.
• "RPMSG_NS_CREATE_ACK", I still doubt at if this will solve peer-to-peer issue. Because, in order to send messages, it needs to know the endpoint address of the other side. If this is the case, I can not clear on why sending NS announcement from both sides cannot solve the issue. From what you described, it looks like you want to allow sending messages rather than NS announcement message to dest_addr==ANY, which is not the current RPMsg expecting to work, this will be another version of RPMsg. I will need some time to think about about it before further comment.

[xiaoxiang781216]

@arnop2 the third patch resolve your issue about the side effect of rpmsg_create_ept, please take a look.

@xiaoxiang-xiaomi: i think this patch break the rpmsg protocol, a rpmsg channel is a service name + one or several associated endpoints.

rpmsg protocol = main endpoint + aided endpoints
But, only main endpoint need the service name and then utilize NS service to exchange the port information, all other endpoints should learn the peer port from the main endpoint message, so the service name is only meaningful for the main endpoint. kernel's rpmsg_create_ept don't use the name in the implementation too.

That's why I add a new API rpmsg_create_anon_ept, to help the user:
1.Create the main endpoint by rpmsg_create_ept with name
2.Create the aided endpoints by rpmsg_create_anon_ept
So both case could support very well, no break here.

When you create an endpoint you have to provide a service name. 2 strategies:

For main endpoint, we need provide the service name, but we don't need provide the name for the additional endpoints since NS service shouldn't activate in this case like Linux kernel.
If we activate NS service for the aided endpoints, Linux kernel will create new rpmsg_device to match these new endpoints, but what we want in kernel is the new rpmsg_endpoint.

• dest_addr == RPMSG_ADDR_ANY: you don't know the remote address that support this service. You have wait answer from remote side to get the dest_addr.
• dest_addr != RPMSG_ADDR_ANY: you want to address a specific endpoint, and you know that this endpoint support the service. This is used mainly when the NS_annoucement is disable, but can be used for any other reason even if NS announcement is enabled.

Another case is in rpmsg_device->ns_bind_cb, we receive the peer port and then supply to rpmsg_create_ept. What I enhance here is to send RPMSG_NS_CREATE_ACK to let the peer know our port information.

[xiaoxiang781216]

Yes, It's a problem. After some thought, we should guard ALL NS annoucement(both RPMSG_NS_CREATE and RPMSG_NS_CREATE_ACK) when name equal NULL. So the caller could bypass all NS by the empty channel name.

This sounds like another version of RPMsg, instead of use NS_ACK feature bit, better to make it RPMsg v2 feature bit. Go back to the issue this patch is trying to solve.
It tries to solve two issues:

• Checking if NS is supported before sending NS announcement, this is better to be a separate patch.

Ok, I will split the pull request to several small one.

• "RPMSG_NS_CREATE_ACK", I still doubt at if this will solve peer-to-peer issue. Because, in order to send messages, it needs to know the endpoint address of the other side. If this is the case, I can not clear on why sending NS announcement from both sides cannot solve the issue.

To learn the peer port, two approach could use:
1.Both side call rpmsg_create_ept with dest_addr == RPMSG_ADDR_ANY and exchange the port information through RPMSG_NS_CREATE message.
2.a.One side(client) call rpmsg_create_ept with dest_addr == RPMSG_ADDR_ANY
b.Another side(server) call rpmsg_create_ept with dest_addr != RPMSG_ADDR_ANY in rpmsg_device->ns_bind_cb
c.Trigger RPMSG_NS_CREATE_ACK to send server port to client
After more thought, yes, you are right. We can use the first approach to achieve the same result as the second.

From what you described, it looks like you want to allow sending messages rather than NS announcement message to dest_addr==ANY, which is not the current RPMsg expecting to work, this will be another version of RPMsg. I will need some time to think about about it before further comment.

No, I don't want to send the normal message before dest_addr != ANY, I just want to exchange the port information automatically.
As previous note, OpenAMP can acheive the goal with RPMSG_NS_CREATE, so the problem is how to resolve the issue from Linux kernel:
1.Reuse RPMSG_NS_CREATE, send this message to OpenAMP after rpmsg_device bind with rpmsg_driver
2.Or send RPMSG_NS_CREATE_ACK instead to explicitly notify OpenAMP this is response of the creatation.
I think RPMSG_NS_CREATE_ACK make it's more clear. How about your thinking?

[wjliang]

I think RPMSG_NS_CREATE_ACK make it's more clear. How about your thinking?

@arnop2 has use cases that there are multiple service endpoints(that is different endpoint addresses) of the same service name. Option 1, Reuse RPMSG_NS_CREATE will not solve this case.
It looks like your RPMSG_NS_CREATE_ACK is better to solve this issue. But please also send patches to the kernel side to have wider discussion.

[xiaoxiang781216]

I think RPMSG_NS_CREATE_ACK make it's more clear. How about your thinking?

@arnop2 has use cases that there are multiple service endpoints(that is different endpoint addresses) of the same service name. Option 1, Reuse RPMSG_NS_CREATE will not solve this case.

@arnop2, could you explain how the second endpoint create?

It looks like your RPMSG_NS_CREATE_ACK is better to solve this issue. But please also send patches to the kernel side to have wider discussion.

Sure, I will send the patch to kernel by end of the week.

[arnopo]

@xiaoxiang-xiaomi: i think this patch break the rpmsg protocol, a rpmsg channel is a service name + one or several associated endpoints.

rpmsg protocol = main endpoint + aided endpoints
But, only main endpoint need the service name and then utilize NS service to exchange the port information, all other endpoints should learn the peer port from the main endpoint message, so the service name is only meaningful for the main endpoint. kernel's rpmsg_create_ept don't use the name in the implementation too.

This partially true for Linux, as there is a channel notion with a default endpoint and a service. you can create endpoints on top of this channel. But in this case an endpoint is still linked to service (attached to the channel).
here is the definition of a channel in Linux kernel (https://elixir.bootlin.com/linux/latest/source/Documentation/rpmsg.txt): Channels are identified by a textual name and have a local ("source") rpmsg address, and remote ("destination") rpmsg
address.
If you have a look to rpmsg_core the name is used for the associated device name and attributes.

In OpenAMP, the channel concept does no more exist.

• you create a first endpoint (A) that generates an NS announcement,
• then 2 Alternatives to create a second endpoint (B):

1. you create a new endpoint with the same seervice name, and providing a destination addr. For instance to communicate to the same remote ept (A)
2. you create a second instance of the service by sending a new NS announcement with the same NS name and (dest_add == RPMSG_ADDR_ANY). wait for another destination address . This one
   But creating an endpoint without service associated is for me not correct, even if NS announcement is disabled

That's why I add a new API rpmsg_create_anon_ept, to help the user:
1.Create the main endpoint by rpmsg_create_ept with name
2.Create the aided endpoints by rpmsg_create_anon_ept
So both case could support very well, no break here.
It probably works, but null service name not clean from my pov as your implementation imposes it.

When you create an endpoint you have to provide a service name. 2 strategies:

For main endpoint, we need provide the service name, but we don't need provide the name for the additional endpoints since NS service shouldn't activate in this case like Linux kernel.
If we activate NS service for the aided endpoints, Linux kernel will create new rpmsg_device to match these new endpoints, but what we want in kernel is the new rpmsg_endpoint.

• dest_addr == RPMSG_ADDR_ANY: you don't know the remote address that support this service. You have wait answer from remote side to get the dest_addr.
• dest_addr != RPMSG_ADDR_ANY: you want to address a specific endpoint, and you know that this endpoint support the service. This is used mainly when the NS_annoucement is disable, but can be used for any other reason even if NS announcement is enabled.

Another case is in rpmsg_device->ns_bind_cb, we receive the peer port and then supply to rpmsg_create_ept. What I enhance here is to send RPMSG_NS_CREATE_ACK to let the peer know our port information.
Yes i understood the patch in rpmsg_create_ept in this way. you need it in rpmsg_create_ept because it could asynchronously created.

[arnopo]

I think RPMSG_NS_CREATE_ACK make it's more clear. How about your thinking?

@arnop2 has use cases that there are multiple service endpoints(that is different endpoint addresses) of the same service name. Option 1, Reuse RPMSG_NS_CREATE will not solve this case.

@arnop2, could you explain how the second endpoint create?
As explains in my previous comment, several endpoints can be created with same service name and unknown destination address. We use it in ST to create virtual TTY over rpmsg (with a rpmsg tty driver on Linux side).This creates a /dev/ttyrpmsg0 and a/dev/ttyrpmsg0 : One to control a co-processor one for co-processor traces.

It looks like your RPMSG_NS_CREATE_ACK is better to solve this issue. But please also send patches to the kernel side to have wider discussion.

Sure, I will send the patch to kernel by end of the week.

[xiaoxiang781216]

@xiaoxiang-xiaomi: i think this patch break the rpmsg protocol, a rpmsg channel is a service name + one or several associated endpoints.

rpmsg protocol = main endpoint + aided endpoints
But, only main endpoint need the service name and then utilize NS service to exchange the port information, all other endpoints should learn the peer port from the main endpoint message, so the service name is only meaningful for the main endpoint. kernel's rpmsg_create_ept don't use the name in the implementation too.

This partially true for Linux, as there is a channel notion with a default endpoint and a service. you can create endpoints on top of this channel. But in this case an endpoint is still linked to service (attached to the channel).
here is the definition of a channel in Linux kernel (https://elixir.bootlin.com/linux/latest/source/Documentation/rpmsg.txt): Channels are identified by a textual name and have a local ("source") rpmsg address, and remote ("destination") rpmsg
address.
If you have a look to rpmsg_core the name is used for the associated device name and attributes.

Since Linux distinguish channel and endpoint, I need clarify a little bit:
1.Channel must have a name, and Linux will automatically create the channel when receive RPMSG_NS_CREATE and map the channel to rpmsg_device(so the name is important here).
2.Endpoint never has name(you can verify this from struct rpmsg_endpoint), and Linux provide rpmsg_create_ept for driver writer to create the endpoint.

In OpenAMP, the channel concept does no more exist.

Yes, the new OpenAMP remove the channel concept, that's why I add rpmsg_create_anon_ept:
1.rpmsg_create_ept for the named channel/endpoint(like Linux's rpmsg_device)
2.rpmsg_create_anon_ept for the unnamed endpoint(like Linux's rpmsg_endpoint)

• you create a first endpoint (A) that generates an NS announcement,
• then 2 Alternatives to create a second endpoint (B):

1. you create a new endpoint with the same seervice name, and providing a destination addr. For instance to communicate to the same remote ept (A)
2. you create a second instance of the service by sending a new NS announcement with the same NS name and (dest_add == RPMSG_ADDR_ANY). wait for another destination address . This one
   But creating an endpoint without service associated is for me not correct, even if NS announcement is disabled

But you can see the endpoint(struct rpmsg_endpoint) in Linux side actually hasn't name at all, only channel(struct rpmsg_device) has name.
As you said that OpenAMP use one struct rpmsg_endpoint to represent Linux's rpmsg_device and rpmsg_endpoint, we have to extend OpenAMP a little bit:
1.rpmsg_endpoint with name equals to Linux's rpmsg_device
2.rpmsg_endpoint without name equals to Linux's rpmsg_endpoint

That's why I add a new API rpmsg_create_anon_ept, to help the user:
1.Create the main endpoint by rpmsg_create_ept with name
2.Create the aided endpoints by rpmsg_create_anon_ept
So both case could support very well, no break here.
It probably works, but null service name not clean from my pov as your implementation imposes it.

When you create an endpoint you have to provide a service name. 2 strategies:

For main endpoint, we need provide the service name, but we don't need provide the name for the additional endpoints since NS service shouldn't activate in this case like Linux kernel.
If we activate NS service for the aided endpoints, Linux kernel will create new rpmsg_device to match these new endpoints, but what we want in kernel is the new rpmsg_endpoint.

• dest_addr == RPMSG_ADDR_ANY: you don't know the remote address that support this service. You have wait answer from remote side to get the dest_addr.
• dest_addr != RPMSG_ADDR_ANY: you want to address a specific endpoint, and you know that this endpoint support the service. This is used mainly when the NS_annoucement is disable, but can be used for any other reason even if NS announcement is enabled.

Another case is in rpmsg_device->ns_bind_cb, we receive the peer port and then supply to rpmsg_create_ept. What I enhance here is to send RPMSG_NS_CREATE_ACK to let the peer know our port information.
Yes i understood the patch in rpmsg_create_ept in this way. you need it in rpmsg_create_ept because it could asynchronously created.

[xiaoxiang781216]

I think RPMSG_NS_CREATE_ACK make it's more clear. How about your thinking?

@arnop2 has use cases that there are multiple service endpoints(that is different endpoint addresses) of the same service name. Option 1, Reuse RPMSG_NS_CREATE will not solve this case.

@arnop2, could you explain how the second endpoint create?
As explains in my previous comment, several endpoints can be created with same service name and unknown destination address.

So you create two endpoints with the same name from OpenAMP then two RPMSG_NS_CREATE with the same name will send to Linux kernel? I think rpmsg_create_channel in drivers/rpmsg/virtio_rpmsg_bus.c will complain:
dev_err(dev, "channel %s:%x:%x already exist\n", chinfo->name, chinfo->src, chinfo->dst);

We use it in ST to create virtual TTY over rpmsg (with a rpmsg tty driver on Linux side).This creates a /dev/ttyrpmsg0 and a/dev/ttyrpmsg0 : One to control a co-processor one for co-processor traces.

So two rpmsg_endpoint with the same name from OpenAMP map to one rpmsg_device from Linux kernel? How kernel rpmsg driver distinguish the different OpenAMP instance?

Actually, we also create a similar driver(rpmsg_tty), but each tty has a different channel name to support the multiple tty instances. Here is the related patch:
xiaoxiang781216/linux@f04d238
xiaoxiang781216/linux@1a41be3

It looks like your RPMSG_NS_CREATE_ACK is better to solve this issue. But please also send patches to the kernel side to have wider discussion.

Sure, I will send the patch to kernel by end of the week.

[arnopo]

@xiaoxiang-xiaomi: i think this patch break the rpmsg protocol, a rpmsg channel is a service name + one or several associated endpoints.

rpmsg protocol = main endpoint + aided endpoints
But, only main endpoint need the service name and then utilize NS service to exchange the port information, all other endpoints should learn the peer port from the main endpoint message, so the service name is only meaningful for the main endpoint. kernel's rpmsg_create_ept don't use the name in the implementation too.

This partially true for Linux, as there is a channel notion with a default endpoint and a service. you can create endpoints on top of this channel. But in this case an endpoint is still linked to service (attached to the channel).
here is the definition of a channel in Linux kernel (https://elixir.bootlin.com/linux/latest/source/Documentation/rpmsg.txt): Channels are identified by a textual name and have a local ("source") rpmsg address, and remote ("destination") rpmsg
address.
If you have a look to rpmsg_core the name is used for the associated device name and attributes.

Since Linux distinguish channel and endpoint, I need clarify a little bit:
1.Channel must have a name, and Linux will automatically create the channel when receive RPMSG_NS_CREATE and map the channel to rpmsg_device(so the name is important here).
2.Endpoint never has name(you can verify this from struct rpmsg_endpoint), and Linux provide rpmsg_create_ept for driver writer to create the endpoint.

In OpenAMP, the channel concept does no more exist.

Yes, the new OpenAMP remove the channel concept, that's why I add rpmsg_create_anon_ept:
1.rpmsg_create_ept for the named channel/endpoint(like Linux's rpmsg_device)
2.rpmsg_create_anon_ept for the unnamed endpoint(like Linux's rpmsg_endpoint)

• you create a first endpoint (A) that generates an NS announcement,
• then 2 Alternatives to create a second endpoint (B):

1. you create a new endpoint with the same seervice name, and providing a destination addr. For instance to communicate to the same remote ept (A)
2. you create a second instance of the service by sending a new NS announcement with the same NS name and (dest_add == RPMSG_ADDR_ANY). wait for another destination address . This one
   But creating an endpoint without service associated is for me not correct, even if NS announcement is disabled

But you can see the endpoint(struct rpmsg_endpoint) in Linux side actually hasn't name at all, only channel(struct rpmsg_device) has name.
As you said that OpenAMP use one struct rpmsg_endpoint to represent Linux's rpmsg_device and rpmsg_endpoint, we have to extend OpenAMP a little bit:
1.rpmsg_endpoint with name equals to Linux's rpmsg_device
2.rpmsg_endpoint without name equals to Linux's rpmsg_endpoint

That's why I add a new API rpmsg_create_anon_ept, to help the user:
1.Create the main endpoint by rpmsg_create_ept with name
2.Create the aided endpoints by rpmsg_create_anon_ept
So both case could support very well, no break here.
It probably works, but null service name not clean from my pov as your implementation imposes it.

When you create an endpoint you have to provide a service name. 2 strategies:

For main endpoint, we need provide the service name, but we don't need provide the name for the additional endpoints since NS service shouldn't activate in this case like Linux kernel.
If we activate NS service for the aided endpoints, Linux kernel will create new rpmsg_device to match these new endpoints, but what we want in kernel is the new rpmsg_endpoint.

• dest_addr == RPMSG_ADDR_ANY: you don't know the remote address that support this service. You have wait answer from remote side to get the dest_addr.
• dest_addr != RPMSG_ADDR_ANY: you want to address a specific endpoint, and you know that this endpoint support the service. This is used mainly when the NS_annoucement is disable, but can be used for any other reason even if NS announcement is enabled.

Another case is in rpmsg_device->ns_bind_cb, we receive the peer port and then supply to rpmsg_create_ept. What I enhance here is to send RPMSG_NS_CREATE_ACK to let the peer know our port information.
Yes i understood the patch in rpmsg_create_ept in this way. you need it in rpmsg_create_ept because it could asynchronously created.

Yes we could consider that the endpoint does not need to have a name. But from my pov this does not correspond to the protocol:

• in Linux Channel and endpoint are linked : the channel struct is provided as parameter of rpmsg_create_ept
• in OPENAMP: https://github.com/OpenAMP/open-amp/wiki/RPMsg-Messaging-Protocol shows that endpoint are on top of channels.
  So your proposal seems to break the existing protocol: need acknowledge of all maintainers...

[xiaoxiang781216]

@xiaoxiang-xiaomi: i think this patch break the rpmsg protocol, a rpmsg channel is a service name + one or several associated endpoints.

rpmsg protocol = main endpoint + aided endpoints
But, only main endpoint need the service name and then utilize NS service to exchange the port information, all other endpoints should learn the peer port from the main endpoint message, so the service name is only meaningful for the main endpoint. kernel's rpmsg_create_ept don't use the name in the implementation too.

This partially true for Linux, as there is a channel notion with a default endpoint and a service. you can create endpoints on top of this channel. But in this case an endpoint is still linked to service (attached to the channel).
here is the definition of a channel in Linux kernel (https://elixir.bootlin.com/linux/latest/source/Documentation/rpmsg.txt): Channels are identified by a textual name and have a local ("source") rpmsg address, and remote ("destination") rpmsg
address.
If you have a look to rpmsg_core the name is used for the associated device name and attributes.

Since Linux distinguish channel and endpoint, I need clarify a little bit:
1.Channel must have a name, and Linux will automatically create the channel when receive RPMSG_NS_CREATE and map the channel to rpmsg_device(so the name is important here).
2.Endpoint never has name(you can verify this from struct rpmsg_endpoint), and Linux provide rpmsg_create_ept for driver writer to create the endpoint.

In OpenAMP, the channel concept does no more exist.

Yes, the new OpenAMP remove the channel concept, that's why I add rpmsg_create_anon_ept:
1.rpmsg_create_ept for the named channel/endpoint(like Linux's rpmsg_device)
2.rpmsg_create_anon_ept for the unnamed endpoint(like Linux's rpmsg_endpoint)

• you create a first endpoint (A) that generates an NS announcement,
• then 2 Alternatives to create a second endpoint (B):

1. you create a new endpoint with the same seervice name, and providing a destination addr. For instance to communicate to the same remote ept (A)
2. you create a second instance of the service by sending a new NS announcement with the same NS name and (dest_add == RPMSG_ADDR_ANY). wait for another destination address . This one
   But creating an endpoint without service associated is for me not correct, even if NS announcement is disabled

But you can see the endpoint(struct rpmsg_endpoint) in Linux side actually hasn't name at all, only channel(struct rpmsg_device) has name.
As you said that OpenAMP use one struct rpmsg_endpoint to represent Linux's rpmsg_device and rpmsg_endpoint, we have to extend OpenAMP a little bit:
1.rpmsg_endpoint with name equals to Linux's rpmsg_device
2.rpmsg_endpoint without name equals to Linux's rpmsg_endpoint

That's why I add a new API rpmsg_create_anon_ept, to help the user:
1.Create the main endpoint by rpmsg_create_ept with name
2.Create the aided endpoints by rpmsg_create_anon_ept
So both case could support very well, no break here.
It probably works, but null service name not clean from my pov as your implementation imposes it.

When you create an endpoint you have to provide a service name. 2 strategies:

For main endpoint, we need provide the service name, but we don't need provide the name for the additional endpoints since NS service shouldn't activate in this case like Linux kernel.
If we activate NS service for the aided endpoints, Linux kernel will create new rpmsg_device to match these new endpoints, but what we want in kernel is the new rpmsg_endpoint.

• dest_addr == RPMSG_ADDR_ANY: you don't know the remote address that support this service. You have wait answer from remote side to get the dest_addr.
• dest_addr != RPMSG_ADDR_ANY: you want to address a specific endpoint, and you know that this endpoint support the service. This is used mainly when the NS_annoucement is disable, but can be used for any other reason even if NS announcement is enabled.

Another case is in rpmsg_device->ns_bind_cb, we receive the peer port and then supply to rpmsg_create_ept. What I enhance here is to send RPMSG_NS_CREATE_ACK to let the peer know our port information.
Yes i understood the patch in rpmsg_create_ept in this way. you need it in rpmsg_create_ept because it could asynchronously created.

Yes we could consider that the endpoint does not need to have a name. But from my pov this does not correspond to the protocol:

• in Linux Channel and endpoint are linked : the channel struct is provided as parameter of rpmsg_create_ept
• in OPENAMP: https://github.com/OpenAMP/open-amp/wiki/RPMsg-Messaging-Protocol shows that endpoint are on top of channels.
  So your proposal seems to break the existing protocol: need acknowledge of all maintainers...

Yes, this patch need collaborate at least between OpenAMP and Linux kernel. We can merge it only when both community agree the change.
Since the related patch for kernel is already sent long time ago, I will push the maintainer to give the feedback.

[xiaoxiang781216]

@arnopo @edmooring the current design has a flaw that OpenAMP can't send any message if the kernel don't send a message first. This patch try to fix this flaw.

[arnopo]

Hi @xiaoxiang781216
To move forward on this topic, we would like to propose you to put it on the agenda of a bi-weekly OpenAMP meeting. This is probably a good place to start discussions with different companies and Linux maintainers.
If you are interested in this proposal, please let me know that we are organizing it.

[xiaoxiang781216]

Sure, it's good to move forward.

[vishnu-banavath]

Hi @arnopo ,
I'm curious to know when is this bi-weekly openamp meetings happen and what is the procedure to join the meeting?
Can I be added to the recipient list please?

Thanks

[arnopo]

Hi @arnopo ,
I'm curious to know when is this bi-weekly openamp meetings happen and what is the procedure to join the meeting?
Can I be added to the recipient list please?

Thanks

Not the good place here as it is not related to the PR. Please create a new discussion thread for this, i will answer it