plugin/ocp_get_feature_fid_c5h:Added the OCP Get Feature FID=C5h comm… by VigneshwaranSaravana · Pull Request #2768 · linux-nvme/nvme-cli

VigneshwaranSaravana · 2025-04-04T11:50:26Z

…and api

Enabled the Get Feature Command FID=C5h command api with nsid field Values

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

HaroPanosyan · 2025-04-04T15:20:34Z

Maybe missing something here, could someone help me understand why we need NSID for this feature?

For get feature, from OCP 2.5 Spec (similar in 2.6):
A Get Features command for Feature Identifier C5h should not be issued because no useful information is
returned.

For set feature, from OCP 2.5 Spec (similar in 2.6):

The host should either clear this to zero or set this to
FFFFFFFFh. If the host sends a valid NSID value other
than zero or FFFFFFFFh, then the device shall fail the
command with Feature Not Namespace Specific. If
the host sends an invalid NSID value, then the device
shall fail the command with Invalid Field in
Command.

ikegami-t · 2025-04-04T16:49:46Z

Semms for the get feature command the valid NSID value also can be specified. So to validate for the OCP specification compliance seems better to able to set the NSID.

But the get feature command always returns CQ entry Dword 0 as zero so no usuful information as described. The get feature command for this PR is it really only for the OCP specification compliance and required the command?

ikegami-t · 2025-04-04T17:11:29Z

Documentation/nvme-ocp-get-latency-monitor.html not necessary added since generated from Documentation/nvme-ocp-get-latency-monitor.txt.

ikegami-t · 2025-04-04T17:12:54Z

By the way will the set feature command be added separately?

HaroPanosyan · 2025-04-04T17:17:36Z

GETF-4 is General Get/Set Feature Requirement, and the keyword is "may":
In addition, the host may set that field to a valid NSID.

because some features can be NSID specific:

LMSF-6 is specific to C5h, so correct, setting valid NSID would be useful only for testing, i.e. get error:
Feature Not Namespace Specific

This will help for FW validation but not to execute get/set feature from host software perspective. Being a host software, I think it is better to follow LMSF-6:
The host should either clear this to zero or set this to FFFFFFFFh.

But if the scope here is also to help FW validation, then yes, allowing NSID as command line option is right thing to do.

ikegami-t

For this feature command is it not really needed to add the no-uuid option?

ikegami-t · 2025-04-04T17:13:35Z

plugins/ocp/ocp-nvme.c

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


No needed to add this blank line.

ikegami-t · 2025-04-04T17:17:10Z

plugins/ocp/ocp-nvme.c

+	if (err)
+		return err;
+
+


No needed to add 2 blank lines but only 1 blank line okay to added.

@ikegami-t - incorporated the review comments

ikegami-t · 2025-04-04T17:24:20Z

Documentation/nvme-ocp-get-latency-monitor.txt

+OPTIONS
+-------
+
+-n <nsid>::


The --nsid not described and also no description for the options -n and --nsid.

@ikegami-t incorporated the review comments

ikegami-t · 2025-04-04T17:25:34Z

plugins/ocp/ocp-nvme.c

+	};
+
+	OPT_ARGS(opts) = {
+		OPT_BYTE("sel", 'S', &cfg.sel, sel),


Is there any reason for the short option S but it is not s?

@ikegami-t - Kept the capital S as per pervious implementation

Understood. In my opition basically -s should be used at first but if for any other option already used -s then the short option -S should be used instead. But for the OCP commands already mixed to be used both -s and -S for the --sel option.

Changed to small letter s

ikegami-t · 2025-04-07T15:24:26Z

plugins/ocp/ocp-nvme.h

 		ENTRY("set-enable-ieee1667-silo", "enable IEEE1667 silo", set_enable_ieee1667_silo)
 		ENTRY("hardware-component-log", "retrieve hardware component log", hwcomp_log)
+		ENTRY("get-latency-monitor", "Get Latency Monitor Feature"
+		, ocp_get_latency_monitor_feature)


- ENTRY("get-latency-monitor", "Get Latency Monitor Feature" - , ocp_get_latency_monitor_feature) + ENTRY("get-latency-monitor", "Get Latency Monitor Feature", + ocp_get_latency_monitor_feature)

@ikegami-t - Incorporated the review comments

ikegami-t · 2025-04-07T15:26:34Z

plugins/ocp/ocp-nvme.c

 }

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


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

then indented as below.

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

@ikegami-t Incorporated the review comments

ikegami-t · 2025-04-07T15:29:42Z

Documentation/nvme-ocp-get-latency-monitor.txt

+--nsid=<nsid>::
+	NSID: This field specifies Valid, Invalid and
+	Inactive NSID value:
+-S <select>::


Needed a blank line as below.

--nsid=<nsid>:: NSID: This field specifies Valid, Invalid and Inactive NSID value: + -S <select>:: --sel=<select>:: Select (SEL): This field specifies which value of the attributes to return in the provided data:

@ikegami-t Incorporated the review comments

igaw · 2025-04-08T12:16:09Z

Looks good to me. Let's wait for @ikegami-t's review

ikegami-t · 2025-04-08T13:34:11Z

Documentation/nvme-ocp-get-latency-monitor.txt

+SYNOPSIS
+--------
+[verse]
+'nvme ocp get-latency-monitor' <device> [--sel=<select> | -s <select>] [--nsid | -n]


- 'nvme ocp get-latency-monitor' <device> [--sel=<select> | -s <select>] [--nsid | -n] + 'nvme ocp get-latency-monitor' <device> [--sel=<select> | -s <select>] [--nsid=<nsid> | -n <nsid>]

But as metioned by the comment #2768 (comment) looks the NSID parameter not necessary so can you delete the parameter?
Also as metioned by the comment #2768 (review) can you add the 'no-uuid' option as same with other OCP commands.
Also if the NSID paramter really needed could you please change the parameter name to namespace-id' from nsid` as same with other NVMe commands.

@ikegami-t - Incorporated the review comments

ikegami-t · 2025-04-08T13:36:06Z

plugins/ocp/ocp-nvme.c

+static int ocp_get_latency_monitor_feature(int argc, char **argv, struct command *cmd,
+					    struct plugin *plugin)
+{
+	const char *desc = "Define Issue Get Feature command (FID : 0xC5) Latency Monitor";


- const char *desc = "Define Issue Get Feature command (FID : 0xC5) Latency Monitor"; + const char *desc = "Define Issue Get Feature command (FID: 0xC5) Latency Monitor";

@ikegami-t - Incorporated the review comments

ikegami-t · 2025-04-08T13:36:34Z

plugins/ocp/ocp-nvme.c

+{
+	const char *desc = "Define Issue Get Feature command (FID : 0xC5) Latency Monitor";
+	const char *sel = "[0-3]: current/default/saved/supported/";
+	const char *nsid = "Byte[04-07]:Namespace Identifier Valid/Invalid/Inactive";


- const char *nsid = "Byte[04-07]:Namespace Identifier Valid/Invalid/Inactive"; + const char *nsid = "Byte[04-07]: Namespace Identifier Valid/Invalid/Inactive";

@ikegami-t - Incorporated the review comments

ikegami-t · 2025-04-08T13:48:46Z

plugins/ocp/ocp-nvme.c

+	OPT_ARGS(opts) = {
+		OPT_BYTE("sel", 's', &cfg.sel, sel),
+		OPT_BYTE("nsid", 'n', &cfg.nsid, nsid),
+		OPT_FLAG("no-uuid", 'u', NULL, no_uuid),


Please update the documentation to add the `no-uuid' parameter.
Also the parameter is not used to check by the command actually so can you please use the option by the command as below for example?

static int eol_plp_failure_mode(int argc, char **argv, struct command *cmd, struct plugin *plugin) { ... err = eol_plp_failure_mode_get(dev, nsid, fid, cfg.sel, !argconfig_parse_seen(opts, "no-uuid"));

And also if the nsid value can be deleted then the no-uuid short option u looks better to change to n as same with other OCP commands.

@ikegami-t - Incorporated the review comments

ikegami-t · 2025-04-08T13:50:30Z

plugins/ocp/ocp-nvme.c

+		nvme_select_to_string(cfg.sel), result);
+
+		if (cfg.sel == NVME_GET_FEATURES_SEL_SUPPORTED)
+			nvme_show_select_result(0xC5, result);


- nvme_show_select_result(0xC5, result); + nvme_show_select_result(OCP_FID_LM, result);

@ikegami-t - We can't pass OCP_FID_LM for this nvme_show_select_result api. Because enum data type is different for that api parameter.

ikegami-t · 2025-04-08T13:53:13Z

plugins/ocp/ocp-nvme.h

 		ENTRY("set-enable-ieee1667-silo", "enable IEEE1667 silo", set_enable_ieee1667_silo)
 		ENTRY("hardware-component-log", "retrieve hardware component log", hwcomp_log)
+		ENTRY("get-latency-monitor", "Get Latency Monitor Feature",
+		       ocp_get_latency_monitor_feature)


ENTRY("get-latency-monitor", "Get Latency Monitor Feature", - ocp_get_latency_monitor_feature) + ocp_get_latency_monitor_feature)

@ikegami-t - Incorporated the review comments

ikegami-t · 2025-04-08T13:55:11Z

For the comment below mentioned by the comment #2768 (comment) is this command really required to support but it does not return useful information as described?

For get feature, from OCP 2.5 Spec (similar in 2.6): A Get Features command for Feature Identifier C5h should not be issued because no useful information is returned.

ikegami-t · 2025-04-09T12:26:56Z

Documentation/nvme-ocp-get-latency-monitor.txt

+SYNOPSIS
+--------
+[verse]
+'nvme ocp get-latency-monitor' <device> [--sel=<select> | -s <select>] [--namespace-id | -n] [--no-uuid | -u]


@ikegami-t Incorporated the review comments

ikegami-t · 2025-04-09T12:28:09Z

Documentation/nvme-ocp-get-latency-monitor.txt

+DESCRIPTION
+-----------
+Define get-latency-monitor.
+No argument prints current mode.


This should be deleted.

@ikegami-t - Incorporated the review comments

ikegami-t · 2025-04-09T12:33:31Z

completions/_nvme

+				/dev/nvme':supply a device to use (required)'
+				--sel=':select from 0 - current, 1 - default, 2 - saved, 3 - supported'
+				-S':alias to --sel'
+				--nsid=':valid, invalid and inactive nsid'


- --nsid=':valid, invalid and inactive nsid' + --namespace-id=':valid, invalid and inactive nsid'

Is this description valid, invalid and inactive nsid really proper explanation?

@ikegami-t - Incorporated the review comments

ikegami-t · 2025-04-09T12:35:58Z

completions/_nvme

+				--sel=':select from 0 - current, 1 - default, 2 - saved, 3 - supported'
+				-S':alias to --sel'
+				--nsid=':valid, invalid and inactive nsid'
+				-n':alias to --nsid'


- -n':alias to --nsid' + -n':alias to --namespace-id'

When change an implemetaion could you please fix all ~~realted~~ related implementaions since you change only the code part metioned but remain related code needed to change.

@ikegami-t - Incorporated the review comments

ikegami-t · 2025-04-09T12:39:06Z

completions/bash-nvme-completion.sh

 			--output-format -o --timeout= -t"
 			;;
+		"get-latency-monitor")
+		opts+=" --sel= -S \


- opts+=" --sel= -S \ + opts+=" --sel= -s \

@ikegami-t - Incorporated the review comments

ikegami-t · 2025-04-09T12:39:35Z

completions/_nvme

+				_ocp_get_latency_monitor_feature=(
+				/dev/nvme':supply a device to use (required)'
+				--sel=':select from 0 - current, 1 - default, 2 - saved, 3 - supported'
+				-S':alias to --sel'


- -S':alias to --sel' + -s':alias to --sel'

@ikegami-t - Incorporated the review comments

ikegami-t · 2025-04-09T12:40:14Z

Documentation/nvme-ocp-get-latency-monitor.txt

+
+OPTIONS
+-------
+


Please delete this blank line.

@ikegami-t - Incorporated the review comments

ikegami-t · 2025-04-09T12:43:02Z

completions/bash-nvme-completion.sh

 			;;
+		"get-latency-monitor")
+		opts+=" --sel= -S \
+			--nsid= -n"


- --nsid= -n" + --namespace-id= -n"

@ikegami-t - Incorporated the review comments

ikegami-t · 2025-04-09T12:43:48Z

completions/bash-nvme-completion.sh

+		"get-latency-monitor")
+		opts+=" --sel= -S \
+			--nsid= -n"
+			;;


Please add --no-uuid.

@ikegami-t - Incorporated the review comments

ikegami-t · 2025-04-09T12:48:04Z

plugins/ocp/ocp-nvme.c

 }

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


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

Seems repeated many times same comments for this part. Please add 5 tab spaces and only 3 white spaces before struct plugin *plugin.

@ikegami-t - Incorporated the review comments

ikegami-t · 2025-04-09T13:01:07Z

Could you please explain the reason to use the argument --namespace-id and add this command itself?
By the way the set feature command seems required instead of this get feature command then do you have any plan to add the set feature command?
About the commit message your Signed-off-by name repeated as Vigneshwaran Saravanan/Vigneshwaran Saravanan but is this expected?

plugin/ocp_get_feature_fid_c5h:Added the OCP Get Feature
 FID=C5h command api Enabled the Get Feature Command FID=C5h command api with
 nsid field Values

Reviewed-by: Karthik Balan <karthik.b82@samsung.com>
Reviewed-by: Arunpandian J <arun.j@samsung.com>
Signed-off-by: Vigneshwaran Saravanan/Vigneshwaran Saravanan <s.vignesh@samsung.com>

VigneshwaranSaravana · 2025-04-10T07:14:47Z

Could you please explain the reason to use the argument --namespace-id and add this command itself? By the way the set feature command seems required instead of this get feature command then do you have any plan to add the set feature command? About the commit message your Signed-off-by name repeated as Vigneshwaran Saravanan/Vigneshwaran Saravanan but is this expected?
plugin/ocp_get_feature_fid_c5h:Added the OCP Get Feature
 FID=C5h command api Enabled the Get Feature Command FID=C5h command api with
 nsid field Values

Reviewed-by: Karthik Balan <karthik.b82@samsung.com>
Reviewed-by: Arunpandian J <arun.j@samsung.com>
Signed-off-by: Vigneshwaran Saravanan/Vigneshwaran Saravanan <s.vignesh@samsung.com>

@ikegami-t - - Incorporated the review comments. Set feature FID=C5h command api already available(ocp_set_latency_monitor_feature).

ikegami-t · 2025-04-10T16:00:48Z

completions/bash-nvme-completion.sh

 			;;
+		"get-latency-monitor")
+		opts+=" --sel= -s \
+			--namespace-id= -n --no-uuid= -u"


- --namespace-id= -n --no-uuid= -u" + --namespace-id= -n --no-uuid -u"

@ikegami-t - Incorporated the review comment

ikegami-t · 2025-04-10T16:06:08Z

plugins/ocp/ocp-nvme.c

+
+	OPT_ARGS(opts) = {
+		OPT_BYTE("sel", 's', &cfg.sel, sel),
+		OPT_BYTE("namespace-id", 'n', &cfg.nsid, nsid),


- OPT_BYTE("namespace-id", 'n', &cfg.nsid, nsid), + OPT_UINT("namespace-id", 'n', &cfg.nsid, nsid),

@ikegami-t - Incorporated the review comment

ikegami-t · 2025-04-10T16:09:09Z

plugins/ocp/ocp-nvme.c

+	const char *desc = "Define Issue Get Feature command (FID: 0xC5) Latency Monitor";
+	const char *sel = "[0-3]: current/default/saved/supported/";
+	const char *nsid = "Byte[04-07]: Namespace Identifier Valid/Invalid/Inactive";
+	struct nvme_dev *dev;


const char *nsid = "Byte[04-07]: Namespace Identifier Valid/Invalid/Inactive"; - struct nvme_dev *dev; + + _cleanup_nvme_dev_ struct nvme_dev *dev = NULL;

@ikegami-t - Incorporated the review comment

ikegami-t · 2025-04-10T16:11:43Z

Looks almost okay but still minor comments so please confirm the comments again. Also can you fix the commit message Signed-off-by as below? Thank you.

- Signed-off-by: Vigneshwaran Saravanan/Vigneshwaran Saravanan <s.vignesh@samsung.com>
+ Signed-off-by: Vigneshwaran Saravanan <s.vignesh@samsung.com>

VigneshwaranSaravana · 2025-04-14T10:34:34Z

Looks almost okay but still minor comments so please confirm the comments again. Also can you fix the commit message Signed-off-by as below? Thank you.
- Signed-off-by: Vigneshwaran Saravanan/Vigneshwaran Saravanan <s.vignesh@samsung.com>
+ Signed-off-by: Vigneshwaran Saravanan <s.vignesh@samsung.com>

@ikegami-t - I have updated the Signed-off-by: Vigneshwaran Saravanan s.vignesh@samsung.com using git commit --amend --signoff command. But I am facing below error.

WARNING: From:/Signed-off-by: email name mismatch: 'From: "Vigneshwaran Saravanan/Vigneshwaran Saravanan" s.vignesh@samsung.com' != 'Signed-off-by: Vigneshwaran Saravanan s.vignesh@samsung.com'
Error: WARNING: From:/Signed-off-by: email name mismatch: 'From: "Vigneshwaran Saravanan/Vigneshwaran Saravanan" s.vignesh@samsung.com' != 'Signed-off-by: Vigneshwaran Saravanan s.vignesh@samsung.com'

ikegami-t · 2025-04-14T12:43:11Z

@VigneshwaranSaravana @igaw The changes look good. About the git commit error seems depended on the git config user.name setting but it is okay for me to remaing the current commit message Signed-off-by. Just FYI: You can check the git config setting by the following page, etc. Thank you.
https://stackoverflow.com/questions/46941346/how-to-know-the-git-username-and-email-saved-during-configuration

VigneshwaranSaravana · 2025-04-28T11:41:51Z

@VigneshwaranSaravana @igaw The changes look good. About the git commit error seems depended on the git config user.name setting but it is okay for me to remaing the current commit message Signed-off-by. Just FYI: You can check the git config setting by the following page, etc. Thank you. https://stackoverflow.com/questions/46941346/how-to-know-the-git-username-and-email-saved-during-configuration

@igaw - Fixed the username issue also. Could you please check.

Enabled the Get Feature command (FID=C5h) 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-07T14:14:27Z

rebase and some commit message reformatting.

igaw · 2025-05-07T14:16:32Z

Thanks!

ikegami-t reviewed Apr 4, 2025

View reviewed changes

VigneshwaranSaravana force-pushed the plugin/ocp_get_feature_fid_c5h branch 3 times, most recently from 0ef6ec1 to 86a4144 Compare April 7, 2025 14:59

ikegami-t reviewed Apr 7, 2025

View reviewed changes

VigneshwaranSaravana force-pushed the plugin/ocp_get_feature_fid_c5h branch 2 times, most recently from b94ba9d to ccd1d35 Compare April 8, 2025 11:12

ikegami-t reviewed Apr 8, 2025

View reviewed changes

VigneshwaranSaravana force-pushed the plugin/ocp_get_feature_fid_c5h branch 2 times, most recently from 9e4eaeb to 66c1eaa Compare April 9, 2025 12:01

ikegami-t reviewed Apr 9, 2025

View reviewed changes

VigneshwaranSaravana force-pushed the plugin/ocp_get_feature_fid_c5h branch 2 times, most recently from c9e0743 to c4ffcc3 Compare April 10, 2025 06:18

ikegami-t reviewed Apr 10, 2025

View reviewed changes

VigneshwaranSaravana force-pushed the plugin/ocp_get_feature_fid_c5h branch 3 times, most recently from 46d4c3b to d162c23 Compare April 14, 2025 10:26

VigneshwaranSaravana force-pushed the plugin/ocp_get_feature_fid_c5h branch from d162c23 to 659a211 Compare April 28, 2025 11:37

VigneshwaranSaravana closed this Apr 28, 2025

VigneshwaranSaravana reopened this Apr 28, 2025

ikegami-t mentioned this pull request May 2, 2025

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

Merged

igaw force-pushed the plugin/ocp_get_feature_fid_c5h branch from 659a211 to 3808b9d Compare May 7, 2025 14:13

igaw merged commit 901c960 into linux-nvme:master May 7, 2025
18 checks passed

ikegami-t mentioned this pull request May 15, 2025

plugin/ocp_get_feature_fid_c8h:Added the OCP Get Feature FID=C8h comm… #2788

Merged


		OPTIONS
		-------

Conversation

VigneshwaranSaravana commented Apr 4, 2025

Uh oh!

HaroPanosyan commented Apr 4, 2025

Uh oh!

ikegami-t commented Apr 4, 2025

Uh oh!

ikegami-t commented Apr 4, 2025

Uh oh!

ikegami-t commented Apr 4, 2025

Uh oh!

HaroPanosyan commented Apr 4, 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!

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 Apr 8, 2025

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!

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!

ikegami-t commented Apr 8, 2025

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!

ikegami-t Apr 9, 2025 •

edited

Loading