AM4376: Rpmsg to PRU message sequence limitations

Hello Jonathan,

Didn't have time to look at this today. This week is kind of crazy, so if I have not replied within another couple of days, please ping the thread to get my attention.

Regards,

Nick

Hi Nick,

Have you had time to look at my questions?

Best regards,

Jonathan

Hi Nick Saulnier ,

Have you had time to look at my questions?

Best regards,

Jonathan

Hi Nick Saulnier,

We have now gotten to the part where we write the communication with the PRU using poll(), read() and write() in C++.

I open the file /dev/rpmsg_pru33 with the O_NONBLOCK flag, then let poll() return when we can write and read to the file. That seems to work so far, have not tested with large buffers as in my question above.

But I have discovered a weird issue that I consider a severe bug in the driver

When I do a read() call to read 4 bytes from rpmsg the read() function returns the entire message, 13 bytes in my test case. I have checked that the data is indeed copied to my buffer, which I did not expect. This could be a severe issue if you use a smaller buffer and read returns more data than expected.

To replicate the issue:

1. Register the file descriptor of the opened file /dev/rpmsg_pru33 with poll() and POLLIN in events.
2. Make sure the PRU sends some data.
3. poll() returns with POLLIN in revents for the file descriptor.
4. Call read() on the file descriptor for a smaller amount of data than what the PRU sent. In my case I tried to read 4 bytes but read() read 13 which was the entire message.

The cause for reading a bit of the data first is that we want to start with some header information that will determine the size of the rest of the data. Using four bytes for this makes it possible to integrate the rpmsg communication with our existing poll() driven communication that uses the same 4 byte header.

When I read the man page of read() it does not indicate that the function can return a larger amount of data than expected, so this behavior does not seem to be standards compliant.

This link indicates that the behavior is against POSIX rules

https://unix.stackexchange.com/questions/641003/can-read-return-value-exceed-the-count-value

Can you confirm this bug Nick Saulnier?

Best regards,

Jonathan

Jonathan K said:

When I read the man page of read() it does not indicate that the function can return a larger amount of data than expected, so this behavior does not seem to be standards compliant.

That is an interesting finding, we have to double check this for sure.

Please note that Nick is currently out of the office. Please allow a couple of days for a response. Thanks.

Nick Saulnier, Andreas Dannenberg has anyone had time to look at my questions and findings?

Originally this message had a description of an issue with poll(), but that was due to the PRU binary not being updated properly, so it can be disregarded.

However I have again confirmed that read() does return the number of bytes in the buffer and does not respect the limiting number of bytes used as argument.

Best regards,

Jonathan

Hello Jonathan,

Apologies for the delayed responses here. Going from the first question down:

1) Linux RPMsg only uses a fixed buffer size of 512 bytes, where 16 bytes of that is a header used by the driver. So each RPMsg message can send 496 bytes of information.

2) RPMsg uses VIRTIO buffers under the hood to pass messages between the cores. You can find more information in the Linux SDK docs: https://software-dl.ti.com/processor-sdk-linux/esd/AM437X/08_02_00_24/exports/docs/linux/Foundational_Components/PRU-ICSS/Linux_Drivers/RPMsg.html 

3) These VIRTIO buffers are a fixed size (512 bytes). So if you try to send more than 496 bytes of data, I would expect that any data after the 496th byte would just get dropped.

4) If you are concerned about buffer size, I would suggest just using 496 byte buffers. My understanding is that when you read from a buffer, the driver then frees up the buffer to be used later in time. So you should expect that when you are performing a read, you are reading ALL the data in that RPMsg packet, because the next read should use the next VIRTIO buffer.

Regards,

Nick

Thank you Nick Saulnier for the reply!

This is what I guessed so in the mean time I've implemented the code this way. But in my comment about a bug i found that the driver read() call does not comply with POSIX as all data is returned ever if you limit the number of bytes with the nbyte argument. This is a bug and can cause read() to write outside of allocated data, as well as not being compliant with POSIX. See the POSIX specification for read(), search for "This number shall never be greater than nbyte".

The POSIX specification for write() also specifies how write shall behave when you write more data than the file descriptor can take at the moment. Since we open the file as Non blocking write() should be able to handle a larger amount by writing what it can on each call, search for "write() shall write what it can and return the number of bytes written" in the standard.

We have a lot of code that depends on those two parts of the POSIX standard. Since I wanted to add this file descriptor to our central non blocking poll() driven code but it does not comply with POSIX I've had to write a lot of handling code specifically for this file descriptor to make it work with the rest of the POSIX compliant calls. It would have been much simpler and easier if this driver behaved as the other calls to read() and write().

Hope that this can be fixed! The read() issue could potentially be dangerous since it can write outside allocated memory.

Best regards,

Jonathan K

Hello Jonathan,

I will pass your feedback on to the developers. Please ping the thread if I have not provided a response within a few business days.

Regards,

Nick

Hi Nick Saulnier,

Have the developers had time to look at this issue?

Best regards,

Jonathan

Hello Jonathan,

I am checking with the developer again, thank you for pinging me.

Regards,

Nick

Hello Jonathan,

The developer is working under a near-term deadline, so I have scheduled a followup meeting with them for Friday May 12 to make sure your query gets attention. Please ping the thread if I have not posted something here by Monday May 15.

Regards,

Nick

Hello Jonathan,

Do you have any sample userspace code snippets that the developer could run to replicate your observations?

Regards,

Nick

Hi Nick,

I think I have described it quite clearly how to reproduce the read issue in my earlier comment: https://e2e.ti.com/support/processors-group/processors/f/processors-forum/1200470/am4376-rpmsg-to-pru-message-sequence-limitations/4557722#4557722

But including code to reproduce the issue below. If you test that with your example PRU echo project or something else that sends more than 4 bytes then you will trigger the issue.

I have not tried to send more than 496 bytes with write due to your comment that it would not work, so my assumption of write not complying with POSIX is based on your reply. That also needs to be fixed but is not a memory bug as the read issue.

Code to reproduce:

/*
 * DOC: Test rpmsg to and from a PRU
 * Assumes it replies something back.
 */
#include <string>
#include <stdint.h>
#include <cstdio>
#include <sys/poll.h>
#include <unistd.h>
#include <fcntl.h>
#include <errno.h>
#include <string.h>
#include <poll.h>

using namespace std;

#define PRU_REMOTEPROC "/sys/class/remoteproc/remoteproc3/state"
#define PRU_DEVICE "/dev/rpmsg_pru30"
#define BUFFER_SIZE (512)
char read_buffer[BUFFER_SIZE];


int main(int argc, char **argv)
{
    struct pollfd pfd_rpmsg;
    struct pollfd pfd_remoteproc;

    // Open the remoteproc file:
    pfd_remoteproc.fd = open(PRU_REMOTEPROC, O_RDWR);
    if (0 > pfd_remoteproc.fd)
    {
        printf("Failed to open %s. %s\n", PRU_REMOTEPROC, strerror(errno));
        return -1;
    }
    printf("Opened %s, blocking\n", PRU_REMOTEPROC);

    // Start the PRU by sending "start" to the remoteproc file.
    printf("Start the PRU.\n");
    if (0 > write(pfd_remoteproc.fd, "start", 5))
    {
        printf("Failed to write! %s\n", strerror(errno));
        return -1;
    }

    // Sleep a bit to let the device start:
    sleep(2);

    printf("Connect to the PRU using rpmsg\n");
    // Open file /dev/rpmsg_pru30:
    pfd_rpmsg.fd = open(PRU_DEVICE, O_RDWR | O_NONBLOCK);
    if (0 > pfd_rpmsg.fd)
    {
        printf("Failed to open %s. %s\n", PRU_DEVICE, strerror(errno));
        return -1;
    }

    printf("Opened %s, non-blocking\n", PRU_DEVICE);

    printf("Call poll() to wait for when the file is ready to be written to.\n");
    pfd_rpmsg.events = POLLOUT | POLLIN;
    if (0  > poll(&pfd_rpmsg, 1, -1))
    {
        printf("Error from poll 1: %s\n", strerror(errno));
    }

    // Write banankana and see what we get back!
    printf("Write 'banankaka!' to the PRU.\n");
    if (0 > write(pfd_rpmsg.fd, "banankaka!", 11))
    {
        printf("Failed to write! %s\n", strerror(errno));
        return -1;
    }

    printf("Call poll() to wait for when the file has data.\n");
    pfd_rpmsg.events = POLLIN;
    if (0  > poll(&pfd_rpmsg, 1, -1))
    {
        printf("Error from poll 2: %s\n", strerror(errno));
    }

    // Try to read from the PRU:
    // Note that we specify 4 bytes but will read whatever the PRU sent!
    if (0 < read(pfd_rpmsg.fd, read_buffer, 4))
    {
        printf("Received back: %s\n", read_buffer);
    }
    else
    {
        printf("Failed to read! %s", strerror(errno));
    }

    // Stop the PRU by sending "stop" to the remoteproc file.
    printf("Stop the PRU.\n");
    if (0 > write(pfd_remoteproc.fd, "stop", 4))
    {
        printf("Failed to write! %s\n", strerror(errno));
        return -1;
    }

    close(pfd_rpmsg.fd);
    close(pfd_remoteproc.fd);

    return 0;
}

Best regards,

Jonathan

Hello Jonathan,

Thank you for sending. I have passed your code along to the developer to test. Please ping the thread if I have not provided an update by Thursday or Friday.

Regards,

Nick

Hello Jonathan,

Partial update: I sat down with the developer to identify the part of the kernel driver we want to add print statements to. If there are issues with read() commands to RPMsg, it looks like it would be an issue with the upstream RPMsg framework, not specifically the TI implementation of the RPMsg framework.

He said he would try to run tests sometime over the next couple of days. Please ping the thread if I have not provided another update by sometime early next week.

Regards,

Nick

Hi Nick Saulnier,

Have you heard anything from the developer regarding this?

Best regards,

Jonathan

Hello Jonathan,

I have pinged the developer again asking for feedback.

Regards,

Nick

Hello Jonathan,

Ok, it looks like this behavior is generic to the Linux RPMsg framework, not the TI-specific driver implementation. The developer was able to replicate your observations with Linux <--> R5F.

I am still talking with the developer about next steps. I am not sure if he will do anything other than clarifying the comment in this example here:
https://git.ti.com/cgit/rpmsg/ti-rpmsg-char/tree/examples/rpmsg_char_simple.c#n77

Regards,

Nick