plugins/micron: Added support for OCP telemetry (internal) log parsing. Datacenter NVMe SSD Specification v2.5r9 section 4.9 #2354

saskchaithanya · 2024-05-24T10:45:05Z

Description:

Added support for OCP telemetry (internal) log parsing as per OCP 2.5 specification - Datacenter NVMe SSD Specification v2.5r9.pdf section 4.9.
The CLI command will take the input binary files (telemetry/internal logs LOG ID 0x07 or 0x08) and string log (0xC9) as inputs and provides a parsed JSON or text file as attached below:

CLI Examples:
$ sudo ./nvme micron ocp-telemetry-log-parse --format=normal --string-log="nvmelog_ocp_c9.bin" --telemetry-log="nvme_host_telemetry_log.bin" --output-file="nvme_cli_telemetry_host_normal.txt" /dev/nvme0

$ sudo ./nvme micron ocp-telemetry-log-parse --format=json --string-log="nvmelog_ocp_c9.bin" --telemetry-log="nvme_host_telemetry_log.bin" --output-file="nvme_cli_telemetry_host_normal.json" /dev/nvme0

$ sudo ./nvme micron ocp-telemetry-log-parse --format=normal --string-log="nvmelog_ocp_c9.bin" --telemetry-log="nvme_host_telemetry_log.bin" > nvme_cli_telemetry_host_console.txt /dev/nvme0

$ sudo ./nvme micron ocp-telemetry-log-parse --format=json --string-log="nvmelog_ocp_c9.bin" --telemetry-log="nvme_host_telemetry_log.bin" > nvme_cli_telemetry_host_console.json /dev/nvme0

Output Files:
nvme_cli_telemetry_host_console.json
nvme_cli_telemetry_host_console.txt
nvme_cli_telemetry_host_normal.json
nvme_cli_telemetry_host_normal.txt

igaw · 2024-05-24T10:53:55Z

From a quick look, there are coding style issues, please see the output from checkpatch. It should help to identify the issues.

saskchaithanya · 2024-05-28T12:42:07Z

From a quick look, there are coding style issues, please see the output from checkpatch. It should help to identify the issues.

Fixed the coding style issues. Please review

igaw · 2024-06-04T13:46:30Z

You should update the first commit instead of adding a new commit. The reason is that we follow the coding and submission rules from the linux kernel. Thus we are explicitly not allowing the github style submissions.

Also the commit message should be properly format and containing some information why you are changing thigns:

plugins/micron: Add support for OCP telemetry log parsing

Add support for OCP (internal) log parsing parsing. [maybe add the rerference to spec what part is implemented. This information should to help to review and also help to debug in future]

Signed-off-by: Chaithanya Shoba <[email protected]>

After looking at the changes, it contains several unrelated changes in one PR. Please split them up, see the kernel documentation https://docs.kernel.org/process/submitting-patches.html#

The checkpatch.pl tool is very unhappy with this change:

$ ../linux/scripts/checkpatch.pl -g HEAD
[...]
ERROR: trailing whitespace
#5360: FILE: plugins/micron/micron-utils.h:48:
+ * @return integer value of hexadecimal $

ERROR: "foo* bar" should be "foo *bar"
#5372: FILE: plugins/micron/micron-utils.h:60:
+char* hex_to_ascii(const char *hex);

ERROR: "foo* bar" should be "foo *bar"
#5384: FILE: plugins/micron/micron-utils.h:72:
+unsigned char* read_binary_file(char *data_dir_path, const char *bin_path, long *buffer_size,

WARNING: line length of 104 exceeds 100 columns
#5400: FILE: plugins/micron/micron-utils.h:88:
+                                                       struct json_object *stats, __u8 spec, FILE *fp);

ERROR: Missing Signed-off-by: line by nominal patch author 'Chaithanya shoba <[email protected]>'

total: 230 errors, 877 warnings, 5229 lines checked

Arunpandian15 · 2024-06-20T01:08:43Z

@saskchaithanya
Text documentation file is missing. Could you please add .txt file in documentation.

Arunpandian15 · 2024-06-20T01:23:43Z

@saskchaithanya
Please fix all the checkpatch review build failure. Most of them are related to coding style and compliance to the nvme-cli.

WARNING: Missing or malformed SPDX-License-Identifier tag in line 1
#6536: FILE: plugins/micron/micron-utils.h:1:
+// SPDX-License-Identifier: GPL-2.0-or-later
Error: WARNING: Missing or malformed SPDX-License-Identifier tag in line 1

ERROR: "foo* bar" should be "foo bar"
#6565: FILE: plugins/micron/micron-utils.h:60:
+char hex_to_ascii(const char hex);
Error: ERROR: "foo bar" should be "foo *bar"

ERROR: "foo* bar" should be "foo bar"
#6574: FILE: plugins/micron/micron-utils.h:72:
+unsigned char read_binary_file(char *data_dir_path, const char *bin_path, long buffer_size,
Error: ERROR: "foo bar" should be "foo *bar"

WARNING: line length of 104 exceeds 100 columns
#6585: FILE: plugins/micron/micron-utils.h:88:

struct json_object *stats, __u8 spec, FILE *fp);
Error: WARNING: line length of 104 exceeds 100 columns

ERROR: Missing Signed-off-by: line(s)
Error: ERROR: Missing Signed-off-by: line(s)

total: 200 errors, 773 warnings, 6430 lines checked

saskchaithanya · 2024-06-24T09:15:50Z

Thanks @igaw and @Arunpandian15 for the review suggestions.
I am working on those and will update the pull request as early as possible.

igaw · 2024-06-25T12:34:13Z

Please squash all changes into one patch, there is no need to see the development version of a change. And please no merges into the PR. Thanks.

plugins/micron/micron-nvme.c

plugins/micron/micron-utils.c

saskchaithanya · 2024-07-01T11:15:22Z

Datacenter NVMe SSD Specification v2.5r9.pdf

Addressed the review comments and fixed the check-patch issues.
Please see the attached spec section 4.9 for reference and review.
Thanks!

saskchaithanya · 2024-07-01T11:37:41Z

@saskchaithanya Please fix all the checkpatch review build failure. Most of them are related to coding style and compliance to the nvme-cli.

WARNING: Missing or malformed SPDX-License-Identifier tag in line 1 #6536: FILE: plugins/micron/micron-utils.h:1: +// SPDX-License-Identifier: GPL-2.0-or-later Error: WARNING: Missing or malformed SPDX-License-Identifier tag in line 1

ERROR: "foo* bar" should be "foo bar" #6565: FILE: plugins/micron/micron-utils.h:60: +char hex_to_ascii(const char hex); Error: ERROR: "foo bar" should be "foo *bar"

ERROR: "foo* bar" should be "foo bar" #6574: FILE: plugins/micron/micron-utils.h:72: +unsigned char read_binary_file(char *data_dir_path, const char *bin_path, long buffer_size, Error: ERROR: "foo bar" should be "foo *bar"

WARNING: line length of 104 exceeds 100 columns #6585: FILE: plugins/micron/micron-utils.h:88:

struct json_object *stats, __u8 spec, FILE *fp);
Error: WARNING: line length of 104 exceeds 100 columns

ERROR: Missing Signed-off-by: line(s) Error: ERROR: Missing Signed-off-by: line(s)

total: 200 errors, 773 warnings, 6430 lines checked

Fixed the coding-style issues.

saskchaithanya · 2024-07-01T11:38:35Z

Please squash all changes into one patch, there is no need to see the development version of a change. And please no merges into the PR. Thanks.

Thanks a lot for the suggestion, Squashed the changes to one patch!

saskchaithanya · 2024-07-01T11:39:47Z

You should update the first commit instead of adding a new commit. The reason is that we follow the coding and submission rules from the linux kernel. Thus we are explicitly not allowing the github style submissions.

Also the commit message should be properly format and containing some information why you are changing thigns:
plugins/micron: Add support for OCP telemetry log parsing

Add support for OCP (internal) log parsing parsing. [maybe add the rerference to spec what part is implemented. This information should to help to review and also help to debug in future]

Signed-off-by: Chaithanya Shoba <[email protected]>
After looking at the changes, it contains several unrelated changes in one PR. Please split them up, see the kernel documentation https://docs.kernel.org/process/submitting-patches.html#

The checkpatch.pl tool is very unhappy with this change:
$ ../linux/scripts/checkpatch.pl -g HEAD
[...]
ERROR: trailing whitespace
#5360: FILE: plugins/micron/micron-utils.h:48:
+ * @return integer value of hexadecimal $

ERROR: "foo* bar" should be "foo *bar"
#5372: FILE: plugins/micron/micron-utils.h:60:
+char* hex_to_ascii(const char *hex);

ERROR: "foo* bar" should be "foo *bar"
#5384: FILE: plugins/micron/micron-utils.h:72:
+unsigned char* read_binary_file(char *data_dir_path, const char *bin_path, long *buffer_size,

WARNING: line length of 104 exceeds 100 columns
#5400: FILE: plugins/micron/micron-utils.h:88:
+                                                       struct json_object *stats, __u8 spec, FILE *fp);

ERROR: Missing Signed-off-by: line by nominal patch author 'Chaithanya shoba <[email protected]>'

total: 230 errors, 877 warnings, 5229 lines checked

Fixed check-patch issues and updated the commit message.

plugins/micron/micron-nvme.c

saskchaithanya · 2024-07-01T14:23:23Z

@saskchaithanya Text documentation file is missing. Could you please add .txt file in documentation.

Done, Thanks!

igaw · 2024-07-01T14:38:10Z

What are you editor settings? Are the tab settings set to 4 instead of 8? There are quiet a few places where the aliment is completely off:

plugins/micron/micron-utils.c

ikegami-t · 2024-07-01T17:36:14Z

plugins/micron/micron-utils.c

+}
+
+
+void print_micron_vs_logs(__u8 *buf, struct micron_vs_logpage *log_page, int field_count,


Still seems the switch case functions can be split more as below. Thank you.

func_bbb() { ... } func_***() { ... } func_aaa() { switch () { case bbb: func_bbb(); break; ... default: func_***(); break; } }

plugins/micron/micron-utils.c

Documentation/meson.build

completions/bash-nvme-completion.sh

ikegami-t · 2024-07-06T11:41:49Z

Still checkpatch.pl failed.

saskchaithanya · 2024-07-06T17:08:17Z

Still checkpatch.pl failed.

I am in-progress of addressing those issues. Thanks for checking. I will address the issues ASAP!

saskchaithanya · 2024-07-06T17:12:08Z

What are you editor settings? Are the tab settings set to 4 instead of 8? There are quiet a few places where the aliment is completely off:

Addressed the indentation issues. Thanks for checking!

saskchaithanya · 2024-07-08T11:34:03Z

Could you please help running checkpatch.pl again. I missed to save the earlier failures?

ikegami-t · 2024-07-08T15:43:27Z

The script included by the linux kernel and can be cloned from the repository below.

git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git

Also the script can be executed as below for example.

tokunori@tokunori-desktop:~/nvme-cli$ ../linux/scripts/checkpatch.pl -g HEAD

saskchaithanya · 2024-07-08T15:49:53Z

The script included by the linux kernel and can be cloned from the repository below.
git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 
Also the script can be executed as below for example.
tokunori@tokunori-desktop:~/nvme-cli$ ../linux/scripts/checkpatch.pl -g HEAD

Thanks a lot, for the information!

Documentation/nvme-micron-ocp-telemetry-log-parse.txt

saskchaithanya · 2024-07-08T18:43:34Z

Address the checkpatch issues and review comments.
Please re-run the checkpatch and review latest changes.
Thanks!

plugins/micron/micron-ocp-telemetry.c

igaw · 2024-07-09T10:32:32Z

Almost there. There are some checkpatch complains left, nothing serious.

Though clang is not happy:

 FAILED: nvme.p/plugins_micron_micron-ocp-telemetry.c.o 
clang -Invme.p -I. -I.. -Iccan -I../ccan -Isubprojects/libnvme/src -I../subprojects/libnvme/src -I/usr/include/json-c -fcolor-diagnostics -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Werror -std=gnu99 -O0 -g -fomit-frame-pointer -D_GNU_SOURCE -include config.h -DCCAN_LIST_DEBUG=1 -DCCAN_STR_DEBUG=1 -MD -MQ nvme.p/plugins_micron_micron-ocp-telemetry.c.o -MF nvme.p/plugins_micron_micron-ocp-telemetry.c.o.d -o nvme.p/plugins_micron_micron-ocp-telemetry.c.o -c ../plugins/micron/micron-ocp-telemetry.c
../plugins/micron/micron-ocp-telemetry.c:697:16: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security]
                        fprintf(fp, buffer);
                                    ^~~~~~
../plugins/micron/micron-ocp-telemetry.c:697:16: note: treat the string as an argument to avoid this
                        fprintf(fp, buffer);
                                    ^
                                    "%s", 
../plugins/micron/micron-ocp-telemetry.c:699:11: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security]
                        printf(buffer);
                               ^~~~~~
../plugins/micron/micron-ocp-telemetry.c:699:11: note: treat the string as an argument to avoid this
                        printf(buffer);
                               ^
                               "%s",

You can trigger this build locally with

$ scripts/build.sh -c clang

Datacenter NVMe SSD Specification v2.5r9, section 4.9. Signed-off-by: Chaithanya Shoba <[email protected]>

saskchaithanya · 2024-07-09T12:12:35Z

Almost there. There are some checkpatch complains left, nothing serious.

Though clang is not happy:

 FAILED: nvme.p/plugins_micron_micron-ocp-telemetry.c.o 
clang -Invme.p -I. -I.. -Iccan -I../ccan -Isubprojects/libnvme/src -I../subprojects/libnvme/src -I/usr/include/json-c -fcolor-diagnostics -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Werror -std=gnu99 -O0 -g -fomit-frame-pointer -D_GNU_SOURCE -include config.h -DCCAN_LIST_DEBUG=1 -DCCAN_STR_DEBUG=1 -MD -MQ nvme.p/plugins_micron_micron-ocp-telemetry.c.o -MF nvme.p/plugins_micron_micron-ocp-telemetry.c.o.d -o nvme.p/plugins_micron_micron-ocp-telemetry.c.o -c ../plugins/micron/micron-ocp-telemetry.c
../plugins/micron/micron-ocp-telemetry.c:697:16: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security]
                        fprintf(fp, buffer);
                                    ^~~~~~
../plugins/micron/micron-ocp-telemetry.c:697:16: note: treat the string as an argument to avoid this
                        fprintf(fp, buffer);
                                    ^
                                    "%s", 
../plugins/micron/micron-ocp-telemetry.c:699:11: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security]
                        printf(buffer);
                               ^~~~~~
../plugins/micron/micron-ocp-telemetry.c:699:11: note: treat the string as an argument to avoid this
                        printf(buffer);
                               ^
                               "%s",

You can trigger this build locally with

$ scripts/build.sh -c clang

Thanks for the suggestion, Fixed the issues.

saskchaithanya · 2024-07-09T12:13:38Z

Addressed the checkpatch issues and review comments.
Please re-run the checkpatch and review latest changes.
Thanks!

igaw · 2024-07-09T12:20:37Z

Thanks!

saskchaithanya · 2024-07-09T16:00:51Z

Thanks @igaw and all!

Move OCP internal log parsing from Micron to OCP Plugin.Datacenter NVMe SSD Specification v2.5r9 section 4.9. Previous PR linux-nvme#2354 Signed-off-by: Chaithanya Shoba <[email protected]>

Move OCP internal log parsing from Micron to OCP Plugin. Datacenter NVMe SSD Specification v2.5r9 section 4.9. Previous PR linux-nvme#2354 Signed-off-by: Chaithanya Shoba <[email protected]>

Move OCP internal log parsing from Micron to OCP Plugin. Previous PR linux-nvme#2354. Datacenter NVMe SSD Specification v2.5r9, section 4.9. Signed-off-by: Chaithanya shoba <[email protected]>

Move OCP internal log parsing from Micron to OCP Plugin. Previous PR #2354. Datacenter NVMe SSD Specification v2.5r9, section 4.9. Signed-off-by: Chaithanya shoba <[email protected]>

saskchaithanya marked this pull request as draft June 28, 2024 06:32

ikegami-t reviewed Jun 28, 2024

View reviewed changes

saskchaithanya marked this pull request as ready for review July 1, 2024 11:15

martin-belanger reviewed Jul 1, 2024

View reviewed changes

plugins/micron/micron-nvme.c Outdated Show resolved Hide resolved

martin-belanger reviewed Jul 1, 2024

View reviewed changes

plugins/micron/micron-nvme.c Outdated Show resolved Hide resolved

martin-belanger reviewed Jul 1, 2024

View reviewed changes

plugins/micron/micron-nvme.c Outdated Show resolved Hide resolved

ikegami-t reviewed Jul 1, 2024

View reviewed changes

HaroPanosyan reviewed Jul 8, 2024

View reviewed changes

Documentation/nvme-micron-ocp-telemetry-log-parse.txt Outdated Show resolved Hide resolved

github-advanced-security bot found potential problems Jul 9, 2024

View reviewed changes

plugins/micron/micron-ocp-telemetry.c Fixed Show resolved Hide resolved

plugins/micron/micron-ocp-telemetry.c Fixed Show resolved Hide resolved

plugins/micron: Add support for OCP telemetry log parsing

b4f4603

Datacenter NVMe SSD Specification v2.5r9, section 4.9. Signed-off-by: Chaithanya Shoba <[email protected]>

igaw merged commit e9c2e8f into linux-nvme:master Jul 9, 2024
16 of 17 checks passed

saskchaithanya deleted the ocp-telemetry-parse-support branch July 9, 2024 15:09

saskchaithanya mentioned this pull request Jul 14, 2024

plugins/micron: Move OCP internal log parsing from Micron to OCP Plugin. #2417

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

plugins/micron: Added support for OCP telemetry (internal) log parsing. Datacenter NVMe SSD Specification v2.5r9 section 4.9 #2354

plugins/micron: Added support for OCP telemetry (internal) log parsing. Datacenter NVMe SSD Specification v2.5r9 section 4.9 #2354

saskchaithanya commented May 24, 2024 •

edited

Loading

igaw commented May 24, 2024

saskchaithanya commented May 28, 2024

igaw commented Jun 4, 2024

Arunpandian15 commented Jun 20, 2024

Arunpandian15 commented Jun 20, 2024

saskchaithanya commented Jun 24, 2024

igaw commented Jun 25, 2024

saskchaithanya commented Jul 1, 2024

saskchaithanya commented Jul 1, 2024

saskchaithanya commented Jul 1, 2024

saskchaithanya commented Jul 1, 2024

saskchaithanya commented Jul 1, 2024 •

edited

Loading

igaw commented Jul 1, 2024

ikegami-t Jul 1, 2024

ikegami-t commented Jul 6, 2024

saskchaithanya commented Jul 6, 2024 •

edited

Loading

saskchaithanya commented Jul 6, 2024

saskchaithanya commented Jul 8, 2024 •

edited

Loading

ikegami-t commented Jul 8, 2024

saskchaithanya commented Jul 8, 2024

saskchaithanya commented Jul 8, 2024

igaw commented Jul 9, 2024

saskchaithanya commented Jul 9, 2024

saskchaithanya commented Jul 9, 2024

igaw commented Jul 9, 2024

saskchaithanya commented Jul 9, 2024

		}


		void print_micron_vs_logs(__u8 buf, struct micron_vs_logpage log_page, int field_count,

plugins/micron: Added support for OCP telemetry (internal) log parsing. Datacenter NVMe SSD Specification v2.5r9 section 4.9 #2354

plugins/micron: Added support for OCP telemetry (internal) log parsing. Datacenter NVMe SSD Specification v2.5r9 section 4.9 #2354

Conversation

saskchaithanya commented May 24, 2024 • edited Loading

igaw commented May 24, 2024

saskchaithanya commented May 28, 2024

igaw commented Jun 4, 2024

Arunpandian15 commented Jun 20, 2024

Arunpandian15 commented Jun 20, 2024

saskchaithanya commented Jun 24, 2024

igaw commented Jun 25, 2024

saskchaithanya commented Jul 1, 2024

saskchaithanya commented Jul 1, 2024

saskchaithanya commented Jul 1, 2024

saskchaithanya commented Jul 1, 2024

saskchaithanya commented Jul 1, 2024 • edited Loading

igaw commented Jul 1, 2024

ikegami-t Jul 1, 2024

Choose a reason for hiding this comment

ikegami-t commented Jul 6, 2024

saskchaithanya commented Jul 6, 2024 • edited Loading

saskchaithanya commented Jul 6, 2024

saskchaithanya commented Jul 8, 2024 • edited Loading

ikegami-t commented Jul 8, 2024

saskchaithanya commented Jul 8, 2024

saskchaithanya commented Jul 8, 2024

igaw commented Jul 9, 2024

saskchaithanya commented Jul 9, 2024

saskchaithanya commented Jul 9, 2024

igaw commented Jul 9, 2024

saskchaithanya commented Jul 9, 2024

saskchaithanya commented May 24, 2024 •

edited

Loading

saskchaithanya commented Jul 1, 2024 •

edited

Loading

saskchaithanya commented Jul 6, 2024 •

edited

Loading

saskchaithanya commented Jul 8, 2024 •

edited

Loading