Comment 3 for bug 1866897

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 address hidden>
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".