OpenAMP / open-amp

The main OpenAMP library implementing RPMSG, Virtio, and Remoteproc for RTOS etc
https://www.openampproject.org/
Other
721 stars 291 forks source link

rpmsg_virtio: fix rpmsg_virtio_get_tx_payload_buffer() error #614

Open wyr-7 opened 2 months ago

wyr-7 commented 2 months ago

if rpmsg_virtio_notify_wait return RPMSG_SUCCESS, we don't call rpmsg_virtio_get_tx_buffer

arnopo commented 1 month ago

Please provide more details on issues you address

  1. don't need check status when get_tx_payload The status it checked because it can have been updated ( for instance host crash). so prefer to keep it

  2. driver/rptun_notify_wait and driver/rpmsg_virtio_notify_wait may return -EAGAIN, so if only judge status == RPMSG_EOPNOTSUPP, it will cause busy loop in rpmsg_virtio_get_tx_payload_buffer; this one seems to fix a bug: if rpmsg_virtio_notify_wait return RPMSG_SUCCESS, we don't call rpmsg_virtio_get_tx_buffer

  3. add assert when get tx buffer failed Pleas detail, why do you consider a logic problem? it can occurs if the host side is busy...

CV-Bowen commented 1 month ago

@arnopo @edmooring About your questions:

don't need check status when get_tx_payload

When the remote core crashed, local core should crashed or reboot to recovery the whole AMP system? So how about add a new ops panicin remoteproc instead slove this problem in rpmsg transport layer?

add assert when get tx buffer failed

We think when local core can't get the tx buffer for 15 seconds, there are already some issues so let's direclty assert to found this issue.

arnopo commented 1 month ago

@arnopo @edmooring About your questions:

don't need check status when get_tx_payload

When the remote core crashed, local core should crashed or reboot to recovery the whole AMP system? So how about add a new ops panicin remoteproc instead slove this problem in rpmsg transport layer?

add assert when get tx buffer failed

We think when local core can't get the tx buffer for 15 seconds, there are already some issues so let's direclty assert to found this issue.

I'm not in favor of deciding the behavior in the library. This should be a system decision. For instance, you can have a processor that manages some safety or critical resources. In such a case, we would not crash if the remote processor crashes. It is better to avoid panics and asserts as much as possible, especially if the result depends on the remote processor.

CV-Bowen commented 1 month ago

@arnopo OK, the scenes you described do indeed require these features. @wyr-7 Let's only keep the commit 2.

wyr-7 commented 1 month ago

@arnopo OK, the scenes you described do indeed require these features. @wyr-7 Let's only keep the commit 2.

Ok, @arnopo @edmooring @CV-Bowen commits 1 and 3 were removed and only keep the commit 2.

wyr-7 commented 1 month ago

Please fix commit message ( uper case , '.', subject, ...)

rpmsg: fix rpmsg_virtio_get_tx_payload_buffer error

If rpmsg_virtio_notify_wait returns RPMSG_SUCCESS, we don't call
rpmsg_virtio_get_tx_buffer.

else look go to go.

OK, I'll pay attention to it later. Done.

arnopo commented 1 month ago

I would like to merge this one in the release as it fixes an issue

@wyr-7 could you address my comment before tomorrow code freeze? @edmooring @tnmysh, could you review it?

Thanks in Advance

tnmysh commented 1 month ago

LGTM.

wyr-7 commented 1 month ago

@arnopo @CV-Bowen Thanks, I have updated the latest discussion results to this PR.

wyr-7 commented 1 month ago

@arnopo There are some errors when running CI, can you help to check it out?

`WARNING: The directory '/github/home/.cache/pip' or its parent directory is not owned or is not writable by the current user. The cache has been disabled. Check the permissions and owner of that directory. If executing pip with sudo, you should use sudo's -H flag.
Collecting cmake
  Downloading cmake-3.30.5-py3-none-manylinux_2_17_x86_64.manylinux2014_x86_64.whl.metadata (6.4 kB)
Downloading cmake-3.30.5-py3-none-manylinux_2_17_x86_64.manylinux2014_x86_64.whl (26.9 MB)
   ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 26.9/26.9 MB 278.6 MB/s eta 0:00:00
Installing collected packages: cmake
Successfully installed cmake-3.30.5
SDK URL=
error: Blank SDK download URL`
arnopo commented 1 month ago

@arnopo There are some errors when running CI, can you help to check it out?

`WARNING: The directory '/github/home/.cache/pip' or its parent directory is not owned or is not writable by the current user. The cache has been disabled. Check the permissions and owner of that directory. If executing pip with sudo, you should use sudo's -H flag.
Collecting cmake
  Downloading cmake-3.30.5-py3-none-manylinux_2_17_x86_64.manylinux2014_x86_64.whl.metadata (6.4 kB)
Downloading cmake-3.30.5-py3-none-manylinux_2_17_x86_64.manylinux2014_x86_64.whl (26.9 MB)
   ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 26.9/26.9 MB 278.6 MB/s eta 0:00:00
Installing collected packages: cmake
Successfully installed cmake-3.30.5
SDK URL=
error: Blank SDK download URL`

Yes something goes wrong with this test, just ignore it for the moment as it is relying to the Zephyr main branch that can not be stage.

wyr-7 commented 1 month ago

Yes something goes wrong with this test, just ignore it for the moment as it is relying to the Zephyr main branch that can not be stage.

Ok, thanks.

arnopo commented 1 month ago

I mergerd PR #624.

@CV-Bowen @wyr-7 you can update this one to update rpmsg_virtio_notify_wait and other API to better meet your needs