plugin/ocp_get_feature_fid_c8h:Added the OCP Get Feature FID=C8h comm… by VigneshwaranSaravana · Pull Request #2788 · linux-nvme/nvme-cli

VigneshwaranSaravana · 2025-04-30T07:23:01Z

…and api

Enabled Get Feature command (FID=C8h) api with sel, namespace-id, no-uuid command line arguments. namespace-id added to the test the command with active, inactive and invalid nsid values.

Reviewed-by: Karthik Balan karthik.b82@samsung.com
Reviewed-by: Arunpandian J arun.j@samsung.com

ikegami-t

Is the namespace-id option really required? Since it looks the `nvme get-feature' command can be used for the testing purpose.
For the commit message seems needed to be followed the following guide line.
https://docs.kernel.org/process/submitting-patches.html#subject-line

ikegami-t · 2025-05-04T06:41:32Z

completions/bash-nvme-completion.sh

 		opts+=" --comp-id= -i --list -l --verbose -v \
 			--output-format -o --timeout= -t"
 			;;
+		"get-latency-monitor")


This command not related to the PR changes.

@ikegami-t - Incorporated the review comments

ikegami-t · 2025-05-04T06:52:05Z

plugins/ocp/ocp-nvme.c

+
+	err = nvme_get_features(&args);
+	if (!err) {
+		printf("get-feature:0xC8 %s value: %#08x\n",


Can you add support for human-readable and json print outputs? (I think it is okay to be done in future separately.)

@ikegami-t - Printing the libnvme api return value. I am thinking json format is not required

About the human-readable I thought the value can be decoded but reconfirmed as the dword 0 value is always zero so understood as not required the both human-readable and json print outputs.

ikegami-t · 2025-05-04T06:57:56Z

plugins/ocp/ocp-nvme.c

 }

+static int ocp_get_telemetry_profile_feature(int argc, char **argv, struct command *cmd,
+					      struct plugin *plugin)


Indentation error should be fixed as below. (5 tab spaces and 5 white spaces needed before struct plugin *plugin)

static int ocp_get_telemetry_profile_feature(int argc, char **argv, struct command *cmd, - struct plugin *plugin) + struct plugin *plugin)

@ikegami-t - Incorporated the review comments

Looks not changed the indentation error part.

igaw · 2025-05-07T17:07:10Z

Besides addressing @ikegami-t comment, a rebase is necessary. Thanks.

VigneshwaranSaravana · 2025-05-09T09:43:25Z

Is the namespace-id option really required? Since it looks the `nvme get-feature' command can be used for the testing purpose.

For the commit message seems needed to be followed the following guide line.
https://docs.kernel.org/process/submitting-patches.html#subject-line

@ikegami-t - We have added the get feature FID=C8h command api to test the command with various NSID value and also ocp-nvme.c library should have set feature and get feature command api for all the FIDs

VigneshwaranSaravana · 2025-05-09T09:46:40Z

Besides addressing @ikegami-t comment, a rebase is necessary. Thanks.

@igaw - Resolved the conflict issue.

ikegami-t · 2025-05-09T14:55:59Z

@ikegami-t - We have added the get feature FID=C8h command api to test the command with various NSID value and also ocp-nvme.c library should have set feature and get feature command api for all the FIDs

Yes I see but the command added will not be used actually I think.

igaw · 2025-05-15T07:45:43Z

What is the status here? Is it ready to be merged or are there open question/request?

ikegami-t · 2025-05-15T15:49:56Z

@igaw The implemetation itself looks okay. (But for me the command seems not usuful since described by the OCP specification as no useful information cleary then not issued. Also this issue is not only for this command but same for the commands already applied the PR: #2785 and #2768.)

igaw · 2025-05-16T06:39:18Z

I am not really deep into the OCP spec, so I can't really comment if this is usual or not.

At least for Samsung it seems useful. Do you have a suggestion how to move forward?

FWIW, we have also added stuff from the nvme spec which doesn't really seem useful but still we have it in the nvme-cli. I'd say as long the code fits into the exiting code base and doesn't impose long time maintainability issues we should accept it.

ikegami-t · 2025-05-16T13:54:28Z

@igaw Thanks for your comment. Yes agreed for now and just ~~thoght~~ thought as in future if there is really any unnecessary command we can delete the command as depreciated by the major version up. Just FYI: By the way just remember before the PR: #596 to add the abort command was rejected as unuseful command. This PR command also looks can be used the admin-passthru command instead since no command specific feature data parse done by the command.

igaw · 2025-05-19T12:11:44Z

Regarding your PR:` #596

The reason why this was not accepted is that this particular command really messes with the transport layer a lot and thus userspace shouldn't really touch these bits. If you really want to add it, we could hide it behind something 'do you really want to do this?' as we have for the format command.

Of course there are more such low level commands which could get the transport out of sync and as you said, it's possible to it with the admin-passthru anyway. nvme-cli has different sets of commands, the plumbing ones like admin-passthru and more user friendly ones. The user friendly ones should at least inform the user when the issued command is dangerous.

Okay, I'll going to merge this now. I understand this one is more like a plumbing command.

Enabled Get Feature command (FID=C8h) api with sel, namespace-id, no-uuid command line arguments. namespace-id added to the test the command with active, inactive and invalid nsid values. Reviewed-by: Karthik Balan <karthik.b82@samsung.com> Reviewed-by: Arunpandian J <arun.j@samsung.com> Signed-off-by: Vigneshwaran Saravanan <s.vignesh@samsung.com>

igaw · 2025-05-19T12:21:30Z

Thanks!

ikegami-t · 2025-05-19T13:22:13Z

Thanks for your explanation. I could unserstand well.

ikegami-t reviewed May 4, 2025

View reviewed changes

ikegami-t mentioned this pull request May 8, 2025

plugin/ocp_get_feature_fid_c3h:Added the OCP Get Feature FID=C3h comm… #2785

Merged

VigneshwaranSaravana force-pushed the plugin/ocp_get_feature_fid_C8h branch from 415e7b5 to aa4d473 Compare May 9, 2025 09:38

igaw force-pushed the plugin/ocp_get_feature_fid_C8h branch from aa4d473 to d421e85 Compare May 19, 2025 12:13

igaw merged commit 5c6d7e7 into linux-nvme:master May 19, 2025
18 checks passed

Conversation

VigneshwaranSaravana commented Apr 30, 2025

Uh oh!

ikegami-t left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igaw commented May 7, 2025

Uh oh!

VigneshwaranSaravana commented May 9, 2025

Uh oh!

VigneshwaranSaravana commented May 9, 2025

Uh oh!

ikegami-t commented May 9, 2025

Uh oh!

igaw commented May 15, 2025

Uh oh!

ikegami-t commented May 15, 2025

Uh oh!

igaw commented May 16, 2025

Uh oh!

ikegami-t commented May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

igaw commented May 19, 2025

Uh oh!

Uh oh!

igaw commented May 19, 2025

Uh oh!

ikegami-t commented May 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ikegami-t commented May 16, 2025 •

edited

Loading