Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
sivaprasad6541 wants to merge 7 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f45b402
nvme: plot eye chart data and hex dumping VS eye data
sivaprasad6541 May 27, 2025
d69c312
plugins/micron: update authors list
sivaprasad6541 May 27, 2025
a404eb3
nvme: plot eye chart and hex dumping VS eye data
sivaprasad6541 May 29, 2025
1b18547
nvme: eye chart plotting and dump VS eye data
sivaprasad6541 May 29, 2025
43097ad
nvme: eye chart plotting and dump VS eye data
sivaprasad6541 Jun 4, 2025
1b88468
wdc: Fix for vs-smart-add-log wdc plugin command
jeff-lien-sndk May 27, 2025
6226e29
Merge branch 'master' into pull-and-parse-eye-data-19h
sivaprasad6541 Jun 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 62 additions & 11 deletions nvme-print-json.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2124,22 +2124,52 @@ static char *json_eom_printable_eye(struct nvme_eom_lane_desc *lane,
struct json_object *r)
{
char *eye = (char *)lane->eye_desc;
char *printable = malloc(lane->nrows * lane->ncols + lane->ncols);
uint16_t nrows = le16_to_cpu(lane->nrows);
uint16_t ncols = le16_to_cpu(lane->ncols);

if (nrows == 0 || ncols == 0)
return NULL;

// Allocate buffer for full printable string (with newlines)
Copy link
Collaborator

@igaw igaw Jun 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Contributor Author

@sivaprasad6541 sivaprasad6541 Jun 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modified as suggested.

char *printable = malloc(nrows * ncols + nrows + 1); // +1 for null terminator
Copy link
Collaborator

@igaw igaw Jun 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@sivaprasad6541 sivaprasad6541 Jun 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modified as suggested.

char *printable_start = printable;
int i, j;

if (!printable)
goto exit;
return NULL;

struct json_object *eye_array = json_create_array();
Copy link
Collaborator

@igaw igaw May 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@sivaprasad6541 sivaprasad6541 May 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@igaw igaw Jun 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again declaration go to the beginning of the block

Copy link
Contributor Author

@sivaprasad6541 sivaprasad6541 Jun 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved this to beginning of the block.


if (!eye_array) {
free(printable);
return NULL;
}

for (int i = 0; i < nrows; i++) {
char *row = malloc(ncols + 1);

if (!row) {
// Cleanup on failure
free(printable_start);
return NULL;
}

for (int j = 0; j < ncols; j++) {
char ch = eye[i * ncols + j];
*printable++ = ch;
row[j] = ch;
}

for (i = 0; i < lane->nrows; i++) {
for (j = 0; j < lane->ncols; j++, printable++)
sprintf(printable, "%c", eye[i * lane->ncols + j]);
sprintf(printable++, "\n");
*printable++ = '\n';
row[ncols] = '\0';

array_add_str(eye_array, row);
free(row);
}

obj_add_str(r, "printable_eye", printable_start);
*printable = '\0';

obj_add_array(r, "printable_eye", eye_array);

exit:
return printable_start;
}

Expand All @@ -2155,6 +2185,8 @@ static void json_phy_rx_eom_descs(struct nvme_phy_rx_eom_log *log,

for (i = 0; i < num_descs; i++) {
struct nvme_eom_lane_desc *desc = p;
unsigned char *vsdata = NULL;
unsigned int vsdataoffset = 0;
struct json_object *jdesc = json_create_object();

obj_add_uint(jdesc, "lid", desc->mstatus);
Expand All @@ -2169,9 +2201,28 @@ static void json_phy_rx_eom_descs(struct nvme_phy_rx_eom_log *log,
obj_add_uint(jdesc, "edlen", le16_to_cpu(desc->edlen));

if (NVME_EOM_ODP_PEFP(log->odp))
allocated_eyes[i] = json_eom_printable_eye(desc, r);
allocated_eyes[i] = json_eom_printable_eye(desc, jdesc);

if (desc->edlen == 0)
continue;

/* 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);
Copy link
Collaborator

@igaw igaw Jun 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@sivaprasad6541 sivaprasad6541 Jun 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modified as suggested.

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

@igaw igaw Jun 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@sivaprasad6541 sivaprasad6541 Jun 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modified as suggested.


if (!hexstr)
return;

char *hexdata = hexstr;

for (int offset = 0; offset < le16_to_cpu(desc->edlen); offset++)
hexdata += sprintf(hexdata, "%02X ", vsdata[offset]);
*(hexdata - 1) = '\0'; // remove trailing space

/* Eye Data field is vendor specific, doesn't map to JSON */
obj_add_str(jdesc, "vsdata_hex", hexstr);

array_add_obj(descs, jdesc);

Expand Down
13 changes: 13 additions & 0 deletions nvme-print-stdout.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -790,6 +790,8 @@ static void stdout_phy_rx_eom_descs(struct nvme_phy_rx_eom_log *log)

for (i = 0; i < log->nd; i++) {
struct nvme_eom_lane_desc *desc = p;
unsigned char *vsdata = NULL;
unsigned int vsdataoffset = 0;

printf("Measurement Status: %s\n",
desc->mstatus ? "Successful" : "Not Successful");
Expand All @@ -807,6 +809,17 @@ static void stdout_phy_rx_eom_descs(struct nvme_phy_rx_eom_log *log)
stdout_eom_printable_eye(desc);

/* Eye Data field is vendor specific */
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);
Copy link
Contributor

@brandon-paupore-sndk brandon-paupore-sndk May 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@sivaprasad6541 sivaprasad6541 May 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

printf("\n");

p += log->dsize;
}
Expand Down
4 changes: 3 additions & 1 deletion plugins/micron/micron-nvme.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@
*
* @file: micron-nvme.c
* @brief: This module contains all the constructs needed for micron nvme-cli plugin.
* @authors:Chaithanya Shoba <ashoba@micron.com>,
* @authors:Hanumanthu H <hanumanthuh@micron.com>
* Chaithanya Shoba <ashoba@micron.com>
* Sivaprasad Gutha <sivaprasadg@micron.com>
*/

#include <stdio.h>
Expand Down
4 changes: 3 additions & 1 deletion plugins/micron/micron-nvme.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@
*
* @file: micron-nvme.h
* @brief: This module contains all the constructs needed for micron nvme-cli plugin.
* @authors:Chaithanya Shoba <ashoba@micron.com>,
* @authors:Hanumanthu H <hanumanthuh@micron.com>
* Chaithanya Shoba <ashoba@micron.com>
* Sivaprasad Gutha <sivaprasadg@micron.com>
*/

#undef CMD_INC_FILE
Expand Down