rpmsg: let rpmsg_virtio_get_tx_buffer() always return idx in host side

[CV-Bowen]

In rpmsg host side, the tx buffer index is not assigned when this buffer is obtained from the reclaimer list, this commit let rpmsg_virtio_get_tx_buffer() always gets correct idx.

Note: the idx in rpmsg host side is not used, so this commit is just an improvement and not fix any issue.

[arnopo]

Something is bad in my test , if such error has not been detected :(
I need to understand the issue, but I plan to get it for the release as a fix

A minor comment

[arnopo]

I crosschecked the behavior. In rpmsg_virtio_return_buffer, we don't use the index for host side. So in theory not need to set it.

Do you have a concrete use case for which your are facing an issue?

[CV-Bowen]

@arnopo The index will be used in rpmsg_virtio_release_tx_buffer(): idx = rp_hdr->reserved;

static int rpmsg_virtio_release_tx_buffer(struct rpmsg_device *rdev, void *txbuf)
{
	struct rpmsg_virtio_device *rvdev;
	struct rpmsg_hdr *rp_hdr = RPMSG_LOCATE_HDR(txbuf);
	void *vbuff = rp_hdr;  /* only used to avoid warning on the cast of a packed structure */
	struct vbuff_reclaimer_t *r_desc = (struct vbuff_reclaimer_t *)vbuff;
	uint16_t idx;

	/*
	 * Reuse the RPMsg buffer to temporary store the vbuff_reclaimer_t structure.
	 * Stores the index locally before overwriting the RPMsg header.
	 */
	idx = rp_hdr->reserved;

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

	metal_mutex_acquire(&rdev->lock);

	r_desc->idx = idx;
	metal_list_add_tail(&rvdev->reclaimer, &r_desc->node);

	metal_mutex_release(&rdev->lock);

	return RPMSG_SUCCESS;
}

And the rp_hdr->reserved is set to idx in rpmsg_virtio_get_tx_payload_buffer()

[arnopo]

@arnopo The index will be used in rpmsg_virtio_release_tx_buffer(): idx = rp_hdr->reserved;

static int rpmsg_virtio_release_tx_buffer(struct rpmsg_device *rdev, void *txbuf)
{
	struct rpmsg_virtio_device *rvdev;
	struct rpmsg_hdr *rp_hdr = RPMSG_LOCATE_HDR(txbuf);
	void *vbuff = rp_hdr;  /* only used to avoid warning on the cast of a packed structure */
	struct vbuff_reclaimer_t *r_desc = (struct vbuff_reclaimer_t *)vbuff;
	uint16_t idx;

	/*
	 * Reuse the RPMsg buffer to temporary store the vbuff_reclaimer_t structure.
	 * Stores the index locally before overwriting the RPMsg header.
	 */
	idx = rp_hdr->reserved;

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

	metal_mutex_acquire(&rdev->lock);

	r_desc->idx = idx;
	metal_list_add_tail(&rvdev->reclaimer, &r_desc->node);

	metal_mutex_release(&rdev->lock);

	return RPMSG_SUCCESS;
}

And the rp_hdr->reserved is set to idx in rpmsg_virtio_get_tx_payload_buffer()

What I would like to figure out is whether this fix is for an existing bug or if it is intended to prevent a potential bug. Upon looking into the code, when role == RPMSG_HOST, the index is not used.

In the end, I think that we should set the idx as you propose. However, what I am trying to determine first is whether this fix is for an existing bug or if it is an improvement to the code's robustness."

[CV-Bowen]

@arnopo Yes, the index is not used in HOST side and after discussing with @GUIDINGLI, we confirm this commit is just an improvement to code's robustness.

[arnopo]

@arnopo Yes, the index is not used in HOST side and after discussing with @GUIDINGLI, we confirm this commit is just an improvement to code's robustness.

Thanks for the confirmation. Please update the commit in consequence, it is not a fix but an improvement

[CV-Bowen]

@arnopo Done

[arnopo]

LGTM

[arnopo]

@edmooring, @tnmysh ,

Please, could your review this PR?

[tnmysh]

LGTM.