Shouldn't send ns message if no VIRTIO_RPMSG_F_NS or name #161
Conversation
lib/include/openamp/rpmsg.h
Outdated
| * it binds an rpmsg address with an rx callback handler. | ||
| */ | ||
|
|
||
| static inline int rpmsg_create_anon_ept(struct rpmsg_endpoint *ept, |
There was a problem hiding this comment.
Why it needs a function to create endpoint without a name? if the endpoint creation function itself allows empty name.
I am fine that to have endpoint without name assuming it is static endpoint. I think it ok to have static endpoint even when NS feature is enabled, but just I don't need it requires a specific wrapper for it. @arnop2, do you have any concern of it?
There was a problem hiding this comment.
Yes, the static endpoint is useful with NS feature.
Linux kernel also provide a special API rpmsg_create_ept for it. this is one reason that I add rpmsg_create_anon_ept function, another is make the creation of static point a little bit easier and visible.
There was a problem hiding this comment.
I still doubt if it is necessary to define such a function and it is only used for a subset of static endpoints.
There was a problem hiding this comment.
Ok, I will remove rpmsg_create_anon_ept, since the nameless endpoint is a seldom used feature.
lib/rpmsg/rpmsg.c
Outdated
| rpmsg_register_endpoint(rdev, ept); | ||
|
|
||
| if (rdev->support_ns && ept->dest_addr == RPMSG_ADDR_ANY) { | ||
| if (ept->name[0] && rdev->support_ns && ept->dest_addr == RPMSG_ADDR_ANY) { |
There was a problem hiding this comment.
why there is endpoint without a name but it doesn't have dest_addr? I had thought the endpoint without a name is static endpoint.
There was a problem hiding this comment.
Yes, the static endpoint should always have the dest_addr, the modification just make it's symmetry with rpmsg_create_ept
There was a problem hiding this comment.
name is empty and dest address is ANY endpoint is just invalid, it looks like this should be check when creating the endpoint, and then, we don't need to check if the name is empty or not here.
There was a problem hiding this comment.
Yes, this is invalid the combination, the check is unnecessary.
Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
|
|
||
| rdev = ept->rdev; | ||
| if (rdev->support_ns && ept->addr != RPMSG_NS_EPT_ADDR) | ||
| if (ept->name[0] && rdev->support_ns && ept->addr != RPMSG_NS_EPT_ADDR) |
There was a problem hiding this comment.
The same comment as before.
There was a problem hiding this comment.
The check is required since the nameless endpoint have the valid local address but RPMSG_NS_DESTROY shouldn't send out.
and support rpmsg_create_ept with name == NULL Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
|
@wjliang do you have any concern about the new patch? |
|
merged. |
2 participants
@wjliang and @arnop2 let focus on the bug fix first, then the enhancement.
I will rebase #160 on this pull after this pull get approved.