Remove layer breaking functions from rpmsg_virtio #561
Conversation
There was a problem hiding this comment.
rpmsg_virtio_create_virtqueues and rpmsg_virtio_delete_virtqueues and others have to be tagged as deprecated. and we need the agrements of at least main contributors before starting the deprecation process.
Please split in 2 commits, one to update the rpmsg_virtio.c and one dedicated to the API
These are only used internally by rpmsg_virtio. They also already exist as part of the virtio API. Do not create an extra wrapper API out of these functions. Deprecate these functions. Signed-off-by: Andrew Davis <[email protected]>
These are only used internally by rpmsg_virtio. They also already exist as part of the virtio API. Do not create an extra wrapper API out of these functions. Use the virtio functions directly in rpmsg_virtio.c. Signed-off-by: Andrew Davis <[email protected]>
Do not expose new functions that reach into structs not belonging to rpmsg_virtio, that breaks layering. Functions to access these members of struct virtio are already provided by virtio, use those instead. Deprecate the unneeded and layer breaking rpmsg_virtio functions for the same. Signed-off-by: Andrew Davis <[email protected]>
lib/rpmsg/rpmsg_virtio.c
Outdated
| @@ -277,10 +277,10 @@ static int rpmsg_virtio_wait_remote_ready(struct rpmsg_virtio_device *rvdev) | |||
| uint8_t status; | |||
|
|
|||
| while (1) { | |||
| status = rpmsg_virtio_get_status(rvdev); | |||
| virtio_get_status(rvdev->vdev, &status); | |||
There was a problem hiding this comment.
if virtio_get_status can return an error this leads to an infinite loop.
we need in such case to propagate the error
There was a problem hiding this comment.
Yes we should, but not checking for error was already the case before.
We actually shouldn't need this function at all as waiting for this flag is already done at this point by rproc_virtio_wait_remote_ready() which is the same function. I am going to remove this function in a patch I will send after this is taken. At that point I will add checks and propagate the error up the stack. One fix at a time 😄
There was a problem hiding this comment.
Yes we should, but not checking for error was already the case before.
The difference is that before rproc_virtio_get_status returns only the status, virtio_get_status test the input parameters
We actually shouldn't need this function at all as waiting for this flag is already done at this point by
rproc_virtio_wait_remote_ready()which is the same function. I am going to remove this function in a patch I will send after this is taken. At that point I will add checks and propagate the error up the stack. One fix at a time 😄
The RPMsg and remoteproc_virtio are two layers that can be used independently. remoteproc_virtio is not necessarily used; some projects define their own transport layer and use this function to synchronize the communication. Therefore, this one should be kept.
There was a problem hiding this comment.
This check belongs to the rproc_virtio layer. There are two function that do the same thing currently (rproc_virtio_wait_remote_ready() and rpmsg_virtio_wait_remote_ready()) what I plan to do is merge them into one that lives in the rproc_virtio layer. Then any layer that uses rproc_virtio can make use of that. This handshake check has nothing to do with RPMSG, so rpmsg_virtio_wait_remote_ready() does not make sense nor is it needed, this check has already been done by the rproc_virtio layer.
There was a problem hiding this comment.
rpmsg_virtio_wait_remote_ready name is probably not good. the name should be something such as rpmsg_virtio_wait_ready.
There are two functions because RPMSG layers and RPROC virtio backend are independent and must be kept independant .
here is an exemple of projet that uses rpmsg but not rproc_virtio:
https://elixir.bootlin.com/linux/latest/source/drivers/virtio/virtio_ring.c
If you want to merge, the service should be implemented in the virtio layer. But does it makes sense to add a virtio API for that? I would prefer not to.
There was a problem hiding this comment.
I think we can figure that out later then, for now to keep this PR moving I'll just make the status check fixes as suggested.
These functions are deprecated, use their replacements. Signed-off-by: Andrew Davis <[email protected]>
If we cannot finish waiting for the remote to be ready to start RPMsg communication then return and propagate an error. Signed-off-by: Andrew Davis <[email protected]>
These functions are only used internally by rpmsg_virtio but are exposed in rpmsg_virtio.h which is included by users of the OpenAMP API. These functions reach into virtio structs which is a layering issue. They also have equivalents in the virtio API which should be used instead for this access. Remove these functions from the header before they get used by someone and turn into API.