-
Notifications
You must be signed in to change notification settings - Fork 697
libnvme: use 64-bit version struct nvme_passtru_cmd everywhere #2963
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
d652774 to
971bd53
Compare
f6a6baa to
78d9bf0
Compare
The linux commit 65e68edce0db ("nvme: allow 64-bit results in passthru
commands") introduced the 64-bit version of the passthru. Reduce the
complexity of the library by just offering one passthru data structure.
Signed-off-by: Daniel Wagner <[email protected]>
842b1c8 to
0eef6c4
Compare
Don't expose the nvme_submit_passthru completely. Instead just provide hooks for printing or deciding if a command needs to be retried. This is similar to the MI interface. Signed-off-by: Daniel Wagner <[email protected]>
The command submit API doesn't need to return explicitly the return value, it's already in struct nvme_passthru_cmd. Signed-off-by: Daniel Wagner <[email protected]>
|
FWIW, I am not totally happy where all the code is. I've placed the code where it currently fits (e.g. logging.c has the decide logic which is a bit wrong IMO). I'd say we move the code around later. |
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.
Pull request overview
This PR unifies the NVMe passthrough command interface by using a single 64-bit version of nvme_passthru_cmd throughout the library, eliminating the complexity of having both 32-bit and 64-bit variants. The change updates the result field from 32-bit to 64-bit and modifies the API to return the result through the command structure instead of an output parameter.
Key Changes:
- Unified passthrough command structure to support 64-bit results everywhere
- Removed
resultparameter from submission functions - results now accessed viacmd.result - Added fallback support for older kernels that only support 32-bit ioctl interface
- Updated format strings and type declarations for 64-bit result handling
Reviewed changes
Copilot reviewed 66 out of 67 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| types.h | Changed result field type from __u32 * to __u64 * in nvme_get_log_args |
| plugins/zns/zns.c | Removed result parameters, changed to access cmd.result directly, updated format strings |
| plugins/wdc/wdc-nvme.c | Updated result variable types to __u64, removed NULL result parameters |
| plugins/transcend/transcend-nvme.c | Removed NULL result parameter from passthru calls |
| plugins/toshiba/toshiba-nvme.c | Simplified nvme_sct_op by using passthru_cmd structure, updated result types |
| plugins/solidigm/*.c | Removed NULL result parameters from nvme_get_log calls across multiple files |
| plugins/shannon/shannon-nvme.c | Changed result variable types from __u32 to __u64 |
| plugins/seagate/seagate-nvme.c | Updated result types and removed NULL parameters |
| plugins/scaleflux/sfx-nvme.c | Changed result types and removed NULL parameters |
| plugins/sandisk/*.c | Updated result types and removed NULL parameters |
| plugins/ocp/*.c | Updated result types, format strings, and removed NULL parameters |
| plugins/netapp/netapp-nvme.c | Removed NULL result parameter |
| plugins/micron/micron-nvme.c | Updated result types and removed NULL parameters |
| plugins/memblaze/memblaze-nvme.c | Updated result types and format strings |
| plugins/lm/lm-nvme.c | Updated result access and removed result parameters |
| plugins/intel/intel-nvme.c | Updated result types |
| plugins/innogrit/innogrit-nvme.c | Updated result parameter types |
| plugins/feat/feat-nvme.c | Updated result types throughout |
| plugins/fdp/fdp.c | Updated result variable type |
| plugins/dera/dera-nvme.c | Removed NULL result parameter |
| plugins/amzn/amzn-nvme.c | Removed NULL result parameter |
| nvme.h | Removed NULL result parameter |
| nvme.c | Major refactoring to use cmd.result, updated format strings, restructured passthru calls |
| nvme-rpmb.c | Removed NULL result parameters |
| nvme-print*.c | Updated function signatures for 64-bit result handling |
| logging.c | Refactored submission hooks with entry/exit callbacks |
| libnvme/src/nvme/linux.c | Added ioctl64 detection and new callback infrastructure |
| libnvme/src/nvme/private.h | Added Linux kernel passthru structures and new hook definitions |
| libnvme/test/*.c | Updated all test code to use new API |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Looks great! Should we have a CI step that tests the backwards compatibility? |
|
Most of existing unit tests use the 32bit variant, a few use the 64bit version. So I think we covering all bases already. So no, I think we are good. Let's see if we get some nasty bug reports, then we can still think about it :) |
The linux commit 65e68edce0db ("nvme: allow 64-bit results in passthru commands") introduced the 64-bit version of the passthru. Reduce the complexity of the library by just offering one passthru data structure.
Obviously, this increased the minimum kernel dependency to v5.4 release in November 2019.Fixes: nvme-experiments#89
TODO: