saskchaithanya commented 4 months ago

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 commented 4 months ago

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

saskchaithanya commented 4 months ago

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 commented 4 months ago

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 <ashoba@micron.com>

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 <chaitu9es@gmail.com>'

total: 230 errors, 877 warnings, 5229 lines checked

Arunpandian15 commented 3 months ago

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

Arunpandian15 commented 3 months ago

@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 commented 3 months ago

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

igaw commented 3 months ago

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.

saskchaithanya commented 3 months ago

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 commented 3 months ago

@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 _buffersize, 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 commented 3 months ago

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 commented 3 months ago

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 <ashoba@micron.com>
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 <chaitu9es@gmail.com>'

total: 230 errors, 877 warnings, 5229 lines checked

Fixed check-patch issues and updated the commit message.

saskchaithanya commented 3 months ago

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

Done, Thanks!

igaw commented 3 months ago

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:

ikegami-t commented 3 months ago

Still checkpatch.pl failed.

saskchaithanya commented 3 months ago

Still checkpatch.pl failed.

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

saskchaithanya commented 3 months ago

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 commented 2 months ago

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

ikegami-t commented 2 months ago

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 commented 2 months ago

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!

saskchaithanya commented 2 months ago

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

igaw commented 2 months ago

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

saskchaithanya commented 2 months ago

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 commented 2 months ago

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

igaw commented 2 months ago

Thanks!

saskchaithanya commented 2 months ago

Thanks @igaw and all!

linux-nvme / nvme-cli

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

6536: FILE: plugins/micron/micron-utils.h:1:

6565: FILE: plugins/micron/micron-utils.h:60:

6574: FILE: plugins/micron/micron-utils.h:72:

6585: FILE: plugins/micron/micron-utils.h:88: