Repository navigation
rpmsg_virtio_send_offchannel_nocopy does not return error when RPMSG_ASSERT fails #342
Description
This routine ultimately returns the number of bytes transmitted to a calling API.
The following fail cases do not propagate a failure back up the call stack and therefore the caller will think the data has been transmitted:
status = metal_io_block_write(io, metal_io_virt_to_offset(io, hdr),
&rp_hdr, sizeof(rp_hdr));
RPMSG_ASSERT(status == sizeof(rp_hdr), "failed to write header\r\n");
. . .
/* Enqueue buffer on virtqueue. */
status = rpmsg_virtio_enqueue_buffer(rvdev, hdr, buff_len, idx);
RPMSG_ASSERT(status == VQUEUE_SUCCESS, "failed to enqueue buffer\r\n");
We could either propagate the error code back up the call stack, return a generic error indicating tx failure or return 0 indicating no data was transmitted.
Activity
edmooring commentedon Feb 6, 2022
This is something worth discussing. The current approach using an assertion will kill the whole process if it fails, AND a special debug symbol is defined. Otherwise, things will attempt to progress as normal, except that an operation has failed without an error indication. I believe that the first case should be treated as a catastrophic error. The second case should return some sort of useful error code, since being out of buffers (for example) could well be a transitory condition that could succeed later.
tammyleino commentedon Mar 24, 2022
@arnopo Do you have an opinion on this topic?
arnopo commentedon Mar 25, 2022
Full agree with with @edmooring analysis. In
rpmsg_virtio_send_offchannel_nocopythe status returned by therpmsg_virtio_enqueue_bufferfunction should be propagated ( transforming VIRTQUEUE error in RPMSG error).tammyleino commentedon Apr 12, 2022
I think the mindset to kill the process stems from not being able to return a buffer to the virtqueue. For example, if rpmsg_virtio_send_offchannel_nocopy fails, rpmsg_virtio_send_offchannel_raw should return the buffer it reserved, but there is no way to do this. So I think this ticket and #361 should be resolved together.
arnopo commentedon May 9, 2023
new rpmsg_release_tx_buffer has been introduced in #401 that should help to fix current issue
github-actions commentedon Oct 23, 2023
This issue has been marked as a stale issue because it has been open (more than) 45 days with no activity.