Skip to content

Conversation

@ikegami-t
Copy link
Contributor

The index is required by the OCP specification.

@ikegami-t ikegami-t mentioned this pull request Nov 24, 2024
@YUEyuan310
Copy link

The desc in log has not been allocated memory, and there should be special handling within the nvme_get_log_simple function. Directly using nvme_get_log_page may cause issues.

@ikegami-t
Copy link
Contributor Author

The desc is not used at the first nvme_get_log_page calling but passed to use the function parameter log instead as below so no need to allocate the memory for the call. Yes at first I also considered to change the function nvme_get_log_simple itself but seems better to keep the current API function behavior for other commands and callers. By the way is there any error caused actually by the changes? Thank you.

                .timeout = NVME_DEFAULT_IOCTL_TIMEOUT,
                .lid = LID_HWCOMP,
                .nsid = NVME_NSID_ALL,
+               .log = log,
+               .len = desc_offset,
        };

@YUEyuan310
Copy link

The desc is not used at the first nvme_get_log_page calling but passed to use the function parameter log instead as below so no need to allocate the memory for the call. Yes at first I also considered to change the function nvme_get_log_simple itself but seems better to keep the current API function behavior for other commands and callers. By the way is there any error caused actually by the changes? Thank you.

                .timeout = NVME_DEFAULT_IOCTL_TIMEOUT,
                .lid = LID_HWCOMP,
                .nsid = NVME_NSID_ALL,
+               .log = log,
+               .len = desc_offset,
        };

According to my previous NVMe feature code, using the modified CLI, the attempt to retrieve content from the hardware component failed.Modifying nvme_get_log_simple to accept uuidx should make it compatible with other calls, defaulting to 0 should be fine.

The index is required by the OCP specification.

Signed-off-by: Tokunori Ikegami <[email protected]>
@ikegami-t
Copy link
Contributor Author

Thank you so much for your confirmation and also very sorry for the error. Just confirmed the cause and fixed the error so could you please retry the changes?

@YUEyuan310
Copy link

Thank you so much for your confirmation and also very sorry for the error. Just confirmed the cause and fixed the error so could you please retry the changes?

I have verified your changes and can correctly obtain the logs.

@ikegami-t
Copy link
Contributor Author

Thank you so much for your verification!

@igaw igaw merged commit 60214cc into linux-nvme:master Dec 2, 2024
17 checks passed
@igaw
Copy link
Collaborator

igaw commented Dec 2, 2024

Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants