openamp: change rx buffer hold flag to count #524

CV-Bowen · 2023-10-17T03:48:35Z

This commit fix one issue, before this commit, if rpmsg_hold_rx_buffer() is called in the endpoint callback to hold current received buffer and call rpmsg_release_rx_buffer() to release this buffer immediately.
This buffer will be returned to the virtqueue in rpmsg_virtio_release_rx_buffer() and returned to virtqueue again in rpmsg_virtio_return_buffer() after the ept->cb().
So same buffer returned to the virtqueue twice, error happened.
Follow shows this process:

rpmsg_virtio_rx_callback()
  - get rp_hdr
  - ept->cb(data = RPMSG_LOCATE_DATA(rp_hdr))
    - rpsmg_hold_rx_buffer(data)
    - rpmsg_release_rx_buffer(data) here has returned buffer to virtqueue
  - rpmsg_virtio_return_buffer() return same buffer to virtqueue again

The root cause of this issue is that the RPMSG_BUF_HELD has been cleared in rpmsg_release_rx_buffer() so rpmsg_virtio_rx_callback() will treat this buffer is not HELD and call rpmsg_virtio_return_buffer() to return this buffer again.

This commit change the HELD flag in rp_hdr to count to fix this issue and also support hold/release rx buffer recursively.

arnopo

Strange to me that you hold the buffer to release it in same function. I would say hold it at the end of the ept->cb if needed...
That say I think it is interesting to use a counter instead of a flag, as we could have several consumers.

What I'm expecting here is that we have same hold mechanism for the RX and the TX buffers. So please update also the TX part

lib/rpmsg/rpmsg_virtio.c

CV-Bowen · 2023-10-20T02:24:25Z

Strange to me that you hold the buffer to release it in same function. I would say hold it at the end of the ept->cb if needed... That say I think it is interesting to use a counter instead of a flag, as we could have several consumers.

What I'm expecting here is that we have same hold mechanism for the RX and the TX buffers. So please update also the TX part

@arnopo Hi, I'm a little confused about the update also the TX part, do you mean add rpmsg_virtio_hold_tx_buffer() in rpmsg_virtio and rpmsg_hold_tx_buffer() in rpmsg, and also add hold_tx_buffer ops to the struct rpmsg_device_ops to make user can call this function?

arnopo · 2023-10-24T12:25:19Z

Strange to me that you hold the buffer to release it in same function. I would say hold it at the end of the ept->cb if needed... That say I think it is interesting to use a counter instead of a flag, as we could have several consumers.
What I'm expecting here is that we have same hold mechanism for the RX and the TX buffers. So please update also the TX part

@arnopo Hi, I'm a little confused about the update also the TX part, do you mean add rpmsg_virtio_hold_tx_buffer() in rpmsg_virtio and rpmsg_hold_tx_buffer() in rpmsg, and also add hold_tx_buffer ops to the struct rpmsg_device_ops to make user can call this function?

My request was not cristal clear.
No need to add user interface to hold TX buffer. my point was just to use RPMSG_BUF_HELD_INC() and RPMSG_BUF_HELD_DEC() internally for the TX buffers.

CV-Bowen · 2023-10-26T06:36:54Z

Strange to me that you hold the buffer to release it in same function. I would say hold it at the end of the ept->cb if needed... That say I think it is interesting to use a counter instead of a flag, as we could have several consumers.
What I'm expecting here is that we have same hold mechanism for the RX and the TX buffers. So please update also the TX part

@arnopo Hi, I'm a little confused about the update also the TX part, do you mean add rpmsg_virtio_hold_tx_buffer() in rpmsg_virtio and rpmsg_hold_tx_buffer() in rpmsg, and also add hold_tx_buffer ops to the struct rpmsg_device_ops to make user can call this function?

My request was not cristal clear. No need to add user interface to hold TX buffer. my point was just to use RPMSG_BUF_HELD_INC() and RPMSG_BUF_HELD_DEC() internally for the TX buffers.

@arnopo Got, and I has deleted the user interface. Could you take a look again.

lib/rpmsg/rpmsg_virtio.c

arnopo · 2023-10-26T11:46:55Z

lib/rpmsg/rpmsg_virtio.c


 	rvdev = metal_container_of(rdev, struct rpmsg_virtio_device, rdev);

 	metal_mutex_acquire(&rdev->lock);

-	r_desc->idx = idx;
+	/* Check the hold status first */
+	if (RPMSG_BUF_HELD_STATUS(rp_hdr) <= 0) {


can be less than 0 ?

Release one buffer twice? Actually, I want to add a RPMSG_ASSERT here to check the incorrect usage of buffer hold/release API. Do you have some suggestions?

I suppose thta the issue is that user call twice time rpmsg_virtio_buf_held_dec_test, right?
so a test could be added in rpmsg_virtio_buf_held_dec_test with a log error

lib/rpmsg/rpmsg_virtio.c

lib/rpmsg/rpmsg_internal.h

lib/rpmsg/rpmsg_virtio.c

lib/rpmsg/rpmsg_internal.h

xiaoxiang781216 · 2023-11-08T16:13:52Z

@arnopo do you have more suggestion?

lib/rpmsg/rpmsg_virtio.c

arnopo

LGTM adressing @xiaoxiang781216 comment

edmooring

Looks good to me.

arnopo

Minor typos to fix before I merge it.
you can also remove "This commit fix one issue, " in the commit message

lib/rpmsg/rpmsg_virtio.c

Before this commit, if rpmsg_hold_rx_buffer() is called in the endpoint callback to hold current rx buffer and call rpmsg_release_rx_buffer() to release this buffer immediately, this rx buffer will be returned to the virtqueue twice: 1. the first time is in rpmsg_virtio_release_rx_buffer() 2. and the second time is in rpmsg_virtio_return_buffer() after ept->cb() Follow shows this process: rpmsg_virtio_rx_callback() - get rp_hdr - ept->cb(data = RPMSG_LOCATE_DATA(rp_hdr)) - rpsmg_hold_rx_buffer(data) - rpmsg_release_rx_buffer(data) return buffer to virtqueue - rpmsg_virtio_return_buffer(data) return same buffer to virtqueue again So this commit changes the HELD flag in rp_hdr to count to avoid this use case and also supports hold/release rx buffer recursively. Signed-off-by: Guiding Li <liguiding1@xiaomi.com> Signed-off-by: Yin Tao <yintao@xiaomi.com> Signed-off-by: Bowen Wang <wangbowen6@xiaomi.com>

CV-Bowen · 2024-01-09T11:56:33Z

@arnopo Thanks, all the comments have been addressed.

CV-Bowen force-pushed the rpmsg-hold-to-count branch 3 times, most recently from d345547 to e98367f Compare October 17, 2023 03:58

arnopo requested review from edmooring, arnopo and tnmysh October 17, 2023 17:11

arnopo requested changes Oct 19, 2023

View reviewed changes

lib/rpmsg/rpmsg_virtio.c Outdated Show resolved Hide resolved

lib/rpmsg/rpmsg_virtio.c Outdated Show resolved Hide resolved

lib/rpmsg/rpmsg_virtio.c Outdated Show resolved Hide resolved

lib/rpmsg/rpmsg_virtio.c Outdated Show resolved Hide resolved

CV-Bowen force-pushed the rpmsg-hold-to-count branch 3 times, most recently from 075dc53 to f7db776 Compare October 20, 2023 03:51

arnopo closed this Oct 24, 2023

arnopo reopened this Oct 24, 2023

CV-Bowen force-pushed the rpmsg-hold-to-count branch 2 times, most recently from c87de36 to 7e2bf84 Compare October 26, 2023 06:35

arnopo requested changes Oct 27, 2023

View reviewed changes

CV-Bowen force-pushed the rpmsg-hold-to-count branch from 7e2bf84 to 7f670be Compare October 30, 2023 11:13

xiaoxiang781216 reviewed Nov 1, 2023

View reviewed changes

lib/rpmsg/rpmsg_internal.h Outdated Show resolved Hide resolved

CV-Bowen force-pushed the rpmsg-hold-to-count branch from 7f670be to e42e1df Compare November 2, 2023 13:53

CV-Bowen mentioned this pull request Nov 3, 2023

rpmsg_virtio: add RPMSG_ASSERT to check the virtqueue add error #526

Merged

arnopo reviewed Nov 10, 2023

View reviewed changes

lib/rpmsg/rpmsg_virtio.c Outdated Show resolved Hide resolved

lib/rpmsg/rpmsg_virtio.c Show resolved Hide resolved

lib/rpmsg/rpmsg_virtio.c Outdated Show resolved Hide resolved

lib/rpmsg/rpmsg_virtio.c Show resolved Hide resolved

CV-Bowen force-pushed the rpmsg-hold-to-count branch 2 times, most recently from ac266f6 to b7bd536 Compare December 4, 2023 11:51

xiaoxiang781216 reviewed Dec 4, 2023

View reviewed changes

lib/rpmsg/rpmsg_virtio.c Outdated Show resolved Hide resolved

lib/rpmsg/rpmsg_virtio.c Outdated Show resolved Hide resolved

arnopo approved these changes Dec 5, 2023

View reviewed changes

CV-Bowen force-pushed the rpmsg-hold-to-count branch from b7bd536 to 30ff35f Compare January 5, 2024 08:47

edmooring approved these changes Jan 5, 2024

View reviewed changes

arnopo reviewed Jan 9, 2024

View reviewed changes

CV-Bowen force-pushed the rpmsg-hold-to-count branch from 30ff35f to 7e09f36 Compare January 9, 2024 11:54

arnopo merged commit b32187e into OpenAMP:main Jan 10, 2024
3 checks passed

arnopo added this to the Release V2024.04 milestone Jan 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

openamp: change rx buffer hold flag to count #524

openamp: change rx buffer hold flag to count #524

CV-Bowen commented Oct 17, 2023 •

edited

Loading

arnopo left a comment

CV-Bowen commented Oct 20, 2023

arnopo commented Oct 24, 2023 •

edited

Loading

CV-Bowen commented Oct 26, 2023

arnopo Oct 26, 2023

CV-Bowen Oct 30, 2023

arnopo Nov 10, 2023

CV-Bowen Dec 4, 2023

xiaoxiang781216 commented Nov 8, 2023

arnopo left a comment

edmooring left a comment

arnopo left a comment

CV-Bowen commented Jan 9, 2024

openamp: change rx buffer hold flag to count #524

openamp: change rx buffer hold flag to count #524

Conversation

CV-Bowen commented Oct 17, 2023 • edited Loading

arnopo left a comment

Choose a reason for hiding this comment

CV-Bowen commented Oct 20, 2023

arnopo commented Oct 24, 2023 • edited Loading

CV-Bowen commented Oct 26, 2023

arnopo Oct 26, 2023

Choose a reason for hiding this comment

CV-Bowen Oct 30, 2023

Choose a reason for hiding this comment

arnopo Nov 10, 2023

Choose a reason for hiding this comment

CV-Bowen Dec 4, 2023

Choose a reason for hiding this comment

xiaoxiang781216 commented Nov 8, 2023

arnopo left a comment

Choose a reason for hiding this comment

edmooring left a comment

Choose a reason for hiding this comment

arnopo left a comment

Choose a reason for hiding this comment

CV-Bowen commented Jan 9, 2024

CV-Bowen commented Oct 17, 2023 •

edited

Loading

arnopo commented Oct 24, 2023 •

edited

Loading