nvme: plot eye chart data and hex dumping VS eye data by sivaprasad6541 · Pull Request #2828 · linux-nvme/nvme-cli

sivaprasad6541 · 2025-05-27T06:21:17Z

Fixed segmentation fault issue in JSON output format for VS Eye data.
Added support to hex dump VS Eye data in both normal and JSON formats.
This enables customers to decode the raw eye chart data if needed.
Ensured compatibility with existing d() and obj_d() hex dump utilities.

- Fixed segmentation fault issue in JSON output format for VS Eye data. - Added support to hex dump VS Eye data in both normal and JSON formats. - This enables customers to decode the raw eye chart data if needed. - Ensured compatibility with existing d() and obj_d() hex dump utilities. Signed-off-by: Sivaprasad Gutha <sivaprasad6541@gmail.com>

- Added contributors list to the AUTHORS section of the Micron plugin. Signed-off-by: Sivaprasad Gutha <sivaprasad6541@gmail.com>

sivaprasad6541 · 2025-05-27T06:41:49Z

Tested both formats and here is the results.
normal.txt
jsondata.json

igaw

A few nitpicks.

The only real issue I see is JSON member name change, because this breaks any existing parser.

igaw · 2025-05-27T07:21:12Z

nvme-print-json.c

 /* Printable Eye string is allocated and returned, caller must free */
 static char *json_eom_printable_eye(struct nvme_eom_lane_desc *lane,
-				    struct json_object *r)
+									struct json_object *r)


unrelated whitespace change, please drop this one

I've undone the unrelated whitespace change as suggested.

igaw · 2025-05-27T07:22:06Z

nvme-print-json.c

-		goto exit;
+		return NULL;
+
+	struct json_object *eye_array = json_create_array();


If we are checking for null pointers, then this one should also be checked I suppose.

And don't we need to free it eventually? In this case use the cleanup helpers so that you can exit the funtion at anytime. Can't remember what json-c does, if it takes the owner ship.

Thanks for the suggestion! I initially tried using cleanup helpers, but encountered a segfault in the caller while printing the root JSON object. Since the caller is responsible for freeing the memory, I've kept the manual cleanup logic in place to avoid ownership issues.
Added a null check for eye_array now.

again declaration go to the beginning of the block

Moved this to beginning of the block.

igaw · 2025-05-27T07:25:10Z

nvme-print-json.c

+		char *row = malloc(ncols + 1);
+
+		if (!row)
+			continue;


Does it make to continue if we run out of memory? Maybe break or return NULL?

Thanks for the catch. I've modified it to return NULL.

igaw · 2025-05-27T07:26:29Z

nvme-print-json.c

-	obj_add_str(r, "printable_eye", printable_start);
+	*printable = '\0';
+
+	obj_add_array(r, "printable_eye_rows", eye_array);


This breaks the 'API', the name changes and existing parsers will be unhappy.

Ah wait, there is a way for this change. We introduced the possibility to select the output format with --output-format-version, e.g. see f8f086c ("nvme: add output-format-version option")

I have renamed it as earlier.
As per #2828 (comment)
If we hear feedback from users later, we can revisit with the --output-format-version approach as you suggested.

igaw · 2025-05-27T07:26:49Z

nvme-print-json.c

+			continue;
+		else {
+			/* Hex dump Vendor Specific Eye Data*/
+			vsdata = (unsigned char *)malloc(desc->edlen);


no need for the cast.

Removed the casting as suggested.

igaw · 2025-05-27T07:27:55Z

nvme-print-json.c

+			char *hexstr = malloc(desc->edlen * 3 + 1); // 2 hex chars + space per byte

+			if (!hexstr)
+				return;


leaking vsdata?

Thanks for the catch.
I've removed memory allocation for vsdata as it is not required.

igaw · 2025-05-27T07:28:35Z

nvme-print-json.c

+			/* Hex dump Vendor Specific Eye Data*/
+			vsdata = (unsigned char *)malloc(desc->edlen);
+			vsdataoffset = (desc->nrows * desc->ncols) +
+								sizeof(struct nvme_eom_lane_desc);


indention seems to be off here.

Thanks for pointing that out! had to break the line due to the 100-column limit, which affected the indentation slightly.
Now, I've modified it to look better indentation.

igaw · 2025-05-27T07:29:20Z

nvme-print-stdout.c

+			/* Hex dump Vendor Specific Eye Data */
+			vsdata = (unsigned char *)malloc(desc->edlen);
+			vsdataoffset = (desc->nrows * desc->ncols) +
+								sizeof(struct nvme_eom_lane_desc);


indention seems to be off.

Thanks for pointing that out! had to break the line due to the 100-column limit, which affected the indentation slightly.
Now, I've modified it to look better indentation.

igaw · 2025-05-27T07:29:41Z

nvme-print-stdout.c

 		/* Eye Data field is vendor specific */
+		if (desc->edlen == 0)
+			continue;
+		else {


no need for the else branch, reduces the indention.

Removed the else branch is suggested.

igaw · 2025-05-27T07:29:48Z

nvme-print-stdout.c

+			continue;
+		else {
+			/* Hex dump Vendor Specific Eye Data */
+			vsdata = (unsigned char *)malloc(desc->edlen);


no need for the cast

Removed the cast as suggested.

igaw · 2025-05-27T07:33:39Z

@brandon-paupore-sndk maybe also have a look at it, as you provided the initial code.

brandon-paupore-sndk

Looks good to me, other than @igaw's comments and some minor little-endian conversion cleanup. I don't have any preference against the main update to the JSON eye object as an array of strings per newline rather than one large string with embedded newlines.

brandon-paupore-sndk · 2025-05-27T15:21:36Z

nvme-print-json.c

+	uint16_t nrows = lane->nrows;
+	uint16_t ncols = lane->ncols;


These should be swapped to the host endianness (i.e. le16_to_cpu(lane->nrows)), not that it's a new issue.

I’ve updated the code to convert the values to host endianness using le16_to_cpu() as suggested.

brandon-paupore-sndk · 2025-05-27T21:50:23Z

nvme-print-json.c

+			vsdata = (unsigned char *)malloc(desc->edlen);
+			vsdataoffset = (desc->nrows * desc->ncols) +


The various __le16 fields in desc should be converted to host-endianness before being used. The uses of these fields below should also be swapped.

I’ve updated the code to convert the values to host endianness using le16_to_cpu() as suggested.

brandon-paupore-sndk · 2025-05-27T21:54:39Z

nvme-print-stdout.c

+			vsdata = (unsigned char *)malloc(desc->edlen);
+			vsdataoffset = (desc->nrows * desc->ncols) +


As on the JSON side, the various __le16 fields in desc should be converted to host-endianness before being used. The use below should also be swapped.

I’ve updated the code to convert the values to host endianness using le16_to_cpu() as suggested.

igaw · 2025-05-28T07:18:10Z

Looks good to me, other than @igaw's comments and some minor little-endian conversion cleanup. I don't have any preference against the main update to the JSON eye object as an array of strings per newline rather than one large string with embedded newlines.

I am a bit wary with json output format changes. If you think it's okay to change to format now (I don't expect many users for this interface anyway) we can do it. If someone complains we could use the --output-format-version trick.

Basically it's your call :)

brandon-paupore-sndk · 2025-05-28T14:45:45Z

Looks good to me, other than @igaw's comments and some minor little-endian conversion cleanup. I don't have any preference against the main update to the JSON eye object as an array of strings per newline rather than one large string with embedded newlines.

I am a bit wary with json output format changes. If you think it's okay to change to format now (I don't expect many users for this interface anyway) we can do it. If someone complains we could use the --output-format-version trick.

Basically it's your call :)

Well thanks! And yeah, I agree that ideally the JSON output would be stable, but I think it'd be early enough to change this without needing different output versions. Particularly given that the other JSON output fixes here address a likely segfault.

If this array output format would be preferred in the long-term then I'm good with making the switch now, or if not then leaving the format as-is and just keeping the fixes.

-Addressing review comments Signed-off-by: Sivaprasad Gutha <sivaprasad6541@gmail.com>

brandon-paupore-sndk · 2025-05-29T14:29:24Z

nvme-print-stdout.c

+		if (desc->edlen == 0)
+			continue;
+
+		/* Hex dump Vendor Specific Eye Data */
+		vsdata = malloc(desc->edlen);
+		vsdataoffset = (desc->nrows * desc->ncols) +
+						sizeof(struct nvme_eom_lane_desc);
+		vsdata = (unsigned char *)((unsigned char *)desc + vsdataoffset);
+		printf("Eye Data:\n");
+		d(vsdata, desc->edlen, 16, 1);


Most of the le16 conversions are done, but there's still a few in this section for desc->(edlen, nrows, ncols).

Sorry for that. I've converted all the remaining ones now.

-Addressing review comments -Converting all the unsigned shorts into host-endianness Signed-off-by: Sivaprasad Gutha <sivaprasad6541@gmail.com>

igaw

Please merge the updates/fixes into the first patch.

igaw · 2025-06-02T12:18:25Z

nvme-print-json.c

+	if (nrows == 0 || ncols == 0)
+		return NULL;
+
+	// Allocate buffer for full printable string (with newlines)


Please use /* */ commet style, e.g.

/* * Allocate buffer for full printable string (with newlines) * +1 for null terminator */ printable = malloc(nrows * ncols + nrows + 1); printable_start = printable;

Modified as suggested.

igaw · 2025-06-02T12:19:02Z

nvme-print-json.c

+		return NULL;
+
+	// Allocate buffer for full printable string (with newlines)
+	char *printable = malloc(nrows * ncols + nrows + 1); // +1 for null terminator


Variable declaration are at the beginning of the block. There only a few exceptions allowed.

Modified as suggested.

igaw · 2025-06-02T12:19:30Z

nvme-print-json.c

-		goto exit;
+		return NULL;
+
+	struct json_object *eye_array = json_create_array();


again declaration go to the beginning of the block

igaw · 2025-06-02T12:21:58Z

nvme-print-json.c

+						sizeof(struct nvme_eom_lane_desc);
+		vsdata = (unsigned char *)((unsigned char *)desc + vsdataoffset);
+		// 2 hex chars + space per byte
+		_cleanup_free_ char *hexstr = malloc(le16_to_cpu(desc->edlen) * 3 + 1);


Please introduce local variable for nrows ncols and edlen so you only need to to the cpu conversion once.

Also while at it please use consistent comment style.

Modified as suggested.

igaw · 2025-06-02T12:22:40Z

nvme-print-json.c

+		/* Hex dump Vendor Specific Eye Data*/
+		vsdataoffset = (le16_to_cpu(desc->nrows) * le16_to_cpu(desc->ncols)) +
+						sizeof(struct nvme_eom_lane_desc);
+		vsdata = (unsigned char *)((unsigned char *)desc + vsdataoffset);


I would first do the allocation (hexstr) before the computation.

Modified as suggested.

igaw · 2025-06-02T12:23:42Z

nvme-print-stdout.c

-		vsdata = malloc(desc->edlen);
-		vsdataoffset = (desc->nrows * desc->ncols) +
+		vsdata = malloc(le16_to_cpu(desc->edlen));
+		vsdataoffset = (le16_to_cpu(desc->nrows) * le16_to_cpu(desc->ncols)) +


please introduce local varables instead redoing the cpu conversion everytime.

Modified as suggested.

- Addressing review comments Signed-off-by: Sivaprasad Gutha <sivaprasad6541@gmail.com>

sivaprasad6541 · 2025-06-04T10:12:51Z

Please merge the updates/fixes into the first patch.

@igaw you mean, do I need to raise separate PR with all the changes I did in 5 commits as a single commit.
Please let me know if my understanding is correct.

sivaprasad6541 · 2025-06-09T06:50:20Z

Please merge the updates/fixes into the first patch.

@igaw you mean, do I need to raise separate PR with all the changes I did in 5 commits as a single commit. Please let me know if my understanding is correct.

@igaw Could you please let me know if my understanding is correct.

Support for the 0xC0 log page needs to be added for the SNTMP device. Removed support for parsing the 0xCA log page for the SNTMP device. Update the wdc plugin version to 2.14.0 Remove vs-drive-info support for SNTMP Signed-off-by: jeff-lien-wdc <jeff.lien@sandisk.com>

-resolving merge conflicts from master Signed-off-by: Sivaprasad Gutha <sivaprasad6541@gmail.com>

igaw · 2025-06-16T13:33:58Z

I'd expect no merges from master into to your PR, see https://github.com/linux-nvme/nvme-cli?tab=readme-ov-file#how-to-cleanup-your-series-before-creating-pr

sivaprasad6541 · 2025-06-17T09:19:17Z

I'd expect no merges from master into to your PR, see https://github.com/linux-nvme/nvme-cli?tab=readme-ov-file#how-to-cleanup-your-series-before-creating-pr

Hi @igaw,
While attempting to squash all commits into a single one, some unintended changes were introduced, resulting in conflicts. To ensure a clean and accurate history, I’m closing this pull request and will open a new one with all the updated and verified changes.

-Fixed segmentation fault issue in JSON output format for VS Eye data -Added support to hex dump VS Eye data in both normal and JSON formats -This enables customers to decode the raw eye chart data if needed -Ensured compatibility with existing d() and obj_d() hex dump utilities -Addressed all the review comments that are given as part of pull request linux-nvme#2828 Signed-off-by: Sivaprasad Gutha <sivaprasad6541@gmail.com>

-Fixed segmentation fault issue in JSON output format for VS Eye data -Added support to hex dump VS Eye data in both normal and JSON formats -This enables customers to decode the raw eye chart data if needed -Ensured compatibility with existing d() and obj_d() hex dump utilities -Addressed all the review comments that are given as part of PR linux-nvme#2828 Signed-off-by: Sivaprasad Gutha <sivaprasad6541@gmail.com>

-Fixed segmentation fault issue in JSON output format for VS Eye data -Added support to hex dump VS Eye data in both normal and JSON formats -This enables customers to decode the raw eye chart data if needed -Ensured compatibility with existing d() and obj_d() hex dump utilities -Addressed all the review comments that are given as part of PR #2828 Signed-off-by: Sivaprasad Gutha <sivaprasad6541@gmail.com>

sivaprasad6541 added 2 commits May 27, 2025 11:45

plugins/micron: update authors list

d69c312

- Added contributors list to the AUTHORS section of the Micron plugin. Signed-off-by: Sivaprasad Gutha <sivaprasad6541@gmail.com>

igaw requested changes May 27, 2025

View reviewed changes

brandon-paupore-sndk reviewed May 27, 2025

View reviewed changes

igaw closed this May 28, 2025

igaw reopened this May 28, 2025

nvme: plot eye chart and hex dumping VS eye data

a404eb3

-Addressing review comments Signed-off-by: Sivaprasad Gutha <sivaprasad6541@gmail.com>

brandon-paupore-sndk reviewed May 29, 2025

View reviewed changes

nvme: eye chart plotting and dump VS eye data

1b18547

-Addressing review comments -Converting all the unsigned shorts into host-endianness Signed-off-by: Sivaprasad Gutha <sivaprasad6541@gmail.com>

sivaprasad6541 requested a review from igaw May 30, 2025 09:12

igaw requested changes Jun 2, 2025

View reviewed changes

nvme: eye chart plotting and dump VS eye data

43097ad

- Addressing review comments Signed-off-by: Sivaprasad Gutha <sivaprasad6541@gmail.com>

sivaprasad6541 force-pushed the pull-and-parse-eye-data-19h branch from 0a5f016 to 1b88468 Compare June 13, 2025 11:26

Merge branch 'master' into pull-and-parse-eye-data-19h

6226e29

-resolving merge conflicts from master Signed-off-by: Sivaprasad Gutha <sivaprasad6541@gmail.com>

sivaprasad6541 closed this Jun 17, 2025

sivaprasad6541 deleted the pull-and-parse-eye-data-19h branch June 17, 2025 09:30

sivaprasad6541 mentioned this pull request Jun 17, 2025

nvme: plot eye chart data and hex dumping VS eye data #2845

Merged

		vsdata = (unsigned char *)malloc(desc->edlen);
		vsdataoffset = (desc->nrows * desc->ncols) +

Conversation

sivaprasad6541 commented May 27, 2025

Uh oh!

sivaprasad6541 commented May 27, 2025

Uh oh!

igaw 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!

sivaprasad6541 May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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 27, 2025

Uh oh!

brandon-paupore-sndk 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!

igaw commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brandon-paupore-sndk commented May 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sivaprasad6541 May 29, 2025 •

edited

Loading

igaw commented May 28, 2025 •

edited

Loading