Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

manifest: Bump libmetal and open-amp #74186

Merged
merged 1 commit into from
Jul 5, 2024

Conversation

carlocaione
Copy link
Collaborator

To v2024.05.0

@zephyrbot
Copy link
Collaborator

zephyrbot commented Jun 12, 2024

The following west manifest projects have been modified in this Pull Request:

Name Old Revision New Revision Diff
libmetal zephyrproject-rtos/libmetal@243eed5 zephyrproject-rtos/libmetal@a6851ba (master) zephyrproject-rtos/libmetal@243eed54..a6851ba6
open-amp zephyrproject-rtos/open-amp@da78aea zephyrproject-rtos/open-amp@76d2168 (master) zephyrproject-rtos/open-amp@da78aea6..76d2168b

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@carlocaione carlocaione force-pushed the bump_v2024.05.0 branch 2 times, most recently from 8418d3d to 3eeb121 Compare June 12, 2024 17:52
@zephyrbot zephyrbot added the DNM This PR should not be merged (Do Not Merge) label Jun 12, 2024
@carlocaione carlocaione marked this pull request as ready for review June 12, 2024 17:53
@carlocaione carlocaione requested a review from arnopo June 12, 2024 17:54
@zephyrbot zephyrbot added the area: IPC Inter-Process Communication label Jun 13, 2024
@arnopo
Copy link
Collaborator

arnopo commented Jun 13, 2024

Hi @carlocaione
building samples/subsys/ipc/openamp_rsc_table/ generate following warning

[24/182] Building C object zephyr/CMakeFiles/zephyr.dir/lib/open-amp/resource_table.c.obj
In file included from /zephyrproject/zephyr/lib/open-amp/./resource_table.h:11,
                 from /zephyrproject/zephyr/lib/open-amp/resource_table.c:30:
/zephyrproject/modules/lib/open-amp/open-amp/lib/include/openamp/virtio.h:83:2: warning: #warning "VIRTIO_DRIVER_SUPPORT and/or VIRTIO_DEVICE_SUPPORT should be defined" [-Wcpp]
   83 | #warning "VIRTIO_DRIVER_SUPPORT and/or VIRTIO_DEVICE_SUPPORT should be defined"
      |  ^~~~~~~
[26/182] Building C object CMakeFiles/app.dir/src/main_remote.c.obj
In file included from /zephyrproject/modules/lib/open-amp/open-amp/lib/include/openamp/rpmsg_virtio.h:19,
                 from /zephyrproject/modules/lib/open-amp/open-amp/lib/include/openamp/open_amp.h:12,
                 from /zephyrproject/zephyr/samples/subsys/ipc/openamp_rsc_table/src/main_remote.c:16:
/zephyrproject/modules/lib/open-amp/open-amp/lib/include/openamp/virtio.h:83:2: warning: #warning "VIRTIO_DRIVER_SUPPORT and/or VIRTIO_DEVICE_SUPPORT should be defined" [-Wcpp]
   83 | #warning "VIRTIO_DRIVER_SUPPORT and/or VIRTIO_DEVICE_SUPPORT should be defined"
      |  ^~~~~~~
      

The issue is linked to the replacement of VIRTIO_DRIVER_ONLY and VIRTIO_DEVICE_ONLY by VIRTIO_DRIVER_SUPPORT and VIRTIO_DEVICE_SUPPORT

I haven't found a clean way to fix it yet

@carlocaione
Copy link
Collaborator Author

carlocaione commented Jun 13, 2024

The issue is linked to the replacement of VIRTIO_DRIVER_ONLY and VIRTIO_DEVICE_ONLY by VIRTIO_DRIVER_SUPPORT and VIRTIO_DEVICE_SUPPORT

I haven't found a clean way to fix it yet

This is my fix 7da4a2c as first commit in this PR.

But it looks like this upgrade is breaking also something else.

@carlocaione
Copy link
Collaborator Author

@arnopo your fix is working so I think it is worth to have an open-amp minor release for that. Still some tests on zephyr are failing with the new release.

@arnopo
Copy link
Collaborator

arnopo commented Jun 14, 2024

@arnopo your fix is working so I think it is worth to have an open-amp minor release for that.

I will send a PR on open-amp.

Still some tests on zephyr are failing with the new release.

Seem related to BT, is rpmsg used in the test project?

@arnopo
Copy link
Collaborator

arnopo commented Jun 14, 2024

@arnopo your fix is working so I think it is worth to have an open-amp minor release for that.

I will send a PR on open-amp.

OpenAMP/open-amp#596

@arnopo
Copy link
Collaborator

arnopo commented Jun 18, 2024

@carlocaione any idea what go wrong in the CI test?
I would prefer to wait to integrate OpenAMP/open-amp#596 until we resolve CI issue, to ensure that there is no other regressions in Open-AMP.

@carlocaione
Copy link
Collaborator Author

@arnopo no idea and I don't even the hardware anymore to reproduce it. @Thalley @carlescufi anyone from Nordic who wants to take a look at this?

@Thalley
Copy link
Collaborator

Thalley commented Jun 18, 2024

@arnopo no idea and I don't even the hardware anymore to reproduce it. @Thalley @carlescufi anyone from Nordic who wants to take a look at this?

Can you elaborate on what you'd like us to look at?

@arnopo
Copy link
Collaborator

arnopo commented Jun 19, 2024

@arnopo no idea and I don't even the hardware anymore to reproduce it. @Thalley @carlescufi anyone from Nordic who wants to take a look at this?

Can you elaborate on what you'd like us to look at?

Hello @Thalley ,

We need your assistance to understand what is failing during the bsim-test. The failure appears to be related to the update of the OpenAMP or libmetal library, but there is nothing evident in the test log. Can you help us investigate this issue?

https://github.com/zephyrproject-rtos/zephyr/actions/runs/9512500748/job/26220609947?pr=74186

@carlocaione
Copy link
Collaborator Author

carlocaione commented Jun 19, 2024

@Thalley
Copy link
Collaborator

Thalley commented Jun 19, 2024

@arnopo no idea and I don't even the hardware anymore to reproduce it. @Thalley @carlescufi anyone from Nordic who wants to take a look at this?

Can you elaborate on what you'd like us to look at?

Hello @Thalley ,

We need your assistance to understand what is failing during the bsim-test. The failure appears to be related to the update of the OpenAMP or libmetal library, but there is nothing evident in the test log. Can you help us investigate this issue?

https://github.com/zephyrproject-rtos/zephyr/actions/runs/9512500748/job/26220609947?pr=74186

The BSIM test that fails looks like it's failing on the nrf52_bsim which doesn't use this, so I think the failing BSIM test is unrelated to this PR. There has been a lot of fixes merged in the last 5 days, so I would suggest to retrigger the test to see if it still fails.

For the logging issue I have no idea unfortunately. To make sure there's an issue, I would also retry that.

@arnopo
Copy link
Collaborator

arnopo commented Jun 20, 2024

Bsim tests still fail, but errors seem different. 😟

@Thalley
Copy link
Collaborator

Thalley commented Jun 20, 2024

Bsim tests still fail, but errors seem different. 😟

At least it's nRF53 failures this time :D Looks like segmentation faults - Should be possible to re-run those tests locally with Valgrind

@aescolar
Copy link
Member

@carlocaione @arnopo

==4312==    at 0x8121455: ept_cb (ipc_rpmsg_static_vrings.c:237)
==4312==    by 0x8157DBD: rpmsg_virtio_rx_callback (rpmsg_virtio.c:594)
==4312==    by 0x81565B9: virtqueue_notification (virtqueue.c:600)
==4312==    by 0x81215F1: mbox_callback_process (ipc_rpmsg_static_vrings.c:313)
==4312==    by 0x815AED8: work_queue_main (work.c:688)
==4312==    by 0x811E482: z_thread_entry (thread_entry.c:48)

The segfault happens because priv (ept->priv in caller) is null and being derefferenced.

@arnopo
Copy link
Collaborator

arnopo commented Jun 24, 2024

I think I have found the regression.

I have updated the OpenAMP/open-amp#596 adding
OpenAMP/open-amp@8d08fae patch

could you please update openamp module and rerun CI?

@carlocaione
Copy link
Collaborator Author

@arnopo done. Let's see.

@carlocaione
Copy link
Collaborator Author

@arnopo yeah, it looks fine now. If you push a new release I'll take that into the zephyr-openamp repo.

@arnopo
Copy link
Collaborator

arnopo commented Jun 25, 2024

@arnopo yeah, it looks fine now. If you push a new release I'll take that into the zephyr-openamp repo.

Yes sure, we have to do more tests and add to v2024.05.01 release to fix this. I will ping you when ready.
Your review on the OpenAMP/open-amp#596 would be appreciate in this context.
Thanks in advance

@nashif
Copy link
Member

nashif commented Jun 27, 2024

@carlocaione can you please update? the PRs were merged, so manifest needs to be updated.

@carlocaione
Copy link
Collaborator Author

@carlocaione can you please update? the PRs were merged, so manifest needs to be updated.

@nashif you are rushing things. We have to wait for OpenAMP/open-amp#596 to be merged, after that I have to fix zephyrproject-rtos/open-amp#19 (that was supposed not to be merged) and after that we can update and merge this one.

@arnopo
Copy link
Collaborator

arnopo commented Jun 28, 2024

@carlocaione can you please update? the PRs were merged, so manifest needs to be updated.

@nashif you are rushing things. We have to wait for OpenAMP/open-amp#596 to be merged, after that I have to fix zephyrproject-rtos/open-amp#19 (that was supposed not to be merged) and after that we can update and merge this one.

the OpenAMP/open-amp#596 is no merged

To v2024.05.1

Signed-off-by: Carlo Caione <ccaione@baylibre.com>
@zephyrbot zephyrbot removed the DNM This PR should not be merged (Do Not Merge) label Jul 4, 2024
@nashif nashif added this to the v3.7.0 milestone Jul 4, 2024
@aescolar aescolar merged commit 2e1c04a into zephyrproject-rtos:main Jul 5, 2024
24 of 25 checks passed
@carlocaione carlocaione deleted the bump_v2024.05.0 branch July 9, 2024 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants