-
Notifications
You must be signed in to change notification settings - Fork 50
Fix build error on Linux 6.11: update virtio_find_vqs() to use virtqueue_info #74
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
Conversation
|
result: |
jserv
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indent with clang-format version 18.
|
The proposed change breaks the build on Linux v6.8: You must ensure the changes are compatible with older Linux versions. |
jserv
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should check the git history of the Linux kernel to locate the exact commit hash that indicates the interface changes to virtio_find_vqs(), so that the commit messages can be more informative.
Example: sysprog21/simplefs@357a813
jychen0611
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refine the assignment of ctx. It is a bool, so avoid using NULL. Use true or false depending on whether extra context is required.
vwifi.c
Outdated
| vqs_info[i].callback = callbacks[i]; | ||
| vqs_info[i].name = names[i]; | ||
| vqs_info[i].ctx = NULL; // optional | ||
| vqs_info[i].ctx = NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ctx is a bool, so setting it to NULL is inappropriate. Please set it explicitly to true or false depending on whether this virtqueue requires maintaining an extra context.
If unsure, check whether the vwifi logic relies on extra context per virtqueue. If context is not needed, use false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing this out. I checked the definition of virtio_find_vqs() in Linux v6.8.12, which passes NULL as the ctx parameter internally.
Our project originally used virtio_find_vqs() instead of the _ctx variant, so I’ve conservatively set ctx = false for all virtqueues to match the original behavior. Since the non-_ctx version does not support passing context flags, switching to virtio_find_vqs_ctx() would be required if any virtqueue truly needs extra context. Given that the original code used the non-_ctx version, I assume that additional context is not needed.
jserv
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use git rebase -i to squash and rework commits. Then, append Close #73 at the end of git commit messages.
652f115 to
7c4fd63
Compare
jserv
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid using backticks in commit subjects. Backticks can be easily confused with single quotes on some terminals, reducing readability. Plain text or single quotes provide sufficient clarity and emphasis.
| for (int i = 0; i < VWIFI_NUM_VQS; i++) { | ||
| vqs_info[i].callback = callbacks[i]; | ||
| vqs_info[i].name = names[i]; | ||
| vqs_info[i].ctx = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a brief comment for .ctx = false, following the project's comment style, to clarify its intended usage for future maintainers.
jychen0611
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tested the virtio-related feature after your modification?
Thanks for the reminder. I initially thought it worked, but after rechecking, I found the virtio feature still has issues. I’ll follow up with a fix soon. |
fab7de1 to
8a2d9c4
Compare
|
This PR addresses API changes introduced in Linux kernel v6.11. During the process, I also noticed additional modifications required for compatibility with v6.15. Please let me know if anything else needs to be adjusted here. Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid using backticks ( ` ) in commit subjects. Backticks can be easily confused with single quotes on some terminals, reducing readability. Plain text or single quotes provide sufficient clarity and emphasis.
Create a new issue to address v6.15 compatibility. Let's concentrate on Linux v6.11 here. |
jserv
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider the following git commit messages:
Support Linux v6.11
Update virtqueue setup to use the new virtio_find_vqs() interface
introduced in Linux commit c502eb8 ("virtio: Introduce struct
virtqueue_info and find_vqs_info() config op"), which now takes an array
of struct virtqueue_info instead of separate parameter arrays.
The legacy virtio_find_vqs() interface was replaced in Linux commit
6c85d6b ("virtio: Rename virtio_find_vqs_info() to virtio_find_vqs")
with a new implementation using struct virtqueue_info.
Since the original code relied on virtio_find_vqs() with implicit NULL
context, the ctx field is set to false to maintain equivalent behavior.
jserv
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refine git commit messages per requested.
Update virtqueue setup to use the new virtio_find_vqs() interface
introduced in Linux commit c502eb8 ("virtio: Introduce struct
virtqueue_info and find_vqs_info() config op"), which now takes an array
of struct virtqueue_info instead of separate parameter arrays.
The legacy virtio_find_vqs() interface was replaced in Linux commit
6c85d6b ("virtio: Rename virtio_find_vqs_info() to virtio_find_vqs")
with a new implementation using struct virtqueue_info.
Since the original code relied on virtio_find_vqs() with implicit NULL
context, the ctx field is set to false to maintain equivalent behavior.
8a2d9c4 to
800ad32
Compare
|
Thank @horseface1110 for contributing! |
This patch updates the call to
virtio_find_vqs()to follow the new signature introduced in Linux 6.11, which replaces the separatecallbacksandnamesarrays with astruct virtqueue_info[].Tested on:
vwifi.kobuilds and loads successfullyThis resolves #73.