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 <[email protected]>
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
[email protected]
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
