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

Unbind rpmsg dev #45

Merged
merged 1 commit into from
Aug 18, 2024
Merged

Unbind rpmsg dev #45

merged 1 commit into from
Aug 18, 2024

Conversation

tnmysh
Copy link
Collaborator

@tnmysh tnmysh commented Jun 28, 2024

Fix linux apps and unbind rpmsg device with driver after rpmsg communication is done.

@tnmysh tnmysh force-pushed the unbind_rpmsg_dev branch from a364e30 to a5dd673 Compare June 28, 2024 03:57
@tnmysh tnmysh requested review from edmooring and arnopo June 28, 2024 18:13
@tnmysh tnmysh self-assigned this Jul 1, 2024
@tnmysh tnmysh marked this pull request as ready for review July 1, 2024 16:29
@arnopo
Copy link
Collaborator

arnopo commented Jul 2, 2024

hi @tnmysh: do you tried to use RPMSG_DESTROY_EPT_IOCTL instead?

@tnmysh
Copy link
Collaborator Author

tnmysh commented Jul 2, 2024

hi @tnmysh: do you tried to use RPMSG_DESTROY_EPT_IOCTL instead?

Actually, I can do that in separate PR, where binding and unbinding both are taken care via IOCTLs, instead of writing to sysfs nodes. For now this PR is only fixing existing workflow.

@arnopo
Copy link
Collaborator

arnopo commented Jul 2, 2024

hi @tnmysh: do you tried to use RPMSG_DESTROY_EPT_IOCTL instead?

Actually, I can do that in separate PR, where binding and unbinding both are taken care via IOCTLs, instead of writing to sysfs nodes. For now this PR is only fixing existing workflow.

you are right, the bind is implemented in current exemples, so the unbind make sense

@tnmysh tnmysh force-pushed the unbind_rpmsg_dev branch from a5dd673 to 3896365 Compare July 3, 2024 17:37
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

@wmamills
Copy link
Collaborator

As discussed in the rpmsg call today, the demos should work if the correct driver is already bound and in this case at least the driver should not be unbound.

If the driver DOES need to be bound, it is debatable if it should be unbound on exit or just leave it knowing that the person that needs something else can do the unbind themselves.

@tnmysh tnmysh force-pushed the unbind_rpmsg_dev branch from 3896365 to 690b6ab Compare July 17, 2024 23:58
@tnmysh tnmysh requested a review from arnopo July 18, 2024 00:01
@tnmysh
Copy link
Collaborator Author

tnmysh commented Jul 18, 2024

As discussed in the rpmsg call today, the demos should work if the correct driver is already bound and in this case at least the driver should not be unbound.

Done.

examples/linux/common/common.c Outdated Show resolved Hide resolved
examples/linux/common/common.c Show resolved Hide resolved
if (ret < 0)
return ret;

if (!is_rpmsg_chrdev_bind(rpmsg_dev)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The function is_rpmsg_chrdev_bind can return true, false or an error. Seems that the error case should be treated

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ack. Thanks for catching this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also, I think is_rpmsg_chrdev_bind should be moved within bind_rpmsg_chrdev.
As it's common to all the apps.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@tnmysh tnmysh force-pushed the unbind_rpmsg_dev branch from 690b6ab to 610c3ea Compare July 24, 2024 18:50
@tnmysh tnmysh requested a review from arnopo July 24, 2024 18:54
@tnmysh tnmysh force-pushed the unbind_rpmsg_dev branch from 610c3ea to 8faf2e2 Compare July 24, 2024 18:55
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.

with a minor comment

close(fd);
return 0;
} else if (strncmp(drv_override, "(null)", strlen("(null)")) != 0) {
printf("error: device %s is busy, drv bind=%s\n",
Copy link
Collaborator

Choose a reason for hiding this comment

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

fprintf?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done.

@tnmysh tnmysh force-pushed the unbind_rpmsg_dev branch from 8faf2e2 to 8e831df Compare July 25, 2024 15:33
Check rpmsg device driver override and if correct driver
is already bound, then execute rest of application. If driver
is not bound, then bind rpmsg char driver.

Signed-off-by: Tanmay Shah <[email protected]>
@tnmysh tnmysh force-pushed the unbind_rpmsg_dev branch from 8e831df to 65b35ea Compare July 25, 2024 15:34
@tnmysh
Copy link
Collaborator Author

tnmysh commented Jul 29, 2024

@edmooring , @wmamills requesting reviews.

@tnmysh
Copy link
Collaborator Author

tnmysh commented Aug 7, 2024

@wmamills , created follow up issue here: #50

@wmamills
Copy link
Collaborator

wmamills commented Aug 15, 2024

I have tested this with openamp-demo.
I first versified the failure mode:
docker run -it --rm openamp/demo-lite
qemu-zcu102 demo3
(login as root)
./demo3A
echo_test

This 2nd echo test fails. Likewise for demo3B and demo3C.

I then built a new image with openamp-ci-builds including this PR and updated the demo.
With the new image echo_test and mat_mul_demo can run multiple times after their associated demo script.

However ./demo3C followed by proxy_app continues to fail.
I belive this is because the proxy app firmware destroys its endpoint afterward and is not releated to this PR but this needs confirmation from @tnmysh

@tnmysh
Copy link
Collaborator Author

tnmysh commented Aug 15, 2024

@wmamills yes I confirm. There will be separate PR to fix that behavior.

Thanks.

Copy link
Collaborator

@wmamills wmamills left a comment

Choose a reason for hiding this comment

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

I have tested using openamp-demo demo3

@wmamills wmamills merged commit 5d25833 into main Aug 18, 2024
1 check passed
@tnmysh tnmysh deleted the unbind_rpmsg_dev branch August 19, 2024 15:41
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.

3 participants