After reviewing the code for adding the cdw12 parameter to nvme_set_feature(), I believe it is safe to drop the parameter.
Analysis: 54f5e4a Add support for decoding IO Determinism features @@ -477,13 +478,13 @@ int nvme_feature(int fd, __u8 opcode, __u32 nsid, __u32 cdw10, __u32 cdw11, return err; } -int nvme_set_feature(int fd, __u32 nsid, __u8 fid, __u32 value, bool save, - __u32 data_len, void *data, __u32 *result) +int nvme_set_feature(int fd, __u32 nsid, __u8 fid, __u32 value, __u32 cdw12, + bool save, __u32 data_len, void *data, __u32 *result) { __u32 cdw10 = fid | (save ? 1 << 31 : 0); return nvme_feature(fd, nvme_admin_set_features, nsid, cdw10, value, - data_len, data, result); + cdw12, data_len, data, result); } The cdw12 parameter is added, and passed straight through to nvme_feature(). >From there, all that is done with it is to set a field in a struct: @@ -459,13 +459,14 @@ int nvme_sanitize_log(int fd, struct nvme_sanitize_log_page *sanitize_log) } int nvme_feature(int fd, __u8 opcode, __u32 nsid, __u32 cdw10, __u32 cdw11, - __u32 data_len, void *data, __u32 *result) + __u32 cdw12, __u32 data_len, void *data, __u32 *result) { struct nvme_admin_cmd cmd = { .opcode = opcode, .nsid = nsid, .cdw10 = cdw10, .cdw11 = cdw11, + .cdw11 = cdw12, .addr = (__u64)(uintptr_t) data, .data_len = data_len, }; Is it just me, or is .cdw11 set twice? Is this a mistake? It is! Let's see commit 2d8627ad3eff1afb11d0cad533ce53d019014eea Author: Muhammad Ahmad <muhammad.ah...@seagate.com> Date: Fri Mar 2 00:07:57 2018 -0600 Fixed a bug where cdw11 was being overwritten by cdw12 diff --git a/nvme-ioctl.c b/nvme-ioctl.c index 5166446..e8ba935 100644 --- a/nvme-ioctl.c +++ b/nvme-ioctl.c @@ -472,7 +472,7 @@ int nvme_feature(int fd, __u8 opcode, __u32 nsid, __u32 cdw10, __u32 cdw11, .nsid = nsid, .cdw10 = cdw10, .cdw11 = cdw11, - .cdw11 = cdw12, + .cdw12 = cdw12, .addr = (__u64)(uintptr_t) data, .data_len = data_len, }; Looking at clear_correctable_errors(), we see that cdw12 is set to 0: + const __u32 cdw12 = 0; + const bool save = false; + __u32 result; + err = nvme_set_feature(fd, namespace_id, feature_id, value, cdw12, save, + 0, NULL, &result); The cdw12 struct member is not used at all in nvme-cli 1.5, the version in bionic, and "54f5e4a Add support for decoding IO Determinism features" does nothing with the variable. I believe it is safe to drop cdw12 from nvme_set_feature(). Note, Dann, your patch is wrong, since you drop the wrong parameter. You are dropping data_len instead of cdw12. The correct patch is: diff --git a/toshiba-nvme.c b/toshiba-nvme.c index a3b6c13..0694b8b 100644 --- a/toshiba-nvme.c +++ b/toshiba-nvme.c @@ -626,7 +626,7 @@ static int clear_correctable_errors(int argc, char **argv, struct command *cmd, const __u32 cdw12 = 0; const bool save = false; __u32 result; - err = nvme_set_feature(fd, namespace_id, feature_id, value, cdw12, save, + err = nvme_set_feature(fd, namespace_id, feature_id, value, save, 0, NULL, &result); if (err) { fprintf(stderr, "%s: couldn't clear PCIe correctable errors \n", __func__); I think we can leave the variable definition "const __u32 cdw12 = 0;" there since it doesn't harm anything. I also agree with you to pull in "5e9239e Fixes for log page access". ** Changed in: nvme-cli (Ubuntu Bionic) Importance: Undecided => Medium ** Changed in: nvme-cli (Ubuntu Bionic) Assignee: (unassigned) => Matthew Ruffell (mruffell) ** Changed in: nvme-cli (Ubuntu Bionic) Status: Confirmed => In Progress -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/1866897 Title: Add toshiba plugin support To manage notifications about this bug go to: https://bugs.launchpad.net/ubuntu/+source/nvme-cli/+bug/1866897/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs