Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add RPMsg API to get buffer size #565

Merged
merged 3 commits into from
Apr 9, 2024
Merged

Conversation

glneo
Copy link
Contributor

@glneo glneo commented Mar 15, 2024

In patch 3 of this series you can see core of the issue, applications have no way of finding if the transport layer under RPMsg has a maximum supported message size. The examples end up having to look directly into the transport layer to find this info. With this API they can ask the RPMsg layer without having to know about the backing transport layer. This is better API layering and allows the core of the examples to work across any transport layer.

Copy link
Collaborator

@arnopo arnopo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comment else LGTM

lib/rpmsg/rpmsg.c Outdated Show resolved Hide resolved
lib/rpmsg/rpmsg.c Outdated Show resolved Hide resolved
@@ -121,21 +131,12 @@ int app (struct rpmsg_device *rdev, void *priv)

if (!i_payload) {
LPERROR("memory allocation failed.\r\n");
rpmsg_destroy_ept(&lept);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reviewing the code I just see that an error returned rpmsg_send error not return a test failed. t
Could you also fix that by managing a ret variable and using go out?

if (!i_payload) {
		LPERROR("memory allocation failed.\r\n");
		ret = -1;
		goto out;
}
[...]
	if (ret) {
		LPERROR("Failed to create RPMsg endpoint.\r\n");
		ret = -1;
		goto free_mem:
	}
[...]
free_mem:
	    metal_free_memory(i_payload);
out:		
	LPRINTF("**********************************\r\n");
	LPRINTF(" Test Results: Error count = %d \r\n", err_cnt);
	LPRINTF("**********************************\r\n");
	/* Destroy the RPMsg endpoint */
	rpmsg_destroy_ept(&lept);
	LPRINTF("Quitting application .. Echo test end\r\n");
	return ret;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into that and it will introduce more churn than I'd think should be part of this series. Would it be better to address that issue in a separate PR?

Add an RPMsg API to get the buffer sizes supported by the backing
transport layer. Add hooks so that transport layers can register
functions to provide this data.

Signed-off-by: Andrew Davis <[email protected]>
RPMsg provides functions to get a transport's backing buffer sizes.
Connect this up for the virtio transport here.

Signed-off-by: Andrew Davis <[email protected]>
The contents of app() in these examples is given a rpmsg_device. We should
not have to know about the backing transport layer. We assume it is virtio
and call into the virtio layer to get the buffer size. This information is
now available from the rpmsg layer. Use those functions to make the app()
agnostic to the backing layer.

Signed-off-by: Andrew Davis <[email protected]>
Copy link
Collaborator

@arnopo arnopo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@edmooring edmooring left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to go.

@arnopo arnopo merged commit 89a8a30 into OpenAMP:main Apr 9, 2024
3 checks passed
@arnopo arnopo added this to the Release V2024.04 milestone Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants