Skip to content

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

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

Merged
merged 1 commit into from
Jul 16, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
104 changes: 88 additions & 16 deletions nvme-print-json.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2124,23 +2124,60 @@ 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);
char *printable_start = printable;
int i, j;
uint16_t nrows = le16_to_cpu(lane->nrows);
uint16_t ncols = le16_to_cpu(lane->ncols);
struct json_object *eye_array = NULL;
char *printable_start = NULL;
char *printable = NULL;

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

eye_array = json_create_array();
if (!eye_array)
return NULL;

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

if (!printable)
goto exit;
goto fail_free_eye_array;

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");
for (int i = 0; i < nrows; i++) {
char *row = malloc(ncols + 1);

if (!row)
goto fail_free_eye_printable;

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

*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:
Copy link
Collaborator

@igaw igaw Jun 30, 2025

Choose a reason for hiding this comment

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

I suggest to use goto error handlilng pattern:

@@ -2126,13 +2126,17 @@ static char *json_eom_printable_eye(struct nvme_eom_lane_desc *lane,
        char *eye = (char *)lane->eye_desc;
        uint16_t nrows = le16_to_cpu(lane->nrows);
        uint16_t ncols = le16_to_cpu(lane->ncols);
-       char *printable = NULL;
+       struct json_object *eye_array;
        char *printable_start = NULL;
-       struct json_object *eye_array = json_create_array();
+       char *printable = NULL;

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

+       eye_array = json_create_array();
+       if (!eye_array)
+               return NULL;
+
        /*
         * Allocate buffer for full printable string (with newlines)
         * +1 for null terminator
@@ -2141,21 +2145,13 @@ static char *json_eom_printable_eye(struct nvme_eom_lane_desc *lane,
        printable_start = printable;

        if (!printable)
-               return NULL;
-
-       if (!eye_array) {
-               free(printable);
-               return NULL;
-       }
+               goto fail_free_eye_array;

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

-               if (!row) {
-                       /* Cleanup on failure */
-                       free(printable_start);
-                       return NULL;
-               }
+               if (!row)
+                       goto fail_free_printable;

                for (int j = 0; j < ncols; j++) {
                        char ch = eye[i * ncols + j];
@@ -2175,6 +2171,13 @@ static char *json_eom_printable_eye(struct nvme_eom_lane_desc *lane,
        obj_add_array(r, "printable_eye", eye_array);

        return printable_start;
+
+fail_free_printable:
+       free(printable);
+fail_free_eye_array:
+       json_free_object(eye_array);
+
+       return NULL;
 }

Copy link
Contributor Author

@sivaprasad6541 sivaprasad6541 Jul 15, 2025

Choose a reason for hiding this comment

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

Apologies for the delay, and thank you so much for your feedback and suggestions. I've incorporated the suggested changes. Please take a moment to review and let me know if everything looks good or if any further adjustments are needed.

return printable_start;

fail_free_eye_printable:
free(printable);
fail_free_eye_array:
json_free_object(eye_array);

return NULL;
}

static void json_phy_rx_eom_descs(struct nvme_phy_rx_eom_log *log,
Expand All @@ -2155,7 +2192,20 @@ 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;
struct json_object *jdesc = json_create_object();
_cleanup_free_ char *hexstr = NULL;
unsigned char *vsdata = NULL;
unsigned int vsdataoffset = 0;
uint16_t nrows, ncols, edlen;
struct json_object *jdesc;
char *hexdata;

jdesc = json_create_object();
if (!desc)
return;

nrows = le16_to_cpu(desc->nrows);
ncols = le16_to_cpu(desc->ncols);
edlen = le16_to_cpu(desc->edlen);

obj_add_uint(jdesc, "lid", desc->mstatus);
obj_add_uint(jdesc, "lane", desc->lane);
Expand All @@ -2164,14 +2214,36 @@ static void json_phy_rx_eom_descs(struct nvme_phy_rx_eom_log *log,
obj_add_uint(jdesc, "bottom", le16_to_cpu(desc->bottom));
obj_add_uint(jdesc, "left", le16_to_cpu(desc->left));
obj_add_uint(jdesc, "right", le16_to_cpu(desc->right));
obj_add_uint(jdesc, "nrows", le16_to_cpu(desc->nrows));
obj_add_uint(jdesc, "ncols", le16_to_cpu(desc->ncols));
obj_add_uint(jdesc, "edlen", le16_to_cpu(desc->edlen));
obj_add_uint(jdesc, "nrows", nrows);
obj_add_uint(jdesc, "ncols", ncols);
obj_add_uint(jdesc, "edlen", 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 (edlen == 0)
continue;
Copy link
Collaborator

@igaw igaw Jun 30, 2025

Choose a reason for hiding this comment

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

isn't this leaking jdesc?

Copy link
Contributor Author

@sivaprasad6541 sivaprasad6541 Jul 15, 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 */
hexstr = malloc(edlen * 3 + 1);

if (!hexstr) {
json_free_object(jdesc);
return;
Copy link
Collaborator

@igaw igaw Jun 30, 2025

Choose a reason for hiding this comment

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

and this leaks jdec for sure.

Copy link
Contributor Author

@sivaprasad6541 sivaprasad6541 Jul 15, 2025

Choose a reason for hiding this comment

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

Added code to free the jdesc.

}

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

hexdata = hexstr;

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

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

@igaw igaw Jun 30, 2025

Choose a reason for hiding this comment

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

so maybe something like

        for (i = 0; i < num_descs; i++) {
                struct nvme_eom_lane_desc *desc = p;
+               _cleanup_free_ char *hexstr = NULL;
                unsigned char *vsdata = NULL;
                unsigned int vsdataoffset = 0;
-               struct json_object *jdesc = json_create_object();
-               uint16_t nrows = le16_to_cpu(desc->nrows);
-               uint16_t ncols = le16_to_cpu(desc->ncols);
-               uint16_t edlen = le16_to_cpu(desc->edlen);
+               uint16_t nrows, ncols, edlen;
+               struct json_object *jdesc;
+               char *hexdata;
+
+               jdesc = json_create_object();
+               if (!jdesc)
+                       return;
+
+               nrows = le16_to_cpu(desc->nrows);
+               ncols = le16_to_cpu(desc->ncols);
+               edlen = le16_to_cpu(desc->edlen);

                obj_add_uint(jdesc, "lid", desc->mstatus);
                obj_add_uint(jdesc, "lane", desc->lane);
@@ -2214,16 +2225,17 @@ static void json_phy_rx_eom_descs(struct nvme_phy_rx_eom_log *log,
                        continue;

                /* 2 hex chars + space per byte */
-               _cleanup_free_ char *hexstr = malloc(edlen * 3 + 1);
-
-               if (!hexstr)
+               hexstr = malloc(edlen * 3 + 1);
+               if (!hexstr) {
+                       json_free_object(jdesc);
                        return;
+               }

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

-               char *hexdata = hexstr;
+               hexdata = hexstr;

                for (int offset = 0; offset < edlen; offset++)

Copy link
Contributor Author

@sivaprasad6541 sivaprasad6541 Jul 15, 2025

Choose a reason for hiding this comment

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

Thanks for the suggestions. I've modified the code as per your suggestion. Please review the code.


array_add_obj(descs, jdesc);

Expand Down
33 changes: 27 additions & 6 deletions nvme-print-stdout.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -776,9 +776,9 @@ static void stdout_eom_printable_eye(struct nvme_eom_lane_desc *lane)
char *eye = (char *)lane->eye_desc;
int i, j;

for (i = 0; i < lane->nrows; i++) {
for (j = 0; j < lane->ncols; j++)
printf("%c", eye[i * lane->ncols + j]);
for (i = 0; i < le16_to_cpu(lane->nrows); i++) {
for (j = 0; j < le16_to_cpu(lane->ncols); j++)
printf("%c", eye[i * le16_to_cpu(lane->ncols) + j]);
printf("\n");
}
}
Expand All @@ -790,6 +790,13 @@ 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;
uint16_t nrows, ncols, edlen;

nrows = le16_to_cpu(desc->nrows);
ncols = le16_to_cpu(desc->ncols);
edlen = le16_to_cpu(desc->edlen);

printf("Measurement Status: %s\n",
desc->mstatus ? "Successful" : "Not Successful");
Expand All @@ -799,14 +806,28 @@ static void stdout_phy_rx_eom_descs(struct nvme_phy_rx_eom_log *log)
printf("Bottom: %u\n", le16_to_cpu(desc->bottom));
printf("Left: %u\n", le16_to_cpu(desc->left));
printf("Right: %u\n", le16_to_cpu(desc->right));
printf("Number of Rows: %u\n", le16_to_cpu(desc->nrows));
printf("Number of Columns: %u\n", le16_to_cpu(desc->ncols));
printf("Eye Data Length: %u\n", le16_to_cpu(desc->edlen));
printf("Number of Rows: %u\n", nrows);
printf("Number of Columns: %u\n", ncols);
printf("Eye Data Length: %u\n", desc->edlen);

if (NVME_EOM_ODP_PEFP(log->odp))
stdout_eom_printable_eye(desc);

/* Eye Data field is vendor specific */
if (edlen == 0)
continue;

/* Hex dump Vendor Specific Eye Data */
vsdata = malloc(edlen);
Copy link
Collaborator

@igaw igaw Jun 30, 2025

Choose a reason for hiding this comment

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

return value is not checked, could be NULL.

Copy link
Contributor Author

@sivaprasad6541 sivaprasad6541 Jul 15, 2025

Choose a reason for hiding this comment

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

Added NULL check as suggested.

if (!vsdata)
return;

vsdataoffset = (nrows * ncols) + sizeof(struct nvme_eom_lane_desc);
vsdata = (unsigned char *)((unsigned char *)desc + vsdataoffset);
Copy link
Collaborator

@igaw igaw Jun 30, 2025

Choose a reason for hiding this comment

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

and here we leak vsdata.

BTW, valgrind is a very good tool find such leaks, give it a try.

Copy link
Contributor Author

@sivaprasad6541 sivaprasad6541 Jul 15, 2025

Choose a reason for hiding this comment

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

Thanks you for the feedback @igaw.
I have ran valgrind after modifying suggested changes and no leeks observed.

printf("Eye Data:\n");
d(vsdata, edlen, 16, 1);
printf("\n");
free(vsdata);

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 <[email protected]>,
* @authors:Hanumanthu H <[email protected]>
* Chaithanya Shoba <[email protected]>
* Sivaprasad Gutha <[email protected]>
*/

#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 <[email protected]>,
* @authors:Hanumanthu H <[email protected]>
* Chaithanya Shoba <[email protected]>
* Sivaprasad Gutha <[email protected]>
*/

#undef CMD_INC_FILE
Expand Down