Fix the wrong address usage #204

xiaoxiang781216 · 2020-04-23T18:24:14Z

No description provided.

xiaoxiang781216 · 2020-04-26T14:17:38Z

@arnopo @edmooring this patch series don't allocate the address between 0-1023 like kernel side. Also fix a potential bug that NS adress(0x35) will be allocated accidentally if user has more than 53 active channels.

arnopo

Following commits fix some issue:
c310169
fbcf487
=> could you send them in a separate PR?

The other patches break the legacy and to be accepted we need justifications that the library can not work properly without them. In this case an approach that consists in extending the exiting API should be privileged.

arnopo · 2020-05-04T17:44:32Z

lib/include/openamp/rpmsg.h

+		    const char *name,
+		    uint32_t src, uint32_t dest,
+		    rpmsg_ept_cb cb,
+		    rpmsg_ns_unbind_cb ns_unbind_cb);


Here you also break the API.
Please keep in mind that it is a library so this is used my several third party applications.
That's means that we have to preserve the existing API as much as possible, even if it is not always perfect...

But I can't understand how the user can call the old API without problem: the user can't receive any callback without calling rpmsg_register_endpoint. It isn't only perfect but is wrong! rpmsg_init_ept is designed for the well known server, right? I suppose that all well known server can send/receive message immediately after calling this API, but it is impossible without this patch. even worse the user can't call rpmsg_register_endpoint by self because it is declared in a private header file.

But I can't understand how the user can call the old API without problem: the user can't receive any callback without calling rpmsg_register_endpoint.

Yes it is the normal behavior. you have to register callback for communication.

It isn't only perfect but is wrong! rpmsg_init_ept is designed for the well known server, right? I suppose that all well known server can send/receive message immediately after calling this API, but it is impossible without this patch. even worse the user can't call rpmsg_register_endpoint by self because it is declared in a private header file.

No, rpmsg_init_ept is designed to initialize the ept structure before registering it. But i agree that something is
not good here.
The normal API for end point management is rpmsg_create_ept. I can not see a reason why rpmsg_init_ept is exposed. For me this function should be declared in rpmsg_internal.h.
Any reason that you need to use it, instead of rpmsg_create_ept?

But I can't understand how the user can call the old API without problem: the user can't receive any callback without calling rpmsg_register_endpoint.

Yes it is the normal behavior. you have to register callback for communication.

But could you tell me how we can register callback if rpmsg_register_endpoint is declared in lib/rpmsg/rpmsg_internal.h?

It isn't only perfect but is wrong! rpmsg_init_ept is designed for the well known server, right? I suppose that all well known server can send/receive message immediately after calling this API, but it is impossible without this patch. even worse the user can't call rpmsg_register_endpoint by self because it is declared in a private header file.

No, rpmsg_init_ept is designed to initialize the ept structure before registering it. But i agree that something is

rpmsg_create_ept initilize all rpmsg_endpoint's fields, why need we call rpmsg_init_ept before that? Does it make sense to initialize the same fields twice?

not good here.
The normal API for end point management is rpmsg_create_ept. I can not see a reason why rpmsg_init_ept is exposed. For me this function should be declared in rpmsg_internal.h.
Any reason that you need to use it, instead of rpmsg_create_ept?

From my interception it's better to:
1.call rpmsg_init_ept for the well known service(both src/dest is fixed) and skip all NS interaction
2.call rpmsg_create_ept for the dynamic serice(either src/dest is any) and utilize NS to find the peer

From my interception it's better to:
1.call rpmsg_init_ept for the well known service(both src/dest is fixed) and skip all NS interaction
2.call rpmsg_create_ept for the dynamic serice(either src/dest is any) and utilize NS to find the peer

In both case rpmsg_create_ept have to be called, this is what has been implemented since the 2018.10 release. This function handles fixed and non fixed addresses, NS is sent only if destination address is set to RPMSG_ADDR_ANY.
That's why rpmsg_register_endpoint is not exposed as API and why rpmsg_init_ept should probably not be exposed.

Anyway I just want to make OpenAMP API design become better. If you refuse my change for rpmsg_init_ept I am fine, I will drop in the next update, but for other bug fix

I don't think that this point is close. I'm agree with you there is something wrong with rpmsg_init_ept. This is for me a case where we have probably to fix the API.
Firsdt of all i would understand why you propose to duplicate the endpoint API by updating rpmsg_init_ept instead of using rpmsg_create_ept? Any usecase that you can not support using rpmsg_create_ept?

"NS" channel is created by rpmsg_init_ept, so it's natural to use it for all well known service. Anyway, rpmsg_init_ept in the current form is broken at least as a public API.

arnopo · 2020-05-05T10:38:54Z

lib/include/openamp/rpmsg.h

+							 data, len, true);
+		metal_sleep_usec(RPMSG_TICKS_PER_INTERVAL);
+	}
+


This patch add an additional check on each send.
Then what's happen if case of timeout in your application? you have another loop to try?
Could you explain the rational of this?
i don't see here a real need except for NS_ACK, and only to try to simplify the application code.
And you mention in previous commit: rpmsg_send_offchannel_raw already checks the destination address

This patch add an additional check on each send.
Then what's happen if case of timeout in your application? you have another loop to try?

No, this loop will wait for 15 seconds, I don't think that more wait can help here.

Could you explain the rational of this?

The patch is designed work with: #160
the caller can do:

rpmsg_create_ept rpmsg_send

just like the socket API.

i don't see here a real need except for NS_ACK, and only to try to simplify the application code.

It also work with your favor solution(the dummy server message). Sometime, it's very natual that the client need send the initial message to server just after the channel get created. Without this patch, you have to wait the sever send the first dummy back which make the code more complex than needed. And become impossible if the two phase handshake done inside the rpmsg except we provide the new callback to user.

And you mention in previous commit: rpmsg_send_offchannel_raw already checks the destination address

Yes, but rpmsg_send_offchannel_raw return failure without the retry logic.

This add useless extra code for each message sent using rpmsg_send for one use case related to #160. Without the NS ack, the end point call back can be use tfor the synchro.
i see here more const than advantage to handle a timeout in the rpmsg_send than in application.

The desgin is following the same principle inside rpmsg_virtio_send_offchannel_raw loop:

static int rpmsg_virtio_send_offchannel_raw(struct rpmsg_device *rdev, uint32_t src, uint32_t dst, const void *data, int size, int wait) { ... if (wait) tick_count = RPMSG_TICK_COUNT / RPMSG_TICKS_PER_INTERVAL; else tick_count = 0; while (1) { ... buffer = rpmsg_virtio_get_tx_buffer(rvdev, &buff_len, &idx); metal_mutex_release(&rdev->lock); if (buffer || !tick_count) break; if (avail_size != 0) return RPMSG_ERR_BUFF_SIZE; metal_sleep_usec(RPMSG_TICKS_PER_INTERVAL); tick_count--; }

Does this mean that we shold drop the above loop too? From the design, rpmsg_send should block and wait the resource available, only rpmsg_try_send can return fail directly wihtout wait.

The desgin is following the same principle inside rpmsg_virtio_send_offchannel_raw loop:

static int rpmsg_virtio_send_offchannel_raw(struct rpmsg_device *rdev, uint32_t src, uint32_t dst, const void *data, int size, int wait) { ... if (wait) tick_count = RPMSG_TICK_COUNT / RPMSG_TICKS_PER_INTERVAL; else tick_count = 0; while (1) { ... buffer = rpmsg_virtio_get_tx_buffer(rvdev, &buff_len, &idx); metal_mutex_release(&rdev->lock); if (buffer || !tick_count) break; if (avail_size != 0) return RPMSG_ERR_BUFF_SIZE; metal_sleep_usec(RPMSG_TICKS_PER_INTERVAL); tick_count--; }

Does this mean that we shold drop the above loop too? From the design, rpmsg_send should block and wait the resource available, only rpmsg_try_send can return fail directly wihtout wait.

No, the loop has to be kept here, blocking fro rpmsg_send, not blocking for rpmsg_try_send ( same behavior than Linux API).

If so, why you consider my patch useless? It also handle the case where the resouce unavailable temporarily.

Simply because you have the API to do this in application, so i don't see a strong reason to integrate it in rpmsg_send.

If so, we just need provide rpmsg_try_send I don't see any reason to add rpmsg_send which is very easy to implement on top of rpmsg_try_send. As I said before, it's a common issue all people will hit immediately after he/she create a rpmsg endpoint proactively. The library need provide a solution to help people mitigate the problem, instead let user find the touch bug and repeat the code again and again.

If so, we just need provide rpmsg_try_send I don't see any reason to add rpmsg_send which is very easy to implement on top of rpmsg_try_send.

rpmsg_try_send returns an error if no TX buffer available, rpmsg_send is blocked waiting a TX_buffer. No relation chip with your usecase

As I said before, it's a common issue all people will hit immediately after he/she create a rpmsg endpoint proactively. The library need provide a solution to help people mitigate the problem, instead let user find the touch bug and repeat the code again and again.

As rpmsg_send returns an error if remote address is not defined it is more a bug in application that does not treat error and /or does not use is_rpmsg_ept_ready as show in examples.
So the issue here seems more the use of the API than the API itself.
If user code is inspirated from the examples they should use is_rpmsg_ept_ready.
But If you have implemented the NS ack for your customer you probably also increases this bad usage, by masking the rpmsg current protocol.

the solution to help user seems more the documentation or example.

If you search examples folder, all client side has this code:

/* Create RPMsg endpoint */ ret = rpmsg_create_ept(&lept, rdev, RPMSG_SERVICE_NAME, RPMSG_ADDR_ANY, RPMSG_ADDR_ANY, rpmsg_endpoint_cb, rpmsg_service_unbind); if (ret) { LPERROR("Failed to create RPMsg endpoint.\r\n"); return ret; } while (!is_rpmsg_ept_ready(&lept)) platform_poll(priv);

As OpenAMP maintainer, do you have any responsbility to improve this situation in the next release? I don't think the code like this can survive in Linux kernel. Actually from my point, this patch is just the first step, the next is:
1.Ensure all system under libmetal/lib/lib implement conditon
2.Replace all poll loop(this and rpmsg_virtio_send_offchannel_raw) with the event trigger just like Linux kernel.

arnopo · 2020-05-05T13:04:06Z

apps/examples/echo/rpmsg-echo.c

@@ -58,7 +58,7 @@ int app(struct rpmsg_device *rdev, void *priv)
 	LPRINTF("Try to create rpmsg endpoint.\r\n");

 	ret = rpmsg_create_ept(&lept, rdev, RPMSG_SERVICE_NAME,
-			       0, RPMSG_ADDR_ANY, rpmsg_endpoint_cb,
+			       1024, RPMSG_ADDR_ANY, rpmsg_endpoint_cb,


if you have to update the example you break the legacy.
Could you explain the rational? reserve memory from 0 to 1023 is a design choice for Linux. Today i don't see any reason to prohibit these addresses for OpenAMP, only 0x35 is reserved.
So to have a stable API i would prefer not integrate this commit if it is not an absolute necessity.

if you have to update the example you break the legacy.

Yes, because the example use the reserved address which encourage people use these special address without any thought.

Could you explain the rational? reserve memory from 0 to 1023 is a design choice for Linux. Today i don't see any reason to prohibit these addresses for OpenAMP, only 0x35 is reserved.

From the original design, this region is reserved for the well known service. Both side(Linux and RTOS) need follow the convention: any private protocol shouldn't use the port inside this region to keep the compatibility. The reserved port may allocate to the well know service in the furture. How can you handle this case in the compability way once it happen?

So to have a stable API i would prefer not integrate this commit if it is not an absolute necessity.

But API implementation is wrong, we should fix the wrong behaviour as early as we can. The compability isn't a good reason to keep the bad behaviour and encourage the people do the wrong thing.

From the original design, this region is reserved for the well known service. Both side(Linux and RTOS) need follow the convention: any private protocol shouldn't use the port inside this region to keep the compatibility. The reserved port may allocate to the well know service in the furture. How can you handle this case in the compability way once it happen?

Could you point me in OpenAMP original design that implement this convention, i had a look in 2018.04 release, addresses 0 to 1024 seems not forbidden?
And on Linux side seems more a design choice, that something inposed by the protocol :
/*

Local addresses are dynamically allocated on-demand.

We do not dynamically assign addresses from the low 1024 range,

in order to reserve that address range for predefined services.
*/
I never saw anything that impose to not use these addresses in documentation...
And because predefined address can be set calling endpoint creation, OpenAMP design choice is to not forbid these address.

From the original design, this region is reserved for the well known service. Both side(Linux and RTOS) need follow the convention: any private protocol shouldn't use the port inside this region to keep the compatibility. The reserved port may allocate to the well know service in the furture. How can you handle this case in the compability way once it happen?

What I mean the original desgin is the Linux Kernel implemenation. It's the golden reference implemntation and other implmenation should follow it's convention except you have a reasonable justification.

This is you opinion...
Another paradigm could be that we respect the Linux convention as Linux does not complain about the remote OpenAMP address...

Anyway I think the good place to continue this discussion is the OpenAMP-RP meeting.
It is an interesting topic to discuss to have more opinions on this topic.
@edmooring, any opinion on this?

One day, if Linux kernel decide to assgin the additional number in this region to some new well known service, your program will stop working suddenly. the code can work today, doesn't mean that it's implmentation is right.
Actually, the bug is already in the code base now, if you create the channel more than 0x35, you wil hit the brick. In the furture, you will hit more brick:)

arnopo · 2020-05-05T13:06:37Z

lib/include/openamp/rpmsg.h

-	return (ept->dest_addr != RPMSG_ADDR_ANY) &&
-		(ept->addr != RPMSG_ADDR_ANY);
+	return ept && ept->rdev && ept->dest_addr &&
+		ept->dest_addr != RPMSG_ADDR_ANY;


Same remark here, this break the legacy

If the demo code want to use 1-1023, the code should call rpmsg_init_ept instead. Here is a summary what I understand:
1.Well known service call rpmsg_init_ept with address inside 1-1023
2.The secondary endpoint call rpmsg_init_ept with address >= 1024( exchange the port through some method)
3.Other service call rpmsg_create_ept with address >= 1024
4.It's prefer to call rpmsg_create_ept with RPMSG_ADDR_ANY(~0)
From my point, the quality of example code is very pool because it demo the bad practice:
1.Use the reserved address
2.Use the fixed address
It's better to correct the demo code to remove the bad usage.

Right use RPMSG_ADDR_ANY for the source address in demo is a best usage than fixing the endpoint. But we have to support both. So a good compromize would be to update only a part of the demos. For instance the echo demo.

I will provide a patch change ALL 0 to RPMSG_ADDR_ANY.

Here is the PR:
#207

xiaoxiang781216 · 2020-05-05T18:04:41Z

Following commits fix some issue:
c310169
fbcf487
=> could you send them in a separate PR?

Here is the PR:
#205

The other patches break the legacy and to be accepted we need justifications that the library can not work properly without them. In this case an approach that consists in extending the exiting API should be privileged.

arnopo · 2020-05-11T08:02:56Z

lib/include/openamp/rpmsg.h

+							 data, len, true);
+		metal_sleep_usec(RPMSG_TICKS_PER_INTERVAL);
+	}
+


This add useless extra code for each message sent using rpmsg_send for one use case related to #160. Without the NS ack, the end point call back can be use tfor the synchro.
i see here more const than advantage to handle a timeout in the rpmsg_send than in application.

arnopo · 2020-05-11T08:25:53Z

apps/examples/echo/rpmsg-echo.c

@@ -58,7 +58,7 @@ int app(struct rpmsg_device *rdev, void *priv)
 	LPRINTF("Try to create rpmsg endpoint.\r\n");

 	ret = rpmsg_create_ept(&lept, rdev, RPMSG_SERVICE_NAME,
-			       0, RPMSG_ADDR_ANY, rpmsg_endpoint_cb,
+			       1024, RPMSG_ADDR_ANY, rpmsg_endpoint_cb,


From the original design, this region is reserved for the well known service. Both side(Linux and RTOS) need follow the convention: any private protocol shouldn't use the port inside this region to keep the compatibility. The reserved port may allocate to the well know service in the furture. How can you handle this case in the compability way once it happen?

Could you point me in OpenAMP original design that implement this convention, i had a look in 2018.04 release, addresses 0 to 1024 seems not forbidden?
And on Linux side seems more a design choice, that something inposed by the protocol :
/*

Local addresses are dynamically allocated on-demand.

We do not dynamically assign addresses from the low 1024 range,

in order to reserve that address range for predefined services.
*/
I never saw anything that impose to not use these addresses in documentation...
And because predefined address can be set calling endpoint creation, OpenAMP design choice is to not forbid these address.

arnopo · 2020-05-11T08:58:20Z

lib/include/openamp/rpmsg.h

+		    const char *name,
+		    uint32_t src, uint32_t dest,
+		    rpmsg_ept_cb cb,
+		    rpmsg_ns_unbind_cb ns_unbind_cb);


But I can't understand how the user can call the old API without problem: the user can't receive any callback without calling rpmsg_register_endpoint.

Yes it is the normal behavior. you have to register callback for communication.

It isn't only perfect but is wrong! rpmsg_init_ept is designed for the well known server, right? I suppose that all well known server can send/receive message immediately after calling this API, but it is impossible without this patch. even worse the user can't call rpmsg_register_endpoint by self because it is declared in a private header file.

No, rpmsg_init_ept is designed to initialize the ept structure before registering it. But i agree that something is
not good here.
The normal API for end point management is rpmsg_create_ept. I can not see a reason why rpmsg_init_ept is exposed. For me this function should be declared in rpmsg_internal.h.
Any reason that you need to use it, instead of rpmsg_create_ept?

arnopo · 2020-05-11T09:08:03Z

lib/include/openamp/rpmsg.h

-	return (ept->dest_addr != RPMSG_ADDR_ANY) &&
-		(ept->addr != RPMSG_ADDR_ANY);
+	return ept && ept->rdev && ept->dest_addr &&
+		ept->dest_addr != RPMSG_ADDR_ANY;


Right use RPMSG_ADDR_ANY for the source address in demo is a best usage than fixing the endpoint. But we have to support both. So a good compromize would be to update only a part of the demos. For instance the echo demo.

xiaoxiang781216 · 2020-05-11T18:07:40Z

@arnopo could merge the unquestioned PR first:
#207
#206
#205
then I can update this PR.

arnopo · 2020-05-12T10:16:35Z

@arnopo could merge the unquestioned PR first:
#207
#206
#205
then I can update this PR.

This patch will be merged when both myself and @edmooring have approved them

Then this PR is coming unreadable as there are several feature in one PR, so many parallel discussion.. Would be nice to have one PR per feature.
And the most important, we have all to stay constructive to find solutions.

Here is a sum-up of my current vision on the 3 mains points discussed:

commit: rpmsg: shouldn't allocate 0-1023 address in rpmsg_create_ept
I'd rather not prevent needs that could never happen.
this can never happen.
this will probably generate more impact such as a the support of a new service
waiting this we could generate regression on exiting code.
So it seems to me, more reasonable to postpone and address this point when a real case will come.
Anyway as i proposed let's discuss the point in bi weekly meeting to have more opinion on this.
commit: rpmsg: wait ept ready in rpmsg_send:
I understand your point of view and your request to simplify the user code. But I have 2 concerns about your implementation:
- Is this need is not linked to the NS ack feature? In current OpenAMP implementation as a first message is needed from the remote ept , could we consider that the start of the send can be conditioned by the first message received ( ept callback trigger)?
- Some other companies complain about latency introduced in rpmsg transfer. So I have also to take this into account, and try to not increase message processing as far as possible.
  So what i request here is to understand if there is a use case in current implementation, that justify it, or if this commit should be discussed with the NS ack. Then to avoid to impact other plaforms, a compromise could be to add a new API.
commit rpmsg: merge rpmsg_register_endpoint into rpmsg_init_ept
I think here there is a misunderstanding between you and me. To solve this, i sent you a personal mail that we review code together that we align our comprehension of the code and find a solution.

xiaoxiang781216 · 2020-05-12T14:53:19Z

@arnopo could merge the unquestioned PR first:
#207
#206
#205
then I can update this PR.

This patch will be merged when both myself and @edmooring have approved them

Then this PR is coming unreadable as there are several feature in one PR, so many parallel discussion.. Would be nice to have one PR per feature.

Ok, once #207, #206 and #205 merge, I will split the PR into serveral one.

And the most important, we have all to stay constructive to find solutions.

Here is a sum-up of my current vision on the 3 mains points discussed:

commit: rpmsg: shouldn't allocate 0-1023 address in rpmsg_create_ept
I'd rather not prevent needs that could never happen.

this can never happen.

this will probably generate more impact such as a the support of a new service

waiting this we could generate regression on exiting code.
So it seems to me, more reasonable to postpone and address this point when a real case will come.

Since rpmsg is the protocol between the different OS, it is no reason to break the protocol without any justification. Do you think htat it's good decision OpenAMP will stop working just because the rpmsg community define new standard service or some vendor want to define the private well known service?

Anyway as i proposed let's discuss the point in bi weekly meeting to have more opinion on this.

Ok, it's better to vote on the meeting.

commit: rpmsg: wait ept ready in rpmsg_send:
I understand your point of view and your request to simplify the user code. But I have 2 concerns about your implementation:

Is this need is not linked to the NS ack feature?

The loop already exist in all example now, but NS ack feature isn't mereged yet, so why you think that this patch link to that feature? This patch can move the loop to common place and allow the furture optimation more easlier.

In current OpenAMP implementation as a first message is needed from the remote ept , could we consider that the start of the send can be conditioned by the first message received ( ept callback trigger)?

Yes, this is a solution. But could you think about why all examples select the busy loop method? If you try to convert an example which require the client to send the message immediately after the endpoint get created, you can understand

Some other companies complain about latency introduced in rpmsg transfer. So I have also to take this into account, and try to not increase message processing as far as possible.

Hi, this patch is a step to address this issue. If you let's caller add the loop by them self, how you can resolve the problem in the furture? Sleeping inside the loop contribute the latency, the best method is to use the condition(wait/signal).

So what i request here is to understand if  there is a use case in current implementation,  that justify it, or if this commit should be discussed with the NS ack. Then to avoid to impact other plaforms, a compromise could be to add a new API.
commit rpmsg: merge rpmsg_register_endpoint into rpmsg_init_ept
I think here there is a misunderstanding between you and me. To solve this, i sent you a personal mail that we review code together that we align our comprehension of the code and find a solution.

xiaoxiang781216 · 2020-05-16T14:15:01Z

@arnopo and @edmooring I create the new PR for the controversial patch:
#208
#209
So the patch left in this PR can be merged first.

lib/rpmsg/rpmsg.c

arnopo · 2020-05-20T12:18:10Z

lib/include/openamp/rpmsg.h

+{
+	return ept && ept->rdev && ept->dest_addr != RPMSG_ADDR_ANY;
+}
+


Please could you not move the position of the declaration in file, that a diff show that you only update the return test here.

Ok, I will split the movement to another patch

For this series , it is sufficient to put the function back to its original place that the diff shows that only the condition as been reworked.
Then LGTM for the rest of the series

You can view the individual patches in commits tab or git log, there isn't real difference. But, I want to keep the patchset as small as possible:
https://github.com/apache/incubator-nuttx/tree/master/openamp

Sorry, forget my previous comment. I didn't realize you just moved the function here now.
Today, there is a real interest in doing it, I would drop it here and add it a series with a real need.

Since you are arguing to add poll inside rpmsg_send, I prefer to merge this simple patch first, so the downstream project can redue the patch need to apply even the poll patch can't upstream.

Doing this we will simplify your downstream but perhaps impact another one... Not a good practice, if we accept this one, then we would have to accept future patches from another platform that could not match with your expectation.
And regarding https://github.com/apache/incubator-nuttx/blob/master/openamp/0004-rpmsg-wait-ept-ready-in-rpmsg_send.patch seems to me that it is minor in term of impact in your downstream.

But, can you please tell me what will impact by this patch? And it is also good thing to put the simple inline function at the begining like other header files:
https://github.com/OpenAMP/open-amp/blob/master/lib/include/openamp/remoteproc.h#L432-L448
https://github.com/OpenAMP/open-amp/blob/master/lib/include/openamp/rpmsg_virtio.h#L65-L110

As explained in my previous comment, this commit seems appears to be for your convenience only. Do I accept it to simplify your patches on top of OpenAMP and potentially impact some other platforms which would have to rework their own patch on top of OpenAMP because this one introduce a conflict? I can not find here a good reason to do this as it does not answer to a real need.

lib/rpmsg/rpmsg.c

arnopo

minor changes requested else ok for me

arnopo · 2020-05-25T10:28:11Z

lib/include/openamp/rpmsg.h

+{
+	return ept && ept->rdev && ept->dest_addr != RPMSG_ADDR_ANY;
+}
+


Sorry, forget my previous comment. I didn't realize you just moved the function here now.
Today, there is a real interest in doing it, I would drop it here and add it a series with a real need.

lib/rpmsg/rpmsg.c

to simplify the work in the furture Signed-off-by: Xiang Xiao <[email protected]>

like what is done inside rpmsg_destroy_ept Signed-off-by: Xiang Xiao <[email protected]>

so is_rpmsg_ept_ready can check the validity more easier Signed-off-by: Xiang Xiao <[email protected]>

edmooring

Looks good to me.

xiaoxiang781216 force-pushed the fix-addr branch 5 times, most recently from 8398029 to 41f4f4a Compare April 23, 2020 20:06

arnopo requested review from edmooring and arnopo May 4, 2020 17:33

xiaoxiang781216 force-pushed the fix-addr branch 2 times, most recently from 3eba159 to d93e093 Compare May 5, 2020 08:54

arnopo requested changes May 5, 2020

View reviewed changes

arnopo requested changes May 11, 2020

View reviewed changes

xiaoxiang781216 force-pushed the fix-addr branch 5 times, most recently from 2a42a87 to 5e9a288 Compare May 16, 2020 13:58

arnopo requested changes May 20, 2020

View reviewed changes

xiaoxiang781216 force-pushed the fix-addr branch from e0b80ec to da610d0 Compare May 20, 2020 13:44

xiaoxiang781216 mentioned this pull request May 26, 2020

rpmsg: reserve 0-1023 for predefined services in rpmsg_create_ept #209

Merged

arnopo requested changes May 26, 2020

View reviewed changes

xiaoxiang781216 added 3 commits May 26, 2020 17:52

rpmsg: rpmsg_unregister_endpoint management cleanup

5298ca7

to simplify the work in the furture Signed-off-by: Xiang Xiao <[email protected]>

rpmsg: rpmsg_create_ept don't send RPMSG_NS_CREATE if name is empty

f56189b

like what is done inside rpmsg_destroy_ept Signed-off-by: Xiang Xiao <[email protected]>

rpmsg: zero out rdev field in rpmsg_unregister_endpoint

f5aa98f

so is_rpmsg_ept_ready can check the validity more easier Signed-off-by: Xiang Xiao <[email protected]>

xiaoxiang781216 force-pushed the fix-addr branch 2 times, most recently from cef1a02 to f5aa98f Compare May 26, 2020 18:26

arnopo approved these changes May 28, 2020

View reviewed changes

edmooring approved these changes May 29, 2020

View reviewed changes

arnopo merged commit 94ca6c2 into OpenAMP:master May 29, 2020

arnopo mentioned this pull request Oct 5, 2020

rpmsg: supress broken rpmsg_init_ept API #220

Closed

Fix the wrong address usage #204

Fix the wrong address usage #204

Conversation

xiaoxiang781216 commented Apr 23, 2020

xiaoxiang781216 commented Apr 26, 2020

arnopo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiaoxiang781216 May 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiaoxiang781216 May 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiaoxiang781216 May 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiaoxiang781216 May 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiaoxiang781216 May 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiaoxiang781216 May 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiaoxiang781216 May 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiaoxiang781216 commented May 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiaoxiang781216 commented May 11, 2020

arnopo commented May 12, 2020

xiaoxiang781216 commented May 12, 2020 • edited Loading

xiaoxiang781216 commented May 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiaoxiang781216 May 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnopo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edmooring left a comment

Choose a reason for hiding this comment

xiaoxiang781216 May 5, 2020 •

edited

Loading

xiaoxiang781216 May 11, 2020 •

edited

Loading

xiaoxiang781216 May 11, 2020 •

edited

Loading

xiaoxiang781216 May 11, 2020 •

edited

Loading

xiaoxiang781216 May 5, 2020 •

edited

Loading

xiaoxiang781216 May 11, 2020 •

edited

Loading

xiaoxiang781216 May 5, 2020 •

edited

Loading

xiaoxiang781216 commented May 12, 2020 •

edited

Loading

xiaoxiang781216 May 25, 2020 •

edited

Loading