[13/15] pciutils-pcilmr: Add option to save margining results in csv form

Commit Message

Nikita Proshkin Dec. 8, 2023, 9:17 a.m. UTC

Reviewed-by: Sergei Miroshnichenko <[e-mail]>
Signed-off-by: Nikita Proshkin <[e-mail]>
---
 lmr_lib/margin_results.c | 108 +++++++++++++++++++++++++++++++++++++++
 lmr_lib/margin_results.h |   2 +
 pcilmr.c                 |  18 ++++++-
 3 files changed, 126 insertions(+), 2 deletions(-)

Comments

Martin Mareš Dec. 8, 2023, 5:44 p.m. UTC | #1

> +  char timestamp[64];
> +  time_t tim = time(NULL);
> +  strftime(timestamp, 64, "%FT%H.%M.%S", gmtime(&tim));

Please use sizeof(timestamp) to make the code more robust.

> +  char *path = (char *)malloc((strlen(dir) + 128) * sizeof(*path));

Please use xmalloc().

> +    sprintf(path, "%s/lmr_%0*x.%02x.%02x.%x_Rx%X_%s.csv", dir,

Please avoid plain sprintf() everywhere and use snprintf() instead.

> +    if (!csv)
> +    {
> +      printf("Error while saving %s\n", path);
> +      free(path);
> +      return;
> +    }

We have die(...) for that.

> +    for (uint8_t j = 0; j < recv->lanes_n; j++)

It's better to use plain integer types ("int" or "unsigned int") for
loop control variables.

				Have a nice fortnight

Nikita Proshkin Dec. 13, 2023, 10:52 a.m. UTC | #2

On Fri, 8 Dec 2023 18:44:23 +0100
Martin Mareš <[e-mail]> wrote:

> > +    if (!csv)
> > +    {
> > +      printf("Error while saving %s\n", path);
> > +      free(path);
> > +      return;
> > +    }
> 
> We have die(...) for that.

I think that die() from common.c is not suitable here, as the utility at that
moment in the code still can and must free up resources malloc'ed in the main
function. 

> > +  char timestamp[64];
> > +  time_t tim = time(NULL);
> > +  strftime(timestamp, 64, "%FT%H.%M.%S", gmtime(&tim));
> 
> Please use sizeof(timestamp) to make the code more robust.
> 
> > +  char *path = (char *)malloc((strlen(dir) + 128) * sizeof(*path));
> 
> Please use xmalloc().
> 
> > +    sprintf(path, "%s/lmr_%0*x.%02x.%02x.%x_Rx%X_%s.csv", dir,
> 
> Please avoid plain sprintf() everywhere and use snprintf() instead.
> 
> > +    for (uint8_t j = 0; j < recv->lanes_n; j++)
> 
> It's better to use plain integer types ("int" or "unsigned int") for
> loop control variables.

Everything else will be fixed. 

Best regards,
Nikita Proshkin

Martin Mareš Dec. 13, 2023, 11:22 a.m. UTC | #3

Hi1

> I think that die() from common.c is not suitable here, as the utility at that
> moment in the code still can and must free up resources malloc'ed in the main
> function.

I don't understand why this is needed -- once the process exits, all memory
allocated by it is freed by the kernel.

					Martin

Nikita Proshkin Dec. 13, 2023, 1:01 p.m. UTC | #4

On Wed, 13 Dec 2023 12:22:45 +0100
Martin Mareš <[e-mail]> wrote:

> > I think that die() from common.c is not suitable here, as the utility at that
> > moment in the code still can and must free up resources malloc'ed in the main
> > function.
> 
> I don't understand why this is needed -- once the process exits, all memory
> allocated by it is freed by the kernel.

Well, yes, I suppose in case of most modern operating systems you are right.

This particular function, about which this thread is about, was conceived as
an optional method to save the results of the utility to a file. According to
the logic of the utility, by this point the testing process has already been
successfully completed and it seemed to me that the utility should no longer
panic.

On the other hand, it makes sense to return a non-zero code to the user if
something went wrong. In fact, there are a few more cases in the main function
of the utility when some checks fail and the utility may exit. If we are
satisfied with the option when the utility does not clean up gracefully,
I can use die() in such cases.

Best regards,
Nikita Proshkin

Patch

diff --git a/lmr_lib/margin_results.c b/lmr_lib/margin_results.c
index 61f483a..cc6132f 100644
--- a/lmr_lib/margin_results.c
+++ b/lmr_lib/margin_results.c
@@ -1,5 +1,7 @@ 
 #include <stdlib.h>
 #include <stdio.h>
+#include <time.h>
+#include <string.h>
 
 #include "margin_results.h"
 
@@ -133,3 +135,109 @@  void margin_results_print_brief(struct margin_results *results, uint8_t recvs_n)
     printf("\n");
   }
 }
+
+void margin_results_save_csv(struct margin_results *results, uint8_t recvs_n, char *dir, struct pci_dev *up_dev)
+{
+  char timestamp[64];
+  time_t tim = time(NULL);
+  strftime(timestamp, 64, "%FT%H.%M.%S", gmtime(&tim));
+
+  char *path = (char *)malloc((strlen(dir) + 128) * sizeof(*path));
+  FILE *csv;
+
+  struct margin_res_lane *lane;
+  struct margin_results *recv;
+  enum lane_rating lane_rating;
+  uint8_t link_speed;
+
+  for (uint8_t i = 0; i < recvs_n; i++)
+  {
+    recv = &(results[i]);
+    link_speed = recv->link_speed - 4;
+
+    if (recv->test_status != MARGIN_TEST_OK)
+      continue;
+    sprintf(path, "%s/lmr_%0*x.%02x.%02x.%x_Rx%X_%s.csv", dir,
+            up_dev->domain_16 == 0xffff ? 8 : 4,
+            up_dev->domain, up_dev->bus, up_dev->dev,
+            up_dev->func, 10 + recv->recvn - 1, timestamp);
+    csv = fopen(path, "w");
+    if (!csv)
+    {
+      printf("Error while saving %s\n", path);
+      free(path);
+      return;
+    }
+
+    fprintf(csv, "Lane,Lane Status,Left %% UI,Left ps,Left Steps,Left Status,"
+                 "Right %% UI,Right ps,Right Steps,Right Status,"
+                 "Time %% UI,Time ps,Time Steps,Time Status,"
+                 "Up mV,Up Steps,Up Status,Down mV,Down Steps,Down Status,"
+                 "Voltage mV,Voltage Steps,Voltage Status\n");
+
+    if (check_recv_weird(recv, MARGIN_TIM_MIN, MARGIN_VOLT_MIN))
+      lane_rating = WEIRD;
+    else
+      lane_rating = INIT;
+
+    for (uint8_t j = 0; j < recv->lanes_n; j++)
+    {
+      lane = &(recv->lanes[j]);
+      double left_ui = lane->steps[TIM_LEFT] * recv->tim_coef;
+      double right_ui = lane->steps[TIM_RIGHT] * recv->tim_coef;
+      double up_volt = lane->steps[VOLT_UP] * recv->volt_coef;
+      double down_volt = lane->steps[VOLT_DOWN] * recv->volt_coef;
+
+      if (lane_rating != WEIRD)
+      {
+        lane_rating = rate_lane(left_ui, MARGIN_TIM_MIN, MARGIN_TIM_RECOMMEND, INIT);
+        if (recv->ind_left_right_tim)
+          lane_rating = rate_lane(right_ui, MARGIN_TIM_MIN, MARGIN_TIM_RECOMMEND, lane_rating);
+        if (recv->volt_support)
+        {
+          lane_rating = rate_lane(up_volt, MARGIN_VOLT_MIN, MARGIN_VOLT_MIN, lane_rating);
+          if (recv->ind_up_down_volt)
+            lane_rating = rate_lane(down_volt, MARGIN_VOLT_MIN, MARGIN_VOLT_MIN, lane_rating);
+        }
+      }
+
+      fprintf(csv, "%d,%s,", lane->lane, grades[lane_rating]);
+      if (recv->ind_left_right_tim)
+      {
+        fprintf(csv, "%f,%f,%d,%s,%f,%f,%d,%s,NA,NA,NA,NA,",
+                left_ui, left_ui * ui[link_speed], lane->steps[TIM_LEFT], sts_strings[lane->statuses[TIM_LEFT]],
+                right_ui, right_ui * ui[link_speed], lane->steps[TIM_RIGHT], sts_strings[lane->statuses[TIM_RIGHT]]);
+      }
+      else
+      {
+        for (uint8_t k = 0; k < 8; k++)
+          fprintf(csv, "NA,");
+        fprintf(csv, "%f,%f,%d,%s,", left_ui, left_ui * ui[link_speed],
+                lane->steps[TIM_LEFT], sts_strings[lane->statuses[TIM_LEFT]]);
+      }
+      if (recv->volt_support)
+      {
+        if (recv->ind_up_down_volt)
+        {
+          fprintf(csv, "%f,%d,%s,%f,%d,%s,NA,NA,NA\n",
+                  up_volt, lane->steps[VOLT_UP], sts_strings[lane->statuses[VOLT_UP]],
+                  down_volt, lane->steps[VOLT_DOWN], sts_strings[lane->statuses[VOLT_DOWN]]);
+        }
+        else
+        {
+          for (uint8_t k = 0; k < 6; k++)
+            fprintf(csv, "NA,");
+          fprintf(csv, "%f,%d,%s\n", up_volt, lane->steps[VOLT_UP], sts_strings[lane->statuses[VOLT_UP]]);
+        }
+      }
+      else
+      {
+        for (uint8_t k = 0; k < 8; k++)
+          fprintf(csv, "NA,");
+        fprintf(csv, "NA\n");
+      }
+    }
+    fclose(csv);
+  }
+  free(path);
+}
diff --git a/lmr_lib/margin_results.h b/lmr_lib/margin_results.h
index ab6ddb0..69847cb 100644
--- a/lmr_lib/margin_results.h
+++ b/lmr_lib/margin_results.h
@@ -10,4 +10,6 @@ 
 
 void margin_results_print_brief(struct margin_results *results, uint8_t recvs_n);
 
+void margin_results_save_csv(struct margin_results *results, uint8_t recvs_n, char *dir, struct pci_dev *up_dev);
+
 #endif
diff --git a/pcilmr.c b/pcilmr.c
index 46b9f24..3da28e5 100644
--- a/pcilmr.c
+++ b/pcilmr.c
@@ -47,7 +47,12 @@  static void usage(void)
          "-v <steps>\t\tSpecify maximum number of steps for Voltage Margining.\n"
          "Use only one of -T/-t options at the same time (same for -V/-v).\n"
          "Without these options utility will use MaxSteps from Device\n"
-         "capabilities as test limit.\n\n");
+         "capabilities as test limit.\n\n"
+         "Margining Log settings:\n"
+         "-o <directory>\t\tSave margining results in csv form into the\n"
+         "\t\t\tspecified directory. Utility will generate file with the\n"
+         "\t\t\tname in form of 'lmr_<downstream component>_Rx#_<timestamp>.csv'\n"
+         "\t\t\tfor each successfully tested receiver.\n");
 }
 
 static struct pci_dev *dev_for_filter(struct pci_access *pacc, char *filter)
@@ -198,6 +203,9 @@  int main(int argc, char **argv)
 
   bool run_margin = true;
 
+  char *dir_for_csv = NULL;
+  bool save_csv = false;
+
   uint64_t total_steps = 0;
 
   pacc = pci_alloc();
@@ -247,7 +255,7 @@  int main(int argc, char **argv)
     break;
   }
 
-  while (status && ((c = getopt(argc, argv, ":r:e:l:cp:t:v:VT")) != -1))
+  while (status && ((c = getopt(argc, argv, ":r:e:l:cp:t:v:VTo:")) != -1))
   {
     switch (c)
     {
@@ -278,6 +286,10 @@  int main(int argc, char **argv)
     case 'r':
       recvs_n = parse_csv_arg(optarg, recvs_arg);
       break;
+    case 'o':
+      dir_for_csv = optarg;
+      save_csv = true;
+      break;
     default:
       printf("Invalid arguments\n");
       status = false;
@@ -494,6 +506,8 @@  int main(int argc, char **argv)
       margin_log_bdfs(down_ports[i], up_ports[i]);
       printf(":\n\n");
       margin_results_print_brief(results[i], results_n[i]);
+      if (save_csv)
+        margin_results_save_csv(results[i], results_n[i], dir_for_csv, up_ports[i]);
       printf("\n");
     }
   }